fix(426): verify cherry-pick landed on master before marking story done
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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 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 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.
|
/// * 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 sk = project_root.join(".storkit").join("work");
|
||||||
let current_path = sk.join("2_current").join(format!("{story_id}.md"));
|
let current_path = sk.join("2_current").join(format!("{story_id}.md"));
|
||||||
let merge_path = sk.join("4_merge").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/"));
|
assert!(result.unwrap_err().contains("not found in work/2_current/"));
|
||||||
}
|
}
|
||||||
|
|
||||||
// ── move_story_to_archived tests ──────────────────────────────────────────
|
// ── move_story_to_done tests ──────────────────────────────────────────
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn move_story_to_archived_finds_in_merge_dir() {
|
fn move_story_to_done_finds_in_merge_dir() {
|
||||||
use std::fs;
|
use std::fs;
|
||||||
let tmp = tempfile::tempdir().unwrap();
|
let tmp = tempfile::tempdir().unwrap();
|
||||||
let root = tmp.path();
|
let root = tmp.path();
|
||||||
@@ -595,16 +595,16 @@ mod tests {
|
|||||||
fs::create_dir_all(&merge_dir).unwrap();
|
fs::create_dir_all(&merge_dir).unwrap();
|
||||||
fs::write(merge_dir.join("22_story_test.md"), "test").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!(!merge_dir.join("22_story_test.md").exists());
|
||||||
assert!(root.join(".storkit/work/5_done/22_story_test.md").exists());
|
assert!(root.join(".storkit/work/5_done/22_story_test.md").exists());
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[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 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"));
|
assert!(result.unwrap_err().contains("4_merge"));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -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 ──────────────────────────────────────────────────
|
// ── Clean up ──────────────────────────────────────────────────
|
||||||
cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch);
|
cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch);
|
||||||
all_output.push_str("=== Merge-queue cleanup complete ===\n");
|
all_output.push_str("=== Merge-queue cleanup complete ===\n");
|
||||||
|
|||||||
@@ -10,7 +10,7 @@ use crate::config::AgentConfig;
|
|||||||
use serde::{Deserialize, Serialize};
|
use serde::{Deserialize, Serialize};
|
||||||
|
|
||||||
pub use lifecycle::{
|
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,
|
move_story_to_merge, move_story_to_qa, move_story_to_stage, reject_story_from_qa,
|
||||||
};
|
};
|
||||||
pub use pool::AgentPool;
|
pub use pool::AgentPool;
|
||||||
|
|||||||
@@ -1729,7 +1729,7 @@ stage = "coder"
|
|||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn archiving_story_removes_agent_entries_from_pool() {
|
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;
|
use std::fs;
|
||||||
|
|
||||||
let tmp = tempfile::tempdir().unwrap();
|
let tmp = tempfile::tempdir().unwrap();
|
||||||
@@ -1746,7 +1746,7 @@ stage = "coder"
|
|||||||
|
|
||||||
assert_eq!(pool.list_agents().unwrap().len(), 3);
|
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");
|
pool.remove_agents_for_story("60_story_cleanup");
|
||||||
|
|
||||||
let remaining = pool.list_agents().unwrap();
|
let remaining = pool.list_agents().unwrap();
|
||||||
|
|||||||
@@ -308,7 +308,7 @@ impl AgentPool {
|
|||||||
"[pipeline] Post-merge tests passed for '{story_id}'. Moving to done."
|
"[pipeline] Post-merge tests passed for '{story_id}'. Moving to done."
|
||||||
);
|
);
|
||||||
if let Err(e) =
|
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}");
|
slog_error!("[pipeline] Failed to move '{story_id}' to done: {e}");
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -104,7 +104,7 @@ impl AgentPool {
|
|||||||
}
|
}
|
||||||
|
|
||||||
let story_archived =
|
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 {
|
if story_archived {
|
||||||
self.remove_agents_for_story(story_id);
|
self.remove_agents_for_story(story_id);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
use crate::agents::{
|
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::context::AppContext;
|
||||||
use crate::http::workflow::{
|
use crate::http::workflow::{
|
||||||
@@ -246,7 +246,7 @@ pub(super) fn tool_accept_story(args: &Value, ctx: &AppContext) -> Result<String
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
move_story_to_archived(&project_root, story_id)?;
|
move_story_to_done(&project_root, story_id)?;
|
||||||
ctx.agents.remove_agents_for_story(story_id);
|
ctx.agents.remove_agents_for_story(story_id);
|
||||||
|
|
||||||
Ok(format!(
|
Ok(format!(
|
||||||
@@ -1331,7 +1331,7 @@ mod tests {
|
|||||||
.output()
|
.output()
|
||||||
.unwrap();
|
.unwrap();
|
||||||
|
|
||||||
// Create story file in current/ so move_story_to_archived would work.
|
// Create story file in current/ so move_story_to_done would work.
|
||||||
let current_dir = tmp.path().join(".storkit/work/2_current");
|
let current_dir = tmp.path().join(".storkit/work/2_current");
|
||||||
std::fs::create_dir_all(¤t_dir).unwrap();
|
std::fs::create_dir_all(¤t_dir).unwrap();
|
||||||
std::fs::write(
|
std::fs::write(
|
||||||
|
|||||||
Reference in New Issue
Block a user