From 81065a3ada7ba4f36e1da0212e92579475f493d6 Mon Sep 17 00:00:00 2001 From: Dave Date: Thu, 26 Feb 2026 14:16:35 +0000 Subject: [PATCH] fix: replace fast-forward with cherry-pick in mergemaster squash-merge The mergemaster pipeline used git merge --ff-only to apply the squash commit from a merge-queue branch onto master. This raced with the filesystem watcher which auto-commits pipeline file moves to master, causing the fast-forward to fail. The mergemaster agent would then improvise by manually moving stories to done without the code merge. - Replace --ff-only with cherry-pick so concurrent watcher commits don't block the merge - Add report_merge_failure MCP tool for explicit failure handling - Update mergemaster prompt to forbid manual file moves - Fix cleanup_merge_workspace to handle stale directories Squash merge of feature/story-205 Co-Authored-By: Claude Opus 4.6 --- .story_kit/project.toml | 15 ++- server/src/agents.rs | 274 +++++++++++++++++++++++++++++++++++++--- server/src/http/mcp.rs | 90 ++++++++++++- 3 files changed, 355 insertions(+), 24 deletions(-) diff --git a/.story_kit/project.toml b/.story_kit/project.toml index 65bb7c7..3d2a3de 100644 --- a/.story_kit/project.toml +++ b/.story_kit/project.toml @@ -229,8 +229,9 @@ Read CLAUDE.md first, then .story_kit/README.md to understand the dev process. 2. Review the result: check success, had_conflicts, conflicts_resolved, gates_passed, and gate_output 3. If merge succeeded and gates passed: report success to the human 4. If conflicts were auto-resolved (conflicts_resolved=true) and gates passed: report success, noting which conflicts were resolved -5. If conflicts could not be auto-resolved: report the conflict details clearly so the human can resolve them. Master is untouched. -6. If gates failed after merge: attempt to fix minor issues (see below), then re-trigger quality gates. After 2 fix attempts, stop and report to the human. +5. If conflicts could not be auto-resolved: call report_merge_failure(story_id='{{story_id}}', reason='') and report to the human. Master is untouched. +6. If merge failed for any other reason: call report_merge_failure(story_id='{{story_id}}', reason='
') and report to the human. +7. If gates failed after merge: attempt to fix minor issues (see below), then re-trigger merge_agent_work. After 2 fix attempts, call report_merge_failure and stop. ## How Conflict Resolution Works The merge pipeline uses a temporary merge-queue branch and worktree to isolate merges from master. Simple additive conflicts (both branches adding code at the same location) are resolved automatically by keeping both additions. Complex conflicts (modifying the same lines differently) are reported without touching master. @@ -251,11 +252,15 @@ If quality gates fail (cargo clippy, cargo test, pnpm build, pnpm test), attempt - Non-trivial refactoring needed - Anything requiring understanding of broader system context -**Max retry limit:** If gates still fail after 2 fix attempts, stop immediately and report the full gate output to the human so they can resolve it. Do not retry further. +**Max retry limit:** If gates still fail after 2 fix attempts, call report_merge_failure to record the failure, then stop immediately and report the full gate output to the human. Do not retry further. -## Rules +## CRITICAL Rules +- NEVER manually move story files between pipeline stages (e.g. from 4_merge/ to 5_done/) +- NEVER call accept_story — only merge_agent_work can move stories to done after a successful merge +- When merge fails, ALWAYS call report_merge_failure to record the failure — do NOT improvise with file moves +- Only use MCP tools (merge_agent_work, report_merge_failure) to drive the merge process - Only attempt fixes that are clearly minor and low-risk - Report conflict resolution outcomes clearly - Report gate failures with full output so the human can act if needed - The server automatically runs acceptance gates when your process exits""" -system_prompt = "You are the mergemaster agent. Your primary responsibility is to trigger the merge_agent_work MCP tool and report the results. For minor gate failures (syntax errors, unused imports, missing semicolons), attempt to fix them yourself — but stop after 2 attempts and report to the human if gates still fail. For complex failures or unresolvable conflicts, report clearly so the human can act. The merge pipeline automatically resolves simple additive conflicts." +system_prompt = "You are the mergemaster agent. Your primary responsibility is to trigger the merge_agent_work MCP tool and report the results. CRITICAL: Never manually move story files or call accept_story. When merge fails, call report_merge_failure to record the failure. For minor gate failures (syntax errors, unused imports, missing semicolons), attempt to fix them yourself — but stop after 2 attempts, call report_merge_failure, and report to the human. For complex failures or unresolvable conflicts, call report_merge_failure and report clearly so the human can act. The merge pipeline automatically resolves simple additive conflicts." diff --git a/server/src/agents.rs b/server/src/agents.rs index 44eb371..29fae06 100644 --- a/server/src/agents.rs +++ b/server/src/agents.rs @@ -1209,7 +1209,7 @@ impl AgentPool { /// 1. Squash-merge the story's feature branch into the current branch (master). /// 2. If conflicts are found: abort the merge and report them. /// 3. Quality gates run **inside the merge worktree** before master is touched. - /// 4. If gates pass: fast-forward master and archive the story. + /// 4. If gates pass: cherry-pick the squash commit onto master and archive the story. /// /// Returns a `MergeReport` with full details of what happened. pub async fn merge_agent_work( @@ -2546,8 +2546,7 @@ struct SquashMergeResult { } /// Squash-merge a feature branch into the current branch using a temporary -/// merge-queue worktree. This avoids the race condition where the filesystem -/// watcher auto-commits conflict markers to master. +/// merge-queue worktree for quality-gate isolation. /// /// **Flow:** /// 1. Create a temporary `merge-queue/{story_id}` branch at current HEAD. @@ -2556,8 +2555,12 @@ struct SquashMergeResult { /// 4. If conflicts arise, attempt automatic resolution for simple additive cases. /// 5. If clean (or resolved), commit in the temp worktree. /// 6. Run quality gates **in the merge worktree** before touching master. -/// 7. If gates pass: fast-forward master to the merge-queue commit. +/// 7. If gates pass: cherry-pick the squash commit onto master. /// 8. Clean up the temporary worktree and branch. +/// +/// Step 7 uses `git cherry-pick` instead of `git merge --ff-only` so that +/// concurrent filesystem-watcher commits on master (pipeline file moves) do +/// not block the merge. fn run_squash_merge( project_root: &Path, branch: &str, @@ -2834,31 +2837,40 @@ fn run_squash_merge( } } - // ── Fast-forward master to the merge-queue commit ───────────── + // ── Cherry-pick the squash commit onto master ────────────────── + // We cherry-pick instead of fast-forward so that concurrent filesystem + // watcher commits on master (e.g. pipeline file moves) don't block the + // merge. Cherry-pick applies the diff of the squash commit cleanly on + // top of master's current HEAD. all_output.push_str(&format!( - "=== Fast-forwarding master to {merge_branch} ===\n" + "=== Cherry-picking squash commit from {merge_branch} onto master ===\n" )); - let ff = Command::new("git") - .args(["merge", "--ff-only", &merge_branch]) + let cp = Command::new("git") + .args(["cherry-pick", &merge_branch]) .current_dir(project_root) .output() - .map_err(|e| format!("Failed to fast-forward master: {e}"))?; + .map_err(|e| format!("Failed to cherry-pick merge-queue commit: {e}"))?; - let ff_stdout = String::from_utf8_lossy(&ff.stdout).to_string(); - let ff_stderr = String::from_utf8_lossy(&ff.stderr).to_string(); - all_output.push_str(&ff_stdout); - all_output.push_str(&ff_stderr); + let cp_stdout = String::from_utf8_lossy(&cp.stdout).to_string(); + let cp_stderr = String::from_utf8_lossy(&cp.stderr).to_string(); + all_output.push_str(&cp_stdout); + all_output.push_str(&cp_stderr); all_output.push('\n'); - if !ff.status.success() { - all_output.push_str("=== Fast-forward failed — master may have diverged ===\n"); + if !cp.status.success() { + // Abort the cherry-pick so master is left clean. + let _ = Command::new("git") + .args(["cherry-pick", "--abort"]) + .current_dir(project_root) + .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, conflict_details: Some(format!( - "Fast-forward to merge-queue failed (master diverged?):\n{ff_stderr}" + "Cherry-pick of squash commit failed (conflict with master?):\n{cp_stderr}" )), output: all_output, gates_passed: true, @@ -2891,6 +2903,12 @@ fn cleanup_merge_workspace( .args(["worktree", "remove", "--force", &wt_str]) .current_dir(project_root) .output(); + // If the directory still exists (e.g. it was a plain directory from a + // previous failed run, not a registered git worktree), remove it so + // the next `git worktree add` can succeed. + if merge_wt_path.exists() { + let _ = std::fs::remove_dir_all(merge_wt_path); + } let _ = Command::new("git") .args(["branch", "-D", merge_branch]) .current_dir(project_root) @@ -5771,6 +5789,230 @@ theirs assert!(report.had_conflicts, "should report conflicts"); } + /// Verifies that `run_squash_merge` succeeds even when master has advanced + /// with unrelated commits after the merge-queue branch was created (the race + /// condition that previously caused fast-forward to fail). + #[tokio::test] + async fn squash_merge_succeeds_when_master_diverges() { + use std::fs; + use tempfile::tempdir; + + let tmp = tempdir().unwrap(); + let repo = tmp.path(); + init_git_repo(repo); + + // Create an initial file on master. + fs::write(repo.join("base.txt"), "base content\n").unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "initial"]) + .current_dir(repo) + .output() + .unwrap(); + + // Create a feature branch with a new file (clean merge, no conflicts). + Command::new("git") + .args(["checkout", "-b", "feature/story-diverge_test"]) + .current_dir(repo) + .output() + .unwrap(); + fs::write(repo.join("feature.txt"), "feature content\n").unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "feature: add file"]) + .current_dir(repo) + .output() + .unwrap(); + + // Switch back to master and simulate a filesystem watcher commit + // (e.g. a pipeline file move) that advances master beyond the point + // where the merge-queue branch will be created. + Command::new("git") + .args(["checkout", "master"]) + .current_dir(repo) + .output() + .unwrap(); + let sk_dir = repo.join(".story_kit/work/4_merge"); + fs::create_dir_all(&sk_dir).unwrap(); + fs::write( + sk_dir.join("diverge_test.md"), + "---\nname: test\n---\n", + ) + .unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "story-kit: queue diverge_test for merge"]) + .current_dir(repo) + .output() + .unwrap(); + + // Run the squash merge. With the old fast-forward approach, this + // would fail because master diverged. With cherry-pick, it succeeds. + 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 + ); + assert!( + !result.had_conflicts, + "no conflicts expected" + ); + + // Verify the feature file landed on master. + assert!( + repo.join("feature.txt").exists(), + "feature file should be on master after cherry-pick" + ); + let feature_content = fs::read_to_string(repo.join("feature.txt")).unwrap(); + assert_eq!(feature_content, "feature content\n"); + + // Verify the watcher commit's file is still present. + assert!( + sk_dir.join("diverge_test.md").exists(), + "watcher-committed file should still be on master" + ); + + // Verify cleanup: no merge-queue branch, no merge workspace. + let branches = Command::new("git") + .args(["branch", "--list", "merge-queue/*"]) + .current_dir(repo) + .output() + .unwrap(); + let branch_list = String::from_utf8_lossy(&branches.stdout); + assert!( + branch_list.trim().is_empty(), + "merge-queue branch should be cleaned up, got: {branch_list}" + ); + assert!( + !repo.join(".story_kit/merge_workspace").exists(), + "merge workspace should be cleaned up" + ); + } + + /// Verifies that `run_squash_merge` returns `success: true` with an empty + /// diff when the feature branch code is already on master. + #[tokio::test] + async fn squash_merge_empty_diff_succeeds() { + use std::fs; + use tempfile::tempdir; + + let tmp = tempdir().unwrap(); + let repo = tmp.path(); + init_git_repo(repo); + + // Create a file on master. + fs::write(repo.join("code.txt"), "content\n").unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "add code"]) + .current_dir(repo) + .output() + .unwrap(); + + // Create a feature branch with NO additional changes (empty diff). + Command::new("git") + .args(["checkout", "-b", "feature/story-empty_test"]) + .current_dir(repo) + .output() + .unwrap(); + // Make a commit that doesn't actually change anything meaningful + // (e.g. the code was already cherry-picked to master). + Command::new("git") + .args(["checkout", "master"]) + .current_dir(repo) + .output() + .unwrap(); + + let result = + run_squash_merge(repo, "feature/story-empty_test", "empty_test").unwrap(); + + // Empty diff should be treated as success (nothing to merge). + assert!( + result.success, + "empty diff merge should succeed: {}", + result.output + ); + + // Cleanup should still happen. + assert!( + !repo.join(".story_kit/merge_workspace").exists(), + "merge workspace should be cleaned up" + ); + } + + /// Verifies that stale merge_workspace directories from previous failed + /// merges are cleaned up before a new merge attempt. + #[tokio::test] + async fn squash_merge_cleans_up_stale_workspace() { + use std::fs; + use tempfile::tempdir; + + let tmp = tempdir().unwrap(); + let repo = tmp.path(); + init_git_repo(repo); + + // Create a feature branch with a file. + Command::new("git") + .args(["checkout", "-b", "feature/story-stale_test"]) + .current_dir(repo) + .output() + .unwrap(); + fs::write(repo.join("stale.txt"), "content\n").unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "feature: stale test"]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["checkout", "master"]) + .current_dir(repo) + .output() + .unwrap(); + + // Simulate a stale merge workspace left from a previous failed merge. + let stale_ws = repo.join(".story_kit/merge_workspace"); + fs::create_dir_all(&stale_ws).unwrap(); + fs::write(stale_ws.join("leftover.txt"), "stale").unwrap(); + + // Run the merge — it should clean up the stale workspace first. + 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 + ); + assert!( + !stale_ws.exists(), + "stale merge workspace should be cleaned up" + ); + } + // ── process health monitoring tests ────────────────────────────────────── /// Demonstrates that the PTY read-loop inactivity timeout fires when no output diff --git a/server/src/http/mcp.rs b/server/src/http/mcp.rs index 93c3443..0a9fe24 100644 --- a/server/src/http/mcp.rs +++ b/server/src/http/mcp.rs @@ -1,6 +1,7 @@ use crate::agents::{close_bug_to_archive, move_story_to_archived, move_story_to_merge, move_story_to_qa, PipelineStage}; use crate::config::ProjectConfig; use crate::log_buffer; +use crate::slog; use crate::slog_warn; use crate::http::context::AppContext; use crate::http::settings::get_editor_command_from_store; @@ -768,6 +769,24 @@ fn handle_tools_list(id: Option) -> JsonRpcResponse { "required": ["story_id"] } }, + { + "name": "report_merge_failure", + "description": "Report that a merge failed for a story. Leaves the story in work/4_merge/ and logs the failure reason. Use this when merge_agent_work returns success=false instead of manually moving the story file.", + "inputSchema": { + "type": "object", + "properties": { + "story_id": { + "type": "string", + "description": "Story identifier (e.g. '52_story_mergemaster_agent_role')" + }, + "reason": { + "type": "string", + "description": "Human-readable explanation of why the merge failed" + } + }, + "required": ["story_id", "reason"] + } + }, { "name": "request_qa", "description": "Trigger QA review of a completed story worktree: moves the item from work/2_current/ to work/3_qa/ and starts the qa agent to run quality gates, tests, and generate a manual testing plan.", @@ -880,6 +899,7 @@ async fn handle_tools_call( // Mergemaster tools "merge_agent_work" => tool_merge_agent_work(&args, ctx).await, "move_story_to_merge" => tool_move_story_to_merge(&args, ctx).await, + "report_merge_failure" => tool_report_merge_failure(&args), // QA tools "request_qa" => tool_request_qa(&args, ctx).await, // Diagnostics @@ -1568,11 +1588,11 @@ async fn tool_merge_agent_work(args: &Value, ctx: &AppContext) -> Result Result Result { + let story_id = args + .get("story_id") + .and_then(|v| v.as_str()) + .ok_or("Missing required argument: story_id")?; + let reason = args + .get("reason") + .and_then(|v| v.as_str()) + .ok_or("Missing required argument: reason")?; + + slog!("[mergemaster] Merge failure reported for '{story_id}': {reason}"); + + Ok(format!( + "Merge failure for '{story_id}' recorded. Story remains in work/4_merge/. Reason: {reason}" + )) +} + // ── QA tool implementations ─────────────────────────────────────── async fn tool_request_qa(args: &Value, ctx: &AppContext) -> Result { @@ -1895,10 +1932,11 @@ mod tests { assert!(names.contains(&"close_bug")); assert!(names.contains(&"merge_agent_work")); assert!(names.contains(&"move_story_to_merge")); + assert!(names.contains(&"report_merge_failure")); assert!(names.contains(&"request_qa")); assert!(names.contains(&"get_server_logs")); assert!(names.contains(&"prompt_permission")); - assert_eq!(tools.len(), 30); + assert_eq!(tools.len(), 31); } #[test] @@ -2579,6 +2617,52 @@ mod tests { assert!(parsed.get("message").is_some()); } + // ── report_merge_failure tool tests ───────────────────────────── + + #[test] + fn report_merge_failure_in_tools_list() { + let resp = handle_tools_list(Some(json!(1))); + let tools = resp.result.unwrap()["tools"].as_array().unwrap().clone(); + let tool = tools.iter().find(|t| t["name"] == "report_merge_failure"); + assert!( + tool.is_some(), + "report_merge_failure missing from tools list" + ); + let t = tool.unwrap(); + assert!(t["description"].is_string()); + let required = t["inputSchema"]["required"].as_array().unwrap(); + let req_names: Vec<&str> = required.iter().map(|v| v.as_str().unwrap()).collect(); + assert!(req_names.contains(&"story_id")); + assert!(req_names.contains(&"reason")); + } + + #[test] + fn tool_report_merge_failure_missing_story_id() { + let result = tool_report_merge_failure(&json!({"reason": "conflicts"})); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("story_id")); + } + + #[test] + fn tool_report_merge_failure_missing_reason() { + let result = tool_report_merge_failure(&json!({"story_id": "42_story_foo"})); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("reason")); + } + + #[test] + fn tool_report_merge_failure_returns_confirmation() { + let result = tool_report_merge_failure(&json!({ + "story_id": "42_story_foo", + "reason": "Unresolvable merge conflicts in src/main.rs" + })); + assert!(result.is_ok()); + let msg = result.unwrap(); + assert!(msg.contains("42_story_foo")); + assert!(msg.contains("work/4_merge/")); + assert!(msg.contains("Unresolvable merge conflicts")); + } + // ── HTTP handler tests (TestClient) ─────────────────────────── fn test_mcp_app(ctx: std::sync::Arc) -> impl poem::Endpoint {