openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-09-20 12:47:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

Consolidate Guardian tests at shared policy and context boundaries (#46064)

Browse Source 
## What changed

- Move approval routing tests into `guardian-reviewer`, covering both reviewer choices across `UnlessTrusted`, `OnRequest`, `Granular`, and `Never` without host session setup.
- Combine reviewer configuration checks into one test for isolation from parent customizations.
- Cover filtering of ordinary developer messages while preserving manual approvals in `guardian-context`, removing the duplicate async scorer test.

GitOrigin-RevId: b39ef21475e2f5d99d758f2d27615449b252dc65
This commit is contained in:
felixxia-oai
2026-09-17 00:25:26 +00:00
committed by copyberry
parent e412b93d08
commit 08663cc91b
6 changed files with 85 additions and 209 deletions
Download Patch File Download Diff File Expand all files Collapse all files

41
codex-rs/core/src/guardian/review_session_tests.rs
View File

@@ -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 }}"),

164
codex-rs/core/src/guardian/tests.rs
View File

@@ -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)
}

4
codex-rs/ext/guardian-reviewer/src/routing.rs
View File

@@ -183,3 +183,7 @@ impl<H: ReviewHost> ReviewRequest<'_, H> {
ReviewDecision::Approved
}
}
#[cfg(test)]
#[path = "routing_tests.rs"]
mod tests;

30
codex-rs/ext/guardian-reviewer/src/routing_tests.rs Normal file
View File

@@ -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:?}");
}
}

44
codex-rs/ext/guardian-v2/src/async_scorer/transcript_tests.rs
View File

@@ -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 =

11
codex-rs/guardian-context/src/transcript_tests.rs
View File

@@ -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(),