From 86694a43839e31efc61981bd8fa054a853f39ba8 Mon Sep 17 00:00:00 2001 From: Dave Date: Tue, 17 Mar 2026 16:33:55 +0000 Subject: [PATCH] story-kit: merge 265_story_spikes_skip_merge_and_stop_for_human_review --- server/src/agents/lifecycle.rs | 3 +- server/src/agents/pool.rs | 162 +++++++++++++++++++++++++++++--- server/src/io/story_metadata.rs | 39 ++++++++ 3 files changed, 187 insertions(+), 17 deletions(-) diff --git a/server/src/agents/lifecycle.rs b/server/src/agents/lifecycle.rs index dbd7b3a..328ed3a 100644 --- a/server/src/agents/lifecycle.rs +++ b/server/src/agents/lifecycle.rs @@ -4,8 +4,7 @@ use std::process::Command; use crate::io::story_metadata::clear_front_matter_field; use crate::slog; -#[allow(dead_code)] -fn item_type_from_id(item_id: &str) -> &'static str { +pub(super) fn item_type_from_id(item_id: &str) -> &'static str { // New format: {digits}_{type}_{slug} let after_num = item_id.trim_start_matches(|c: char| c.is_ascii_digit()); if after_num.starts_with("_bug_") { diff --git a/server/src/agents/pool.rs b/server/src/agents/pool.rs index 3986cbc..b2cc192 100644 --- a/server/src/agents/pool.rs +++ b/server/src/agents/pool.rs @@ -889,21 +889,39 @@ impl AgentPool { }; if coverage_passed { - slog!( - "[pipeline] QA passed gates and coverage for '{story_id}'. Moving to merge." - ); - if let Err(e) = super::lifecycle::move_story_to_merge(&project_root, story_id) { - slog_error!("[pipeline] Failed to move '{story_id}' to 4_merge/: {e}"); - return; + // Spikes skip merge — they stay in 3_qa/ for human review. + if super::lifecycle::item_type_from_id(story_id) == "spike" { + // Mark the spike as held for review so auto-assign won't + // restart QA on it. + let qa_dir = project_root.join(".story_kit/work/3_qa"); + let spike_path = qa_dir.join(format!("{story_id}.md")); + if let Err(e) = crate::io::story_metadata::write_review_hold(&spike_path) { + slog_error!("[pipeline] Failed to set review_hold on '{story_id}': {e}"); + } + slog!( + "[pipeline] QA passed for spike '{story_id}'. \ + Stopping for human review (skipping merge). \ + Worktree preserved at: {worktree_path:?}" + ); + // Free up the QA slot without advancing the spike. + self.auto_assign_available_work(&project_root).await; + } else { + slog!( + "[pipeline] QA passed gates and coverage for '{story_id}'. Moving to merge." + ); + if let Err(e) = super::lifecycle::move_story_to_merge(&project_root, story_id) { + slog_error!("[pipeline] Failed to move '{story_id}' to 4_merge/: {e}"); + return; + } + if let Err(e) = self + .start_agent(&project_root, story_id, Some("mergemaster"), None) + .await + { + slog_error!("[pipeline] Failed to start mergemaster for '{story_id}': {e}"); + } + // QA slot is now free — pick up any other unassigned work in 3_qa/. + self.auto_assign_available_work(&project_root).await; } - if let Err(e) = self - .start_agent(&project_root, story_id, Some("mergemaster"), None) - .await - { - slog_error!("[pipeline] Failed to start mergemaster for '{story_id}': {e}"); - } - // QA slot is now free — pick up any other unassigned work in 3_qa/. - self.auto_assign_available_work(&project_root).await; } else { slog!( "[pipeline] QA coverage gate failed for '{story_id}'. Restarting QA." @@ -1444,6 +1462,12 @@ impl AgentPool { } for story_id in &items { + // Items marked with review_hold (e.g. spikes after QA passes) stay + // in their current stage for human review — don't auto-assign agents. + if has_review_hold(project_root, stage_dir, story_id) { + continue; + } + // Re-acquire the lock on each iteration to see state changes // from previous start_agent calls in the same pass. let preferred_agent = @@ -1707,7 +1731,25 @@ impl AgentPool { }; if coverage_passed { - if let Err(e) = super::lifecycle::move_story_to_merge(project_root, story_id) { + // Spikes skip the merge stage — stay in 3_qa/ for human review. + if super::lifecycle::item_type_from_id(story_id) == "spike" { + let spike_path = project_root + .join(".story_kit/work/3_qa") + .join(format!("{story_id}.md")); + if let Err(e) = crate::io::story_metadata::write_review_hold(&spike_path) { + eprintln!( + "[startup:reconcile] Failed to set review_hold on spike '{story_id}': {e}" + ); + } + eprintln!( + "[startup:reconcile] Spike '{story_id}' passed QA — holding for human review." + ); + let _ = progress_tx.send(ReconciliationEvent { + story_id: story_id.clone(), + status: "review_hold".to_string(), + message: "Spike passed QA — waiting for human review.".to_string(), + }); + } else if let Err(e) = super::lifecycle::move_story_to_merge(project_root, story_id) { eprintln!( "[startup:reconcile] Failed to move '{story_id}' to 4_merge/: {e}" ); @@ -1922,6 +1964,24 @@ fn read_story_front_matter_agent(project_root: &Path, stage_dir: &str, story_id: parse_front_matter(&contents).ok()?.agent } +/// Return `true` if the story file in the given stage has `review_hold: true` in its front matter. +fn has_review_hold(project_root: &Path, stage_dir: &str, story_id: &str) -> bool { + use crate::io::story_metadata::parse_front_matter; + let path = project_root + .join(".story_kit") + .join("work") + .join(stage_dir) + .join(format!("{story_id}.md")); + let contents = match std::fs::read_to_string(path) { + Ok(c) => c, + Err(_) => return false, + }; + parse_front_matter(&contents) + .ok() + .and_then(|m| m.review_hold) + .unwrap_or(false) +} + /// Return `true` if `agent_name` has no active (pending/running) entry in the pool. fn is_agent_free(agents: &HashMap, agent_name: &str) -> bool { !agents.values().any(|a| { @@ -4621,4 +4681,76 @@ stage = "coder" "story should be in 2_current/ or 3_qa/ after reconciliation" ); } + + #[test] + fn has_review_hold_returns_true_when_set() { + let tmp = tempfile::tempdir().unwrap(); + let qa_dir = tmp.path().join(".story_kit/work/3_qa"); + std::fs::create_dir_all(&qa_dir).unwrap(); + let spike_path = qa_dir.join("10_spike_research.md"); + std::fs::write( + &spike_path, + "---\nname: Research spike\nreview_hold: true\n---\n# Spike\n", + ) + .unwrap(); + assert!(has_review_hold(tmp.path(), "3_qa", "10_spike_research")); + } + + #[test] + fn has_review_hold_returns_false_when_not_set() { + let tmp = tempfile::tempdir().unwrap(); + let qa_dir = tmp.path().join(".story_kit/work/3_qa"); + std::fs::create_dir_all(&qa_dir).unwrap(); + let spike_path = qa_dir.join("10_spike_research.md"); + std::fs::write( + &spike_path, + "---\nname: Research spike\n---\n# Spike\n", + ) + .unwrap(); + assert!(!has_review_hold(tmp.path(), "3_qa", "10_spike_research")); + } + + #[test] + fn has_review_hold_returns_false_when_file_missing() { + let tmp = tempfile::tempdir().unwrap(); + assert!(!has_review_hold(tmp.path(), "3_qa", "99_spike_missing")); + } + + /// Story 265: auto_assign_available_work must skip spikes in 3_qa/ that + /// have review_hold: true set in their front matter. + #[tokio::test] + async fn auto_assign_skips_spikes_with_review_hold() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + // Create project.toml with a QA agent. + let sk = root.join(".story_kit"); + std::fs::create_dir_all(&sk).unwrap(); + std::fs::write( + sk.join("project.toml"), + "[[agents]]\nname = \"qa\"\nrole = \"qa\"\nmodel = \"test\"\nprompt = \"test\"\n", + ) + .unwrap(); + + // Put a spike in 3_qa/ with review_hold: true. + let qa_dir = root.join(".story_kit/work/3_qa"); + std::fs::create_dir_all(&qa_dir).unwrap(); + std::fs::write( + qa_dir.join("20_spike_test.md"), + "---\nname: Test Spike\nreview_hold: true\n---\n# Spike\n", + ) + .unwrap(); + + let (watcher_tx, _) = broadcast::channel::(4); + let pool = AgentPool::new(3001, watcher_tx); + + pool.auto_assign_available_work(root).await; + + // No agent should have been started for the spike. + let agents = pool.agents.lock().unwrap(); + assert!( + agents.is_empty(), + "No agents should be assigned to a spike with review_hold" + ); + } } diff --git a/server/src/io/story_metadata.rs b/server/src/io/story_metadata.rs index 4acef86..6d8c17e 100644 --- a/server/src/io/story_metadata.rs +++ b/server/src/io/story_metadata.rs @@ -8,6 +8,7 @@ pub struct StoryMetadata { pub coverage_baseline: Option, pub merge_failure: Option, pub agent: Option, + pub review_hold: Option, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -31,6 +32,7 @@ struct FrontMatter { coverage_baseline: Option, merge_failure: Option, agent: Option, + review_hold: Option, } pub fn parse_front_matter(contents: &str) -> Result { @@ -64,6 +66,7 @@ fn build_metadata(front: FrontMatter) -> StoryMetadata { coverage_baseline: front.coverage_baseline, merge_failure: front.merge_failure, agent: front.agent, + review_hold: front.review_hold, } } @@ -98,6 +101,17 @@ pub fn write_merge_failure(path: &Path, reason: &str) -> Result<(), String> { Ok(()) } +/// Write `review_hold: true` to the YAML front matter of a story file. +/// +/// Used to mark spikes that have passed QA and are waiting for human review. +pub fn write_review_hold(path: &Path) -> Result<(), String> { + let contents = + fs::read_to_string(path).map_err(|e| format!("Failed to read story file: {e}"))?; + let updated = set_front_matter_field(&contents, "review_hold", "true"); + fs::write(path, &updated).map_err(|e| format!("Failed to write story file: {e}"))?; + Ok(()) +} + /// Remove a key from the YAML front matter of a story file on disk. /// /// If front matter is present and contains the key, the line is removed. @@ -328,4 +342,29 @@ workflow: tdd let input = " - [ ] Indented item\n"; assert_eq!(parse_unchecked_todos(input), vec!["Indented item"]); } + + #[test] + fn parses_review_hold_from_front_matter() { + let input = "---\nname: Spike\nreview_hold: true\n---\n# Spike\n"; + let meta = parse_front_matter(input).expect("front matter"); + assert_eq!(meta.review_hold, Some(true)); + } + + #[test] + fn review_hold_defaults_to_none() { + let input = "---\nname: Story\n---\n# Story\n"; + let meta = parse_front_matter(input).expect("front matter"); + assert_eq!(meta.review_hold, None); + } + + #[test] + fn write_review_hold_sets_field() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("spike.md"); + std::fs::write(&path, "---\nname: My Spike\n---\n# Spike\n").unwrap(); + write_review_hold(&path).unwrap(); + let contents = std::fs::read_to_string(&path).unwrap(); + assert!(contents.contains("review_hold: true")); + assert!(contents.contains("name: My Spike")); + } }