fix(missions): close the three seams behind this run of failures

Seam 1 — delivery inferred checkout state from the tree, so whether work
survived depended on what the agent happened to do. 019fc444 committed
and left a clean tree; 019fc476 had its base advanced to match HEAD;
019fc450 survived only because a phase FAILED to commit and left the tree
dirty. Same code, opposite outcomes, decided by the agent.

mark_phase_started records the fact at phase launch, before the agent
acts, so every one of those states answers identically. The tree checks
remain as a second line of defence for pre-existing checkouts.

Seam 2 — phase config was accepted, stored and read by nobody. That was
`task`: every phase of every mission got identical instructions. The new
phase_config registry names the reader for each live key and lists the
eight that are declared-but-unimplemented, reporting both at mission
creation so an author sees what will not happen. Its CI test found one I
had missed: security_hardening.toml sets phase-level mcp_bundles asking
for gitea_forge + security_scan, but bundles come from the TEAM template
and the phase gets neither.

Seam 4 — push_url_for collapsed a failed query, an unbound repo and a
missing clone_url into one None, so a database fault was recorded as
"nothing to push to" and metadata read `pushed: null, push_error: null` —
the same ambiguity commit_error already fixed. Each case now carries its
reason into the artifact, and a local git failure during publish is
recorded rather than dropped by .ok().

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

crates/cm-api/src/lib.rs

@@ -17,6 +17,7 @@ mod mcp_skills;
 pub mod mission_orchestrator;
 pub mod mission_refiner;
 pub mod mission_delivery;
+pub mod phase_config;
 pub mod runtime_preflight;
 pub mod mission_runtime;
 pub mod mission_workspace;

crates/cm-api/src/mission_delivery.rs

@@ -295,6 +295,7 @@ pub async fn capture_phase_diff_at(
     // still has its patch on disk and its work on a local branch.
     let mut outcome: Option<TestOutcome> = None;
     let mut published: Option<Publish> = None;
+    let mut publish_error: Option<String> = None;
     if let Some(c) = committed.as_ref() {
         if !empty {
             if gate == Gate::OnGreenTests {
@@ -313,17 +314,38 @@ pub async fn capture_phase_diff_at(
                 outcome = Some(o);
             }
             match push_url_for(pool, mission_id).await {
-                Some(url) => {
+                Ok(Some(url)) => {
                     let verified = outcome.as_ref().and_then(TestOutcome::verified);
-                    published = publish_phase_branch(&repo, &url, &c.branch, gate, verified)
+                    match publish_phase_branch(&repo, &url, &c.branch, gate, verified).await {
-                        .await
+                        Ok(p) => published = Some(p),
-                        .ok();
+                        // `publish_phase_branch` only returns Err for a local
-                }
+                        // git failure; a rejected push is Ok with an error
-                None => eprintln!(
+                        // inside. Both must reach the artifact.
-                    "mission_delivery: mission {mission_id} has no push URL — work is \
+                        Err(e) => {
-                     committed locally on {} but not published",
+                            eprintln!(
+                                "mission_delivery: mission {mission_id} phase {phase_id} \
+                                 could not publish {}: {e}",
                                 c.branch
-                ),
+                            );
+                            publish_error = Some(e.chars().take(500).collect());
+                        }
+                    }
+                }
+                Ok(None) => {
+                    // Legitimate: a mission with no repo bound has nowhere to
+                    // push. Still recorded, because "not pushed" with no reason
+                    // is the ambiguity this whole pass exists to remove.
+                    publish_error =
+                        Some("mission has no repo bound; work is committed locally only".into());
+                }
+                Err(e) => {
+                    eprintln!(
+                        "mission_delivery: mission {mission_id} could not resolve a push \
+                         URL ({e}) — work is committed locally on {} but not published",
+                        c.branch
+                    );
+                    publish_error = Some(format!("could not resolve push URL: {e}"));
+                }
             }
         }
     }
@@ -348,7 +370,10 @@ pub async fn capture_phase_diff_at(
         "tests_status": outcome.as_ref().map(TestOutcome::status),
         "tests_detail": outcome.as_ref().and_then(TestOutcome::detail),
         "pushed": published.as_ref().map(|p| p.pushed),
-        "push_error": published.as_ref().and_then(|p| p.error.clone()),
+        "push_error": published
+            .as_ref()
+            .and_then(|p| p.error.clone())
+            .or(publish_error),
         "commit_error": commit_error,
         "files_changed": files_changed,
         "insertions": insertions,
@@ -680,16 +705,19 @@ pub fn discover_test_command(repo: &Path) -> Option<Vec<String>> {
 /// after clone because agents run as root in a container that mounts the
 /// checkout. Building it here also means a rotated token takes effect
 /// immediately instead of at the next clone.
-async fn push_url_for(pool: &sqlx::PgPool, mission_id: Uuid) -> Option<String> {
+async fn push_url_for(pool: &sqlx::PgPool, mission_id: Uuid) -> Result<Option<String>, String> {
     let url: Option<String> = sqlx::query_scalar(
         "SELECT r.clone_url FROM missions m JOIN repos r ON r.id = m.repo_id WHERE m.id = $1",
     )
     .bind(mission_id)
     .fetch_optional(pool)
     .await
-    .ok()
+    // `.ok().flatten()` used to collapse a failed query into the same `None`
+    // as a mission with no repo bound, so a database fault was recorded as
+    // "nothing to push to" — the shape that made `commit_error` necessary.
+    .map_err(|e| format!("query push URL: {e}"))?
     .flatten();
-    url.map(|u| mission_workspace::with_ambient_auth(&u))
+    Ok(url.map(|u| mission_workspace::with_ambient_auth(&u)))
 }
 
 /// Run the gate, then push the branch if the gate allows it.

crates/cm-api/src/mission_workspace.rs

@@ -73,7 +73,11 @@ pub async fn ensure_checkout(
         // `ensure_checkout` runs at every phase launch, not once per mission.
         // Freshening a pristine checkout is right; freshening one that already
         // holds this mission's work destroys it. See `has_local_work`.
-        if has_local_work(&path, default_branch) {
+        // Marker first: it is a fact we recorded, not a state we inferred.
+        // The tree checks stay as a second line of defence for checkouts
+        // created before the marker existed, and for the case where the
+        // marker write itself failed.
+        if checkout_in_use(&path) || has_local_work(&path, default_branch) {
             eprintln!(
                 "mission_workspace: {} already holds mission work — skipping \
                  fetch/reset so earlier phases' output survives",
@@ -140,6 +144,41 @@ async fn clone(path: &std::path::Path, url: &str) -> Result<(), String> {
     Ok(())
 }
 
+/// Record that a phase has started working in this checkout.
+///
+/// The explicit half of the "is this checkout in use" question. `ensure_checkout`
+/// runs per phase launch and refreshes on reuse; whether that refresh is safe
+/// depends on whether a phase has already run here, which is a fact about the
+/// *mission* and not about the tree.
+///
+/// It was previously inferred from the tree — dirty status, HEAD versus the
+/// remote tip — and inference is what made delivery depend on what an agent
+/// happened to do. Mission `019fc444` lost work because its phase committed and
+/// left a clean tree; `019fc476` lost work because the capture base had advanced
+/// to match HEAD; `019fc450` survived only because a phase *failed* to commit
+/// and left the tree dirty. Same code, opposite outcomes, decided by the agent.
+///
+/// A marker is not a heuristic. Once a phase has begun, the checkout is in use
+/// until the mission ends, whatever the agent did or did not do inside it.
+pub(crate) fn mark_phase_started(path: &std::path::Path) {
+    let marker = path.join(".git/clawmates-in-use");
+    if marker.exists() {
+        return;
+    }
+    if let Err(e) = std::fs::write(&marker, "1\n") {
+        eprintln!(
+            "mission_workspace: could not mark {} as in use ({e}) — a later phase may \
+             refresh the checkout and discard earlier work",
+            path.display()
+        );
+    }
+}
+
+/// Has a phase already started work in this checkout?
+fn checkout_in_use(path: &std::path::Path) -> bool {
+    path.join(".git/clawmates-in-use").exists()
+}
+
 /// Has anything happened in this checkout since it was created?
 ///
 /// `ensure_checkout` is called once per *phase launch*, not once per mission,
@@ -729,4 +768,57 @@ mod tests {
             .output()
             .unwrap();
     }
+
+    /// A checkout in use must be recognised regardless of what the agent did.
+    ///
+    /// This is the Seam-1 property. The tree-state heuristics were each correct
+    /// in isolation and each blind to a different case: `019fc444` committed
+    /// and left a clean tree, `019fc476` had its base advanced to match HEAD,
+    /// `019fc450` survived only because a phase FAILED to commit. Whether the
+    /// work survived was decided by the agent, not by us.
+    ///
+    /// The marker is set when a phase launches, before the agent does anything,
+    /// so every one of those states answers the same way.
+    #[test]
+    fn an_in_use_checkout_is_recognized_whatever_the_agent_did() {
+        let tmp = tempfile::tempdir().unwrap();
+        let repo = &tmp.path().join("repo");
+        std::fs::create_dir_all(repo).unwrap();
+        seed(repo, &tmp.path().join("remote.git"));
+        let repo = repo.as_path();
+
+        assert!(!checkout_in_use(repo), "a fresh clone is not in use");
+
+        mark_phase_started(repo);
+        assert!(checkout_in_use(repo), "a launched phase marks the checkout");
+
+        // The three production states, all of which must now answer the same.
+        // (a) agent wrote nothing at all — the case every tree heuristic misses.
+        assert!(checkout_in_use(repo), "clean tree at the base commit");
+
+        // (b) agent committed, leaving a clean tree at a moved HEAD.
+        std::fs::write(repo.join("WORK.md"), "work\n").unwrap();
+        git_in(repo, &["add", "WORK.md"]);
+        git_in(repo, &["commit", "--quiet", "-m", "phase work"]);
+        let head = std::process::Command::new("git")
+            .arg("-C")
+            .arg(repo)
+            .args(["rev-parse", "HEAD"])
+            .output()
+            .unwrap();
+        let head = String::from_utf8_lossy(&head.stdout).trim().to_string();
+        assert!(checkout_in_use(repo));
+
+        // (c) capture advanced the base to match HEAD — the collision that
+        // defeated the HEAD-versus-base check on 019fc476.
+        advance_base_commit(repo, &head);
+        assert!(
+            checkout_in_use(repo),
+            "an advanced base must not make an in-use checkout look pristine"
+        );
+
+        // Marking twice is safe; phases launch repeatedly across a mission.
+        mark_phase_started(repo);
+        assert!(checkout_in_use(repo));
+    }
 }

crates/cm-api/src/phase_config.rs

@@ -0,0 +1,262 @@
+//! Which phase-config keys the platform actually reads.
+//!
+//! `mission_phases.config` is free-form JSONB written by workflow recipes, the
+//! mission wizard and the API. Nothing connected a key to the code that reads
+//! it, so a key could be accepted, validated, stored, rendered — and consumed
+//! by nobody.
+//!
+//! `task` was exactly that. Every phase of every mission received identical
+//! instructions because the runner selected only the mission description; the
+//! per-phase task sat in Postgres unread. Mission `019fc42b` is what surfaced
+//! it: two coding phases with different `task` values produced the same two
+//! files. There was no error, because there is nothing to fail — an unread key
+//! is indistinguishable from a key whose value happens not to matter.
+//!
+//! This module is the missing link. Every key here names the code that reads
+//! it, `unknown_keys` reports anything else, and a test asserts the shipped
+//! recipes only write keys that exist. It cannot make a reader appear, but it
+//! makes an absent one visible.
+
+/// A phase-config key and where it is consumed.
+pub struct KnownKey {
+    pub key: &'static str,
+    /// The code path that reads it. Kept as prose so this survives refactors
+    /// that a symbol reference would not.
+    pub read_by: &'static str,
+}
+
+/// Keys with a reader in the current build.
+///
+/// Adding a key here without a reader defeats the purpose. The rule is: a key
+/// earns its entry when something consumes it, not when something writes it.
+pub const KNOWN_KEYS: &[KnownKey] = &[
+    KnownKey {
+        key: "done_when",
+        read_by: "cm_db::repo::missions::create — promoted to the done_when column, \
+                  swept by phase_runner::evaluate_finished_phases",
+    },
+    KnownKey {
+        key: "max_iterations",
+        read_by: "cm_db::repo::missions::create — promoted to the max_iterations column",
+    },
+    KnownKey {
+        key: "task",
+        read_by: "phase_runner::start_pending_phases — injected by phase_task_text",
+    },
+    KnownKey {
+        key: "commit_policy",
+        read_by: "mission_delivery::Gate::parse — selects the delivery gate",
+    },
+];
+
+/// Keys a recipe may carry that are deliberately not consumed *yet*.
+///
+/// Distinguished from unknown keys so the report stays useful: these are known
+/// gaps with an owner, not typos. Every one is a feature described in a shipped
+/// workflow recipe whose implementation does not exist — which is worth seeing
+/// listed, because a recipe promising `loop = "until_done"` reads to an
+/// operator like something that loops.
+pub const DECLARED_BUT_UNREAD: &[KnownKey] = &[
+    KnownKey {
+        key: "loop",
+        read_by: "NOT IMPLEMENTED — phase iteration uses max_iterations + done_when",
+    },
+    KnownKey {
+        key: "produces",
+        read_by: "NOT IMPLEMENTED — artifact rendering is not driven by this",
+    },
+    KnownKey {
+        key: "input_from_phase",
+        read_by: "NOT IMPLEMENTED — phases share a checkout, not declared inputs",
+    },
+    KnownKey {
+        key: "mode",
+        read_by: "NOT IMPLEMENTED — benchmark/refactor mode selection",
+    },
+    KnownKey {
+        key: "harness",
+        read_by: "NOT IMPLEMENTED — benchmark harness selection",
+    },
+    KnownKey {
+        key: "tools",
+        read_by: "NOT IMPLEMENTED — per-phase tool selection",
+    },
+    KnownKey {
+        key: "benchmark",
+        read_by: "NOT IMPLEMENTED — nested benchmark settings",
+    },
+    KnownKey {
+        key: "mcp_bundles",
+        read_by: "NOT IMPLEMENTED at phase level — bundles come from the TEAM \
+                  template (mission_orchestrator binds template.mcp_bundles) and \
+                  runtime_provision writes agents.<alias>.mcp_bundles. A recipe \
+                  setting this per phase changes nothing: security_hardening.toml \
+                  asks for gitea_forge + security_scan and its phase gets neither",
+    },
+    KnownKey {
+        key: "test_command",
+        read_by: "NOT IMPLEMENTED — mission_delivery::discover_test_command infers \
+                  from the repo and does not consult config",
+    },
+];
+
+fn is_listed(key: &str, list: &[KnownKey]) -> bool {
+    list.iter().any(|k| k.key == key)
+}
+
+/// Keys in this config that no code reads and that are not known gaps.
+///
+/// Almost always a typo or a setting invented for a feature that was never
+/// built. Returned rather than rejected: a mission whose config carries an
+/// unread key is not *wrong*, it is just doing less than its author believes,
+/// and failing the request would break recipes that already ship these.
+pub fn unknown_keys(config: &serde_json::Value) -> Vec<String> {
+    let Some(obj) = config.as_object() else {
+        return Vec::new();
+    };
+    obj.keys()
+        .filter(|k| !is_listed(k, KNOWN_KEYS) && !is_listed(k, DECLARED_BUT_UNREAD))
+        .cloned()
+        .collect()
+}
+
+/// Keys that are recognised but that nothing consumes.
+pub fn inert_keys(config: &serde_json::Value) -> Vec<String> {
+    let Some(obj) = config.as_object() else {
+        return Vec::new();
+    };
+    obj.keys()
+        .filter(|k| is_listed(k, DECLARED_BUT_UNREAD))
+        .cloned()
+        .collect()
+}
+
+/// Log what a phase's config asked for that will not happen.
+///
+/// Called once per phase at mission creation. Deliberately not an error: the
+/// point is that the author's intent and the platform's behaviour have
+/// diverged, and the author should be able to see that without being blocked.
+pub fn report(kind: &str, order_idx: i32, config: &serde_json::Value) {
+    let unknown = unknown_keys(config);
+    if !unknown.is_empty() {
+        eprintln!(
+            "phase_config: phase {order_idx} ({kind}) sets unrecognised key(s) {} — \
+             nothing reads them; check for a typo",
+            unknown.join(", ")
+        );
+    }
+    let inert = inert_keys(config);
+    if !inert.is_empty() {
+        eprintln!(
+            "phase_config: phase {order_idx} ({kind}) sets {} — recognised but NOT \
+             IMPLEMENTED, so it will have no effect on this run",
+            inert.join(", ")
+        );
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn a_key_cannot_be_both_read_and_unread() {
+        for k in KNOWN_KEYS {
+            assert!(
+                !is_listed(k.key, DECLARED_BUT_UNREAD),
+                "{} is listed as both read and unread",
+                k.key
+            );
+        }
+    }
+
+    #[test]
+    fn every_known_key_names_its_reader() {
+        for k in KNOWN_KEYS {
+            assert!(
+                !k.read_by.is_empty() && !k.read_by.starts_with("NOT IMPLEMENTED"),
+                "{} claims to be read but names no reader",
+                k.key
+            );
+        }
+        for k in DECLARED_BUT_UNREAD {
+            assert!(
+                k.read_by.starts_with("NOT IMPLEMENTED"),
+                "{} is listed as unread but names a reader — promote it to KNOWN_KEYS",
+                k.key
+            );
+        }
+    }
+
+    /// The regression that motivated the module: `task` must stay claimed.
+    #[test]
+    fn the_per_phase_task_key_has_a_reader() {
+        assert!(
+            is_listed("task", KNOWN_KEYS),
+            "task lost its reader again — every phase will get identical instructions"
+        );
+    }
+
+    #[test]
+    fn unknown_and_inert_keys_are_reported_separately() {
+        let cfg = serde_json::json!({
+            "done_when": "tests pass",
+            "loop": "until_done",
+            "typpo": true,
+        });
+        assert_eq!(unknown_keys(&cfg), vec!["typpo".to_string()]);
+        assert_eq!(inert_keys(&cfg), vec!["loop".to_string()]);
+    }
+
+    /// Every key the shipped workflow recipes write must be accounted for.
+    ///
+    /// This is the CI-time half: a recipe that invents `comit_policy` should
+    /// fail here rather than run a mission whose gate silently defaults.
+    #[test]
+    fn shipped_recipes_only_write_accounted_keys() {
+        let dir = concat!(env!("CARGO_MANIFEST_DIR"), "/../../templates/workflows");
+        let Ok(entries) = std::fs::read_dir(dir) else {
+            return; // templates not present in this build context
+        };
+        // Keys that belong to the recipe/phase envelope rather than to the
+        // phase config blob itself.
+        const ENVELOPE: &[&str] = &[
+            "key",
+            "name",
+            "title",
+            "blurb",
+            "kind",
+            "order_idx",
+            "requires_repo",
+            "default_team_template",
+            "default_topology",
+            "phases",
+            "description",
+        ];
+        for entry in entries.flatten() {
+            let path = entry.path();
+            if path.extension().and_then(|e| e.to_str()) != Some("toml") {
+                continue;
+            }
+            let body = std::fs::read_to_string(&path).unwrap();
+            for line in body.lines() {
+                let line = line.trim();
+                if line.starts_with('#') || !line.contains('=') {
+                    continue;
+                }
+                let key = line.split('=').next().unwrap().trim();
+                if key.is_empty() || key.contains(' ') || key.contains('[') {
+                    continue;
+                }
+                let accounted = ENVELOPE.contains(&key)
+                    || is_listed(key, KNOWN_KEYS)
+                    || is_listed(key, DECLARED_BUT_UNREAD);
+                assert!(
+                    accounted,
+                    "{} writes `{key}`, which no reader claims and no gap declares",
+                    path.display()
+                );
+            }
+        }
+    }
+}

crates/cm-api/src/phase_runner.rs

@@ -271,10 +271,17 @@ async fn launch_phase(pool: &PgPool, p: PhaseLaunch<'_>) -> Result<(), String> {
     )
     .await
     {
-        Ok(Some(path)) => eprintln!(
+        Ok(Some(path)) => {
+            eprintln!(
                 "phase_runner: repo checked out at {} for mission {mission_id} phase {phase_id}",
                 path.display()
-        ),
+            );
+            // From here the checkout belongs to a running phase. Recording it
+            // explicitly is what stops the *next* phase's launch from
+            // refreshing the tree out from under this one's output — a
+            // decision that must not depend on what the agent leaves behind.
+            crate::mission_workspace::mark_phase_started(&path);
+        }
         Ok(None) => {}
         Err(e) => eprintln!(
             "phase_runner: repo checkout for mission {mission_id} phase {phase_id} failed (continuing): {e}"

crates/cm-api/src/routes/missions.rs

@@ -196,10 +196,16 @@ fn phases_for_create(
                 })
                 .map(|rp| rp.config.clone())
                 .unwrap_or(Value::Null);
+            let config = merge_config(base, p.config);
+            // Say what this phase asked for that will not happen. A config key
+            // nothing reads is silent by construction — `task` sat unread
+            // through every mission until two phases with different tasks
+            // produced identical output.
+            crate::phase_config::report(&p.kind, p.order_idx, &config);
             NewMissionPhase {
                 kind: p.kind,
                 order_idx: p.order_idx,
-                config: merge_config(base, p.config),
+                config,
             }
         })
         .collect()