Merge: an empty coding phase is a failure, not a completion

Verified on the deployed stack: verify-mission-delivery.sh all → 9/9,
with the noop negative control showing 'phase 0 failed 0' where the same
shape read 'completed' in mission 019fcf62.

Co-Authored-By: Claude Opus 5 <[email protected]>

crates/cm-api/src/mission_delivery.rs

@@ -109,6 +109,10 @@ pub struct Capture {
     /// The phase changed nothing. Still recorded — "this coding phase wrote no
     /// code" is currently invisible to an operator, and it is worth saying.
     pub empty: bool,
+    /// Why the diff could not be computed, if it could not be. `empty` is only
+    /// meaningful when this is `None`: otherwise the tree was never read, and
+    /// callers deciding anything on the strength of "no changes" must not.
+    pub diff_error: Option<String>,
     pub truncated: bool,
     pub patch_path: PathBuf,
     /// Set once the work has been committed to a mission branch.
@@ -463,6 +467,7 @@ pub async fn capture_phase_diff_at(
         insertions,
         deletions,
         empty,
+        diff_error,
         truncated,
         patch_path,
     }))

crates/cm-api/src/phase_config.rs

@@ -47,6 +47,11 @@ pub const KNOWN_KEYS: &[KnownKey] = &[
         key: "commit_policy",
         read_by: "mission_delivery::Gate::parse — selects the delivery gate",
     },
+    KnownKey {
+        key: "allow_empty",
+        read_by: "phase_runner::empty_delivery_is_a_failure — when true, a coding \
+                  phase that changes no files still completes",
+    },
 ];
 
 /// Keys a recipe may carry that are deliberately not consumed *yet*.

crates/cm-api/src/phase_runner.rs

@@ -89,7 +89,7 @@ const CAPTURE_BATCH: i64 = 5;
 /// with a `NOT EXISTS` guard covers both and is retryable by construction.
 async fn capture_finished_coding_phases(pool: &PgPool) -> Result<(), String> {
     let rows = sqlx::query(
-        "SELECT mp.id, mp.mission_id
+        "SELECT mp.id, mp.mission_id, mp.kind, mp.config
            FROM mission_phases mp
            JOIN missions m ON m.id = mp.mission_id
           WHERE mp.status = 'completed'
@@ -112,6 +112,8 @@ async fn capture_finished_coding_phases(pool: &PgPool) -> Result<(), String> {
         use sqlx::Row;
         let phase_id: Uuid = row.get("id");
         let mission_id: Uuid = row.get("mission_id");
+        let kind: String = row.get("kind");
+        let config: serde_json::Value = row.get("config");
         // Pull the agent's work back onto the host before capturing it.
         // Unpacks over the same checkout path, so capture below is unchanged.
         if crate::mission_fs::copy_mode() {
@@ -130,7 +132,33 @@ async fn capture_finished_coding_phases(pool: &PgPool) -> Result<(), String> {
             }
         }
         match crate::mission_delivery::capture_phase_diff(pool, mission_id, phase_id).await {
-            Ok(Some(_)) => {}
+            Ok(Some(c)) => {
+                // Capture is the first moment the platform knows whether the
+                // phase produced anything — it runs after the phase is already
+                // `completed`, because capture selects on that status. So the
+                // verdict is applied here rather than at completion.
+                if empty_delivery_is_a_failure(
+                    &kind,
+                    c.files_changed,
+                    c.diff_error.as_deref(),
+                    &config,
+                ) {
+                    eprintln!(
+                        "phase_runner: phase {phase_id} ({kind}) of mission {mission_id} \
+                         delivered NO files — failing it. Set config.allow_empty = true if \
+                         this phase is meant to verify rather than change."
+                    );
+                    if let Err(e) = sqlx::query(
+                        "UPDATE mission_phases SET status = 'failed' WHERE id = $1",
+                    )
+                    .bind(phase_id)
+                    .execute(pool)
+                    .await
+                    {
+                        eprintln!("phase_runner: failing empty phase {phase_id}: {e}");
+                    }
+                }
+            }
             Ok(None) => {
                 // The checkout is gone — reaped before capture reached this
                 // phase. Record that, or the row stays eligible forever; and
@@ -157,6 +185,37 @@ async fn capture_finished_coding_phases(pool: &PgPool) -> Result<(), String> {
     Ok(())
 }
 
+/// Did this phase finish without delivering the work it exists to produce?
+///
+/// A coding phase that changes no files has done nothing, and until now that
+/// was reported as `completed` — the same status as a phase that delivered a
+/// tested, reviewed, pushed change. Mission `019fcf62` completed that way while
+/// its agents were silently unpinned from the repo, and nothing in the platform
+/// said otherwise; the failure was found by a script diffing the forge.
+///
+/// Three things must all hold before calling it a failure, because a false
+/// positive here fails honest work:
+///
+///   - **The phase is a coding phase.** Research phases legitimately write
+///     nothing to the tree.
+///   - **The diff was actually computed.** An uncomputable diff also reports
+///     zero files (see `mission_delivery::untrusted_empty_reason`); treating it
+///     as an empty delivery would blame the agent for a platform fault.
+///   - **`allow_empty` is not set.** The escape hatch for a coding phase whose
+///     job is genuinely to verify rather than to change — asserted for, not
+///     assumed.
+fn empty_delivery_is_a_failure(
+    kind: &str,
+    files_changed: usize,
+    diff_error: Option<&str>,
+    config: &serde_json::Value,
+) -> bool {
+    kind == "coding"
+        && files_changed == 0
+        && diff_error.is_none()
+        && config.get("allow_empty").and_then(|v| v.as_bool()) != Some(true)
+}
+
 /// Enqueue topology_runs for every phase whose predecessors are done.
 async fn start_pending_phases(pool: &PgPool) -> Result<(), String> {
     // Eligible = pending phase, mission running, all lower-order phases
@@ -793,6 +852,27 @@ async fn close_finished_missions(pool: &PgPool) -> Result<(), String> {
               SELECT 1 FROM mission_phases mp
               WHERE mp.mission_id = m.id
                 AND mp.status NOT IN ('completed', 'failed', 'skipped')
+            )
+            -- A repo-bearing mission must not be declared finished before its
+            -- work has been captured. Capture is batched (CAPTURE_BATCH per
+            -- tick) and runs AFTER a phase completes, so without this a
+            -- backlogged mission closes as 'completed' and only then does
+            -- capture discover the phase delivered nothing — leaving a
+            -- 'completed' mission holding a 'failed' phase, with the mission
+            -- status unfixable because the CASE above only touches 'running'
+            -- rows. Waiting a tick costs nothing; the alternative is a mission
+            -- whose headline outcome contradicts its own phases.
+            AND NOT EXISTS (
+              SELECT 1 FROM mission_phases mp
+              WHERE mp.mission_id = m.id
+                AND mp.status = 'completed'
+                AND m.repo_id IS NOT NULL
+                AND NOT EXISTS (
+                  SELECT 1 FROM mission_artifacts a
+                   WHERE a.mission_id = mp.mission_id
+                     AND a.phase_id = mp.id
+                     AND a.kind = 'code_diff'
+                )
             )",
     )
     .execute(pool)
@@ -898,6 +978,71 @@ mod tests {
         pairs.iter().map(|(k, v)| (k.to_string(), *v)).collect()
     }
 
+    /// The regression from mission `019fcf62`: a coding phase whose agents were
+    /// silently unpinned from the repo wrote nothing and reported `completed`,
+    /// the same status a fully delivered phase gets.
+    #[test]
+    fn a_coding_phase_that_delivers_nothing_is_a_failure() {
+        let none = serde_json::json!({});
+        assert!(
+            empty_delivery_is_a_failure("coding", 0, None, &none),
+            "zero files from a coding phase is not success"
+        );
+        assert!(
+            !empty_delivery_is_a_failure("coding", 1, None, &none),
+            "a phase that changed a file delivered"
+        );
+    }
+
+    /// A research phase legitimately writes nothing to the tree; failing it
+    /// would break every research→coding mission.
+    #[test]
+    fn only_coding_phases_are_held_to_delivering_files() {
+        let none = serde_json::json!({});
+        for kind in ["research", "benchmark", "security_scan"] {
+            assert!(
+                !empty_delivery_is_a_failure(kind, 0, None, &none),
+                "{kind} phases are not required to change files"
+            );
+        }
+    }
+
+    /// The distinction this whole defect class turns on: an uncomputable diff
+    /// ALSO reports zero files. Blaming the agent for a platform fault would
+    /// re-encode the failure as an ordinary outcome — the exact mistake the
+    /// `diff_error` field exists to prevent.
+    #[test]
+    fn an_uncomputable_diff_does_not_fail_the_phase() {
+        let none = serde_json::json!({});
+        assert!(
+            !empty_delivery_is_a_failure("coding", 0, Some("patch: fatal: bad object"), &none),
+            "a diff we could not compute is a platform fault, not an empty delivery"
+        );
+    }
+
+    /// The escape hatch must be asserted for, not assumed: only an explicit
+    /// `true` opts out, so a typo leaves the check armed.
+    #[test]
+    fn allow_empty_must_be_an_explicit_true() {
+        assert!(!empty_delivery_is_a_failure(
+            "coding",
+            0,
+            None,
+            &serde_json::json!({"allow_empty": true})
+        ));
+        for wrong in [
+            serde_json::json!({"allow_empty": "true"}),
+            serde_json::json!({"allow_empty": 1}),
+            serde_json::json!({"allow_empty": false}),
+            serde_json::json!({"allowEmpty": true}),
+        ] {
+            assert!(
+                empty_delivery_is_a_failure("coding", 0, None, &wrong),
+                "{wrong} must not disable the check"
+            );
+        }
+    }
+
     /// A phase's own task must reach the agent, and two phases of one mission
     /// must not receive identical text.
     ///

scripts/verify-mission-delivery.sh

@@ -29,6 +29,7 @@
 #   scripts/verify-mission-delivery.sh uids <mission>  # uid probe, one mission
 #   scripts/verify-mission-delivery.sh chain           # phase continuity
 #   scripts/verify-mission-delivery.sh multirole       # 3 roles + real tests
+#   scripts/verify-mission-delivery.sh noop            # empty phase must FAIL
 #   scripts/verify-mission-delivery.sh all             # everything
 #
 # Environment:
@@ -347,6 +348,44 @@ assert_multirole() { # <token> <mission> <report>
   esac
 }
 
+# ── Scenario: a phase that delivers nothing must FAIL ────────────
+#
+# The negative control for the delivery guard, and the same discipline as the
+# uid self-test: a check that has never been seen to fire has not been shown to
+# work. This phase is told to change nothing, so `empty_delivery_is_a_failure`
+# must catch it — and the scenario PASSES when the phase comes back `failed`.
+
+NOOP_BODY=$(cat <<JSON
+{"title":"verify: a phase that delivers nothing must fail",
+ "template_kind":"research_and_code",
+ "team_template_id":"$TEAM_TEMPLATE",
+ "repo_id":"$REPO_ID",
+ "description":"Negative control for the empty-delivery guard.",
+ "phases":[
+  {"kind":"coding","order_idx":0,"config":{"commit_policy":"always","max_iterations":1,
+    "task":"Do NOT create, modify or delete any file. Read README.md and reply with a one-sentence summary of it as your final answer. Leave the working tree exactly as you found it."}}
+ ]}
+JSON
+)
+
+assert_noop() { # <token> <mission> <report>
+  local report="$3" saw_failed=0
+  while read -r idx status files _pushed _branch _cerr _perr; do
+    case "$files" in
+      0|-) ;;
+      *) fail "noop: phase $idx changed $files file(s) — the scenario did not \
+exercise the guard (agent ignored the instruction; re-run)"; continue ;;
+    esac
+    if [ "$status" = "failed" ]; then
+      saw_failed=1
+    else
+      fail "noop: phase $idx delivered nothing but reports status=$status"
+    fi
+  done <<<"$report"
+  [ "$saw_failed" = "1" ] && pass "noop: an empty coding phase was failed, not completed"
+  return 0
+}
+
 # ── Entry point ──────────────────────────────────────────────────
 
 ssh -o BatchMode=yes -o ConnectTimeout=10 "$HOST" true 2>/dev/null \
@@ -370,14 +409,18 @@ case "${1:-all}" in
     body=${MULTIROLE_BODY//__TEAM__/$TEAM_TEMPLATE}
     run_scenario multirole "${body//__REPO__/$REPO_ID}" assert_multirole
     ;;
+  noop)
+    run_scenario noop "$NOOP_BODY" assert_noop
+    ;;
   all)
     selftest_uid_probe
     run_scenario chain "$CHAIN_BODY" assert_chain
     body=${MULTIROLE_BODY//__TEAM__/$TEAM_TEMPLATE}
     run_scenario multirole "${body//__REPO__/$REPO_ID}" assert_multirole
+    run_scenario noop "$NOOP_BODY" assert_noop
     ;;
   *)
-    die "unknown scenario: $1 (selftest|uids|chain|multirole|all)"
+    die "unknown scenario: $1 (selftest|uids|chain|multirole|noop|all)"
     ;;
 esac