dave/storkit
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 7 Wiki Activity

fix: replace fast-forward with cherry-pick in mergemaster squash-merge

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Dave
2026-02-26 14:16:35 +00:00
parent c435d86d1a
commit 81065a3ada
3 changed files with 355 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

274
server/src/agents.rs
View File

@@ -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

90
server/src/http/mcp.rs
View File

@@ -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<Value>) -> 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<String,
} 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. Report the conflict details so the human can resolve them."
"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."
"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."
};
serde_json::to_string_pretty(&json!({
@@ -1625,6 +1645,23 @@ async fn tool_move_story_to_merge(args: &Value, ctx: &AppContext) -> Result<Stri
.map_err(|e| format!("Serialization error: {e}"))
}
fn tool_report_merge_failure(args: &Value) -> Result<String, String> {
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<String, String> {
@@ -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<AppContext>) -> impl poem::Endpoint {