diff --git a/server/src/agents/lifecycle.rs b/server/src/agents/lifecycle.rs index b86cb41c..3c1af782 100644 --- a/server/src/agents/lifecycle.rs +++ b/server/src/agents/lifecycle.rs @@ -12,8 +12,8 @@ use std::process::Command; use crate::db::yaml_legacy::clear_front_matter_field_in_content; use crate::pipeline_state::{ - ApplyError, ArchiveReason, BranchName, GitSha, PipelineEvent, Stage, apply_transition, - stage_label, + ApplyError, ArchiveReason, BranchName, GitSha, PipelineEvent, Stage, TransitionFired, + apply_transition, stage_label, }; use crate::slog; @@ -248,13 +248,22 @@ pub fn transition_to_blocked(story_id: &str, reason: &str) -> Result<(), String> .map_err(|e| e.to_string()) } -/// Transition a story from `Stage::Merge` to `Stage::MergeFailure` via the state machine. +/// 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 front /// matter so it survives server restarts. +/// +/// When the story is already in `MergeFailure`, this is a silent self-loop: the +/// returned `TransitionFired::before` will be `Stage::MergeFailure`. Callers +/// should suppress re-notification in that case to avoid duplicate chat messages. +/// /// Returns `Err` on `TransitionError` — callers must NOT fall back to direct register writes. -pub fn transition_to_merge_failure(story_id: &str, reason: &str) -> Result<(), String> { +pub fn transition_to_merge_failure( + story_id: &str, + reason: &str, +) -> Result { let reason_owned = reason.to_string(); let transform: Box String> = Box::new(move |content: &str| { crate::db::yaml_legacy::write_merge_failure_in_content(content, &reason_owned) @@ -266,7 +275,6 @@ pub fn transition_to_merge_failure(story_id: &str, reason: &str) -> Result<(), S }, Some(&*transform), ) - .map(|_| ()) .map_err(|e| e.to_string()) } diff --git a/server/src/agents/pool/pipeline/merge/runner.rs b/server/src/agents/pool/pipeline/merge/runner.rs index c5d3dd84..4f8e84db 100644 --- a/server/src/agents/pool/pipeline/merge/runner.rs +++ b/server/src/agents/pool/pipeline/merge/runner.rs @@ -126,19 +126,30 @@ impl AgentPool { }); } else { // Transition through the state machine (Merge → MergeFailure). - if let Err(e) = - crate::agents::lifecycle::transition_to_merge_failure(&sid, &reason) - { - crate::slog_error!( - "[merge] Failed to transition '{sid}' to MergeFailure: {e}" - ); + // Only send 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( + &sid, &reason, + ) { + Ok(fired) => !matches!( + fired.before, + crate::pipeline_state::Stage::MergeFailure { .. } + ), + Err(e) => { + crate::slog_error!( + "[merge] Failed to transition '{sid}' to MergeFailure: {e}" + ); + true + } + }; + if should_notify { + let _ = + pool.watcher_tx + .send(crate::io::watcher::WatcherEvent::MergeFailure { + story_id: sid.clone(), + reason, + }); } - let _ = pool - .watcher_tx - .send(crate::io::watcher::WatcherEvent::MergeFailure { - story_id: sid.clone(), - reason, - }); } } diff --git a/server/src/http/mcp/merge_tools.rs b/server/src/http/mcp/merge_tools.rs index 30bd1617..253dd6c7 100644 --- a/server/src/http/mcp/merge_tools.rs +++ b/server/src/http/mcp/merge_tools.rs @@ -179,19 +179,28 @@ 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); - // Broadcast the failure so the Matrix notification listener can post an - // error message to configured rooms without coupling this tool to the bot. - let _ = ctx - .watcher_tx - .send(crate::io::watcher::WatcherEvent::MergeFailure { - story_id: story_id.to_string(), - reason: reason.to_string(), - }); - // Route the failure through the typed state machine (Merge → MergeFailure). // This persists the reason in front matter and updates the CRDT stage. - if let Err(e) = crate::agents::lifecycle::transition_to_merge_failure(story_id, reason) { - slog_warn!("[mergemaster] Failed to transition '{story_id}' to MergeFailure: {e}"); + // 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 + } + }; + if should_notify { + let _ = ctx + .watcher_tx + .send(crate::io::watcher::WatcherEvent::MergeFailure { + story_id: story_id.to_string(), + reason: reason.to_string(), + }); } Ok(format!( diff --git a/server/src/pipeline_state/tests.rs b/server/src/pipeline_state/tests.rs index e4a11144..123589c4 100644 --- a/server/src/pipeline_state/tests.rs +++ b/server/src/pipeline_state/tests.rs @@ -750,4 +750,82 @@ fn merge_failure_transition_emits_event_with_full_reason() { ); } +// ── Story 913: MergeFailure + MergeFailed self-loop ──────────────── + +/// AC1: `MergeFailure + MergeFailed` is a valid self-transition — no error logged. +#[test] +fn merge_failure_plus_merge_failed_is_self_loop() { + let s = Stage::MergeFailure { + reason: "initial failure".into(), + }; + let result = transition( + s, + PipelineEvent::MergeFailed { + reason: "second failure".into(), + }, + ); + assert!( + matches!(result, Ok(Stage::MergeFailure { .. })), + "MergeFailure + MergeFailed should self-loop to MergeFailure, got: {result:?}" + ); +} + +/// AC2 + AC3: applying `MergeFailed` to a story already in `MergeFailure` succeeds and +/// the `TransitionFired::before` is `MergeFailure`, allowing callers to suppress the +/// duplicate notification. +#[test] +fn repeated_merge_failure_apply_transition_no_error_no_duplicate_notification() { + crate::crdt_state::init_for_test(); + crate::db::ensure_content_store(); + + let story_id = "99913_story_merge_failure_selfloop"; + crate::db::write_item_with_content( + story_id, + "4_merge_failure", + "---\nname: MergeFailure Self-loop Test\n---\n# Story\n", + crate::db::ItemMeta::from_yaml("---\nname: MergeFailure Self-loop Test\n---\n# Story\n"), + ); + + // Apply a second MergeFailed to a story already in MergeFailure. + let fired = super::apply::apply_transition( + story_id, + PipelineEvent::MergeFailed { + reason: "duplicate failure".into(), + }, + None, + ) + .expect("MergeFailed on already-failed story should succeed without error"); + + // The before-stage was MergeFailure: this was a self-loop. + // Callers check this to decide whether to suppress the chat notification. + assert!( + matches!(fired.before, Stage::MergeFailure { .. }), + "fired.before should be MergeFailure (self-loop): {:?}", + fired.before + ); + assert!( + matches!(fired.after, Stage::MergeFailure { .. }), + "fired.after should remain MergeFailure: {:?}", + fired.after + ); + + // Verify the CRDT stage is still 4_merge_failure. + let item = read_typed(story_id) + .expect("CRDT read should succeed") + .expect("item should still exist"); + assert_eq!( + item.stage.dir_name(), + "4_merge_failure", + "CRDT stage should remain 4_merge_failure after self-loop" + ); + + // Simulate the caller's de-dup logic: since fired.before is already MergeFailure, + // no notification should be dispatched. + let should_notify = !matches!(fired.before, Stage::MergeFailure { .. }); + assert!( + !should_notify, + "should_notify must be false for a self-loop to prevent duplicate notification" + ); +} + // ── ProjectionError Display ───────────────────────────────────────── diff --git a/server/src/pipeline_state/transition.rs b/server/src/pipeline_state/transition.rs index 2061d3fc..4ee81bf2 100644 --- a/server/src/pipeline_state/transition.rs +++ b/server/src/pipeline_state/transition.rs @@ -181,6 +181,12 @@ pub fn transition(state: Stage, event: PipelineEvent) -> Result Ok(MergeFailure { reason }), + // ── MergeFailure self-loop: repeated failure is a no-op ───────────── + // When the mergemaster retries and fails again while the story is already + // in MergeFailure, treat it as a silent self-transition so callers can + // detect the no-op via `fired.before == MergeFailure` and skip re-notifying. + (MergeFailure { .. }, MergeFailed { reason }) => Ok(MergeFailure { reason }), + (Merge { .. }, MergeFailedFinal { reason }) => Ok(Archived { archived_at: now, reason: ArchiveReason::MergeFailed { reason },