From 2c5326f33961ee8b0876168d949bc38ce1b5f237 Mon Sep 17 00:00:00 2001 From: dave Date: Tue, 12 May 2026 14:43:27 +0000 Subject: [PATCH] huskies: merge 890 --- .../agents/pool/auto_assign/auto_assign.rs | 6 +- server/src/agents/pool/auto_assign/merge.rs | 123 ++++---- .../agents/pool/auto_assign/story_checks.rs | 262 +++++++++++------- server/src/crdt_state/state/mod.rs | 13 +- server/src/db/yaml_legacy.rs | 5 - 5 files changed, 227 insertions(+), 182 deletions(-) diff --git a/server/src/agents/pool/auto_assign/auto_assign.rs b/server/src/agents/pool/auto_assign/auto_assign.rs index ae977b2d..6d08b27f 100644 --- a/server/src/agents/pool/auto_assign/auto_assign.rs +++ b/server/src/agents/pool/auto_assign/auto_assign.rs @@ -518,10 +518,11 @@ mod tests { "[[agent]]\nname = \"mergemaster\"\nstage = \"mergemaster\"\n", ) .unwrap(); + crate::crdt_state::init_for_test(); crate::db::ensure_content_store(); crate::db::write_item_with_content( "9860_story_conflict", - "4_merge", + "4_merge_failure", "---\nname: Conflict\nmerge_failure: \"CONFLICT (content): server/src/lib.rs\"\n---\n", crate::db::ItemMeta::from_yaml( "---\nname: Conflict\nmerge_failure: \"CONFLICT (content): server/src/lib.rs\"\n---\n", @@ -555,10 +556,11 @@ mod tests { "[[agent]]\nname = \"mergemaster\"\nstage = \"mergemaster\"\n", ) .unwrap(); + crate::crdt_state::init_for_test(); crate::db::ensure_content_store(); crate::db::write_item_with_content( "9861_story_nothing", - "4_merge", + "4_merge_failure", "---\nname: Nothing\nmerge_failure: \"nothing to commit, working tree clean\"\n---\n", crate::db::ItemMeta::from_yaml( "---\nname: Nothing\nmerge_failure: \"nothing to commit, working tree clean\"\n---\n", diff --git a/server/src/agents/pool/auto_assign/merge.rs b/server/src/agents/pool/auto_assign/merge.rs index 47532cb8..cd6a4e69 100644 --- a/server/src/agents/pool/auto_assign/merge.rs +++ b/server/src/agents/pool/auto_assign/merge.rs @@ -12,7 +12,7 @@ use super::super::super::PipelineStage; use super::super::AgentPool; use super::scan::{find_free_agent_for_stage, is_story_assigned_for_stage, scan_stage_items}; use super::story_checks::{ - has_content_conflict_failure, has_merge_failure, has_mergemaster_attempted, has_review_hold, + has_content_conflict_failure, has_mergemaster_attempted, has_review_hold, has_unmet_dependencies, is_story_blocked, is_story_frozen, }; @@ -36,75 +36,6 @@ impl AgentPool { // call invokes the LLM-driven recovery path. let merge_items = scan_stage_items(project_root, "4_merge"); for story_id in &merge_items { - // Stories with a recorded merge failure may be eligible for - // automatic mergemaster dispatch when the failure is a content - // conflict — otherwise they need human intervention. - if has_merge_failure(project_root, "4_merge", story_id) { - // Auto-spawn mergemaster for content conflicts, but only once. - if has_content_conflict_failure(project_root, "4_merge", story_id) - && !has_mergemaster_attempted(project_root, "4_merge", story_id) - && !is_story_blocked(project_root, "4_merge", story_id) - { - // Find the mergemaster agent. - let mergemaster_agent = { - let agents = match self.agents.lock() { - Ok(a) => a, - Err(e) => { - slog_error!( - "[auto-assign] Failed to lock agents for mergemaster check: {e}" - ); - continue; - } - }; - if is_story_assigned_for_stage( - config, - &agents, - story_id, - &PipelineStage::Mergemaster, - ) { - // Already running — don't spawn again. - None - } else { - find_free_agent_for_stage(config, &agents, &PipelineStage::Mergemaster) - .map(str::to_string) - } - }; - - if let Some(agent_name) = mergemaster_agent { - slog!( - "[auto-assign] Content conflict on '{story_id}'; \ - auto-spawning mergemaster '{agent_name}'." - ); - // Record mergemaster_attempted before spawning so a - // crash/restart doesn't re-trigger an infinite loop. - if let Some(contents) = crate::db::read_content(story_id) { - let updated = - crate::db::yaml_legacy::write_mergemaster_attempted_in_content( - &contents, - ); - crate::db::write_content(story_id, &updated); - crate::db::write_item_with_content( - story_id, - "4_merge", - &updated, - crate::db::ItemMeta::from_yaml(&updated), - ); - } - crate::crdt_state::set_mergemaster_attempted(story_id, true); - if let Err(e) = self - .start_agent(project_root, story_id, Some(&agent_name), None, None) - .await - { - slog!( - "[auto-assign] Failed to start mergemaster '{agent_name}' \ - for '{story_id}': {e}" - ); - } - } - } - continue; - } - if has_review_hold(project_root, "4_merge", story_id) { continue; } @@ -176,5 +107,57 @@ impl AgentPool { slog!("[auto-assign] Triggering server-side merge for '{story_id}' in 4_merge/"); self.trigger_server_side_merge(project_root, story_id); } + + // ── 4_merge_failure: auto-spawn mergemaster on content conflict ─────── + // + // Stories transition to 4_merge_failure when the server-side merge fails. + // Content conflicts get one automatic mergemaster attempt; other failures + // require human intervention. + let merge_failure_items = scan_stage_items(project_root, "4_merge_failure"); + for story_id in &merge_failure_items { + if has_content_conflict_failure(project_root, "4_merge_failure", story_id) + && !has_mergemaster_attempted(project_root, "4_merge_failure", story_id) + { + let mergemaster_agent = { + let agents = match self.agents.lock() { + Ok(a) => a, + Err(e) => { + slog_error!( + "[auto-assign] Failed to lock agents for mergemaster check: {e}" + ); + continue; + } + }; + if is_story_assigned_for_stage( + config, + &agents, + story_id, + &PipelineStage::Mergemaster, + ) { + None + } else { + find_free_agent_for_stage(config, &agents, &PipelineStage::Mergemaster) + .map(str::to_string) + } + }; + + if let Some(agent_name) = mergemaster_agent { + slog!( + "[auto-assign] Content conflict on '{story_id}'; \ + auto-spawning mergemaster '{agent_name}'." + ); + crate::crdt_state::set_mergemaster_attempted(story_id, true); + if let Err(e) = self + .start_agent(project_root, story_id, Some(&agent_name), None, None) + .await + { + slog!( + "[auto-assign] Failed to start mergemaster '{agent_name}' \ + for '{story_id}': {e}" + ); + } + } + } + } } } diff --git a/server/src/agents/pool/auto_assign/story_checks.rs b/server/src/agents/pool/auto_assign/story_checks.rs index 5e4aacd5..4ca7db3e 100644 --- a/server/src/agents/pool/auto_assign/story_checks.rs +++ b/server/src/agents/pool/auto_assign/story_checks.rs @@ -29,37 +29,79 @@ pub(super) fn read_story_front_matter_agent( parse_front_matter(&contents).ok()?.agent } -/// Return `true` if the story file in the given stage has `review_hold: true` in its front matter. -pub(super) fn has_review_hold(project_root: &Path, _stage_dir: &str, story_id: &str) -> bool { - use crate::db::yaml_legacy::parse_front_matter; - let contents = match read_story_contents(project_root, story_id) { - Some(c) => c, - None => return false, - }; - parse_front_matter(&contents) +/// Return `true` if the story is in the `Frozen` pipeline stage. +/// +/// In the typed CRDT model, `Frozen` is the authoritative representation of +/// stories that are held for human review (replacing the legacy +/// `review_hold: true` YAML front-matter field). The typed stage register is +/// the only source consulted — stale YAML is ignored. +pub(super) fn has_review_hold(_project_root: &Path, _stage_dir: &str, story_id: &str) -> bool { + crate::pipeline_state::read_typed(story_id) .ok() - .and_then(|m| m.review_hold) + .flatten() + .map(|item| item.stage.is_frozen()) .unwrap_or(false) } -/// Return `true` if the story is blocked — either via the typed `Stage::Blocked` -/// variant or the legacy `blocked: true` front-matter field. -pub(super) fn is_story_blocked(project_root: &Path, _stage_dir: &str, story_id: &str) -> bool { - // Check the typed stage first (authoritative after story 866). - if let Ok(Some(item)) = crate::pipeline_state::read_typed(story_id) - && item.stage.is_blocked() - { - return true; - } - // Legacy fallback: check front-matter field for backward compatibility. - use crate::db::yaml_legacy::parse_front_matter; - let contents = match read_story_contents(project_root, story_id) { - Some(c) => c, - None => return false, - }; - parse_front_matter(&contents) +/// Return `true` if the story is blocked via the typed `Stage::Blocked` or +/// `Stage::MergeFailure` variant (or the legacy `Archived(Blocked)` state). +/// +/// The typed pipeline stage register is the only source consulted — the legacy +/// `blocked: true` YAML front-matter field is no longer checked. +pub(super) fn is_story_blocked(_project_root: &Path, _stage_dir: &str, story_id: &str) -> bool { + crate::pipeline_state::read_typed(story_id) .ok() - .and_then(|m| m.blocked) + .flatten() + .map(|item| item.stage.is_blocked()) + .unwrap_or(false) +} + +/// Return `true` if the story's merge failure contains a git content-conflict +/// marker (`"Merge conflict"` or `"CONFLICT (content):"`). +/// +/// Used by the auto-assigner to decide whether to spawn mergemaster automatically. +/// The typed stage register is consulted first; the CRDT content store is then +/// scanned for conflict markers (the projection layer does not carry the reason +/// string). No YAML front-matter parsing is performed. +pub(super) fn has_content_conflict_failure( + _project_root: &Path, + _stage_dir: &str, + story_id: &str, +) -> bool { + let is_merge_failure = crate::pipeline_state::read_typed(story_id) + .ok() + .flatten() + .map(|item| { + matches!( + item.stage, + crate::pipeline_state::Stage::MergeFailure { .. } + ) + }) + .unwrap_or(false); + if !is_merge_failure { + return false; + } + // The projection does not carry the reason string; read the raw content + // from the CRDT content store and scan for conflict markers. + crate::db::read_content(story_id) + .map(|content| { + content.contains("Merge conflict") || content.contains("CONFLICT (content):") + }) + .unwrap_or(false) +} + +/// Return `true` if the CRDT `mergemaster_attempted` register is set for this story. +/// +/// Used to prevent the auto-assigner from repeatedly spawning mergemaster for +/// the same story after a failed mergemaster session. The CRDT register is the +/// only source consulted — the legacy YAML field is no longer checked. +pub(super) fn has_mergemaster_attempted( + _project_root: &Path, + _stage_dir: &str, + story_id: &str, +) -> bool { + crate::crdt_state::read_item(story_id) + .and_then(|view| view.mergemaster_attempted) .unwrap_or(false) } @@ -120,97 +162,115 @@ pub(super) fn is_story_frozen(_project_root: &Path, _stage_dir: &str, story_id: .unwrap_or(false) } -/// Return `true` if the story file has a `merge_failure` field in its front matter. -pub(super) fn has_merge_failure(project_root: &Path, _stage_dir: &str, story_id: &str) -> bool { - use crate::db::yaml_legacy::parse_front_matter; - let contents = match read_story_contents(project_root, story_id) { - Some(c) => c, - None => return false, - }; - parse_front_matter(&contents) - .ok() - .and_then(|m| m.merge_failure) - .is_some() -} - -/// Return `true` if the story's `merge_failure` contains a git content-conflict -/// marker (`"Merge conflict"` or `"CONFLICT (content):"`). -/// -/// Used by the auto-assigner to decide whether to spawn mergemaster automatically. -pub(super) fn has_content_conflict_failure( - project_root: &Path, - _stage_dir: &str, - story_id: &str, -) -> bool { - use crate::db::yaml_legacy::parse_front_matter; - let contents = match read_story_contents(project_root, story_id) { - Some(c) => c, - None => return false, - }; - parse_front_matter(&contents) - .ok() - .and_then(|m| m.merge_failure) - .map(|reason| reason.contains("Merge conflict") || reason.contains("CONFLICT (content):")) - .unwrap_or(false) -} - -/// Return `true` if the story has `mergemaster_attempted: true` in its front matter. -/// -/// Used to prevent the auto-assigner from repeatedly spawning mergemaster for -/// the same story after a failed mergemaster session. -pub(super) fn has_mergemaster_attempted( - project_root: &Path, - _stage_dir: &str, - story_id: &str, -) -> bool { - use crate::db::yaml_legacy::parse_front_matter; - let contents = match read_story_contents(project_root, story_id) { - Some(c) => c, - None => return false, - }; - parse_front_matter(&contents) - .ok() - .and_then(|m| m.mergemaster_attempted) - .unwrap_or(false) -} - // ── Tests ────────────────────────────────────────────────────────────────── #[cfg(test)] mod tests { use super::*; + // ── has_review_hold ─────────────────────────────────────────────────────── + #[test] - fn has_review_hold_returns_true_when_set() { - let tmp = tempfile::tempdir().unwrap(); + fn has_review_hold_returns_true_when_frozen() { + crate::crdt_state::init_for_test(); crate::db::ensure_content_store(); - crate::db::write_item_with_content( - "10_spike_research", - "3_qa", - "---\nname: Research spike\nreview_hold: true\n---\n# Spike\n", - crate::db::ItemMeta::from_yaml( - "---\nname: Research spike\nreview_hold: true\n---\n# Spike\n", - ), - ); - assert!(has_review_hold(tmp.path(), "3_qa", "10_spike_research")); - } - - #[test] - fn has_review_hold_returns_false_when_not_set() { let tmp = tempfile::tempdir().unwrap(); - let qa_dir = tmp.path().join(".huskies/work/3_qa"); - std::fs::create_dir_all(&qa_dir).unwrap(); - let spike_path = qa_dir.join("10_spike_research.md"); - std::fs::write(&spike_path, "---\nname: Research spike\n---\n# Spike\n").unwrap(); - assert!(!has_review_hold(tmp.path(), "3_qa", "10_spike_research")); + crate::db::write_item_with_content( + "890_spike_frozen", + "7_frozen", + "---\nname: Frozen Spike\n---\n# Spike\n", + crate::db::ItemMeta::named("Frozen Spike"), + ); + assert!(has_review_hold(tmp.path(), "3_qa", "890_spike_frozen")); } #[test] - fn has_review_hold_returns_false_when_file_missing() { + fn has_review_hold_returns_false_for_qa_stage() { + crate::crdt_state::init_for_test(); + crate::db::ensure_content_store(); + let tmp = tempfile::tempdir().unwrap(); + crate::db::write_item_with_content( + "890_spike_active_qa", + "3_qa", + "---\nname: Active QA Spike\n---\n# Spike\n", + crate::db::ItemMeta::named("Active QA Spike"), + ); + assert!(!has_review_hold(tmp.path(), "3_qa", "890_spike_active_qa")); + } + + #[test] + fn has_review_hold_returns_false_when_story_unknown() { let tmp = tempfile::tempdir().unwrap(); assert!(!has_review_hold(tmp.path(), "3_qa", "99_spike_missing")); } + // ── is_story_blocked — regression: typed stage is sole authority ────────── + + #[test] + fn is_story_blocked_set_via_typed_stage_returns_true() { + crate::crdt_state::init_for_test(); + crate::db::ensure_content_store(); + let tmp = tempfile::tempdir().unwrap(); + crate::db::write_item_with_content( + "890_story_blocked_set", + "2_blocked", + "---\nname: Blocked Story\n---\n", + crate::db::ItemMeta::named("Blocked Story"), + ); + assert!(is_story_blocked( + tmp.path(), + "2_blocked", + "890_story_blocked_set" + )); + } + + #[test] + fn is_story_blocked_cleared_via_typed_stage_returns_false() { + crate::crdt_state::init_for_test(); + crate::db::ensure_content_store(); + let tmp = tempfile::tempdir().unwrap(); + // First set to blocked. + crate::db::write_item_with_content( + "890_story_blocked_clear", + "2_blocked", + "---\nname: Clearable Story\n---\n", + crate::db::ItemMeta::named("Clearable Story"), + ); + // Then clear by transitioning to an active stage. + crate::db::write_item_with_content( + "890_story_blocked_clear", + "2_current", + "---\nname: Clearable Story\n---\n", + crate::db::ItemMeta::named("Clearable Story"), + ); + assert!(!is_story_blocked( + tmp.path(), + "2_current", + "890_story_blocked_clear" + )); + } + + #[test] + fn is_story_blocked_stale_yaml_is_ignored() { + crate::crdt_state::init_for_test(); + crate::db::ensure_content_store(); + let tmp = tempfile::tempdir().unwrap(); + // YAML front matter says `blocked: true`, but the typed CRDT stage is backlog. + // After removing the YAML fallback, the function must return false. + crate::db::write_item_with_content( + "890_story_stale_yaml", + "1_backlog", + "---\nname: Stale\nblocked: true\n---\n", + crate::db::ItemMeta::named("Stale"), + ); + assert!( + !is_story_blocked(tmp.path(), "1_backlog", "890_story_stale_yaml"), + "stale YAML `blocked: true` must not be reported as blocked when typed stage is Backlog" + ); + } + + // ── has_unmet_dependencies ──────────────────────────────────────────────── + #[test] fn has_unmet_dependencies_returns_true_when_dep_not_done() { let tmp = tempfile::tempdir().unwrap(); diff --git a/server/src/crdt_state/state/mod.rs b/server/src/crdt_state/state/mod.rs index 4a58a78b..f4d640a2 100644 --- a/server/src/crdt_state/state/mod.rs +++ b/server/src/crdt_state/state/mod.rs @@ -113,9 +113,10 @@ pub(super) fn get_crdt() -> Option<&'static Mutex> { /// Initialise a minimal in-memory CRDT state for unit tests. /// -/// This avoids the async SQLite setup from `init()`. Ops are accepted via a -/// channel whose receiver is immediately dropped, so nothing is persisted. -/// Safe to call multiple times — subsequent calls are no-ops (OnceLock). +/// This avoids the async SQLite setup from `init()`. Ops are sent to a +/// channel whose receiver is leaked (so nothing is persisted, but the channel +/// stays open and `apply_and_persist` succeeds silently). +/// Safe to call multiple times — subsequent calls are no-ops (thread-local). #[cfg(test)] pub fn init_for_test() { // Initialise thread-local CRDT for test isolation. @@ -126,7 +127,11 @@ pub fn init_for_test() { if lock.get().is_none() { let keypair = make_keypair(); let crdt = BaseCrdt::::new(&keypair); - let (persist_tx, _rx) = mpsc::unbounded_channel(); + let (persist_tx, rx) = mpsc::unbounded_channel(); + // Leak the receiver so the channel stays open: apply_and_persist + // can then send without error, preventing [crdt_persist] WARNs + // from racing with other tests that watch the global log buffer. + std::mem::forget(rx); let state = CrdtState { crdt, keypair, diff --git a/server/src/db/yaml_legacy.rs b/server/src/db/yaml_legacy.rs index e44aadd5..a9588645 100644 --- a/server/src/db/yaml_legacy.rs +++ b/server/src/db/yaml_legacy.rs @@ -188,11 +188,6 @@ pub(crate) fn write_review_hold_in_content(contents: &str) -> String { set_front_matter_field(contents, "review_hold", "true") } -/// Write `mergemaster_attempted: true` to story content. -pub(crate) fn write_mergemaster_attempted_in_content(contents: &str) -> String { - set_front_matter_field(contents, "mergemaster_attempted", "true") -} - /// Remove a key from the YAML front matter of a story file on disk. /// /// Legacy filesystem-backed wrapper around