From 91fbad568af5aef7a5e902b6d8f510f5a9fb658d Mon Sep 17 00:00:00 2001 From: dave Date: Wed, 13 May 2026 15:30:03 +0000 Subject: [PATCH] huskies: merge 982 --- server/src/agents/lifecycle.rs | 41 +++---- .../agents/pool/auto_assign/auto_assign.rs | 68 ++++++++++++ server/src/agents/pool/auto_assign/merge.rs | 2 +- .../agents/pool/auto_assign/story_checks.rs | 105 +++++++++++++++--- .../src/agents/pool/pipeline/advance/mod.rs | 9 +- .../src/agents/pool/pipeline/merge/runner.rs | 56 +++++----- server/src/chat/commands/status/render.rs | 8 +- server/src/chat/commands/status/tests.rs | 4 +- server/src/crdt_state/read.rs | 20 +++- server/src/crdt_state/write/migrations.rs | 2 +- server/src/http/mcp/merge_tools.rs | 30 ++--- server/src/pipeline_state/mod.rs | 4 +- server/src/pipeline_state/tests.rs | 24 ++-- server/src/pipeline_state/transition.rs | 18 +-- server/src/pipeline_state/types.rs | 83 +++++++++++++- 15 files changed, 357 insertions(+), 117 deletions(-) diff --git a/server/src/agents/lifecycle.rs b/server/src/agents/lifecycle.rs index 64b650e7..658f52da 100644 --- a/server/src/agents/lifecycle.rs +++ b/server/src/agents/lifecycle.rs @@ -11,8 +11,8 @@ use std::path::Path; use std::process::Command; use crate::pipeline_state::{ - ApplyError, ArchiveReason, BranchName, GitSha, PipelineEvent, Stage, TransitionFired, - apply_transition, stage_label, + ApplyError, ArchiveReason, BranchName, GitSha, MergeFailureKind, PipelineEvent, Stage, + TransitionFired, apply_transition, stage_label, }; use crate::slog; @@ -246,10 +246,12 @@ pub fn transition_to_blocked(story_id: &str, reason: &str) -> Result<(), String> /// Transition a story from `Stage::Merge` (or `Stage::MergeFailure`) to /// `Stage::MergeFailure` via the state machine. /// -/// Builds a `PipelineEvent::MergeFailed { reason }`, validates the transition, -/// writes the resulting `Stage::MergeFailure` to the CRDT, and persists the -/// reason to the typed `MergeJob.error` CRDT register so it survives server -/// restarts (story 929: the legacy YAML write of `merge_failure: "..."` is gone). +/// Builds a `PipelineEvent::MergeFailed { kind }`, validates the transition, +/// writes the resulting `Stage::MergeFailure` to the CRDT, and persists two +/// display-only copies for status tools: +/// - `ContentKey::GateOutput`: the kind's gate-output string so the CRDT +/// projection layer can reconstruct the kind after a server restart. +/// - `MergeJob.error`: human-readable description for status renderers. /// /// When the story is already in `MergeFailure`, this is a silent self-loop: the /// returned `TransitionFired::before` will be `Stage::MergeFailure`. Callers @@ -258,26 +260,27 @@ pub fn transition_to_blocked(story_id: &str, reason: &str) -> Result<(), String> /// Returns `Err` on `TransitionError` — callers must NOT fall back to direct register writes. pub fn transition_to_merge_failure( story_id: &str, - reason: &str, + kind: MergeFailureKind, ) -> Result { - let fired = apply_transition( - story_id, - PipelineEvent::MergeFailed { - reason: reason.to_string(), - }, - None, - ) - .map_err(|e| e.to_string())?; + let display = kind.display_reason(); + let gate_output = kind.to_gate_output(); - // Persist the failure reason on the MergeJob CRDT entry so display tools - // (status_tools, chat status renderer, pipeline.rs::load_pipeline_state) - // can surface it without re-parsing YAML. + let fired = apply_transition(story_id, PipelineEvent::MergeFailed { kind }, None) + .map_err(|e| e.to_string())?; + + // Persist gate-output string so the CRDT projection can reconstruct the + // MergeFailureKind on server restart (display-only; scheduling uses the + // typed kind from the Stage variant). + crate::db::write_content(crate::db::ContentKey::GateOutput(story_id), &gate_output); + + // Persist human-readable description on the MergeJob CRDT entry so display + // tools (status renderer, pipeline state view) can surface it. crate::crdt_state::write_merge_job( story_id, "failed", chrono::Utc::now().timestamp() as f64, None, - Some(reason), + Some(&display), ); Ok(fired) diff --git a/server/src/agents/pool/auto_assign/auto_assign.rs b/server/src/agents/pool/auto_assign/auto_assign.rs index 8fb3d64a..63bdeaa1 100644 --- a/server/src/agents/pool/auto_assign/auto_assign.rs +++ b/server/src/agents/pool/auto_assign/auto_assign.rs @@ -901,4 +901,72 @@ mod tests { through the watcher bridge (story 958 regression)" ); } + + /// AC5 (story 982): a merge failure with content conflicts — seeded via the + /// typed `transition_to_merge_failure(ConflictDetected)` path without any + /// direct content-store or MergeJob writes in the test — produces + /// `Stage::MergeFailure { kind: ConflictDetected(_), .. }` and + /// auto-spawn-mergemaster fires within one `auto_assign_available_work` call. + #[tokio::test] + async fn auto_spawn_mergemaster_for_conflict_detected_kind_without_content_store_writes() { + let tmp = tempfile::tempdir().unwrap(); + let sk = tmp.path().join(".huskies"); + std::fs::create_dir_all(&sk).unwrap(); + std::fs::write( + sk.join("project.toml"), + "[[agent]]\nname = \"mergemaster\"\nstage = \"mergemaster\"\n", + ) + .unwrap(); + crate::crdt_state::init_for_test(); + crate::db::ensure_content_store(); + + let story_id = "982_ac5_conflict_auto_spawn"; + // Seed at Merge stage so the transition is valid. + crate::db::write_item_with_content( + story_id, + "4_merge", + "---\nname: AC5 auto-spawn test\n---\n", + crate::db::ItemMeta::named("AC5 auto-spawn test"), + ); + // Transition to MergeFailure(ConflictDetected) via lifecycle — no direct + // content-store writes in this test body. + crate::agents::lifecycle::transition_to_merge_failure( + story_id, + crate::pipeline_state::MergeFailureKind::ConflictDetected(Some( + "CONFLICT (content): server/src/lib.rs".to_string(), + )), + ) + .expect("transition to MergeFailure(ConflictDetected) should succeed"); + + // Verify the stage kind before triggering auto-assign. + let item = crate::pipeline_state::read_typed(story_id) + .unwrap() + .unwrap(); + assert!( + matches!( + item.stage, + crate::pipeline_state::Stage::MergeFailure { + kind: crate::pipeline_state::MergeFailureKind::ConflictDetected(_), + .. + } + ), + "stage must be MergeFailure(ConflictDetected) before auto-assign: {:?}", + item.stage + ); + + // One auto-assign cycle should spawn mergemaster. + let pool = AgentPool::new_test(3001); + pool.auto_assign_available_work(tmp.path()).await; + + let agents = pool.agents.lock().unwrap(); + let mergemaster_spawned = agents.iter().any(|(key, a)| { + key.contains(story_id) + && a.agent_name == "mergemaster" + && matches!(a.status, AgentStatus::Pending | AgentStatus::Running) + }); + assert!( + mergemaster_spawned, + "mergemaster must be auto-spawned for ConflictDetected kind in one auto-assign cycle" + ); + } } diff --git a/server/src/agents/pool/auto_assign/merge.rs b/server/src/agents/pool/auto_assign/merge.rs index 9a5d993b..c0f61753 100644 --- a/server/src/agents/pool/auto_assign/merge.rs +++ b/server/src/agents/pool/auto_assign/merge.rs @@ -120,7 +120,7 @@ impl AgentPool { // Content conflicts get one automatic mergemaster attempt; other failures // require human intervention. let merge_failure_stage = Stage::MergeFailure { - reason: String::new(), + kind: crate::pipeline_state::MergeFailureKind::Other(String::new()), feature_branch: BranchName(String::new()), commits_ahead: NonZeroU32::new(1).expect("1 is non-zero"), }; diff --git a/server/src/agents/pool/auto_assign/story_checks.rs b/server/src/agents/pool/auto_assign/story_checks.rs index baa41f5f..0b912b74 100644 --- a/server/src/agents/pool/auto_assign/story_checks.rs +++ b/server/src/agents/pool/auto_assign/story_checks.rs @@ -39,34 +39,26 @@ pub(super) fn is_story_blocked(story_id: &str) -> bool { .unwrap_or(false) } -/// Return `true` if the story's merge failure contains a git content-conflict -/// marker (`"Merge conflict"` or `"CONFLICT (content):"`). +/// Return `true` if the story's merge failure is a git content-conflict +/// (`Stage::MergeFailure { kind: ConflictDetected(_), .. }`). /// /// 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. +/// The typed kind is carried by the CRDT projection layer (which reads +/// `ContentKey::GateOutput` on projection to reconstruct the kind on restart), +/// so no direct content-store access is needed here (story 982). pub(super) fn has_content_conflict_failure(story_id: &str) -> bool { - let is_merge_failure = crate::pipeline_state::read_typed(story_id) + crate::pipeline_state::read_typed(story_id) .ok() .flatten() .map(|item| { matches!( item.stage, - crate::pipeline_state::Stage::MergeFailure { .. } + crate::pipeline_state::Stage::MergeFailure { + kind: crate::pipeline_state::MergeFailureKind::ConflictDetected(_), + .. + } ) }) - .unwrap_or(false); - if !is_merge_failure { - return false; - } - // The projection does not carry the reason string; read the gate output - // (where the merge runner persists the failure message) and scan for - // conflict markers. - crate::db::read_content(crate::db::ContentKey::GateOutput(story_id)) - .map(|content| { - content.contains("Merge conflict") || content.contains("CONFLICT (content):") - }) .unwrap_or(false) } @@ -337,4 +329,81 @@ mod tests { let archived_deps = check_archived_dependencies("503_story_waiting"); assert!(archived_deps.is_empty()); } + + // ── Story 982: typed MergeFailureKind — has_content_conflict_failure ────── + + /// AC2 (story 982): `has_content_conflict_failure` returns `true` when the + /// story is in `Stage::MergeFailure { kind: ConflictDetected(_), .. }`. + /// The test seeds the stage via `transition_to_merge_failure` (no direct + /// content-store or MergeJob writes in the test body). + #[test] + fn has_content_conflict_failure_true_for_conflict_detected_kind() { + crate::crdt_state::init_for_test(); + crate::db::ensure_content_store(); + let story_id = "982_ac2_conflict_detected"; + // Seed at Merge stage so the transition is valid. + crate::db::write_item_with_content( + story_id, + "4_merge", + "---\nname: AC2 conflict test\n---\n", + crate::db::ItemMeta::named("AC2 conflict test"), + ); + // Transition via the lifecycle helper — internally writes ContentKey::GateOutput + // so the CRDT projection can reconstruct the kind; no content-store writes here. + crate::agents::lifecycle::transition_to_merge_failure( + story_id, + crate::pipeline_state::MergeFailureKind::ConflictDetected(Some( + "CONFLICT (content): server/src/lib.rs".to_string(), + )), + ) + .expect("transition should succeed"); + + // The typed match now drives the predicate — no substring scan. + assert!( + has_content_conflict_failure(story_id), + "has_content_conflict_failure must be true for ConflictDetected kind" + ); + // Verify the projected stage carries the typed kind. + let item = crate::pipeline_state::read_typed(story_id) + .unwrap() + .unwrap(); + assert!( + matches!( + item.stage, + crate::pipeline_state::Stage::MergeFailure { + kind: crate::pipeline_state::MergeFailureKind::ConflictDetected(_), + .. + } + ), + "stage must be MergeFailure(ConflictDetected): {:?}", + item.stage + ); + } + + /// AC2 (story 982): `has_content_conflict_failure` returns `false` when the + /// kind is `GatesFailed` — no mergemaster spawn for gate-only failures. + #[test] + fn has_content_conflict_failure_false_for_gates_failed_kind() { + crate::crdt_state::init_for_test(); + crate::db::ensure_content_store(); + let story_id = "982_ac2_gates_failed"; + crate::db::write_item_with_content( + story_id, + "4_merge", + "---\nname: AC2 gates test\n---\n", + crate::db::ItemMeta::named("AC2 gates test"), + ); + crate::agents::lifecycle::transition_to_merge_failure( + story_id, + crate::pipeline_state::MergeFailureKind::GatesFailed( + "error[clippy::unused_variable]".to_string(), + ), + ) + .expect("transition should succeed"); + + assert!( + !has_content_conflict_failure(story_id), + "has_content_conflict_failure must be false for GatesFailed kind" + ); + } } diff --git a/server/src/agents/pool/pipeline/advance/mod.rs b/server/src/agents/pool/pipeline/advance/mod.rs index 06669ca1..c7628c31 100644 --- a/server/src/agents/pool/pipeline/advance/mod.rs +++ b/server/src/agents/pool/pipeline/advance/mod.rs @@ -186,12 +186,13 @@ impl AgentPool { fixup failure: {e}" ); } - let reason = format!( + let kind = crate::pipeline_state::MergeFailureKind::GatesFailed(format!( "Merge fixup coder could not resolve gate failures: {}", truncate_gate_output(&completion.gate_output) - ); + )); + let display = kind.display_reason(); if let Err(e) = - crate::agents::lifecycle::transition_to_merge_failure(story_id, &reason) + crate::agents::lifecycle::transition_to_merge_failure(story_id, kind) { slog_error!( "[pipeline] Failed to transition '{story_id}' to MergeFailure \ @@ -200,7 +201,7 @@ impl AgentPool { } let _ = self.watcher_tx.send(WatcherEvent::MergeFailure { story_id: story_id.to_string(), - reason, + reason: display, }); } } else if completion.gates_passed { diff --git a/server/src/agents/pool/pipeline/merge/runner.rs b/server/src/agents/pool/pipeline/merge/runner.rs index 52504d25..82def784 100644 --- a/server/src/agents/pool/pipeline/merge/runner.rs +++ b/server/src/agents/pool/pipeline/merge/runner.rs @@ -129,32 +129,35 @@ impl AgentPool { // On any failure: record merge_failure in CRDT and emit notification. if !success { - let reason = match &report { + let kind = match &report { Ok(r) => { if r.had_conflicts { - format!( - "Merge conflict: {}", - r.conflict_details - .as_deref() - .unwrap_or("conflicts detected") + crate::pipeline_state::MergeFailureKind::ConflictDetected( + r.conflict_details.clone(), ) } else { - format!("Quality gates failed: {}", r.gate_output) + crate::pipeline_state::MergeFailureKind::GatesFailed( + r.gate_output.clone(), + ) } } - Err(e) => e.clone(), + Err(e) => crate::pipeline_state::MergeFailureKind::Other(e.clone()), }; - let is_no_commits = reason.contains("no commits to merge"); - // Self-evident fix: gate-only failure (no conflicts) whose output matches - // a pattern a fixup coder can resolve in one short session (story 981). - let gate_output = match &report { - Ok(r) if !r.had_conflicts => r.gate_output.clone(), - _ => String::new(), + let is_no_commits = matches!( + &kind, + crate::pipeline_state::MergeFailureKind::Other(r) if r.contains("no commits to merge") + ); + // Self-evident fix: gate-only failure whose output matches a pattern + // a fixup coder can resolve in one short session (story 981). + let fixup_output = match &kind { + crate::pipeline_state::MergeFailureKind::GatesFailed(o) => o.as_str(), + _ => "", }; let is_fixup = - !is_no_commits && !gate_output.is_empty() && is_self_evident_fix(&gate_output); + !is_no_commits && !fixup_output.is_empty() && is_self_evident_fix(fixup_output); if is_no_commits { + let reason = kind.display_reason(); if let Err(e) = crate::agents::lifecycle::transition_to_blocked(&sid, &reason) { slog_error!("[merge] Failed to transition '{sid}' to Blocked: {e}"); } @@ -165,15 +168,16 @@ impl AgentPool { reason, }); } else if is_fixup { - // Save gate output and mark fixup pending before any state transition - // so that a concurrent auto-assign that fires after the state change - // sees the keys already set. - crate::db::write_content(crate::db::ContentKey::GateOutput(&sid), &gate_output); + // Mark fixup pending before any state transition so a concurrent + // auto-assign that fires after the state change sees the key set. crate::db::write_content(crate::db::ContentKey::MergeFixupPending(&sid), "1"); // Merge → MergeFailure → Coding. FixupRequested also sets - // retry_count=1 so maybe_inject_gate_failure injects the gate - // output into --append-system-prompt on the fixup spawn. - let _ = crate::agents::lifecycle::transition_to_merge_failure(&sid, &reason); + // retry_count=1 so maybe_inject_gate_failure injects gate output + // into --append-system-prompt on the fixup spawn. + // transition_to_merge_failure also writes ContentKey::GateOutput. + let display = kind.display_reason(); + let _ = + crate::agents::lifecycle::transition_to_merge_failure(sid.as_str(), kind); match crate::agents::lifecycle::move_story_to_stage(&sid, "current") { Ok(_) => { slog!( @@ -203,7 +207,7 @@ impl AgentPool { let _ = pool.watcher_tx.send( crate::io::watcher::WatcherEvent::MergeFailure { story_id: sid.clone(), - reason, + reason: display, }, ); } @@ -212,8 +216,10 @@ impl AgentPool { // Transition through the state machine (Merge → MergeFailure). // Only send the notification when the stage actually changed; if the // story was already in MergeFailure (self-loop), suppress the duplicate. + let display = kind.display_reason(); let should_notify = match crate::agents::lifecycle::transition_to_merge_failure( - &sid, &reason, + sid.as_str(), + kind, ) { Ok(fired) => !matches!( fired.before, @@ -231,7 +237,7 @@ impl AgentPool { pool.watcher_tx .send(crate::io::watcher::WatcherEvent::MergeFailure { story_id: sid.clone(), - reason, + reason: display, }); } } diff --git a/server/src/chat/commands/status/render.rs b/server/src/chat/commands/status/render.rs index bc7b35bc..f5f67030 100644 --- a/server/src/chat/commands/status/render.rs +++ b/server/src/chat/commands/status/render.rs @@ -235,12 +235,12 @@ fn render_item_line( ) { match &item.stage { // MergeFailureFinal: mergemaster already tried and gave up — always ⛔. - Stage::MergeFailureFinal { reason } => { - let snippet = first_non_empty_snippet(reason, 120); + Stage::MergeFailureFinal { kind } => { + let snippet = first_non_empty_snippet(&kind.display_reason(), 120); return format!(" \u{26D4} {display}{cost_suffix}{dep_suffix} — {snippet}\n"); } // MergeFailure: a recovery agent may be running or queued. - Stage::MergeFailure { reason, .. } => { + Stage::MergeFailure { kind, .. } => { return match agent.map(|a| &a.status) { Some(AgentStatus::Running) => format!( " \u{1F916} {display}{cost_suffix}{dep_suffix} — mergemaster running\n" @@ -249,7 +249,7 @@ fn render_item_line( " \u{23F3} {display}{cost_suffix}{dep_suffix} — mergemaster queued\n" ), _ => { - let snippet = first_non_empty_snippet(reason, 120); + let snippet = first_non_empty_snippet(&kind.display_reason(), 120); format!(" \u{26D4} {display}{cost_suffix}{dep_suffix} — {snippet}\n") } }; diff --git a/server/src/chat/commands/status/tests.rs b/server/src/chat/commands/status/tests.rs index c8d85be9..afc9202b 100644 --- a/server/src/chat/commands/status/tests.rs +++ b/server/src/chat/commands/status/tests.rs @@ -901,7 +901,9 @@ fn merge_failure_item_appears_in_merge_section_not_blocked() { "100_story_merge_fail", "Merge Failure Story", Stage::MergeFailure { - reason: "conflict in lib.rs".to_string(), + kind: crate::pipeline_state::MergeFailureKind::ConflictDetected(Some( + "conflict in lib.rs".to_string(), + )), feature_branch: BranchName("feature/100".to_string()), commits_ahead: std::num::NonZeroU32::new(1).unwrap(), }, diff --git a/server/src/crdt_state/read.rs b/server/src/crdt_state/read.rs index 0de1b020..62234c8b 100644 --- a/server/src/crdt_state/read.rs +++ b/server/src/crdt_state/read.rs @@ -442,13 +442,21 @@ fn project_stage_for_view( feature_branch: BranchName(format!("feature/story-{story_id}")), commits_ahead: NonZeroU32::new(1).expect("1 is non-zero"), }), - "merge_failure" => Some(Stage::MergeFailure { - reason: String::new(), - feature_branch: BranchName(format!("feature/story-{story_id}")), - commits_ahead: NonZeroU32::new(1).expect("1 is non-zero"), - }), + "merge_failure" => { + // Reconstruct the typed kind from ContentKey::GateOutput so the + // auto-assigner can match on the variant after a server restart. + // This is the sole persistence backing for MergeFailureKind. + let kind = crate::db::read_content(crate::db::ContentKey::GateOutput(story_id)) + .map(|s| crate::pipeline_state::MergeFailureKind::infer_from_gate_output(&s)) + .unwrap_or(crate::pipeline_state::MergeFailureKind::Other(String::new())); + Some(Stage::MergeFailure { + kind, + feature_branch: BranchName(format!("feature/story-{story_id}")), + commits_ahead: NonZeroU32::new(1).expect("1 is non-zero"), + }) + } "merge_failure_final" => Some(Stage::MergeFailureFinal { - reason: String::new(), + kind: crate::pipeline_state::MergeFailureKind::Other(String::new()), }), "frozen" => Some(Stage::Frozen { resume_to: resume_target(), diff --git a/server/src/crdt_state/write/migrations.rs b/server/src/crdt_state/write/migrations.rs index 60d509b0..d9d331d3 100644 --- a/server/src/crdt_state/write/migrations.rs +++ b/server/src/crdt_state/write/migrations.rs @@ -338,7 +338,7 @@ mod stage_migration_tests { "9507_legacy_merge_failure", "4_merge_failure", Stage::MergeFailure { - reason: String::new(), + kind: crate::pipeline_state::MergeFailureKind::Other(String::new()), feature_branch: crate::pipeline_state::BranchName(String::new()), commits_ahead: NonZeroU32::new(1).unwrap(), }, diff --git a/server/src/http/mcp/merge_tools.rs b/server/src/http/mcp/merge_tools.rs index 34908c7e..c01ed899 100644 --- a/server/src/http/mcp/merge_tools.rs +++ b/server/src/http/mcp/merge_tools.rs @@ -177,27 +177,31 @@ pub(super) fn tool_report_merge_failure(args: &Value, ctx: &AppContext) -> Resul slog!("[mergemaster] Merge failure reported for '{story_id}': {reason}"); ctx.services.agents.set_merge_failure_reported(story_id); + // The mergemaster provides a freeform reason string; use Other so the + // auto-assigner does not re-spawn another mergemaster after this one fails. + let kind = crate::pipeline_state::MergeFailureKind::Other(reason.to_string()); + let display = kind.display_reason(); + // Route the failure through the typed state machine (Merge → MergeFailure). - // This persists the reason in front matter and updates the CRDT stage. // Only broadcast the notification when the stage actually changed; if the // story was already in MergeFailure (self-loop), suppress the duplicate. - let should_notify = - match crate::agents::lifecycle::transition_to_merge_failure(story_id, reason) { - Ok(fired) => !matches!( - fired.before, - crate::pipeline_state::Stage::MergeFailure { .. } - ), - Err(e) => { - slog_warn!("[mergemaster] Failed to transition '{story_id}' to MergeFailure: {e}"); - true - } - }; + let should_notify = match crate::agents::lifecycle::transition_to_merge_failure(story_id, kind) + { + Ok(fired) => !matches!( + fired.before, + crate::pipeline_state::Stage::MergeFailure { .. } + ), + Err(e) => { + slog_warn!("[mergemaster] Failed to transition '{story_id}' to MergeFailure: {e}"); + true + } + }; if should_notify { let _ = ctx .watcher_tx .send(crate::io::watcher::WatcherEvent::MergeFailure { story_id: story_id.to_string(), - reason: reason.to_string(), + reason: display, }); } diff --git a/server/src/pipeline_state/mod.rs b/server/src/pipeline_state/mod.rs index 5a8ddc83..65636a56 100644 --- a/server/src/pipeline_state/mod.rs +++ b/server/src/pipeline_state/mod.rs @@ -40,8 +40,8 @@ mod tests; #[allow(unused_imports)] pub use types::{ - AgentName, ArchiveReason, BranchName, ExecutionState, GitSha, NodePubkey, PipelineItem, Stage, - StoryId, TransitionError, stage_dir_name, stage_label, + AgentName, ArchiveReason, BranchName, ExecutionState, GitSha, MergeFailureKind, NodePubkey, + PipelineItem, Stage, StoryId, TransitionError, stage_dir_name, stage_label, }; #[allow(unused_imports)] diff --git a/server/src/pipeline_state/tests.rs b/server/src/pipeline_state/tests.rs index e93deaa2..067b6e3f 100644 --- a/server/src/pipeline_state/tests.rs +++ b/server/src/pipeline_state/tests.rs @@ -648,16 +648,20 @@ fn merge_failure_transition_emits_event_with_full_reason() { let fired = super::apply::apply_transition( story_id, PipelineEvent::MergeFailed { - reason: reason.to_string(), + kind: MergeFailureKind::Other(reason.to_string()), }, None, ) .expect("MergeFailed transition should succeed"); - // The emitted event payload carries the full reason string. + // The emitted event payload carries the full reason via display_reason(). match &fired.event { - PipelineEvent::MergeFailed { reason: r } => { - assert_eq!(r, reason, "emitted event should carry the full reason"); + PipelineEvent::MergeFailed { kind } => { + assert_eq!( + kind.display_reason(), + reason, + "emitted event should carry the full reason" + ); } other => panic!("expected MergeFailed event, got: {other:?}"), } @@ -686,14 +690,14 @@ fn merge_failure_transition_emits_event_with_full_reason() { #[test] fn merge_failure_plus_merge_failed_is_self_loop() { let s = Stage::MergeFailure { - reason: "initial failure".into(), + kind: MergeFailureKind::Other("initial failure".into()), feature_branch: fb("feature/story-1"), commits_ahead: nz(1), }; let result = transition( s, PipelineEvent::MergeFailed { - reason: "second failure".into(), + kind: MergeFailureKind::Other("second failure".into()), }, ); assert!( @@ -722,7 +726,7 @@ fn repeated_merge_failure_apply_transition_no_error_no_duplicate_notification() let fired = super::apply::apply_transition( story_id, PipelineEvent::MergeFailed { - reason: "duplicate failure".into(), + kind: MergeFailureKind::Other("duplicate failure".into()), }, None, ) @@ -766,7 +770,7 @@ fn repeated_merge_failure_apply_transition_no_error_no_duplicate_notification() #[test] fn merge_failure_unblock_returns_to_merge() { let s = Stage::MergeFailure { - reason: "conflicts in server/src/main.rs".into(), + kind: MergeFailureKind::ConflictDetected(Some("conflicts in server/src/main.rs".into())), feature_branch: fb("feature/story-42"), commits_ahead: nz(3), }; @@ -781,7 +785,7 @@ fn merge_failure_unblock_returns_to_merge() { #[test] fn merge_failure_demote_returns_to_backlog() { let s = Stage::MergeFailure { - reason: "conflicts".into(), + kind: MergeFailureKind::Other("conflicts".into()), feature_branch: fb("feature/story-1"), commits_ahead: nz(1), }; @@ -799,7 +803,7 @@ fn merge_failure_demote_returns_to_backlog() { #[test] fn merge_failure_accept_pure_transition() { let s = Stage::MergeFailure { - reason: "conflicts unresolvable".into(), + kind: MergeFailureKind::ConflictDetected(Some("conflicts unresolvable".into())), feature_branch: fb("feature/story-1"), commits_ahead: nz(1), }; diff --git a/server/src/pipeline_state/transition.rs b/server/src/pipeline_state/transition.rs index e2723642..a0c62c72 100644 --- a/server/src/pipeline_state/transition.rs +++ b/server/src/pipeline_state/transition.rs @@ -4,8 +4,8 @@ use chrono::Utc; use serde::{Deserialize, Serialize}; use super::{ - AgentName, ArchiveReason, BranchName, ExecutionState, GitSha, Stage, StoryId, TransitionError, - stage_label, + AgentName, ArchiveReason, BranchName, ExecutionState, GitSha, MergeFailureKind, Stage, StoryId, + TransitionError, stage_label, }; // ── Pipeline events ───────────────────────────────────────────────────────── @@ -35,7 +35,7 @@ pub enum PipelineEvent { MergeSucceeded { merge_commit: GitSha }, /// Merge pipeline failed (conflicts or gate failures); story moves to /// `Stage::MergeFailure` awaiting human intervention or retry. - MergeFailed { reason: String }, + MergeFailed { kind: MergeFailureKind }, /// Mergemaster gave up after retry budget. MergeFailedFinal { reason: String }, /// Story accepted (Done → Archived). @@ -214,9 +214,9 @@ pub fn transition(state: Stage, event: PipelineEvent) -> Result Ok(MergeFailure { - reason, + kind, feature_branch, commits_ahead, }), @@ -231,9 +231,9 @@ pub fn transition(state: Stage, event: PipelineEvent) -> Result Ok(MergeFailure { - reason, + kind, feature_branch, commits_ahead, }), @@ -326,8 +326,8 @@ pub fn transition(state: Stage, event: PipelineEvent) -> Result Ok(Coding), // ── MergemasterAttempted: MergeFailure → MergeFailureFinal ───── - (MergeFailure { reason, .. }, MergemasterAttempted) => Ok(MergeFailureFinal { reason }), - (MergeFailureFinal { reason }, MergemasterAttempted) => Ok(MergeFailureFinal { reason }), + (MergeFailure { kind, .. }, MergemasterAttempted) => Ok(MergeFailureFinal { kind }), + (MergeFailureFinal { kind }, MergemasterAttempted) => Ok(MergeFailureFinal { kind }), // ── Unblock: from Frozen/ReviewHold → resume_to ──────────────── (Frozen { resume_to }, Unblock) => Ok(*resume_to), diff --git a/server/src/pipeline_state/types.rs b/server/src/pipeline_state/types.rs index 03562f15..2926abbc 100644 --- a/server/src/pipeline_state/types.rs +++ b/server/src/pipeline_state/types.rs @@ -39,6 +39,76 @@ impl fmt::Display for AgentName { } } +// ── Merge failure kind ────────────────────────────────────────────────────── + +/// Typed reason for a merge pipeline failure. +/// +/// Replaces the freeform `reason: String` that was previously stored on +/// `Stage::MergeFailure` (story 982). Consumers match on the variant instead +/// of scanning substrings of an opaque string. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum MergeFailureKind { + /// Git content conflicts detected during the squash-rebase. The payload + /// is the raw conflict output (e.g. the git CONFLICT lines), if available. + ConflictDetected(Option), + /// Quality gates (fmt, clippy, tests) failed after the squash merge. + /// The payload is the gate runner output. + GatesFailed(String), + /// Feature branch has no code changes ahead of master (empty diff). + EmptyDiff, + /// Squash merge produced no commits (e.g. empty-commit history). + NoCommits, + /// Unclassified merge failure; the payload carries the original message. + Other(String), +} + +impl MergeFailureKind { + /// Human-readable description for display tools and `MergeJob.error`. + pub fn display_reason(&self) -> String { + match self { + MergeFailureKind::ConflictDetected(details) => format!( + "Merge conflict: {}", + details.as_deref().unwrap_or("conflicts detected") + ), + MergeFailureKind::GatesFailed(output) => format!("Quality gates failed: {output}"), + MergeFailureKind::EmptyDiff => "Feature branch has no code changes".to_string(), + MergeFailureKind::NoCommits => "No commits to merge".to_string(), + MergeFailureKind::Other(reason) => reason.clone(), + } + } + + /// String to persist in `ContentKey::GateOutput` so the CRDT projection + /// layer can reconstruct the kind after a server restart. + pub fn to_gate_output(&self) -> String { + match self { + // Always prefix with "Merge conflict:" so `infer_from_gate_output` + // can identify ConflictDetected on reload. + MergeFailureKind::ConflictDetected(details) => format!( + "Merge conflict: {}", + details.as_deref().unwrap_or("conflicts detected") + ), + // Raw gate output is the natural identifier for a GatesFailed kind. + MergeFailureKind::GatesFailed(output) => output.clone(), + MergeFailureKind::EmptyDiff => "empty diff".to_string(), + MergeFailureKind::NoCommits => "no commits to merge".to_string(), + MergeFailureKind::Other(reason) => reason.clone(), + } + } + + /// Infer a kind from a `ContentKey::GateOutput` string persisted by a + /// previous call to [`Self::to_gate_output`] or by legacy code that wrote + /// conflict markers directly. Used by the CRDT projection layer. + pub fn infer_from_gate_output(gate_output: &str) -> Self { + if gate_output.contains("CONFLICT (content):") || gate_output.contains("Merge conflict") { + MergeFailureKind::ConflictDetected(Some(gate_output.to_string())) + } else if gate_output.is_empty() { + MergeFailureKind::Other(String::new()) + } else { + MergeFailureKind::GatesFailed(gate_output.to_string()) + } + } +} + // ── Synced pipeline stage (lives in CRDT, converges across nodes) ─────────── /// The pipeline stage for a work item. @@ -111,7 +181,9 @@ pub enum Stage { /// this is a recoverable intermediate state — `Unblock` returns to `Merge` /// (re-queues the merge) and `Demote` returns to `Backlog` (manual park). MergeFailure { - reason: String, + /// Typed failure reason — callers match on the variant instead of + /// substring-scanning a freeform string (story 982). + kind: MergeFailureKind, /// Branch and commit count preserved from the preceding `Merge` state /// so `Unblock` can reconstruct the exact `Merge` variant. feature_branch: BranchName, @@ -122,7 +194,10 @@ pub enum Stage { /// recover; the agent gave up. The story stays here awaiting human /// intervention — the auto-assigner will NOT spawn mergemaster again. /// Replaces the legacy `mergemaster_attempted: true` boolean flag. - MergeFailureFinal { reason: String }, + MergeFailureFinal { + /// Typed failure reason carried through from the preceding `MergeFailure`. + kind: MergeFailureKind, + }, /// Story is frozen — kept at this stage as a snapshot of its previous /// stage. Replaces the legacy `frozen: true` boolean flag: there is no @@ -229,12 +304,12 @@ impl Stage { commits_ahead: NonZeroU32::new(1).expect("1 is non-zero"), }), "merge_failure" => Some(Stage::MergeFailure { - reason: String::new(), + kind: MergeFailureKind::Other(String::new()), feature_branch: BranchName(String::new()), commits_ahead: NonZeroU32::new(1).expect("1 is non-zero"), }), "merge_failure_final" => Some(Stage::MergeFailureFinal { - reason: String::new(), + kind: MergeFailureKind::Other(String::new()), }), "frozen" => Some(Stage::Frozen { resume_to: Box::new(Stage::Coding),