From d52e09d03a243d0c301a424a08c28d691918df11 Mon Sep 17 00:00:00 2001 From: samiamorwas Date: Tue, 1 Sep 2026 20:38:46 -0400 Subject: [PATCH 1/4] fix(guard): reach every env guard from teardown-guard (#298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `teardown-guard` swept only the invocation cwd, but the write guard now arms inside each per-(group, condition) task env. Run mid-campaign it printed "No write guard was installed ā€” nothing to remove" while both envs held live guards ā€” the most costly moment for a false all-clear, since the command exists for hand-editing files the guard would block. It already accepted `CommonArgs`; the dispatch discarded them. Thread them through and walk the iteration's staged envs the way `teardown` and the `finalize` reminder already do, still without touching the staged skill set or the workspace. Where those flags resolve no run, the sweep now names the scopes it actually checked and warns that the env guards were not among them. Before: $ eval-magic teardown-guard --iteration 1 No write guard was installed ā€” nothing to remove. After: $ eval-magic teardown-guard --skill demo --workspace-dir ws --iteration 1 šŸ›” Write guard removed: 2 task envs in iteration 1. $ eval-magic teardown-guard # from an unrelated cwd No write guard was installed ā€” nothing to remove (checked the invocation cwd). āš  Task env guards were not checked, so any that were armed still are: ā€¦ Add the run's target flags, or run `eval-magic teardown`. Verified: cargo test, cargo clippy --all-targets -- -D warnings, cargo fmt --check, plus a real run ā†’ teardown-guard against a scaffolded skill. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NJjf8XMm1e1XtRZaGnkmvr --- src/cli/args.rs | 10 ++++- src/cli/commands/guard.rs | 86 +++++++++++++++++++++++++++++++----- src/cli/commands/pipeline.rs | 5 ++- src/cli/mod.rs | 2 +- tests/cli/guard.rs | 17 +++++++ tests/run/lifecycle.rs | 27 ++++++++--- 6 files changed, 123 insertions(+), 24 deletions(-) diff --git a/src/cli/args.rs b/src/cli/args.rs index db541c9..6aca364 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -652,8 +652,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..dabcbf8 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,79 @@ 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()), + } + + let envs_phrase = |count: usize| { + let iteration = checked.map(|(iteration, _)| iteration).unwrap_or_default(); + 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 envs_torn > 0 { + removed.push(envs_phrase(envs_torn)); + } + if removed.is_empty() { + let mut scopes = vec!["the invocation cwd".to_string()]; + if let Some((_, count)) = checked { + scopes.push(envs_phrase(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/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/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/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] From bc883cea1da918b416db9027c03809fbf5a8b8fc Mon Sep 17 00:00:00 2001 From: samiamorwas Date: Tue, 1 Sep 2026 20:41:34 -0400 Subject: [PATCH 2/4] fix(run): keep the task-local scratch directory out of diff scope (#298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every dispatch prompt designates `/tmp` for temporary work and tells the agent to use it. An agent that complied then had everything it put there measured as part of its change: in one prerelease run `tmp/` accounted for 3 of 11 files touched and 228 of 296 lines added. That trips `diff_scope` budgets on throwaway notes, puts scratch files in the `diff.patch` a judge reads as the deliverable, and does so asymmetrically ā€” only in the arm that happened to use the directory it was told to use. `.eval-magic-outputs/` already never counts, for the same reason. The scratch directory now shares that treatment, on both surfaces: the env's `.git/info/exclude` and each harness's `framework_ignore_paths`. Both were spelling the outputs entry separately, and the diff-scope test fixture spelled it a third time ā€” so the fixture could not fail when the rule changed. `sandbox::framework_owned_entries` is now the one definition all three read. Only files created under `tmp/` are affected: gitignore rules never apply to tracked paths, so a codebase that genuinely tracks a `tmp/` directory keeps its files measured. Verified: cargo test, cargo clippy --all-targets -- -D warnings, cargo fmt --check, plus a real `run` ā€” a scratch file under `tmp/` is invisible to `git status` in the staged env while the agent's own file still shows, and both arms get the same `.prettierignore` block. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NJjf8XMm1e1XtRZaGnkmvr --- docs/guides/codebase.md | 5 +++++ src/adapters/descriptor_adapter.rs | 2 +- src/adapters/harness.rs | 16 ++++++++++------ src/cli/args.rs | 6 +++++- src/cli/run/orchestrate/git.rs | 10 ++++++---- src/pipeline/diff_scope.rs | 11 ++++++----- src/pipeline/diff_scope/tests.rs | 15 +++++++++++++-- src/sandbox/mod.rs | 15 +++++++++++++++ tests/run/git_isolation.rs | 16 +++++++++------- tests/run/ignore_files.rs | 1 + 10 files changed, 71 insertions(+), 26 deletions(-) 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 6aca364..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 diff --git a/src/cli/run/orchestrate/git.rs b/src/cli/run/orchestrate/git.rs index d33dfa5..719b7f4 100644 --- a/src/cli/run/orchestrate/git.rs +++ b/src/cli/run/orchestrate/git.rs @@ -109,7 +109,9 @@ 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") + 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 output exclusion: {error}"))?; let hooks_path = hooks_dir.to_string_lossy().into_owned(); @@ -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/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", ] { From acd3b6581c850f836fa2c148e8587c8d6b61276f Mon Sep 17 00:00:00 2001 From: samiamorwas Date: Tue, 1 Sep 2026 20:42:22 -0400 Subject: [PATCH 3/4] fix(cli): name the real workspace path in teardown's discard hint (#298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Teardown's kept-iteration warning offered `.eval-magic//` as the directory to delete. That was right before the eval home moved out of the skill repo; since then no such path exists, and the hint printed it one line below a `promote-baseline` command carrying the correct absolute `--workspace-dir`. `ctx.workspace_root` was already in scope and already rendered correctly by `command_target_args` in the same message. Before: eval-magic promote-baseline ā€¦ --workspace-dir /home/u/.local/share/eval-magic/skills-c61a1930 ā€¦ or delete .eval-magic/working-with-tdd/ manually to discard. After: or delete /home/u/.local/share/eval-magic/skills-c61a1930/working-with-tdd/ manually to discard. The existing teardown test could not catch this: it runs with EVAL_MAGIC_WORKSPACE_DIR=.eval-magic, where the two spellings coincide. The new case puts the workspace elsewhere. Verified: cargo test, cargo clippy --all-targets -- -D warnings, cargo fmt --check. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NJjf8XMm1e1XtRZaGnkmvr --- src/cli/commands/workspace.rs | 4 ++-- tests/cli/workspace.rs | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) 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/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()); +} From 66d0611bc277899d44fc9c18b5e9f023856f8c5b Mon Sep 17 00:00:00 2001 From: samiamorwas Date: Tue, 1 Sep 2026 20:44:16 -0400 Subject: [PATCH 4/4] fix(guard): drop the unreachable iteration default in the sweep report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the #298 fixes. `envs_phrase` read the iteration out of `checked` with `unwrap_or_default`, which no call site can reach ā€” but a future one would silently print "iteration 0". Take the iteration as an argument so the invariant is in the signature. The task repository's exclude file now covers more than the outputs dir, so its failure message says "framework path exclusion". Verified: cargo test, cargo clippy --all-targets -- -D warnings, cargo fmt --check. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NJjf8XMm1e1XtRZaGnkmvr --- src/cli/commands/guard.rs | 16 ++++++++++------ src/cli/run/orchestrate/git.rs | 2 +- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/cli/commands/guard.rs b/src/cli/commands/guard.rs index dabcbf8..7e77595 100644 --- a/src/cli/commands/guard.rs +++ b/src/cli/commands/guard.rs @@ -93,8 +93,10 @@ pub(crate) fn run_teardown_guard(args: CommonArgs) -> anyhow::Result<()> { Err(error) => unchecked = Some(error.to_string()), } - let envs_phrase = |count: usize| { - let iteration = checked.map(|(iteration, _)| iteration).unwrap_or_default(); + // 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" } @@ -104,13 +106,15 @@ pub(crate) fn run_teardown_guard(args: CommonArgs) -> anyhow::Result<()> { if cwd_torn { removed.push("the invocation cwd".to_string()); } - if envs_torn > 0 { - removed.push(envs_phrase(envs_torn)); + 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((_, count)) = checked { - scopes.push(envs_phrase(count)); + if let Some((iteration, count)) = checked { + scopes.push(envs_phrase(iteration, count)); } println!( "No write guard was installed ā€” nothing to remove (checked {}).", diff --git a/src/cli/run/orchestrate/git.rs b/src/cli/run/orchestrate/git.rs index 719b7f4..4c2840e 100644 --- a/src/cli/run/orchestrate/git.rs +++ b/src/cli/run/orchestrate/git.rs @@ -112,7 +112,7 @@ fn initialize_task_repository(plan: &TaskRepository) -> Result<(), String> { 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 output exclusion: {error}"))?; + .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 [