From b2574dc9ddc71257a855feda3f2fa5ec23bdefaa Mon Sep 17 00:00:00 2001 From: samiamorwas Date: Mon, 31 Aug 2026 20:06:25 -0400 Subject: [PATCH 1/2] fix(cli): re-emit --harness-file in generated commands, warn on descriptor drift (#294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every follow-up command a run generates (RUNBOOK.md, Next: lines) dropped the global --harness-file flag, so an operator following the runbook silently resolved a different harness descriptor than the iteration was prepared with — while plugin-shadow.json kept the prep-time isolation declaration, producing a recorded-as-isolated run whose comparison is invalid. The selector also invented a --skill-dir the invocation never used, which stages every sibling skill ambiently: a different experiment. command_target_args now reproduces the invocation: --skill-dir only when the invocation supplied one (else --skill names the absolute skill subdir), an absolute --workspace-dir as before, and --harness-file when one was loaded (remembered at registry init, threaded through RunContext). Backstop: run records the resolved descriptor digest and the --harness-file path in conditions.json; dispatch and ingest compare the digest they resolve against the prep-time one and warn loudly on drift, naming the file to re-run with. --- docs/guides/byoh.md | 7 +- docs/guides/isolation.md | 7 ++ src/adapters/registry.rs | 28 +++++- src/cli/args.rs | 7 ++ src/cli/commands/pipeline.rs | 18 ++-- src/cli/commands/run.rs | 3 + src/cli/mod.rs | 157 ++++++++++++++++++++++++++----- src/cli/run/orchestrate/build.rs | 5 + src/core/context.rs | 16 ++-- src/core/fs.rs | 15 +++ src/core/types.rs | 13 +++ tests/cli/init.rs | 17 ++-- tests/run/byoh.rs | 118 +++++++++++++++++++++++ tests/run/staging.rs | 14 ++- 14 files changed, 376 insertions(+), 49 deletions(-) diff --git a/docs/guides/byoh.md b/docs/guides/byoh.md index 15ecfa7..e1d3fc6 100644 --- a/docs/guides/byoh.md +++ b/docs/guides/byoh.md @@ -36,7 +36,12 @@ For a descriptor that should not live in the project, pass it directly: eval-magic run --harness-file ./cool-custom-harness.toml ``` -Its `label` becomes the default harness for that invocation. +Its `label` becomes the default harness for that invocation. Every follow-up command `run` +generates — the printed Next: steps and each `eval-magic …` line in RUNBOOK.md — re-emits +`--harness-file`, because the descriptor can decide whether a comparison is valid (dispatch +templates, shadow isolation). Follow the generated commands verbatim: `run` records the resolved +descriptor's digest in `conditions.json`, and `dispatch`/`ingest` warn loudly when a follow-up +resolves a different descriptor than the iteration was prepared with. ## Add a dispatch command first diff --git a/docs/guides/isolation.md b/docs/guides/isolation.md index a8140f7..ef8a954 100644 --- a/docs/guides/isolation.md +++ b/docs/guides/isolation.md @@ -94,6 +94,13 @@ intrinsic severity as provenance. `run` presents operator-environment findings a and `aggregate` omits those warnings only while no transcript evidence contradicts the declaration. Codebase-sourced findings remain warnings regardless of this descriptor setting. +The declaration is written from the descriptor resolved at prep time, so the remedy must ride along +with every follow-up. When the descriptor arrives via `--harness-file`, the generated RUNBOOK.md +and Next: commands re-emit the flag — follow them verbatim. Dropping it silently reverts to the +un-overlaid descriptor while the declaration stands, the comparison-invalid state this guide exists +to prevent; `dispatch` and `ingest` compare the resolved descriptor's digest against the prep-time +one in `conditions.json` and warn when they differ. + Do not set it when: - Any reported source remains discoverable. diff --git a/src/adapters/registry.rs b/src/adapters/registry.rs index 73bb4f7..7a9342e 100644 --- a/src/adapters/registry.rs +++ b/src/adapters/registry.rs @@ -7,7 +7,7 @@ //! behind the artifact `Deserialize`), [`Harness::known`] enumerates the //! entries, and [`adapter_for`] serves each handle's [`HarnessAdapter`]. -use std::path::Path; +use std::path::{Path, PathBuf}; use std::sync::{LazyLock, OnceLock}; use crate::core::Harness; @@ -79,6 +79,11 @@ pub enum RegistryInitError { /// via `--harness`. static SESSION_DEFAULT_HARNESS: OnceLock<&'static str> = OnceLock::new(); +/// When `--harness-file` is in play, its absolutized path — consulted by the +/// CLI when it renders generated follow-up commands, which must re-emit the +/// flag or they silently resolve a different descriptor (#294). +static SESSION_HARNESS_FILE: OnceLock = OnceLock::new(); + /// Initialize the registry from every descriptor layer: embedded built-ins → /// user-global (`/harnesses/*.toml`) → project-local /// (`/.eval-magic/harnesses/*.toml`) → the optional one-off @@ -100,6 +105,12 @@ pub fn init_registry(harness_file: Option<&Path>) -> Result<(), RegistryInitErro for warning in io_warnings.iter().chain(&built.warnings) { eprintln!("⚠ {warning}"); } + if let Some(file) = harness_file { + // The file was read during discovery, so it exists; fall back to the + // passed spelling only if resolution itself fails. + let resolved = crate::core::fs::real_path(file).unwrap_or_else(|_| file.to_path_buf()); + let _ = SESSION_HARNESS_FILE.set(resolved); + } if harness_file.is_some() && let Some(entry) = built .entries @@ -337,6 +348,21 @@ pub fn descriptor_value_for(harness: Harness) -> &'static serde_json::Value { .value } +/// The absolutized `--harness-file` path this session loaded, if any. +pub fn session_harness_file() -> Option<&'static Path> { + SESSION_HARNESS_FILE.get().map(PathBuf::as_path) +} + +/// A stable digest of the fully-resolved (layer-merged) descriptor behind +/// `harness`, over its canonical JSON form. A run records it at prep time so +/// a later invocation can tell whether it resolved the same descriptor the +/// run was prepared with (#294). +pub fn descriptor_digest(harness: Harness) -> String { + let canonical = serde_json::to_string(descriptor_value_for(harness)) + .expect("a descriptor value serializes"); + crate::core::fs::fnv1a_hex(canonical.as_bytes()) +} + /// True when `harness` has an embedded built-in descriptor among its sources /// — i.e. it is not defined by user-supplied descriptor files alone. Preflight /// uses this to hard-reject `--guard` on user-only harnesses (the write guard diff --git a/src/cli/args.rs b/src/cli/args.rs index 1bd7bd7..43a2460 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -44,6 +44,13 @@ pub(crate) struct Cli { /// label becomes the invocation's default harness. Unlike discovered /// descriptor files (skipped with a warning when broken), errors in this /// explicitly named file are fatal. + /// + /// Every command `run` generates — the printed Next: steps and the + /// RUNBOOK.md — re-emits this flag, because the descriptor it loads can + /// decide whether a comparison is valid (dispatch templates, shadow + /// isolation). Follow those commands verbatim: `run` records the + /// prep-time descriptor digest in conditions.json, and `dispatch`/`ingest` + /// warn when a follow-up resolves a different one. #[arg(long, global = true, value_name = "PATH")] pub harness_file: Option, #[command(subcommand)] diff --git a/src/cli/commands/pipeline.rs b/src/cli/commands/pipeline.rs index 8098283..6d28d9f 100644 --- a/src/cli/commands/pipeline.rs +++ b/src/cli/commands/pipeline.rs @@ -7,7 +7,10 @@ use anyhow::bail; use crate::cli::args::{CommonArgs, GradeArgs}; use crate::cli::command_target_args; use crate::cli::run; -use crate::cli::{iteration_dir, resolve_iteration, run_context_from, staged_env_roots}; +use crate::cli::{ + harness_descriptor_drift_warning, iteration_dir, resolve_iteration, run_context_from, + staged_env_roots, +}; use crate::core::RunContext; use crate::pipeline; use crate::sandbox; @@ -59,6 +62,13 @@ fn run_step(step: &run::steps::StepCommand) -> anyhow::Result<()> { pub(crate) fn run_ingest(args: CommonArgs) -> anyhow::Result<()> { let ctx = run_context_from(&args)?; let iteration = resolve_iteration(&ctx, args.iteration)?; + let dir = ctx + .workspace_root + .join(&ctx.skill_name) + .join(format!("iteration-{iteration}")); + if let Some(warning) = harness_descriptor_drift_warning(&ctx, &dir) { + eprintln!("⚠ {warning}"); + } let steps = run::steps::build_ingest_commands(&run::steps::StepParams { skill_dir: args.skill_dir.as_deref(), @@ -73,11 +83,7 @@ pub(crate) fn run_ingest(args: CommonArgs) -> anyhow::Result<()> { ); } - let judge_path = ctx - .workspace_root - .join(&ctx.skill_name) - .join(format!("iteration-{iteration}")) - .join("judge-tasks.json"); + let judge_path = dir.join("judge-tasks.json"); let total_tasks = std::fs::read_to_string(&judge_path) .ok() .and_then(|s| serde_json::from_str::(&s).ok()) diff --git a/src/cli/commands/run.rs b/src/cli/commands/run.rs index f02f964..6ed8025 100644 --- a/src/cli/commands/run.rs +++ b/src/cli/commands/run.rs @@ -64,6 +64,9 @@ pub(crate) fn run_run(args: RunArgs) -> anyhow::Result<()> { pub(crate) fn run_dispatch(args: DispatchArgs) -> anyhow::Result<()> { let ctx = run_context_from(&args.common)?; let iteration_dir = iteration_dir(&ctx, args.common.iteration)?; + if let Some(warning) = crate::cli::harness_descriptor_drift_warning(&ctx, &iteration_dir) { + eprintln!("⚠ {warning}"); + } // `--timeout 0` means "no deadline", which is the only way to say it with a // plain seconds flag. let timeout = (args.timeout > 0).then(|| std::time::Duration::from_secs(args.timeout)); diff --git a/src/cli/mod.rs b/src/cli/mod.rs index e30fcbc..0fa5fd7 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -153,6 +153,7 @@ pub(crate) fn run_context_with_bootstrap( bootstrap, workspace_dir: args.workspace_dir.clone(), harness, + harness_file: crate::adapters::registry::session_harness_file().map(Path::to_path_buf), cwd: None, })?; // `core` returns warnings rather than printing them; this is the one place @@ -175,21 +176,69 @@ pub(crate) fn parse_id_list(v: Option<&str>) -> Option> { /// Render a fully self-sufficient target selector for the current run context. /// -/// Always names `--skill-dir`, `--skill`, and `--workspace-dir` (all three are -/// always populated in [`RunContext`] and always re-resolve), so the printed -/// "Next:" commands are copy-pasteable from any cwd — not just the one `run` -/// happened to start in. The absolute `--workspace-dir` is what lets the human -/// run `ingest`/`finalize` from a per-`(group, condition)` env dir: without it, -/// `workspace_root` would fall back to the derived default (`detect_run_context`), -/// which is keyed on the skill directory rather than on the cwd, and the -/// iteration tree above the env would not resolve. +/// The selector reproduces the invocation that built the context, so a printed +/// "Next:" command is copy-pasteable from any cwd AND re-runs the same +/// experiment (#294): +/// +/// * `--skill-dir … --skill …` only when the invocation used `--skill-dir` +/// (`stage_siblings`); otherwise `--skill` names the absolute skill subdir. +/// Inventing a `--skill-dir` would stage every sibling skill ambiently — a +/// different experiment from the one just prepared. +/// * an absolute `--workspace-dir`, so the human can run `ingest`/`finalize` +/// from a per-`(group, condition)` env dir: without it, `workspace_root` +/// would fall back to the derived default (`detect_run_context`), which is +/// keyed on the skill directory rather than on the cwd, and the iteration +/// tree above the env would not resolve. +/// * `--harness-file …` when the invocation loaded one; dropping it silently +/// resolves a different descriptor than the run was prepared with. pub(crate) fn command_target_args(ctx: &RunContext) -> String { - format!( - " --skill-dir {} --skill {} --workspace-dir {}", - artifact_path(&ctx.skill_dir), - ctx.skill_name, - artifact_path(&ctx.workspace_root), - ) + let mut args = String::new(); + if ctx.stage_siblings { + args.push_str(&format!( + " --skill-dir {} --skill {}", + artifact_path(&ctx.skill_dir), + ctx.skill_name + )); + } else { + args.push_str(&format!(" --skill {}", artifact_path(&ctx.skill_subdir))); + } + args.push_str(&format!( + " --workspace-dir {}", + artifact_path(&ctx.workspace_root) + )); + if let Some(file) = &ctx.harness_file { + args.push_str(&format!(" --harness-file {}", artifact_path(file))); + } + args +} + +/// The warn-loudly backstop for #294. Post-prep stages resolve the harness +/// descriptor by label, so a follow-up that drops `--harness-file` (or runs +/// where project descriptor layers differ) silently switches descriptors +/// mid-campaign while the iteration's artifacts keep the prep-time +/// declarations. Returns the warning when this invocation's resolved +/// descriptor differs from the one the iteration was prepared with; `None` +/// when they match, or when the iteration predates descriptor provenance. +pub(crate) fn harness_descriptor_drift_warning( + ctx: &RunContext, + iteration_dir: &Path, +) -> Option { + let raw = std::fs::read_to_string(iteration_dir.join("conditions.json")).ok()?; + let conditions: crate::core::ConditionsRecord = serde_json::from_str(&raw).ok()?; + let prepared = conditions.harness_descriptor_digest?; + let current = crate::adapters::registry::descriptor_digest(ctx.harness); + if prepared == current { + return None; + } + let label = ctx.harness.name(); + Some(match (&conditions.harness_file, &ctx.harness_file) { + (Some(file), None) => format!( + "harness descriptor drift: this iteration was prepared with --harness-file {file} (descriptor digest {prepared}), but this invocation resolved '{label}' as digest {current}. Re-run with --harness-file {file}: the iteration's dispatch templates and shadow declarations came from that descriptor." + ), + _ => format!( + "harness descriptor drift: '{label}' resolves to digest {current}, but this iteration was prepared with digest {prepared} — descriptor files changed since prep. Stages resolve the descriptor by label, so continuing mixes two descriptors in one comparison." + ), + }) } /// Resolve the explicit iteration, or default to the latest existing @@ -279,10 +328,13 @@ mod tests { subdir } - /// The selector must be copy-pasteable: even when `run` was invoked from - /// inside the skill dir (the case that used to render an empty selector), it - /// must name both `--skill-dir` and `--skill`, and re-resolve to the same - /// skill from an unrelated cwd. + /// The selector must be copy-pasteable *and* behavior-preserving (#294): + /// even when `run` was invoked from inside the skill dir (the case that + /// used to render an empty selector), it names `--skill` as an absolute + /// path and re-resolves to the same skill from an unrelated cwd. It must + /// NOT invent a `--skill-dir` the invocation never used — `--skill-dir` + /// sets `stage_siblings`, so re-running from that selector would stage + /// every sibling skill ambiently and run a different experiment. #[test] fn target_args_are_self_sufficient_when_run_from_inside_skill_dir() { let tmp = TempDir::new().unwrap(); @@ -298,26 +350,81 @@ mod tests { let args = command_target_args(&ctx); assert!( - args.contains("--skill-dir"), - "selector names --skill-dir: {args}" + !args.contains("--skill-dir"), + "an invocation without --skill-dir must not gain one: {args}" ); assert!( - args.contains("--skill mr-review"), - "selector names --skill: {args}" + args.contains(&format!("--skill {}", artifact_path(&skill_subdir))), + "selector names --skill as an absolute path: {args}" ); // Round-trip: feeding the rendered selector back from an unrelated cwd - // resolves the same skill. + // resolves the same skill with the same staging behavior. let other = root.join("elsewhere"); fs::create_dir_all(&other).unwrap(); let resolved = detect_run_context(DetectInput { - skill_dir: Some(ctx.skill_dir.display().to_string()), - skill: Some(ctx.skill_name.clone()), + skill: Some(ctx.skill_subdir.display().to_string()), cwd: Some(other), ..Default::default() }) .unwrap(); assert_eq!(resolved.skill_subdir, ctx.skill_subdir); + assert!(!resolved.stage_siblings); + } + + /// `--skill-dir --skill ` is the sibling-staging form, so when + /// the invocation used it the selector reproduces it exactly. + #[test] + fn target_args_keep_skill_dir_when_the_invocation_used_it() { + let tmp = TempDir::new().unwrap(); + let root = fs::canonicalize(tmp.path()).unwrap(); + let skill_subdir = make_skill(&root, "skills", "mr-review"); + let skill_dir = skill_subdir.parent().unwrap().to_path_buf(); + + let ctx = detect_run_context(DetectInput { + skill_dir: Some(skill_dir.display().to_string()), + skill: Some("mr-review".to_string()), + cwd: Some(root.clone()), + ..Default::default() + }) + .unwrap(); + + let args = command_target_args(&ctx); + assert!( + args.contains(&format!("--skill-dir {}", artifact_path(&skill_dir))), + "selector keeps --skill-dir: {args}" + ); + assert!(args.contains("--skill mr-review"), "{args}"); + } + + /// A run prepared with `--harness-file` must re-emit the flag in every + /// generated follow-up command, or the follow-up silently resolves a + /// different descriptor (#294). + #[test] + fn target_args_reemit_harness_file() { + let tmp = TempDir::new().unwrap(); + let root = fs::canonicalize(tmp.path()).unwrap(); + let skill_subdir = make_skill(&root, "skills", "mr-review"); + let harness_file = root.join("overlay.toml"); + fs::write( + &harness_file, + r#"label = "claude-code" +"#, + ) + .unwrap(); + + let ctx = detect_run_context(DetectInput { + cwd: Some(skill_subdir), + harness_file: Some(harness_file.clone()), + ..Default::default() + }) + .unwrap(); + + let args = command_target_args(&ctx); + assert!( + args.contains(&format!("--harness-file {}", artifact_path(&harness_file))), + "selector re-emits --harness-file: {args}" + ); } /// The human runs `ingest`/`finalize` from a per-`(group, condition)` env dir. diff --git a/src/cli/run/orchestrate/build.rs b/src/cli/run/orchestrate/build.rs index 8fe2a7f..9834434 100644 --- a/src/cli/run/orchestrate/build.rs +++ b/src/cli/run/orchestrate/build.rs @@ -83,6 +83,11 @@ pub(super) fn write_dispatch( label: opts.label.map(str::to_owned), codebases: r.codebases.iter().map(super::RunCodebase::usage).collect(), skill_source: Some(r.skill.record()), + // Prep-time descriptor provenance (#294): the digest lets a later + // stage detect that it resolved a different descriptor than this run + // was prepared with; the path makes the remedy copy-pasteable. + harness_file: ctx.harness_file.as_deref().map(artifact_path), + harness_descriptor_digest: Some(crate::adapters::registry::descriptor_digest(ctx.harness)), }; write_json(&r.iteration_dir.join("conditions.json"), &conditions)?; // One record, cloned into every task: grading reads `run.json` alone, so a diff --git a/src/core/context.rs b/src/core/context.rs index 609ba2d..9ca6db9 100644 --- a/src/core/context.rs +++ b/src/core/context.rs @@ -55,6 +55,11 @@ pub struct RunContext { pub stage_root: PathBuf, pub bootstrap_path: Option, pub harness: Harness, + /// The absolutized `--harness-file` descriptor this invocation loaded, if + /// any. Carried so generated follow-up commands can re-emit the flag + /// (#294) and the iteration can record which descriptor it was prepared + /// with. + pub harness_file: Option, /// Things the operator should know about how this context resolved. `core` /// never prints; `cli::run_context_with_bootstrap` owns the `⚠ ` prefix. pub warnings: Vec, @@ -71,6 +76,9 @@ pub struct DetectInput { pub bootstrap: Option, pub workspace_dir: Option, pub harness: Option, + /// The `--harness-file` this invocation loaded, already absolutized by the + /// registry init layer; passed through to [`RunContext`] untouched. + pub harness_file: Option, pub cwd: Option, } @@ -196,12 +204,7 @@ fn workspace_slug(skill_dir: &Path) -> String { /// that every generated command embeds; a toolchain upgrade silently relocating /// someone's workspace is the one failure it must not have. fn path_digest(path: &Path) -> String { - let mut hash: u64 = 0xcbf2_9ce4_8422_2325; - for byte in path.to_string_lossy().as_bytes() { - hash ^= u64::from(*byte); - hash = hash.wrapping_mul(0x0000_0100_0000_01b3); - } - format!("{hash:016x}")[..8].to_string() + crate::core::fs::fnv1a_hex(path.to_string_lossy().as_bytes())[..8].to_string() } /// Resolve the eval home from explicit/environment inputs: `$EVAL_MAGIC_WORKSPACE_DIR` @@ -361,6 +364,7 @@ pub fn detect_run_context(input: DetectInput) -> Result String { path.to_string_lossy().into_owned() } +/// FNV-1a over `bytes`, as 16 hex characters. +/// +/// Hand-rolled rather than `DefaultHasher`, which carries no stability +/// guarantee across Rust releases: digests that outlive a process (workspace +/// slugs, prep-time descriptor provenance) must not shift under a toolchain +/// upgrade. +pub fn fnv1a_hex(bytes: &[u8]) -> String { + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + for byte in bytes { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(0x0000_0100_0000_01b3); + } + format!("{hash:016x}") +} + /// The one spelling of `path` that every participant in a run agrees on. /// /// `getcwd` and `canonicalize` resolve symlink aliases, so resolving once at the diff --git a/src/core/types.rs b/src/core/types.rs index c54e97d..d37e6d2 100644 --- a/src/core/types.rs +++ b/src/core/types.rs @@ -485,6 +485,17 @@ pub struct ConditionsRecord { /// still round-trips. #[serde(default, skip_serializing_if = "Option::is_none")] pub skill_source: Option, + /// The `--harness-file` descriptor the iteration was prepared with + /// (absolute, wire-format path). With `harness_descriptor_digest`, this + /// lets a later stage detect a follow-up invocation that resolves a + /// different descriptor than the run was prepared with (#294). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub harness_file: Option, + /// FNV-1a hex of the fully-resolved harness descriptor's canonical JSON at + /// prep time. Absent in records written before descriptor provenance + /// existed, which read as "nothing to compare against". + #[serde(default, skip_serializing_if = "Option::is_none")] + pub harness_descriptor_digest: Option, } /// Comparison mode for a run. @@ -892,6 +903,8 @@ mod tests { label: None, codebases: Vec::new(), skill_source: None, + harness_file: None, + harness_descriptor_digest: None, }; let out = serde_json::to_value(&rec).unwrap(); assert_eq!(out.get("mode"), Some(&Value::String("new-skill".into()))); diff --git a/tests/cli/init.rs b/tests/cli/init.rs index e807c6a..c63af41 100644 --- a/tests/cli/init.rs +++ b/tests/cli/init.rs @@ -249,12 +249,14 @@ fn init_rejects_a_missing_or_non_directory_codebase_before_prompting() { } /// Even when `init` runs from inside the skill dir, the printed "Next:" commands -/// must be copy-pasteable: each carries `--skill-dir`/`--skill` so it resolves -/// from any cwd. +/// must be copy-pasteable AND behavior-preserving (#294): `--skill` names the +/// absolute skill subdir so the command resolves from any cwd, and no +/// `--skill-dir` is invented (it would stage sibling skills ambiently). #[test] fn init_from_skill_dir_prints_copy_pasteable_next_steps() { let (_tmp, root) = canonical_root(); let (_skill_dir, skill_sub) = write_skill(&root); + let skill_flag = format!("--skill {}", skill_sub.display()); skill_eval() .current_dir(&skill_sub) @@ -269,13 +271,14 @@ fn init_from_skill_dir_prints_copy_pasteable_next_steps() { ]) .assert() .success() - .stdout(contains(" eval-magic run --skill-dir")) - .stdout(contains("--skill mr-review --workspace-dir")) + .stdout(contains(format!(" eval-magic run {skill_flag}"))) + .stdout(contains("--workspace-dir")) + .stdout(contains("--skill-dir").not()) .stdout(contains("--guard").not()) .stdout(contains("follow the generated RUNBOOK.md")) - .stdout(contains(" eval-magic ingest --skill-dir").not()) - .stdout(contains(" eval-magic finalize --skill-dir").not()) - .stdout(contains(" eval-magic promote-baseline --skill-dir").not()); + .stdout(contains(" eval-magic ingest").not()) + .stdout(contains(" eval-magic finalize").not()) + .stdout(contains(" eval-magic promote-baseline").not()); } #[test] diff --git a/tests/run/byoh.rs b/tests/run/byoh.rs index b8d5115..0595efd 100644 --- a/tests/run/byoh.rs +++ b/tests/run/byoh.rs @@ -428,3 +428,121 @@ exec_template = "cool-cli run --cd > = runbook + .lines() + .filter(|line| line.starts_with("eval-magic ")) + .collect(); + assert!(!commands.is_empty(), "the runbook carries commands"); + for command in commands { + assert!( + command.contains(&flag), + "every runbook command re-emits --harness-file: {command}" + ); + } + + // Prep-time provenance for the drift backstop: the descriptor the run was + // prepared with is recorded next to the conditions it produced. + let conditions = read_json(&iteration_dir(&cwd).join("conditions.json")); + assert_eq!(conditions["harness_file"], wire_path(&file)); + let digest = conditions["harness_descriptor_digest"] + .as_str() + .expect("conditions record the resolved descriptor digest"); + assert_eq!(digest.len(), 16, "FNV-1a hex digest: {digest}"); +} + +/// Issue #294 backstop: a follow-up stage that resolves a descriptor different +/// from the prep-time one warns loudly instead of silently switching. The +/// overlay keeps the built-in label so the flag-less follow-up *resolves* +/// (against the un-overlaid built-in) instead of failing on an unknown label. +#[test] +fn dispatch_and_ingest_warn_when_the_resolved_descriptor_drifted_from_prep() { + let tmp = tempfile::TempDir::new().unwrap(); + let (skill_dir, cwd) = setup(tmp.path(), DEFAULT_EVALS); + // Overlay the claude-code label, retuning dispatch onto a missing binary so + // nothing real is ever spawned; every other field merges from the built-in. + let file = tmp.path().join("iso.toml"); + fs::write( + &file, + r#"label = "claude-code" + +[dispatch] +exec_template = "definitely-missing-cli " +"#, + ) + .unwrap(); + + skill_eval() + .current_dir(&cwd) + .args(["run", "--skill-dir"]) + .arg(&skill_dir) + .args(["--skill", "mr-review", "--mode", "new-skill"]) + .arg("--harness-file") + .arg(&file) + .assert() + .success(); + + // The flag-less follow-up resolves the un-overlaid built-in descriptor: + // the digest no longer matches the prep-time one. + skill_eval() + .current_dir(&cwd) + .args(["dispatch", "--skill-dir"]) + .arg(&skill_dir) + .args(["--skill", "mr-review", "--iteration", "1"]) + .assert() + .stderr( + contains("harness descriptor drift") + .and(contains("--harness-file")) + .and(contains(wire_path(&file))), + ); + skill_eval() + .current_dir(&cwd) + .args(["ingest", "--skill-dir"]) + .arg(&skill_dir) + .args(["--skill", "mr-review", "--iteration", "1"]) + .assert() + .stderr(contains("harness descriptor drift")); + + // Re-emitting the flag resolves the same descriptor the run was prepared + // with: digests match, no drift warning. (Dispatch still fails: the + // overlaid exec template names a missing binary.) + skill_eval() + .current_dir(&cwd) + .arg("--harness-file") + .arg(&file) + .args(["dispatch", "--skill-dir"]) + .arg(&skill_dir) + .args(["--skill", "mr-review", "--iteration", "1"]) + .assert() + .stderr(contains("harness descriptor drift").not()); +} diff --git a/tests/run/staging.rs b/tests/run/staging.rs index 888538d..9e890f2 100644 --- a/tests/run/staging.rs +++ b/tests/run/staging.rs @@ -138,10 +138,18 @@ fn run_from_skill_dir_defaults_to_new_skill_without_staging_siblings() { // Run from inside the skill dir with no args: the auto-derived target selector // (`command_target_args`) is threaded into the RUNBOOK's pipeline commands. The - // RUNBOOK lives in the iteration dir (Cli dispatch has no single env/). + // RUNBOOK lives in the iteration dir (Cli dispatch has no single env/). The + // invocation used no --skill-dir, so the selector must not invent one: + // --skill-dir stages sibling skills, changing the experiment (#294). let runbook = read_str(&direct_iteration_dir(&skill_sub).join("RUNBOOK.md")); - assert!(runbook.contains("ingest --skill-dir")); - assert!(runbook.contains("--skill mr-review --workspace-dir")); + assert!( + !runbook.contains("--skill-dir"), + "the selector must not add --skill-dir the invocation never used: {runbook}" + ); + assert!( + runbook.contains(&format!("ingest --skill {}", wire_path(&skill_sub))), + "the selector names --skill as an absolute path: {runbook}" + ); assert!(runbook.contains("--iteration 1")); let dispatch = read_json(&direct_iteration_dir(&skill_sub).join("dispatch.json")); From 4f2b1ede1d71f40045db2a0788cfb5fdcae6f9c6 Mon Sep 17 00:00:00 2001 From: samiamorwas Date: Mon, 31 Aug 2026 21:51:31 -0400 Subject: [PATCH 2/2] docs(harness-file): state the drift check plainly in help and guides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Describe the mechanism — dispatch/ingest compare the resolved descriptor against the digest run records in conditions.json and warn when they differ — instead of the process label "warn loudly". --- docs/guides/byoh.md | 6 +++--- docs/guides/isolation.md | 4 ++-- src/cli/args.rs | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/guides/byoh.md b/docs/guides/byoh.md index e1d3fc6..b377c6a 100644 --- a/docs/guides/byoh.md +++ b/docs/guides/byoh.md @@ -39,9 +39,9 @@ eval-magic run --harness-file ./cool-custom-harness.toml Its `label` becomes the default harness for that invocation. Every follow-up command `run` generates — the printed Next: steps and each `eval-magic …` line in RUNBOOK.md — re-emits `--harness-file`, because the descriptor can decide whether a comparison is valid (dispatch -templates, shadow isolation). Follow the generated commands verbatim: `run` records the resolved -descriptor's digest in `conditions.json`, and `dispatch`/`ingest` warn loudly when a follow-up -resolves a different descriptor than the iteration was prepared with. +templates, shadow isolation). Follow the generated commands verbatim: dropping the flag resolves a +different descriptor, and `dispatch`/`ingest` compare the resolved descriptor against the digest +`run` records in `conditions.json` and warn when the two differ. ## Add a dispatch command first diff --git a/docs/guides/isolation.md b/docs/guides/isolation.md index ef8a954..cd13cdc 100644 --- a/docs/guides/isolation.md +++ b/docs/guides/isolation.md @@ -98,8 +98,8 @@ The declaration is written from the descriptor resolved at prep time, so the rem with every follow-up. When the descriptor arrives via `--harness-file`, the generated RUNBOOK.md and Next: commands re-emit the flag — follow them verbatim. Dropping it silently reverts to the un-overlaid descriptor while the declaration stands, the comparison-invalid state this guide exists -to prevent; `dispatch` and `ingest` compare the resolved descriptor's digest against the prep-time -one in `conditions.json` and warn when they differ. +to prevent. `dispatch` and `ingest` compare the resolved descriptor against the digest `run` +records in `conditions.json` and warn when the two differ. Do not set it when: diff --git a/src/cli/args.rs b/src/cli/args.rs index 43a2460..331d7c9 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -48,9 +48,9 @@ pub(crate) struct Cli { /// Every command `run` generates — the printed Next: steps and the /// RUNBOOK.md — re-emits this flag, because the descriptor it loads can /// decide whether a comparison is valid (dispatch templates, shadow - /// isolation). Follow those commands verbatim: `run` records the - /// prep-time descriptor digest in conditions.json, and `dispatch`/`ingest` - /// warn when a follow-up resolves a different one. + /// isolation). Follow those commands verbatim: `run` records the resolved + /// descriptor's digest in conditions.json, and `dispatch`/`ingest` warn + /// when a follow-up resolves a different one. #[arg(long, global = true, value_name = "PATH")] pub harness_file: Option, #[command(subcommand)]