From 184c214c345fa41edd976173a0ec8205f568542d Mon Sep 17 00:00:00 2001 From: dave Date: Wed, 13 May 2026 11:58:50 +0000 Subject: [PATCH] huskies: merge 962 --- .../agents/pool/auto_assign/story_checks.rs | 4 +- server/src/agents/pool/start/mod.rs | 9 +- server/src/agents/pool/start/validation.rs | 9 +- server/src/chat/commands/triage.rs | 5 +- server/src/chat/transport/matrix/assign.rs | 5 +- server/src/config/agent_name.rs | 125 ++++++++++++++++++ server/src/config/mod.rs | 16 +++ server/src/crdt_state/read.rs | 4 +- server/src/crdt_state/types.rs | 10 +- server/src/crdt_state/write/item.rs | 4 +- server/src/crdt_state/write/tests.rs | 20 ++- server/src/db/mod.rs | 2 +- server/src/db/ops.rs | 2 +- server/src/http/mcp/qa_tools.rs | 5 +- server/src/http/mcp/status_tools.rs | 5 +- .../src/http/mcp/story_tools/story/update.rs | 8 +- server/src/pipeline_state/projection.rs | 2 +- server/src/service/agents/mod.rs | 2 +- server/src/service/work_item/assign.rs | 11 +- 19 files changed, 204 insertions(+), 44 deletions(-) create mode 100644 server/src/config/agent_name.rs diff --git a/server/src/agents/pool/auto_assign/story_checks.rs b/server/src/agents/pool/auto_assign/story_checks.rs index d7a71344..f34eed3d 100644 --- a/server/src/agents/pool/auto_assign/story_checks.rs +++ b/server/src/agents/pool/auto_assign/story_checks.rs @@ -16,9 +16,7 @@ pub(super) fn read_story_front_matter_agent( // Story 929: agent name comes from the CRDT register. The previous // YAML fallback is gone — post-891 every story has its CRDT entry, // and any story without one is treated as having no pinned agent. - crate::crdt_state::read_item(story_id) - .and_then(|w| w.agent().map(str::to_string)) - .filter(|s| !s.is_empty()) + crate::crdt_state::read_item(story_id).and_then(|w| w.agent().map(|a| a.to_string())) } /// Return `true` if the story is in `Stage::ReviewHold`. diff --git a/server/src/agents/pool/start/mod.rs b/server/src/agents/pool/start/mod.rs index e4f03dc4..b793c067 100644 --- a/server/src/agents/pool/start/mod.rs +++ b/server/src/agents/pool/start/mod.rs @@ -130,7 +130,8 @@ impl AgentPool { // Read the preferred agent from the story's front matter before acquiring // the lock. (See validation::read_front_matter_agent.) - let front_matter_agent: Option = read_front_matter_agent(story_id, agent_name); + let front_matter_agent: Option = + read_front_matter_agent(story_id, agent_name); // Atomically resolve agent name, check availability, and register as // Pending. When `agent_name` is `None` the first idle coder is @@ -158,12 +159,12 @@ impl AgentPool { // (bug 379). Mirrors the auto_assign selection logic. if let Some(ref pref) = front_matter_agent { let stage_matches = config - .find_agent(pref) + .find_agent(pref.as_str()) .map(|cfg| agent_config_stage(cfg) == PipelineStage::Coder) .unwrap_or(false); if stage_matches { - if auto_assign::is_agent_free(&agents, pref) { - pref.clone() + if auto_assign::is_agent_free(&agents, pref.as_str()) { + pref.to_string() } else { return Err(format!( "Preferred agent '{pref}' from story front matter is busy; \ diff --git a/server/src/agents/pool/start/validation.rs b/server/src/agents/pool/start/validation.rs index b712161d..46333f5b 100644 --- a/server/src/agents/pool/start/validation.rs +++ b/server/src/agents/pool/start/validation.rs @@ -56,14 +56,15 @@ pub(super) fn validate_agent_stage( /// `start_agent` honour an explicit `agent: coder-opus` written by the /// `assign` command (bug 379). Returns `None` when an explicit agent_name /// was already supplied or when the story has no front-matter preference. -pub(super) fn read_front_matter_agent(story_id: &str, agent_name: Option<&str>) -> Option { +pub(super) fn read_front_matter_agent( + story_id: &str, + agent_name: Option<&str>, +) -> Option { if agent_name.is_some() { return None; } // Story 929: the agent pin lives in the CRDT typed register; the // legacy YAML fallback is gone — post-891 every story has its CRDT // entry and any story without one has no pinned agent. - crate::crdt_state::read_item(story_id) - .and_then(|w| w.agent().map(str::to_string)) - .filter(|s| !s.is_empty()) + crate::crdt_state::read_item(story_id).and_then(|w| w.agent()) } diff --git a/server/src/chat/commands/triage.rs b/server/src/chat/commands/triage.rs index f54263ba..903cdb0e 100644 --- a/server/src/chat/commands/triage.rs +++ b/server/src/chat/commands/triage.rs @@ -419,7 +419,10 @@ mod tests { "", Some("Agent Story"), ); - crate::crdt_state::set_agent("55_story_agent_story", Some("coder-1")); + crate::crdt_state::set_agent( + "55_story_agent_story", + Some(crate::config::AgentName::Coder1), + ); let output = status_triage_cmd(tmp.path(), "55").unwrap(); assert!( output.contains("coder-1"), diff --git a/server/src/chat/transport/matrix/assign.rs b/server/src/chat/transport/matrix/assign.rs index 4b69ad49..78bd635c 100644 --- a/server/src/chat/transport/matrix/assign.rs +++ b/server/src/chat/transport/matrix/assign.rs @@ -122,7 +122,10 @@ pub async fn handle_assign( if running_coders.is_empty() { // No coder running — persist the CRDT agent pin for the future start. - crate::crdt_state::set_agent(&story_id, Some(&agent_name)); + crate::crdt_state::set_agent( + &story_id, + agent_name.parse::().ok(), + ); return format!( "Assigned **{agent_name}** to **{story_name}** (story {story_number}). \ The model will be used when the story starts." diff --git a/server/src/config/agent_name.rs b/server/src/config/agent_name.rs new file mode 100644 index 00000000..bba4f770 --- /dev/null +++ b/server/src/config/agent_name.rs @@ -0,0 +1,125 @@ +//! Typed agent-name enum mirroring the `agents.toml` roster. +//! +//! `AgentName` is the single source of truth for valid agent identifiers. +//! Adding a new agent to `agents.toml` without a corresponding variant here +//! causes server startup to fail — `validate_agents` calls `AgentName::from_str` +//! for every configured agent name and returns an error for any unknown name. + +use std::fmt; +use std::str::FromStr; + +/// The set of named agents defined in `agents.toml`. +/// +/// Variants mirror the `name` field of each `[[agent]]` entry. Serialises to +/// the canonical dash-form string (e.g. `"coder-1"`). +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +pub enum AgentName { + /// `name = "coder-1"` + Coder1, + /// `name = "coder-2"` + Coder2, + /// `name = "coder-3"` + Coder3, + /// `name = "coder-opus"` + CoderOpus, + /// `name = "qa"` + Qa, + /// `name = "qa-2"` + Qa2, + /// `name = "mergemaster"` + Mergemaster, +} + +impl AgentName { + /// The canonical string form, matching the `name` field in `agents.toml`. + pub fn as_str(self) -> &'static str { + match self { + Self::Coder1 => "coder-1", + Self::Coder2 => "coder-2", + Self::Coder3 => "coder-3", + Self::CoderOpus => "coder-opus", + Self::Qa => "qa", + Self::Qa2 => "qa-2", + Self::Mergemaster => "mergemaster", + } + } + + /// All known variants, in agents.toml declaration order. + pub fn all() -> &'static [AgentName] { + &[ + Self::Coder1, + Self::Coder2, + Self::Coder3, + Self::CoderOpus, + Self::Qa, + Self::Qa2, + Self::Mergemaster, + ] + } +} + +impl fmt::Display for AgentName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.as_str()) + } +} + +impl FromStr for AgentName { + type Err = String; + + fn from_str(s: &str) -> Result { + match s { + "coder-1" => Ok(Self::Coder1), + "coder-2" => Ok(Self::Coder2), + "coder-3" => Ok(Self::Coder3), + "coder-opus" => Ok(Self::CoderOpus), + "qa" => Ok(Self::Qa), + "qa-2" => Ok(Self::Qa2), + "mergemaster" => Ok(Self::Mergemaster), + other => Err(format!( + "unknown agent name '{other}'; add a variant to AgentName or update agents.toml" + )), + } + } +} + +impl serde::Serialize for AgentName { + fn serialize(&self, s: S) -> Result { + s.serialize_str(self.as_str()) + } +} + +impl<'de> serde::Deserialize<'de> for AgentName { + fn deserialize>(d: D) -> Result { + let s = String::deserialize(d)?; + Self::from_str(&s).map_err(serde::de::Error::custom) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn round_trip_all_variants() { + for &name in AgentName::all() { + let s = name.as_str(); + let parsed: AgentName = s.parse().expect("parse should succeed"); + assert_eq!(parsed, name); + assert_eq!(parsed.to_string(), s); + } + } + + #[test] + fn unknown_name_returns_err() { + assert!("unknown-agent".parse::().is_err()); + assert!("".parse::().is_err()); + } + + #[test] + fn display_matches_as_str() { + assert_eq!(AgentName::Coder1.to_string(), "coder-1"); + assert_eq!(AgentName::CoderOpus.to_string(), "coder-opus"); + assert_eq!(AgentName::Mergemaster.to_string(), "mergemaster"); + } +} diff --git a/server/src/config/mod.rs b/server/src/config/mod.rs index 6f2d582c..24a5ae2e 100644 --- a/server/src/config/mod.rs +++ b/server/src/config/mod.rs @@ -1,4 +1,9 @@ //! Project configuration — parses `project.toml` for agents, components, and server settings. + +/// Typed agent name (mirrors the agents.toml roster). +pub mod agent_name; +pub use agent_name::AgentName; + use crate::slog; use serde::Deserialize; use std::collections::HashSet; @@ -646,6 +651,17 @@ fn validate_agents(agents: &[AgentConfig]) -> Result<(), String> { } } } + // Skip the AgentName roster check in test mode — unit tests use + // synthetic agent names (e.g. "first", "second") that are not in the + // production enum. The check is meaningful only at server startup. + #[cfg(not(test))] + agent.name.parse::().map_err(|_| { + format!( + "Agent '{}': name is not in the AgentName roster; \ + add a variant to `config::AgentName` or remove this agent from agents.toml", + agent.name + ) + })?; } Ok(()) } diff --git a/server/src/crdt_state/read.rs b/server/src/crdt_state/read.rs index 1ba4394f..0de1b020 100644 --- a/server/src/crdt_state/read.rs +++ b/server/src/crdt_state/read.rs @@ -312,7 +312,7 @@ pub(super) fn extract_item_view(item: &PipelineItemCrdt) -> Option return None, }; let agent = match item.agent.view() { - JsonValue::String(s) if !s.is_empty() => Some(s), + JsonValue::String(s) if !s.is_empty() => s.parse::().ok(), _ => None, }; let retry_count = match item.retry_count.view() { @@ -584,7 +584,7 @@ mod tests { assert_eq!(view.story_id, "40_story_view"); assert!(matches!(view.stage, crate::pipeline_state::Stage::Qa)); assert_eq!(view.name, "View Test"); - assert_eq!(view.agent.as_deref(), Some("coder-1")); + assert_eq!(view.agent.map(|a| a.as_str()), Some("coder-1")); assert_eq!(view.retry_count, 2u32); assert_eq!(view.depends_on, vec![10u32, 20u32]); } diff --git a/server/src/crdt_state/types.rs b/server/src/crdt_state/types.rs index 4d97f6d3..445e7e52 100644 --- a/server/src/crdt_state/types.rs +++ b/server/src/crdt_state/types.rs @@ -175,7 +175,7 @@ pub struct WorkItem { /// out of all read paths) when this register is empty — a nameless item is /// treated as malformed, not surfaced with an empty string. pub(super) name: String, - pub(super) agent: Option, + pub(super) agent: Option, /// Retry counter — `0` when the CRDT register is unset. pub(super) retry_count: u32, /// Dependency story numbers — empty `Vec` when the register is unset. @@ -210,9 +210,9 @@ impl WorkItem { &self.name } - /// Agent name pinned to this item, or `None` when unset. - pub fn agent(&self) -> Option<&str> { - self.agent.as_deref() + /// Typed agent pinned to this item, or `None` when unset. + pub fn agent(&self) -> Option { + self.agent } /// Retry counter. Returns `0` when the register is unset. @@ -255,7 +255,7 @@ impl WorkItem { story_id: impl Into, stage: crate::pipeline_state::Stage, name: impl Into, - agent: Option, + agent: Option, retry_count: u32, depends_on: Vec, claim: Option, diff --git a/server/src/crdt_state/write/item.rs b/server/src/crdt_state/write/item.rs index 25463b0a..b7ca8235 100644 --- a/server/src/crdt_state/write/item.rs +++ b/server/src/crdt_state/write/item.rs @@ -147,7 +147,7 @@ pub fn set_name(story_id: &str, name: Option<&str>) -> bool { /// other fields to be known. /// /// Returns `true` if the item was found and the write was performed. -pub fn set_agent(story_id: &str, agent: Option<&str>) -> bool { +pub fn set_agent(story_id: &str, agent: Option) -> bool { let Some(state_mutex) = get_crdt() else { return false; }; @@ -157,7 +157,7 @@ pub fn set_agent(story_id: &str, agent: Option<&str>) -> bool { let Some(&idx) = state.index.get(story_id) else { return false; }; - let value = agent.unwrap_or("").to_string(); + let value = agent.map(|a| a.as_str().to_string()).unwrap_or_default(); apply_and_persist(&mut state, |s| { s.crdt.doc.items[idx].agent.set(value.clone()) }); diff --git a/server/src/crdt_state/write/tests.rs b/server/src/crdt_state/write/tests.rs index 50bddc65..768341e2 100644 --- a/server/src/crdt_state/write/tests.rs +++ b/server/src/crdt_state/write/tests.rs @@ -213,7 +213,7 @@ fn migrate_story_ids_to_numeric_preserves_stage_and_name() { let item = read_item("45").expect("item must be accessible by numeric ID"); assert!(matches!(item.stage, crate::pipeline_state::Stage::Coding)); assert_eq!(item.name, "Crash Bug"); - assert_eq!(item.agent.as_deref(), Some("coder-1")); + assert_eq!(item.agent.map(|a| a.as_str()), Some("coder-1")); } #[test] @@ -356,12 +356,15 @@ fn set_agent_some_writes_name() { None, ); - let found = set_agent("871_story_set_agent_write", Some("coder-1")); + let found = set_agent( + "871_story_set_agent_write", + Some(crate::config::AgentName::Coder1), + ); assert!(found, "set_agent should return true for an existing item"); let item = read_item("871_story_set_agent_write").expect("item must exist"); assert_eq!( - item.agent.as_deref(), + item.agent.map(|a| a.as_str()), Some("coder-1"), "agent should be written to CRDT register" ); @@ -385,7 +388,7 @@ fn set_agent_none_clears_register() { // Confirm agent is set. let before = read_item("871_story_set_agent_clear").expect("item must exist"); - assert_eq!(before.agent.as_deref(), Some("coder-2")); + assert_eq!(before.agent.map(|a| a.as_str()), Some("coder-2")); // Clear it. let found = set_agent("871_story_set_agent_clear", None); @@ -393,8 +396,8 @@ fn set_agent_none_clears_register() { let after = read_item("871_story_set_agent_clear").expect("item must exist"); assert!( - after.agent.as_deref().unwrap_or("").is_empty(), - "agent should be cleared (empty string) after set_agent(None)" + after.agent.is_none(), + "agent should be cleared after set_agent(None)" ); } @@ -402,7 +405,10 @@ fn set_agent_none_clears_register() { fn set_agent_returns_false_for_unknown_story() { init_for_test(); - let found = set_agent("999_story_nonexistent", Some("coder-1")); + let found = set_agent( + "999_story_nonexistent", + Some(crate::config::AgentName::Coder1), + ); assert!( !found, "set_agent should return false when story is not in the CRDT" diff --git a/server/src/db/mod.rs b/server/src/db/mod.rs index 320d3a63..359e79c3 100644 --- a/server/src/db/mod.rs +++ b/server/src/db/mod.rs @@ -328,7 +328,7 @@ mod tests { let view = crate::crdt_state::read_item(story_id).expect("story exists in CRDT"); assert_eq!(view.stage().dir_name(), "coding"); assert_eq!(view.name(), "Typed Name"); - assert_eq!(view.agent(), Some("coder-1")); + assert_eq!(view.agent(), Some(crate::config::AgentName::Coder1)); assert_eq!(view.retry_count(), 2); assert_eq!(view.depends_on(), &[100, 200]); diff --git a/server/src/db/ops.rs b/server/src/db/ops.rs index d8b8c70a..ec2290ce 100644 --- a/server/src/db/ops.rs +++ b/server/src/db/ops.rs @@ -171,7 +171,7 @@ pub fn move_item_stage( if let Some(db) = PIPELINE_DB.get() { let view = crate::crdt_state::read_item(story_id); let name = view.as_ref().map(|v| v.name().to_string()); - let agent = view.as_ref().and_then(|v| v.agent().map(str::to_string)); + let agent = view.as_ref().and_then(|v| v.agent().map(|a| a.to_string())); let depends_on = view .as_ref() .map(|v| v.depends_on()) diff --git a/server/src/http/mcp/qa_tools.rs b/server/src/http/mcp/qa_tools.rs index 56a4abd2..920266f2 100644 --- a/server/src/http/mcp/qa_tools.rs +++ b/server/src/http/mcp/qa_tools.rs @@ -141,9 +141,8 @@ pub(super) async fn tool_reject_qa(args: &Value, ctx: &AppContext) -> Result Result().ok()); } if let Some(epic) = args.get("epic").and_then(|v| v.as_str()) { crate::crdt_state::set_epic(story_id, crate::crdt_state::EpicId::from_crdt_str(epic)); @@ -42,8 +42,10 @@ pub(crate) fn tool_update_story(args: &Value, ctx: &AppContext) -> Result { - let s = value.as_str().filter(|s| !s.is_empty()); - crate::crdt_state::set_agent(story_id, s); + let parsed = value + .as_str() + .and_then(|s| s.parse::().ok()); + crate::crdt_state::set_agent(story_id, parsed); } "qa" => { let mode = value diff --git a/server/src/pipeline_state/projection.rs b/server/src/pipeline_state/projection.rs index dd274cd8..5264d5f8 100644 --- a/server/src/pipeline_state/projection.rs +++ b/server/src/pipeline_state/projection.rs @@ -161,7 +161,7 @@ mod tests { "42_story_test", Stage::Coding, "Test", - Some("coder-1".to_string()), + Some(crate::config::AgentName::Coder1), 2u32, vec![], None, diff --git a/server/src/service/agents/mod.rs b/server/src/service/agents/mod.rs index 2ea839ed..21089713 100644 --- a/server/src/service/agents/mod.rs +++ b/server/src/service/agents/mod.rs @@ -165,7 +165,7 @@ pub fn get_work_item_content( let crdt_name = crdt_view.as_ref().map(|v| v.name().to_string()); let crdt_agent = crdt_view .as_ref() - .and_then(|v| v.agent().map(str::to_string)); + .and_then(|v| v.agent().map(|a| a.to_string())); for (stage_dir, stage) in &stages { if let Some(content) = io::read_work_item_from_stage(&work_dir, stage_dir, &filename)? { diff --git a/server/src/service/work_item/assign.rs b/server/src/service/work_item/assign.rs index be1be4e5..e60980ad 100644 --- a/server/src/service/work_item/assign.rs +++ b/server/src/service/work_item/assign.rs @@ -25,7 +25,10 @@ pub async fn assign_and_start( project_root: &Path, agents: &AgentPool, ) -> Result { - crate::crdt_state::set_agent(story_id, Some(agent_name)); + crate::crdt_state::set_agent( + story_id, + agent_name.parse::().ok(), + ); agents .start_agent(project_root, story_id, Some(agent_name), None, None) .await @@ -114,10 +117,10 @@ mod tests { 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; + let _ = assign_and_start(story_id_a, "coder-opus", 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; + let _ = assign_and_start(story_id_b, "coder-opus", tmp.path(), &agents).await; // Both must leave the same CRDT agent register value. for sid in &[story_id_a, story_id_b] { @@ -129,7 +132,7 @@ mod tests { .expect("item must be in CRDT"); assert_eq!( item.agent.as_deref(), - Some("coder-sonnet"), + Some("coder-opus"), "CRDT agent register must match for story {sid}" ); }