fix(missions): merge phase config instead of replacing it; conditions are per-phase

Two defects in the goal-condition work, both found while tracing a
research->coding mission end to end.

1. Setting a condition silently dropped the recipe's phase config.

phases_for_create treated a caller-supplied config as a wholesale replacement.
The wizard sends {done_when, max_iterations} as the entire config, so every
other recipe key was discarded. Harmless for research_and_code, where nothing
reads `produces` or `default_topology` -- but a conditioned security_hardening
phase lost its `tools` list, which security_scan.rs DOES read, so the scan
would run with nothing configured and report clean. A green security scan that
scanned nothing is the worst possible failure mode for that feature.

The recipe is now the base and the caller's keys override individually.
Shallow merge is deliberate: phase config is a flat settings bag, and a caller
sending `tools: [...]` means to replace the list, not union it. A non-object
override still replaces outright rather than silently picking a side.

2. One condition was applied to every phase.

The wizard had a single mission-level "Done when" that got copied onto all
phases. For research->coding that is actively wrong: "cargo test reported 0
failures" cannot hold while the research phase is running, so research would
burn all its passes and give up before coding ever started. Conditions are now
per phase, keyed by order_idx, with a per-kind placeholder that demonstrates
the rule that actually governs whether a condition works -- it must be
provable from what the agents wrote, because the checker cannot run commands.

Phases with no condition are sent unchanged, so they keep the recipe's
settings and finish in one pass exactly as before.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

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

@@ -148,14 +148,20 @@ pub async fn list(
     ))
 }
 
-/// Resolve the phase list for a new mission, filling in each phase's `config`
-/// from the workflow recipe when the caller didn't supply one.
+/// Resolve the phase list for a new mission, merging each phase's `config` over
+/// the workflow recipe's.
 ///
 /// `mission_phases.config` is where per-phase settings live (`done_when`,
 /// `max_iterations`, `harness`, `tools`). The client's phase list historically
 /// carried only `{kind, order_idx}`, so every wizard-created mission landed
-/// with a null config and every recipe setting was silently inert. Callers can
-/// still override by sending a non-null config per phase.
+/// with a null config and every recipe setting was silently inert.
+///
+/// The recipe is the **base** and the caller's keys override individually —
+/// not wholesale. A caller that sends `{done_when: "..."}` is adding a
+/// completion condition, not declaring that the phase has no other settings.
+/// Replacing here meant a conditioned `security_hardening` phase lost its
+/// `tools` list, which `security_scan.rs` reads, so the scan would silently
+/// run with no tools configured.
 fn phases_for_create(
     recipe: Option<&crate::workflow_registry::WorkflowRecipe>,
     requested: Vec<PhaseSpec>,
@@ -176,34 +182,51 @@ fn phases_for_create(
             .unwrap_or_default();
     }
 
-    // Phases requested: honour them, but backfill config from the matching
-    // recipe phase (by kind + order_idx, falling back to kind alone) so a
-    // client that only knows the shape still gets the recipe's settings.
+    // Phases requested: honour the shape, and merge the caller's config over
+    // the matching recipe phase's (matched by kind + order_idx, then kind).
     requested
         .into_iter()
         .map(|p| {
-            let config = if p.config.is_null() {
-                recipe
-                    .and_then(|r| {
-                        r.phases
-                            .iter()
-                            .find(|rp| rp.kind == p.kind && rp.order_idx == p.order_idx)
-                            .or_else(|| r.phases.iter().find(|rp| rp.kind == p.kind))
-                    })
-                    .map(|rp| rp.config.clone())
-                    .unwrap_or(Value::Null)
-            } else {
-                p.config
-            };
+            let base = recipe
+                .and_then(|r| {
+                    r.phases
+                        .iter()
+                        .find(|rp| rp.kind == p.kind && rp.order_idx == p.order_idx)
+                        .or_else(|| r.phases.iter().find(|rp| rp.kind == p.kind))
+                })
+                .map(|rp| rp.config.clone())
+                .unwrap_or(Value::Null);
             NewMissionPhase {
                 kind: p.kind,
                 order_idx: p.order_idx,
-                config,
+                config: merge_config(base, p.config),
             }
         })
         .collect()
 }
 
+/// Shallow-merge `over` onto `base`, key by key.
+///
+/// Shallow is deliberate: phase config is a flat settings bag, and a caller
+/// that sends `tools: [...]` means to replace the list, not union it.
+fn merge_config(base: Value, over: Value) -> Value {
+    match (base, over) {
+        (Value::Object(mut b), Value::Object(o)) => {
+            for (k, v) in o {
+                b.insert(k, v);
+            }
+            Value::Object(b)
+        }
+        // Nothing to merge onto, or nothing to merge in.
+        (base, Value::Null) => base,
+        (Value::Null, over) => over,
+        // A non-object override replaces outright — there is no sane merge of
+        // e.g. an array onto an object, and silently picking one would hide
+        // the caller's mistake.
+        (_, over) => over,
+    }
+}
+
 /// `GET /api/workflows` — the workflow recipe catalog.
 ///
 /// Serves `templates/workflows/*.toml` so the client can drop its inline
@@ -1067,9 +1090,9 @@ mod tests {
         assert_eq!(phases[1].kind, "coding");
     }
 
-    /// An explicit config always wins over the recipe's.
+    /// An explicit key wins over the recipe's value for that key.
     #[test]
-    fn explicit_phase_config_is_not_overwritten() {
+    fn explicit_phase_config_overrides_the_recipe_key() {
         let requested = vec![PhaseSpec {
             kind: "coding".into(),
             order_idx: 1,
@@ -1082,6 +1105,46 @@ mod tests {
         );
     }
 
+    /// ...but overriding one key must NOT drop the rest of the recipe's
+    /// config. Sending `{done_when}` means "also apply this condition", not
+    /// "this phase has no other settings".
+    ///
+    /// The case that motivated this: a `security_hardening` phase with a
+    /// completion condition lost its `tools` list, which `security_scan.rs`
+    /// reads — so the scan ran with nothing configured and reported clean.
+    #[test]
+    fn adding_a_condition_preserves_the_rest_of_the_recipe_config() {
+        let requested = vec![PhaseSpec {
+            kind: "coding".into(),
+            order_idx: 1,
+            config: serde_json::json!({"done_when": "tests pass", "max_iterations": 3}),
+        }];
+        let phases = phases_for_create(Some(&test_recipe()), requested);
+        let c = &phases[0].config;
+        assert_eq!(
+            c.get("done_when").and_then(|v| v.as_str()),
+            Some("tests pass"),
+            "the caller's condition must land"
+        );
+        assert_eq!(
+            c.get("commit_policy").and_then(|v| v.as_str()),
+            Some("on_green_tests"),
+            "recipe keys the caller didn't mention must survive"
+        );
+        assert_eq!(
+            c.get("loop").and_then(|v| v.as_str()),
+            Some("until_no_more_int_items")
+        );
+    }
+
+    #[test]
+    fn merge_config_handles_null_on_either_side() {
+        let base = serde_json::json!({"a": 1});
+        assert_eq!(merge_config(base.clone(), Value::Null), base);
+        assert_eq!(merge_config(Value::Null, base.clone()), base);
+        assert_eq!(merge_config(Value::Null, Value::Null), Value::Null);
+    }
+
     /// An unknown template must not fabricate phases or panic.
     #[test]
     fn unknown_template_yields_no_phases() {