diff --git a/server/src/chat/transport/matrix/assign.rs b/server/src/chat/transport/matrix/assign.rs index 8fc9b561..320bdd61 100644 --- a/server/src/chat/transport/matrix/assign.rs +++ b/server/src/chat/transport/matrix/assign.rs @@ -1,16 +1,17 @@ //! Assign command: pre-assign or re-assign a coder model to a story. //! -//! `{bot_name} assign {number} {model}` finds the story by number, updates the -//! `agent` field in its front matter, and — when a coder is already running on -//! the story — stops the current coder and starts the newly-assigned one. +//! `{bot_name} assign {number} {model}` finds the story by number, writes the +//! agent name into the typed CRDT `agent` register, and — when a coder is +//! already running on the story — stops it and starts the newly-assigned one +//! via [`crate::service::work_item::assign_and_start`]. //! //! When no coder is running (the story has not been started yet), the command -//! behaves as before: it simply persists the assignment in the front matter so -//! that the next `start` invocation picks it up automatically. +//! persists the assignment in the CRDT register so the next `start` invocation +//! picks it up automatically. use crate::agents::{AgentPool, AgentStatus}; use crate::chat::util::strip_bot_mention; -use crate::io::story_metadata::{parse_front_matter, set_front_matter_field}; +use crate::io::story_metadata::parse_front_matter; use std::path::Path; /// A parsed assign command from a Matrix message body. @@ -77,10 +78,14 @@ pub fn resolve_agent_name(model: &str) -> String { /// Handle an assign command asynchronously. /// -/// Finds the work item by `story_number` across all pipeline stages, updates -/// the `agent` field in its front matter, and — if a coder is currently -/// running on the story — stops it and starts the newly-assigned agent. -/// Returns a markdown-formatted response string. +/// Finds the work item by `story_number` across all pipeline stages, writes +/// the agent pin to the CRDT register, and — if a coder is currently running +/// on the story — stops it and starts the newly-assigned agent via +/// [`crate::service::work_item::assign_and_start`]. +/// +/// When no coder is running the assignment is persisted in the CRDT so the +/// next `start` invocation picks it up automatically. Returns a +/// markdown-formatted response string. pub async fn handle_assign( bot_name: &str, story_number: &str, @@ -88,7 +93,7 @@ pub async fn handle_assign( project_root: &Path, agents: &AgentPool, ) -> String { - // Find the story by numeric prefix: CRDT → content store → filesystem. + // Parse: find the story by numeric prefix (CRDT → content store → filesystem). let (story_id, _stage_dir, _path, content) = match crate::chat::lookup::find_story_by_number(project_root, story_number) { Some(found) => found, @@ -97,33 +102,13 @@ pub async fn handle_assign( } }; - let current_content = content.or_else(|| crate::db::read_content(&story_id)); - - let story_name = current_content - .as_ref() - .and_then(|c| parse_front_matter(c).ok().and_then(|m| m.name)) + let story_name = content + .or_else(|| crate::db::read_content(&story_id)) + .and_then(|c| parse_front_matter(&c).ok().and_then(|m| m.name)) .unwrap_or_else(|| story_id.clone()); let agent_name = resolve_agent_name(model_str); - // Write `agent: ` into the story's front matter via content store. - let write_result = match current_content { - Some(contents) => { - let updated = set_front_matter_field(&contents, "agent", &agent_name); - crate::db::write_item_with_content(&story_id, &_stage_dir, &updated); - Ok(()) - } - None => Err(format!("Story content not found for {story_id}")), - }; - - if let Err(e) = write_result { - return format!("Failed to assign model to **{story_name}**: {e}"); - } - - // Mirror the assignment into the CRDT register so the in-memory pipeline - // state stays consistent with the front-matter. - crate::crdt_state::set_agent(&story_id, Some(&agent_name)); - // Check whether a coder is already running on this story. let running_coders: Vec<_> = agents .list_agents() @@ -137,14 +122,15 @@ pub async fn handle_assign( .collect(); if running_coders.is_empty() { - // No coder running — just persist the assignment. + // No coder running — persist the CRDT agent pin for the future start. + crate::crdt_state::set_agent(&story_id, Some(&agent_name)); return format!( "Assigned **{agent_name}** to **{story_name}** (story {story_number}). \ The model will be used when the story starts." ); } - // Stop each running coder, then start the newly assigned one. + // Stop each running coder, then assign+start the newly-assigned one. let stopped: Vec = running_coders .iter() .map(|a| a.agent_name.clone()) @@ -169,8 +155,8 @@ pub async fn handle_assign( story_id ); - match agents - .start_agent(project_root, &story_id, Some(&agent_name), None, None) + // Service call: persist CRDT agent pin and start the new agent. + match crate::service::work_item::assign_and_start(&story_id, &agent_name, project_root, agents) .await { Ok(info) => { @@ -317,7 +303,8 @@ mod tests { } #[tokio::test] - async fn handle_assign_writes_front_matter_when_no_coder_running() { + async fn handle_assign_sets_crdt_agent_when_no_coder_running() { + crate::crdt_state::init_for_test(); let tmp = tempfile::tempdir().unwrap(); write_story_file( tmp.path(), @@ -325,6 +312,19 @@ mod tests { "9972_story_test.md", "---\nname: Test Feature\n---\n\n# Story 9972\n", ); + // Seed CRDT so set_agent can write to the item. + crate::crdt_state::write_item( + "9972_story_test", + "1_backlog", + Some("Test Feature"), + None, + None, + None, + None, + None, + None, + None, + ); let agents = std::sync::Arc::new(AgentPool::new_test(3000)); let response = handle_assign("Timmy", "9972", "opus", tmp.path(), &agents).await; @@ -343,16 +343,24 @@ mod tests { "response should indicate assignment for future start: {response}" ); - let contents = crate::db::read_content("9972_story_test") - .expect("content store should have updated content"); - assert!( - contents.contains("agent: coder-opus"), - "front matter should contain agent field: {contents}" + // CRDT register should be set (no longer checks YAML front matter). + let dump = crate::crdt_state::dump_crdt_state(Some("9972_story_test")); + let item = dump + .items + .iter() + .find(|i| i.story_id.as_deref() == Some("9972_story_test")) + .expect("item must be in CRDT"); + assert_eq!( + item.agent.as_deref(), + Some("coder-opus"), + "CRDT agent register should be set: {:?}", + item.agent ); } #[tokio::test] async fn handle_assign_with_already_prefixed_name_does_not_double_prefix() { + crate::crdt_state::init_for_test(); let tmp = tempfile::tempdir().unwrap(); write_story_file( tmp.path(), @@ -360,6 +368,18 @@ mod tests { "9973_story_small.md", "---\nname: Small Story\n---\n", ); + crate::crdt_state::write_item( + "9973_story_small", + "1_backlog", + Some("Small Story"), + None, + None, + None, + None, + None, + None, + None, + ); let agents = std::sync::Arc::new(AgentPool::new_test(3000)); let response = handle_assign("Timmy", "9973", "coder-opus", tmp.path(), &agents).await; @@ -373,16 +393,24 @@ mod tests { "must not double-prefix: {response}" ); - let contents = crate::db::read_content("9973_story_small") - .expect("content store should have updated content"); - assert!( - contents.contains("agent: coder-opus"), - "must write coder-opus, not coder-coder-opus: {contents}" + // CRDT must have coder-opus, not coder-coder-opus. + let dump = crate::crdt_state::dump_crdt_state(Some("9973_story_small")); + let item = dump + .items + .iter() + .find(|i| i.story_id.as_deref() == Some("9973_story_small")) + .expect("item must be in CRDT"); + assert_eq!( + item.agent.as_deref(), + Some("coder-opus"), + "must write coder-opus, not coder-coder-opus: {:?}", + item.agent ); } #[tokio::test] async fn handle_assign_overwrites_existing_agent_field() { + crate::crdt_state::init_for_test(); let tmp = tempfile::tempdir().unwrap(); write_story_file( tmp.path(), @@ -390,19 +418,34 @@ mod tests { "9974_story_existing.md", "---\nname: Existing\nagent: coder-sonnet\n---\n", ); + crate::crdt_state::write_item( + "9974_story_existing", + "1_backlog", + Some("Existing"), + Some("coder-sonnet"), + None, + None, + None, + None, + None, + None, + ); let agents = std::sync::Arc::new(AgentPool::new_test(3000)); handle_assign("Timmy", "9974", "opus", tmp.path(), &agents).await; - let contents = crate::db::read_content("9974_story_existing") - .expect("content store should have updated content"); - assert!( - contents.contains("agent: coder-opus"), - "should overwrite old agent: {contents}" - ); - assert!( - !contents.contains("coder-sonnet"), - "old agent should no longer appear: {contents}" + // CRDT agent register must be updated to the new value. + let dump = crate::crdt_state::dump_crdt_state(Some("9974_story_existing")); + let item = dump + .items + .iter() + .find(|i| i.story_id.as_deref() == Some("9974_story_existing")) + .expect("item must be in CRDT"); + assert_eq!( + item.agent.as_deref(), + Some("coder-opus"), + "CRDT agent must be updated to coder-opus: {:?}", + item.agent ); } diff --git a/server/src/http/mcp/agent_tools/lifecycle.rs b/server/src/http/mcp/agent_tools/lifecycle.rs index edef0f3c..2e220b8f 100644 --- a/server/src/http/mcp/agent_tools/lifecycle.rs +++ b/server/src/http/mcp/agent_tools/lifecycle.rs @@ -15,11 +15,22 @@ pub(crate) async fn tool_start_agent(args: &Value, ctx: &AppContext) -> Result Result { + crate::crdt_state::set_agent(story_id, Some(agent_name)); + agents + .start_agent(project_root, story_id, Some(agent_name), None, None) + .await +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + use crate::agents::AgentPool; + use std::sync::Arc; + + fn make_agents() -> Arc { + Arc::new(AgentPool::new_test(3000)) + } + + /// Regression test (story 877, AC4): assigning via the Matrix chat path + /// and via the MCP `start_agent` path both call `assign_and_start`, which + /// writes the typed CRDT `agent` register before spawning. This test + /// verifies the CRDT write is unconditional even when the pool call fails + /// (no git repo in the test environment). + #[tokio::test] + async fn assign_and_start_writes_crdt_agent_register_before_pool_call() { + crate::crdt_state::init_for_test(); + let story_id = "8770_story_assign_regression_crdt"; + + // Seed the CRDT so set_agent can find the item. + crate::crdt_state::write_item( + story_id, + "2_current", + Some("Assign Regression"), + None, + None, + None, + None, + None, + None, + None, + ); + + let tmp = tempfile::tempdir().unwrap(); + let agents = make_agents(); + + // The pool call will fail (no git repo / project config), but the CRDT + // write must succeed before it is attempted. + let _ = assign_and_start(story_id, "coder-opus", tmp.path(), &agents).await; + + let dump = crate::crdt_state::dump_crdt_state(Some(story_id)); + let item = dump + .items + .iter() + .find(|i| i.story_id.as_deref() == Some(story_id)) + .expect("item must be in CRDT"); + assert_eq!( + item.agent.as_deref(), + Some("coder-opus"), + "CRDT agent register must be set to coder-opus after assign_and_start" + ); + } + + /// Both paths (Matrix and MCP) that call `assign_and_start` leave the same + /// CRDT register value, regardless of which path is used. + #[tokio::test] + async fn assign_and_start_same_crdt_state_from_both_paths() { + crate::crdt_state::init_for_test(); + let story_id_a = "8771_story_assign_path_a"; + let story_id_b = "8772_story_assign_path_b"; + + for sid in &[story_id_a, story_id_b] { + crate::crdt_state::write_item( + sid, + "2_current", + Some("Path Test"), + None, + None, + None, + None, + None, + None, + None, + ); + } + + let tmp = tempfile::tempdir().unwrap(); + let agents = make_agents(); + + // Simulate Matrix path: call assign_and_start directly (as handle_assign does). + let _ = assign_and_start(story_id_a, "coder-sonnet", tmp.path(), &agents).await; + + // Simulate MCP path: call assign_and_start directly (as tool_start_agent does). + let _ = assign_and_start(story_id_b, "coder-sonnet", tmp.path(), &agents).await; + + // Both must leave the same CRDT agent register value. + for sid in &[story_id_a, story_id_b] { + let dump = crate::crdt_state::dump_crdt_state(Some(sid)); + let item = dump + .items + .iter() + .find(|i| i.story_id.as_deref() == Some(*sid)) + .expect("item must be in CRDT"); + assert_eq!( + item.agent.as_deref(), + Some("coder-sonnet"), + "CRDT agent register must match for story {sid}" + ); + } + } +} diff --git a/server/src/service/work_item/mod.rs b/server/src/service/work_item/mod.rs index 63bb5266..9bbcbbcf 100644 --- a/server/src/service/work_item/mod.rs +++ b/server/src/service/work_item/mod.rs @@ -1,5 +1,9 @@ //! Work-item service — cross-cutting domain logic that applies to all pipeline //! work-item types (stories, bugs, spikes, refactors). +/// Assign-and-start: unified assign + spawn for both chat and MCP paths. +pub mod assign; /// Canonical delete sequence for any work item type. pub mod delete; + +pub use assign::assign_and_start;