From 1076f00fc4dde0562778f4881e5df22cfba807d6 Mon Sep 17 00:00:00 2001 From: samiamorwas Date: Wed, 2 Sep 2026 20:12:27 -0400 Subject: [PATCH] fix(grade): skip incomplete command checks Gate held-out command checks on the runner-owned run record so partial ingest leaves undispatched environments untouched. Bind reusable results to both the assertion and run record, and warn when legacy artifacts imply possible contamination. --- docs/guides/judging.md | 17 +- docs/progressive-enhancements.md | 5 +- schema/command-check.schema.json | 6 +- src/cli/args.rs | 24 ++- src/cli/commands/pipeline.rs | 6 +- src/pipeline/grade/command_check.rs | 85 +++++---- src/pipeline/grade/command_check/cache.rs | 40 ++++ .../grade/command_check/tests/lifecycle.rs | 174 +++++++++++++++++ src/pipeline/grade/command_check/tests/mod.rs | 177 ++---------------- .../grade/command_check/tests/staleness.rs | 168 +++++++++++++++-- tests/cli/basics.rs | 4 +- tests/cli/command_check.rs | 14 +- tests/cli/docs.rs | 2 + tests/run/command_check.rs | 1 + tests/run/command_check/partial_dispatch.rs | 170 +++++++++++++++++ 15 files changed, 650 insertions(+), 243 deletions(-) create mode 100644 src/pipeline/grade/command_check/cache.rs create mode 100644 src/pipeline/grade/command_check/tests/lifecycle.rs create mode 100644 tests/run/command_check/partial_dispatch.rs diff --git a/docs/guides/judging.md b/docs/guides/judging.md index c7a5e5e..244befe 100644 --- a/docs/guides/judging.md +++ b/docs/guides/judging.md @@ -66,10 +66,19 @@ assertion set, so a benchmark can be read against the instrument that produced i cannot be read leaves the run-time copy in place with a warning; one that fails validation stops grading rather than measuring with assertions you have already replaced. -Cached results are keyed by assertion id. Editing an assertion in place — rewording an `llm_judge` -rubric, changing a `command_check` command — leaves the verdict or result the previous definition -produced, and `grade` reports every one it reused. Re-run `eval-magic grade --overwrite` to -re-execute command checks and `eval-magic dispatch --judges --overwrite` to re-judge. +Cached judge responses are keyed by assertion id. Rewording an `llm_judge` rubric under the same id +leaves the previous verdict in place; use `eval-magic dispatch --judges --overwrite` to re-judge it. + +Command-check results use both the authored definition digest and the exact `run.json` digest as +their cache key. `grade` reuses only an exact match. A missing or mismatched digest executes the +check again and replaces the result, so a legacy result, an edited check, or a changed run record +refreshes without `eval-magic grade --overwrite`; that option forces even an exact match to execute +again. + +A command check is eligible only after its task has a runner-owned `run.json`. Partial ingest leaves +an incomplete task environment untouched: no held-out setup, command execution, or result write. If +an incomplete task already carries a cached command-check result, `grade` warns that an older grader +may have contaminated the environment. That task must not be resumed; build a fresh iteration. ## What the bundle contains diff --git a/docs/progressive-enhancements.md b/docs/progressive-enhancements.md index c3e7cc8..93bf759 100644 --- a/docs/progressive-enhancements.md +++ b/docs/progressive-enhancements.md @@ -121,8 +121,9 @@ 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` -copies the assertion's held-out `setup_files` from the skill's `evals/` directory into that root and -executes the trusted command through the platform shell. Root `.git` paths are reserved for both +grades only tasks with a runner-owned `run.json`, copies the assertion's held-out `setup_files` from +the skill's `evals/` directory into that root, and executes the trusted command through the platform +shell. Root `.git` paths are reserved for both visible overlays and held-out setup files, while nested repositories remain valid. The runner clears inherited Git routing variables before optional `env` values override the environment; optional `matrix` values execute every Cartesian-product cell and persist per-cell results. The files are diff --git a/schema/command-check.schema.json b/schema/command-check.schema.json index 2fec500..34dbb20 100644 --- a/schema/command-check.schema.json +++ b/schema/command-check.schema.json @@ -39,7 +39,11 @@ }, "definition_digest": { "type": "string", - "description": "Digest of the authored command_check this result came from. Reuse is keyed by assertion id, so this is what lets a later grade report that the check under that id has been edited since. Absent in results written before it was recorded, which make no claim either way." + "description": "Digest of the authored command_check this result came from. A result is reusable only when this and run_record_digest match the current check and run record. Absent in legacy results, which are rerun." + }, + "run_record_digest": { + "type": "string", + "description": "Digest of the exact runner-owned run.json bytes this result graded. A result is reusable only when this and definition_digest match the current run record and check. Absent in legacy results, which are rerun." }, "cells": { "type": "array", diff --git a/src/cli/args.rs b/src/cli/args.rs index 5b2726f..ebb5782 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -679,8 +679,12 @@ pub(crate) enum Commands { /// `diff_scope` grading for finalize, injects held-out /// `command_check.setup_files`, and executes each /// runner-owned command check in its task environment, applying its - /// environment overrides and running every environment matrix cell. Diff - /// scope is captured before held-out files are injected. Then stops at the + /// environment overrides and running every environment matrix cell. A task + /// is eligible only after its runner-owned `run.json` exists, so a partial + /// ingest leaves incomplete task environments untouched. A cached result is + /// reused only when its authored definition and run-record digests match; + /// legacy or stale results are executed again. Diff scope is captured before + /// held-out files are injected. Then stops at the /// judge hand-off, writing one bounded `judge-evidence.md` per recorded run /// and listing the effective sample count of judge tasks per `llm_judge` /// assertion. The exact evidence bundle is shared by that run's tasks and @@ -761,8 +765,11 @@ pub(crate) enum Commands { /// 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 - /// are reused. Before emitting tasks, writes one `judge-evidence.md` beside + /// every environment matrix cell. Tasks without `run.json` are skipped before + /// setup injection or command execution. A command result is reused only when + /// its authored definition and run-record digests match; legacy or stale + /// results execute again. Completed diff-scope results are reused. Before + /// emitting tasks, writes one `judge-evidence.md` beside /// every recorded run. This 98,304-byte bounded bundle combines task context, /// completion state, diff evidence, conversation, tool summary, and source /// paths; its exact bytes are inlined into each run's LLM-judge prompts. The @@ -793,10 +800,11 @@ pub(crate) enum Commands { /// assertions from the run's own evidence, after the dispatch they grade. Every /// invocation prints the file it read them from, and each `grading.json` records it /// under `assertion_source`. An unreadable live file leaves the run-time copy in - /// place with a warning; an invalid one stops grading. Cached judge verdicts and - /// command-check results are keyed by assertion id, so an assertion edited in place - /// is reported rather than silently reused: `--overwrite` re-executes command - /// checks; `dispatch --judges --overwrite` re-judges. See `eval-magic docs judging`. + /// place with a warning; an invalid one stops grading. Cached judge verdicts are + /// keyed by assertion id, so `dispatch --judges --overwrite` re-judges an edited + /// assertion. Command checks invalidate automatically when their definition or + /// run record changes; `grade --overwrite` also re-executes an exact cache match. + /// See `eval-magic docs judging`. Grade(GradeArgs), /// Aggregate before/after benchmark deltas. /// diff --git a/src/cli/commands/pipeline.rs b/src/cli/commands/pipeline.rs index 16abb3e..c13c3bf 100644 --- a/src/cli/commands/pipeline.rs +++ b/src/cli/commands/pipeline.rs @@ -342,10 +342,10 @@ pub(crate) fn run_grade(args: GradeArgs) -> anyhow::Result<()> { diffs.measured, diffs.reused, diffs.missing_baseline, diffs.shared_environment ); let commands = pipeline::grade_command_checks(&dir, &instrument, common.overwrite)?; - if commands.executed + commands.reused > 0 { + if commands.executed + commands.reused + commands.skipped_incomplete > 0 { println!( - "Command checks: {} executed, {} reused, {} failed", - commands.executed, commands.reused, commands.failed + "Command checks: {} executed, {} reused, {} failed, {} skipped (missing run.json)", + commands.executed, commands.reused, commands.failed, commands.skipped_incomplete ); } for w in &commands.warnings { diff --git a/src/pipeline/grade/command_check.rs b/src/pipeline/grade/command_check.rs index 859b113..db9bfdc 100644 --- a/src/pipeline/grade/command_check.rs +++ b/src/pipeline/grade/command_check.rs @@ -14,8 +14,12 @@ use crate::pipeline::error::PipelineError; use crate::pipeline::grade::instrument::GradingInstrument; use crate::validation::{SchemaName, validate_against_schema}; +use cache::{definition_digest, has_cached_results, run_record_digest}; + const DIAGNOSTIC_LIMIT: usize = 2 * 1024; +mod cache; + /// The schema-gated intermediate result persisted before finalize converts it /// into a normal [`crate::core::AssertionResult`]. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -29,12 +33,14 @@ pub struct CommandCheckResult { pub stderr: String, #[serde(skip_serializing_if = "Option::is_none")] pub cells: Option>, - /// Digest of the `command_check` this result came from. Reuse is keyed by - /// assertion id, so this is what tells a later grade that the check under - /// that id has been edited since. Absent in results that predate the record, - /// which make no claim either way. + /// Digest of the `command_check` this result came from. Reuse requires it to + /// match the current definition; an absent legacy value is never reusable. #[serde(default, skip_serializing_if = "Option::is_none")] pub definition_digest: Option, + /// Digest of the exact runner-owned `run.json` bytes this result graded. + /// Absent in legacy results, which are never reusable as a cache entry. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub run_record_digest: Option, } /// The result of one environment-matrix cell. @@ -53,8 +59,10 @@ pub struct CommandCheckSummary { pub executed: usize, pub reused: usize, pub failed: usize, - /// Reused results whose check has since been edited. Returned rather than - /// printed: the CLI handler owns how a warning reads. + /// Tasks skipped because their runner-owned `run.json` is absent. + pub skipped_incomplete: usize, + /// Safety warnings returned rather than printed: the CLI handler owns how + /// user-facing warnings read. pub warnings: Vec, } @@ -68,12 +76,14 @@ struct DispatchFile { struct DispatchTask { eval_id: String, condition: String, + #[serde(default)] + run_index: Option, eval_root: Option, run_record_path: String, } -/// Inject held-out setup files and execute all command checks in declaration -/// order for every matching dispatch task. +/// For each matching task with a completed run record, inject held-out setup +/// files and execute all command checks in declaration order. pub fn grade_command_checks( iteration_dir: &Path, instrument: &GradingInstrument, @@ -127,6 +137,26 @@ pub fn grade_command_checks( continue; } + let run_record_path = Path::new(&task.run_record_path); + let run_dir = run_record_path.parent().ok_or_else(|| { + PipelineError::Message(format!( + "command_check task '{}'/{} has no run directory in run_record_path", + task.eval_id, task.condition + )) + })?; + let results_dir = run_dir.join("command-checks"); + if !run_record_path.exists() { + summary.skipped_incomplete += 1; + if has_cached_results(&results_dir)? { + summary.warnings.push(format!( + "command_check results already exist for {}, but run.json is missing. An older eval-magic version may have copied held-out setup files or executed commands in this task environment; it may already be contaminated and must not be resumed. Build a fresh iteration.", + task_label(task) + )); + } + continue; + } + let run_record_digest = run_record_digest(run_record_path)?; + let eval_root = task .eval_root .as_deref() @@ -138,13 +168,6 @@ pub fn grade_command_checks( )); } let eval_root = Path::new(eval_root); - let run_dir = Path::new(&task.run_record_path).parent().ok_or_else(|| { - PipelineError::Message(format!( - "command_check task '{}'/{} has no run directory in run_record_path", - task.eval_id, task.condition - )) - })?; - let results_dir = run_dir.join("command-checks"); for check in checks { validate_assertion_id(&check.id)?; @@ -157,21 +180,17 @@ pub fn grade_command_checks( &value, &result_path.to_string_lossy(), )?; - if reused - .definition_digest - .is_some_and(|recorded| recorded != digest) + if reused.definition_digest.as_deref() == Some(digest.as_str()) + && reused.run_record_digest.as_deref() == Some(run_record_digest.as_str()) { - summary.warnings.push(format!( - "command_check '{}' for {}/{} changed since its cached result was produced; that result is reused as-is. Re-run grade with --overwrite to execute the edited check.", - check.id, task.eval_id, task.condition - )); + summary.reused += 1; + continue; } - summary.reused += 1; - continue; } inject_setup_files(check, instrument.setup_root_for(&task.eval_id), eval_root)?; - let result = execute_command_check(check, eval_root)?; + let mut result = execute_command_check(check, eval_root)?; + result.run_record_digest = Some(run_record_digest.clone()); if !result.passed { summary.failed += 1; } @@ -189,14 +208,12 @@ pub fn grade_command_checks( Ok(summary) } -/// Digest of a check's authored definition, so reuse can tell an edited check -/// from the one that produced the cached result. -fn definition_digest(check: &AssertionCommandCheck) -> String { - crate::core::fs::fnv1a_hex( - serde_json::to_string(check) - .expect("an authored command_check serializes") - .as_bytes(), - ) +fn task_label(task: &DispatchTask) -> String { + let run = task + .run_index + .map(|index| format!("/run-{index}")) + .unwrap_or_default(); + format!("{}/{}{run}", task.eval_id, task.condition) } fn isolation_error(task: &DispatchTask, detail: &str) -> PipelineError { @@ -274,6 +291,7 @@ pub(super) fn execute_command_check( stderr: cell.stderr, cells: None, definition_digest: Some(definition_digest(assertion)), + run_record_digest: None, }); }; @@ -312,6 +330,7 @@ pub(super) fn execute_command_check( stderr: String::new(), cells: Some(cells), definition_digest: Some(definition_digest(assertion)), + run_record_digest: None, }) } diff --git a/src/pipeline/grade/command_check/cache.rs b/src/pipeline/grade/command_check/cache.rs new file mode 100644 index 0000000..0dd5d49 --- /dev/null +++ b/src/pipeline/grade/command_check/cache.rs @@ -0,0 +1,40 @@ +//! Command-check cache identity and legacy-result detection. + +use std::fs; +use std::path::Path; + +use crate::core::AssertionCommandCheck; +use crate::pipeline::error::PipelineError; + +/// Digest of a check's authored definition, so reuse never crosses an edit. +pub(super) fn definition_digest(check: &AssertionCommandCheck) -> String { + crate::core::fs::fnv1a_hex( + serde_json::to_string(check) + .expect("an authored command_check serializes") + .as_bytes(), + ) +} + +/// Digest the exact runner-owned record bytes that a result grades. +pub(super) fn run_record_digest(path: &Path) -> Result { + Ok(crate::core::fs::fnv1a_hex(&fs::read(path)?)) +} + +/// True when an incomplete task already carries a result left by an older +/// grader. Presence alone is enough for the safety warning: a malformed legacy +/// result still means held-out setup or its command may have touched the env. +pub(super) fn has_cached_results(results_dir: &Path) -> Result { + if !results_dir.exists() { + return Ok(false); + } + for entry in fs::read_dir(results_dir)? { + if entry? + .path() + .extension() + .is_some_and(|extension| extension == "json") + { + return Ok(true); + } + } + Ok(false) +} diff --git a/src/pipeline/grade/command_check/tests/lifecycle.rs b/src/pipeline/grade/command_check/tests/lifecycle.rs new file mode 100644 index 0000000..e94b1cb --- /dev/null +++ b/src/pipeline/grade/command_check/tests/lifecycle.rs @@ -0,0 +1,174 @@ +//! Persistence, overwrite, isolation, and declaration-order behavior. + +use super::*; + +#[test] +fn persisted_results_are_reused_and_overwrite_reruns_in_declaration_order() { + let root = tempfile::TempDir::new().unwrap(); + let skill_dir = root.path().join("skill"); + let iteration_dir = root.path().join("iteration-1"); + let eval_root = iteration_dir.join("env-g1-with_skill"); + fs::create_dir_all(skill_dir.join("evals/holdout")).unwrap(); + fs::create_dir_all(&eval_root).unwrap(); + fs::write(skill_dir.join("evals/holdout/secret.txt"), "held out").unwrap(); + write_dispatch(&iteration_dir, &eval_root, false); + assert!(!eval_root.join("holdout/secret.txt").exists()); + + let first = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); + assert_eq!(first.executed, 1); + assert_eq!(first.reused, 0); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "x" + ); + assert_eq!( + fs::read_to_string(eval_root.join("holdout/secret.txt")).unwrap(), + "held out" + ); + let result_path = iteration_dir.join("eval-e1/with_skill/command-checks/check.json"); + assert!(result_path.exists()); + assert!( + serde_json::from_str::(&fs::read_to_string(&result_path).unwrap()) + .unwrap() + .get("run_record_digest") + .is_some(), + "a persisted command check identifies the run record it graded" + ); + + let reused = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); + assert_eq!(reused.executed, 0); + assert_eq!(reused.reused, 1); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "x" + ); + + let overwritten = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, true); + assert_eq!(overwritten.executed, 1); + assert_eq!(overwritten.reused, 0); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "xx" + ); +} + +#[test] +fn persisted_matrix_results_are_schema_gated_and_reused() { + let root = tempfile::TempDir::new().unwrap(); + let skill_dir = root.path().join("skill"); + let iteration_dir = root.path().join("iteration-1"); + let eval_root = iteration_dir.join("env-g1-with_skill"); + fs::create_dir_all(skill_dir.join("evals/holdout")).unwrap(); + fs::create_dir_all(&eval_root).unwrap(); + fs::write(skill_dir.join("evals/holdout/secret.txt"), "held out").unwrap(); + write_dispatch(&iteration_dir, &eval_root, false); + + let mut config = evals(&append_command()); + let crate::core::Assertion::CommandCheck(check) = config.evals[0] + .assertions + .as_mut() + .unwrap() + .first_mut() + .unwrap() + else { + panic!("expected command_check"); + }; + check.matrix = Some(std::collections::BTreeMap::from([( + "TZ".into(), + vec!["UTC".into(), "Europe/Berlin".into()], + )])); + + let first = grade_frozen(&iteration_dir, config.clone(), &skill_dir, false); + assert_eq!(first.executed, 1); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "xx" + ); + let result_path = iteration_dir.join("eval-e1/with_skill/command-checks/check.json"); + let result: CommandCheckResult = + serde_json::from_str(&fs::read_to_string(&result_path).unwrap()).unwrap(); + assert_eq!(result.cells.as_ref().unwrap().len(), 2); + + let reused = grade_frozen(&iteration_dir, config.clone(), &skill_dir, false); + assert_eq!(reused.executed, 0); + assert_eq!(reused.reused, 1); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "xx" + ); + + let overwritten = grade_frozen(&iteration_dir, config.clone(), &skill_dir, true); + assert_eq!(overwritten.executed, 1); + assert_eq!(overwritten.reused, 0); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "xxxx" + ); +} + +#[test] +fn shared_eval_root_is_rejected_with_fresh_iteration_guidance() { + let root = tempfile::TempDir::new().unwrap(); + let skill_dir = root.path().join("skill"); + let iteration_dir = root.path().join("iteration-1"); + let eval_root = iteration_dir.join("env-g1-with_skill"); + fs::create_dir_all(skill_dir.join("evals/holdout")).unwrap(); + fs::create_dir_all(&eval_root).unwrap(); + fs::write(skill_dir.join("evals/holdout/secret.txt"), "held out").unwrap(); + write_dispatch(&iteration_dir, &eval_root, true); + + let error = grade_command_checks( + &iteration_dir, + &GradingInstrument::frozen(evals(&exit_command(0)), &skill_dir), + false, + ) + .unwrap_err() + .to_string(); + assert!(error.contains("shares eval_root"), "{error}"); + assert!(error.contains("fresh iteration"), "{error}"); +} + +#[test] +fn multiple_checks_execute_in_declaration_order_against_one_env() { + let root = tempfile::TempDir::new().unwrap(); + let skill_dir = root.path().join("skill"); + let iteration_dir = root.path().join("iteration-1"); + let eval_root = iteration_dir.join("env-g1-with_skill"); + fs::create_dir_all(skill_dir.join("evals")).unwrap(); + fs::create_dir_all(&eval_root).unwrap(); + write_dispatch(&iteration_dir, &eval_root, false); + let evals: EvalsConfig = serde_json::from_value(json!({ + "skill_name": "demo", + "evals": [{ + "id": "e1", + "prompt": "p", + "expected_output": "o", + "assertions": [ + { + "id": "first", + "type": "command_check", + "command": fixture(&["--text", "ready", "--write", "state.txt"]) + }, + { + "id": "second", + "type": "command_check", + "command": fixture(&["--require-file-text", "state.txt", "ready"]) + } + ] + }] + })) + .unwrap(); + + let summary = grade_frozen(&iteration_dir, evals, &skill_dir, false); + assert_eq!(summary.executed, 2); + for id in ["first", "second"] { + let result: CommandCheckResult = serde_json::from_str( + &fs::read_to_string( + iteration_dir.join(format!("eval-e1/with_skill/command-checks/{id}.json")), + ) + .unwrap(), + ) + .unwrap(); + assert!(result.passed, "{id}: {}", result.evidence); + } +} diff --git a/src/pipeline/grade/command_check/tests/mod.rs b/src/pipeline/grade/command_check/tests/mod.rs index c470730..2ad5b60 100644 --- a/src/pipeline/grade/command_check/tests/mod.rs +++ b/src/pipeline/grade/command_check/tests/mod.rs @@ -4,6 +4,7 @@ use crate::pipeline::grade::instrument::GradingInstrument; use serde_json::json; use std::fs; +mod lifecycle; mod staleness; fn check(command: &str) -> AssertionCommandCheck { @@ -104,25 +105,33 @@ fn grade_frozen( fn write_dispatch(iteration_dir: &Path, eval_root: &Path, shared: bool) { fs::create_dir_all(iteration_dir).unwrap(); + let first_run = iteration_dir.join("eval-e1/with_skill/run.json"); let mut tasks = vec![json!({ "eval_id": "e1", "condition": "with_skill", "eval_root": eval_root, - "run_record_path": iteration_dir.join("eval-e1/with_skill/run.json") + "run_record_path": first_run })]; + let mut run_records = vec![(first_run, json!({"run": "first"}))]; if shared { + let second_run = iteration_dir.join("eval-e2/with_skill/run.json"); tasks.push(json!({ "eval_id": "e2", "condition": "with_skill", "eval_root": eval_root, - "run_record_path": iteration_dir.join("eval-e2/with_skill/run.json") + "run_record_path": second_run })); + run_records.push((second_run, json!({"run": "second"}))); } fs::write( iteration_dir.join("dispatch.json"), serde_json::to_vec(&json!({ "tasks": tasks })).unwrap(), ) .unwrap(); + for (path, record) in run_records { + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, serde_json::to_vec(&record).unwrap()).unwrap(); + } } #[test] @@ -441,167 +450,3 @@ fn signal_termination_is_an_ordinary_failed_assertion() { assert_eq!(result.actual_exit_code, None); assert!(result.evidence.contains("terminated by signal")); } - -#[test] -fn persisted_results_are_reused_and_overwrite_reruns_in_declaration_order() { - let root = tempfile::TempDir::new().unwrap(); - let skill_dir = root.path().join("skill"); - let iteration_dir = root.path().join("iteration-1"); - let eval_root = iteration_dir.join("env-g1-with_skill"); - fs::create_dir_all(skill_dir.join("evals/holdout")).unwrap(); - fs::create_dir_all(&eval_root).unwrap(); - fs::write(skill_dir.join("evals/holdout/secret.txt"), "held out").unwrap(); - write_dispatch(&iteration_dir, &eval_root, false); - assert!(!eval_root.join("holdout/secret.txt").exists()); - - let first = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); - assert_eq!(first.executed, 1); - assert_eq!(first.reused, 0); - assert_eq!( - fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), - "x" - ); - assert_eq!( - fs::read_to_string(eval_root.join("holdout/secret.txt")).unwrap(), - "held out" - ); - let result_path = iteration_dir.join("eval-e1/with_skill/command-checks/check.json"); - assert!(result_path.exists()); - - let reused = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); - assert_eq!(reused.executed, 0); - assert_eq!(reused.reused, 1); - assert_eq!( - fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), - "x" - ); - - let overwritten = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, true); - assert_eq!(overwritten.executed, 1); - assert_eq!(overwritten.reused, 0); - assert_eq!( - fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), - "xx" - ); -} - -#[test] -fn persisted_matrix_results_are_schema_gated_and_reused() { - let root = tempfile::TempDir::new().unwrap(); - let skill_dir = root.path().join("skill"); - let iteration_dir = root.path().join("iteration-1"); - let eval_root = iteration_dir.join("env-g1-with_skill"); - fs::create_dir_all(skill_dir.join("evals/holdout")).unwrap(); - fs::create_dir_all(&eval_root).unwrap(); - fs::write(skill_dir.join("evals/holdout/secret.txt"), "held out").unwrap(); - write_dispatch(&iteration_dir, &eval_root, false); - - let mut config = evals(&append_command()); - let crate::core::Assertion::CommandCheck(check) = config.evals[0] - .assertions - .as_mut() - .unwrap() - .first_mut() - .unwrap() - else { - panic!("expected command_check"); - }; - check.matrix = Some(std::collections::BTreeMap::from([( - "TZ".into(), - vec!["UTC".into(), "Europe/Berlin".into()], - )])); - - let first = grade_frozen(&iteration_dir, config.clone(), &skill_dir, false); - assert_eq!(first.executed, 1); - assert_eq!( - fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), - "xx" - ); - let result_path = iteration_dir.join("eval-e1/with_skill/command-checks/check.json"); - let result: CommandCheckResult = - serde_json::from_str(&fs::read_to_string(&result_path).unwrap()).unwrap(); - assert_eq!(result.cells.as_ref().unwrap().len(), 2); - - let reused = grade_frozen(&iteration_dir, config.clone(), &skill_dir, false); - assert_eq!(reused.executed, 0); - assert_eq!(reused.reused, 1); - assert_eq!( - fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), - "xx" - ); - - let overwritten = grade_frozen(&iteration_dir, config.clone(), &skill_dir, true); - assert_eq!(overwritten.executed, 1); - assert_eq!(overwritten.reused, 0); - assert_eq!( - fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), - "xxxx" - ); -} - -#[test] -fn shared_eval_root_is_rejected_with_fresh_iteration_guidance() { - let root = tempfile::TempDir::new().unwrap(); - let skill_dir = root.path().join("skill"); - let iteration_dir = root.path().join("iteration-1"); - let eval_root = iteration_dir.join("env-g1-with_skill"); - fs::create_dir_all(skill_dir.join("evals/holdout")).unwrap(); - fs::create_dir_all(&eval_root).unwrap(); - fs::write(skill_dir.join("evals/holdout/secret.txt"), "held out").unwrap(); - write_dispatch(&iteration_dir, &eval_root, true); - - let error = grade_command_checks( - &iteration_dir, - &GradingInstrument::frozen(evals(&exit_command(0)), &skill_dir), - false, - ) - .unwrap_err() - .to_string(); - assert!(error.contains("shares eval_root"), "{error}"); - assert!(error.contains("fresh iteration"), "{error}"); -} - -#[test] -fn multiple_checks_execute_in_declaration_order_against_one_env() { - let root = tempfile::TempDir::new().unwrap(); - let skill_dir = root.path().join("skill"); - let iteration_dir = root.path().join("iteration-1"); - let eval_root = iteration_dir.join("env-g1-with_skill"); - fs::create_dir_all(skill_dir.join("evals")).unwrap(); - fs::create_dir_all(&eval_root).unwrap(); - write_dispatch(&iteration_dir, &eval_root, false); - let evals: EvalsConfig = serde_json::from_value(json!({ - "skill_name": "demo", - "evals": [{ - "id": "e1", - "prompt": "p", - "expected_output": "o", - "assertions": [ - { - "id": "first", - "type": "command_check", - "command": fixture(&["--text", "ready", "--write", "state.txt"]) - }, - { - "id": "second", - "type": "command_check", - "command": fixture(&["--require-file-text", "state.txt", "ready"]) - } - ] - }] - })) - .unwrap(); - - let summary = grade_frozen(&iteration_dir, evals, &skill_dir, false); - assert_eq!(summary.executed, 2); - for id in ["first", "second"] { - let result: CommandCheckResult = serde_json::from_str( - &fs::read_to_string( - iteration_dir.join(format!("eval-e1/with_skill/command-checks/{id}.json")), - ) - .unwrap(), - ) - .unwrap(); - assert!(result.passed, "{id}: {}", result.evidence); - } -} diff --git a/src/pipeline/grade/command_check/tests/staleness.rs b/src/pipeline/grade/command_check/tests/staleness.rs index 173802a..30e1efc 100644 --- a/src/pipeline/grade/command_check/tests/staleness.rs +++ b/src/pipeline/grade/command_check/tests/staleness.rs @@ -1,4 +1,4 @@ -//! Command checks read and reused after the assertion set has moved on. +//! Command-check cache eligibility across definition, run, and completion changes. use super::*; @@ -55,11 +55,9 @@ fn setup_files_for_an_assertion_added_after_the_run_come_from_the_live_tree() { ); } -/// Cached results are keyed by assertion id, so an edited `command_check` under -/// an unchanged id would silently be reported from the old command. Now that -/// assertions can be edited after the run (#295), say so instead. +/// A cached result is reusable only for the authored check that produced it. #[test] -fn a_reused_result_whose_check_changed_is_reported_as_stale() { +fn a_changed_command_check_is_executed_instead_of_reusing_the_old_result() { let root = tempfile::TempDir::new().unwrap(); let skill_dir = root.path().join("skill"); let iteration_dir = root.path().join("iteration-1"); @@ -74,21 +72,155 @@ fn a_reused_result_whose_check_changed_is_reported_as_stale() { assert!(first.warnings.is_empty()); // Same assertion id, different command. - let edited = grade_frozen(&iteration_dir, evals(&exit_command(3)), &skill_dir, false); + let edited = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); - assert_eq!(edited.reused, 1, "the persisted result is still reused"); - let warning = edited.warnings.join("\n"); - assert!( - warning.contains("check"), - "the stale check is named: {warning}" - ); - assert!( - warning.contains("--overwrite"), - "and the way to re-execute it: {warning}" + assert_eq!(edited.executed, 1); + assert_eq!(edited.reused, 0); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "x" ); - // An unedited check stays quiet. - let unchanged = grade_frozen(&iteration_dir, evals(&exit_command(0)), &skill_dir, false); + let unchanged = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); assert_eq!(unchanged.reused, 1); - assert!(unchanged.warnings.is_empty(), "{:?}", unchanged.warnings); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "x" + ); +} + +#[test] +fn a_result_for_another_run_record_is_executed_once_then_reused() { + let root = tempfile::TempDir::new().unwrap(); + let skill_dir = root.path().join("skill"); + let iteration_dir = root.path().join("iteration-1"); + let eval_root = iteration_dir.join("env-g1-with_skill"); + let run_record = iteration_dir.join("eval-e1/with_skill/run.json"); + fs::create_dir_all(skill_dir.join("evals/holdout")).unwrap(); + fs::create_dir_all(&eval_root).unwrap(); + fs::write(skill_dir.join("evals/holdout/secret.txt"), "held out").unwrap(); + write_dispatch(&iteration_dir, &eval_root, false); + + let first = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); + assert_eq!(first.executed, 1); + let first_result = + fs::read_to_string(iteration_dir.join("eval-e1/with_skill/command-checks/check.json")) + .unwrap(); + let first_digest = + serde_json::from_str::(&first_result).unwrap()["run_record_digest"] + .as_str() + .unwrap() + .to_string(); + + fs::write(&run_record, r#"{"run":"replacement"}"#).unwrap(); + let replacement = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); + assert_eq!(replacement.executed, 1); + assert_eq!(replacement.reused, 0); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "xx" + ); + let replacement_result = + fs::read_to_string(iteration_dir.join("eval-e1/with_skill/command-checks/check.json")) + .unwrap(); + let replacement_digest = serde_json::from_str::(&replacement_result) + .unwrap()["run_record_digest"] + .as_str() + .unwrap() + .to_string(); + assert_ne!(replacement_digest, first_digest); + + let reused = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); + assert_eq!(reused.executed, 0); + assert_eq!(reused.reused, 1); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "xx" + ); +} + +#[test] +fn a_legacy_result_without_run_identity_is_executed_once_then_reused() { + let root = tempfile::TempDir::new().unwrap(); + let skill_dir = root.path().join("skill"); + let iteration_dir = root.path().join("iteration-1"); + let eval_root = iteration_dir.join("env-g1-with_skill"); + fs::create_dir_all(skill_dir.join("evals/holdout")).unwrap(); + fs::create_dir_all(&eval_root).unwrap(); + fs::write(skill_dir.join("evals/holdout/secret.txt"), "held out").unwrap(); + write_dispatch(&iteration_dir, &eval_root, false); + + let legacy = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); + assert_eq!(legacy.executed, 1); + let result_path = iteration_dir.join("eval-e1/with_skill/command-checks/check.json"); + let mut legacy_value: serde_json::Value = + serde_json::from_str(&fs::read_to_string(&result_path).unwrap()).unwrap(); + legacy_value + .as_object_mut() + .unwrap() + .remove("run_record_digest"); + fs::write(&result_path, serde_json::to_vec(&legacy_value).unwrap()).unwrap(); + + let refreshed = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); + assert_eq!(refreshed.executed, 1); + assert_eq!(refreshed.reused, 0); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "xx" + ); + + let reused = grade_frozen(&iteration_dir, evals(&append_command()), &skill_dir, false); + assert_eq!(reused.executed, 0); + assert_eq!(reused.reused, 1); + assert_eq!( + fs::read_to_string(eval_root.join("command-runs.txt")).unwrap(), + "xx" + ); +} + +#[test] +fn an_incomplete_task_with_a_cached_result_is_never_touched_and_warns() { + let root = tempfile::TempDir::new().unwrap(); + let skill_dir = root.path().join("skill"); + let iteration_dir = root.path().join("iteration-1"); + let eval_root = iteration_dir.join("env-g1-with_skill"); + let run_record = iteration_dir.join("eval-e1/with_skill/run.json"); + let result_path = iteration_dir.join("eval-e1/with_skill/command-checks/check.json"); + fs::create_dir_all(skill_dir.join("evals/holdout")).unwrap(); + fs::create_dir_all(&eval_root).unwrap(); + fs::write(skill_dir.join("evals/holdout/secret.txt"), "held out").unwrap(); + write_dispatch(&iteration_dir, &eval_root, false); + fs::remove_file(run_record).unwrap(); + fs::create_dir_all(result_path.parent().unwrap()).unwrap(); + let legacy = json!({ + "id": "check", + "passed": true, + "evidence": "old result", + "expected_exit_code": 0, + "actual_exit_code": 0, + "stdout": "", + "stderr": "" + }) + .to_string(); + fs::write(&result_path, &legacy).unwrap(); + + for overwrite in [false, true] { + let summary = grade_frozen( + &iteration_dir, + evals(&append_command()), + &skill_dir, + overwrite, + ); + assert_eq!(summary.skipped_incomplete, 1); + assert_eq!(summary.executed, 0); + assert_eq!(summary.reused, 0); + let warning = summary.warnings.join("\n"); + assert!(warning.contains("e1/with_skill"), "{warning}"); + assert!(warning.contains("may already be contaminated"), "{warning}"); + assert!(warning.contains("must not be resumed"), "{warning}"); + assert!(warning.contains("fresh iteration"), "{warning}"); + assert_eq!(fs::read_to_string(&result_path).unwrap(), legacy); + assert!(!eval_root.join("holdout/secret.txt").exists()); + assert!(!eval_root.join("command-runs.txt").exists()); + } } diff --git a/tests/cli/basics.rs b/tests/cli/basics.rs index 55d5fa1..d4e7f10 100644 --- a/tests/cli/basics.rs +++ b/tests/cli/basics.rs @@ -280,8 +280,8 @@ fn grade_and_ingest_help_document_runner_owned_command_checks() { .success() .stdout(contains("command_check")) .stdout(contains("runner")) - .stdout(contains("held-out")) - .stdout(contains("environment matrix")); + .stdout(contains("held-out").and(contains("environment matrix"))) + .stdout(contains("run.json").and(contains("run-record digests"))); } } diff --git a/tests/cli/command_check.rs b/tests/cli/command_check.rs index 5536238..f72e030 100644 --- a/tests/cli/command_check.rs +++ b/tests/cli/command_check.rs @@ -84,11 +84,10 @@ fn finalize_folds_command_check_result_into_normal_pass_rate() { assert_eq!(grading["assertion_results"][0]["confidence"], json!(1.0)); } -/// #295 follow-on: cached command-check results are keyed by assertion id, so -/// an edited check under an unchanged id is reported from the old command. The -/// operator has to be told, and told how to re-execute it. +/// An incomplete task carrying an old result may already have received held-out +/// setup or executed a command, so grade must tell the operator not to resume it. #[test] -fn an_edited_command_check_reports_the_result_it_reuses() { +fn an_incomplete_task_with_a_cached_command_check_warns_not_to_resume() { let (_tmp, root) = canonical_root(); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); @@ -173,6 +172,9 @@ fn an_edited_command_check_reports_the_result_it_reuses() { .args(["--skill", "mr-review", "--iteration", "1"]) .assert() .success() - .stderr(predicates::str::contains("held-out-tests")) - .stderr(predicates::str::contains("--overwrite")); + .stdout(predicates::str::contains("1 skipped (missing run.json)")) + .stderr(predicates::str::contains("pos-eval/with_skill")) + .stderr(predicates::str::contains("may already be contaminated")) + .stderr(predicates::str::contains("must not be resumed")) + .stderr(predicates::str::contains("fresh iteration")); } diff --git a/tests/cli/docs.rs b/tests/cli/docs.rs index db6e11e..53c32b4 100644 --- a/tests/cli/docs.rs +++ b/tests/cli/docs.rs @@ -234,6 +234,8 @@ fn docs_judging_keeps_bundle_bounds_truncation_and_retention_contract() { .stdout(contains("skill_should_trigger")) .stdout(contains("assertion_source")) .stdout(contains("eval-magic grade --overwrite")) + .stdout(contains("exact `run.json` digest")) + .stdout(contains("must not be resumed")) .stdout(contains("eval-magic dispatch --judges --overwrite")); // The isolation guide explains why the copy exists, so it has to carry the diff --git a/tests/run/command_check.rs b/tests/run/command_check.rs index 2ada5c2..72fc788 100644 --- a/tests/run/command_check.rs +++ b/tests/run/command_check.rs @@ -459,3 +459,4 @@ fn ingests_without_transcripts_while_guard_is_armed_and_aggregates() { } mod matrix; +mod partial_dispatch; diff --git a/tests/run/command_check/partial_dispatch.rs b/tests/run/command_check/partial_dispatch.rs new file mode 100644 index 0000000..e62226f --- /dev/null +++ b/tests/run/command_check/partial_dispatch.rs @@ -0,0 +1,170 @@ +use super::*; + +fn append_after_setup_command() -> String { + fixture(&[ + "--require-file", + "holdout/secret.txt", + "--text", + "x", + "--append", + "command-runs.txt", + ]) +} + +fn tree_snapshot(root: &Path) -> Vec<(PathBuf, Option>)> { + let mut snapshot = walk_paths(root) + .into_iter() + .map(|path| { + let relative = path.strip_prefix(root).unwrap().to_path_buf(); + let contents = path.is_file().then(|| fs::read(&path).unwrap()); + (relative, contents) + }) + .collect::>(); + snapshot.sort_by(|left, right| left.0.cmp(&right.0)); + snapshot +} + +#[test] +fn grades_only_completed_tasks_then_resumes_idempotently() { + let tmp = tempfile::TempDir::new().unwrap(); + let evals = json!({ + "skill_name": "mr-review", + "evals": [{ + "id": "held-out", + "prompt": "do the task", + "expected_output": "passes held-out check", + "skill_should_trigger": false, + "assertions": [{ + "id": "held-out-check", + "type": "command_check", + "setup_files": ["holdout/secret.txt"], + "command": append_after_setup_command() + }] + }] + }); + let (skill_dir, cwd) = setup(tmp.path(), &serde_json::to_string(&evals).unwrap()); + fs::create_dir_all(skill_dir.join("mr-review/evals/holdout")).unwrap(); + fs::write( + skill_dir.join("mr-review/evals/holdout/secret.txt"), + "secret", + ) + .unwrap(); + write_project_descriptor(&cwd); + + skill_eval() + .current_dir(&cwd) + .args(["run", "--skill-dir"]) + .arg(&skill_dir) + .args([ + "--skill", + "mr-review", + "--mode", + "new-skill", + "--harness", + "cool-custom-harness", + ]) + .assert() + .success(); + + let tasks = dispatch_tasks(&cwd); + assert_eq!(tasks.len(), 2); + let completed = &tasks[0]; + let incomplete = &tasks[1]; + write_cool_task_result(&cwd, completed); + + let completed_root = resolve(&cwd, completed["eval_root"].as_str().unwrap()); + let incomplete_root = resolve(&cwd, incomplete["eval_root"].as_str().unwrap()); + let completed_result = resolve(&cwd, completed["run_record_path"].as_str().unwrap()) + .parent() + .unwrap() + .join("command-checks/held-out-check.json"); + let incomplete_result = resolve(&cwd, incomplete["run_record_path"].as_str().unwrap()) + .parent() + .unwrap() + .join("command-checks/held-out-check.json"); + let incomplete_before = tree_snapshot(&incomplete_root); + + skill_eval() + .current_dir(&cwd) + .args(["ingest", "--skill-dir"]) + .arg(&skill_dir) + .args([ + "--skill", + "mr-review", + "--harness", + "cool-custom-harness", + "--iteration", + "1", + ]) + .assert() + .success() + .stdout(contains( + "Command checks: 1 executed, 0 reused, 0 failed, 1 skipped (missing run.json)", + )); + + assert!(completed_result.exists()); + assert_eq!( + fs::read_to_string(completed_root.join("command-runs.txt")).unwrap(), + "x" + ); + assert!(!incomplete_result.exists()); + assert!(!incomplete_root.join("holdout/secret.txt").exists()); + assert!(!incomplete_root.join("command-runs.txt").exists()); + assert_eq!(tree_snapshot(&incomplete_root), incomplete_before); + + write_cool_task_result(&cwd, incomplete); + skill_eval() + .current_dir(&cwd) + .args(["ingest", "--skill-dir"]) + .arg(&skill_dir) + .args([ + "--skill", + "mr-review", + "--harness", + "cool-custom-harness", + "--iteration", + "1", + ]) + .assert() + .success() + .stdout(contains( + "Command checks: 1 executed, 1 reused, 0 failed, 0 skipped (missing run.json)", + )); + + assert_eq!( + fs::read_to_string(completed_root.join("command-runs.txt")).unwrap(), + "x" + ); + assert_eq!( + fs::read_to_string(incomplete_root.join("command-runs.txt")).unwrap(), + "x" + ); + assert!(incomplete_result.exists()); + + skill_eval() + .current_dir(&cwd) + .args(["ingest", "--skill-dir"]) + .arg(&skill_dir) + .args([ + "--skill", + "mr-review", + "--harness", + "cool-custom-harness", + "--iteration", + "1", + ]) + .assert() + .success() + .stdout(contains( + "Command checks: 0 executed, 2 reused, 0 failed, 0 skipped (missing run.json)", + )); + + assert_eq!( + fs::read_to_string(completed_root.join("command-runs.txt")).unwrap(), + "x" + ); + assert_eq!( + fs::read_to_string(incomplete_root.join("command-runs.txt")).unwrap(), + "x" + ); +}