story-kit: merge 230_story_prevent_duplicate_stage_agents_on_same_story

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.