diff --git a/docs/guides/codebase.md b/docs/guides/codebase.md index b71801d..e59ff5f 100644 --- a/docs/guides/codebase.md +++ b/docs/guides/codebase.md @@ -277,6 +277,11 @@ What counts is what Git counts, under the same rules the baseline commit was bui - Overlay files 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. +- The task-local scratch directory never counts. Dispatch prompts designate `/tmp/` for + temporary work and tell the agent to use it, so what lands there is the framework's instruction + being followed, not the change under measurement. It is excluded from the project's own ignore + files for the same reason, and it never reaches `diff.patch`, so a judge never reads scratch notes + as the deliverable. - 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. diff --git a/src/adapters/descriptor_adapter.rs b/src/adapters/descriptor_adapter.rs index ffff3ea..aa80ba8 100644 --- a/src/adapters/descriptor_adapter.rs +++ b/src/adapters/descriptor_adapter.rs @@ -127,7 +127,7 @@ impl HarnessAdapter for DescriptorAdapter { } fn framework_ignore_paths(&self) -> Vec { - let mut paths = vec![format!("/{}/", crate::sandbox::GUARD_DENIALS_DIR)]; + let mut paths = crate::sandbox::framework_owned_entries().to_vec(); if let Some(skills_dir) = &self.descriptor.skills_dir { paths.push(format!("/{skills_dir}/")); } diff --git a/src/adapters/harness.rs b/src/adapters/harness.rs index b5241f5..2f1b18a 100644 --- a/src/adapters/harness.rs +++ b/src/adapters/harness.rs @@ -81,17 +81,18 @@ pub trait HarnessAdapter { self.skills_dir(repo_root).into_iter().collect() } - /// Env-relative paths the runner itself places in every task environment, - /// as gitignore-style patterns. + /// Env-relative paths the framework owns in every task environment, as + /// gitignore-style patterns. /// /// Staged skills sit *inside* the task repository, so a codebase whose lint /// or format step globs the whole tree reports the framework's artifacts as /// project failures — and only in the arm that stages a skill. `run` writes /// these patterns into the project's own ignore files /// ([`crate::workspace::tool_ignore`]) to keep that from happening. The - /// baseline is the framework outputs dir every harness produces. + /// baseline is [`crate::sandbox::framework_owned_entries`], which every + /// harness contributes; what a harness adds on top is what it stages. fn framework_ignore_paths(&self) -> Vec { - vec![format!("/{}/", crate::sandbox::GUARD_DENIALS_DIR)] + crate::sandbox::framework_owned_entries().to_vec() } // ── Run-option capabilities (defaulted) ────────────────────────────────── @@ -564,11 +565,12 @@ mod tests { } #[test] - fn framework_ignore_paths_cover_the_staged_skills_the_guard_file_and_the_outputs_dir() { + fn framework_ignore_paths_cover_the_staged_skills_the_guard_file_and_what_the_framework_owns() { assert_eq!( adapter_for(Harness::resolve("claude-code").unwrap()).framework_ignore_paths(), vec![ "/.eval-magic-outputs/".to_string(), + "/tmp/".to_string(), "/.claude/skills/".to_string(), "/.claude/settings.local.json".to_string(), ] @@ -578,6 +580,7 @@ mod tests { adapter_for(Harness::resolve("opencode").unwrap()).framework_ignore_paths(), vec![ "/.eval-magic-outputs/".to_string(), + "/tmp/".to_string(), "/.opencode/skills/".to_string(), "/.opencode/plugins/slow-powers-eval-guard.js".to_string(), ] @@ -586,6 +589,7 @@ mod tests { adapter_for(Harness::resolve("codex").unwrap()).framework_ignore_paths(), vec![ "/.eval-magic-outputs/".to_string(), + "/tmp/".to_string(), "/.agents/skills/".to_string(), "/.codex/hooks.json".to_string(), ] @@ -602,7 +606,7 @@ mod tests { assert_eq!( adapter.framework_ignore_paths(), - vec!["/.eval-magic-outputs/".to_string()] + vec!["/.eval-magic-outputs/".to_string(), "/tmp/".to_string()] ); } diff --git a/src/cli/args.rs b/src/cli/args.rs index db541c9..5b2726f 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -386,7 +386,11 @@ pub struct RunArgs { /// while dispatches run. The task env is its sole allowed write root; host temp /// directories are out of bounds. Dispatch prompts name `/tmp` as /// the task-local scratch directory (create it when needed); eval-magic does - /// not rewrite `TMPDIR`, `TMP`, or `TEMP`. + /// not rewrite `TMPDIR`, `TMP`, or `TEMP`. Because the framework designates + /// that directory, what an agent puts there is excluded from diff scope and + /// from the project's own ignore files — so a `diff_scope` budget covers the + /// change, not the scratch work, and judges never read scratch notes as the + /// deliverable. /// Because the harness already cwd-bounds the agent's direct file tools to the /// env, the guard's main remaining value is blocking Bash-subprocess escapes the /// cwd boundary doesn't cover and acting as a backstop when the isolated session @@ -652,8 +656,14 @@ pub(crate) enum Commands { /// Disarm the write guard. /// /// Removes only the write guard (e.g. mid-run, before hand-editing files the - /// guard would block). The full `teardown` removes the guard AND the staged - /// skill set. + /// guard would block) — at the invocation cwd, and in every + /// per-`(group, condition)` env of the iteration the shared target flags + /// select (`--skill-dir`/`--skill`, `--workspace-dir`, `--iteration`; + /// `--iteration` defaults to the latest). Running it from inside a task env + /// needs no flags: that env is the cwd. Where those flags resolve no run, it + /// sweeps the cwd alone and says which guards it could not check, rather + /// than reporting an all-clear for them. The full `teardown` removes the + /// guard AND the staged skill set, and reclaims the workspace. TeardownGuard(CommonArgs), /// Ingest recorded transcripts into run records. /// diff --git a/src/cli/commands/guard.rs b/src/cli/commands/guard.rs index 9ef73bf..7e77595 100644 --- a/src/cli/commands/guard.rs +++ b/src/cli/commands/guard.rs @@ -11,6 +11,8 @@ use std::io; use std::path::PathBuf; use crate::adapters::{HarnessAdapter, adapter_for}; +use crate::cli::args::CommonArgs; +use crate::cli::{iteration_dir, resolve_iteration, run_context_from, staged_env_roots}; use crate::core::Harness; use crate::sandbox; @@ -53,19 +55,83 @@ pub(crate) fn run_guard_hook(harness_name: &str, marker: Option) -> anyh Ok(()) } -/// Disarm the write guard for the current directory. Cwd-only by design: the -/// guard lives under harness-local config in the current repo, so this needs no -/// `--skill-dir`/`--skill` flags. -pub(crate) fn run_teardown_guard() -> anyhow::Result<()> { - let torn = sandbox::teardown_guard(&std::env::current_dir()?); - println!( - "{}", - if torn { - "🛡 Write guard removed." - } else { - "No write guard was installed — nothing to remove." +/// Disarm the write guard: at the invocation cwd, and — when the shared target +/// flags resolve a run — in every per-`(group, condition)` env of the selected +/// iteration. +/// +/// The cwd sweep needs no flags, so disarming from inside a task env still works +/// bare. The env walk is best-effort, because the guard is only *usually* +/// reachable from where the operator stands: a cwd that resolves no skill, or a +/// skill with no such iteration, leaves those guards armed. That case reports +/// what it could not check rather than the all-clear it never established — this +/// command exists for mid-run hand-editing, which is exactly when a false +/// "nothing to remove" costs the most (#298). +/// +/// Guard-only by design: the staged skill set and the workspace are what full +/// `teardown` additionally removes. +pub(crate) fn run_teardown_guard(args: CommonArgs) -> anyhow::Result<()> { + // Cwd first. When the cwd *is* a task env, its guard is already gone by the + // time the walk below reaches it, so no guard is counted in both scopes. + let cwd_torn = sandbox::teardown_guard(&std::env::current_dir()?); + + let mut envs_torn = 0usize; + let mut checked: Option<(u32, usize)> = None; + let mut unchecked: Option = None; + match run_context_from(&args).and_then(|ctx| { + let iteration = resolve_iteration(&ctx, args.iteration)?; + let dir = iteration_dir(&ctx, Some(iteration))?; + Ok((iteration, staged_env_roots(&dir))) + }) { + Ok((iteration, envs)) => { + for env in &envs { + if sandbox::teardown_guard(env) { + envs_torn += 1; + } + } + checked = Some((iteration, envs.len())); } - ); + Err(error) => unchecked = Some(error.to_string()), + } + + // Takes the iteration rather than reading `checked`: naming a count of envs + // without the iteration they belong to is meaningless, and there is no such + // thing as a sensible default for it. + let envs_phrase = |iteration: u32, count: usize| { + format!( + "{count} task env{} in iteration {iteration}", + if count == 1 { "" } else { "s" } + ) + }; + let mut removed = Vec::new(); + if cwd_torn { + removed.push("the invocation cwd".to_string()); + } + if let Some((iteration, _)) = checked + && envs_torn > 0 + { + removed.push(envs_phrase(iteration, envs_torn)); + } + if removed.is_empty() { + let mut scopes = vec!["the invocation cwd".to_string()]; + if let Some((iteration, count)) = checked { + scopes.push(envs_phrase(iteration, count)); + } + println!( + "No write guard was installed — nothing to remove (checked {}).", + scopes.join(" and ") + ); + } else { + println!("🛡 Write guard removed: {}.", removed.join(", ")); + } + if let Some(reason) = unchecked { + // The resolution error already names the flags that would have resolved + // a run, so this adds only what it cannot know: that guards may survive, + // and that full `teardown` is the other way to reach them. + eprintln!( + "⚠ Task env guards were not checked, so any that were armed still are: \ + {reason}.\n Add the run's target flags, or run `eval-magic teardown`." + ); + } Ok(()) } diff --git a/src/cli/commands/pipeline.rs b/src/cli/commands/pipeline.rs index a46a777..16abb3e 100644 --- a/src/cli/commands/pipeline.rs +++ b/src/cli/commands/pipeline.rs @@ -123,8 +123,9 @@ pub(crate) fn run_finalize(args: CommonArgs) -> anyhow::Result<()> { "\n✅ Finalize complete. Read the benchmark above, then tear down: eval-magic teardown{target_args}" ); // Warn if a guard is still armed. There is one env per (group, condition), so - // walk each per-env marker as well as the cwd. `teardown` (not the cwd-only - // `teardown-guard`) is what disarms them all. + // walk each per-env marker as well as the cwd. The reminder names `teardown` + // rather than `teardown-guard`: both disarm every one of these, but at end of + // run the staged skill set and the workspace want reclaiming too. let mut armed = sandbox::guard_is_armed(&ctx.stage_root); if !armed && let Ok(dir) = iteration_dir(&ctx, Some(iteration)) { armed = staged_env_roots(&dir) diff --git a/src/cli/commands/workspace.rs b/src/cli/commands/workspace.rs index 43ba70b..db53600 100644 --- a/src/cli/commands/workspace.rs +++ b/src/cli/commands/workspace.rs @@ -3,7 +3,6 @@ use std::collections::HashSet; use std::fs; -use std::path::Path; use crate::adapters::adapter_for; use crate::cli::args::{CommonArgs, PromoteBaselineArgs, SnapshotArgs}; @@ -11,6 +10,7 @@ use crate::cli::{ command_target_args, iteration_dir, resolve_iteration, run_context_from, staged_env_roots, }; use crate::core::SkillNames; +use crate::core::fs::artifact_path; use crate::sandbox; use crate::workspace; @@ -183,7 +183,7 @@ pub(crate) fn run_teardown(args: CommonArgs) -> anyhow::Result<()> { eprintln!( "⚠ Kept {} workspace iteration(s) with results not yet committed:\n{lines}\n Commit them, e.g.:\n eval-magic promote-baseline{target_args} --iteration \n or delete {}/ manually to discard.", ws.kept_iterations.len(), - Path::new(".eval-magic").join(&ctx.skill_name).display() + artifact_path(&ctx.workspace_root.join(&ctx.skill_name)) ); } Ok(()) diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 0fa5fd7..289f69d 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -114,7 +114,7 @@ fn dispatch(command: Option, harness_file: Option<&str>) -> anyhow::Re Commands::Finalize(args) => run_finalize(args), Commands::Init(args) => run_init(args), Commands::Validate(args) => run_validate(args), - Commands::TeardownGuard(_) => run_teardown_guard(), + Commands::TeardownGuard(args) => run_teardown_guard(args), Commands::Guard { marker } => run_guard(marker), Commands::GuardCodex { marker } => run_guard_codex(marker), Commands::GuardHook { harness, marker } => run_guard_hook(&harness, marker), diff --git a/src/cli/run/orchestrate/git.rs b/src/cli/run/orchestrate/git.rs index d33dfa5..4c2840e 100644 --- a/src/cli/run/orchestrate/git.rs +++ b/src/cli/run/orchestrate/git.rs @@ -109,8 +109,10 @@ fn initialize_task_repository(plan: &TaskRepository) -> Result<(), String> { .map_err(|error| format!("could not create Git info directory: {error}"))?; fs::create_dir_all(&hooks_dir) .map_err(|error| format!("could not create empty Git hooks directory: {error}"))?; - fs::write(root.join(".git/info/exclude"), "/.eval-magic-outputs/\n") - .map_err(|error| format!("could not configure framework output exclusion: {error}"))?; + let mut exclude = crate::sandbox::framework_owned_entries().join("\n"); + exclude.push('\n'); + fs::write(root.join(".git/info/exclude"), exclude) + .map_err(|error| format!("could not configure framework path exclusion: {error}"))?; let hooks_path = hooks_dir.to_string_lossy().into_owned(); for (name, value) in [ @@ -127,9 +129,9 @@ fn initialize_task_repository(plan: &TaskRepository) -> Result<(), String> { // its build output, and a forced add here would commit `target/` or // `node_modules/` into the baseline every environment starts from. // - // 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. + // No exclude pathspec for the framework-owned paths: `.git/info/exclude` + // above already ignores them, and an unforced add honors that. The pathspecs + // this replaces existed only to carve them back out of a forced add. 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 diff --git a/src/pipeline/diff_scope.rs b/src/pipeline/diff_scope.rs index 3ff01bb..8aeca65 100644 --- a/src/pipeline/diff_scope.rs +++ b/src/pipeline/diff_scope.rs @@ -4,8 +4,9 @@ //! 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. +//! codebase's own `.gitignore` and the runner's exclusion of what the framework +//! itself owns (its outputs dir and the task-local scratch directory dispatch +//! prompts designate — see [`crate::sandbox::framework_owned_entries`]). use std::collections::HashMap; use std::fs; @@ -246,9 +247,9 @@ fn measure_task_diff(eval_root: &Path, run_dir: &Path) -> Result [String; 2] { + [ + format!("/{GUARD_DENIALS_DIR}/"), + format!("/{TASK_SCRATCH_DIR}/"), + ] +} + /// Current wall clock in epoch milliseconds. chrono ships without its `clock` /// feature (it parses timestamps but never reads the clock), so the time comes /// from `std::time`. Shared by the guard's expiry check and marker stamping. diff --git a/tests/cli/guard.rs b/tests/cli/guard.rs index 79921f7..c478a80 100644 --- a/tests/cli/guard.rs +++ b/tests/cli/guard.rs @@ -479,3 +479,20 @@ fn guard_hook_skips_descriptor_discovery() { .stdout(contains(r#""permissionDecision":"deny""#)) .stderr(contains("skipping harness descriptor").not()); } + +/// A `teardown-guard` that cannot resolve a run must not imply it checked the +/// per-`(group, condition)` env guards. The false all-clear it used to print is +/// most costly exactly here — mid-run, before hand-editing files (#298). +#[test] +fn teardown_guard_says_when_it_could_not_check_the_env_guards() { + let tmp = TempDir::new().unwrap(); + + skill_eval() + .arg("teardown-guard") + .current_dir(tmp.path()) + .assert() + .success() + .stdout(contains("invocation cwd")) + .stderr(contains("Task env guards were not checked")) + .stderr(contains("eval-magic teardown")); +} diff --git a/tests/cli/workspace.rs b/tests/cli/workspace.rs index 27bb77c..45aa816 100644 --- a/tests/cli/workspace.rs +++ b/tests/cli/workspace.rs @@ -498,3 +498,37 @@ fn teardown_reclaims_promoted_and_keeps_uncommitted() { assert!(!promoted.exists()); assert!(kept.exists()); } + +/// `teardown`: the discard hint names the workspace the run actually used. +/// +/// The eval home moved out of the skill repo, but the hint kept naming the old +/// `.eval-magic//` default — a path that no longer exists, printed beside +/// a `promote-baseline` command carrying the correct absolute `--workspace-dir` +/// (#298). Only a workspace away from the legacy default can catch it. +#[test] +fn teardown_discard_hint_names_the_workspace_the_run_used() { + let (_tmp, root) = canonical_root(); + let (skill_dir, _skill_sub) = write_skill_md(&root, "---\nname: mr-review\n---\nbody\n"); + + let cwd = root.join("work"); + let workspace = root.join("eval-home"); + let kept = workspace.join("mr-review").join("iteration-1"); + fs::create_dir_all(&cwd).unwrap(); + fs::create_dir_all(&kept).unwrap(); + fs::write(kept.join("benchmark.json"), "{}").unwrap(); + + skill_eval() + .current_dir(&cwd) + .args(["teardown", "--skill-dir"]) + .arg(&skill_dir) + .args(["--skill", "mr-review", "--workspace-dir"]) + .arg(&workspace) + .assert() + .success() + .stderr(contains(format!( + "delete {}/ manually", + workspace.join("mr-review").display() + ))); + + assert!(kept.exists()); +} diff --git a/tests/run/git_isolation.rs b/tests/run/git_isolation.rs index 5d8b777..6e4800e 100644 --- a/tests/run/git_isolation.rs +++ b/tests/run/git_isolation.rs @@ -77,13 +77,15 @@ fn every_task_is_a_clean_local_git_repo_inside_a_dirty_ignored_parent_repo() { assert_eq!(git(eval_root, &["symbolic-ref", "--short", "HEAD"]), "work"); assert_eq!(git(eval_root, &["status", "--porcelain"]), ""); assert_eq!(git(eval_root, &["remote"]), ""); - assert_eq!( - git( - eval_root, - &["check-ignore", ".eval-magic-outputs/probe.txt"] - ), - ".eval-magic-outputs/probe.txt" - ); + // Both framework-owned directories: the outputs dir, and the task-local + // scratch directory dispatch prompts designate (#298). Neither is the + // agent's change, so neither may reach a measurement. + for framework_owned in [".eval-magic-outputs/probe.txt", "tmp/probe.txt"] { + assert_eq!( + git(eval_root, &["check-ignore", framework_owned]), + framework_owned + ); + } // Git spells a zero UTC offset either `+00:00` (2.43) or `Z` (newer). // Both name the same instant, so normalize rather than pin a version. let log = git( diff --git a/tests/run/ignore_files.rs b/tests/run/ignore_files.rs index b3d0336..a671097 100644 --- a/tests/run/ignore_files.rs +++ b/tests/run/ignore_files.rs @@ -61,6 +61,7 @@ fn both_arms_get_the_same_ignore_file_hiding_the_staged_skills() { ); for entry in [ "/.eval-magic-outputs/", + "/tmp/", "/.claude/skills/", "/.claude/settings.local.json", ] { diff --git a/tests/run/lifecycle.rs b/tests/run/lifecycle.rs index 6107923..11d808e 100644 --- a/tests/run/lifecycle.rs +++ b/tests/run/lifecycle.rs @@ -56,19 +56,32 @@ fn guard_installs_pretooluse_hook_under_env() { // Nothing is armed at the invocation cwd anymore. assert!(!cwd.join(".claude/settings.local.json").exists()); - // `teardown-guard` operates at the invocation cwd, so it does not reach the - // env-scoped guard: this is a transitional no-op, reconciled when the loop runs - // inside the env session / teardown is reworked. The env is disposable - // and the guard auto-expires (6h TTL); full `teardown` reclaims it (see - // `teardown_reclaims_workspace_and_env_guard`). + // `teardown-guard` reaches the per-(group, condition) env guards when the shared + // target flags point it at the run — the mid-run "disarm before I hand-edit" + // path the command exists for. It stays guard-only: the staged skill set and + // the workspace survive, which is what separates it from full `teardown`. skill_eval() .current_dir(&cwd) .args(["teardown-guard", "--skill-dir"]) .arg(&skill_dir) .args(["--skill", "mr-review"]) .assert() - .success(); - assert!(settings.exists(), "env guard survives a cwd teardown-guard"); + .success() + .stdout(contains("Write guard removed")) + .stdout(contains("task env")); + assert!(!settings.exists(), "env guard survived teardown-guard"); + assert!( + !cli_env_dir(&cwd, "g1", "with_skill") + .join(".claude/skills/.slow-powers-eval-guard.json") + .exists(), + "env guard marker survived teardown-guard" + ); + assert!( + cli_env_dir(&cwd, "g1", "with_skill") + .join(".claude/skills") + .exists(), + "teardown-guard removed the staged skill set — that is `teardown`'s job" + ); } #[test]