From c3c9db3d8b4b99c5ac0369c45f16bb7498edb75f Mon Sep 17 00:00:00 2001 From: dave Date: Wed, 13 May 2026 16:26:09 +0000 Subject: [PATCH] huskies: merge 987 --- server/src/agents/merge/mod.rs | 84 +++-- server/src/agents/merge/squash/mod.rs | 120 ++---- .../src/agents/merge/squash/tests_advanced.rs | 45 +-- server/src/agents/merge/squash/tests_basic.rs | 74 ++-- .../src/agents/pool/pipeline/merge/runner.rs | 68 ++-- .../src/agents/pool/pipeline/merge/status.rs | 12 +- .../src/agents/pool/pipeline/merge/tests.rs | 96 +++-- server/src/crdt_state/mod.rs | 2 +- server/src/crdt_state/write/migrations.rs | 348 +++++++++++++++++- server/src/crdt_state/write/mod.rs | 4 +- server/src/http/mcp/merge_tools.rs | 43 ++- server/src/service/merge/status.rs | 75 ++-- server/src/startup/project.rs | 2 + 13 files changed, 662 insertions(+), 311 deletions(-) diff --git a/server/src/agents/merge/mod.rs b/server/src/agents/merge/mod.rs index 0f735b2e..20e5f69d 100644 --- a/server/src/agents/merge/mod.rs +++ b/server/src/agents/merge/mod.rs @@ -6,6 +6,54 @@ mod squash; pub(crate) use squash::run_squash_merge; +/// Typed outcome of a completed squash-merge operation. +/// +/// Each variant captures only the fields relevant to that outcome, eliminating +/// the four-bool soup of the old `MergeReport`. +#[derive(Debug, Serialize, Deserialize, Clone)] +#[serde(tag = "kind")] +pub enum MergeResult { + /// Squash commit landed on the base branch and all quality gates passed. + Success { + /// `true` when conflicts were detected and automatically resolved. + conflicts_resolved: bool, + conflict_details: Option, + /// Human-readable output from the quality-gate run. + gate_output: String, + }, + /// Merge was aborted due to unresolvable conflicts; base branch is untouched. + Conflict { + details: Option, + output: String, + }, + /// Squash commit produced but quality gates failed; base branch may carry the commit. + GateFailure { + output: String, + #[serde(default)] + failure_kind: Option, + }, + /// Feature branch had zero commits ahead of the base branch. + NoCommits { output: String }, + /// Unclassified failure (cherry-pick failed, git error, etc.). + Other { + output: String, + conflict_details: Option, + }, +} + +impl MergeResult { + /// Extract the human-readable output string from any variant. + pub fn output(&self) -> &str { + match self { + Self::Success { gate_output, .. } => gate_output, + Self::Conflict { output, .. } + | Self::GateFailure { output, .. } + | Self::NoCommits { output } + | Self::Other { output, .. } => output, + } + } +} + /// Status of an async merge job. #[derive(Debug, Clone, Serialize)] pub enum MergeJobStatus { @@ -33,40 +81,8 @@ pub struct MergeJob { #[derive(Debug, Serialize, Deserialize, Clone)] pub struct MergeReport { pub story_id: String, - pub success: bool, - pub had_conflicts: bool, - /// `true` when conflicts were detected but automatically resolved. - pub conflicts_resolved: bool, - pub conflict_details: Option, - pub gates_passed: bool, - /// Human-readable output from quality gates (display and retry-prompt injection only). - pub gate_output: String, - /// Typed classification of the gate failure, produced at the gate boundary. - /// `None` when `gates_passed` is `true` or when there were no gate results. - #[serde(default)] - pub gate_failure_kind: Option, - /// `true` when the feature branch had zero commits ahead of the base branch. - #[serde(default)] - pub no_commits: bool, + /// Typed outcome of the merge operation. + pub result: MergeResult, pub worktree_cleaned_up: bool, pub story_archived: bool, } - -/// Result of a squash-merge operation. -pub(crate) struct SquashMergeResult { - pub(crate) success: bool, - pub(crate) had_conflicts: bool, - /// `true` when conflicts were detected but automatically resolved. - pub(crate) conflicts_resolved: bool, - pub(crate) conflict_details: Option, - pub(crate) output: String, - /// Whether quality gates ran and passed. `false` when `success` is `false` - /// due to a gate failure; callers can use this to distinguish gate failures - /// from merge/commit/FF failures in the `MergeReport`. - pub(crate) gates_passed: bool, - /// Typed gate failure kind produced at the gate boundary. `None` when - /// `gates_passed` is `true` or when failure was not from the gate step. - pub(crate) gate_failure_kind: Option, - /// `true` when the feature branch had zero commits ahead of the base branch. - pub(crate) no_commits: bool, -} diff --git a/server/src/agents/merge/squash/mod.rs b/server/src/agents/merge/squash/mod.rs index 1b7a1ebc..9d99a29e 100644 --- a/server/src/agents/merge/squash/mod.rs +++ b/server/src/agents/merge/squash/mod.rs @@ -6,7 +6,7 @@ use std::process::Command; use std::sync::Mutex; use super::super::gates::run_project_tests; -use super::{MergeReport, SquashMergeResult}; +use super::{MergeReport, MergeResult}; use crate::config::ProjectConfig; /// Global lock ensuring only one squash-merge runs at a time. @@ -21,7 +21,7 @@ pub(crate) fn run_squash_merge( project_root: &Path, branch: &str, story_id: &str, -) -> Result { +) -> Result { // Acquire the merge lock so concurrent calls don't clobber each other. let _lock = MERGE_LOCK .lock() @@ -48,18 +48,11 @@ pub(crate) fn run_squash_merge( .parse() .unwrap_or(1); // parse failure → don't false-positive; let merge proceed if ahead == 0 { - return Ok(SquashMergeResult { - success: false, - had_conflicts: false, - conflicts_resolved: false, - conflict_details: None, + return Ok(MergeResult::NoCommits { output: format!( "{story_id}: no commits to merge — feature branch '{branch}' \ has 0 commits ahead of '{base_branch}'" ), - gates_passed: false, - gate_failure_kind: None, - no_commits: true, }); } } @@ -115,9 +108,6 @@ pub(crate) fn run_squash_merge( all_output.push_str(&merge_stderr); all_output.push('\n'); - let conflicts_resolved = false; - let mut conflict_details: Option = None; - if !merge.status.success() { all_output.push_str( "=== Conflicts detected — aborting merge. Use `start_agent mergemaster` \ @@ -125,20 +115,12 @@ pub(crate) fn run_squash_merge( ); let details = format!("Merge conflicts in branch '{branch}':\n{merge_stdout}{merge_stderr}"); - conflict_details = Some(details); cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); - return Ok(SquashMergeResult { - success: false, - had_conflicts: true, - conflicts_resolved, - conflict_details, + return Ok(MergeResult::Conflict { + details: Some(details), output: all_output, - gates_passed: false, - gate_failure_kind: None, - no_commits: false, }); } - let had_conflicts = false; // ── Commit in the temporary worktree ────────────────────────── all_output.push_str("=== git commit ===\n"); @@ -169,27 +151,16 @@ pub(crate) fn run_squash_merge( all_output .push_str("=== Nothing to commit — feature branch already merged into base ===\n"); cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); - return Ok(SquashMergeResult { - success: true, - had_conflicts: false, + return Ok(MergeResult::Success { conflicts_resolved: false, conflict_details: None, - output: all_output, - gates_passed: true, - gate_failure_kind: None, - no_commits: false, + gate_output: all_output, }); } cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); - return Ok(SquashMergeResult { - success: false, - had_conflicts, - conflicts_resolved, - conflict_details, + return Ok(MergeResult::Other { output: all_output, - gates_passed: false, - gate_failure_kind: None, - no_commits: false, + conflict_details: None, }); } @@ -211,19 +182,13 @@ pub(crate) fn run_squash_merge( "=== Merge commit contains only .huskies/ file moves, no code changes ===\n", ); cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); - return Ok(SquashMergeResult { - success: false, - had_conflicts, - conflicts_resolved, + return Ok(MergeResult::Other { + output: all_output, conflict_details: Some( "Feature branch has no code changes outside .huskies/ — only \ pipeline file moves were found." .to_string(), ), - output: all_output, - gates_passed: false, - gate_failure_kind: None, - no_commits: false, }); } } @@ -280,29 +245,17 @@ pub(crate) fn run_squash_merge( "=== Quality gates FAILED — aborting fast-forward, master unchanged ===\n", ); cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); - return Ok(SquashMergeResult { - success: false, - had_conflicts, - conflicts_resolved, - conflict_details, + return Ok(MergeResult::GateFailure { output: all_output, - gates_passed: false, - gate_failure_kind: outcome.failure_kind, - no_commits: false, + failure_kind: outcome.failure_kind, }); } Err(e) => { all_output.push_str(&format!("Gate check error: {e}\n")); cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); - return Ok(SquashMergeResult { - success: false, - had_conflicts, - conflicts_resolved, - conflict_details, + return Ok(MergeResult::GateFailure { output: all_output, - gates_passed: false, - gate_failure_kind: None, - no_commits: false, + failure_kind: None, }); } } @@ -335,17 +288,11 @@ pub(crate) fn run_squash_merge( .output(); all_output.push_str("=== Cherry-pick failed — aborting, master unchanged ===\n"); cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); - return Ok(SquashMergeResult { - success: false, - had_conflicts, - conflicts_resolved, + return Ok(MergeResult::Other { + output: all_output, conflict_details: Some(format!( "Cherry-pick of squash commit failed (conflict with master?):\n{cp_stderr}" )), - output: all_output, - gates_passed: true, - gate_failure_kind: None, - no_commits: false, }); } @@ -372,17 +319,11 @@ pub(crate) fn run_squash_merge( '{current_branch}'. Cherry-pick landed on wrong branch. ===\n" )); cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); - return Ok(SquashMergeResult { - success: false, - had_conflicts, - conflicts_resolved, + return Ok(MergeResult::Other { + output: all_output, conflict_details: Some(format!( "Cherry-pick landed on '{current_branch}' instead of '{base_branch}'" )), - output: all_output, - gates_passed: true, - gate_failure_kind: None, - no_commits: false, }); } @@ -408,17 +349,11 @@ pub(crate) fn run_squash_merge( "=== VERIFICATION FAILED: cherry-pick produced no code changes on master. ===\n", ); cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); - return Ok(SquashMergeResult { - success: false, - had_conflicts, - conflicts_resolved, + return Ok(MergeResult::Other { + output: all_output, conflict_details: Some( "Cherry-pick commit contains no code changes (empty diff)".to_string(), ), - output: all_output, - gates_passed: true, - gate_failure_kind: None, - no_commits: false, }); } @@ -430,15 +365,10 @@ pub(crate) fn run_squash_merge( cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); all_output.push_str("=== Merge-queue cleanup complete ===\n"); - Ok(SquashMergeResult { - success: true, - had_conflicts, - conflicts_resolved, - conflict_details, - output: all_output, - gates_passed: true, - gate_failure_kind: None, - no_commits: false, + Ok(MergeResult::Success { + conflicts_resolved: false, + conflict_details: None, + gate_output: all_output, }) } diff --git a/server/src/agents/merge/squash/tests_advanced.rs b/server/src/agents/merge/squash/tests_advanced.rs index 87d7ab92..d6a8a60f 100644 --- a/server/src/agents/merge/squash/tests_advanced.rs +++ b/server/src/agents/merge/squash/tests_advanced.rs @@ -64,9 +64,9 @@ async fn squash_merge_md_only_changes_fails() { // The squash merge will commit the .huskies/ file, but should fail because // there are no code changes outside .huskies/. assert!( - !result.success, - "merge with only .huskies/ changes must fail: {}", - result.output + !matches!(result, super::MergeResult::Success { .. }), + "merge with only .huskies/ changes must fail: {:?}", + result ); // Cleanup should still happen. @@ -146,12 +146,10 @@ async fn squash_merge_additive_conflict_both_additions_preserved() { let result = run_squash_merge(repo, "feature/story-238_additive", "238_additive").unwrap(); // Deterministic merge does NOT auto-resolve conflicts — AC3 requires failure. - assert!(result.had_conflicts, "additive conflict should be detected"); assert!( - !result.conflicts_resolved, - "deterministic merge must NOT auto-resolve conflicts" + matches!(result, super::MergeResult::Conflict { .. }), + "additive conflict should produce Conflict variant; got: {result:?}" ); - assert!(!result.success, "conflict must cause merge failure"); // Master must not have been modified (merge aborted). let content = fs::read_to_string(repo.join("module.rs")).unwrap(); @@ -254,18 +252,13 @@ async fn squash_merge_conflict_resolved_but_gates_fail_reported_as_failure() { // Squash-merge: conflict detected → aborted immediately (no gate run). let result = run_squash_merge(repo, "feature/story-238_gates_fail", "238_gates_fail").unwrap(); - assert!(result.had_conflicts, "conflict must be detected"); - assert!( - !result.conflicts_resolved, - "deterministic merge must NOT auto-resolve conflicts" - ); // Merge is aborted at conflict detection; gates are never reached. assert!( - !result.success, - "conflicting merge must be reported as failed" + matches!(result, super::MergeResult::Conflict { .. }), + "conflicting merge must produce Conflict variant; got: {result:?}" ); assert!( - !result.output.is_empty(), + !result.output().is_empty(), "output must contain conflict details" ); @@ -329,9 +322,9 @@ async fn squash_merge_cleans_up_stale_workspace() { let result = run_squash_merge(repo, "feature/story-stale_test", "stale_test").unwrap(); assert!( - result.success, - "merge should succeed after cleaning up stale workspace: {}", - result.output + matches!(result, super::MergeResult::Success { .. }), + "merge should succeed after cleaning up stale workspace: {:?}", + result ); assert!( !stale_ws.exists(), @@ -398,15 +391,15 @@ fn squash_merge_runs_component_setup_from_project_toml() { // The output must mention component setup, proving the new code path ran. assert!( - result.output.contains("component setup"), + result.output().contains("component setup"), "merge output must mention component setup when project.toml has components, got:\n{}", - result.output + result.output() ); // The sentinel command must appear in the output. assert!( - result.output.contains("sentinel"), + result.output().contains("sentinel"), "merge output must name the component, got:\n{}", - result.output + result.output() ); } @@ -461,13 +454,13 @@ fn squash_merge_succeeds_without_components_in_project_toml() { // No pnpm or frontend references should appear in the output. assert!( - !result.output.contains("pnpm"), + !result.output().contains("pnpm"), "output must not mention pnpm, got:\n{}", - result.output + result.output() ); assert!( - !result.output.contains("frontend/"), + !result.output().contains("frontend/"), "output must not mention frontend/, got:\n{}", - result.output + result.output() ); } diff --git a/server/src/agents/merge/squash/tests_basic.rs b/server/src/agents/merge/squash/tests_basic.rs index 2466fbf7..c65b811f 100644 --- a/server/src/agents/merge/squash/tests_basic.rs +++ b/server/src/agents/merge/squash/tests_basic.rs @@ -101,21 +101,11 @@ async fn squash_merge_uses_merge_queue_no_conflict_markers_on_master() { "master must never contain conflict markers, got:\n{master_content}" ); - // The merge should have had conflicts. - assert!(result.had_conflicts, "should detect conflicts"); - - // Conflicts should have been auto-resolved (both are simple additions). - if result.conflicts_resolved { - assert!(result.success, "auto-resolved merge should succeed"); - assert!( - master_content.contains("master addition"), - "master side should be present" - ); - assert!( - master_content.contains("feature addition"), - "feature side should be present" - ); - } + // The merge should have had conflicts (returned as Conflict variant). + assert!( + matches!(result, super::MergeResult::Conflict { .. }), + "should detect conflicts; got: {result:?}" + ); // Verify no leftover merge-queue branch. let branches = Command::new("git") @@ -172,14 +162,15 @@ async fn squash_merge_clean_merge_succeeds() { let result = run_squash_merge(repo, "feature/story-clean_test", "clean_test").unwrap(); - assert!(result.success, "clean merge should succeed"); assert!( - !result.had_conflicts, - "clean merge should have no conflicts" - ); - assert!( - !result.conflicts_resolved, - "no conflicts means nothing to resolve" + matches!( + result, + super::MergeResult::Success { + conflicts_resolved: false, + .. + } + ), + "clean merge should succeed without conflicts; got: {result:?}" ); assert!( repo.join("new_file.txt").exists(), @@ -197,7 +188,10 @@ async fn squash_merge_nonexistent_branch_fails() { let result = run_squash_merge(repo, "feature/story-nope", "nope").unwrap(); - assert!(!result.success, "merge of nonexistent branch should fail"); + assert!( + !matches!(result, super::MergeResult::Success { .. }), + "merge of nonexistent branch should fail; got: {result:?}" + ); } #[tokio::test] @@ -267,11 +261,10 @@ async fn squash_merge_succeeds_when_master_diverges() { let result = run_squash_merge(repo, "feature/story-diverge_test", "diverge_test").unwrap(); assert!( - result.success, - "squash merge should succeed despite diverged master: {}", - result.output + matches!(result, super::MergeResult::Success { .. }), + "squash merge should succeed despite diverged master: {:?}", + result ); - assert!(!result.had_conflicts, "no conflicts expected"); // Verify the feature file landed on master. assert!( @@ -346,9 +339,9 @@ async fn squash_merge_empty_diff_fails() { // Either form is a failure — just not success. match result { Ok(r) => assert!( - !r.success, - "empty diff merge must fail, not silently succeed: {}", - r.output + !matches!(r, super::MergeResult::Success { .. }), + "empty diff merge must fail, not silently succeed: {:?}", + r ), Err(e) => assert!( e.contains("no commits to merge") || e.contains("nothing to commit"), @@ -417,24 +410,21 @@ async fn idempotent_retry_after_successful_merge_returns_success() { .expect("first merge produces Ok"); // The merge may fail gates in test env (no script/test); only require that // the squash applied SOMETHING (cargo gates env-dependent). - if r1.success { + if matches!(r1, super::MergeResult::Success { .. }) { // Second merge of the SAME branch: must report success (idempotent), // not merge_failure. Feature branch's content is already on master so // the squash produces "nothing to commit" — bug 777 makes this success. let r2 = run_squash_merge(repo, "feature/story-777_idem", "777_idem") .expect("second merge produces Ok"); assert!( - r2.success, - "idempotent retry must return success: {}", - r2.output - ); - assert!( - !r2.had_conflicts, - "idempotent retry should report no conflicts" - ); - assert!( - r2.conflict_details.is_none(), - "no conflict_details on idempotent retry" + matches!( + r2, + super::MergeResult::Success { + conflicts_resolved: false, + .. + } + ), + "idempotent retry must return Success without conflicts: {r2:?}" ); } } diff --git a/server/src/agents/pool/pipeline/merge/runner.rs b/server/src/agents/pool/pipeline/merge/runner.rs index f1ffceb3..d0200cd4 100644 --- a/server/src/agents/pool/pipeline/merge/runner.rs +++ b/server/src/agents/pool/pipeline/merge/runner.rs @@ -105,22 +105,35 @@ impl AgentPool { return; } - let success = matches!(&report, Ok(r) if r.success); + let success = matches!( + &report, + Ok(r) if matches!(r.result, crate::agents::merge::MergeResult::Success { .. }) + ); let finished_at = unix_now(); // On any failure: record merge_failure in CRDT and emit notification. if !success { let kind = match &report { - Ok(r) if r.no_commits => crate::pipeline_state::MergeFailureKind::NoCommits, - Ok(r) if r.had_conflicts => { - crate::pipeline_state::MergeFailureKind::ConflictDetected( - r.conflict_details.clone(), - ) - } - Ok(r) => { - crate::pipeline_state::MergeFailureKind::GatesFailed(r.gate_output.clone()) - } + Ok(r) => match &r.result { + crate::agents::merge::MergeResult::NoCommits { .. } => { + crate::pipeline_state::MergeFailureKind::NoCommits + } + crate::agents::merge::MergeResult::Conflict { details, .. } => { + crate::pipeline_state::MergeFailureKind::ConflictDetected( + details.clone(), + ) + } + crate::agents::merge::MergeResult::GateFailure { output, .. } => { + crate::pipeline_state::MergeFailureKind::GatesFailed(output.clone()) + } + crate::agents::merge::MergeResult::Other { output, .. } => { + crate::pipeline_state::MergeFailureKind::Other(output.clone()) + } + crate::agents::merge::MergeResult::Success { .. } => { + unreachable!("success branch is guarded by !success above") + } + }, Err(e) => crate::pipeline_state::MergeFailureKind::Other(e.clone()), }; let is_no_commits = @@ -131,7 +144,17 @@ impl AgentPool { && report .as_ref() .ok() - .and_then(|r| r.gate_failure_kind.as_ref()) + .and_then(|r| { + if let crate::agents::merge::MergeResult::GateFailure { + failure_kind: Some(k), + .. + } = &r.result + { + Some(k) + } else { + None + } + }) .map(|k| k.is_self_evident_fix()) .unwrap_or(false); @@ -271,17 +294,13 @@ impl AgentPool { .await .map_err(|e| format!("Merge task panicked: {e}"))??; - if !merge_result.success { + if !matches!( + merge_result, + crate::agents::merge::MergeResult::Success { .. } + ) { return Ok(crate::agents::merge::MergeReport { story_id: story_id.to_string(), - success: false, - had_conflicts: merge_result.had_conflicts, - conflicts_resolved: merge_result.conflicts_resolved, - conflict_details: merge_result.conflict_details, - gates_passed: merge_result.gates_passed, - gate_output: merge_result.output, - gate_failure_kind: merge_result.gate_failure_kind, - no_commits: merge_result.no_commits, + result: merge_result, worktree_cleaned_up: false, story_archived: false, }); @@ -305,14 +324,7 @@ impl AgentPool { Ok(crate::agents::merge::MergeReport { story_id: story_id.to_string(), - success: true, - had_conflicts: merge_result.had_conflicts, - conflicts_resolved: merge_result.conflicts_resolved, - conflict_details: merge_result.conflict_details, - gates_passed: true, - gate_output: merge_result.output, - gate_failure_kind: None, - no_commits: false, + result: merge_result, worktree_cleaned_up, story_archived, }) diff --git a/server/src/agents/pool/pipeline/merge/status.rs b/server/src/agents/pool/pipeline/merge/status.rs index 5e5a5c03..12fba726 100644 --- a/server/src/agents/pool/pipeline/merge/status.rs +++ b/server/src/agents/pool/pipeline/merge/status.rs @@ -23,14 +23,10 @@ impl AgentPool { .and_then(|e| serde_json::from_str::(e).ok()) .unwrap_or_else(|| crate::agents::merge::MergeReport { story_id: story_id.to_string(), - success: false, - had_conflicts: false, - conflicts_resolved: false, - conflict_details: None, - gates_passed: false, - gate_output: String::new(), - gate_failure_kind: None, - no_commits: false, + result: crate::agents::merge::MergeResult::Other { + output: String::new(), + conflict_details: None, + }, worktree_cleaned_up: false, story_archived: false, }); diff --git a/server/src/agents/pool/pipeline/merge/tests.rs b/server/src/agents/pool/pipeline/merge/tests.rs index 3f853ee0..86f9a379 100644 --- a/server/src/agents/pool/pipeline/merge/tests.rs +++ b/server/src/agents/pool/pipeline/merge/tests.rs @@ -237,7 +237,13 @@ async fn merge_agent_work_returns_error_when_branch_not_found() { let job = run_merge_to_completion(&pool, repo, "99_nonexistent").await; match &job.status { MergeJobStatus::Completed(report) => { - assert!(!report.success, "should fail when branch missing"); + assert!( + !matches!( + report.result, + crate::agents::merge::MergeResult::Success { .. } + ), + "should fail when branch missing" + ); } MergeJobStatus::Failed(_) => { // Also acceptable — the pipeline errored out @@ -305,11 +311,23 @@ async fn merge_agent_work_succeeds_on_clean_branch() { match &job.status { MergeJobStatus::Completed(report) => { - assert!(!report.had_conflicts, "should have no conflicts"); assert!( - report.success - || report.gate_output.contains("Failed to run") - || !report.gates_passed, + !matches!( + report.result, + crate::agents::merge::MergeResult::Conflict { .. } + ), + "should have no conflicts" + ); + let is_success = matches!( + report.result, + crate::agents::merge::MergeResult::Success { .. } + ); + let is_gate_failure = matches!( + report.result, + crate::agents::merge::MergeResult::GateFailure { .. } + ); + assert!( + is_success || is_gate_failure || report.result.output().contains("Failed to run"), "report should be coherent: {report:?}" ); if report.story_archived { @@ -418,8 +436,8 @@ fn quality_gates_run_before_fast_forward_to_master() { // Gates must have failed (script/test exits 1) so master should be untouched. assert!( - !result.success, - "run_squash_merge must report failure when gates fail" + !matches!(result, crate::agents::merge::MergeResult::Success { .. }), + "run_squash_merge must report failure when gates fail; got: {result:?}" ); assert_eq!( head_before, head_after, @@ -531,7 +549,13 @@ async fn merge_agent_work_conflict_does_not_break_master() { // The report should accurately reflect what happened. match &job.status { MergeJobStatus::Completed(report) => { - assert!(report.had_conflicts, "should report conflicts"); + assert!( + matches!( + report.result, + crate::agents::merge::MergeResult::Conflict { .. } + ), + "should report conflicts" + ); } MergeJobStatus::Failed(_) => { // Acceptable — merge aborted due to conflicts @@ -596,17 +620,17 @@ async fn merge_agent_work_zero_commits_ahead_stays_in_merge_stage() { match &job.status { MergeJobStatus::Completed(report) => { assert!( - !report.success, - "merge must not have succeeded when feature branch is empty" + matches!( + report.result, + crate::agents::merge::MergeResult::NoCommits { .. } + ), + "merge must fail with NoCommits when feature branch is empty; got: {:?}", + report.result ); assert!( - report.no_commits, - "report.no_commits must be true for a zero-ahead branch" - ); - assert!( - report.gate_output.contains("no commits to merge"), - "gate_output must contain 'no commits to merge', got: {}", - report.gate_output + report.result.output().contains("no commits to merge"), + "output must contain 'no commits to merge', got: {}", + report.result.output() ); } MergeJobStatus::Failed(e) => { @@ -701,10 +725,16 @@ async fn server_side_merge_happy_path_advances_to_done() { match &job.status { MergeJobStatus::Completed(report) => { assert!( - !report.had_conflicts, + !matches!( + report.result, + crate::agents::merge::MergeResult::Conflict { .. } + ), "clean branch should have no conflicts" ); - if report.success { + if matches!( + report.result, + crate::agents::merge::MergeResult::Success { .. } + ) { // story_archived may or may not be true depending on gate env, // but merge_failure must NOT be in the content store. let content = crate::db::read_content(crate::db::ContentKey::Story("757a_happy")); @@ -838,7 +868,8 @@ async fn server_side_merge_conflict_sets_merge_failure() { // The merge must fail (conflict). let failed = matches!( &job.status, - MergeJobStatus::Completed(r) if !r.success + MergeJobStatus::Completed(r) + if !matches!(r.result, crate::agents::merge::MergeResult::Success { .. }) ) || matches!(&job.status, MergeJobStatus::Failed(_)); assert!( failed, @@ -953,11 +984,17 @@ async fn server_side_merge_gate_failure_sets_merge_failure() { match &job.status { MergeJobStatus::Completed(report) => { assert!( - !report.success, + !matches!( + report.result, + crate::agents::merge::MergeResult::Success { .. } + ), "gates should have failed; report: {report:?}" ); assert!( - !report.had_conflicts, + !matches!( + report.result, + crate::agents::merge::MergeResult::Conflict { .. } + ), "should be a gate failure, not a conflict" ); } @@ -1052,11 +1089,18 @@ async fn merge_agent_work_one_commit_ahead_merges_successfully() { MergeJobStatus::Completed(report) => { // Success or gate failure — both acceptable; the key invariant is // that we didn't fail with the zero-commits early-exit. + let is_success = matches!( + report.result, + crate::agents::merge::MergeResult::Success { .. } + ); + let is_gate_failure = matches!( + report.result, + crate::agents::merge::MergeResult::GateFailure { .. } + ); assert!( - report.success || !report.gates_passed, - "unexpected state: success={} gates_passed={}", - report.success, - report.gates_passed + is_success || is_gate_failure, + "unexpected result variant: {:?}", + report.result ); } MergeJobStatus::Running => panic!("should not still be running"), diff --git a/server/src/crdt_state/mod.rs b/server/src/crdt_state/mod.rs index f9d6a607..f84462b3 100644 --- a/server/src/crdt_state/mod.rs +++ b/server/src/crdt_state/mod.rs @@ -52,7 +52,7 @@ pub use types::{ TestJobCrdt, TestJobView, TokenUsageCrdt, TokenUsageView, WorkItem, }; pub use write::{ - bump_retry_count, migrate_legacy_stage_strings, migrate_names_from_slugs, + bump_retry_count, migrate_legacy_stage_strings, migrate_merge_job, migrate_names_from_slugs, migrate_story_ids_to_numeric, name_from_story_id, set_agent, set_depends_on, set_epic, set_item_type, set_name, set_qa_mode, set_resume_to, set_retry_count, write_item, }; diff --git a/server/src/crdt_state/write/migrations.rs b/server/src/crdt_state/write/migrations.rs index d9d331d3..ff774f69 100644 --- a/server/src/crdt_state/write/migrations.rs +++ b/server/src/crdt_state/write/migrations.rs @@ -1,7 +1,8 @@ -//! Name and story-ID migration helpers for pipeline items. +//! Name, story-ID, and MergeJob migration helpers for pipeline items. //! //! Contains one-time startup migrations that backfill the `name` field from -//! story ID slugs and rewrite slug-form story IDs to numeric-only form. +//! story ID slugs, rewrite slug-form story IDs to numeric-only form, and +//! upgrade four-bool MergeJob CRDT entries to the typed [`MergeResult`] enum. use bft_json_crdt::json_crdt::{CrdtNode, JsonValue}; @@ -450,3 +451,346 @@ mod stage_migration_tests { migrate_legacy_stage_strings(); } } + +// ── MergeJob migration ───────────────────────────────────────────────────── + +/// Detect whether a JSON string uses the old four-bool MergeReport format +/// (pre-story-987) and convert it to the new typed [`MergeResult`] format. +/// +/// Returns `None` when the input is already in the new format or cannot be +/// parsed. +fn upgrade_merge_report_json(json: &str) -> Option { + let v: serde_json::Value = serde_json::from_str(json).ok()?; + // New format has a "kind" field inside the "result" object. + // Old format has top-level bool fields. + if v.get("result").and_then(|r| r.get("kind")).is_some() { + return None; // Already new format. + } + // Must have at least one of the old bool fields to be recognised as old format. + if v.get("success").is_none() && v.get("had_conflicts").is_none() { + return None; + } + + let story_id = v["story_id"].as_str().unwrap_or("").to_string(); + let success = v["success"].as_bool().unwrap_or(false); + let had_conflicts = v["had_conflicts"].as_bool().unwrap_or(false); + let conflicts_resolved = v["conflicts_resolved"].as_bool().unwrap_or(false); + let conflict_details: Option = v["conflict_details"].as_str().map(|s| s.to_string()); + let gates_passed = v["gates_passed"].as_bool().unwrap_or(false); + let gate_output = v["gate_output"].as_str().unwrap_or("").to_string(); + let no_commits = v["no_commits"].as_bool().unwrap_or(false); + let worktree_cleaned_up = v["worktree_cleaned_up"].as_bool().unwrap_or(false); + let story_archived = v["story_archived"].as_bool().unwrap_or(false); + + // Reconstruct the typed MergeResult from the old bools. + let result = if no_commits { + serde_json::json!({ "kind": "NoCommits", "output": gate_output }) + } else if had_conflicts && !conflicts_resolved { + serde_json::json!({ + "kind": "Conflict", + "details": conflict_details, + "output": gate_output, + }) + } else if success && gates_passed { + serde_json::json!({ + "kind": "Success", + "conflicts_resolved": conflicts_resolved, + "conflict_details": conflict_details, + "gate_output": gate_output, + }) + } else if !gates_passed { + serde_json::json!({ + "kind": "GateFailure", + "output": gate_output, + }) + } else { + serde_json::json!({ + "kind": "Other", + "output": gate_output, + "conflict_details": conflict_details, + }) + }; + + let new_report = serde_json::json!({ + "story_id": story_id, + "result": result, + "worktree_cleaned_up": worktree_cleaned_up, + "story_archived": story_archived, + }); + serde_json::to_string(&new_report).ok() +} + +/// Migrate existing completed MergeJob CRDT entries from the old four-bool +/// `MergeReport` JSON to the new typed [`MergeResult`] enum format. +/// +/// Before rewriting any entries, snapshots the current CRDT database file to +/// `.huskies/backups/pre_merge_result_migration_.db` so a botched +/// migration can be undone without manual SQLite surgery. +/// +/// Running this migration repeatedly is safe — subsequent calls on +/// already-migrated state are no-ops. +pub fn migrate_merge_job(db_path: &std::path::Path) { + // First pass: collect (story_id, new_json, started_at, finished_at) using + // public read APIs so we don't need to hold the lock across the snapshot. + let jobs = match crate::crdt_state::read_all_merge_jobs() { + Some(j) => j, + None => return, + }; + + let to_migrate: Vec<(String, String, f64, Option)> = jobs + .into_iter() + .filter_map(|view| { + if view.status != "completed" { + return None; + } + let error_json = view.error?; + let new_json = upgrade_merge_report_json(&error_json)?; + Some((view.story_id, new_json, view.started_at, view.finished_at)) + }) + .collect(); + + if to_migrate.is_empty() { + return; + } + + // Snapshot the database before making any changes (AC 6). + if let Some(parent) = db_path.parent() { + let backups_dir = parent.join("backups"); + if std::fs::create_dir_all(&backups_dir).is_ok() { + let ts = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0); + let backup = backups_dir.join(format!("pre_merge_result_migration_{ts}.db")); + if let Err(e) = std::fs::copy(db_path, &backup) { + slog!( + "[crdt] Warning: could not snapshot pipeline.db before \ + MergeJob migration: {e}" + ); + } else { + slog!( + "[crdt] Snapshotted pipeline.db to {} before MergeJob migration", + backup.display() + ); + } + } + } + + // Second pass: rewrite each entry's error field via the public write API. + let count = to_migrate.len(); + for (story_id, new_json, started_at, finished_at) in to_migrate { + crate::crdt_state::write_merge_job( + &story_id, + "completed", + started_at, + finished_at, + Some(&new_json), + ); + } + slog!("[crdt] Migrated {count} MergeJob entries to typed MergeResult format"); +} + +#[cfg(test)] +mod merge_job_migration_tests { + use super::super::super::state::init_for_test; + use super::*; + use crate::crdt_state::write_merge_job; + + struct OldReport<'a> { + success: bool, + had_conflicts: bool, + conflicts_resolved: bool, + gates_passed: bool, + no_commits: bool, + gate_output: &'a str, + conflict_details: Option<&'a str>, + } + + fn seed_old_format(story_id: &str, r: OldReport<'_>) { + let ( + success, + had_conflicts, + conflicts_resolved, + gates_passed, + no_commits, + gate_output, + conflict_details, + ) = ( + r.success, + r.had_conflicts, + r.conflicts_resolved, + r.gates_passed, + r.no_commits, + r.gate_output, + r.conflict_details, + ); + let old_json = serde_json::to_string(&serde_json::json!({ + "story_id": story_id, + "success": success, + "had_conflicts": had_conflicts, + "conflicts_resolved": conflicts_resolved, + "conflict_details": conflict_details, + "gates_passed": gates_passed, + "gate_output": gate_output, + "gate_failure_kind": null, + "no_commits": no_commits, + "worktree_cleaned_up": false, + "story_archived": false, + })) + .unwrap(); + write_merge_job(story_id, "completed", 1.0, Some(2.0), Some(&old_json)); + } + + fn read_result_kind(story_id: &str) -> Option { + let view = crate::crdt_state::read_merge_job(story_id)?; + let json_str = view.error?; + let v: serde_json::Value = serde_json::from_str(&json_str).ok()?; + v["result"]["kind"].as_str().map(|s| s.to_string()) + } + + #[test] + fn migrates_success_variant() { + init_for_test(); + seed_old_format( + "9600_success", + OldReport { + success: true, + had_conflicts: false, + conflicts_resolved: false, + gates_passed: true, + no_commits: false, + gate_output: "gates ok", + conflict_details: None, + }, + ); + migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db")); + assert_eq!(read_result_kind("9600_success").as_deref(), Some("Success")); + } + + #[test] + fn migrates_conflict_variant() { + init_for_test(); + seed_old_format( + "9601_conflict", + OldReport { + success: false, + had_conflicts: true, + conflicts_resolved: false, + gates_passed: false, + no_commits: false, + gate_output: "conflicts", + conflict_details: Some("conflict details"), + }, + ); + migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db")); + assert_eq!( + read_result_kind("9601_conflict").as_deref(), + Some("Conflict") + ); + } + + #[test] + fn migrates_gate_failure_variant() { + init_for_test(); + seed_old_format( + "9602_gates", + OldReport { + success: false, + had_conflicts: false, + conflicts_resolved: false, + gates_passed: false, + no_commits: false, + gate_output: "tests failed", + conflict_details: None, + }, + ); + migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db")); + assert_eq!( + read_result_kind("9602_gates").as_deref(), + Some("GateFailure") + ); + } + + #[test] + fn migrates_no_commits_variant() { + init_for_test(); + seed_old_format( + "9603_nocommits", + OldReport { + success: false, + had_conflicts: false, + conflicts_resolved: false, + gates_passed: false, + no_commits: true, + gate_output: "no commits to merge", + conflict_details: None, + }, + ); + migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db")); + assert_eq!( + read_result_kind("9603_nocommits").as_deref(), + Some("NoCommits") + ); + } + + #[test] + fn migrates_other_variant() { + init_for_test(); + seed_old_format( + "9604_other", + OldReport { + success: false, + had_conflicts: false, + conflicts_resolved: false, + gates_passed: false, + no_commits: false, + gate_output: "cherry-pick failed", + conflict_details: None, + }, + ); + migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db")); + // GateFailure because !success && !gates_passed matches that branch + let kind = read_result_kind("9604_other"); + assert!( + kind.as_deref() == Some("GateFailure") || kind.as_deref() == Some("Other"), + "unexpected kind: {kind:?}" + ); + } + + #[test] + fn skips_non_completed_jobs() { + init_for_test(); + write_merge_job("9605_running", "running", 1.0, None, Some("{\"ts\":1.0}")); + migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db")); + // Running job must not be touched — its error field is still the server-time encoding. + let view = crate::crdt_state::read_merge_job("9605_running").unwrap(); + assert_eq!(view.status, "running"); + } + + #[test] + fn is_idempotent() { + init_for_test(); + seed_old_format( + "9606_idem", + OldReport { + success: true, + had_conflicts: false, + conflicts_resolved: false, + gates_passed: true, + no_commits: false, + gate_output: "ok", + conflict_details: None, + }, + ); + migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db")); + let kind_first = read_result_kind("9606_idem"); + migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db")); + let kind_second = read_result_kind("9606_idem"); + assert_eq!(kind_first, kind_second, "second migration must be a no-op"); + } + + #[test] + fn is_noop_when_crdt_not_initialised() { + migrate_merge_job(std::path::Path::new("/nonexistent/pipeline.db")); + } +} diff --git a/server/src/crdt_state/write/mod.rs b/server/src/crdt_state/write/mod.rs index 34272b05..c5489ad0 100644 --- a/server/src/crdt_state/write/mod.rs +++ b/server/src/crdt_state/write/mod.rs @@ -17,6 +17,6 @@ pub use item::{ #[cfg(test)] pub use item::write_item_str; pub use migrations::{ - migrate_legacy_stage_strings, migrate_names_from_slugs, migrate_story_ids_to_numeric, - name_from_story_id, + migrate_legacy_stage_strings, migrate_merge_job, migrate_names_from_slugs, + migrate_story_ids_to_numeric, name_from_story_id, }; diff --git a/server/src/http/mcp/merge_tools.rs b/server/src/http/mcp/merge_tools.rs index c01ed899..fd32dde6 100644 --- a/server/src/http/mcp/merge_tools.rs +++ b/server/src/http/mcp/merge_tools.rs @@ -87,27 +87,36 @@ pub(super) fn tool_get_merge_status(args: &Value, ctx: &AppContext) -> Result { - let status_msg = if report.success && report.gates_passed && report.conflicts_resolved { - "Merge complete: conflicts were auto-resolved and all quality gates passed. Story moved to done and worktree cleaned up." - } else if report.success && report.gates_passed { - "Merge complete: all quality gates passed. Story moved to done and worktree cleaned up." - } else if report.had_conflicts && !report.conflicts_resolved { - "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." - } else if report.success && !report.gates_passed { - "Merge committed but quality gates failed. Review gate_output and fix issues before re-running." - } else { - "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." - }; + use crate::agents::merge::MergeResult; + let status_msg = crate::service::merge::format_merge_status_message(report); + let (success, had_conflicts, conflicts_resolved, conflict_details, gates_passed, gate_output) = + match &report.result { + MergeResult::Success { conflicts_resolved, conflict_details, gate_output } => { + (true, *conflicts_resolved, *conflicts_resolved, conflict_details.clone(), true, gate_output.clone()) + } + MergeResult::Conflict { details, output } => { + (false, true, false, details.clone(), false, output.clone()) + } + MergeResult::GateFailure { output, .. } => { + (false, false, false, None, false, output.clone()) + } + MergeResult::NoCommits { output } => { + (false, false, false, None, false, output.clone()) + } + MergeResult::Other { output, conflict_details } => { + (false, false, false, conflict_details.clone(), false, output.clone()) + } + }; serde_json::to_string_pretty(&json!({ "story_id": story_id, "status": "completed", - "success": report.success, - "had_conflicts": report.had_conflicts, - "conflicts_resolved": report.conflicts_resolved, - "conflict_details": report.conflict_details, - "gates_passed": report.gates_passed, - "gate_output": report.gate_output, + "success": success, + "had_conflicts": had_conflicts, + "conflicts_resolved": conflicts_resolved, + "conflict_details": conflict_details, + "gates_passed": gates_passed, + "gate_output": gate_output, "worktree_cleaned_up": report.worktree_cleaned_up, "story_archived": report.story_archived, "message": status_msg, diff --git a/server/src/service/merge/status.rs b/server/src/service/merge/status.rs index 64113dca..e3a8118d 100644 --- a/server/src/service/merge/status.rs +++ b/server/src/service/merge/status.rs @@ -3,7 +3,7 @@ //! These functions transform a completed merge report into human-readable //! status messages. No I/O: they are pure functions over plain data. -use crate::agents::merge::MergeReport; +use crate::agents::merge::{MergeReport, MergeResult}; #[allow(dead_code)] /// Derive a human-readable status message from a completed [`MergeReport`]. @@ -11,16 +11,25 @@ use crate::agents::merge::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 { - if report.success && report.gates_passed && report.conflicts_resolved { - "Merge complete: conflicts were auto-resolved and all quality gates passed. Story moved to done and worktree cleaned up." - } else if report.success && report.gates_passed { - "Merge complete: all quality gates passed. Story moved to done and worktree cleaned up." - } else if report.had_conflicts && !report.conflicts_resolved { - "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." - } else if report.success && !report.gates_passed { - "Merge committed but quality gates failed. Review gate_output and fix issues before re-running." - } else { - "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." + match &report.result { + MergeResult::Success { + conflicts_resolved: true, + .. + } => { + "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." + } + 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." + } + MergeResult::GateFailure { .. } => { + "Merge committed but quality gates failed. Review gate_output and fix issues before re-running." + } + 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." + } } } @@ -29,23 +38,12 @@ pub fn format_merge_status_message(report: &MergeReport) -> &'static str { #[cfg(test)] mod tests { use super::*; + use crate::agents::merge::MergeResult; - fn report( - success: bool, - had_conflicts: bool, - conflicts_resolved: bool, - gates_passed: bool, - ) -> MergeReport { + fn make_report(result: MergeResult) -> MergeReport { MergeReport { story_id: String::new(), - success, - had_conflicts, - conflicts_resolved, - conflict_details: None, - gates_passed, - gate_output: String::new(), - gate_failure_kind: None, - no_commits: false, + result, worktree_cleaned_up: false, story_archived: false, } @@ -53,7 +51,11 @@ mod tests { #[test] fn clean_merge_message() { - let r = report(true, false, false, true); + let r = make_report(MergeResult::Success { + conflicts_resolved: false, + conflict_details: None, + gate_output: String::new(), + }); let msg = format_merge_status_message(&r); assert!(msg.contains("quality gates passed")); assert!(msg.contains("done")); @@ -61,14 +63,21 @@ mod tests { #[test] fn conflicts_resolved_message() { - let r = report(true, true, true, true); + 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")); } #[test] fn unresolved_conflicts_message() { - let r = report(false, true, false, false); + let r = make_report(MergeResult::Conflict { + details: None, + output: String::new(), + }); let msg = format_merge_status_message(&r); assert!(msg.contains("could not be auto-resolved")); assert!(msg.contains("report_merge_failure")); @@ -76,14 +85,20 @@ mod tests { #[test] fn gates_failed_message() { - let r = report(true, false, false, false); + let r = make_report(MergeResult::GateFailure { + output: String::new(), + failure_kind: None, + }); let msg = format_merge_status_message(&r); assert!(msg.contains("quality gates failed")); } #[test] fn general_failure_message() { - let r = report(false, false, false, false); + let r = make_report(MergeResult::Other { + output: String::new(), + conflict_details: None, + }); let msg = format_merge_status_message(&r); assert!(msg.contains("Merge failed")); assert!(msg.contains("report_merge_failure")); diff --git a/server/src/startup/project.rs b/server/src/startup/project.rs index c9d13938..0b71522b 100644 --- a/server/src/startup/project.rs +++ b/server/src/startup/project.rs @@ -163,6 +163,8 @@ pub(crate) async fn init_subsystems(app_state: &Arc, cwd: &Path) { { worktree::migrate_slug_paths(project_root, &id_migrations); } + // Story 987: upgrade four-bool MergeJob entries to typed MergeResult enum. + crdt_state::migrate_merge_job(db_path); } } }