diff --git a/src/command_surface.rs b/src/command_surface.rs index 1ccbafc..faaee06 100644 --- a/src/command_surface.rs +++ b/src/command_surface.rs @@ -15,7 +15,7 @@ use crate::{ SkillRunArgs, SkillTargetArg, SkillUninstallArgs, }, commands, - state::SessionHandle, + state::{SessionHandle, SessionType}, watch, }; @@ -82,8 +82,14 @@ pub fn run(args: &SkillRunArgs, registry: &WorkflowRegistry) -> Result { create_args, } => { let session_name = create_or_join(&create_args, registry)?; + let requested_agent = resolve_run_command_requested_agent( + &store, + &workspace, + &session_name, + &agent_name, + ); let actual_name = - ensure_agent_registered(&session_name, &agent_name, registry)?; + ensure_agent_registered(&session_name, &requested_agent, registry)?; let code = watch::next_with_registry( &session_name, &actual_name, @@ -98,8 +104,9 @@ pub fn run(args: &SkillRunArgs, registry: &WorkflowRegistry) -> Result { println!("session complete; stop working"); } - store.by_workspace.insert( - workspace, + remember_saved_context( + &mut store, + &workspace, SavedRunContext { session: session_name, agent: actual_name, @@ -131,6 +138,15 @@ pub fn run(args: &SkillRunArgs, registry: &WorkflowRegistry) -> Result { } else { args.first.clone() }; + let saved_agent = resolve_saved_agent_fallback( + &store, + &workspace, + args.session.as_deref(), + args.agent.as_deref(), + args.second.as_deref(), + explicit_invite.as_ref(), + saved.as_ref(), + ); let mut prompter = StdinPrompter; let resolved = resolve_run_context( @@ -139,6 +155,7 @@ pub fn run(args: &SkillRunArgs, registry: &WorkflowRegistry) -> Result { args.second.clone(), explicit_invite, saved.as_ref(), + saved_agent.as_deref(), &mut prompter, )?; @@ -157,8 +174,9 @@ pub fn run(args: &SkillRunArgs, registry: &WorkflowRegistry) -> Result { println!("session complete; stop working"); } - store.by_workspace.insert( - workspace, + remember_saved_context( + &mut store, + &workspace, SavedRunContext { session: resolved.session, agent: actual_name, @@ -593,6 +611,8 @@ struct ResolvedRunContext { struct RunContextStore { #[serde(default)] by_workspace: BTreeMap, + #[serde(default)] + by_identity: BTreeMap, } #[derive(Clone, Debug, Serialize, Deserialize)] @@ -986,8 +1006,9 @@ fn parse_invite_token(raw: &str) -> Option { pub(crate) fn normalize_agent_selector(raw: &str) -> String { let lowered = raw.trim().to_ascii_lowercase(); match lowered.as_str() { - "implement" => "implement".to_string(), + "implement" | "implementer" => "implement".to_string(), "review" => "review".to_string(), + "reviewer" => "review".to_string(), "review1" | "review-1" => "review-1".to_string(), _ => raw.trim().to_string(), } @@ -999,6 +1020,7 @@ fn resolve_run_context( role_selector: Option, invite: Option, saved: Option<&SavedRunContext>, + saved_agent: Option<&str>, prompter: &mut dyn Prompter, ) -> Result { let mut session = args.session.clone(); @@ -1034,9 +1056,9 @@ fn resolve_run_context( session = Some(saved_ctx.session.clone()); } if agent.is_none() - && let Some(saved_ctx) = saved + && let Some(saved_value) = saved_agent { - agent = Some(saved_ctx.agent.clone()); + agent = Some(saved_value.to_string()); } if session.is_none() { @@ -1072,6 +1094,90 @@ fn resolve_run_context( }) } +fn resolve_saved_agent_fallback( + store: &RunContextStore, + workspace: &str, + explicit_session: Option<&str>, + explicit_agent: Option<&str>, + role_selector: Option<&str>, + invite: Option<&InviteContext>, + saved_workspace: Option<&SavedRunContext>, +) -> Option { + let session = explicit_session + .map(str::to_string) + .or_else(|| invite.map(|ctx| ctx.session.clone())) + .or_else(|| saved_workspace.map(|ctx| ctx.session.clone())); + let requested_agent = explicit_agent + .map(normalize_agent_selector) + .or_else(|| role_selector.map(normalize_agent_selector)) + .or_else(|| invite.map(|ctx| ctx.agent.clone())); + + if let (Some(session), Some(requested_agent)) = (session.as_ref(), requested_agent.as_ref()) + && let Some(saved) = + find_saved_context_by_identity(store, workspace, session, requested_agent) + { + return Some(saved.agent.clone()); + } + + if let Some(saved) = saved_workspace + && allow_saved_agent_resume(saved) + { + return Some(saved.agent.clone()); + } + + None +} + +fn allow_saved_agent_resume(saved: &SavedRunContext) -> bool { + let Ok(handle) = SessionHandle::open(&saved.session) else { + return false; + }; + let Ok(state) = handle.load_state() else { + return false; + }; + allows_saved_agent_resume_for_expected_agents(state.config.expected_agents) +} + +fn allows_saved_agent_resume_for_expected_agents(expected_agents: u32) -> bool { + expected_agents <= 1 +} + +fn context_identity_key(workspace: &str, session: &str, agent: &str) -> String { + format!("{workspace}\u{1f}{session}\u{1f}{agent}") +} + +fn find_saved_context_by_identity<'a>( + store: &'a RunContextStore, + workspace: &str, + session: &str, + agent: &str, +) -> Option<&'a SavedRunContext> { + store + .by_identity + .get(&context_identity_key(workspace, session, agent)) +} + +fn resolve_run_command_requested_agent( + store: &RunContextStore, + workspace: &str, + session: &str, + requested_agent: &str, +) -> String { + find_saved_context_by_identity(store, workspace, session, requested_agent) + .map(|saved| saved.agent.clone()) + .unwrap_or_else(|| requested_agent.to_string()) +} + +fn remember_saved_context(store: &mut RunContextStore, workspace: &str, saved: SavedRunContext) { + store + .by_workspace + .insert(workspace.to_string(), saved.clone()); + store.by_identity.insert( + context_identity_key(workspace, &saved.session, &saved.agent), + saved, + ); +} + fn create_or_join( create_args: &crate::cli::CreateArgs, registry: &WorkflowRegistry, @@ -1099,11 +1205,7 @@ fn ensure_agent_registered( agent: &str, registry: &WorkflowRegistry, ) -> Result { - // "review" is the auto-assign base name — always go through join so the - // auto-numbering logic in join_with_registry can assign review-1, review-2, etc. - let needs_auto_assign = agent == "review"; - - if !needs_auto_assign { + if agent != "review" { let known = match SessionHandle::open(session) { Ok(handle) => { let state = handle.load_state()?; @@ -1117,6 +1219,64 @@ fn ensure_agent_registered( if known { return Ok(agent.to_string()); } + return commands::join_with_registry( + &JoinArgs { + session: session.to_string(), + agent: agent.to_string(), + timeout: Some(2), + }, + registry, + ); + } + + // "review" is auto-assign base name; dedupe when a single reviewer slot is present. + let handle = SessionHandle::open(session) + .with_context(|| format!("resolving session '{}' before join", session))?; + let state = handle.load_state()?; + + if state.session_type != SessionType::Implement { + if state.agents.contains_key(agent) { + return Ok(agent.to_string()); + } + return commands::join_with_registry( + &JoinArgs { + session: session.to_string(), + agent: agent.to_string(), + timeout: Some(2), + }, + registry, + ); + } + + let reviewer_slots = state.config.expected_agents.saturating_sub(1) as usize; + if reviewer_slots == 0 { + bail!("implement session '{}' has no reviewer slots", session); + } + + let mut existing_reviewers = state + .agents + .keys() + .filter_map(|name| parse_review_index(name).map(|idx| (idx, name.clone()))) + .filter(|(idx, _)| *idx <= reviewer_slots) + .collect::>(); + existing_reviewers.sort_by_key(|(idx, _)| *idx); + + if reviewer_slots == 1 + && existing_reviewers + .iter() + .any(|(_, name)| name == "review-1") + { + return Ok("review-1".to_string()); + } + + if existing_reviewers.len() >= reviewer_slots { + if existing_reviewers.len() == 1 { + return Ok(existing_reviewers[0].1.clone()); + } + bail!( + "all reviewer slots are filled for session '{}'; re-run with --as review-N to target an existing reviewer", + session + ); } commands::join_with_registry( @@ -1129,6 +1289,14 @@ fn ensure_agent_registered( ) } +fn parse_review_index(agent: &str) -> Option { + let idx = agent.strip_prefix("review-")?.parse::().ok()?; + if idx == 0 { + return None; + } + Some(idx) +} + fn workspace_key() -> Result { let cwd = env::current_dir().context("resolving current working directory")?; let canonical = cwd.canonicalize().unwrap_or(cwd); @@ -1588,6 +1756,7 @@ mod tests { Some("review".to_string()), Some(invite), Some(&saved), + Some(saved.agent.as_str()), &mut prompter, )?; assert_eq!(resolved.session, "explicit-session"); @@ -1612,8 +1781,15 @@ mod tests { agent: "review-1".to_string(), }; let mut prompter = StaticPrompter::new(&[]); - let resolved = - resolve_run_context(&args, None, None, Some(invite), Some(&saved), &mut prompter)?; + let resolved = resolve_run_context( + &args, + None, + None, + Some(invite), + Some(&saved), + Some(saved.agent.as_str()), + &mut prompter, + )?; assert_eq!(resolved.session, "token-session"); assert_eq!(resolved.agent, "review-1"); Ok(()) @@ -1631,18 +1807,97 @@ mod tests { workflow: Some("build".to_string()), }; let mut prompter = StaticPrompter::new(&[]); - let resolved = resolve_run_context(&args, None, None, None, Some(&saved), &mut prompter)?; + let resolved = resolve_run_context( + &args, + None, + None, + None, + Some(&saved), + Some(saved.agent.as_str()), + &mut prompter, + )?; assert_eq!(resolved.session, "saved-session"); assert_eq!(resolved.agent, "saved-agent"); assert_eq!(resolved.workflow.as_deref(), Some("build")); Ok(()) } + #[test] + fn run_resolution_prompts_for_agent_when_saved_agent_fallback_disabled() -> Result<()> { + let args = run_args(); + let saved = SavedRunContext { + session: "saved-session".to_string(), + agent: "implement".to_string(), + workflow: Some("implement".to_string()), + }; + let mut prompter = StaticPrompter::new(&["review"]); + let resolved = + resolve_run_context(&args, None, None, None, Some(&saved), None, &mut prompter)?; + assert_eq!(resolved.session, "saved-session"); + assert_eq!(resolved.agent, "review"); + assert_eq!(resolved.workflow.as_deref(), Some("implement")); + Ok(()) + } + + #[test] + fn saved_context_identity_lookup_requires_exact_agent_match() { + let mut store = RunContextStore::default(); + remember_saved_context( + &mut store, + "/tmp/ws", + SavedRunContext { + session: "s".to_string(), + agent: "review-1".to_string(), + workflow: Some("implement".to_string()), + }, + ); + + assert!( + find_saved_context_by_identity(&store, "/tmp/ws", "s", "review-1").is_some(), + "exact agent key should resolve" + ); + assert!( + find_saved_context_by_identity(&store, "/tmp/ws", "s", "review").is_none(), + "base review selector should not resolve review-1 identity" + ); + } + + #[test] + fn run_command_requested_agent_uses_identity_context_only() { + let mut store = RunContextStore::default(); + remember_saved_context( + &mut store, + "/tmp/ws", + SavedRunContext { + session: "s".to_string(), + agent: "review-1".to_string(), + workflow: Some("implement".to_string()), + }, + ); + + assert_eq!( + resolve_run_command_requested_agent(&store, "/tmp/ws", "s", "review-1"), + "review-1" + ); + assert_eq!( + resolve_run_command_requested_agent(&store, "/tmp/ws", "s", "review"), + "review", + "generic review selector should not inherit review-1 from shared workspace context" + ); + } + + #[test] + fn allows_saved_agent_resume_requires_single_agent_session() { + assert!(allows_saved_agent_resume_for_expected_agents(1)); + assert!(!allows_saved_agent_resume_for_expected_agents(2)); + assert!(!allows_saved_agent_resume_for_expected_agents(3)); + } + #[test] fn run_resolution_prompts_interactively_when_unresolved() -> Result<()> { let args = run_args(); let mut prompter = StaticPrompter::new(&["prompt-session", "review"]); - let resolved = resolve_run_context(&args, None, None, None, None, &mut prompter)?; + let resolved = resolve_run_context(&args, None, None, None, None, None, &mut prompter)?; assert_eq!(resolved.session, "prompt-session"); assert_eq!(resolved.agent, "review"); Ok(()) @@ -1655,7 +1910,7 @@ mod tests { ..run_args() }; let mut prompter = StaticPrompter::new(&[]); - let err = resolve_run_context(&args, None, None, None, None, &mut prompter) + let err = resolve_run_context(&args, None, None, None, None, None, &mut prompter) .expect_err("missing context should error"); assert!( err.to_string() @@ -1687,6 +1942,7 @@ mod tests { let new_path = home.join(".rally").join(CONTEXT_STORE_FILE); let loaded = load_context_store(&new_path, Some(&legacy)); assert_eq!(loaded.by_workspace.len(), 1); + assert!(loaded.by_identity.is_empty()); Ok(()) } @@ -1705,4 +1961,11 @@ mod tests { "rally skill run implement 'todos/my todo.md' review" ); } + + #[test] + fn normalize_agent_selector_maps_common_aliases() { + assert_eq!(normalize_agent_selector("implementer"), "implement"); + assert_eq!(normalize_agent_selector("reviewer"), "review"); + assert_eq!(normalize_agent_selector("review-1"), "review-1"); + } } diff --git a/src/commands.rs b/src/commands.rs index 2a3d01a..480b31c 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -40,88 +40,86 @@ pub fn create_with_registry(args: &CreateArgs, registry: &WorkflowRegistry) -> R bail!("expected_agents must be at least 1"); } - let (todo_path, workspace, workflow_id, workflow_version, workflow_state) = - match session_type { - SessionType::Plan => ( - args.todo.as_ref().map(|p| p.display().to_string()), - args.workspace.as_ref().map(|p| p.display().to_string()), - Some(workflow::builtin::PLAN_WORKFLOW_ID.to_string()), + let (todo_path, workspace, workflow_id, workflow_version, workflow_state) = match session_type { + SessionType::Plan => ( + args.todo.as_ref().map(|p| p.display().to_string()), + args.workspace.as_ref().map(|p| p.display().to_string()), + Some(workflow::builtin::PLAN_WORKFLOW_ID.to_string()), + None, + Some(plan::initial_workflow_state()?), + ), + SessionType::Implement => { + let todo = args + .todo + .as_ref() + .ok_or_else(|| anyhow::anyhow!("--todo is required for implement sessions"))?; + let workspace = args + .workspace + .as_ref() + .ok_or_else(|| anyhow::anyhow!("--workspace is required for implement sessions"))?; + if !workspace.exists() || !workspace.is_dir() { + bail!( + "--workspace must be an existing directory: {}", + workspace.display() + ); + } + let git_check = ProcessCommand::new("git") + .arg("-C") + .arg(workspace) + .arg("rev-parse") + .arg("--is-inside-work-tree") + .output()?; + if !git_check.status.success() { + bail!( + "--workspace must be a git workspace: {}", + workspace.display() + ); + } + let steps = implement::parse_steps_from_todo(todo)?; + ( + Some(todo.display().to_string()), + Some(workspace.display().to_string()), + Some(workflow::builtin::BUILD_WORKFLOW_ID.to_string()), None, - Some(plan::initial_workflow_state()?), - ), - SessionType::Implement => { - let todo = args - .todo - .as_ref() - .ok_or_else(|| anyhow::anyhow!("--todo is required for implement sessions"))?; - let workspace = args.workspace.as_ref().ok_or_else(|| { - anyhow::anyhow!("--workspace is required for implement sessions") - })?; - if !workspace.exists() || !workspace.is_dir() { - bail!( - "--workspace must be an existing directory: {}", - workspace.display() - ); - } - let git_check = ProcessCommand::new("git") - .arg("-C") - .arg(workspace) - .arg("rev-parse") - .arg("--is-inside-work-tree") - .output()?; - if !git_check.status.success() { - bail!( - "--workspace must be a git workspace: {}", - workspace.display() - ); - } - let steps = implement::parse_steps_from_todo(todo)?; - ( - Some(todo.display().to_string()), - Some(workspace.display().to_string()), - Some(workflow::builtin::BUILD_WORKFLOW_ID.to_string()), - None, - Some(implement::initial_workflow_state( - args.implementer.clone(), - steps, - args.pause_after_step, - )?), - ) + Some(implement::initial_workflow_state( + args.implementer.clone(), + steps, + args.pause_after_step, + )?), + ) + } + SessionType::Workflow => { + let workflow_id = args + .workflow + .clone() + .ok_or_else(|| anyhow::anyhow!("--workflow is required for create workflow"))?; + let _ = registry.resolve_str(&workflow_id)?; + if workflow_id == workflow::builtin::NEGOTIATE_WORKFLOW_ID && expected_agents < 2 { + bail!( + "workflow '{}' requires --agents >= 2 (got {})", + workflow_id, + expected_agents + ); } - SessionType::Workflow => { - let workflow_id = args - .workflow - .clone() - .ok_or_else(|| anyhow::anyhow!("--workflow is required for create workflow"))?; - let _ = registry.resolve_str(&workflow_id)?; - if workflow_id == workflow::builtin::NEGOTIATE_WORKFLOW_ID - && expected_agents < 2 - { - bail!( - "workflow '{}' requires --agents >= 2 (got {})", - workflow_id, - expected_agents - ); - } - - if let Some(workspace) = &args.workspace - && (!workspace.exists() || !workspace.is_dir()) - { - bail!( - "--workspace must be an existing directory: {}", - workspace.display() - ); - } - - ( - args.todo.as_ref().map(|p| p.display().to_string()), - args.workspace.as_ref().map(|p| p.display().to_string()), - Some(workflow_id), - None, - Some(json!({})), - ) + + if let Some(workspace) = &args.workspace + && (!workspace.exists() || !workspace.is_dir()) + { + bail!( + "--workspace must be an existing directory: {}", + workspace.display() + ); } - }; + + ( + args.todo.as_ref().map(|p| p.display().to_string()), + args.workspace.as_ref().map(|p| p.display().to_string()), + Some(workflow_id), + None, + Some(json!({})), + ) + } + }; let phase = SessionPhase::Registration; let state = SessionState { @@ -192,21 +190,33 @@ pub fn join_with_registry(args: &JoinArgs, registry: &WorkflowRegistry) -> Resul let handle = SessionHandle::open(&args.session)?; let mut state = handle.load_state()?; - // Auto-number: if agent name is the base "review", assign next available review-N - let actual_agent = if args.agent == "review" { + // Auto-number: implement reviewer base name "review" maps to review-N slots. + let actual_agent = if args.agent == "review" && state.session_type == SessionType::Implement { + let reviewer_slots = reviewer_slot_count(&state); + if reviewer_slots == 0 { + bail!("implement session '{}' has no reviewer slots", args.session); + } let mut name = None; - for n in 1..=state.config.expected_agents as usize { + for n in 1..=reviewer_slots { let candidate = format!("review-{n}"); if !state.agents.contains_key(&candidate) { name = Some(candidate); break; } } - name.unwrap_or_else(|| args.agent.clone()) + name.ok_or_else(|| anyhow::anyhow!("all reviewer slots are filled"))? } else { args.agent.clone() }; + let is_new_agent = !state.agents.contains_key(&actual_agent); + if is_new_agent && state.agents.len() >= state.config.expected_agents as usize { + bail!("session is full ({} agents)", state.config.expected_agents); + } + if is_new_agent && state.session_type == SessionType::Implement { + validate_new_implement_agent(&state, &actual_agent)?; + } + let now_ts = now(); state @@ -235,13 +245,18 @@ pub fn join_with_registry(args: &JoinArgs, registry: &WorkflowRegistry) -> Resul } dispatch_join_hook(&mut state, &handle.session_dir, &actual_agent, registry)?; - if state.phase == SessionPhase::Registration - && state.agents.len() as u32 >= state.config.expected_agents - { - state.phase = match state.session_type { - SessionType::Plan => SessionPhase::Proposal, - SessionType::Implement => SessionPhase::Implement, - SessionType::Workflow => SessionPhase::Implement, + if state.phase == SessionPhase::Registration { + state.phase = match &state.session_type { + SessionType::Plan if state.agents.len() as u32 >= state.config.expected_agents => { + SessionPhase::Proposal + } + SessionType::Workflow if state.agents.len() as u32 >= state.config.expected_agents => { + SessionPhase::Implement + } + SessionType::Implement if implement_registration_complete(&state)? => { + SessionPhase::Implement + } + _ => state.phase, }; } @@ -253,6 +268,61 @@ pub fn join_with_registry(args: &JoinArgs, registry: &WorkflowRegistry) -> Resul Ok(actual_agent) } +fn reviewer_slot_count(state: &SessionState) -> usize { + state.config.expected_agents.saturating_sub(1) as usize +} + +fn parse_reviewer_index(agent: &str) -> Option { + let idx = agent.strip_prefix("review-")?.parse::().ok()?; + if idx == 0 { + return None; + } + Some(idx) +} + +fn is_valid_reviewer_name(agent: &str, reviewer_slots: usize) -> bool { + parse_reviewer_index(agent).is_some_and(|idx| idx <= reviewer_slots) +} + +fn validate_new_implement_agent(state: &SessionState, agent: &str) -> Result<()> { + let workflow_state = implement::read_workflow_state(state)?; + if workflow_state.implementer_agent == agent { + return Ok(()); + } + + let reviewer_slots = reviewer_slot_count(state); + if is_valid_reviewer_name(agent, reviewer_slots) { + return Ok(()); + } + + if reviewer_slots == 0 { + bail!( + "invalid implement session agent '{}'; only implementer '{}' is allowed (no reviewer slots configured)", + agent, + workflow_state.implementer_agent + ); + } + + bail!( + "invalid implement session agent '{}'; expected '{}' or review-N where N is 1..={}", + agent, + workflow_state.implementer_agent, + reviewer_slots + ); +} + +fn implement_registration_complete(state: &SessionState) -> Result { + let workflow_state = implement::read_workflow_state(state)?; + let has_implementer = state.agents.contains_key(&workflow_state.implementer_agent); + let reviewer_slots = reviewer_slot_count(state); + let reviewer_count = state + .agents + .keys() + .filter(|agent| is_valid_reviewer_name(agent, reviewer_slots)) + .count(); + Ok(has_implementer && reviewer_count >= reviewer_slots) +} + pub fn next_with_registry(args: &NextArgs, registry: &WorkflowRegistry) -> Result { watch::next_with_registry(&args.session, &args.agent, args.timeout, registry) } @@ -518,14 +588,22 @@ pub fn reset() -> Result<()> { let handle = match SessionHandle::open(name) { Ok(h) => h, Err(_) => { - entries.push((name.clone(), String::new(), format!("{:<40} (corrupt)", name))); + entries.push(( + name.clone(), + String::new(), + format!("{:<40} (corrupt)", name), + )); continue; } }; let state = match handle.load_state() { Ok(s) => s, Err(_) => { - entries.push((name.clone(), String::new(), format!("{:<40} (corrupt)", name))); + entries.push(( + name.clone(), + String::new(), + format!("{:<40} (corrupt)", name), + )); continue; } }; @@ -538,7 +616,10 @@ pub fn reset() -> Result<()> { let agent_count = state.agents.len(); let line = format!( "{:<40} {:>2} agents {:?} {}", - name, agent_count, state.phase, most_recent.format("%Y-%m-%d %H:%M") + name, + agent_count, + state.phase, + most_recent.format("%Y-%m-%d %H:%M") ); entries.push((name.clone(), most_recent.to_rfc3339(), line)); } @@ -551,7 +632,11 @@ pub fn reset() -> Result<()> { .join("\n"); let mut child = ProcessCommand::new("fzf") - .args(["--multi", "--header", "Select session(s) to delete (TAB to multi-select)"]) + .args([ + "--multi", + "--header", + "Select session(s) to delete (TAB to multi-select)", + ]) .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) .spawn() diff --git a/tests/command_install_run.rs b/tests/command_install_run.rs index 8c06049..6ae61c3 100644 --- a/tests/command_install_run.rs +++ b/tests/command_install_run.rs @@ -451,6 +451,7 @@ fn command_install_run_entrypoint_integration() -> Result<()> { } // second run with same todo hits the create-or-join path (session already exists) + // and should reuse the implement agent in a zero-reviewer session. let run_cmd_result2 = run_cli( &[ "rally", @@ -458,7 +459,7 @@ fn command_install_run_entrypoint_integration() -> Result<()> { "run", "implement", &run_cmd_todo_rel, - "review", + "implement", "--reviewers", "0", "--non-interactive", @@ -473,7 +474,8 @@ fn command_install_run_entrypoint_integration() -> Result<()> { { let handle = SessionHandle::open(&run_cmd_session)?; let state = handle.load_state()?; - assert!(state.agents.contains_key("review-1")); + assert!(state.agents.contains_key("implement")); + assert!(!state.agents.contains_key("review-1")); } if run_cmd_todo.exists() { diff --git a/tests/extensibility_mvp.rs b/tests/extensibility_mvp.rs index a1c958f..b144572 100644 --- a/tests/extensibility_mvp.rs +++ b/tests/extensibility_mvp.rs @@ -736,6 +736,101 @@ fn negotiate_happy_path_runs_analysis_positions_and_finalization() -> Result<()> Ok(()) } +#[test] +fn implement_join_enforces_reviewer_slots_and_valid_agent_names() -> Result<()> { + let registry = builtin_registry()?; + let workspace = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let session = unique_name("rally-build-join-guards-it"); + let todo = build_todo_path(&session); + let session_dir = state::session_dir(&session); + let _ = fs::remove_dir_all(&session_dir); + + run_cli( + &[ + "rally", + "create", + "implement", + "--name", + &session, + "--todo", + todo.to_str().expect("utf8 path"), + "--workspace", + workspace.to_str().expect("utf8 path"), + "--reviewers", + "1", + ], + ®istry, + )?; + + let first = commands::join_with_registry( + &JoinArgs { + session: session.clone(), + agent: "review".to_string(), + timeout: None, + }, + ®istry, + )?; + assert_eq!(first, "review-1"); + + let handle = SessionHandle::open(&session)?; + let state_after_first_review = handle.load_state()?; + assert_eq!(state_after_first_review.phase, SessionPhase::Registration); + drop(handle); + + let err = commands::join_with_registry( + &JoinArgs { + session: session.clone(), + agent: "review".to_string(), + timeout: None, + }, + ®istry, + ) + .expect_err("second implicit reviewer should be rejected for --reviewers=1"); + assert!(err.to_string().contains("all reviewer slots are filled")); + + let err = commands::join_with_registry( + &JoinArgs { + session: session.clone(), + agent: "review-2".to_string(), + timeout: None, + }, + ®istry, + ) + .expect_err("out-of-range reviewer slot should be rejected"); + assert!(err.to_string().contains("invalid implement session agent")); + + let err = commands::join_with_registry( + &JoinArgs { + session: session.clone(), + agent: "todos/microvm-v1.md".to_string(), + timeout: None, + }, + ®istry, + ) + .expect_err("arbitrary non-role agent should be rejected"); + assert!(err.to_string().contains("invalid implement session agent")); + + commands::join_with_registry( + &JoinArgs { + session: session.clone(), + agent: "implement".to_string(), + timeout: None, + }, + ®istry, + )?; + + let handle = SessionHandle::open(&session)?; + let final_state = handle.load_state()?; + assert_eq!(final_state.phase, SessionPhase::Implement); + assert_eq!(final_state.agents.len(), 2); + assert!(final_state.agents.contains_key("implement")); + assert!(final_state.agents.contains_key("review-1")); + drop(handle); + + let _ = fs::remove_dir_all(&session_dir); + Ok(()) +} + #[test] fn negotiate_failure_cases_cover_identity_peer_timeout_agree_and_approver() -> Result<()> { let registry = builtin_registry()?;