From d27a389a21120ad382cc03d9cc361cb795792ba1 Mon Sep 17 00:00:00 2001 From: Dave Date: Thu, 19 Mar 2026 19:30:41 +0000 Subject: [PATCH] story-kit: merge 312_bug_auto_assign_assigns_mergemaster_to_coding_stage_stories --- server/src/agents/pool.rs | 235 +++++++++++++++++++++++++++++++++++++- 1 file changed, 229 insertions(+), 6 deletions(-) diff --git a/server/src/agents/pool.rs b/server/src/agents/pool.rs index 25e006d..5148d66 100644 --- a/server/src/agents/pool.rs +++ b/server/src/agents/pool.rs @@ -219,6 +219,33 @@ impl AgentPool { // a no-op. super::lifecycle::move_story_to_current(project_root, story_id)?; + // Validate that the agent's configured stage matches the story's + // pipeline stage. This prevents any caller (auto-assign, MCP tool, + // pipeline advance, supervisor) from starting a wrong-stage agent on + // a story — e.g. mergemaster on a coding-stage story (bug 312). + if let Some(name) = agent_name { + let agent_stage = config + .find_agent(name) + .map(agent_config_stage) + .unwrap_or_else(|| pipeline_stage(name)); + if agent_stage != PipelineStage::Other + && let Some(story_stage_dir) = find_active_story_stage(project_root, story_id) + { + let expected_stage = match story_stage_dir { + "2_current" => PipelineStage::Coder, + "3_qa" => PipelineStage::Qa, + "4_merge" => PipelineStage::Mergemaster, + _ => PipelineStage::Other, + }; + if expected_stage != PipelineStage::Other && expected_stage != agent_stage { + return Err(format!( + "Agent '{name}' (stage: {agent_stage:?}) cannot be assigned to \ + story '{story_id}' in {story_stage_dir}/ (requires stage: {expected_stage:?})" + )); + } + } + } + // Atomically resolve agent name, check availability, and register as // Pending. When `agent_name` is `None` the first idle coder is // selected inside the lock so no TOCTOU race can occur between the @@ -4205,10 +4232,14 @@ stage = "coder" let tmp = tempfile::tempdir().unwrap(); let root = tmp.path(); - // Minimal project.toml with a "qa" agent. + // Minimal project.toml with a coder agent (must match 2_current/ stage). let sk_dir = root.join(".story_kit"); fs::create_dir_all(&sk_dir).unwrap(); - fs::write(sk_dir.join("project.toml"), "[[agent]]\nname = \"qa\"\n").unwrap(); + fs::write( + sk_dir.join("project.toml"), + "[[agent]]\nname = \"coder-1\"\nstage = \"coder\"\n", + ) + .unwrap(); // Create the story in upcoming so `move_story_to_current` succeeds, // but do NOT init a git repo — `create_worktree` will fail in the spawn. @@ -4219,7 +4250,7 @@ stage = "coder" let pool = AgentPool::new_test(3099); let result = pool - .start_agent(root, "50_story_test", Some("qa"), None) + .start_agent(root, "50_story_test", Some("coder-1"), None) .await; // With the non-blocking flow, start_agent returns Ok(Pending) immediately. @@ -4238,7 +4269,7 @@ stage = "coder" // Wait for the background task to reach a terminal state. // It must fail (no git repo → create_worktree returns an error). let final_info = pool - .wait_for_agent("50_story_test", "qa", 5000) + .wait_for_agent("50_story_test", "coder-1", 5000) .await .expect("wait_for_agent should not time out"); assert_eq!( @@ -4251,7 +4282,7 @@ stage = "coder" let agents = pool.agents.lock().unwrap(); let failed_entry = agents .values() - .find(|a| a.agent_name == "qa" && a.status == AgentStatus::Failed); + .find(|a| a.agent_name == "coder-1" && a.status == AgentStatus::Failed); assert!( failed_entry.is_some(), "agent pool must retain a Failed entry so the UI can show the error state" @@ -4261,7 +4292,7 @@ stage = "coder" // The AgentEvent::Error must be persisted in the event_log so late // subscribers / polling clients can see the failure reason. let events = pool - .drain_events("50_story_test", "qa") + .drain_events("50_story_test", "coder-1") .expect("drain_events should succeed"); let has_error_event = events.iter().any(|e| matches!(e, AgentEvent::Error { .. })); assert!( @@ -4661,6 +4692,198 @@ stage = "coder" ); } + // ── bug 312: stage-pipeline mismatch guard in start_agent ────────────── + + /// Bug 312: start_agent must reject a mergemaster on a story in 2_current/. + #[tokio::test] + async fn start_agent_rejects_mergemaster_on_coding_stage_story() { + use std::fs; + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let sk_dir = root.join(".story_kit"); + fs::create_dir_all(sk_dir.join("work/2_current")).unwrap(); + fs::write( + sk_dir.join("project.toml"), + "[[agent]]\nname = \"coder-1\"\nstage = \"coder\"\n\n\ + [[agent]]\nname = \"mergemaster\"\nstage = \"mergemaster\"\n", + ) + .unwrap(); + fs::write( + sk_dir.join("work/2_current/310_story_foo.md"), + "---\nname: Foo\n---\n", + ) + .unwrap(); + + let pool = AgentPool::new_test(3099); + let result = pool + .start_agent(root, "310_story_foo", Some("mergemaster"), None) + .await; + + assert!( + result.is_err(), + "mergemaster must not be assigned to a story in 2_current/" + ); + let err = result.unwrap_err(); + assert!( + err.contains("stage") && err.contains("2_current"), + "error must mention stage mismatch, got: '{err}'" + ); + } + + /// Bug 312: start_agent must reject a coder on a story in 3_qa/. + #[tokio::test] + async fn start_agent_rejects_coder_on_qa_stage_story() { + use std::fs; + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let sk_dir = root.join(".story_kit"); + fs::create_dir_all(sk_dir.join("work/3_qa")).unwrap(); + fs::write( + sk_dir.join("project.toml"), + "[[agent]]\nname = \"coder-1\"\nstage = \"coder\"\n\n\ + [[agent]]\nname = \"qa\"\nstage = \"qa\"\n", + ) + .unwrap(); + fs::write( + sk_dir.join("work/3_qa/42_story_bar.md"), + "---\nname: Bar\n---\n", + ) + .unwrap(); + + let pool = AgentPool::new_test(3099); + let result = pool + .start_agent(root, "42_story_bar", Some("coder-1"), None) + .await; + + assert!( + result.is_err(), + "coder must not be assigned to a story in 3_qa/" + ); + let err = result.unwrap_err(); + assert!( + err.contains("stage") && err.contains("3_qa"), + "error must mention stage mismatch, got: '{err}'" + ); + } + + /// Bug 312: start_agent must reject a QA agent on a story in 4_merge/. + #[tokio::test] + async fn start_agent_rejects_qa_on_merge_stage_story() { + use std::fs; + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let sk_dir = root.join(".story_kit"); + fs::create_dir_all(sk_dir.join("work/4_merge")).unwrap(); + fs::write( + sk_dir.join("project.toml"), + "[[agent]]\nname = \"qa\"\nstage = \"qa\"\n\n\ + [[agent]]\nname = \"mergemaster\"\nstage = \"mergemaster\"\n", + ) + .unwrap(); + fs::write( + sk_dir.join("work/4_merge/55_story_baz.md"), + "---\nname: Baz\n---\n", + ) + .unwrap(); + + let pool = AgentPool::new_test(3099); + let result = pool + .start_agent(root, "55_story_baz", Some("qa"), None) + .await; + + assert!( + result.is_err(), + "qa must not be assigned to a story in 4_merge/" + ); + let err = result.unwrap_err(); + assert!( + err.contains("stage") && err.contains("4_merge"), + "error must mention stage mismatch, got: '{err}'" + ); + } + + /// Bug 312: supervisor (stage=other) should be allowed on any pipeline stage. + #[tokio::test] + async fn start_agent_allows_supervisor_on_any_stage() { + use std::fs; + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let sk_dir = root.join(".story_kit"); + fs::create_dir_all(sk_dir.join("work/2_current")).unwrap(); + fs::write( + sk_dir.join("project.toml"), + "[[agent]]\nname = \"supervisor\"\nstage = \"other\"\n", + ) + .unwrap(); + fs::write( + sk_dir.join("work/2_current/77_story_sup.md"), + "---\nname: Sup\n---\n", + ) + .unwrap(); + + let pool = AgentPool::new_test(3099); + // start_agent will fail for git/worktree reasons, but NOT for stage + // mismatch. We just need to verify it doesn't fail with a stage error. + let result = pool + .start_agent(root, "77_story_sup", Some("supervisor"), None) + .await; + + match result { + Ok(_) => {} // Fine — no stage error. + Err(e) => { + assert!( + !e.contains("stage:") || !e.contains("cannot be assigned"), + "supervisor should not be rejected for stage mismatch, got: '{e}'" + ); + } + } + } + + /// Bug 312: correct stage agent should still be allowed. + #[tokio::test] + async fn start_agent_allows_correct_stage_agent() { + use std::fs; + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let sk_dir = root.join(".story_kit"); + fs::create_dir_all(sk_dir.join("work/4_merge")).unwrap(); + fs::write( + sk_dir.join("project.toml"), + "[[agent]]\nname = \"mergemaster\"\nstage = \"mergemaster\"\n", + ) + .unwrap(); + fs::write( + sk_dir.join("work/4_merge/88_story_ok.md"), + "---\nname: OK\n---\n", + ) + .unwrap(); + + let pool = AgentPool::new_test(3099); + let result = pool + .start_agent(root, "88_story_ok", Some("mergemaster"), None) + .await; + + match result { + Ok(_) => {} // Fine — correct stage. + Err(e) => { + assert!( + !e.contains("cannot be assigned"), + "mergemaster on 4_merge/ story should not fail stage check, got: '{e}'" + ); + } + } + } + // ── merge_agent_work tests ──────────────────────────────────────────────── /// Helper: start a merge and poll until terminal state.