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 96326a327b..a9210cafc9 100644 --- a/src/crates/assembly/core/src/agentic/agents/registry/external.rs +++ b/src/crates/assembly/core/src/agentic/agents/registry/external.rs @@ -1,7 +1,9 @@ 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::deep_review_policy::{ + CODE_REVIEW_AGENT_TYPE, DEEP_REVIEW_AGENT_TYPE, REVIEW_FIXER_AGENT_TYPE, +}; use crate::agentic::workspace::canonical_local_workspace_path; use bitfun_agent_runtime::prompt_cache::prompt_cache_scope_key; use bitfun_core_types::{ @@ -614,11 +616,16 @@ fn local_binding(logical_id: &str, runtime_agent_key: &str) -> ExternalSubagentI /// 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. +/// `agentType=CodeReview` (standard) or `agentType=DeepReview` (strict), and +/// the remediation phase of either session runs with `agentType=ReviewFixer`. +/// All three must resolve through the primary-agent path for create, turn, +/// restore, and compaction. Other subagents (e.g. `ReviewWorker`, +/// `ReviewJudge`) stay restricted. fn is_builtin_session_primary_agent(id: &str) -> bool { - matches!(id, CODE_REVIEW_AGENT_TYPE | DEEP_REVIEW_AGENT_TYPE) + matches!( + id, + CODE_REVIEW_AGENT_TYPE | DEEP_REVIEW_AGENT_TYPE | REVIEW_FIXER_AGENT_TYPE + ) } /// Whether a locally-resolved agent entry may act as a session primary agent. 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 4ecdf08fea..dfa63a70dc 100644 --- a/src/crates/assembly/core/src/agentic/agents/registry/tests.rs +++ b/src/crates/assembly/core/src/agentic/agents/registry/tests.rs @@ -1594,7 +1594,7 @@ fn external_primary_route_follows_the_session_execution_worktree() { fn builtin_review_agents_resolve_as_local_session_primaries() { let registry = AgentRegistry::new(); - for agent_type in ["CodeReview", "DeepReview"] { + for agent_type in ["CodeReview", "DeepReview", "ReviewFixer"] { let binding = registry .resolve_primary_agent_for_turn(agent_type, None, false, None) .unwrap_or_else(|| { @@ -1613,22 +1613,53 @@ 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()); + for agent_type in ["ReviewWorker", "ReviewJudge"] { + assert!( + registry + .resolve_primary_agent_for_turn(agent_type, None, false, None) + .is_none(), + "{agent_type} must not resolve as a session primary agent" + ); + } // 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()); + for agent_type in ["CodeReview", "DeepReview", "ReviewFixer"] { + assert!( + registry + .resolve_primary_agent_for_turn( + agent_type, + None, + false, + Some(bitfun_core_types::SessionAgentRouteOwner::External), + ) + .is_none(), + "{agent_type} must fail closed for an external owner" + ); + } +} + +#[test] +fn non_builtin_same_name_review_agent_does_not_resolve_as_session_primary() { + let registry = AgentRegistry::new(); + + // Custom-agent loading currently filters ids that conflict with builtin + // entries, but the session-primary allowlist is source-gated regardless: + // a non-Builtin entry occupying the builtin "ReviewFixer" id must fail + // closed instead of inheriting the builtin primary path. + registry.write_agents().insert( + "ReviewFixer".to_string(), + test_source_custom_entry("ReviewFixer", "shadow", CustomSubagentKind::User), + ); + + assert!( + registry + .resolve_primary_agent_for_turn("ReviewFixer", None, false, None) + .is_none(), + "a non-Builtin entry named ReviewFixer must not resolve as a session primary agent" + ); } #[test] @@ -1641,13 +1672,15 @@ fn local_route_resolves_review_agents_as_session_primaries() { [ ("CodeReview".to_string(), ExternalSubagentRoute::Local), ("DeepReview".to_string(), ExternalSubagentRoute::Local), + ("ReviewFixer".to_string(), ExternalSubagentRoute::Local), ("ReviewWorker".to_string(), ExternalSubagentRoute::Local), + ("ReviewJudge".to_string(), ExternalSubagentRoute::Local), ] .into_iter() .collect(), ); - for agent_type in ["CodeReview", "DeepReview"] { + for agent_type in ["CodeReview", "DeepReview", "ReviewFixer"] { 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")); @@ -1659,7 +1692,12 @@ fn local_route_resolves_review_agents_as_session_primaries() { } // 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()); + for agent_type in ["ReviewWorker", "ReviewJudge"] { + assert!( + registry + .resolve_primary_agent_for_turn(agent_type, Some(&workspace), true, None) + .is_none(), + "{agent_type} must not resolve through a Local route" + ); + } } diff --git a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs index e95fefc0bb..67e6889a8e 100644 --- a/src/crates/assembly/core/src/agentic/coordination/coordinator.rs +++ b/src/crates/assembly/core/src/agentic/coordination/coordinator.rs @@ -354,6 +354,17 @@ fn resolve_subagent_model_selection( } } +/// Whether a turn belongs to the review phase of a review child session. +/// +/// Only `CodeReview`/`DeepReview` receive the `deep_review_run_manifest` +/// context injection (from turn metadata or persisted session metadata). +/// `ReviewFixer` is intentionally excluded: remediation runs outside the +/// DeepReview execution policy gates (launching it during a review pass is +/// rejected until explicit user approval), and its scope comes from the +/// product-surface remediation prompt rather than the review-phase manifest. +/// Keep this list in sync with the review session primary agents resolved by +/// the agent registry (`is_builtin_session_primary_agent`), i.e. add a new +/// review-phase agent type here, but keep the remediation agent out. fn is_review_agent_type(agent_type: &str) -> bool { matches!( agent_type.to_ascii_lowercase().as_str(), diff --git a/src/crates/assembly/core/src/agentic/deep_review/AGENTS.md b/src/crates/assembly/core/src/agentic/deep_review/AGENTS.md index a89d5d230b..449fdf91a3 100644 --- a/src/crates/assembly/core/src/agentic/deep_review/AGENTS.md +++ b/src/crates/assembly/core/src/agentic/deep_review/AGENTS.md @@ -16,6 +16,14 @@ This file applies to DeepReview runtime internals in this directory. reviewer agents in `src/crates/assembly/core/src/agentic/agents`. - Reviewer subagents stay read-only; `ReviewFixer` is not part of the review pass. +- `ReviewFixer` may only act as a session primary in the user-approved + remediation phase of a review child session. The agent registry resolves it + through the builtin primary-agent path without itself checking an approval + flag, so product surfaces must obtain explicit user approval before starting + remediation (mirroring `DeepReviewExecutionPolicy::classify_subagent`, which + rejects `ReviewFixer` during review execution). Do not route `ReviewFixer` + through review-phase manifest injection in the coordinator; its scope is + carried by the remediation prompt. - When queue or report fields change, update the matching frontend DTOs and DeepReview UI state.