diff --git a/src/crates/assembly/core/src/agentic/agents/registry/external.rs b/src/crates/assembly/core/src/agentic/agents/registry/external.rs index df1badffd4..96326a327b 100644 --- a/src/crates/assembly/core/src/agentic/agents/registry/external.rs +++ b/src/crates/assembly/core/src/agentic/agents/registry/external.rs @@ -1,6 +1,7 @@ use super::types::{AgentCategory, AgentEntry, AgentInfo, AgentSource, SubAgentSource}; use super::AgentRegistry; use crate::agentic::agents::{Agent, SubagentVisibilityPolicy}; +use crate::agentic::deep_review_policy::{CODE_REVIEW_AGENT_TYPE, DEEP_REVIEW_AGENT_TYPE}; use crate::agentic::workspace::canonical_local_workspace_path; use bitfun_agent_runtime::prompt_cache::prompt_cache_scope_key; use bitfun_core_types::{ @@ -8,6 +9,7 @@ use bitfun_core_types::{ }; use bitfun_product_domains::external_sources::EcosystemId; use bitfun_product_domains::external_subagents::ExternalSubagentMode; +use log::{debug, warn}; use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::sync::{Arc, RwLock, Weak}; @@ -436,10 +438,23 @@ impl AgentRegistry { .cloned() { let binding = match route { - ExternalSubagentRoute::Local => self - .find_agent_entry(logical_id, Some(workspace_root)) - .filter(|entry| entry.category == AgentCategory::Mode) - .map(|entry| local_primary_binding(entry.agent.id())), + ExternalSubagentRoute::Local => { + match self.find_agent_entry(logical_id, Some(workspace_root)) { + Some(entry) if is_local_session_primary_entry(&entry) => { + Some(local_primary_binding(entry.agent.id())) + } + Some(entry) => { + warn!( + "Session primary agent resolution rejected a registered non-mode agent under a Local route: logical_id={}, category={:?}, source={:?}", + logical_id, + entry.category, + entry.source + ); + None + } + None => None, + } + } ExternalSubagentRoute::External(runtime_key) => { self.external_subagents.acquire_primary(&runtime_key) } @@ -454,9 +469,28 @@ impl AgentRegistry { if expected_owner == Some(SessionAgentRouteOwner::External) { return None; } - self.find_agent_entry(logical_id, workspace_root) - .filter(|entry| entry.category == AgentCategory::Mode) - .map(|entry| local_primary_binding(entry.agent.id())) + match self.find_agent_entry(logical_id, workspace_root) { + Some(entry) if is_local_session_primary_entry(&entry) => { + Some(local_primary_binding(entry.agent.id())) + } + Some(entry) => { + warn!( + "Session primary agent resolution rejected a registered non-mode agent: logical_id={}, category={:?}, source={:?}, expected_owner={:?}", + logical_id, + entry.category, + entry.source, + expected_owner + ); + None + } + None => { + debug!( + "Session primary agent resolution found no registered agent: logical_id={}, expected_owner={:?}", + logical_id, expected_owner + ); + None + } + } } /// Resolve only the currently approved external route for an exact @@ -576,6 +610,29 @@ fn local_binding(logical_id: &str, runtime_agent_key: &str) -> ExternalSubagentI } } +/// Builtin agents that are allowed to act as the main agent of a session even +/// though they are not registered as `Mode` (review child sessions). +/// +/// Review child sessions are created by the product surfaces with +/// `agentType=CodeReview` (standard) or `agentType=DeepReview` (strict) and +/// must resolve through the primary-agent path for create, turn, restore, and +/// compaction. Other subagents (e.g. `ReviewWorker`) stay restricted. +fn is_builtin_session_primary_agent(id: &str) -> bool { + matches!(id, CODE_REVIEW_AGENT_TYPE | DEEP_REVIEW_AGENT_TYPE) +} + +/// Whether a locally-resolved agent entry may act as a session primary agent. +/// +/// Used by both the explicit `ExternalSubagentRoute::Local` branch and the +/// no-route fallback so review child sessions (CodeReview/DeepReview) resolve +/// identically regardless of whether a workspace route table pins them to the +/// local implementation. +fn is_local_session_primary_entry(entry: &AgentEntry) -> bool { + entry.category == AgentCategory::Mode + || (entry.source == AgentSource::Builtin + && is_builtin_session_primary_agent(entry.agent.id())) +} + fn local_primary_binding(runtime_agent_key: &str) -> ExternalPrimaryAgentTurnBinding { ExternalPrimaryAgentTurnBinding { runtime_agent_key: runtime_agent_key.to_string(), diff --git a/src/crates/assembly/core/src/agentic/agents/registry/tests.rs b/src/crates/assembly/core/src/agentic/agents/registry/tests.rs index ee6934f8f8..4ecdf08fea 100644 --- a/src/crates/assembly/core/src/agentic/agents/registry/tests.rs +++ b/src/crates/assembly/core/src/agentic/agents/registry/tests.rs @@ -1589,3 +1589,77 @@ fn external_primary_route_follows_the_session_execution_worktree() { assert_eq!(binding.runtime_agent_key, "external::worktree"); } + +#[test] +fn builtin_review_agents_resolve_as_local_session_primaries() { + let registry = AgentRegistry::new(); + + for agent_type in ["CodeReview", "DeepReview"] { + let binding = registry + .resolve_primary_agent_for_turn(agent_type, None, false, None) + .unwrap_or_else(|| { + panic!("{agent_type} must resolve as a session primary agent for review children") + }); + assert_eq!(binding.runtime_agent_key, agent_type); + assert_eq!( + binding.route_owner, + bitfun_core_types::SessionAgentRouteOwner::Local + ); + } +} + +#[test] +fn non_session_primary_subagents_and_unknown_ids_do_not_resolve() { + let registry = AgentRegistry::new(); + + // Registered subagents that are not session-capable stay restricted. + assert!(registry + .resolve_primary_agent_for_turn("ReviewWorker", None, false, None) + .is_none()); + // Unknown ids remain unknown. + assert!(registry + .resolve_primary_agent_for_turn("does-not-exist", None, false, None) + .is_none()); + // The external-owner guard still fails closed for review agents. + assert!(registry + .resolve_primary_agent_for_turn( + "CodeReview", + None, + false, + Some(bitfun_core_types::SessionAgentRouteOwner::External), + ) + .is_none()); +} + +#[test] +fn local_route_resolves_review_agents_as_session_primaries() { + let registry = AgentRegistry::new(); + let workspace = PathBuf::from("D:/workspace/review-local-route"); + registry.install_external_subagent_routes( + &workspace, + Vec::new(), + [ + ("CodeReview".to_string(), ExternalSubagentRoute::Local), + ("DeepReview".to_string(), ExternalSubagentRoute::Local), + ("ReviewWorker".to_string(), ExternalSubagentRoute::Local), + ] + .into_iter() + .collect(), + ); + + for agent_type in ["CodeReview", "DeepReview"] { + let binding = registry + .resolve_primary_agent_for_turn(agent_type, Some(&workspace), true, None) + .unwrap_or_else(|| panic!("{agent_type} must resolve through an explicit Local route")); + assert_eq!(binding.runtime_agent_key, agent_type); + assert_eq!( + binding.route_owner, + bitfun_core_types::SessionAgentRouteOwner::Local + ); + } + + // Non-session-primary subagents stay restricted even under a Local route. + assert!(registry + .resolve_primary_agent_for_turn("ReviewWorker", Some(&workspace), true, None) + .is_none()); +} diff --git a/src/crates/assembly/core/src/agentic/agents/registry/types.rs b/src/crates/assembly/core/src/agentic/agents/registry/types.rs index c8b8ac6371..0bcc9bbc2d 100644 --- a/src/crates/assembly/core/src/agentic/agents/registry/types.rs +++ b/src/crates/assembly/core/src/agentic/agents/registry/types.rs @@ -6,7 +6,9 @@ use crate::agentic::agents::{ mode_config_profile_label, mode_config_profile_member_mode_ids, resolve_mode_config_profile_id, Agent, AgentToolPolicyOverrides, }; -use crate::agentic::deep_review_policy::{is_review_worker_agent_type, REVIEW_JUDGE_AGENT_TYPE}; +use crate::agentic::deep_review_policy::{ + is_review_worker_agent_type, CODE_REVIEW_AGENT_TYPE, REVIEW_JUDGE_AGENT_TYPE, +}; pub(super) use bitfun_agent_runtime::agents::SubagentOverrideState; pub use bitfun_agent_runtime::agents::{ BuiltinAgentCategory as AgentCategory, SubAgentSource, SubagentListScope, SubagentQueryContext, @@ -231,7 +233,7 @@ pub(crate) fn is_review_agent_entry(entry: &AgentEntry) -> bool { } is_review_worker_agent_type(agent.id()) - || matches!(agent.id(), REVIEW_JUDGE_AGENT_TYPE | "CodeReview") + || matches!(agent.id(), REVIEW_JUDGE_AGENT_TYPE | CODE_REVIEW_AGENT_TYPE) } pub(crate) fn custom_agent_path(agent: &dyn Agent) -> Option { diff --git a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs index 4c55aec7b3..e95fefc0bb 100644 --- a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs +++ b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs @@ -14535,6 +14535,31 @@ mod tests { assert!(session_manager.get_session("ownership-conflict").is_none()); } + #[tokio::test] + async fn review_agent_child_sessions_create_successfully() { + let (coordinator, _session_manager) = test_coordinator(); + + for agent_type in ["CodeReview", "DeepReview"] { + let workspace = tempfile::tempdir().expect("review workspace"); + let session = coordinator + .create_session_with_workspace( + None, + format!("Review child: {agent_type}"), + agent_type.to_string(), + SessionConfig { + workspace_path: Some(workspace.path().to_string_lossy().into_owned()), + ..Default::default() + }, + workspace.path().to_string_lossy().into_owned(), + ) + .await + .unwrap_or_else(|error| { + panic!("{agent_type} review child session must create: {error}") + }); + assert_eq!(session.agent_type, agent_type); + } + } + #[tokio::test] async fn assistant_bootstrap_checks_runtime_ownership_before_files_or_attach() { let ownership_root = tempfile::tempdir().expect("ownership root"); diff --git a/src/crates/assembly/core/src/agentic/deep_review_policy.rs b/src/crates/assembly/core/src/agentic/deep_review_policy.rs index 0354f32da6..c0eb4e8e9a 100644 --- a/src/crates/assembly/core/src/agentic/deep_review_policy.rs +++ b/src/crates/assembly/core/src/agentic/deep_review_policy.rs @@ -41,9 +41,9 @@ pub use bitfun_agent_runtime::deep_review::{ DeepReviewSharedContextMeasurementSnapshot, DeepReviewStrategyLevel, DeepReviewSubagentRole, FocusedReviewAssignment, FocusedReviewBudgetClaim, ReviewStrategyManifestProfile, ReviewTeamDefinition, ReviewTeamExecutionPolicyDefinition, ReviewTeamRoleDefinition, - CONDITIONAL_REVIEWER_AGENT_TYPES, CORE_REVIEWER_AGENT_TYPES, DEEP_REVIEW_AGENT_TYPE, - LEGACY_REVIEW_WORKER_AGENT_TYPES, REVIEW_FIXER_AGENT_TYPE, REVIEW_JUDGE_AGENT_TYPE, - REVIEW_WORKER_AGENT_TYPE, + CODE_REVIEW_AGENT_TYPE, CONDITIONAL_REVIEWER_AGENT_TYPES, CORE_REVIEWER_AGENT_TYPES, + DEEP_REVIEW_AGENT_TYPE, LEGACY_REVIEW_WORKER_AGENT_TYPES, REVIEW_FIXER_AGENT_TYPE, + REVIEW_JUDGE_AGENT_TYPE, REVIEW_WORKER_AGENT_TYPE, }; const DEFAULT_REVIEW_TEAM_CONFIG_PATH: &str = "ai.review_teams.default"; diff --git a/src/crates/execution/agent-runtime/src/deep_review/constants.rs b/src/crates/execution/agent-runtime/src/deep_review/constants.rs index 1b28b92543..05f833de99 100644 --- a/src/crates/execution/agent-runtime/src/deep_review/constants.rs +++ b/src/crates/execution/agent-runtime/src/deep_review/constants.rs @@ -1,6 +1,7 @@ //! Deep Review agent type and role constants. pub const DEEP_REVIEW_AGENT_TYPE: &str = "DeepReview"; +pub const CODE_REVIEW_AGENT_TYPE: &str = "CodeReview"; pub const REVIEW_JUDGE_AGENT_TYPE: &str = "ReviewJudge"; pub const REVIEW_FIXER_AGENT_TYPE: &str = "ReviewFixer"; pub const REVIEW_WORKER_AGENT_TYPE: &str = "ReviewWorker"; diff --git a/src/crates/execution/agent-runtime/src/deep_review/mod.rs b/src/crates/execution/agent-runtime/src/deep_review/mod.rs index 8ea6099d5f..9f21d090aa 100644 --- a/src/crates/execution/agent-runtime/src/deep_review/mod.rs +++ b/src/crates/execution/agent-runtime/src/deep_review/mod.rs @@ -28,7 +28,7 @@ pub use budget::{ }; pub use concurrency_policy::{DeepReviewConcurrencyPolicy, DeepReviewEffectiveConcurrencySnapshot}; pub use constants::{ - canonical_review_worker_agent_type, is_review_worker_agent_type, + canonical_review_worker_agent_type, is_review_worker_agent_type, CODE_REVIEW_AGENT_TYPE, CONDITIONAL_REVIEWER_AGENT_TYPES, CORE_REVIEWER_AGENT_TYPES, DEEP_REVIEW_AGENT_TYPE, LEGACY_REVIEW_WORKER_AGENT_TYPES, REVIEW_FIXER_AGENT_TYPE, REVIEW_JUDGE_AGENT_TYPE, REVIEW_WORKER_AGENT_TYPE,