From 13ddd08cd9154b2aaf76fe3687297bf6d0990ab7 Mon Sep 17 00:00:00 2001 From: Tant Date: Thu, 6 Aug 2026 16:48:13 +0800 Subject: [PATCH 1/3] fix(review): restore CodeReview/DeepReview session creation and turns Review child sessions are created with agentType=CodeReview (standard) or DeepReview (strict), but resolve_primary_agent_for_turn only accepted Mode-category agents, so session creation failed with "Unknown session mode: CodeReview" and persisted review sessions could be silently rewritten to agentic on restore. Allow the builtin CodeReview/DeepReview agents to resolve as local session primaries while keeping all other subagents restricted, and add diagnostics at the rejection point. Regression introduced by ca94825ad. --- .../src/agentic/agents/registry/external.rs | 41 +++++++++++++++++-- .../core/src/agentic/agents/registry/tests.rs | 41 +++++++++++++++++++ .../src/agentic/coordination/coordinator.rs | 25 +++++++++++ 3 files changed, 104 insertions(+), 3 deletions(-) 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 df1badffd..a06835e19 100644 --- a/src/crates/assembly/core/src/agentic/agents/registry/external.rs +++ b/src/crates/assembly/core/src/agentic/agents/registry/external.rs @@ -8,6 +8,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}; @@ -454,9 +455,32 @@ 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 entry.category == AgentCategory::Mode + || (entry.source == AgentSource::Builtin + && is_builtin_session_primary_agent(entry.agent.id())) => + { + 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 +600,17 @@ 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, "CodeReview" | "DeepReview") +} + 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 ee6934f8f..3753dd9a5 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,44 @@ 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()); +} diff --git a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs index 4c55aec7b..e95fefc0b 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"); From cdde99b1401944eabb6251949b2024e09dfe05a2 Mon Sep 17 00:00:00 2001 From: Tant Date: Thu, 6 Aug 2026 17:11:20 +0800 Subject: [PATCH 2/3] refactor(review): add CODE_REVIEW_AGENT_TYPE constant for review agent ids Introduce CODE_REVIEW_AGENT_TYPE in agent-runtime deep_review constants and use it in registry review-entry detection and session-primary resolution, replacing the scattered "CodeReview" magic string. This keeps review agent ids consistent with the existing DEEP_REVIEW_AGENT_TYPE / REVIEW_JUDGE_AGENT_TYPE constants. This cleanup also served as the verification content for the /review session fix in this PR: it was written into the workspace and reviewed end-to-end with the /review command after installing the fixed build, confirming review sessions now create and run correctly. --- .../assembly/core/src/agentic/agents/registry/external.rs | 3 ++- .../assembly/core/src/agentic/agents/registry/types.rs | 6 ++++-- src/crates/assembly/core/src/agentic/deep_review_policy.rs | 6 +++--- .../execution/agent-runtime/src/deep_review/constants.rs | 1 + src/crates/execution/agent-runtime/src/deep_review/mod.rs | 2 +- 5 files changed, 11 insertions(+), 7 deletions(-) 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 a06835e19..d1e9ff40f 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::{ @@ -608,7 +609,7 @@ fn local_binding(logical_id: &str, runtime_agent_key: &str) -> ExternalSubagentI /// 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, "CodeReview" | "DeepReview") + matches!(id, CODE_REVIEW_AGENT_TYPE | DEEP_REVIEW_AGENT_TYPE) } fn local_primary_binding(runtime_agent_key: &str) -> ExternalPrimaryAgentTurnBinding { 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 c8b8ac637..0bcc9bbc2 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/deep_review_policy.rs b/src/crates/assembly/core/src/agentic/deep_review_policy.rs index 0354f32da..c0eb4e8e9 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 1b28b9254..05f833de9 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 8ea6099d5..9f21d090a 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, From c02740ad37f340df47db4f387b30b272ca764951 Mon Sep 17 00:00:00 2001 From: Tant Date: Thu, 6 Aug 2026 17:36:32 +0800 Subject: [PATCH 3/3] fix(review): honor explicit Local routes when resolving review session primaries resolve_primary_agent_for_turn still filtered the ExternalSubagentRoute::Local branch to AgentCategory::Mode, so workspaces whose route table pins CodeReview/DeepReview to the local implementation (same-name conflict resolved to the local candidate) could still fail to create, restore, or start review sessions. Extract a shared is_local_session_primary_entry predicate used by both the explicit Local-route branch and the no-route fallback, and add a diagnostic warn when a registered entry is rejected under a Local route. --- .../src/agentic/agents/registry/external.rs | 39 ++++++++++++++----- .../core/src/agentic/agents/registry/tests.rs | 33 ++++++++++++++++ 2 files changed, 63 insertions(+), 9 deletions(-) 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 d1e9ff40f..96326a327 100644 --- a/src/crates/assembly/core/src/agentic/agents/registry/external.rs +++ b/src/crates/assembly/core/src/agentic/agents/registry/external.rs @@ -438,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) } @@ -457,11 +470,7 @@ impl AgentRegistry { return None; } match self.find_agent_entry(logical_id, workspace_root) { - Some(entry) - if entry.category == AgentCategory::Mode - || (entry.source == AgentSource::Builtin - && is_builtin_session_primary_agent(entry.agent.id())) => - { + Some(entry) if is_local_session_primary_entry(&entry) => { Some(local_primary_binding(entry.agent.id())) } Some(entry) => { @@ -612,6 +621,18 @@ 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 3753dd9a5..4ecdf08fe 100644 --- a/src/crates/assembly/core/src/agentic/agents/registry/tests.rs +++ b/src/crates/assembly/core/src/agentic/agents/registry/tests.rs @@ -1630,3 +1630,36 @@ fn non_session_primary_subagents_and_unknown_ids_do_not_resolve() { ) .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()); +}