From f8c13ec2746113f263655e9f9228db458c91b6aa Mon Sep 17 00:00:00 2001 From: Dave Date: Fri, 27 Feb 2026 11:03:26 +0000 Subject: [PATCH] story-kit: merge 230_story_prevent_duplicate_stage_agents_on_same_story --- server/src/agents.rs | 223 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 223 insertions(+) diff --git a/server/src/agents.rs b/server/src/agents.rs index 4a23ccf..6f1b77f 100644 --- a/server/src/agents.rs +++ b/server/src/agents.rs @@ -411,6 +411,42 @@ impl AgentPool { agent.status )); } + // Enforce single-stage concurrency: reject if there is already a + // Running/Pending agent at the same pipeline stage for this story. + // This prevents two coders (or two QA/mergemaster agents) from + // corrupting each other's work in the same worktree. + // Applies to both explicit and auto-selected agents; the Other + // stage (supervisors, unknown agents) is exempt. + let resolved_stage = config + .find_agent(&resolved_name) + .map(agent_config_stage) + .unwrap_or_else(|| pipeline_stage(&resolved_name)); + if resolved_stage != PipelineStage::Other + && let Some(conflicting_name) = agents.iter().find_map(|(k, a)| { + let k_story = k.rsplit_once(':').map(|(s, _)| s).unwrap_or(k); + if k_story == story_id + && a.agent_name != resolved_name + && matches!(a.status, AgentStatus::Running | AgentStatus::Pending) + { + let a_stage = config + .find_agent(&a.agent_name) + .map(agent_config_stage) + .unwrap_or_else(|| pipeline_stage(&a.agent_name)); + if a_stage == resolved_stage { + Some(a.agent_name.clone()) + } else { + None + } + } else { + None + } + }) + { + return Err(format!( + "Cannot start '{resolved_name}' on story '{story_id}': \ + '{conflicting_name}' is already active at the same pipeline stage" + )); + } // Enforce single-instance concurrency for explicitly-named agents: // if this agent is already running on any other story, reject. // Auto-selected agents are already guaranteed idle by @@ -5482,6 +5518,193 @@ stage = "qa" ); } + // ── story-230: prevent duplicate stage agents on same story ─────────────── + + /// start_agent must reject a second coder on a story that already has a + /// Running coder, even if they are *different* agent names. + #[tokio::test] + async fn start_agent_rejects_second_coder_stage_on_same_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).unwrap(); + fs::write( + sk_dir.join("project.toml"), + "[[agent]]\nname = \"coder-1\"\n\n[[agent]]\nname = \"coder-2\"\n", + ) + .unwrap(); + + let pool = AgentPool::new_test(3099); + // coder-1 is already running on the story. + pool.inject_test_agent("42_story_foo", "coder-1", AgentStatus::Running); + + // Attempt to start coder-2 on the *same* story — must be rejected. + let result = pool + .start_agent(root, "42_story_foo", Some("coder-2"), None) + .await; + + assert!(result.is_err(), "second coder on same story must be rejected"); + let err = result.unwrap_err(); + assert!( + err.contains("same pipeline stage"), + "error must mention same pipeline stage, got: '{err}'" + ); + assert!( + err.contains("coder-1") && err.contains("coder-2"), + "error must name both agents, got: '{err}'" + ); + } + + /// The stage-conflict check must also cover QA: a second QA agent on the + /// same story must be rejected. + #[tokio::test] + async fn start_agent_rejects_second_qa_stage_on_same_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).unwrap(); + // Two qa agents using the explicit stage field so name-based detection + // doesn't interfere. + fs::write( + sk_dir.join("project.toml"), + "[[agent]]\nname = \"qa-1\"\nstage = \"qa\"\n\n\ + [[agent]]\nname = \"qa-2\"\nstage = \"qa\"\n", + ) + .unwrap(); + + let pool = AgentPool::new_test(3099); + pool.inject_test_agent("55_story_bar", "qa-1", AgentStatus::Running); + + let result = pool + .start_agent(root, "55_story_bar", Some("qa-2"), None) + .await; + + assert!(result.is_err(), "second qa on same story must be rejected"); + let err = result.unwrap_err(); + assert!( + err.contains("same pipeline stage"), + "error must mention same pipeline stage, got: '{err}'" + ); + } + + /// Regression test (story 230): concurrent start_agent calls with two + /// different coder names on the same story — exactly one must succeed + /// (or fail for infrastructure reasons), and exactly one must be rejected + /// with a stage-conflict error. + /// + /// The story is pre-placed in `2_current/` so that both concurrent + /// `move_story_to_current` calls are no-ops, guaranteeing both reach the + /// lock where the stage-conflict check fires. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn start_agent_concurrent_two_coders_same_story_exactly_one_stage_rejection() { + use std::fs; + use std::sync::Arc; + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().to_path_buf(); + + let sk_dir = root.join(".story_kit"); + // Place story directly in 2_current/ so move_story_to_current is a + // no-op for both concurrent callers, letting both reach the lock. + fs::create_dir_all(sk_dir.join("work/2_current")).unwrap(); + fs::write( + root.join(".story_kit/project.toml"), + "[[agent]]\nname = \"coder-1\"\n\n[[agent]]\nname = \"coder-2\"\n", + ) + .unwrap(); + fs::write( + root.join(".story_kit/work/2_current/42_story_foo.md"), + "---\nname: Foo\n---\n", + ) + .unwrap(); + + let pool = Arc::new(AgentPool::new_test(3099)); + + let pool1 = pool.clone(); + let root1 = root.clone(); + let t1 = tokio::spawn(async move { + pool1 + .start_agent(&root1, "42_story_foo", Some("coder-1"), None) + .await + }); + + let pool2 = pool.clone(); + let root2 = root.clone(); + let t2 = tokio::spawn(async move { + pool2 + .start_agent(&root2, "42_story_foo", Some("coder-2"), None) + .await + }); + + let (r1, r2) = tokio::join!(t1, t2); + let r1 = r1.unwrap(); + let r2 = r2.unwrap(); + + // Exactly one call must be rejected with a stage-conflict error. + let stage_rejections = [&r1, &r2] + .iter() + .filter(|r| { + r.as_ref() + .is_err_and(|e| e.contains("same pipeline stage")) + }) + .count(); + + assert_eq!( + stage_rejections, 1, + "exactly one call must be rejected by the stage-conflict check; \ + got r1={r1:?} r2={r2:?}" + ); + } + + /// Regression test (story 230): two coders on *different* stories must + /// not trigger the stage-conflict guard — the guard is per-story. + #[tokio::test] + async fn start_agent_two_coders_different_stories_not_blocked_by_stage_check() { + 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/1_upcoming")).unwrap(); + fs::write( + root.join(".story_kit/project.toml"), + "[[agent]]\nname = \"coder-1\"\n\n[[agent]]\nname = \"coder-2\"\n", + ) + .unwrap(); + fs::write( + root.join(".story_kit/work/1_upcoming/99_story_baz.md"), + "---\nname: Baz\n---\n", + ) + .unwrap(); + + let pool = AgentPool::new_test(3099); + // coder-1 is running on a *different* story. + pool.inject_test_agent("42_story_foo", "coder-1", AgentStatus::Running); + + // Starting coder-2 on story-99 must NOT be rejected by the stage + // guard (it may fail for infrastructure reasons like missing git repo, + // but not because of the stage-conflict check). + let result = pool + .start_agent(root, "99_story_baz", Some("coder-2"), None) + .await; + + if let Err(ref e) = result { + assert!( + !e.contains("same pipeline stage"), + "stage-conflict guard must not fire for agents on different stories; \ + got: '{e}'" + ); + } + // result may be Ok (unlikely in test env) or Err for infra reasons — both fine. + } + /// Two concurrent auto_assign_available_work calls must not assign the same /// agent to two stories simultaneously. After both complete, at most one /// Pending/Running entry must exist per agent name.