From 953fce2ca648f7feb63fa15555f3803c07522ae2 Mon Sep 17 00:00:00 2001 From: dave Date: Sat, 28 Mar 2026 12:37:03 +0000 Subject: [PATCH] fix(426): verify cherry-pick landed on master before marking story done MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After the cherry-pick step in run_squash_merge, verify: 1. project_root is on the base branch (not a merge-queue branch) 2. HEAD commit has actual code changes (not an empty/story-only diff) If either check fails, return success=false so the story stays in merge stage for retry instead of being phantom-advanced to done. Also rename move_story_to_archived → move_story_to_done. Co-Authored-By: Claude Opus 4.6 (1M context) --- server/src/agents/lifecycle.rs | 12 ++-- server/src/agents/merge.rs | 65 ++++++++++++++++++++++ server/src/agents/mod.rs | 2 +- server/src/agents/pool/lifecycle.rs | 4 +- server/src/agents/pool/pipeline/advance.rs | 2 +- server/src/agents/pool/pipeline/merge.rs | 2 +- server/src/http/mcp/story_tools.rs | 6 +- 7 files changed, 79 insertions(+), 14 deletions(-) diff --git a/server/src/agents/lifecycle.rs b/server/src/agents/lifecycle.rs index 9e33018b..3a6896d3 100644 --- a/server/src/agents/lifecycle.rs +++ b/server/src/agents/lifecycle.rs @@ -102,7 +102,7 @@ pub fn feature_branch_has_unmerged_changes(project_root: &Path, story_id: &str) /// * If the story is in `4_merge/`, it is moved to `5_done/` and committed. /// * If the story is already in `5_done/` or `6_archived/`, this is a no-op (idempotent). /// * If the story is not found in `2_current/`, `4_merge/`, `5_done/`, or `6_archived/`, an error is returned. -pub fn move_story_to_archived(project_root: &Path, story_id: &str) -> Result<(), String> { +pub fn move_story_to_done(project_root: &Path, story_id: &str) -> Result<(), String> { let sk = project_root.join(".storkit").join("work"); let current_path = sk.join("2_current").join(format!("{story_id}.md")); let merge_path = sk.join("4_merge").join(format!("{story_id}.md")); @@ -584,10 +584,10 @@ mod tests { assert!(result.unwrap_err().contains("not found in work/2_current/")); } - // ── move_story_to_archived tests ────────────────────────────────────────── + // ── move_story_to_done tests ────────────────────────────────────────── #[test] - fn move_story_to_archived_finds_in_merge_dir() { + fn move_story_to_done_finds_in_merge_dir() { use std::fs; let tmp = tempfile::tempdir().unwrap(); let root = tmp.path(); @@ -595,16 +595,16 @@ mod tests { fs::create_dir_all(&merge_dir).unwrap(); fs::write(merge_dir.join("22_story_test.md"), "test").unwrap(); - move_story_to_archived(root, "22_story_test").unwrap(); + move_story_to_done(root, "22_story_test").unwrap(); assert!(!merge_dir.join("22_story_test.md").exists()); assert!(root.join(".storkit/work/5_done/22_story_test.md").exists()); } #[test] - fn move_story_to_archived_error_when_not_in_current_or_merge() { + fn move_story_to_done_error_when_not_in_current_or_merge() { let tmp = tempfile::tempdir().unwrap(); - let result = move_story_to_archived(tmp.path(), "99_nonexistent"); + let result = move_story_to_done(tmp.path(), "99_nonexistent"); assert!(result.unwrap_err().contains("4_merge")); } diff --git a/server/src/agents/merge.rs b/server/src/agents/merge.rs index f496bc11..88a7790e 100644 --- a/server/src/agents/merge.rs +++ b/server/src/agents/merge.rs @@ -383,6 +383,71 @@ pub(crate) fn run_squash_merge( }); } + // ── Verify code landed on the correct branch ────────────────── + // Guard against the cherry-pick silently landing on the wrong branch + // (e.g. a merge-queue branch from a concurrent merge). If the current + // branch is not the base branch, or the HEAD commit has no code diff, + // treat the merge as failed so the story stays in the merge stage. + let current_branch = Command::new("git") + .args(["rev-parse", "--abbrev-ref", "HEAD"]) + .current_dir(project_root) + .output() + .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_string()) + .unwrap_or_default(); + + let base_branch = crate::config::ProjectConfig::load(project_root) + .ok() + .and_then(|c| c.base_branch.clone()) + .unwrap_or_else(|| "master".to_string()); + + if current_branch != base_branch { + all_output.push_str(&format!( + "=== VERIFICATION FAILED: expected branch '{base_branch}' but HEAD is on \ + '{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, + conflict_details: Some(format!( + "Cherry-pick landed on '{current_branch}' instead of '{base_branch}'" + )), + output: all_output, + gates_passed: true, + }); + } + + // Verify HEAD commit has actual code changes (not an empty cherry-pick). + // Exclude .storkit/ so that story-file-only commits don't pass this check. + let diff_stat = Command::new("git") + .args(["diff", "--stat", "HEAD~1..HEAD", "--", ".", ":(exclude).storkit"]) + .current_dir(project_root) + .output() + .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_string()) + .unwrap_or_default(); + + if diff_stat.is_empty() { + all_output.push_str( + "=== 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, + conflict_details: Some( + "Cherry-pick commit contains no code changes (empty diff)".to_string(), + ), + output: all_output, + gates_passed: true, + }); + } + + all_output.push_str(&format!( + "=== Verified: cherry-pick landed on '{base_branch}' with code changes ===\n" + )); + // ── Clean up ────────────────────────────────────────────────── cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); all_output.push_str("=== Merge-queue cleanup complete ===\n"); diff --git a/server/src/agents/mod.rs b/server/src/agents/mod.rs index 81bbb379..31ef12ab 100644 --- a/server/src/agents/mod.rs +++ b/server/src/agents/mod.rs @@ -10,7 +10,7 @@ use crate::config::AgentConfig; use serde::{Deserialize, Serialize}; pub use lifecycle::{ - close_bug_to_archive, feature_branch_has_unmerged_changes, move_story_to_archived, + close_bug_to_archive, feature_branch_has_unmerged_changes, move_story_to_done, move_story_to_merge, move_story_to_qa, move_story_to_stage, reject_story_from_qa, }; pub use pool::AgentPool; diff --git a/server/src/agents/pool/lifecycle.rs b/server/src/agents/pool/lifecycle.rs index 55c2ad6c..ecfa1272 100644 --- a/server/src/agents/pool/lifecycle.rs +++ b/server/src/agents/pool/lifecycle.rs @@ -1729,7 +1729,7 @@ stage = "coder" #[tokio::test] async fn archiving_story_removes_agent_entries_from_pool() { - use crate::agents::lifecycle::move_story_to_archived; + use crate::agents::lifecycle::move_story_to_done; use std::fs; let tmp = tempfile::tempdir().unwrap(); @@ -1746,7 +1746,7 @@ stage = "coder" assert_eq!(pool.list_agents().unwrap().len(), 3); - move_story_to_archived(root, "60_story_cleanup").unwrap(); + move_story_to_done(root, "60_story_cleanup").unwrap(); pool.remove_agents_for_story("60_story_cleanup"); let remaining = pool.list_agents().unwrap(); diff --git a/server/src/agents/pool/pipeline/advance.rs b/server/src/agents/pool/pipeline/advance.rs index 8e75107c..ab0b7a2b 100644 --- a/server/src/agents/pool/pipeline/advance.rs +++ b/server/src/agents/pool/pipeline/advance.rs @@ -308,7 +308,7 @@ impl AgentPool { "[pipeline] Post-merge tests passed for '{story_id}'. Moving to done." ); if let Err(e) = - crate::agents::lifecycle::move_story_to_archived(&project_root, story_id) + crate::agents::lifecycle::move_story_to_done(&project_root, story_id) { slog_error!("[pipeline] Failed to move '{story_id}' to done: {e}"); } diff --git a/server/src/agents/pool/pipeline/merge.rs b/server/src/agents/pool/pipeline/merge.rs index 30b94ae8..82a697cd 100644 --- a/server/src/agents/pool/pipeline/merge.rs +++ b/server/src/agents/pool/pipeline/merge.rs @@ -104,7 +104,7 @@ impl AgentPool { } let story_archived = - crate::agents::lifecycle::move_story_to_archived(project_root, story_id).is_ok(); + crate::agents::lifecycle::move_story_to_done(project_root, story_id).is_ok(); if story_archived { self.remove_agents_for_story(story_id); } diff --git a/server/src/http/mcp/story_tools.rs b/server/src/http/mcp/story_tools.rs index f699507a..067fc14a 100644 --- a/server/src/http/mcp/story_tools.rs +++ b/server/src/http/mcp/story_tools.rs @@ -1,5 +1,5 @@ use crate::agents::{ - close_bug_to_archive, feature_branch_has_unmerged_changes, move_story_to_archived, + close_bug_to_archive, feature_branch_has_unmerged_changes, move_story_to_done, }; use crate::http::context::AppContext; use crate::http::workflow::{ @@ -246,7 +246,7 @@ pub(super) fn tool_accept_story(args: &Value, ctx: &AppContext) -> Result