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/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/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/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/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/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}: {}", 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() }