From 850ca15a6c881d4b1493520eee58e5b479e4e37d Mon Sep 17 00:00:00 2001 From: Dave Date: Fri, 27 Feb 2026 10:37:27 +0000 Subject: [PATCH] story-kit: merge 226_bug_mergemaster_accepts_stories_without_squash_merging_code --- server/src/agents.rs | 209 ++++++++++++++++++++++++++++++++++++++--- server/src/http/mcp.rs | 83 +++++++++++++++- 2 files changed, 279 insertions(+), 13 deletions(-) diff --git a/server/src/agents.rs b/server/src/agents.rs index 6b96821..53fb7ba 100644 --- a/server/src/agents.rs +++ b/server/src/agents.rs @@ -2270,6 +2270,37 @@ pub fn move_story_to_current(project_root: &Path, story_id: &str) -> Result<(), Ok(()) } +/// Check whether a feature branch `feature/story-{story_id}` exists and has +/// commits that are not yet on master. Returns `true` when there is unmerged +/// work, `false` when there is no branch or all its commits are already +/// reachable from master. +pub fn feature_branch_has_unmerged_changes(project_root: &Path, story_id: &str) -> bool { + let branch = format!("feature/story-{story_id}"); + + // Check if the branch exists. + let branch_check = Command::new("git") + .args(["rev-parse", "--verify", &branch]) + .current_dir(project_root) + .output(); + match branch_check { + Ok(out) if out.status.success() => {} + _ => return false, // No feature branch → nothing to merge. + } + + // Check if the branch has commits not reachable from master. + let log = Command::new("git") + .args(["log", &format!("master..{branch}"), "--oneline"]) + .current_dir(project_root) + .output(); + match log { + Ok(out) => { + let stdout = String::from_utf8_lossy(&out.stdout); + !stdout.trim().is_empty() + } + Err(_) => false, + } +} + /// Move a story from `work/2_current/` to `work/5_done/` and auto-commit. /// /// * If the story is in `2_current/`, it is moved to `5_done/` and committed. @@ -2778,18 +2809,27 @@ fn run_squash_merge( all_output.push('\n'); if !commit.status.success() { - // Nothing to commit (e.g. empty diff) — treat as success. + // Bug 226: "nothing to commit" means the feature branch has no changes + // beyond what's already on master. This must NOT be treated as success + // — it means the code was never actually merged. if commit_stderr.contains("nothing to commit") || commit_stdout.contains("nothing to commit") { + all_output.push_str( + "=== Nothing to commit — feature branch has no changes beyond master ===\n", + ); cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); return Ok(SquashMergeResult { - success: true, + success: false, had_conflicts, conflicts_resolved, - conflict_details, + conflict_details: Some( + "Squash-merge resulted in an empty diff — the feature branch has no \ + code changes to merge into master." + .to_string(), + ), output: all_output, - gates_passed: true, + gates_passed: false, }); } cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); @@ -2803,6 +2843,39 @@ fn run_squash_merge( }); } + // ── Bug 226: Verify the commit contains real code changes ───── + // If the merge only brought in .story_kit/ files (pipeline file moves), + // there are no actual code changes to land on master. Abort. + { + let diff_check = Command::new("git") + .args(["diff", "--name-only", "HEAD~1..HEAD"]) + .current_dir(&merge_wt_path) + .output() + .map_err(|e| format!("Failed to check merge diff: {e}"))?; + let changed_files = String::from_utf8_lossy(&diff_check.stdout); + let has_code_changes = changed_files + .lines() + .any(|f| !f.starts_with(".story_kit/")); + if !has_code_changes { + all_output.push_str( + "=== Merge commit contains only .story_kit/ 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, + conflict_details: Some( + "Feature branch has no code changes outside .story_kit/ — only \ + pipeline file moves were found." + .to_string(), + ), + output: all_output, + gates_passed: false, + }); + } + } + // ── Run component setup from project.toml (same as worktree creation) ────────── { let config = ProjectConfig::load(&merge_wt_path).unwrap_or_default(); @@ -5909,10 +5982,10 @@ theirs ); } - /// Verifies that `run_squash_merge` returns `success: true` with an empty - /// diff when the feature branch code is already on master. + /// Bug 226: Verifies that `run_squash_merge` returns `success: false` when + /// the feature branch has no changes beyond what's already on master (empty diff). #[tokio::test] - async fn squash_merge_empty_diff_succeeds() { + async fn squash_merge_empty_diff_fails() { use std::fs; use tempfile::tempdir; @@ -5939,8 +6012,6 @@ theirs .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) @@ -5950,10 +6021,10 @@ theirs let result = run_squash_merge(repo, "feature/story-empty_test", "empty_test").unwrap(); - // Empty diff should be treated as success (nothing to merge). + // Bug 226: empty diff must NOT be treated as success. assert!( - result.success, - "empty diff merge should succeed: {}", + !result.success, + "empty diff merge must fail, not silently succeed: {}", result.output ); @@ -5964,6 +6035,120 @@ theirs ); } + /// Bug 226: Verifies that `run_squash_merge` fails when the feature branch + /// only contains .story_kit/ file moves with no real code changes. + #[tokio::test] + async fn squash_merge_md_only_changes_fails() { + use std::fs; + use tempfile::tempdir; + + let tmp = tempdir().unwrap(); + let repo = tmp.path(); + init_git_repo(repo); + + // Create a feature branch that only moves a .story_kit/ file. + Command::new("git") + .args(["checkout", "-b", "feature/story-md_only_test"]) + .current_dir(repo) + .output() + .unwrap(); + let sk_dir = repo.join(".story_kit/work/2_current"); + fs::create_dir_all(&sk_dir).unwrap(); + fs::write( + sk_dir.join("md_only_test.md"), + "---\nname: Test\n---\n", + ) + .unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "move story file"]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["checkout", "master"]) + .current_dir(repo) + .output() + .unwrap(); + + let result = + run_squash_merge(repo, "feature/story-md_only_test", "md_only_test").unwrap(); + + // The squash merge will commit the .story_kit/ file, but should fail because + // there are no code changes outside .story_kit/. + assert!( + !result.success, + "merge with only .story_kit/ changes must fail: {}", + result.output + ); + + // Cleanup should still happen. + assert!( + !repo.join(".story_kit/merge_workspace").exists(), + "merge workspace should be cleaned up" + ); + } + + /// Bug 226: feature_branch_has_unmerged_changes returns true when the + /// feature branch has commits not on master. + #[test] + fn feature_branch_has_unmerged_changes_detects_unmerged_code() { + use std::fs; + use tempfile::tempdir; + + let tmp = tempdir().unwrap(); + let repo = tmp.path(); + init_git_repo(repo); + + // Create a feature branch with a code commit. + Command::new("git") + .args(["checkout", "-b", "feature/story-50_story_test"]) + .current_dir(repo) + .output() + .unwrap(); + fs::write(repo.join("feature.rs"), "fn main() {}").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(); + + assert!( + feature_branch_has_unmerged_changes(repo, "50_story_test"), + "should detect unmerged changes on feature branch" + ); + } + + /// Bug 226: feature_branch_has_unmerged_changes returns false when no + /// feature branch exists. + #[test] + fn feature_branch_has_unmerged_changes_false_when_no_branch() { + use tempfile::tempdir; + + let tmp = tempdir().unwrap(); + let repo = tmp.path(); + init_git_repo(repo); + + assert!( + !feature_branch_has_unmerged_changes(repo, "99_nonexistent"), + "should return false when no feature branch" + ); + } + /// Verifies that stale merge_workspace directories from previous failed /// merges are cleaned up before a new merge attempt. #[tokio::test] diff --git a/server/src/http/mcp.rs b/server/src/http/mcp.rs index 148caf0..d24fe2e 100644 --- a/server/src/http/mcp.rs +++ b/server/src/http/mcp.rs @@ -1,4 +1,4 @@ -use crate::agents::{close_bug_to_archive, move_story_to_archived, move_story_to_merge, move_story_to_qa, PipelineStage}; +use crate::agents::{close_bug_to_archive, feature_branch_has_unmerged_changes, move_story_to_archived, move_story_to_merge, move_story_to_qa, PipelineStage}; use crate::config::ProjectConfig; use crate::log_buffer; use crate::slog; @@ -1432,6 +1432,17 @@ fn tool_accept_story(args: &Value, ctx: &AppContext) -> Result { .ok_or("Missing required argument: story_id")?; let project_root = ctx.agents.get_project_root(&ctx.state)?; + + // Bug 226: Refuse to accept if the feature branch has unmerged code. + // The code must be squash-merged via merge_agent_work first. + if feature_branch_has_unmerged_changes(&project_root, story_id) { + return Err(format!( + "Cannot accept story '{story_id}': feature branch 'feature/story-{story_id}' \ + has unmerged changes. Use merge_agent_work to squash-merge the code into \ + master first." + )); + } + move_story_to_archived(&project_root, story_id)?; ctx.agents.remove_agents_for_story(story_id); @@ -3113,6 +3124,76 @@ mod tests { assert!(result.is_err()); } + /// Bug 226: accept_story must refuse when the feature branch has unmerged code. + #[test] + fn tool_accept_story_refuses_when_feature_branch_has_unmerged_code() { + let tmp = tempfile::tempdir().unwrap(); + setup_git_repo_in(tmp.path()); + + // Create a feature branch with code changes. + std::process::Command::new("git") + .args(["checkout", "-b", "feature/story-50_story_test"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::fs::write(tmp.path().join("feature.rs"), "fn main() {}").unwrap(); + std::process::Command::new("git") + .args(["add", "."]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["commit", "-m", "add feature"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["checkout", "master"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + + // Create story file in current/ so move_story_to_archived would work. + let current_dir = tmp.path().join(".story_kit/work/2_current"); + std::fs::create_dir_all(¤t_dir).unwrap(); + std::fs::write( + current_dir.join("50_story_test.md"), + "---\nname: Test\n---\n", + ) + .unwrap(); + + let ctx = test_ctx(tmp.path()); + let result = + tool_accept_story(&json!({"story_id": "50_story_test"}), &ctx); + assert!(result.is_err(), "should refuse when feature branch has unmerged code"); + let err = result.unwrap_err(); + assert!( + err.contains("unmerged"), + "error should mention unmerged changes: {err}" + ); + } + + /// Bug 226: accept_story succeeds when no feature branch exists (e.g. manual stories). + #[test] + fn tool_accept_story_succeeds_when_no_feature_branch() { + let tmp = tempfile::tempdir().unwrap(); + setup_git_repo_in(tmp.path()); + + // Create story file in current/ (no feature branch). + let current_dir = tmp.path().join(".story_kit/work/2_current"); + std::fs::create_dir_all(¤t_dir).unwrap(); + std::fs::write( + current_dir.join("51_story_no_branch.md"), + "---\nname: No Branch\n---\n", + ) + .unwrap(); + + let ctx = test_ctx(tmp.path()); + let result = + tool_accept_story(&json!({"story_id": "51_story_no_branch"}), &ctx); + assert!(result.is_ok(), "should succeed when no feature branch: {result:?}"); + } + // ── tool_check_criterion tests ──────────────────────────────── #[test]