diff --git a/codex-rs/core/src/guardian/review_session_tests.rs b/codex-rs/core/src/guardian/review_session_tests.rs index 6220fd5a58..9ec712aeff 100644 --- a/codex-rs/core/src/guardian/review_session_tests.rs +++ b/codex-rs/core/src/guardian/review_session_tests.rs @@ -570,47 +570,6 @@ async fn guardian_review_session_compact_scope_change_invalidates_cached_session assert_ne!(cached_reuse_key, next_reuse_key); } -#[tokio::test] -async fn guardian_review_session_config_disables_hooks() { - let mut parent_config = crate::config::test_config().await; - parent_config - .features - .enable(Feature::CodexHooks) - .expect("enable hooks on parent config"); - - let guardian_config = build_guardian_review_session_config( - crate::guardian::test_host::build_reviewer_config(&parent_config).expect("reviewer config"), - /*live_network_config*/ None, - "active-model", - /*reasoning_effort*/ None, - ReasoningSummaryConfig::default(), - /*personality*/ None, - ResolvedModelMessages::bundled(), - ) - .expect("guardian config"); - - assert!(!guardian_config.features.enabled(Feature::CodexHooks)); -} - -#[tokio::test] -async fn guardian_review_session_config_disables_skill_instructions() { - let mut parent_config = crate::config::test_config().await; - parent_config.include_skill_instructions = true; - - let guardian_config = build_guardian_review_session_config( - crate::guardian::test_host::build_reviewer_config(&parent_config).expect("reviewer config"), - /*live_network_config*/ None, - "active-model", - /*reasoning_effort*/ None, - ReasoningSummaryConfig::default(), - /*personality*/ None, - ResolvedModelMessages::bundled(), - ) - .expect("guardian config"); - - assert!(!guardian_config.include_skill_instructions); -} - #[test_case::test_case( Some("Use the managed Guardian policy."), Some("Configured Guardian template:\n{{ tenant_policy_config }}"), diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 3d430ba72b..45287045b9 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -42,7 +42,6 @@ use codex_prompts::ResolvedModelMessages; use codex_protocol::ThreadId; use codex_protocol::approvals::GuardianAssessmentAction; use codex_protocol::approvals::NetworkApprovalProtocol; -use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::ReasoningSummary; use codex_protocol::models::ContentItem; use codex_protocol::models::PermissionProfile; @@ -58,7 +57,6 @@ use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; -use codex_protocol::protocol::GranularApprovalConfig; use codex_protocol::protocol::GuardianAssessmentStatus; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::TurnCompleteEvent; @@ -1486,42 +1484,6 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning( assert!(warnings.is_empty()); } -#[tokio::test] -async fn routes_approval_to_guardian_requires_guardian_reviewer() { - let (_session, mut turn) = crate::session::tests::make_session_and_context().await; - let mut config = (*turn.config).clone(); - config.approvals_reviewer = ApprovalsReviewer::User; - turn.config = Arc::new(config.clone()); - - assert!(!routes_approval_to_guardian(&turn)); - - config.approvals_reviewer = ApprovalsReviewer::AutoReview; - turn.config = Arc::new(config); - - assert!(routes_approval_to_guardian(&turn)); -} - -#[tokio::test] -async fn routes_approval_to_guardian_allows_granular_review_policy() { - let (_session, mut turn) = crate::session::tests::make_session_and_context().await; - let mut config = (*turn.config).clone(); - config.approvals_reviewer = ApprovalsReviewer::AutoReview; - turn.config = Arc::new(config); - Arc::make_mut(&mut turn.config) - .permissions - .approval_policy - .set(AskForApproval::Granular(GranularApprovalConfig { - sandbox_approval: true, - rules: true, - skill_approval: true, - request_permissions: true, - mcp_elicitations: true, - })) - .expect("test setup should allow updating approval policy"); - - assert!(routes_approval_to_guardian(&turn)); -} - #[test] fn build_guardian_transcript_reserves_separate_budget_for_tool_evidence() { let repeated = "signal ".repeat(8_000); @@ -3686,60 +3648,6 @@ async fn guardian_review_session_config_preserves_context_overrides_for_same_eff ); } -#[tokio::test] -async fn guardian_review_session_config_clears_parent_developer_instructions() { - let defaults = ResolvedModelMessages::bundled().auto_review(); - let mut parent_config = test_config().await; - parent_config.developer_instructions = - Some("parent or managed config should not replace guardian policy".to_string()); - - let guardian_config = build_guardian_review_session_config_for_test( - crate::guardian::test_host::build_reviewer_config(&parent_config).expect("reviewer config"), - /*live_network_config*/ None, - "active-model", - /*reasoning_effort*/ None, - ReasoningSummary::default(), - /*personality*/ None, - ResolvedModelMessages::bundled(), - ) - .expect("guardian config"); - - assert_eq!(guardian_config.developer_instructions, None); - assert_eq!( - guardian_config.base_instructions, - Some( - GuardianPolicyInstructions::new( - defaults.policy, - defaults.policy_template, - guardian_output_contract_prompt(), - ) - .render() - ) - ); -} - -#[tokio::test] -async fn guardian_review_session_config_clears_legacy_notify() { - let mut parent_config = test_config().await; - parent_config.notify = Some(vec![ - "/path/to/notify".to_string(), - "turn-ended".to_string(), - ]); - - let guardian_config = build_guardian_review_session_config_for_test( - crate::guardian::test_host::build_reviewer_config(&parent_config).expect("reviewer config"), - /*live_network_config*/ None, - "active-model", - /*reasoning_effort*/ None, - ReasoningSummary::default(), - /*personality*/ None, - ResolvedModelMessages::bundled(), - ) - .expect("guardian config"); - - assert_eq!(guardian_config.notify, None); -} - #[tokio::test] async fn guardian_review_session_config_uses_live_network_proxy_state() { let mut parent_config = test_config().await; @@ -3788,7 +3696,8 @@ async fn guardian_review_session_config_uses_live_network_proxy_state() { } #[tokio::test] -async fn guardian_review_session_config_disables_mcp_apps_plugins_memories_and_guardian_v2() { +async fn guardian_review_session_config_isolates_parent_customizations() { + let defaults = ResolvedModelMessages::bundled().auto_review(); let mut parent_config = test_config().await; let server: McpServerConfig = toml::from_str("command = \"docs-server\"").expect("deserialize MCP server"); @@ -3796,21 +3705,28 @@ async fn guardian_review_session_config_disables_mcp_apps_plugins_memories_and_g .mcp_servers .set(HashMap::from([("docs".to_string(), server)])) .expect("parent MCP servers are configurable"); - parent_config - .features - .enable(Feature::Apps) - .expect("apps feature is configurable"); - parent_config - .features - .enable(Feature::Plugins) - .expect("plugins feature is configurable"); - parent_config - .features - .enable(Feature::GuardianV2) - .expect("guardian v2 feature is configurable"); + let disabled_features = [ + Feature::Apps, + Feature::Plugins, + Feature::GuardianV2, + Feature::CodexHooks, + ]; + for feature in disabled_features { + parent_config + .features + .enable(feature) + .expect("enable feature on parent config"); + } parent_config.include_apps_instructions = true; + parent_config.include_skill_instructions = true; parent_config.memories.use_memories = true; parent_config.memories.dedicated_tools = true; + parent_config.developer_instructions = + Some("parent or managed config should not replace guardian policy".to_string()); + parent_config.notify = Some(vec![ + "/path/to/notify".to_string(), + "turn-ended".to_string(), + ]); let guardian_config = build_guardian_review_session_config_for_test( crate::guardian::test_host::build_reviewer_config(&parent_config).expect("reviewer config"), @@ -3824,12 +3740,29 @@ async fn guardian_review_session_config_disables_mcp_apps_plugins_memories_and_g .expect("guardian config"); assert!(guardian_config.mcp_servers.get().is_empty()); - assert!(!guardian_config.features.enabled(Feature::Apps)); - assert!(!guardian_config.features.enabled(Feature::Plugins)); - assert!(!guardian_config.features.enabled(Feature::GuardianV2)); + for feature in disabled_features { + assert!( + !guardian_config.features.enabled(feature), + "{feature:?} should be disabled in the reviewer" + ); + } assert!(!guardian_config.include_apps_instructions); + assert!(!guardian_config.include_skill_instructions); assert!(!guardian_config.memories.use_memories); assert!(!guardian_config.memories.dedicated_tools); + assert_eq!(guardian_config.notify, None); + assert_eq!(guardian_config.developer_instructions, None); + assert_eq!( + guardian_config.base_instructions, + Some( + GuardianPolicyInstructions::new( + defaults.policy, + defaults.policy_template, + guardian_output_contract_prompt(), + ) + .render() + ) + ); } #[tokio::test] @@ -3978,18 +3911,3 @@ async fn review_approval_request( .await .expect("Guardian should handle the request") } - -/// Whether this turn should route allowed approval prompts through the guardian -/// reviewer instead of surfacing them to the user. ARC may still block actions -/// earlier in the flow. -fn routes_approval_to_guardian(turn: &crate::session::turn_context::TurnContext) -> bool { - routes_approval_to_guardian_with_reviewer(turn, turn.config.approvals_reviewer) -} - -/// Whether an approval with its own reviewer selection should be routed through guardian. -fn routes_approval_to_guardian_with_reviewer( - turn: &crate::session::turn_context::TurnContext, - approvals_reviewer: ApprovalsReviewer, -) -> bool { - routes_approval_policy_to_guardian(turn.approval_policy(), approvals_reviewer) -} diff --git a/codex-rs/ext/guardian-reviewer/src/routing.rs b/codex-rs/ext/guardian-reviewer/src/routing.rs index 9dfbfd2b06..87a872f211 100644 --- a/codex-rs/ext/guardian-reviewer/src/routing.rs +++ b/codex-rs/ext/guardian-reviewer/src/routing.rs @@ -183,3 +183,7 @@ impl ReviewRequest<'_, H> { ReviewDecision::Approved } } + +#[cfg(test)] +#[path = "routing_tests.rs"] +mod tests; diff --git a/codex-rs/ext/guardian-reviewer/src/routing_tests.rs b/codex-rs/ext/guardian-reviewer/src/routing_tests.rs new file mode 100644 index 0000000000..3a59454bb9 --- /dev/null +++ b/codex-rs/ext/guardian-reviewer/src/routing_tests.rs @@ -0,0 +1,30 @@ +//! Approval routing coverage at the policy boundary, without host session setup. + +use super::routes_approval_policy_to_guardian; +use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::GranularApprovalConfig; +use pretty_assertions::assert_eq; + +#[test] +fn approval_routing_depends_on_policy_and_reviewer() { + for (policy, expected) in [ + (AskForApproval::UnlessTrusted, [false, false]), + (AskForApproval::OnRequest, [false, true]), + ( + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: true, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + [false, true], + ), + (AskForApproval::Never, [false, false]), + ] { + let actual = [ApprovalsReviewer::User, ApprovalsReviewer::AutoReview] + .map(|reviewer| routes_approval_policy_to_guardian(policy, reviewer)); + assert_eq!(actual, expected, "approval policy: {policy:?}"); + } +} diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/transcript_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/transcript_tests.rs index f8ebb0aca6..ea586bc52a 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/transcript_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/transcript_tests.rs @@ -1078,50 +1078,6 @@ fn configured_reasoning_counts_against_message_budget() { } } -#[test] -fn transcript_keeps_only_manual_approval_developer_messages() { - let approval_text = format!("{MANUAL_APPROVAL_DEVELOPER_PREFIX}\n\nApproved action:\n{{}}"); - let items = vec![ - ResponseItem::Message { - id: None, - role: "developer".to_string(), - content: vec![ContentItem::InputText { - text: "ordinary developer context".to_string(), - }], - phase: None, - internal_chat_message_metadata_passthrough: None, - }, - ResponseItem::Message { - id: None, - role: "developer".to_string(), - content: vec![ContentItem::InputText { - text: approval_text.clone(), - }], - phase: None, - internal_chat_message_metadata_passthrough: None, - }, - ]; - - let transcript = TranscriptConfig::default() - .build_context(ContextInput { - target: ContextTarget::Async, - history: &TestConversationHistory(&items), - root_conversation: &[], - trusted_user_answers: &[], - planned_action: None, - previous_reviews: None, - trusted_tool: None, - trusted_skill_paths: &[], - node_repl_images: None, - }) - .expect("collect transcript") - .transcript_entries(); - assert_eq!( - transcript, - vec![format!("[1] developer: {approval_text}\n")] - ); -} - #[test] fn transcript_omits_media_payloads_and_keeps_readable_content() { let oversized_image = diff --git a/codex-rs/guardian-context/src/transcript_tests.rs b/codex-rs/guardian-context/src/transcript_tests.rs index 329fe158ef..9e3adb362b 100644 --- a/codex-rs/guardian-context/src/transcript_tests.rs +++ b/codex-rs/guardian-context/src/transcript_tests.rs @@ -42,7 +42,7 @@ fn entry(kind: ConversationTranscriptEntryKind, text: &str) -> ConversationTrans } #[test] -fn registered_transcript_preserves_shared_roles_and_node_repl_tool_attribution() { +fn registered_transcript_filters_roles_and_preserves_node_repl_tool_attribution() { let approved_action = format!( "{MANUAL_APPROVAL_DEVELOPER_PREFIX}\nApproved action: {}", "exact action ".repeat(/*n*/ 1_000) @@ -57,6 +57,15 @@ fn registered_transcript_preserves_shared_roles_and_node_repl_tool_attribution() phase: None, internal_chat_message_metadata_passthrough: None, }, + ResponseItem::Message { + id: None, + role: "developer".to_string(), + content: vec![ContentItem::InputText { + text: "ordinary developer context".to_string(), + }], + phase: None, + internal_chat_message_metadata_passthrough: None, + }, ResponseItem::Message { id: None, role: "developer".to_string(),