From f1473954d932ffc76e92e020fe0628e61d0350f1 Mon Sep 17 00:00:00 2001 From: samiamorwas Date: Thu, 20 Aug 2026 03:41:25 -0400 Subject: [PATCH 1/2] refactor(core): share one isolated Git invocation helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three places spawned git with the operator's configuration held off, each with its own copy of the setup: the source resolver, the task-repository lifecycle, and — next — diff-scope measurement. Move `IsolatedGit` to `core`, where `run_git` already lives, and give `run` an `env` parameter so the baseline commit's committer identity rides on the shared helper instead of a parallel one. `BASELINE_REF` moves with it. It was private to the runner, but it is the contract between whoever writes the ref and whoever measures against it, so it needs one spelling. Co-Authored-By: Claude Opus 5 --- src/cli/run/orchestrate/git.rs | 202 ++++++++++----------------------- src/{source => core}/git.rs | 41 +++++-- src/core/mod.rs | 6 +- src/source/mod.rs | 10 +- 4 files changed, 97 insertions(+), 162 deletions(-) rename src/{source => core}/git.rs (59%) diff --git a/src/cli/run/orchestrate/git.rs b/src/cli/run/orchestrate/git.rs index ced515a..a3d98b3 100644 --- a/src/cli/run/orchestrate/git.rs +++ b/src/cli/run/orchestrate/git.rs @@ -1,12 +1,10 @@ //! Runner-owned Git lifecycle for private task environments. -use std::ffi::{OsStr, OsString}; use std::fs; use std::path::{Path, PathBuf}; -use std::process::{Command, Output}; use crate::adapters::registry::all_config_dir_names; -use crate::core::{clear_git_environment, run_git}; +use crate::core::{BASELINE_REF, GitOutput, IsolatedGit, run_git}; use crate::source::INITIALIZED_BRANCH; use super::super::RunError; @@ -15,8 +13,6 @@ use super::Resolved; use super::envs::{EnvLayoutInput, env_targets}; use crate::core::RunContext; -/// Marks the state every environment starts from, for later diffing. -const BASELINE_REF: &str = "refs/eval-magic/baseline"; const BASELINE_MESSAGE: &str = "eval-magic task baseline"; const BASELINE_NAME: &str = "eval-magic"; const BASELINE_EMAIL: &str = "eval-magic@localhost"; @@ -145,34 +141,27 @@ fn path_budget_hint(root: &Path, windows: bool) -> Option { fn initialize_task_repository(plan: &TaskRepository) -> Result<(), String> { let root = plan.root.as_path(); - - let isolated = tempfile::TempDir::new() - .map_err(|error| format!("could not create isolated Git configuration: {error}"))?; - let template_dir = isolated.path().join("template"); - let global_config = isolated.path().join("global-config"); - fs::create_dir(&template_dir) - .map_err(|error| format!("could not create empty Git template directory: {error}"))?; - fs::write(&global_config, "") - .map_err(|error| format!("could not create empty Git configuration: {error}"))?; + let git = IsolatedGit::new()?; if plan.sourced { // The clone's history is the point of sourcing a codebase, so this is // the one case that must not reset `.git`. - strip_remotes(root, &global_config)?; + strip_remotes(root, &git)?; } else { remove_existing_git_dir(root)?; + let template = git.template_dir().to_string_lossy().into_owned(); run_checked( + &git, root, &[ - OsString::from("init"), - OsString::from("--quiet"), - OsString::from("--initial-branch"), - OsString::from(&plan.branch), - OsString::from("--template"), - template_dir.into_os_string(), - OsString::from("."), + "init", + "--quiet", + "--initial-branch", + &plan.branch, + "--template", + &template, + ".", ], - &global_config, &[], )?; } @@ -185,30 +174,21 @@ fn initialize_task_repository(plan: &TaskRepository) -> Result<(), String> { fs::write(root.join(".git/info/exclude"), "/.eval-magic-outputs/\n") .map_err(|error| format!("could not configure framework output exclusion: {error}"))?; + let hooks_path = hooks_dir.to_string_lossy().into_owned(); for (name, value) in [ - ("user.name", OsString::from(BASELINE_NAME)), - ("user.email", OsString::from(BASELINE_EMAIL)), - ("commit.gpgSign", OsString::from("false")), - ("tag.gpgSign", OsString::from("false")), - ("core.hooksPath", hooks_dir.into_os_string()), + ("user.name", BASELINE_NAME), + ("user.email", BASELINE_EMAIL), + ("commit.gpgSign", "false"), + ("tag.gpgSign", "false"), + ("core.hooksPath", hooks_path.as_str()), // 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")), + ("core.longpaths", "true"), ] { - run_checked( - root, - &[ - OsString::from("config"), - OsString::from("--local"), - OsString::from(name), - value, - ], - &global_config, - &[], - )?; + run_checked(&git, root, &["config", "--local", name, value], &[])?; } // Respects the sourced codebase's `.gitignore`: a real repository ignores @@ -218,41 +198,27 @@ fn initialize_task_repository(plan: &TaskRepository) -> Result<(), String> { // No exclude pathspec for `.eval-magic-outputs`: `.git/info/exclude` above // already ignores it, and an unforced add honors that. The pathspecs this // replaces existed only to carve it back out of a forced add. - run_checked( - root, - &[ - OsString::from("add"), - OsString::from("--all"), - OsString::from("--"), - OsString::from("."), - ], - &global_config, - &[], - )?; + run_checked(&git, root, &["add", "--all", "--", "."], &[])?; // What the runner itself placed is forced in on top, so a codebase that // ignores `.claude/` cannot hide the staged skill from the baseline — which // would leave the condition under test outside every later diff. if !plan.forced_paths.is_empty() { - let mut args = vec![ - OsString::from("add"), - OsString::from("--force"), - OsString::from("--"), - ]; - args.extend(plan.forced_paths.iter().map(OsString::from)); - run_checked(root, &args, &global_config, &[])?; + let mut args = vec!["add", "--force", "--"]; + args.extend(plan.forced_paths.iter().map(String::as_str)); + run_checked(&git, root, &args, &[])?; } run_checked( + &git, root, &[ - OsString::from("commit"), - OsString::from("--quiet"), - OsString::from("--allow-empty"), - OsString::from("--no-gpg-sign"), - OsString::from("--no-verify"), - OsString::from("-m"), - OsString::from(BASELINE_MESSAGE), + "commit", + "--quiet", + "--allow-empty", + "--no-gpg-sign", + "--no-verify", + "-m", + BASELINE_MESSAGE, ], - &global_config, &[ ("GIT_AUTHOR_NAME", BASELINE_NAME), ("GIT_AUTHOR_EMAIL", BASELINE_EMAIL), @@ -269,39 +235,23 @@ fn initialize_task_repository(plan: &TaskRepository) -> Result<(), String> { // // Deliberately outside `refs/heads/`: it never appears in `git branch`, so // it adds nothing to what the agent under test sees. - run_checked( - root, - &[ - OsString::from("update-ref"), - OsString::from(BASELINE_REF), - OsString::from("HEAD"), - ], - &global_config, - &[], - )?; + run_checked(&git, root, &["update-ref", BASELINE_REF, "HEAD"], &[])?; - verify_task_repository(root, &global_config) + verify_task_repository(root, &git) } /// Drop every remote, so nothing in the environment can reach the source it was /// cloned from — or push to it. -fn strip_remotes(root: &Path, global_config: &Path) -> Result<(), String> { - let listed = run_checked(root, &[OsString::from("remote")], global_config, &[])?; - for remote in String::from_utf8_lossy(&listed.stdout) +fn strip_remotes(root: &Path, git: &IsolatedGit) -> Result<(), String> { + let listed = run_checked(git, root, &["remote"], &[])?; + let remotes: Vec = String::from_utf8_lossy(&listed.stdout) .lines() .map(str::trim) .filter(|name| !name.is_empty()) - { - run_checked( - root, - &[ - OsString::from("remote"), - OsString::from("remove"), - OsString::from(remote), - ], - global_config, - &[], - )?; + .map(str::to_string) + .collect(); + for remote in &remotes { + run_checked(git, root, &["remote", "remove", remote], &[])?; } Ok(()) } @@ -326,16 +276,8 @@ fn remove_existing_git_dir(root: &Path) -> Result<(), String> { }) } -fn verify_task_repository(root: &Path, global_config: &Path) -> Result<(), String> { - let top_level = run_checked( - root, - &[ - OsString::from("rev-parse"), - OsString::from("--show-toplevel"), - ], - global_config, - &[], - )?; +fn verify_task_repository(root: &Path, git: &IsolatedGit) -> Result<(), String> { + let top_level = run_checked(git, root, &["rev-parse", "--show-toplevel"], &[])?; let reported = PathBuf::from(String::from_utf8_lossy(&top_level.stdout).trim()); let expected = fs::canonicalize(root) .map_err(|error| format!("could not canonicalize task root: {error}"))?; @@ -354,13 +296,9 @@ fn verify_task_repository(root: &Path, global_config: &Path) -> Result<(), Strin } let status = run_checked( + git, root, - &[ - OsString::from("status"), - OsString::from("--porcelain=v1"), - OsString::from("--untracked-files=all"), - ], - global_config, + &["status", "--porcelain=v1", "--untracked-files=all"], &[], )?; if !status.stdout.is_empty() { @@ -370,7 +308,7 @@ fn verify_task_repository(root: &Path, global_config: &Path) -> Result<(), Strin )); } - let remotes = run_checked(root, &[OsString::from("remote")], global_config, &[])?; + let remotes = run_checked(git, root, &["remote"], &[])?; if !remotes.stdout.is_empty() { return Err(format!( "task repository unexpectedly has remotes: {}", @@ -381,46 +319,20 @@ fn verify_task_repository(root: &Path, global_config: &Path) -> Result<(), Strin } fn run_checked( + git: &IsolatedGit, cwd: &Path, - args: &[OsString], - global_config: &Path, + args: &[&str], env: &[(&str, &str)], -) -> 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") - .env("GIT_CONFIG_GLOBAL", global_config) - .env_remove("GIT_CONFIG_COUNT") - .env_remove("GIT_CONFIG_PARAMETERS"); - clear_git_environment(&mut command); - for (name, value) in env { - command.env(name, value); - } - let output = command.output().map_err(|error| { - format!( - "git {} could not start: {error}", - display_args(args.iter().map(OsString::as_os_str)) - ) - })?; - if output.status.success() { - return Ok(output); +) -> Result { + let output = git.run(cwd, args, env); + match output.status { + Some(0) => Ok(output), + status => Err(format!( + "git {} failed: {}", + args.join(" "), + git_diagnostic(status, &output.stderr) + )), } - Err(format!( - "git {} failed: {}", - display_args(args.iter().map(OsString::as_os_str)), - git_diagnostic(output.status.code(), &output.stderr) - )) -} - -fn display_args<'a>(args: impl Iterator) -> String { - args.map(|arg| arg.to_string_lossy()) - .collect::>() - .join(" ") } fn git_diagnostic(status: Option, stderr: &[u8]) -> String { diff --git a/src/source/git.rs b/src/core/git.rs similarity index 59% rename from src/source/git.rs rename to src/core/git.rs index 2fc7eab..014ba01 100644 --- a/src/source/git.rs +++ b/src/core/git.rs @@ -1,21 +1,30 @@ //! Running git with the operator's configuration held off. //! -//! Sourcing a codebase runs git against a URL from an eval config, on a host -//! whose git configuration belongs to someone else. Left inherited, that -//! configuration decides things the runner has to decide itself: `insteadOf` -//! rewrites the URL, so the tree sourced is not the tree the report cites; -//! `init.templateDir` installs hooks into a repository the guard assumes has -//! none; `commit.gpgSign` blocks the baseline commit on a passphrase prompt. +//! The runner spawns git on a host whose git configuration belongs to someone +//! else. Left inherited, that configuration decides things the runner has to +//! decide itself: `insteadOf` rewrites a URL, so the tree sourced is not the +//! tree the report cites; `init.templateDir` installs hooks into a repository +//! the guard assumes has none; `commit.gpgSign` blocks the baseline commit on a +//! passphrase prompt; `core.excludesFile` and `core.autocrlf` change which files +//! a diff reports and how many lines it counts. //! -//! So every git invocation in this module runs with system and global -//! configuration switched off and the environment-variable configuration -//! mechanism cleared. +//! So every caller that needs an answer git alone should decide runs through +//! [`IsolatedGit`]: system and global configuration switched off, and the +//! environment-variable configuration mechanism cleared. use std::path::{Path, PathBuf}; use std::process::Command; use crate::core::{GitOutput, clear_git_environment}; +/// Marks the state every task environment starts from. +/// +/// The runner writes it once, when it establishes the environment's +/// repository; every later measurement is the difference from it. Deliberately +/// outside `refs/heads/`: it never appears in `git branch`, so it adds nothing +/// to what the agent under test sees. +pub const BASELINE_REF: &str = "refs/eval-magic/baseline"; + /// A scratch git configuration that resolves to nothing. /// /// Holds the `TempDir` alive: dropping it removes the empty global config file @@ -49,7 +58,16 @@ impl IsolatedGit { &self.template_dir } - pub(crate) fn run(&self, cwd: &Path, args: &[&str]) -> GitOutput { + /// Invoke git in `cwd`. `env` sets variables for this invocation only — + /// the committer identity a deterministic baseline commit needs, or the + /// scratch index a measurement builds; configuration still comes from the + /// isolated files above. + /// + /// `env` is applied *after* the routing variables are cleared, deliberately: + /// `GIT_INDEX_FILE` is one of the variables cleared, so a caller pointing + /// git at an index of its own has to win over the inherited state rather + /// than be swept up with it. + pub(crate) fn run(&self, cwd: &Path, args: &[&str], env: &[(&str, &str)]) -> GitOutput { let mut command = Command::new("git"); command // `git clone` and `git init` create paths inside `.git` before any @@ -66,6 +84,9 @@ impl IsolatedGit { .env_remove("GIT_CONFIG_COUNT") .env_remove("GIT_CONFIG_PARAMETERS"); clear_git_environment(&mut command); + for (name, value) in env { + command.env(name, value); + } match command.output() { Ok(output) => GitOutput { status: output.status.code(), diff --git a/src/core/mod.rs b/src/core/mod.rs index e620085..aaf3830 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -3,7 +3,8 @@ //! - [`types`] — domain types (`Eval`, `RunRecord`, `Assertion`, `GradingResult`, …) //! - [`context`] — `RunContext` detection from parsed flags / environment //! - [`capabilities`] — per-harness run-option capabilities -//! - [`runtime`] — runtime helpers (git spawning) +//! - [`git`] — git spawned with the operator's configuration held off +//! - [`runtime`] — runtime helpers (plain git spawning, POSIX shell discovery) //! //! The submodules are re-exported flat here so downstream code writes //! `crate::core::Eval` rather than `crate::core::types::Eval`. @@ -11,11 +12,14 @@ pub mod capabilities; pub mod context; pub mod fs; +pub mod git; pub mod runtime; pub mod types; pub use capabilities::HarnessRunCapabilities; pub use context::{ContextError, DetectInput, Harness, RunContext, detect_run_context}; +pub use git::BASELINE_REF; +pub(crate) use git::IsolatedGit; pub(crate) use runtime::{ GIT_ROUTING_ENV_VARS, POSIX_RECIPE_TOOLS, POSIX_TOOLING_REQUIREMENT, clear_git_environment, posix_shell, require_posix_toolchain, validate_agent_environment_entry, diff --git a/src/source/mod.rs b/src/source/mod.rs index 2562048..a1ea1c8 100644 --- a/src/source/mod.rs +++ b/src/source/mod.rs @@ -14,9 +14,7 @@ use std::path::Path; -mod git; - -use git::IsolatedGit; +use crate::core::IsolatedGit; /// Branch a source that carries no Git history of its own is initialized on. /// Matches the branch a fixture-only task repository has always used, so a run @@ -121,7 +119,7 @@ fn resolve_path( let git = IsolatedGit::new().map_err(SourceError::msg)?; let text = |args: &[&str]| { - let output = git.run(&directory, args); + let output = git.run(&directory, args, &[]); (output.status == Some(0)) .then(|| String::from_utf8_lossy(&output.stdout).trim().to_string()) .filter(|value| !value.is_empty()) @@ -398,7 +396,7 @@ fn copy_from_cache(cache: &Path, dest: &Path) -> Result<(), SourceError> { /// Run git in `cwd`, turning a non-zero exit into an error naming the intent. fn checked(git: &IsolatedGit, cwd: &Path, args: &[&str], intent: &str) -> Result<(), SourceError> { - let output = git.run(cwd, args); + let output = git.run(cwd, args, &[]); if output.status == Some(0) { return Ok(()); } @@ -437,7 +435,7 @@ fn default_branch(refs: &[(String, String)], url: &str) -> Result Result, SourceError> { let git = IsolatedGit::new().map_err(SourceError::msg)?; - let output = git.run(Path::new("."), &["ls-remote", "--symref", url]); + let output = git.run(Path::new("."), &["ls-remote", "--symref", url], &[]); if output.status != Some(0) { return Err(SourceError::msg(format!( "could not read {subject} repository {url}: {}", From 5f5c1962389d5463459f10c9c6e0365f1e523495 Mon Sep 17 00:00:00 2001 From: samiamorwas Date: Thu, 20 Aug 2026 03:41:38 -0400 Subject: [PATCH 2/2] feat(diff-scope): measure and capture diffs with Git MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diff-scope snapshotted a task's start state by copying every file in the environment into `diff-scope-baseline/files`, then walked both trees to produce four counters. Against a real codebase that doubles disk per environment and adds a full tree walk per task — and it never produced the diff itself, which is the evidence a judge needs to answer whether the code got better. Every environment is now a Git repository marked with `eval-magic/baseline` at the state the agent started from, so Git can supply both. Measurement seeds a scratch index from that ref, brings it up to the working tree with one `git add`, and diffs the two trees: creations, modifications, and deletions fall out of one pass, and untracked creations are not missed. The scratch index lives outside the repository, so an eval that ran git itself keeps its own index and HEAD. Each run now also gets `diff.patch` beside its metrics, capped and marked when a diff runs past the cap, and a changed-file list in `diff-scope.json`. What counts is what Git counts, which changes two documented behaviors. The codebase's own `.gitignore` now applies, so a run that compiles no longer reports its build output as thousands of touched files — the same rule the baseline commit was already built under. And nested repository metadata is no longer measurable at all, because Git indexes no path with a `.git` component. Renames are switched off deliberately: a rename is two touched files, which is what the metric has always meant. The four existing integration tests pass with their metric expectations unchanged. Co-Authored-By: Claude Opus 5 --- Cargo.lock | 36 -- Cargo.toml | 2 - docs/guides/codebase.md | 41 ++- docs/progressive-enhancements.md | 42 ++- schema/diff-scope.schema.json | 34 +- schema/evals.schema.json | 4 +- src/cli/args.rs | 21 +- src/cli/run/orchestrate/build.rs | 6 +- src/core/fs.rs | 178 +-------- src/pipeline/diff_scope.rs | 599 +++++++++++++++---------------- src/pipeline/diff_scope/tests.rs | 464 ++++++++++++++++++++++++ src/pipeline/mod.rs | 2 +- src/validation/schema.rs | 30 ++ tests/cli/basics.rs | 13 + tests/cli/docs.rs | 11 +- tests/run/diff_scope.rs | 135 +++++-- tests/run/env_layout.rs | 21 +- tests/run/helpers.rs | 41 +++ 18 files changed, 1092 insertions(+), 588 deletions(-) create mode 100644 src/pipeline/diff_scope/tests.rs diff --git a/Cargo.lock b/Cargo.lock index 4c67cff..02767af 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -287,11 +287,9 @@ dependencies = [ "regex", "serde", "serde_json", - "similar", "tempfile", "thiserror", "toml", - "walkdir", ] [[package]] @@ -881,15 +879,6 @@ version = "1.0.22" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b39cdef0fa800fc44525c84ccb54a029961a8215f9619753635a9c0d2538d46d" -[[package]] -name = "same-file" -version = "1.0.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "93fc1dc3aaa9bfed95e02e6eadabb4baf7e3078b0bd1b4d7b6b0b68378900502" -dependencies = [ - "winapi-util", -] - [[package]] name = "scopeguard" version = "1.2.0" @@ -949,12 +938,6 @@ dependencies = [ "serde_core", ] -[[package]] -name = "similar" -version = "3.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6505efef05804732ed8a3f2d4f279429eb485bd69d5b0cc6b19cc02005cda16" - [[package]] name = "smallvec" version = "1.15.1" @@ -1137,16 +1120,6 @@ dependencies = [ "libc", ] -[[package]] -name = "walkdir" -version = "2.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "29790946404f91d9c5d06f9874efddea1dc06c5efe94541a7d6863108e3a5e4b" -dependencies = [ - "same-file", - "winapi-util", -] - [[package]] name = "wasip2" version = "1.0.3+wasi-0.2.9" @@ -1201,15 +1174,6 @@ dependencies = [ "unicode-ident", ] -[[package]] -name = "winapi-util" -version = "0.1.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" -dependencies = [ - "windows-sys", -] - [[package]] name = "windows-link" version = "0.2.1" diff --git a/Cargo.toml b/Cargo.toml index ba06307..81ec299 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,14 +44,12 @@ jsonschema = { version = "0.46.5", default-features = false } regex = { version = "1.12.3", default-features = false, features = ["std", "perf"] } serde = { version = "1.0.228", features = ["derive"] } serde_json = { version = "1.0.150", features = ["preserve_order"] } -similar = { version = "3.1.1", default-features = false } tempfile = "3.27.0" thiserror = "2.0.18" # Harness descriptor files (harnesses/*.toml). `display` serializes the # resolved (layer-merged) descriptor back to authorable TOML for # `harness show`. toml = { version = "0.9", default-features = false, features = ["parse", "serde", "display"] } -walkdir = "2.5.0" [dev-dependencies] assert_cmd = "2.2.2" diff --git a/docs/guides/codebase.md b/docs/guides/codebase.md index ac8a448..5537ff0 100644 --- a/docs/guides/codebase.md +++ b/docs/guides/codebase.md @@ -68,6 +68,34 @@ Each dispatch gets its own private environment holding: An eval that declares no `codebase` still gets a Git repository, initialized on `work`, exactly as it always has. +## The baseline ref is what the run is measured against + +Nothing writes into an environment after that ref is written, so it names exactly what the agent +started from — and everything the agent did is the difference from it. + +During `ingest`, Git measures that difference. Each run gets: + +- `diff-scope.json` — `files_touched`, `lines_added`, `lines_removed`, and `hunks`, plus the list of + changed files with a status of `added`, `modified`, or `deleted` +- `diff.patch` — the diff itself, which is the evidence a judge reads to answer whether the work was + any good. It always exists; for a run that changed nothing it is empty. A diff past the capture + cap is cut at a line boundary and carries a marker saying so, and `patch.truncated` in + `diff-scope.json` records it. + +What counts is what Git counts, under the same rules the baseline commit was built under: + +- The codebase's own `.gitignore` holds, so a run that compiles does not report its build output as + thousands of touched files. +- Fixtures and staged skills count even when the codebase ignores their paths — they are committed + into the baseline regardless, so a change to one is always visible. +- Framework artifacts under `.eval-magic-outputs/` never count. +- A nested repository's internals never count: Git tracks no path with a `.git` component. +- A rename counts as two touched files, one created and one deleted. +- A binary file counts as one touched file, contributing no lines. + +A `diff_scope` assertion gates `max_files_touched`, `max_lines_changed` (added plus removed), or +both, against exactly these numbers. + ## One checkout per iteration Every environment a run provisions — each `(eval, condition, run)` cell — is built from one cached @@ -103,7 +131,8 @@ paths. Seeding a task-specific file into a real project is the common case: A fixture overwrites a codebase file of the same path. The baseline the runner commits respects the codebase's `.gitignore`, so ignored build output stays -out of it. Fixtures and staged skills are committed regardless of what the codebase ignores. +out of it. Fixtures and staged skills are committed regardless of what the codebase ignores — which +is also what keeps them inside every later measurement. ## A `path` source is not reproducible elsewhere @@ -133,6 +162,16 @@ git status --porcelain `git remote -v` and `git status --porcelain` are both empty, and the two revisions match: the baseline ref names exactly what the agent started from. +After a dispatch and `ingest`, read what the run produced: + +```sh +jq '{files_touched, lines_added, lines_removed, hunks, files, patch}' diff-scope.json +head -50 diff.patch +``` + +The same difference, spelled by Git itself, is `git diff refs/eval-magic/baseline` inside the +environment. + The resolved commit appears in `conditions.json`, each `run.json`, `benchmark.json`, and the `BASELINE.md` written by `promote-baseline` — alongside the skill the run measured, which is recorded the same way: diff --git a/docs/progressive-enhancements.md b/docs/progressive-enhancements.md index eda9dfb..21f5d55 100644 --- a/docs/progressive-enhancements.md +++ b/docs/progressive-enhancements.md @@ -37,8 +37,9 @@ A harness qualifies at baseline with no harness-specific code beyond naming itse That baseline already yields a working eval: `llm_judge` assertions grade soft behavior, runner-owned `command_check` assertions can inject held-out files and execute deterministically, -runner-owned final-environment metrics land in `diff-scope.json`, `diff_scope` assertions gate -files/lines deterministically, and the `detect-stray-writes` post-pass (folded into `ingest`) audits +runner-owned final-environment metrics land in `diff-scope.json` with the diff itself in +`diff.patch`, `diff_scope` assertions gate files/lines deterministically, and the +`detect-stray-writes` post-pass (folded into `ingest`) audits writes that leave the private task environment. Run records without transcript ingest are assembled from `outputs/final-message.md` or by hand per `schema/run-record.schema.json`. @@ -91,19 +92,34 @@ generic fresh-session fallback can preserve the meaning of a canned reply. ## Runner-owned environment checks are baseline Every canonical `(eval, condition, run)` gets a distinct `eval_root`. After fixtures, staging, and -guard installation, `run` recreates a runner-owned Git repository at that root, commits the task -state on branch `work`, runs shadow preflight at the resulting repository boundary, and snapshots -the task environment. Git is therefore a runtime prerequisite; each task starts clean and has no -remotes. During `ingest`, before any held-out setup is injected, the runner compares that baseline -with the final environment and writes raw `files_touched`, `lines_added`, `lines_removed`, and -zero-context Myers `hunks` to `diff-scope.json`. Framework artifacts under the task root's -`.eval-magic-outputs/` and runner-owned `.git/` are excluded; nested repository metadata and all -other new files count. `benchmark.json` preserves these metrics per run even without a `diff_scope` -assertion. An assertion may gate `max_files_touched`, `max_lines_changed` (added plus removed), or -both. +guard installation, `run` establishes a runner-owned Git repository at that root, commits the task +state, marks it with `refs/eval-magic/baseline`, and runs shadow preflight at the resulting +repository boundary. Git is therefore a runtime prerequisite; each task starts clean and has no +remotes. Nothing writes into an environment after the ref is written, so it names exactly what the +agent started from. + +During `ingest`, before any held-out setup is injected, Git measures the final environment against +that ref. The runner seeds a scratch index from the baseline, brings it up to the working tree with +one `git add`, and diffs the two trees — so creations, modifications, and deletions all fall out of +one pass, and an untracked creation is not missed. Raw `files_touched`, `lines_added`, +`lines_removed`, and zero-context `hunks` go to `diff-scope.json`, alongside the changed-file list; +the diff itself goes to `diff.patch` beside it, capped and marked when a diff exceeds the cap. +`benchmark.json` preserves the metrics per run even without a `diff_scope` assertion. An assertion +may gate `max_files_touched`, `max_lines_changed` (added plus removed), or both. + +**What counts is what Git counts.** The measurement runs under the same rules the baseline commit +was built under: the codebase's own `.gitignore` holds, so a run that compiles does not report its +build output as thousands of touched files, and the `.git/info/exclude` entry keeps framework +artifacts under `.eval-magic-outputs/` out. Paths the runner force-added despite those rules — the +harness config directories and the declared fixture overlay — are tracked in the baseline and stay +measured. Git indexes no path with a `.git` component, so a nested repository's internals are +invisible, not just the runner-owned root `.git`. Renames are switched off deliberately: a rename is +two touched files, one created and one deleted, which is what the metric has always meant. A binary +file counts as one touched file with no countable lines. This is deliberately a secondary signal: a smaller diff can be focused, but it can also be -incomplete. Pair a scope gate with a correctness assertion. +incomplete. Pair a scope gate with a correctness assertion. The patch is the evidence that closes +that gap — it is what a judge reads to answer whether the work was any good. `command_check` is intentionally not a harness enhancement. `run` detects the assertion before dispatch so it can validate held-out sources before building. After diff-scope capture, `ingest` diff --git a/schema/diff-scope.schema.json b/schema/diff-scope.schema.json index dc5aceb..ad77d3c 100644 --- a/schema/diff-scope.schema.json +++ b/schema/diff-scope.schema.json @@ -2,14 +2,40 @@ "$schema": "http://json-schema.org/draft-07/schema#", "$id": "https://slow-powers.dev/schemas/diff-scope.schema.json", "title": "Diff Scope Metrics", - "description": "Runner-owned final-environment diff metrics for one eval run. Compares the complete task environment with its post-staging, post-guard baseline while excluding .eval-magic-outputs framework artifacts. Lives beside run.json as diff-scope.json.", + "description": "Runner-owned final-environment diff evidence for one eval run. Git measures the complete task environment against the refs/eval-magic/baseline ref its environment was marked with, honoring the codebase's own .gitignore and the .eval-magic-outputs framework exclusion. Lives beside run.json as diff-scope.json, with the diff itself in diff.patch.", "type": "object", "required": ["files_touched", "lines_added", "lines_removed", "hunks"], "additionalProperties": false, "properties": { "files_touched": { "type": "integer", "minimum": 0 }, - "lines_added": { "type": "integer", "minimum": 0, "description": "Byte-lines inserted by a Myers diff." }, - "lines_removed": { "type": "integer", "minimum": 0, "description": "Byte-lines deleted by a Myers diff." }, - "hunks": { "type": "integer", "minimum": 0, "description": "Contiguous non-equal operation groups, with zero context." } + "lines_added": { "type": "integer", "minimum": 0, "description": "Lines inserted, as git diff --numstat counts them. A binary file contributes none." }, + "lines_removed": { "type": "integer", "minimum": 0, "description": "Lines deleted, as git diff --numstat counts them. A binary file contributes none." }, + "hunks": { "type": "integer", "minimum": 0, "description": "Contiguous non-equal operation groups, counted at zero context." }, + "files": { + "type": "array", + "description": "Every changed file, ordered by path as Git reports them. Omitted for iterations created before the changed-file list.", + "items": { + "type": "object", + "required": ["path", "status", "lines_added", "lines_removed"], + "additionalProperties": false, + "properties": { + "path": { "type": "string", "description": "Environment-relative path, spelled with forward slashes as Git spells it." }, + "status": { "type": "string", "enum": ["added", "modified", "deleted"] }, + "lines_added": { "type": "integer", "minimum": 0 }, + "lines_removed": { "type": "integer", "minimum": 0 } + } + } + }, + "patch": { + "type": "object", + "description": "The captured diff beside this record. Omitted for iterations created before patch capture.", + "required": ["path", "bytes", "truncated"], + "additionalProperties": false, + "properties": { + "path": { "type": "string", "description": "Run-relative name of the patch file." }, + "bytes": { "type": "integer", "minimum": 0, "description": "Size of the written patch, including any truncation marker." }, + "truncated": { "type": "boolean", "description": "True when the diff exceeded the capture cap and the file carries a marker in place of the rest." } + } + } } } diff --git a/schema/evals.schema.json b/schema/evals.schema.json index d118cb5..c0b5d17 100644 --- a/schema/evals.schema.json +++ b/schema/evals.schema.json @@ -278,12 +278,12 @@ "max_files_touched": { "type": "integer", "minimum": 0, - "description": "Maximum number of changed, deleted, or newly-created files allowed in the final task environment. Framework files under the task root's .eval-magic-outputs and runner-owned .git are excluded; nested .git metadata remains measurable." + "description": "Maximum number of changed, deleted, or newly-created files allowed in the final task environment, as Git reports them against the refs/eval-magic/baseline ref. The codebase's own .gitignore applies, so ignored build output does not count; framework files under .eval-magic-outputs and anything under a .git directory never count; a rename counts as two files." }, "max_lines_changed": { "type": "integer", "minimum": 0, - "description": "Maximum total byte-lines added plus byte-lines removed allowed. Diffing uses Myers operations with zero-context hunks." + "description": "Maximum total lines added plus lines removed allowed, as git diff --numstat counts them. A binary file contributes no lines. Hunks are counted at zero context." } } } diff --git a/src/cli/args.rs b/src/cli/args.rs index 7978454..69d0aff 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -624,8 +624,10 @@ pub(crate) enum Commands { /// grade. Assembles each task's `run.json` + `timing.json`, scans for stray /// writes, and maps raw per-env guard logs through `dispatch.json` into /// `guard-denials.json` (including tasks without `run.json`). Malformed raw - /// records fail with their source path and line number. It captures always-on - /// final-environment files/lines/hunks in `diff-scope.json`, grades + /// records fail with their source path and line number. It measures the + /// finished environment against the `eval-magic/baseline` ref it was marked + /// with, writing always-on files/lines/hunks and the changed-file list to + /// `diff-scope.json` and the diff itself to `diff.patch`, grades /// `transcript_check` assertions, prepares /// `diff_scope` grading for finalize, injects held-out /// `command_check.setup_files`, and executes each @@ -645,7 +647,9 @@ pub(crate) enum Commands { /// runner-owned `command_check` results, and deterministic `diff_scope` /// files/lines thresholds into normal `grading.json` files, then writes /// `benchmark.json` with a per-assertion `passed`/`n` rollup from observed - /// assertion results and raw per-run metrics from `diff-scope.json`. If a live + /// assertion results and raw per-run metrics from `diff-scope.json`. The + /// per-run changed-file list and `diff.patch` stay beside each run rather + /// than being rolled up. If a live /// guard remains armed — the cwd guard, or any per-task Cli env guard — prints /// a `teardown` reminder before source edits. Requires `--iteration`. Finalize(CommonArgs), @@ -698,12 +702,16 @@ pub(crate) enum Commands { DetectStrayWrites(CommonArgs), /// Grade run records (runner checks + LLM-judge task emission). /// - /// Captures always-on final-environment files/lines/hunks in `diff-scope.json` + /// Captures always-on final-environment files/lines/hunks plus the + /// changed-file list in `diff-scope.json`, writes the diff itself to + /// `diff.patch` beside it (truncated with a marker past its size cap), /// and evaluates `transcript_check` assertions directly: regex against /// tool invocations or, for scripted evals, assistant messages across rounds. /// Checks can require a match before the final completion claim or before the /// first write/patch tool call. A `diff_scope` assertion gates the captured file count - /// and/or added-plus-removed line count. Grade captures scope before it injects + /// and/or added-plus-removed line count. Git supplies both, so the codebase's + /// own `.gitignore` decides what counts and ignored build output stays out. + /// Grade captures scope before it injects /// held-out `command_check.setup_files` and executes each runner-owned command /// in its task environment, applying fixed environment overrides and running /// every environment matrix cell; completed command and diff-scope results @@ -729,7 +737,8 @@ pub(crate) enum Commands { /// grouped findings in schema-v2 `plugin-shadow.json` (legacy unversioned /// reports remain readable) unless it records the resolved descriptor's /// `isolates_live_sources = true` assertion), and raw per-run files/lines/hunks - /// from `diff-scope.json`. Shadow findings retain their intrinsic warning or + /// from `diff-scope.json`. Each run's changed-file list and its `diff.patch` + /// stay in the run directory. Shadow findings retain their intrinsic warning or /// comparison-invalid severity, per-cell appearances, resolution, and /// remediation. A timing metric with `n: 0` is unavailable, not a measured /// zero. The top-level `diff_scope` field is omitted for compatible older diff --git a/src/cli/run/orchestrate/build.rs b/src/cli/run/orchestrate/build.rs index a9707d7..e9a71f8 100644 --- a/src/cli/run/orchestrate/build.rs +++ b/src/cli/run/orchestrate/build.rs @@ -416,10 +416,12 @@ pub(super) fn post_build( // exist, but before project-local skill discovery inspects ancestor state. // Recreating `.git` also resets explicit iteration rebuilds to one clean, // runner-owned baseline with no inherited history or remotes. + // + // This is also where the diff baseline is captured: the `eval-magic/baseline` + // ref written here marks the state every later measurement is the difference + // from. Nothing below writes into an environment, so the ref stays exact. super::git::initialize_task_repositories(ctx, r)?; super::shadow_preflight::run(ctx, opts, r, staged, &targets)?; - crate::pipeline::capture_iteration_baselines(&r.iteration_dir) - .map_err(|error| RunError::msg(error.to_string()))?; Ok(()) } diff --git a/src/core/fs.rs b/src/core/fs.rs index 1529bfe..71fa8d2 100644 --- a/src/core/fs.rs +++ b/src/core/fs.rs @@ -6,16 +6,12 @@ //! 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*: -//! -//! - [`copy_entry`] mirrors structure, recreating symlinks as symlinks. Right -//! when the copy must round-trip faithfully — the diff-scope baseline, which -//! is later compared byte-for-byte against the live tree. -//! - [`copy_entry_materialized`] resolves symlinks into their target's content. -//! Right for everything else here: staging and fixtures copy *into* an -//! isolated task env, where a preserved link would point back out of the -//! sandbox, and snapshots must freeze content so a later run compares against -//! what was captured. +//! [`copy_entry_materialized`] is the one way to copy here, and it resolves +//! symlinks into their target's content rather than mirroring them. Every +//! destination in this tree wants that: staging and fixtures copy *into* an +//! isolated task env, where a preserved link would point back out of the +//! sandbox, and a snapshot must freeze content so a later run compares against +//! what was captured rather than whatever the link now points at. //! //! Every function returns [`std::io::Result`], which each consumer error enum //! (`PipelineError`, `WorkspaceError`, `RunError`) already absorbs via @@ -133,40 +129,12 @@ pub fn write_json(path: &Path, value: &T) -> io::Result<( fs::write(path, text) } -/// Copy `source` to `destination`, recursing into directories and **preserving** -/// symlinks as symlinks. Missing parent directories of `destination` are created. -/// -/// Use this only when the copy must round-trip faithfully; see -/// [`copy_entry_materialized`] for the content-freezing counterpart, which is -/// what callers copying into a task env or a snapshot want. -pub fn copy_entry(source: &Path, destination: &Path) -> io::Result<()> { - let metadata = fs::symlink_metadata(source)?; - if metadata.file_type().is_symlink() { - create_parent(destination)?; - let target = fs::read_link(source)?; - 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)? { - let entry = entry?; - copy_entry(&entry.path(), &destination.join(entry.file_name()))?; - } - } else { - create_parent(destination)?; - fs::copy(source, destination)?; - } - Ok(()) -} - /// Copy `source` to `destination`, recursing into directories and **resolving** /// symlinks into their target's content. /// -/// The counterpart to [`copy_entry`], for callers that must freeze content -/// rather than mirror structure: a snapshot exists to be compared against -/// later, so a preserved link would silently track whatever it points at -/// instead of what was captured. Prefer [`copy_entry`] unless you specifically -/// need that guarantee. +/// Callers here must freeze content rather than mirror structure: a snapshot +/// exists to be compared against later, so a preserved link would silently +/// track whatever it points at instead of what was captured. pub fn copy_entry_materialized(source: &Path, destination: &Path) -> io::Result<()> { // `metadata` (unlike `symlink_metadata`) follows links, so a symlinked // directory recurses and a symlinked file lands in the `fs::copy` arm. @@ -210,10 +178,15 @@ pub fn hardlinks_available(from: &Path, to: &Path) -> bool { /// Create a symlink at `link` pointing at `target`. /// +/// Test support. Copying here resolves links into content rather than +/// recreating them, so the only callers left are fixtures that need a link to +/// exist and the probe that asks whether this host permits one. +/// /// `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. +#[cfg(test)] pub(crate) fn create_symlink(target: &Path, link: &Path, to_directory: bool) -> io::Result<()> { #[cfg(unix)] { @@ -457,120 +430,6 @@ mod tests { ); } - #[test] - fn copy_entry_copies_a_single_file() { - let tmp = TempDir::new().unwrap(); - let source = tmp.path().join("src.txt"); - fs::write(&source, "payload").unwrap(); - - copy_entry(&source, &tmp.path().join("dst.txt")).unwrap(); - - assert_eq!( - fs::read_to_string(tmp.path().join("dst.txt")).unwrap(), - "payload" - ); - } - - #[test] - fn copy_entry_recurses_into_directories() { - let tmp = TempDir::new().unwrap(); - let source = tmp.path().join("tree"); - fs::create_dir_all(source.join("nested/deeper")).unwrap(); - fs::write(source.join("top.txt"), "top").unwrap(); - fs::write(source.join("nested/deeper/leaf.txt"), "leaf").unwrap(); - - let destination = tmp.path().join("copied"); - copy_entry(&source, &destination).unwrap(); - - assert_eq!( - fs::read_to_string(destination.join("top.txt")).unwrap(), - "top" - ); - assert_eq!( - fs::read_to_string(destination.join("nested/deeper/leaf.txt")).unwrap(), - "leaf" - ); - } - - /// The destination's parent may not exist yet (staging writes into a tree it - /// is still building). Failing here would make the helper's usability depend - /// on caller ordering. - #[test] - fn copy_entry_creates_missing_destination_parents() { - let tmp = TempDir::new().unwrap(); - let source = tmp.path().join("src.txt"); - fs::write(&source, "payload").unwrap(); - - let destination = tmp.path().join("a/b/c/dst.txt"); - copy_entry(&source, &destination).unwrap(); - - assert_eq!(fs::read_to_string(&destination).unwrap(), "payload"); - } - - /// The behavior that used to differ between the five copies: a symlink must - /// 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. - #[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"); - create_symlink(&target, &link, false).unwrap(); - - let destination = tmp.path().join("copied-link.txt"); - copy_entry(&link, &destination).unwrap(); - - assert!( - fs::symlink_metadata(&destination) - .unwrap() - .file_type() - .is_symlink(), - "the copy is still a symlink, not a materialized file" - ); - assert_eq!(fs::read_link(&destination).unwrap(), target); - } - - /// A symlink nested inside a copied directory survives too — the recursion - /// arm must route back through the symlink arm, not through `fs::copy`. - #[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(); - create_symlink(Path::new("real.txt"), &source.join("alias.txt"), false).unwrap(); - - let destination = tmp.path().join("copied"); - copy_entry(&source, &destination).unwrap(); - - assert!( - fs::symlink_metadata(destination.join("alias.txt")) - .unwrap() - .file_type() - .is_symlink(), - "the nested symlink is still a symlink" - ); - assert_eq!( - fs::read_link(destination.join("alias.txt")).unwrap(), - Path::new("real.txt"), - "the link target is preserved verbatim, including its relativeness" - ); - } - /// 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. @@ -622,15 +481,6 @@ mod tests { ); } - #[test] - fn copy_entry_reports_a_missing_source() { - let tmp = TempDir::new().unwrap(); - - let err = copy_entry(&tmp.path().join("absent"), &tmp.path().join("dst")).unwrap_err(); - - assert_eq!(err.kind(), io::ErrorKind::NotFound); - } - /// The probe both succeeds and cleans up after itself: it runs inside the /// per-iteration codebase cache, where a leftover file would ship into the /// next environment built from it. diff --git a/src/pipeline/diff_scope.rs b/src/pipeline/diff_scope.rs index 9640dd8..3ff01bb 100644 --- a/src/pipeline/diff_scope.rs +++ b/src/pipeline/diff_scope.rs @@ -1,24 +1,31 @@ -//! Baseline capture and deterministic final-environment diff metrics. +//! Deterministic final-environment diff metrics, measured with Git. //! -//! A baseline snapshots every file in a task's private `eval_root` after -//! framework setup. Measurement compares the completed task with that snapshot, -//! excluding only the task's `.eval-magic-outputs` subtree. - -use std::collections::{BTreeSet, HashMap}; +//! Every task environment is a Git repository marked with [`BASELINE_REF`] at +//! the state the agent started from, so the difference between that ref and the +//! finished working tree *is* the measurement. Nothing is copied and nothing is +//! walked: Git already knows what changed, and it knows it while honoring the +//! codebase's own `.gitignore` and the `.eval-magic-outputs` exclusion the +//! runner writes. + +use std::collections::HashMap; use std::fs; -use std::path::{Path, PathBuf}; +use std::path::Path; use serde::{Deserialize, Serialize}; -use similar::{Algorithm, DiffTag, capture_diff_slices}; -use walkdir::{DirEntry, WalkDir}; -use crate::core::fs::{copy_entry, write_json}; +use crate::core::fs::write_json; +use crate::core::{BASELINE_REF, IsolatedGit}; use crate::pipeline::error::PipelineError; -const BASELINE_DIR: &str = "diff-scope-baseline"; -const BASELINE_MANIFEST: &str = "manifest.json"; -const BASELINE_FILES: &str = "files"; const RESULT_FILE: &str = "diff-scope.json"; +/// The diff itself, beside the metrics that summarize it. Named in the record +/// rather than only known by convention, so a reader of `diff-scope.json` can +/// find it without knowing this constant. +const PATCH_FILE: &str = "diff.patch"; +/// How much of a run's diff is captured. A safety valve against an agent that +/// rewrites a whole tree, not a judging budget — a realistic task diff is far +/// below it, and bounding evidence for a judge is a separate concern. +const PATCH_BYTE_LIMIT: usize = 1_048_576; #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub struct DiffScopeMetrics { @@ -28,17 +35,58 @@ pub struct DiffScopeMetrics { pub hunks: u64, } +/// One run's complete diff evidence: the counters, and where the patch is. +/// +/// Written as `diff-scope.json`. The counters stay flattened at the top level — +/// they are what `benchmark.json` aggregates and what a `diff_scope` assertion +/// grades, and a reader that wants only those can still deserialize +/// [`DiffScopeMetrics`] straight from this artifact. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct DiffScopeRecord { + #[serde(flatten)] + pub metrics: DiffScopeMetrics, + /// Every changed file, ordered by path as Git reports them. + pub files: Vec, + pub patch: PatchRecord, +} + +/// One file the task changed. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ChangedFile { + /// Environment-relative path, spelled with forward slashes as Git spells it. + pub path: String, + pub status: ChangeStatus, + pub lines_added: u64, + pub lines_removed: u64, +} + +/// What happened to a changed file. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ChangeStatus { + Added, + Modified, + Deleted, +} + +/// Where a run's patch is and whether it is the whole diff. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PatchRecord { + /// Name of the patch file beside this record. + pub path: String, + pub bytes: u64, + /// True when the diff exceeded the cap and the file carries a marker in + /// place of the rest. A grader reading a truncated patch is reading part of + /// the story, and has to be able to tell. + pub truncated: bool, +} + impl DiffScopeMetrics { pub fn lines_changed(self) -> u64 { self.lines_added.saturating_add(self.lines_removed) } } -#[derive(Debug, Serialize, Deserialize)] -struct BaselineManifest { - preexisting_files: Vec, -} - #[derive(Debug, Deserialize)] struct DispatchFile { #[serde(default)] @@ -68,72 +116,6 @@ pub struct DiffScopeSummary { pub warnings: Vec, } -#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] -struct FileDiff { - lines_added: u64, - lines_removed: u64, - hunks: u64, -} - -fn diff_bytes(old: &[u8], new: &[u8]) -> FileDiff { - let old_lines: Vec<&[u8]> = old.split_inclusive(|byte| *byte == b'\n').collect(); - let new_lines: Vec<&[u8]> = new.split_inclusive(|byte| *byte == b'\n').collect(); - let mut result = FileDiff::default(); - let mut in_hunk = false; - - for operation in capture_diff_slices(Algorithm::Myers, &old_lines, &new_lines) { - let (tag, old_range, new_range) = operation.as_tag_tuple(); - match tag { - DiffTag::Equal => in_hunk = false, - DiffTag::Delete => { - if !in_hunk { - result.hunks += 1; - in_hunk = true; - } - result.lines_removed += old_range.len() as u64; - } - DiffTag::Insert => { - if !in_hunk { - result.hunks += 1; - in_hunk = true; - } - result.lines_added += new_range.len() as u64; - } - DiffTag::Replace => { - if !in_hunk { - result.hunks += 1; - in_hunk = true; - } - result.lines_removed += old_range.len() as u64; - result.lines_added += new_range.len() as u64; - } - } - } - result -} - -pub fn capture_iteration_baselines(iteration_dir: &Path) -> Result<(), PipelineError> { - let dispatch_path = iteration_dir.join("dispatch.json"); - let dispatch: DispatchFile = serde_json::from_str(&fs::read_to_string(&dispatch_path)?)?; - for task in dispatch.tasks { - let eval_root = task.eval_root.ok_or_else(|| { - PipelineError::Message(format!( - "dispatch task in {} has no eval_root for diff-scope capture", - dispatch_path.display() - )) - })?; - let run_dir = Path::new(&task.run_record_path).parent().ok_or_else(|| { - PipelineError::Message(format!( - "diff-scope task has no run directory in run_record_path: {}", - task.run_record_path - )) - })?; - fs::create_dir_all(run_dir)?; - capture_task_baseline(Path::new(&eval_root), run_dir)?; - } - Ok(()) -} - pub fn measure_iteration_diff_scopes( iteration_dir: &Path, ) -> Result { @@ -197,9 +179,9 @@ pub fn measure_iteration_diff_scopes( summary.shared_environment += 1; continue; } - if !run_dir.join(BASELINE_DIR).join(BASELINE_MANIFEST).exists() { + if !has_baseline(Path::new(eval_root))? { summary.warnings.push(format!( - "{}/{}{run_label} has no pre-dispatch baseline — diff-scope unavailable; rebuild the iteration to capture metrics", + "{}/{}{run_label} has no {BASELINE_REF} in its environment — diff-scope unavailable; the environment was removed, or the iteration predates the baseline ref and needs rebuilding", task.eval_id, task.condition )); summary.missing_baseline += 1; @@ -212,268 +194,251 @@ pub fn measure_iteration_diff_scopes( ))); } - let metrics = measure_task_diff(Path::new(eval_root), run_dir)?; - crate::validation::validate_against_schema::( + let record = measure_task_diff(Path::new(eval_root), run_dir)?; + crate::validation::validate_against_schema::( crate::validation::SchemaName::DiffScope, - &serde_json::to_value(metrics)?, + &serde_json::to_value(&record)?, &result_path.to_string_lossy(), )?; - write_json(&result_path, &metrics)?; + write_json(&result_path, &record)?; summary.measured += 1; } Ok(summary) } -fn capture_task_baseline(eval_root: &Path, run_dir: &Path) -> Result<(), PipelineError> { - let baseline_dir = run_dir.join(BASELINE_DIR); - if baseline_dir.exists() { - fs::remove_dir_all(&baseline_dir)?; - } - let file_snapshot = baseline_dir.join(BASELINE_FILES); - fs::create_dir_all(&file_snapshot)?; +/// Whether `eval_root` still carries the ref a measurement is taken against. +/// +/// False for a torn-down environment, a root that is not a repository, and an +/// iteration built before the baseline ref existed — all reported gaps rather +/// than failures, so one unmeasurable task does not stop the stage. A host that +/// cannot give git an isolated configuration is a different thing entirely, and +/// errors rather than being reported as one more missing baseline. +fn has_baseline(eval_root: &Path) -> Result { + let git = IsolatedGit::new().map_err(PipelineError::Message)?; + Ok(git + .run( + eval_root, + &["rev-parse", "--verify", "--quiet", BASELINE_REF], + &[], + ) + .status + == Some(0)) +} - let excluded_roots = [ - eval_root.join(".eval-magic-outputs"), - eval_root.join(".git"), - ]; - let mut preexisting_paths = walk_files(eval_root, &excluded_roots)?; - preexisting_paths.sort(); - let preexisting_files = preexisting_paths - .iter() - .map(|path| relative_key(eval_root, path)) - .collect::, _>>()?; - write_json( - &baseline_dir.join(BASELINE_MANIFEST), - &BaselineManifest { preexisting_files }, +/// Measure the final environment against the state the agent started from. +/// +/// The environment is a Git repository whose start state is [`BASELINE_REF`], so +/// Git supplies the metrics: a scratch index seeded from that ref, brought up to +/// the working tree, is exactly "everything the agent changed". Creations, +/// modifications, and deletions all fall out of one `git add`, and untracked +/// creations are not missed. +/// +/// The scratch index lives outside the repository so the environment's own index +/// and `HEAD` are untouched — an eval may legitimately have run `git` itself. The +/// blobs `git add` writes do land in the environment's object store; measurement +/// runs post-dispatch against a disposable artifact, and re-running it over the +/// same working tree yields the same tree, so that is harmless. +fn measure_task_diff(eval_root: &Path, run_dir: &Path) -> Result { + let git = IsolatedGit::new().map_err(PipelineError::Message)?; + let scratch = tempfile::TempDir::new()?; + let index = scratch.path().join("index").to_string_lossy().into_owned(); + let env = [("GIT_INDEX_FILE", index.as_str())]; + + git_checked(&git, eval_root, &["read-tree", BASELINE_REF], &env)?; + // Unforced, so the codebase's own `.gitignore` and the `.git/info/exclude` + // entry for `.eval-magic-outputs/` both hold — the same rules the baseline + // commit was built under. A path the runner force-added despite those rules + // is already tracked by `read-tree`, and stays measured. + git_checked(&git, eval_root, &["add", "--all", "--", "."], &env)?; + let measured = git_checked(&git, eval_root, &["write-tree"], &env)?; + let measured = String::from_utf8_lossy(&measured).trim().to_string(); + + let numstat = git_checked( + &git, + eval_root, + &diff_args(&["--numstat", "-z"], &measured), + &env, + )?; + let statuses = git_checked( + &git, + eval_root, + &diff_args(&["--name-status", "-z"], &measured), + &env, )?; + let zero_context = git_checked( + &git, + eval_root, + &diff_args(&["--unified=0"], &measured), + &env, + )?; + let files = changed_files(&numstat, &statuses)?; + let metrics = DiffScopeMetrics { + files_touched: files.len() as u64, + lines_added: files.iter().map(|file| file.lines_added).sum(), + lines_removed: files.iter().map(|file| file.lines_removed).sum(), + hunks: count_hunks(&zero_context), + }; - for source in preexisting_paths { - let relative = source.strip_prefix(eval_root).map_err(|_| { - PipelineError::Message(format!( - "diff-scope baseline path {} is outside {}", - source.display(), - eval_root.display() - )) - })?; - copy_entry(&source, &file_snapshot.join(relative))?; + let patch = git_checked( + &git, + eval_root, + &diff_args(&["--unified=3"], &measured), + &env, + )?; + let (captured, truncated) = truncate_patch(&patch, PATCH_BYTE_LIMIT); + fs::write(run_dir.join(PATCH_FILE), &captured)?; + Ok(DiffScopeRecord { + metrics, + files, + patch: PatchRecord { + path: PATCH_FILE.to_string(), + bytes: captured.len() as u64, + truncated, + }, + }) +} + +/// `patch` capped at `limit`, cut on a line boundary so the last diff line kept +/// is whole, with a marker in place of the rest. +/// +/// The marker is unconditional once the cap is crossed: a patch that stops +/// early and does not say so reads as a complete, smaller diff, and a grader +/// would draw the wrong conclusion from it. A single line longer than the whole +/// cap has no boundary to cut on, so it is cut at the cap — an uncapped +/// artifact is the thing being prevented. +fn truncate_patch(patch: &[u8], limit: usize) -> (Vec, bool) { + if patch.len() <= limit { + return (patch.to_vec(), false); } - Ok(()) + let head = &patch[..limit]; + let end = match head.iter().rposition(|byte| *byte == b'\n') { + Some(newline) => newline + 1, + None => limit, + }; + let mut captured = patch[..end].to_vec(); + captured.extend_from_slice( + format!( + "[eval-magic] patch truncated at {limit} bytes of {}; the remainder is not captured\n", + patch.len() + ) + .as_bytes(), + ); + (captured, true) } -fn measure_task_diff(eval_root: &Path, run_dir: &Path) -> Result { - let baseline_dir = run_dir.join(BASELINE_DIR); - let manifest: BaselineManifest = - serde_json::from_str(&fs::read_to_string(baseline_dir.join(BASELINE_MANIFEST))?)?; - let file_snapshot = baseline_dir.join(BASELINE_FILES); - let mut candidates = BTreeSet::new(); +/// A diff of the baseline against `measured`, with every configurable influence +/// on the numbers pinned. +/// +/// `--no-renames` because `diff.renames` defaults on: a detected rename reports +/// one entry with no line changes, where a rename is two touched files — one +/// created and one deleted. `--no-ext-diff` and `--no-textconv` keep a sourced +/// codebase's `.gitattributes` from deciding what a measurement sees. +fn diff_args<'a>(format: &[&'a str], measured: &'a str) -> Vec<&'a str> { + let mut args = vec!["diff", "--no-renames", "--no-ext-diff", "--no-textconv"]; + args.extend_from_slice(format); + args.push(BASELINE_REF); + args.push(measured); + args +} - for relative in manifest.preexisting_files { - if fs::symlink_metadata(file_snapshot.join(&relative)).is_err() { - return Err(PipelineError::Message(format!( - "diff-scope baseline is incomplete: missing snapshot for {relative}" - ))); - } - candidates.insert(relative); - } - let excluded_roots = [ - eval_root.join(".eval-magic-outputs"), - eval_root.join(".git"), - ]; - for path in walk_files(eval_root, &excluded_roots)? { - candidates.insert(relative_key(eval_root, &path)?); +/// Every changed file, from the two views Git offers of one diff. +/// +/// `--numstat -z` carries the line counts as `added\tremoved\tpath` per record; +/// `--name-status -z` carries the status as a `status`, `path` pair. Neither +/// format offers both, and no single `git diff` invocation emits both, so they +/// are joined by path here. +fn changed_files(numstat: &[u8], name_status: &[u8]) -> Result, PipelineError> { + // Keyed by the same lossy conversion the numstat side uses. A path Git spells + // in bytes that are not UTF-8 has to reach both sides identically, or it + // joins against nothing and loses its status. + let mut statuses: HashMap = HashMap::new(); + let mut fields = name_status + .split(|byte| *byte == 0) + .filter(|field| !field.is_empty()); + while let (Some(status), Some(path)) = (fields.next(), fields.next()) { + statuses.insert( + String::from_utf8_lossy(path).into_owned(), + change_status(status), + ); } - let mut metrics = DiffScopeMetrics::default(); - for relative in candidates { - let before = file_snapshot.join(&relative); - let after = eval_root.join(&relative); - let old = file_content(&before)?; - let new = file_content(&after)?; - if old == new { + let mut files = Vec::new(); + for record in numstat.split(|byte| *byte == 0) { + if record.is_empty() { continue; } - metrics.files_touched += 1; - let diff = match (old, new) { - (FileContent::Regular(old), FileContent::Regular(new)) => Some(diff_bytes(&old, &new)), - (FileContent::Missing, FileContent::Regular(new)) => Some(diff_bytes(&[], &new)), - (FileContent::Regular(old), FileContent::Missing) => Some(diff_bytes(&old, &[])), - _ => None, + let text = String::from_utf8_lossy(record); + let mut columns = text.splitn(3, '\t'); + let (Some(added), Some(removed), Some(path)) = + (columns.next(), columns.next(), columns.next()) + else { + return Err(PipelineError::Message(format!( + "could not read a diff-scope numstat record: {text:?}" + ))); }; - if let Some(diff) = diff { - metrics.lines_added += diff.lines_added; - metrics.lines_removed += diff.lines_removed; - metrics.hunks += diff.hunks; - } + files.push(ChangedFile { + path: path.to_string(), + status: statuses + .get(path) + .copied() + .unwrap_or(ChangeStatus::Modified), + lines_added: parse_count(added, &text)?, + lines_removed: parse_count(removed, &text)?, + }); } - Ok(metrics) + Ok(files) } -#[derive(Debug, PartialEq, Eq)] -enum FileContent { - Missing, - Regular(Vec), - Symlink(PathBuf), -} - -fn file_content(path: &Path) -> Result { - let metadata = match fs::symlink_metadata(path) { - Ok(metadata) => metadata, - Err(error) if error.kind() == std::io::ErrorKind::NotFound => { - return Ok(FileContent::Missing); - } - Err(error) => return Err(error.into()), - }; - if metadata.file_type().is_symlink() { - return Ok(FileContent::Symlink(fs::read_link(path)?)); - } - if metadata.is_file() { - return Ok(FileContent::Regular(fs::read(path)?)); +/// Git's status letter for a path. Renames and copies are off, and a tree diff +/// has no unmerged entries, so what remains beyond added and deleted is a change +/// to a path that existed before — a content edit, a mode change, or a swap +/// between a file and a symlink. +fn change_status(letter: &[u8]) -> ChangeStatus { + match letter.first() { + Some(b'A') => ChangeStatus::Added, + Some(b'D') => ChangeStatus::Deleted, + _ => ChangeStatus::Modified, } - Ok(FileContent::Missing) } -fn walk_files(root: &Path, excluded_roots: &[PathBuf]) -> Result, PipelineError> { - if !root.exists() { - return Ok(Vec::new()); +fn parse_count(field: &str, record: &str) -> Result { + if field == "-" { + return Ok(0); } - WalkDir::new(root) - .follow_links(false) - .into_iter() - .filter_entry(|entry| !is_excluded(entry, excluded_roots)) - .filter_map(|entry| match entry { - Ok(entry) if entry.file_type().is_file() || entry.file_type().is_symlink() => { - Some(Ok(entry.into_path())) - } - Ok(_) => None, - Err(error) => Some(Err(PipelineError::Message(format!( - "could not walk diff-scope path under {}: {error}", - root.display() - )))), - }) - .collect() -} - -fn is_excluded(entry: &DirEntry, excluded_roots: &[PathBuf]) -> bool { - excluded_roots - .iter() - .any(|excluded| entry.path().starts_with(excluded)) -} - -fn relative_key(root: &Path, path: &Path) -> Result { - let relative = path.strip_prefix(root).map_err(|_| { + field.parse().map_err(|_| { PipelineError::Message(format!( - "diff-scope path {} is outside {}", - path.display(), - root.display() + "could not read a diff-scope line count from {record:?}" )) - })?; - Ok(relative - .components() - .map(|component| component.as_os_str().to_string_lossy()) - .collect::>() - .join("/")) + }) } -#[cfg(test)] -mod tests { - use super::*; - use std::fs; - - #[test] - fn byte_line_diff_counts_changes_and_zero_context_hunks() { - let diff = diff_bytes(b"old\nsame\nbefore\n", b"new\nsame\nafter\n"); - assert_eq!(diff.lines_added, 2); - assert_eq!(diff.lines_removed, 2); - assert_eq!(diff.hunks, 2); - } - - #[test] - fn byte_line_diff_handles_empty_trailing_newline_and_non_utf8_inputs() { - assert_eq!(diff_bytes(b"", b""), FileDiff::default()); - - let trailing = diff_bytes(b"value", b"value\n"); - assert_eq!(trailing.lines_added, 1); - assert_eq!(trailing.lines_removed, 1); - assert_eq!(trailing.hunks, 1); - - let binary = diff_bytes(&[0xff, b'\n'], &[0xfe, b'\n']); - assert_eq!(binary.lines_added, 1); - assert_eq!(binary.lines_removed, 1); - assert_eq!(binary.hunks, 1); - } - - #[test] - fn lines_changed_saturates_untrusted_artifact_totals() { - let metrics = DiffScopeMetrics { - lines_added: u64::MAX, - lines_removed: 1, - ..DiffScopeMetrics::default() - }; - assert_eq!(metrics.lines_changed(), u64::MAX); - } - - #[test] - fn baseline_measurement_counts_all_task_changes_except_framework_outputs() { - let temp = tempfile::TempDir::new().unwrap(); - let eval_root = temp.path().join("env"); - let run_dir = temp.path().join("run"); - let outputs_dir = eval_root.join(".eval-magic-outputs/eval-e1/with_skill"); - fs::create_dir_all(eval_root.join("src")).unwrap(); - fs::create_dir_all(&outputs_dir).unwrap(); - fs::write(eval_root.join("src/changed.txt"), "old\nsame\n").unwrap(); - fs::write(eval_root.join("src/deleted.txt"), "gone\n").unwrap(); - fs::write(eval_root.join("framework.txt"), "before\n").unwrap(); - - capture_task_baseline(&eval_root, &run_dir).unwrap(); - - fs::write(eval_root.join("src/changed.txt"), "new\nsame\n").unwrap(); - fs::remove_file(eval_root.join("src/deleted.txt")).unwrap(); - fs::write(eval_root.join("framework.txt"), "after\n").unwrap(); - fs::write(eval_root.join("notes.txt"), "one\ntwo\n").unwrap(); - fs::write(outputs_dir.join("final-message.md"), "ignored\n").unwrap(); - fs::write( - eval_root.join(".eval-magic-outputs/agent-created.txt"), - "also ignored\n", - ) - .unwrap(); - - let metrics = measure_task_diff(&eval_root, &run_dir).unwrap(); - assert_eq!( - metrics, - DiffScopeMetrics { - files_touched: 4, - lines_added: 4, - lines_removed: 3, - hunks: 4, - } - ); - } - - #[test] - fn baseline_ignores_only_runner_owned_root_git_metadata() { - let temp = tempfile::TempDir::new().unwrap(); - let eval_root = temp.path().join("env"); - let run_dir = temp.path().join("run"); - fs::create_dir_all(eval_root.join(".git")).unwrap(); - fs::create_dir_all(eval_root.join("vendor/.git")).unwrap(); - fs::write(eval_root.join(".git/config"), "root-before\n").unwrap(); - fs::write(eval_root.join("vendor/.git/config"), "nested-before\n").unwrap(); - fs::write(eval_root.join("source.txt"), "before\n").unwrap(); - - capture_task_baseline(&eval_root, &run_dir).unwrap(); - - fs::write(eval_root.join(".git/config"), "root-after\n").unwrap(); - fs::write(eval_root.join("vendor/.git/config"), "nested-after\n").unwrap(); - fs::write(eval_root.join("source.txt"), "after\n").unwrap(); +/// Contiguous non-equal groups, with zero context: at `--unified=0` every `@@` +/// header is one such group. No content line can be mistaken for one — a diff +/// prefixes those with `+`, `-`, or a space. +fn count_hunks(patch: &[u8]) -> u64 { + patch + .split(|byte| *byte == b'\n') + .filter(|line| line.starts_with(b"@@")) + .count() as u64 +} - assert_eq!( - measure_task_diff(&eval_root, &run_dir).unwrap(), - DiffScopeMetrics { - files_touched: 2, - lines_added: 2, - lines_removed: 2, - hunks: 2, - } - ); +fn git_checked( + git: &IsolatedGit, + cwd: &Path, + args: &[&str], + env: &[(&str, &str)], +) -> Result, PipelineError> { + let output = git.run(cwd, args, env); + if output.status == Some(0) { + return Ok(output.stdout); } + Err(PipelineError::Message(format!( + "git {} failed in {}: {}", + args.join(" "), + cwd.display(), + String::from_utf8_lossy(&output.stderr).trim() + ))) } + +#[cfg(test)] +mod tests; diff --git a/src/pipeline/diff_scope/tests.rs b/src/pipeline/diff_scope/tests.rs new file mode 100644 index 0000000..adbf3ca --- /dev/null +++ b/src/pipeline/diff_scope/tests.rs @@ -0,0 +1,464 @@ +//! Measuring a task environment against its Git baseline. +//! +//! Every case here drives real `git`: the measurement is a claim about what +//! Git reports, and a stubbed one would only restate this module's own +//! assumptions. + +use super::*; +use std::fs; + +/// Invoke git in `root`, failing the test with git's own diagnostic. +fn git(isolated: &IsolatedGit, root: &Path, args: &[&str]) { + let output = isolated.run(root, args, &[]); + assert_eq!( + output.status, + Some(0), + "git {} failed: {}", + args.join(" "), + String::from_utf8_lossy(&output.stderr) + ); +} + +/// A task environment as `run` leaves one: a Git repository whose start +/// state is `refs/eval-magic/baseline`, with framework outputs excluded. +/// +/// Mirrors the steps of `initialize_task_repository` +/// (`src/cli/run/orchestrate/git.rs`) that a measurement actually depends +/// on, so each test below states only its own mutation. +fn baselined_repo(root: &Path) { + let isolated = IsolatedGit::new().expect("isolated Git configuration"); + let template = isolated.template_dir().to_string_lossy().into_owned(); + git( + &isolated, + root, + &[ + "init", + "--quiet", + "--initial-branch", + "work", + "--template", + &template, + ".", + ], + ); + fs::create_dir_all(root.join(".git/info")).unwrap(); + fs::write(root.join(".git/info/exclude"), "/.eval-magic-outputs/\n").unwrap(); + for (name, value) in [ + ("user.name", "eval-magic"), + ("user.email", "eval-magic@localhost"), + ("commit.gpgSign", "false"), + ] { + git(&isolated, root, &["config", "--local", name, value]); + } + git(&isolated, root, &["add", "--all", "--", "."]); + // What the runner places is forced in on top of the codebase's ignore + // rules, exactly as `runner_placed_paths` does. + if root.join(".claude").exists() { + git(&isolated, root, &["add", "--force", "--", ".claude"]); + } + git( + &isolated, + root, + &[ + "commit", + "--quiet", + "--allow-empty", + "--no-gpg-sign", + "--no-verify", + "-m", + "baseline", + ], + ); + git(&isolated, root, &["update-ref", BASELINE_REF, "HEAD"]); +} + +#[test] +fn lines_changed_saturates_untrusted_artifact_totals() { + let metrics = DiffScopeMetrics { + lines_added: u64::MAX, + lines_removed: 1, + ..DiffScopeMetrics::default() + }; + assert_eq!(metrics.lines_changed(), u64::MAX); +} + +#[test] +fn measurement_counts_all_task_changes_except_framework_outputs() { + let temp = tempfile::TempDir::new().unwrap(); + let eval_root = temp.path().join("env"); + let run_dir = temp.path().join("run"); + let outputs_dir = eval_root.join(".eval-magic-outputs/eval-e1/with_skill"); + fs::create_dir_all(&run_dir).unwrap(); + fs::create_dir_all(eval_root.join("src")).unwrap(); + fs::create_dir_all(&outputs_dir).unwrap(); + fs::write(eval_root.join("src/changed.txt"), "old\nsame\n").unwrap(); + fs::write(eval_root.join("src/deleted.txt"), "gone\n").unwrap(); + fs::write(eval_root.join("framework.txt"), "before\n").unwrap(); + + baselined_repo(&eval_root); + + fs::write(eval_root.join("src/changed.txt"), "new\nsame\n").unwrap(); + fs::remove_file(eval_root.join("src/deleted.txt")).unwrap(); + fs::write(eval_root.join("framework.txt"), "after\n").unwrap(); + fs::write(eval_root.join("notes.txt"), "one\ntwo\n").unwrap(); + fs::write(outputs_dir.join("final-message.md"), "ignored\n").unwrap(); + fs::write( + eval_root.join(".eval-magic-outputs/agent-created.txt"), + "also ignored\n", + ) + .unwrap(); + + let record = measure_task_diff(&eval_root, &run_dir).unwrap(); + assert_eq!( + record.metrics, + DiffScopeMetrics { + files_touched: 4, + lines_added: 4, + lines_removed: 3, + hunks: 4, + } + ); +} + +/// Git refuses to index any path with a `.git` component, so a nested +/// repository's internals are invisible to a measurement — not just the +/// runner-owned root `.git`. +#[test] +fn measurement_ignores_every_git_directory_not_just_the_runner_owned_root() { + let temp = tempfile::TempDir::new().unwrap(); + let eval_root = temp.path().join("env"); + let run_dir = temp.path().join("run"); + fs::create_dir_all(&run_dir).unwrap(); + fs::create_dir_all(eval_root.join("vendor/.git")).unwrap(); + fs::write(eval_root.join("vendor/.git/config"), "nested-before\n").unwrap(); + fs::write(eval_root.join("source.txt"), "before\n").unwrap(); + + baselined_repo(&eval_root); + + fs::write(eval_root.join(".git/config-probe"), "root-after\n").unwrap(); + fs::write(eval_root.join("vendor/.git/config"), "nested-after\n").unwrap(); + fs::write(eval_root.join("source.txt"), "after\n").unwrap(); + + assert_eq!( + measure_task_diff(&eval_root, &run_dir).unwrap().metrics, + DiffScopeMetrics { + files_touched: 1, + lines_added: 1, + lines_removed: 1, + hunks: 1, + } + ); +} + +/// An iteration holding one dispatched task against `eval_root`, complete +/// enough for `measure_iteration_diff_scopes` to reach the measurement. +fn iteration_with_one_task(iteration_dir: &Path, eval_root: &Path) -> std::path::PathBuf { + let run_dir = iteration_dir.join("eval-e1/with_skill"); + fs::create_dir_all(&run_dir).unwrap(); + let run_record_path = run_dir.join("run.json"); + fs::write(&run_record_path, "{}").unwrap(); + fs::write( + iteration_dir.join("dispatch.json"), + serde_json::json!({ + "tasks": [{ + "eval_id": "e1", + "condition": "with_skill", + "eval_root": eval_root.to_string_lossy(), + "run_record_path": run_record_path.to_string_lossy(), + }], + }) + .to_string(), + ) + .unwrap(); + run_dir +} + +#[test] +fn a_baselined_environment_is_measured_from_its_ref() { + let temp = tempfile::TempDir::new().unwrap(); + let eval_root = temp.path().join("env"); + let iteration_dir = temp.path().join("iteration-1"); + fs::create_dir_all(&eval_root).unwrap(); + fs::create_dir_all(&iteration_dir).unwrap(); + fs::write(eval_root.join("source.txt"), "before\n").unwrap(); + + baselined_repo(&eval_root); + let run_dir = iteration_with_one_task(&iteration_dir, &eval_root); + fs::write(eval_root.join("source.txt"), "after\n").unwrap(); + + let summary = measure_iteration_diff_scopes(&iteration_dir).unwrap(); + assert_eq!(summary.measured, 1, "{summary:?}"); + assert_eq!(summary.missing_baseline, 0, "{summary:?}"); + assert_eq!( + serde_json::from_str::( + &fs::read_to_string(run_dir.join(RESULT_FILE)).unwrap() + ) + .unwrap(), + DiffScopeMetrics { + files_touched: 1, + lines_added: 1, + lines_removed: 1, + hunks: 1, + } + ); +} + +/// A torn-down environment, or one from an iteration built before the +/// baseline ref existed, has nothing to measure against. That is a reported +/// gap, not a failure of the whole stage. +#[test] +fn an_environment_without_a_baseline_ref_is_reported_as_unmeasurable() { + let temp = tempfile::TempDir::new().unwrap(); + let eval_root = temp.path().join("env"); + let iteration_dir = temp.path().join("iteration-1"); + fs::create_dir_all(&eval_root).unwrap(); + fs::create_dir_all(&iteration_dir).unwrap(); + let run_dir = iteration_with_one_task(&iteration_dir, &eval_root); + + let summary = measure_iteration_diff_scopes(&iteration_dir).unwrap(); + assert_eq!(summary.missing_baseline, 1, "{summary:?}"); + assert_eq!(summary.measured, 0, "{summary:?}"); + assert!( + summary.warnings[0].contains("e1/with_skill"), + "{:?}", + summary.warnings + ); + assert!( + !run_dir.join(RESULT_FILE).exists(), + "an unmeasurable task must not freeze a result" + ); +} + +/// A changed environment yields a patch beside its metrics — the evidence a +/// judge needs, which the counters alone cannot carry. +#[test] +fn a_measurement_writes_the_patch_beside_its_metrics() { + let temp = tempfile::TempDir::new().unwrap(); + let eval_root = temp.path().join("env"); + let run_dir = temp.path().join("run"); + fs::create_dir_all(&eval_root).unwrap(); + fs::create_dir_all(&run_dir).unwrap(); + fs::write(eval_root.join("source.txt"), "before\n").unwrap(); + + baselined_repo(&eval_root); + fs::write(eval_root.join("source.txt"), "after\n").unwrap(); + + let record = measure_task_diff(&eval_root, &run_dir).unwrap(); + let patch = fs::read_to_string(run_dir.join(PATCH_FILE)).unwrap(); + assert!(patch.contains("--- a/source.txt"), "{patch}"); + assert!(patch.contains("-before"), "{patch}"); + assert!(patch.contains("+after"), "{patch}"); + assert!(!record.patch.truncated, "{record:?}"); + assert_eq!(record.patch.bytes, patch.len() as u64); + assert_eq!(record.patch.path, PATCH_FILE); +} + +/// An agent that changed nothing is a real, reportable outcome: zero +/// metrics and a patch that exists and is empty, never a missing artifact. +#[test] +fn a_run_with_no_changes_reports_zero_metrics_and_an_empty_patch() { + let temp = tempfile::TempDir::new().unwrap(); + let eval_root = temp.path().join("env"); + let run_dir = temp.path().join("run"); + fs::create_dir_all(&eval_root).unwrap(); + fs::create_dir_all(&run_dir).unwrap(); + fs::write(eval_root.join("source.txt"), "untouched\n").unwrap(); + + baselined_repo(&eval_root); + + let record = measure_task_diff(&eval_root, &run_dir).unwrap(); + assert_eq!(record.metrics, DiffScopeMetrics::default()); + assert_eq!(record.patch.bytes, 0); + assert!(!record.patch.truncated); + assert_eq!(fs::read_to_string(run_dir.join(PATCH_FILE)).unwrap(), ""); +} + +/// The counters say how much changed; this says what. A judge reading the +/// record can see the shape of the work before opening the patch. +#[test] +fn the_record_lists_every_changed_file_with_its_status() { + let temp = tempfile::TempDir::new().unwrap(); + let eval_root = temp.path().join("env"); + let run_dir = temp.path().join("run"); + fs::create_dir_all(&eval_root).unwrap(); + fs::create_dir_all(&run_dir).unwrap(); + fs::write(eval_root.join("kept.txt"), "steady\n").unwrap(); + fs::write(eval_root.join("changed.txt"), "old\n").unwrap(); + fs::write(eval_root.join("removed.txt"), "gone\n").unwrap(); + + baselined_repo(&eval_root); + + fs::write(eval_root.join("changed.txt"), "new\n").unwrap(); + fs::remove_file(eval_root.join("removed.txt")).unwrap(); + fs::write(eval_root.join("created.txt"), "fresh\nlines\n").unwrap(); + + let record = measure_task_diff(&eval_root, &run_dir).unwrap(); + assert_eq!( + record.files, + vec![ + ChangedFile { + path: "changed.txt".to_string(), + status: ChangeStatus::Modified, + lines_added: 1, + lines_removed: 1, + }, + ChangedFile { + path: "created.txt".to_string(), + status: ChangeStatus::Added, + lines_added: 2, + lines_removed: 0, + }, + ChangedFile { + path: "removed.txt".to_string(), + status: ChangeStatus::Deleted, + lines_added: 0, + lines_removed: 1, + }, + ] + ); +} + +#[test] +fn a_patch_within_the_cap_is_written_whole() { + let (kept, truncated) = truncate_patch(b"one\ntwo\n", 64); + assert_eq!(kept, b"one\ntwo\n"); + assert!(!truncated); +} + +#[test] +fn a_patch_past_the_cap_keeps_whole_lines_and_says_it_was_cut() { + let (kept, truncated) = truncate_patch(b"aaaa\nbbbb\ncccc\n", 12); + assert!(truncated); + let text = String::from_utf8(kept).unwrap(); + assert!(text.starts_with("aaaa\nbbbb\n"), "{text}"); + assert!(!text.contains("cccc"), "{text}"); + assert!(text.contains("truncated"), "{text}"); + assert!(text.ends_with('\n'), "{text}"); +} + +/// One line longer than the whole cap has no boundary to cut on. Capping +/// still wins — an uncapped artifact is the thing being prevented. +#[test] +fn a_patch_with_no_line_boundary_inside_the_cap_is_still_cut() { + let (kept, truncated) = truncate_patch(b"aaaaaaaaaaaaaaaaaaaa\n", 8); + assert!(truncated); + let text = String::from_utf8(kept).unwrap(); + assert!(text.starts_with("aaaaaaaa"), "{text}"); + assert!(text.contains("truncated"), "{text}"); +} + +/// Capping the evidence must not cap the measurement: the counters describe +/// the whole diff even when the patch beside them stops early. +#[test] +fn a_diff_past_the_cap_is_captured_truncated_while_the_metrics_stay_whole() { + let temp = tempfile::TempDir::new().unwrap(); + let eval_root = temp.path().join("env"); + let run_dir = temp.path().join("run"); + fs::create_dir_all(&eval_root).unwrap(); + fs::create_dir_all(&run_dir).unwrap(); + + baselined_repo(&eval_root); + + let lines = 200_000; + let bulk: String = (0..lines) + .map(|n| format!("generated line {n}\n")) + .collect(); + assert!( + bulk.len() > PATCH_BYTE_LIMIT, + "the fixture must exceed the cap" + ); + fs::write(eval_root.join("generated.txt"), &bulk).unwrap(); + + let record = measure_task_diff(&eval_root, &run_dir).unwrap(); + assert!(record.patch.truncated, "{:?}", record.patch); + assert_eq!(record.metrics.lines_added, lines); + assert_eq!(record.metrics.files_touched, 1); + + let patch = fs::read(run_dir.join(PATCH_FILE)).unwrap(); + assert_eq!(record.patch.bytes, patch.len() as u64); + assert!( + patch.len() < bulk.len(), + "a capped patch must be smaller than the diff it stands for" + ); + let text = String::from_utf8_lossy(&patch); + assert!( + text.trim_end().ends_with("is not captured"), + "{}", + &text[text.len() - 200..] + ); +} + +/// A real repository ignores its build output, and the baseline commit was +/// built under those same rules — so a run that compiles does not report +/// thousands of touched files. What the runner force-added is tracked +/// despite the rules, and stays measured. +#[test] +fn ignored_files_do_not_count_but_a_force_added_path_still_does() { + let temp = tempfile::TempDir::new().unwrap(); + let eval_root = temp.path().join("env"); + let run_dir = temp.path().join("run"); + fs::create_dir_all(eval_root.join(".claude/skills")).unwrap(); + fs::create_dir_all(eval_root.join("src")).unwrap(); + fs::create_dir_all(&run_dir).unwrap(); + fs::write(eval_root.join(".gitignore"), "/build/\n/.claude/\n").unwrap(); + fs::write(eval_root.join(".claude/skills/SKILL.md"), "staged\n").unwrap(); + fs::write(eval_root.join("src/main.rs"), "fn main() {}\n").unwrap(); + + baselined_repo(&eval_root); + + fs::create_dir_all(eval_root.join("build")).unwrap(); + fs::write(eval_root.join("build/out.o"), "compiled\n").unwrap(); + fs::write(eval_root.join(".claude/skills/SKILL.md"), "edited\n").unwrap(); + + let record = measure_task_diff(&eval_root, &run_dir).unwrap(); + assert_eq!( + record + .files + .iter() + .map(|file| file.path.as_str()) + .collect::>(), + vec![".claude/skills/SKILL.md"], + "{:?}", + record.files + ); + assert_eq!(record.metrics.files_touched, 1); +} + +/// Git detects renames by default, and would report one entry with no line +/// changes. A rename is two touched files — one created and one deleted — +/// which is what the metric has always meant. +#[test] +fn a_rename_counts_as_the_two_files_it_touches() { + let temp = tempfile::TempDir::new().unwrap(); + let eval_root = temp.path().join("env"); + let run_dir = temp.path().join("run"); + fs::create_dir_all(&eval_root).unwrap(); + fs::create_dir_all(&run_dir).unwrap(); + let body = "alpha\nbeta\ngamma\ndelta\n"; + fs::write(eval_root.join("original.txt"), body).unwrap(); + + baselined_repo(&eval_root); + + fs::remove_file(eval_root.join("original.txt")).unwrap(); + fs::write(eval_root.join("moved.txt"), body).unwrap(); + + let record = measure_task_diff(&eval_root, &run_dir).unwrap(); + assert_eq!( + record.files, + vec![ + ChangedFile { + path: "moved.txt".to_string(), + status: ChangeStatus::Added, + lines_added: 4, + lines_removed: 0, + }, + ChangedFile { + path: "original.txt".to_string(), + status: ChangeStatus::Deleted, + lines_added: 0, + lines_removed: 4, + }, + ] + ); + assert_eq!(record.metrics.files_touched, 2); +} diff --git a/src/pipeline/mod.rs b/src/pipeline/mod.rs index b457229..4df7f4c 100644 --- a/src/pipeline/mod.rs +++ b/src/pipeline/mod.rs @@ -27,7 +27,7 @@ pub use detect_stray_writes::{ detect_stray_writes_report, }; pub use diff_scope::{ - DiffScopeMetrics, DiffScopeSummary, capture_iteration_baselines, measure_iteration_diff_scopes, + DiffScopeMetrics, DiffScopeRecord, DiffScopeSummary, PatchRecord, measure_iteration_diff_scopes, }; pub use error::PipelineError; pub use fill_transcripts::{FillTranscriptsResult, fill_transcripts}; diff --git a/src/validation/schema.rs b/src/validation/schema.rs index fa45651..2a4d275 100644 --- a/src/validation/schema.rs +++ b/src/validation/schema.rs @@ -346,6 +346,36 @@ mod tests { validate_against_schema(SchemaName::DiffScope, &metrics, "diff-scope.json"); assert!(valid.is_ok(), "{valid:?}"); + // The changed-file list and the patch record are additive: a record + // carrying them validates, and one written before they existed still + // does, so an older iteration stays gradeable. + let mut complete = metrics.clone(); + complete["files"] = json!([ + { "path": "src/main.rs", "status": "modified", "lines_added": 4, "lines_removed": 1 } + ]); + complete["patch"] = json!({ "path": "diff.patch", "bytes": 512, "truncated": false }); + let valid: Result = + validate_against_schema(SchemaName::DiffScope, &complete, "diff-scope.json"); + assert!(valid.is_ok(), "{valid:?}"); + + let mut unknown_status = complete.clone(); + unknown_status["files"][0]["status"] = json!("renamed"); + let invalid: Result = + validate_against_schema(SchemaName::DiffScope, &unknown_status, "diff-scope.json"); + assert!( + invalid.is_err(), + "renames are off, so no record may claim one" + ); + + let mut partial_patch = complete; + partial_patch["patch"] = json!({ "path": "diff.patch" }); + let invalid: Result = + validate_against_schema(SchemaName::DiffScope, &partial_patch, "diff-scope.json"); + assert!( + invalid.is_err(), + "a patch record without `truncated` cannot say whether it is whole" + ); + let mut extra = metrics; extra["paths"] = json!(["src/main.rs"]); let invalid: Result = diff --git a/tests/cli/basics.rs b/tests/cli/basics.rs index 4d816d0..8168b7c 100644 --- a/tests/cli/basics.rs +++ b/tests/cli/basics.rs @@ -270,6 +270,19 @@ fn pipeline_help_documents_always_on_diff_scope_metrics() { } } +/// The patch is an artifact an operator has to be able to find, and the two +/// commands that produce it are the two that must name it. +#[test] +fn ingest_and_grade_help_document_the_captured_diff() { + for command in ["ingest", "grade"] { + skill_eval() + .args([command, "--help"]) + .assert() + .success() + .stdout(contains("diff.patch")); + } +} + #[test] fn finalize_and_aggregate_help_document_per_assertion_rollups() { for command in ["finalize", "aggregate"] { diff --git a/tests/cli/docs.rs b/tests/cli/docs.rs index 633c81f..f7c206d 100644 --- a/tests/cli/docs.rs +++ b/tests/cli/docs.rs @@ -164,8 +164,10 @@ fn docs_isolation_keeps_remedies_and_verification() { /// The codebase guide is the reference surface for a feature with no CLI flag, /// so the parts a config author cannot infer have to survive an edit: that a /// git ref is mandatory, that `files` layers over the checkout, that a local -/// path is not reproducible by anyone reading the results, and how the -/// per-iteration cache provisions environments. +/// path is not reproducible by anyone reading the results, how the +/// per-iteration cache provisions environments, and what the baseline ref is +/// measured into — including the `.gitignore` rule, which silently decides +/// whether a `diff_scope` threshold is reachable at all. #[test] fn docs_codebase_keeps_the_declaration_rules_caveat_and_provisioning_contract() { skill_eval() @@ -181,7 +183,10 @@ fn docs_codebase_keeps_the_declaration_rules_caveat_and_provisioning_contract() .stdout(contains("not reproducible")) .stdout(contains("materialized once")) .stdout(contains("hard-link")) - .stdout(contains("independent working tree")); + .stdout(contains("independent working tree")) + .stdout(contains("diff-scope.json")) + .stdout(contains("diff.patch")) + .stdout(contains(".gitignore")); } #[test] diff --git a/tests/run/diff_scope.rs b/tests/run/diff_scope.rs index 2da4501..64794a2 100644 --- a/tests/run/diff_scope.rs +++ b/tests/run/diff_scope.rs @@ -49,33 +49,52 @@ fn ingest_writes_diff_scope_for_every_run_without_an_assertion() { .assert() .success(); - let with = read_json( - &iteration_dir(&cwd) - .join("eval-edit/with_skill") - .join("diff-scope.json"), - ); + let with_dir = iteration_dir(&cwd).join("eval-edit/with_skill"); + let with = read_json(&with_dir.join("diff-scope.json")); + assert_eq!(with["files_touched"], 2); + assert_eq!(with["lines_added"], 2); + assert_eq!(with["lines_removed"], 1); + assert_eq!(with["hunks"], 2); assert_eq!( - with, - json!({ - "files_touched": 2, - "lines_added": 2, - "lines_removed": 1, - "hunks": 2 - }) + with["files"], + json!([ + { "path": "notes.txt", "status": "added", "lines_added": 1, "lines_removed": 0 }, + { "path": "source.txt", "status": "modified", "lines_added": 1, "lines_removed": 1 }, + ]) ); - let without = read_json( - &iteration_dir(&cwd) - .join("eval-edit/without_skill") - .join("diff-scope.json"), + assert_eq!(with["patch"]["path"], "diff.patch"); + assert_eq!(with["patch"]["truncated"], false); + let patch = read_str(&with_dir.join("diff.patch")); + assert!(patch.contains("-old"), "{patch}"); + assert!(patch.contains("+new"), "{patch}"); + assert!(patch.contains("+one"), "{patch}"); + assert!( + !patch.contains("artifact.txt"), + "framework outputs stay out of the patch: {patch}" ); + assert_eq!(with["patch"]["bytes"].as_u64().unwrap(), patch.len() as u64); + + let without_dir = iteration_dir(&cwd).join("eval-edit/without_skill"); + let without = read_json(&without_dir.join("diff-scope.json")); + assert_eq!(without["files_touched"], 0); + assert_eq!(without["lines_added"], 0); + assert_eq!(without["lines_removed"], 0); + assert_eq!(without["hunks"], 0); + assert_eq!(without["files"], json!([])); assert_eq!( - without, - json!({ - "files_touched": 0, - "lines_added": 0, - "lines_removed": 0, - "hunks": 0 - }) + read_str(&without_dir.join("diff.patch")), + "", + "a run that changed nothing still gets a patch, and it is empty" + ); + + // The copy tree Git replaced must not reappear anywhere under the iteration. + let copied: Vec<_> = walk_paths(&iteration_dir(&cwd)) + .into_iter() + .filter(|path| path.ends_with("diff-scope-baseline")) + .collect(); + assert!( + copied.is_empty(), + "no baseline is copied any more: {copied:?}" ); skill_eval() @@ -403,3 +422,73 @@ fn benchmark_diff_scope_is_ordered_by_eval_id_then_run_index() { ] ); } + +/// Mode B measures the same way Mode A does. Its conditions are two skill +/// revisions rather than skill-versus-none, but the evidence a run produces — +/// metrics and a patch per cell — must not depend on which mode produced it. +#[test] +fn revision_mode_measures_and_captures_the_diff_for_both_arms() { + let tmp = tempfile::TempDir::new().unwrap(); + let evals = r#"{ "skill_name": "mr-review", "evals": [ + { "id": "edit", "prompt": "fix source.txt", "expected_output": "fixed", + "skill_should_trigger": false, "files": ["source.txt"] } ] }"#; + let (skill_dir, cwd) = setup(tmp.path(), evals); + fs::write(skill_dir.join("mr-review/evals/source.txt"), "old\n").unwrap(); + + skill_eval() + .current_dir(&cwd) + .args(["snapshot", "--skill-dir"]) + .arg(&skill_dir) + .args(["--skill", "mr-review", "--label", "baseline"]) + .assert() + .success(); + skill_eval() + .current_dir(&cwd) + .args(["run", "--skill-dir"]) + .arg(&skill_dir) + .args(["--skill", "mr-review", "--mode", "revision", "--no-guard"]) + .assert() + .success(); + + let dispatch = read_json(&iteration_dir(&cwd).join("dispatch.json")); + for task in dispatch["tasks"].as_array().unwrap() { + let eval_root = Path::new(task["eval_root"].as_str().unwrap()); + let outputs_dir = Path::new(task["outputs_dir"].as_str().unwrap()); + fs::write(outputs_dir.join("final-message.md"), "done").unwrap(); + fs::write(eval_root.join("source.txt"), "new\n").unwrap(); + } + + skill_eval() + .current_dir(&cwd) + .args(["ingest", "--skill-dir"]) + .arg(&skill_dir) + .args([ + "--skill", + "mr-review", + "--iteration", + "1", + "--harness", + "claude-code", + ]) + .assert() + .success(); + + for condition in ["old_skill", "new_skill"] { + let cell = iteration_dir(&cwd).join("eval-edit").join(condition); + let measured = read_json(&cell.join("diff-scope.json")); + assert_eq!(measured["files_touched"], 1, "{condition}"); + assert_eq!(measured["lines_added"], 1, "{condition}"); + assert_eq!(measured["lines_removed"], 1, "{condition}"); + assert_eq!(measured["files"][0]["path"], "source.txt", "{condition}"); + let patch = read_str(&cell.join("diff.patch")); + assert!(patch.contains("-old"), "{condition}: {patch}"); + assert!(patch.contains("+new"), "{condition}: {patch}"); + } + + // The codebase and skill provenance #244 requires must survive the change. + let conditions = read_json(&iteration_dir(&cwd).join("conditions.json")); + assert!( + conditions["skill_source"].is_object(), + "the skill source must still reach conditions.json: {conditions}" + ); +} diff --git a/tests/run/env_layout.rs b/tests/run/env_layout.rs index 400eb10..410fda9 100644 --- a/tests/run/env_layout.rs +++ b/tests/run/env_layout.rs @@ -201,7 +201,7 @@ fn dispatch_tasks_grouped_by_condition() { } #[test] -fn every_dispatch_has_a_private_env_and_post_guard_diff_baseline() { +fn every_dispatch_has_a_private_env_and_a_post_guard_baseline_ref() { let tmp = tempfile::TempDir::new().unwrap(); let evals = r#"{ "skill_name": "mr-review", "evals": [ { "id": "e1", "prompt": "review", "expected_output": "a review" }, @@ -229,20 +229,13 @@ fn every_dispatch_has_a_private_env_and_post_guard_diff_baseline() { ); for task in tasks { - let run_dir = Path::new(task["run_record_path"].as_str().unwrap()) - .parent() - .unwrap(); - let manifest = read_json(&run_dir.join("diff-scope-baseline/manifest.json")); + let eval_root = Path::new(task["eval_root"].as_str().unwrap()); + let tracked = git_stdout(eval_root, &["ls-tree", "-r", "--name-only", BASELINE_REF]); assert!( - manifest["preexisting_files"] - .as_array() - .unwrap() - .iter() - .any(|path| path - .as_str() - .unwrap() - .ends_with(".slow-powers-eval-guard.json")), - "baseline must be captured after guard installation: {manifest}" + tracked + .lines() + .any(|path| path.ends_with(".slow-powers-eval-guard.json")), + "the baseline ref must be written after guard installation: {tracked}" ); } } diff --git a/tests/run/helpers.rs b/tests/run/helpers.rs index 904b683..801fd27 100644 --- a/tests/run/helpers.rs +++ b/tests/run/helpers.rs @@ -102,10 +102,51 @@ pub fn resolved(path: &Path) -> PathBuf { } } +/// The ref a task environment carries at the state the agent started from. +/// Mirrors `eval_magic::core::BASELINE_REF` for the integration tests, which +/// observe the environment through git rather than through the library. +pub const BASELINE_REF: &str = "refs/eval-magic/baseline"; + +/// Ask git about a task environment, as an operator inspecting one would. +/// Panics with git's own diagnostic, so a broken environment names itself. +pub fn git_stdout(root: &Path, args: &[&str]) -> String { + let output = std::process::Command::new("git") + .args(args) + .current_dir(root) + .output() + .unwrap_or_else(|error| panic!("git {} could not start: {error}", args.join(" "))); + assert!( + output.status.success(), + "git {} failed in {}: {}", + args.join(" "), + root.display(), + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout).into_owned() +} + pub fn read_json(path: &Path) -> Value { serde_json::from_str(&fs::read_to_string(path).unwrap()).unwrap() } +/// Every path under `root`, directories included. For asserting that +/// something is absent from an artifact tree, where a targeted `exists()` check +/// would only cover the one place it was expected. +pub fn walk_paths(root: &Path) -> Vec { + let mut found = Vec::new(); + let Ok(entries) = fs::read_dir(root) else { + return found; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + found.extend(walk_paths(&path)); + } + found.push(path); + } + found +} + pub fn read_str(path: &Path) -> String { fs::read_to_string(path).unwrap() }