diff --git a/server/src/agents/lifecycle.rs b/server/src/agents/lifecycle.rs index 46c0f35a..8261e596 100644 --- a/server/src/agents/lifecycle.rs +++ b/server/src/agents/lifecycle.rs @@ -121,7 +121,7 @@ pub fn move_story_to_done(story_id: &str) -> Result<(), String> { Stage::Merge { .. } => PipelineEvent::MergeSucceeded { merge_commit: GitSha("accepted".to_string()), }, - Stage::MergeFailure { .. } => PipelineEvent::Accepted, + Stage::MergeFailure { .. } | Stage::MergeFailureFinal { .. } => PipelineEvent::Accepted, Stage::Coding { .. } | Stage::Qa | Stage::Backlog => PipelineEvent::Close, _ => { return Err(format!( @@ -627,6 +627,34 @@ mod tests { ); } + /// Regression test (story 1178): a story in `Stage::MergeFailureFinal` + /// whose merge is later retried and succeeds must be movable to Done. + /// Before this fix, `move_story_to_done` had no arm for + /// `MergeFailureFinal`, so it always returned an error even after a + /// real, successful re-merge — the exact "trap state" this story fixes. + #[test] + fn move_story_to_done_from_merge_failure_final_succeeds() { + crate::db::ensure_content_store(); + crate::db::write_item_with_content( + "99952_story_merge_failure_final", + "merge_failure_final", + "---\nname: Merge Failure Final Test\n---\n# Story\n", + crate::db::ItemMeta::named("Merge Failure Final Test"), + ); + + move_story_to_done("99952_story_merge_failure_final") + .expect("move_story_to_done should succeed from MergeFailureFinal"); + + let item = crate::pipeline_state::read_typed("99952_story_merge_failure_final") + .expect("CRDT read should succeed") + .expect("item should exist in CRDT"); + assert_eq!( + item.stage.dir_name(), + "done", + "item should be in done after move from MergeFailureFinal" + ); + } + // ── item_type_from_id tests ──────────────────────────────────────────────── #[test] diff --git a/server/src/agents/pool/pipeline/merge/runner.rs b/server/src/agents/pool/pipeline/merge/runner.rs index 21286aff..c1c102b4 100644 --- a/server/src/agents/pool/pipeline/merge/runner.rs +++ b/server/src/agents/pool/pipeline/merge/runner.rs @@ -361,15 +361,23 @@ impl AgentPool { } let story_archived = crate::agents::lifecycle::move_story_to_done(story_id).is_ok(); - if story_archived { - self.remove_agents_for_story(story_id).await; - } - let worktree_cleaned_up = if wt_path.exists() { - let config = crate::config::ProjectConfig::load(project_root).unwrap_or_default(); - worktree::remove_worktree_by_story_id(project_root, story_id, &config) - .await - .is_ok() + // Story 1178: only delete the feature branch once the state transition + // to Done is confirmed. Deleting it unconditionally here meant a + // successful squash merge whose CRDT transition failed (e.g. the story + // was in a stage `move_story_to_done` didn't yet handle) would still + // lose its feature branch, making the failure unrecoverable — the + // story couldn't be retried because the branch it needed was gone. + let worktree_cleaned_up = if story_archived { + self.remove_agents_for_story(story_id).await; + if wt_path.exists() { + let config = crate::config::ProjectConfig::load(project_root).unwrap_or_default(); + worktree::remove_worktree_by_story_id(project_root, story_id, &config) + .await + .is_ok() + } else { + false + } } else { false }; diff --git a/server/src/agents/pool/pipeline/merge/tests.rs b/server/src/agents/pool/pipeline/merge/tests.rs index c1afb995..425e463c 100644 --- a/server/src/agents/pool/pipeline/merge/tests.rs +++ b/server/src/agents/pool/pipeline/merge/tests.rs @@ -409,6 +409,97 @@ async fn merge_agent_work_succeeds_on_clean_branch() { } } +/// Regression test (story 1178, AC3): when the squash merge itself succeeds +/// but the CRDT state transition to Done fails (here: no CRDT entry exists +/// for the story, so `move_story_to_done` errors and `story_archived` is +/// false), the feature branch must NOT be deleted — otherwise a retry has +/// nothing to merge from. +#[tokio::test] +async fn merge_success_without_story_archived_keeps_feature_branch() { + let _serial = serial_test_lock(); + use std::fs; + use tempfile::tempdir; + + crate::crdt_state::init_for_test(); + let tmp = tempdir().unwrap(); + let repo = tmp.path(); + init_git_repo(repo); + + let branch = "feature/story-1178_branch_kept"; + Command::new("git") + .args(["checkout", "-b", branch]) + .current_dir(repo) + .output() + .unwrap(); + fs::write(repo.join("feature.txt"), "feature content").unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "add feature"]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["checkout", "master"]) + .current_dir(repo) + .output() + .unwrap(); + + let merge_dir = repo.join(".huskies/work/4_merge"); + fs::create_dir_all(&merge_dir).unwrap(); + fs::write( + merge_dir.join("1178_branch_kept.md"), + "---\nname: Branch Kept Test\n---\n", + ) + .unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "add story in merge"]) + .current_dir(repo) + .output() + .unwrap(); + + let pool = Arc::new(AgentPool::new_test(3001)); + // Note: no CRDT entry is written for this story, so `move_story_to_done` + // will fail with NotFound — `story_archived` will be false regardless of + // git merge outcome. That is exactly the scenario this test protects. + let job = run_merge_to_completion(&pool, repo, "1178_branch_kept").await; + + let MergeJobStatus::Completed(report) = &job.status else { + panic!("expected a completed job, got: {:?}", job.status); + }; + if matches!( + report.result, + crate::agents::merge::MergeResult::Success { .. } + ) { + assert!( + !report.story_archived, + "story_archived should be false: no CRDT entry exists for this story" + ); + assert!( + !report.worktree_cleaned_up, + "worktree/branch must not be cleaned up when story_archived is false" + ); + let branch_check = Command::new("git") + .args(["rev-parse", "--verify", branch]) + .current_dir(repo) + .output() + .unwrap(); + assert!( + branch_check.status.success(), + "feature branch '{branch}' must still exist after a merge whose state \ + transition to Done failed, so a retry stays possible" + ); + } +} + // ── quality gate ordering test ──────────────────────────────── /// Regression test for bug 142: quality gates must run BEFORE the fast-forward diff --git a/server/src/pipeline_state/tests.rs b/server/src/pipeline_state/tests.rs index 449c93ab..679cf320 100644 --- a/server/src/pipeline_state/tests.rs +++ b/server/src/pipeline_state/tests.rs @@ -760,6 +760,26 @@ fn merge_failure_transition_emits_event_with_full_reason() { ); } +// ── Story 1178: MergeFailureFinal is not a trap state ─────────────── + +/// Regression test (story 1178): a story stuck in `Stage::MergeFailureFinal` +/// whose merge is later retried and actually succeeds must be able to reach +/// `Stage::Done` via `PipelineEvent::Accepted` — mirroring the existing +/// `MergeFailure -> Done` recovery path. Before this fix, `MergeFailureFinal` +/// had no `Accepted` arm at all, so `move_story_to_done` always errored even +/// after a successful re-merge. +#[test] +fn merge_failure_final_accepted_transitions_to_done() { + let s = Stage::MergeFailureFinal { + kind: MergeFailureKind::Other("gate failures".into()), + }; + let s = transition(s, PipelineEvent::Accepted).unwrap(); + assert!( + matches!(s, Stage::Done { .. }), + "MergeFailureFinal + Accepted should land in Done, got: {s:?}" + ); +} + // ── Story 913: MergeFailure + MergeFailed self-loop ──────────────── /// AC1: `MergeFailure + MergeFailed` is a valid self-transition — no error logged. diff --git a/server/src/pipeline_state/transition.rs b/server/src/pipeline_state/transition.rs index 07a4883f..ae168faf 100644 --- a/server/src/pipeline_state/transition.rs +++ b/server/src/pipeline_state/transition.rs @@ -210,6 +210,17 @@ pub fn transition(state: Stage, event: PipelineEvent) -> Result Done` arm above. + (MergeFailureFinal { .. }, Accepted) => Ok(Done { + merged_at: now, + merge_commit: GitSha("manual".to_string()), + }), + // ── Block: any active → Blocked ────────────────────────────── (Backlog, Block { reason }) | (Coding { .. }, Block { reason }) diff --git a/server/src/service/merge/status.rs b/server/src/service/merge/status.rs index e3a8118d..33086180 100644 --- a/server/src/service/merge/status.rs +++ b/server/src/service/merge/status.rs @@ -5,30 +5,41 @@ use crate::agents::merge::{MergeReport, MergeResult}; -#[allow(dead_code)] /// Derive a human-readable status message from a completed [`MergeReport`]. /// /// The message explains what happened and (on failure) what the caller -/// should do next. -pub fn format_merge_status_message(report: &MergeReport) -> &'static str { +/// should do next. On success, the wording reflects what +/// [`MergeReport::story_archived`] and [`MergeReport::worktree_cleaned_up`] +/// actually recorded — story 1178: the message must never claim "moved to +/// done" when the CRDT state transition to `Done` failed. +pub fn format_merge_status_message(report: &MergeReport) -> String { match &report.result { MergeResult::Success { - conflicts_resolved: true, - .. + conflicts_resolved, .. } => { - "Merge complete: conflicts were auto-resolved and all quality gates passed. Story moved to done and worktree cleaned up." - } - MergeResult::Success { .. } => { - "Merge complete: all quality gates passed. Story moved to done and worktree cleaned up." + let prefix = if *conflicts_resolved { + "Merge complete: conflicts were auto-resolved and all quality gates passed." + } else { + "Merge complete: all quality gates passed." + }; + match (report.story_archived, report.worktree_cleaned_up) { + (true, true) => format!("{prefix} Story moved to done and worktree cleaned up."), + (true, false) => { + format!("{prefix} Story moved to done, but worktree cleanup failed.") + } + (false, _) => format!( + "{prefix} The merge landed on master, but the story could not be moved to done — it needs manual follow-up." + ), + } } MergeResult::Conflict { .. } => { - "Merge failed: conflicts detected that could not be auto-resolved. Merge was aborted — master is untouched. Call report_merge_failure with the conflict details so the human can resolve them. Do NOT manually move the story file or call accept_story." + "Merge failed: conflicts detected that could not be auto-resolved. Merge was aborted — master is untouched. Call report_merge_failure with the conflict details so the human can resolve them. Do NOT manually move the story file or call accept_story.".to_string() } MergeResult::GateFailure { .. } => { - "Merge committed but quality gates failed. Review gate_output and fix issues before re-running." + "Merge committed but quality gates failed. Review gate_output and fix issues before re-running.".to_string() } MergeResult::NoCommits { .. } | MergeResult::Other { .. } => { - "Merge failed. Review gate_output for details. Call report_merge_failure to record the failure. Do NOT manually move the story file or call accept_story." + "Merge failed. Review gate_output for details. Call report_merge_failure to record the failure. Do NOT manually move the story file or call accept_story.".to_string() } } } @@ -51,25 +62,48 @@ mod tests { #[test] fn clean_merge_message() { + let mut r = make_report(MergeResult::Success { + conflicts_resolved: false, + conflict_details: None, + gate_output: String::new(), + }); + r.story_archived = true; + r.worktree_cleaned_up = true; + let msg = format_merge_status_message(&r); + assert!(msg.contains("quality gates passed")); + assert!(msg.contains("moved to done")); + } + + #[test] + fn conflicts_resolved_message() { + let mut r = make_report(MergeResult::Success { + conflicts_resolved: true, + conflict_details: None, + gate_output: String::new(), + }); + r.story_archived = true; + r.worktree_cleaned_up = true; + let msg = format_merge_status_message(&r); + assert!(msg.contains("auto-resolved")); + } + + /// Regression test (story 1178): a git-level merge success whose CRDT + /// state transition to Done failed (`story_archived: false`) must never + /// produce a message claiming the story was "moved to done". + #[test] + fn success_without_archival_does_not_claim_moved_to_done() { let r = make_report(MergeResult::Success { conflicts_resolved: false, conflict_details: None, gate_output: String::new(), }); + assert!(!r.story_archived); let msg = format_merge_status_message(&r); - assert!(msg.contains("quality gates passed")); - assert!(msg.contains("done")); - } - - #[test] - fn conflicts_resolved_message() { - let r = make_report(MergeResult::Success { - conflicts_resolved: true, - conflict_details: None, - gate_output: String::new(), - }); - let msg = format_merge_status_message(&r); - assert!(msg.contains("auto-resolved")); + assert!( + !msg.contains("Story moved to done"), + "message must not claim the story moved to done when story_archived is false: {msg}" + ); + assert!(msg.contains("could not be moved to done")); } #[test]