From 0d46f892db65afd75b0fe8cfe7ac880fffbd8db1 Mon Sep 17 00:00:00 2001 From: Omar Sobh Date: Mon, 21 Sep 2026 10:31:33 -0500 Subject: [PATCH] fix(missions): a caller's own task does not inherit the recipe's done_when MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A recipe's completion condition is a condition on the recipe's own task. phases_for_create merged the recipe config under the caller's, so a phase that supplied a different task and no condition inherited a condition about work it was never given: research_and_code's coding phase carries "an implementation for each INT-XX item in IMPLEMENTATION_BRIEF", and a phase asked to write CHAIN.md failed on it, honestly, every time (01a0c20d, 01a0c493). Decided from the caller's config before the merge (afterwards a recipe task and a caller task look the same): caller task + no caller condition → the recipe's done_when/done_when_check are not inherited. A caller condition is kept; a phase with neither keeps the recipe's pair. Test fixture now carries a recipe task+condition. Harness: the triage agreement line dedupes per skill and excludes skills whose Trigger is not observable (always_inject is inlined). First live datapoint, 01a0c493: for 'create CHAIN.md and commit' Jev's top picks were workspace-repo-commit-protocol 0.63 / small-focused-commits 0.57; the agent read code-review-checklist (~0) and nothing else. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WZb5A2kfVfjpdwSochkuHz --- crates/cm-api/src/routes/missions.rs | 87 +++++++++++++++++++++++++++- scripts/verify-mission-delivery.sh | 21 +++++-- 2 files changed, 101 insertions(+), 7 deletions(-) diff --git a/crates/cm-api/src/routes/missions.rs b/crates/cm-api/src/routes/missions.rs index f41d8a5..9b38254 100644 --- a/crates/cm-api/src/routes/missions.rs +++ b/crates/cm-api/src/routes/missions.rs @@ -206,7 +206,21 @@ fn phases_for_create( }) .map(|rp| rp.config.clone()) .unwrap_or(Value::Null); + // Decided from the caller's config BEFORE the merge: afterwards a + // recipe task and a caller task are indistinguishable. + let caller_task = p + .config + .get("task") + .and_then(|t| t.as_str()) + .is_some_and(|s| !s.trim().is_empty()); + let caller_condition = + p.config.get("done_when").is_some() || p.config.get("done_when_check").is_some(); let config = merge_config(base, p.config); + let config = if caller_task && !caller_condition { + drop_orphaned_condition(config, &p.kind, p.order_idx) + } else { + 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 @@ -221,6 +235,36 @@ fn phases_for_create( .collect() } +/// A recipe's completion condition is a condition on the recipe's own task. +/// When the caller supplied a different `task` and no condition of its own, +/// keeping the recipe's `done_when` judges the phase against work it was +/// never asked to do — `research_and_code`'s coding phase inherits "an +/// implementation for each INT-XX item in IMPLEMENTATION_BRIEF", and a +/// phase asked to write CHAIN.md fails on it, honestly, every time (missions +/// 01a0c20d, 01a0c493). The condition and the check travel with the task +/// they were written for; a phase with its own task gets its own, or none. +/// +/// Called only when the caller supplied a task and no condition; the +/// decision is made before the merge, where the two are still telling apart. +fn drop_orphaned_condition(config: Value, kind: &str, order_idx: i32) -> Value { + let Value::Object(mut c) = config else { return config }; + let dropped: Vec<&str> = ["done_when", "done_when_check"] + .into_iter() + .filter(|k| c.contains_key(*k)) + .collect(); + if !dropped.is_empty() { + eprintln!( + "phase {kind}[{order_idx}]: caller supplied its own task and no completion \ + condition — the recipe's {} is NOT inherited (it describes the recipe's task)", + dropped.join("/") + ); + for k in dropped { + c.remove(k); + } + } + Value::Object(c) +} + /// Shallow-merge `over` onto `base`, key by key. /// /// Shallow is deliberate: phase config is a flat settings bag, and a caller @@ -1755,6 +1799,44 @@ mod tests { ); } + /// A caller's own task does not inherit the recipe's condition; a + /// caller's own condition is kept; a phase with neither keeps the + /// recipe's pair as before. + #[test] + fn a_custom_task_does_not_inherit_the_recipes_done_when() { + let recipe = test_recipe(); + let coding_has_condition = recipe + .phases + .iter() + .any(|p| p.kind == "coding" && p.config.get("done_when").is_some()); + assert!(coding_has_condition, "the fixture must carry a recipe condition"); + let phases = phases_for_create( + Some(&recipe), + vec![ + PhaseSpec { + kind: "coding".into(), + order_idx: 1, + config: serde_json::json!({"task": "write CHAIN.md"}), + }, + PhaseSpec { + kind: "coding".into(), + order_idx: 2, + config: serde_json::json!({"task": "write X", "done_when": "X exists"}), + }, + PhaseSpec { + kind: "coding".into(), + order_idx: 3, + config: Value::Null, + }, + ], + ); + assert!(phases[0].config.get("done_when").is_none(), "{:?}", phases[0].config); + assert_eq!(phases[1].config["done_when"], "X exists"); + assert!(phases[2].config.get("done_when").is_some(), "{:?}", phases[2].config); + // The rest of the recipe's config still backfills the custom-task phase. + assert!(phases[0].config.get("loop").is_some()); + } + /// Omitting phases entirely takes the recipe's list wholesale. #[test] fn phases_default_to_the_recipe() { @@ -1853,7 +1935,10 @@ mod tests { order_idx: 1, config: serde_json::json!({ "loop": "until_no_more_int_items", - "commit_policy": "on_green_tests" + "commit_policy": "on_green_tests", + // The recipe's own task and the condition written for it. + "task": "implement every INT-XX item", + "done_when": "an implementation exists for each INT-XX item" }), }, ], diff --git a/scripts/verify-mission-delivery.sh b/scripts/verify-mission-delivery.sh index 26ca3bd..ae3bac5 100755 --- a/scripts/verify-mission-delivery.sh +++ b/scripts/verify-mission-delivery.sh @@ -1246,12 +1246,21 @@ assert_skill_triage() { #