fix(delivery): the on_green_tests gate ran the suite in the live checkout

Fourth instance of the same defect, and the last of the three commands that run
as root against a mission tree.

`verify_tests` execs the project's test command with `workdir = repo` — the live
checkout — inside a container running as ROOT. `cargo test` writes `target/`, so
the checkout ends up owned by two uids and the next phase's cargo hits
permission-denied. The harness reported `uids=0,65532` the first time this gate
ever ran end to end.

It survived because it had never run. Every one of the ten harness fixtures used
`commit_policy: "always"`; `on_green_tests` and `on_reviewer_approval` were
parsed, implemented, and never exercised — and `Gate`'s own doc already records
that three recipes carried this policy while it "did precisely nothing" for want
of a reader. A policy that is never exercised is indistinguishable from one that
is ignored.

Consolidated rather than fixed a third time. `root_copy` now owns the pattern —
copy through `mission_fs::pack_dir` into a SIBLING of the mission dir, run there,
and purge FROM INSIDE THE CONTAINER, because the copy's `target/` is root-owned
and the server (uid 65532) cannot delete it. `benchmark_runner` moved onto it;
`evaluator_tools::Sandbox` keeps its own copy logic for now (it carries an
allow-list and a judge-facing API, so folding it in is a larger change than this
moment warrants — noted, not done).

The gate fails CLOSED if the copy cannot be made: an unverifiable suite must not
license a push.

Also adds the `refactor` scenario, which is what found this. I had written it off
as "structurally identical to four existing scenarios" — wrong: it is the only
recipe carrying `on_green_tests`, and that made it the only one testing this
code path at all.

245 lib tests, 20 test binaries.

crates/cm-api/src/benchmark_runner.rs

@@ -171,11 +171,11 @@ pub async fn run(
     // same way: the harness's uid probe, reporting `uids=0,65532`. Measurement
     // must not mutate what it measures — the rule this codebase already applies
     // to the judge and to the `verifier` subagent.
-    let copy_root = bench_copy_path(mission_id);
+    let copy_root = crate::root_copy::copy_root("_bench", mission_id);
     // A stale copy from a previous run is ROOT-owned (see `purge_copy`), so it
     // must be removed the same way it was created — from inside the container.
-    purge_copy(&container, &copy_root).await;
+    crate::root_copy::purge(&container, &copy_root).await;
-    let copy = BenchCopy::of(&workdir, &copy_root)?;
+    let copy = crate::root_copy::RootCopy::of(&workdir, &copy_root)?;
     let cmd = harness.command();
     let result = docker_exec(&container, copy.workdir(), &cmd)
         .await
@@ -184,7 +184,7 @@ pub async fn run(
     // writes `target/` as root, and the server process is uid 65532: its
     // `remove_dir_all` cannot delete root-owned files and silently leaves the
     // whole copy behind — measured at 1.2 MB per run, growing forever.
-    purge_copy(&container, &copy_root).await;
+    crate::root_copy::purge(&container, &copy_root).await;
     let raw = result?;
     let metrics = parse_output(&raw, &harness);
     Ok((metrics, harness.driver_name().to_string()))
@@ -259,74 +259,6 @@ async fn phase_config(pool: &PgPool, phase_id: Uuid) -> Result<Value, String> {
 
 /// Post-task-#23: shared runtime container + per-mission working dir.
 /// See security_scan::exec_target for the same convention.
-/// `<missions_root>/_bench/<mission>` — a sibling of the per-mission dirs, like
-/// `_verify` and `_outputs`, so reaping a mission never races a running bench.
-fn bench_copy_path(mission_id: Uuid) -> std::path::PathBuf {
-    crate::mission_workspace::missions_root()
-        .join("_bench")
-        .join(mission_id.to_string())
-}
-
-/// Delete a benchmark copy from inside the container, as root.
-///
-/// The copy's `target/` belongs to root because the benchmark that created it
-/// ran as root. `std::fs::remove_dir_all` from the server (uid 65532) fails on
-/// those files, so the tree survives — quietly, because the error was
-/// discarded. Removing it where it was written is the only thing that works.
-async fn purge_copy(container: &str, root: &std::path::Path) {
-    let cmd = vec![
-        "rm".to_string(),
-        "-rf".to_string(),
-        root.display().to_string(),
-    ];
-    if let Err(e) = docker_exec(container, std::path::Path::new("/"), &cmd).await {
-        eprintln!(
-            "benchmark_runner: could not remove bench copy {}: {e}",
-            root.display()
-        );
-    }
-}
-
-/// A throwaway copy of the checkout for one benchmark run.
-///
-/// Removed on drop, including on the error paths — a `target/` left behind is
-/// both disk and a stale tree a later run could measure by mistake.
-struct BenchCopy {
-    root: std::path::PathBuf,
-    workdir: std::path::PathBuf,
-}
-
-impl BenchCopy {
-    fn of(source: &std::path::Path, root: &std::path::Path) -> Result<Self, String> {
-        // Through the transport packer, so the copy carries exactly what the
-        // delivered diff carries: no `target/`, no `node_modules/`. One
-        // exclusion list, now four consumers.
-        let _ = std::fs::remove_dir_all(root);
-        let archive = crate::mission_fs::pack_dir(source, "repo")
-            .map_err(|e| format!("pack checkout for benchmark: {e}"))?;
-        crate::mission_fs::unpack_into(&archive, root)
-            .map_err(|e| format!("unpack benchmark copy: {e}"))?;
-        let workdir = root.join("repo");
-        if !workdir.is_dir() {
-            return Err(format!("benchmark copy missing at {}", workdir.display()));
-        }
-        Ok(Self { root: root.to_path_buf(), workdir })
-    }
-
-    fn workdir(&self) -> &std::path::Path {
-        &self.workdir
-    }
-}
-
-impl Drop for BenchCopy {
-    /// Best-effort only. This CANNOT remove the root-owned `target/` a benchmark
-    /// leaves behind — `purge_copy` is what actually clears it, and this stays
-    /// as a fallback for the early-error paths where nothing ran as root yet.
-    fn drop(&mut self) {
-        let _ = std::fs::remove_dir_all(&self.root);
-    }
-}
-
 async fn exec_target(
     pool: &PgPool,
     mission_id: Uuid,
@@ -508,7 +440,7 @@ mod bench_copy_tests {
     #[test]
     fn a_benchmark_runs_in_a_copy_outside_the_mission_directory() {
         let mission = Uuid::now_v7();
-        let copy = bench_copy_path(mission);
+        let copy = crate::root_copy::copy_root("_bench", mission);
         let live = crate::mission_workspace::checkout_path(mission);
         assert_ne!(copy, live, "the bench copy must not be the checkout");
         assert!(

crates/cm-api/src/lib.rs

@@ -39,6 +39,7 @@ pub mod mission_runtime;
 pub mod mission_workspace;
 pub mod node_rules;
 pub mod phase_runner;
+pub mod root_copy;
 pub mod phase_summarizer;
 pub mod quota;
 mod recursive_exec;

crates/cm-api/src/mission_delivery.rs

@@ -345,7 +345,29 @@ pub async fn capture_phase_diff_at(
             if gate == Gate::OnGreenTests {
                 let container = std::env::var("CLAWMATES_RUNTIME_CONTAINER")
                     .unwrap_or_else(|_| "clawmates-runtime".to_string());
-                let o = verify_tests(&repo, &container).await;
+                // Against a COPY, never the checkout. `verify_tests` execs
+                // `cargo test` in a container running as ROOT, which writes
+                // `target/` — in the live tree that leaves root-owned build
+                // output in a checkout owned by uid 65532 and breaks the
+                // single-writer invariant. Measured the first time this gate
+                // ever ran end to end: `uids=0,65532`.
+                //
+                // The gate had been implemented but never exercised (every
+                // harness fixture used `commit_policy: "always"`), which is why
+                // a bug this mechanical survived in it.
+                let gate_root = crate::root_copy::copy_root("_gate", mission_id);
+                crate::root_copy::purge(&container, &gate_root).await;
+                let o = match crate::root_copy::RootCopy::of(&repo, &gate_root) {
+                    Ok(copy) => {
+                        let r = verify_tests(copy.workdir(), &container).await;
+                        crate::root_copy::purge(&container, &gate_root).await;
+                        r
+                    }
+                    // Fail-closed: an unverifiable suite must not license a push.
+                    Err(e) => TestOutcome::CouldNotRun(format!(
+                        "could not copy the checkout to test it: {e}"
+                    )),
+                };
                 // An infrastructure fault must be loud. The gate degrades
                 // safely either way, but "we could not run the suite" is a
                 // problem with the platform and needs to look like one.

crates/cm-api/src/root_copy.rs

@@ -0,0 +1,137 @@
+//! A throwaway copy of a mission checkout, for commands that run as ROOT.
+//!
+//! Three places in this codebase run a real command against a mission's tree —
+//! the judge's verification (`evaluator_tools::Sandbox`), the benchmark runner,
+//! and the `on_green_tests` delivery gate. All three enter a container running as
+//! root with the missions root bind-mounted, and all three run something that
+//! writes `target/`.
+//!
+//! Run against the live checkout, that breaks the single-writer invariant: the
+//! tree is owned by uid 65532 and now contains root-owned build output, so the
+//! next phase's `cargo` hits permission-denied on a directory it cannot write.
+//! The harness's uid probe reports it as `uids=0,65532`.
+//!
+//! # The cleanup half, which is the part that keeps being got wrong
+//!
+//! The copy inherits the same problem: its `target/` is root-owned, so the
+//! server process (uid 65532) **cannot delete it**. A `Drop` calling
+//! `std::fs::remove_dir_all` fails, and because that error is discarded the tree
+//! survives forever — measured at 1.2 MB per benchmark run and 16 MB of stranded
+//! judge sandboxes before this existed.
+//!
+//! So removal goes back through the container, as root, where the files were
+//! written. `Drop` remains only as a fallback for the paths where nothing has
+//! run as root yet, and does not pretend to be more.
+
+use std::path::{Path, PathBuf};
+
+/// Where throwaway copies live: siblings of the per-mission directories, like
+/// `_outputs` and `_verify`, so reaping a mission cannot race a running command.
+pub fn copy_root(kind: &str, mission_id: uuid::Uuid) -> PathBuf {
+    crate::mission_workspace::missions_root()
+        .join(kind)
+        .join(mission_id.to_string())
+}
+
+/// Delete a copy from inside the container that wrote it.
+///
+/// Best-effort and loud: a housekeeping failure must not cost a real verdict or
+/// a real benchmark, but it must not be silent either — silence is how the leaks
+/// this module exists for went unnoticed for a day.
+pub async fn purge(container: &str, root: &Path) {
+    let Ok(docker) = crate::container_exec::connect() else {
+        return;
+    };
+    let argv = vec![
+        "rm".to_string(),
+        "-rf".to_string(),
+        root.display().to_string(),
+    ];
+    if let Err(e) = crate::container_exec::exec(
+        &docker,
+        container,
+        Some("/"),
+        &argv,
+        std::time::Duration::from_secs(120),
+    )
+    .await
+    {
+        eprintln!(
+            "root_copy: could not remove {} from {container}: {e}",
+            root.display()
+        );
+    }
+}
+
+/// A copy of a checkout, removed when it goes out of scope.
+pub struct RootCopy {
+    root: PathBuf,
+    workdir: PathBuf,
+}
+
+impl RootCopy {
+    /// Copy `source` into `root`, returning a handle whose `workdir` is the tree
+    /// to run in.
+    ///
+    /// Packed through `mission_fs::pack_dir`, so the copy carries exactly what a
+    /// delivered diff carries — no `target/`, no `node_modules/`. One exclusion
+    /// list, four consumers.
+    pub fn of(source: &Path, root: &Path) -> Result<RootCopy, String> {
+        let archive = crate::mission_fs::pack_dir(source, "repo")
+            .map_err(|e| format!("pack {} for a root-run command: {e}", source.display()))?;
+        crate::mission_fs::unpack_into(&archive, root)
+            .map_err(|e| format!("unpack copy into {}: {e}", root.display()))?;
+        let workdir = root.join("repo");
+        if !workdir.is_dir() {
+            return Err(format!("copy missing at {}", workdir.display()));
+        }
+        Ok(RootCopy {
+            root: root.to_path_buf(),
+            workdir,
+        })
+    }
+
+    pub fn workdir(&self) -> &Path {
+        &self.workdir
+    }
+}
+
+impl Drop for RootCopy {
+    /// Fallback only. This CANNOT remove root-owned build output — see
+    /// [`purge`], which is what actually clears a copy something has run in.
+    fn drop(&mut self) {
+        let _ = std::fs::remove_dir_all(&self.root);
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// A copy must be a SIBLING of the per-mission directory, never inside it:
+    /// `teardown_container` removes `<missions_root>/<mission_id>` wholesale and
+    /// would take a running command's tree with it.
+    #[test]
+    fn copies_live_beside_the_mission_directory_not_inside_it() {
+        let mission = uuid::Uuid::now_v7();
+        let mission_dir = crate::mission_workspace::missions_root().join(mission.to_string());
+        for kind in ["_bench", "_gate"] {
+            let root = copy_root(kind, mission);
+            assert!(!root.starts_with(&mission_dir), "{root:?}");
+            assert!(
+                root.starts_with(crate::mission_workspace::missions_root().join(kind)),
+                "{root:?}"
+            );
+        }
+    }
+
+    /// The copy is not the checkout. Stated as a test because the whole defect
+    /// class is "ran the real command against the real tree".
+    #[test]
+    fn a_copy_is_never_the_checkout() {
+        let mission = uuid::Uuid::now_v7();
+        let live = crate::mission_workspace::checkout_path(mission);
+        assert_ne!(copy_root("_bench", mission).join("repo"), live);
+        assert_ne!(copy_root("_gate", mission).join("repo"), live);
+    }
+}

scripts/verify-mission-delivery.sh

@@ -579,6 +579,54 @@ assert_security() { # <token> <mission> <report>
     || fail "security: nothing delivered mentions gitleaks — the scan may not have run"
 }
 
+# ── Scenario: refactor — the on_green_tests commit gate ───────────
+#
+# The fifth and last recipe, and NOT just another coding phase: it declares
+# `commit_policy = "on_green_tests"`, and all ten other fixtures use `"always"`.
+# So the gate that decides whether work is published on the mission branch or
+# diverted for review has never run end to end — and `Gate`'s own doc records
+# that three recipes carried this policy while it "did precisely nothing",
+# because it had no reader at all.
+#
+# A policy that is parsed but never exercised is indistinguishable from one that
+# is ignored. This runs it: tests pass, so the work must land on the mission
+# branch, not a review branch.
+REFACTOR_BODY=$(cat <<JSON
+{"title":"verify: the on_green_tests gate publishes when tests pass",
+ "template_kind":"refactor",
+ "team_template_id":"$TEAM_TEMPLATE",
+ "repo_id":"$REPO_ID",
+ "description":"Prove commit_policy=on_green_tests reaches delivery.",
+ "phases":[
+  {"kind":"coding","order_idx":0,"config":{"commit_policy":"on_green_tests","max_iterations":1,
+    "task":"Add a file KEEP.md at the repository root containing one sentence about this crate. Do not modify any existing file, and do not add or change any test."}}
+ ]}
+JSON
+)
+
+assert_refactor() { # <token> <mission> <report>
+  local report="$3" branch_seen=""
+  while read -r idx status files pushed branch cerr perr; do
+    [ "$status" = "completed" ] \
+      && pass "refactor: phase $idx completed" \
+      || fail "refactor: phase $idx status=$status (commit_error=$cerr push_error=$perr)"
+    case "$files" in
+      0|-) fail "refactor: phase $idx delivered no files" ;;
+      *)   pass "refactor: phase $idx delivered $files file(s)" ;;
+    esac
+    branch_seen="$branch"
+  done <<<"$report"
+
+  # The gate's whole job is WHERE the work lands. A failed gate does not discard
+  # work — it diverts it to a review branch — so a green gate must NOT divert.
+  case "$branch_seen" in
+    "" |-) fail "refactor: no branch recorded — the gate's outcome is unobservable" ;;
+    *-review|*-needs-review)
+      fail "refactor: work landed on a REVIEW branch ($branch_seen) though the gate should have passed" ;;
+    *) pass "refactor: on_green_tests published to the mission branch ($branch_seen)" ;;
+  esac
+}
+
 # ── Scenario: the two engines composed ───────────────────────────
 #
 # A `team_engine=composed` mission is a durable ZeroClaw graph whose every node
@@ -977,6 +1025,9 @@ case "${1:-all}" in
   security)
     run_scenario security "$(echo "$SECURITY_BODY" | tr -d '\n')" assert_security
     ;;
+  refactor)
+    run_scenario refactor "$(echo "$REFACTOR_BODY" | tr -d '\n')" assert_refactor
+    ;;
   composed)
     run_scenario composed "$(echo "$COMPOSED_BODY" | tr -d '\n')" assert_composed
     ;;
@@ -995,11 +1046,12 @@ case "${1:-all}" in
     run_scenario research-only "$(echo "$RESEARCH_ONLY_BODY" | tr -d '\n')" assert_research_only no-checkout
     run_scenario benchmark "$(echo "$BENCHMARK_BODY" | tr -d '\n')" assert_benchmark
     run_scenario security "$(echo "$SECURITY_BODY" | tr -d '\n')" assert_security
+    run_scenario refactor "$(echo "$REFACTOR_BODY" | tr -d '\n')" assert_refactor
     run_scenario composed "$(echo "$COMPOSED_BODY" | tr -d '\n')" assert_composed
     scenario_roster
     ;;
   *)
-    die "unknown scenario: $1 (selftest|uids|chain|multirole|noop|microvm|gatecap|research-only|benchmark|security|composed|roster|all)"
+    die "unknown scenario: $1 (selftest|uids|chain|multirole|noop|microvm|gatecap|research-only|benchmark|security|refactor|composed|roster|all)"
     ;;
 esac