From e412b93d08eb3d9d116373727349c4123baca16e Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Thu, 17 Sep 2026 00:25:25 +0000 Subject: [PATCH] Trim Guardian tests and tighten request layout assertions (#46063) ## What changed Remove standalone tests across Guardian review sessions, configuration, assessment schemas, transcripts, and trusted skill paths. Strengthen the existing request layout snapshot test to check exact transcript separators, the action header, and parent session ID placement before snapshot whitespace normalization. Concatenate user input text without inserting newlines so the assertions validate the actual request formatting. GitOrigin-RevId: 06d262a1a85b1a8401796958f300c894146d2e1b --- .../core/src/guardian/review_session_tests.rs | 46 ---- codex-rs/core/src/guardian/tests.rs | 200 +----------------- .../guardian-reviewer/src/assessment_tests.rs | 31 --- .../src/async_scorer/transcript_tests.rs | 90 -------- .../src/trusted_skills_tests.rs | 8 - 5 files changed, 5 insertions(+), 370 deletions(-) diff --git a/codex-rs/core/src/guardian/review_session_tests.rs b/codex-rs/core/src/guardian/review_session_tests.rs index 255adf4276..6220fd5a58 100644 --- a/codex-rs/core/src/guardian/review_session_tests.rs +++ b/codex-rs/core/src/guardian/review_session_tests.rs @@ -695,17 +695,6 @@ async fn guardian_review_session_config_resolves_policy_and_template( ); } -#[test] -fn had_prior_review_context_tracks_prompt_mode() { - assert!(!had_prior_review_context(&GuardianPromptMode::Full)); - assert!(had_prior_review_context(&GuardianPromptMode::Delta { - cursor: GuardianTranscriptCursor { - parent_history_version: 7, - transcript_entry_count: 42, - } - })); -} - #[test] fn token_usage_delta_never_reports_negative_usage() { let start = TokenUsage { @@ -847,41 +836,6 @@ async fn run_review_removes_trunk_when_event_stream_is_broken() { assert!(manager.trunk().await.is_none()); } -#[tokio::test] -async fn wait_for_guardian_review_ignores_prior_turn_completion() { - let (review_session, tx_event, _rx_sub) = test_review_session().await; - tx_event - .send(turn_complete_event("prior-turn", Some("stale"), Some(9))) - .await - .expect("queue prior turn completion"); - tx_event - .send(turn_complete_event("current-turn", Some("fresh"), Some(42))) - .await - .expect("queue current turn completion"); - - let mut analytics_result = GuardianReviewAnalyticsResult::without_session(); - let codex_guardian_reviewer::ReviewTurnResult { - outcome, - disposition, - turn_completed, - } = wait_for_guardian_review( - &review_session, - "current-turn", - tokio::time::Instant::now() + Duration::from_secs(1), - /*external_cancel*/ None, - &mut analytics_result, - ) - .await; - - let GuardianReviewSessionOutcome::Completed(Ok(last_agent_message)) = outcome else { - panic!("expected current turn completion"); - }; - assert_eq!(last_agent_message.as_deref(), Some("fresh")); - assert_eq!(analytics_result.time_to_first_token_ms, Some(42)); - assert_eq!(disposition, SessionDisposition::Reusable); - assert!(turn_completed); -} - #[tokio::test] async fn wait_for_guardian_review_ignores_prior_turn_errors() { let (review_session, tx_event, _rx_sub) = test_review_session().await; diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 647b9d6173..3d430ba72b 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -398,40 +398,6 @@ fn last_user_message_text_from_body(body: &serde_json::Value) -> String { .collect::() } -#[tokio::test(flavor = "current_thread")] -async fn build_guardian_prompt_full_mode_preserves_initial_review_format() -> anyhow::Result<()> { - let (session, turn) = guardian_test_session_and_turn_with_base_url("http://localhost").await; - seed_guardian_parent_history(&session, &turn).await; - - let prompt = build_guardian_prompt_items( - session.as_ref(), - Some("Sandbox denied outbound git push to github.com.".to_string()), - GuardianApprovalRequest::ExecCommand { - id: "shell-1".to_string(), - environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), - command: vec!["git".to_string(), "push".to_string()], - cwd: test_path_buf("/repo/codex-rs/core").abs().into(), - guardian_cwd: native_guardian_cwd("/repo/codex-rs/core"), - sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, - additional_permissions: None, - justification: Some("Need to push the reviewed docs fix.".to_string()), - tty: false, - }, - GuardianPromptMode::Full, - ) - .await?; - - let text = guardian_prompt_text(&prompt.context.into_user_inputs()?); - assert!(text.contains("whose request action you are assessing")); - assert!(text.contains(">>> TRANSCRIPT START\n")); - assert!(text.contains(">>> TRANSCRIPT END\n")); - assert!(text.contains("The Codex agent has requested the following action:\n")); - assert!(!text.contains("TRANSCRIPT DELTA")); - assert_eq!(prompt.transcript_cursor.transcript_entry_count, 4); - - Ok(()) -} - #[tokio::test(flavor = "current_thread")] async fn build_guardian_prompt_prefers_retry_reason_over_approval_reason() -> anyhow::Result<()> { let (session, turn) = guardian_test_session_and_turn_with_base_url("http://localhost").await; @@ -896,43 +862,6 @@ fn collect_guardian_transcript_entries_skips_contextual_user_messages() { ); } -#[test] -fn collect_guardian_transcript_entries_keeps_manual_approval_developer_message() { - let approval_text = - format!("{AUTO_REVIEW_DENIED_ACTION_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 entries = collect_guardian_transcript_entries(&items, GUARDIAN_MAX_TOOL_ENTRY_TOKENS); - - assert_eq!( - entries, - vec![ConversationTranscriptEntry { - kind: ConversationTranscriptEntryKind::Developer, - original_bytes: approval_text.len(), - text: approval_text, - }] - ); -} - #[test] fn collect_guardian_transcript_entries_includes_recent_tool_calls_and_output() { let mut items = vec![ @@ -1572,20 +1501,6 @@ async fn routes_approval_to_guardian_requires_guardian_reviewer() { assert!(routes_approval_to_guardian(&turn)); } -#[tokio::test] -async fn routes_approval_to_guardian_can_use_app_reviewer_override() { - let (_session, turn) = crate::session::tests::make_session_and_context().await; - - assert!(!routes_approval_to_guardian_with_reviewer( - &turn, - ApprovalsReviewer::User - )); - assert!(routes_approval_to_guardian_with_reviewer( - &turn, - ApprovalsReviewer::AutoReview - )); -} - #[tokio::test] async fn routes_approval_to_guardian_allows_granular_review_policy() { let (_session, mut turn) = crate::session::tests::make_session_and_context().await; @@ -2223,9 +2138,12 @@ async fn guardian_review_request_layout_matches_model_visible_request_snapshot() guardian_nested_tool_names, vec!["exec_command", "view_image", "write_stdin"] ); - let guardian_user_text = request.message_input_texts("user").join("\n"); + // Check exact separators before the snapshot normalizes trailing whitespace. + let guardian_user_text = request.message_input_texts("user").concat(); + assert!(guardian_user_text.contains(">>> TRANSCRIPT START\n")); + assert!(guardian_user_text.contains("The Codex agent has requested the following action:\n")); assert!(guardian_user_text.contains(&format!( - "Reviewed Codex session id: {}", + ">>> TRANSCRIPT END\nReviewed Codex session id: {}\n", fixed_guardian_parent_session_id() ))); assert!( @@ -2304,48 +2222,6 @@ async fn guardian_review_request_layout_matches_model_visible_request_snapshot() Ok(()) } -#[tokio::test] -async fn build_guardian_prompt_items_includes_parent_session_id() -> anyhow::Result<()> { - let (session, _) = crate::session::tests::make_session_and_context().await; - let prompt = build_guardian_prompt_items( - &session, - /*retry_reason*/ None, - GuardianApprovalRequest::ExecCommand { - id: "shell-1".to_string(), - environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), - command: vec!["git".to_string(), "status".to_string()], - cwd: test_path_buf("/repo").abs().into(), - guardian_cwd: native_guardian_cwd("/repo"), - sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, - additional_permissions: None, - justification: None, - tty: false, - }, - GuardianPromptMode::Full, - ) - .await?; - let prompt_text = prompt - .context - .into_user_inputs()? - .into_iter() - .map(|item| match item { - codex_protocol::user_input::UserInput::Text { text, .. } => text, - codex_protocol::user_input::UserInput::Image { .. } => String::new(), - _ => String::new(), - }) - .collect::(); - - assert!( - prompt_text.contains(&format!( - ">>> TRANSCRIPT END\nReviewed Codex session id: {}\n", - session.thread_id - )), - "guardian prompt should expose the parent session id immediately after the transcript end" - ); - - Ok(()) -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); @@ -3986,25 +3862,6 @@ async fn guardian_review_session_config_allows_pinned_disabled_feature() { assert!(!guardian_config.include_apps_instructions); } -#[tokio::test] -async fn guardian_review_session_config_uses_parent_active_model_instead_of_hardcoded_slug() { - let mut parent_config = test_config().await; - parent_config.model = Some("configured-model".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.model, Some("active-model".to_string())); -} - #[tokio::test] async fn guardian_review_session_config_keeps_bedrock_provider_for_bedrock_gpt_5_4() { let mut parent_config = test_config().await; @@ -4096,53 +3953,6 @@ async fn guardian_review_session_config_uses_requirements_guardian_policy_config ); } -#[tokio::test] -async fn guardian_review_session_config_uses_default_guardian_policy_without_requirements_override() -{ - let defaults = ResolvedModelMessages::bundled().auto_review(); - let codex_home = tempfile::tempdir().expect("create temp dir"); - let workspace = tempfile::tempdir().expect("create temp dir"); - let config_layer_stack = - ConfigLayerStack::new(Vec::new(), Default::default(), Default::default()) - .expect("config layer stack"); - let parent_config = Config::load_config_with_layer_stack( - LOCAL_FS.as_ref(), - ConfigToml::default(), - ConfigOverrides { - cwd: Some(workspace.path().to_path_buf()), - ..Default::default() - }, - codex_home.abs(), - config_layer_stack, - ) - .await - .expect("load config"); - - 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() - ) - ); -} - // Keep the existing reviewer tests on the production decision path. async fn review_approval_request( session: &Arc, diff --git a/codex-rs/ext/guardian-reviewer/src/assessment_tests.rs b/codex-rs/ext/guardian-reviewer/src/assessment_tests.rs index f30113a7aa..5cdd8e311e 100644 --- a/codex-rs/ext/guardian-reviewer/src/assessment_tests.rs +++ b/codex-rs/ext/guardian-reviewer/src/assessment_tests.rs @@ -50,34 +50,3 @@ fn parse_guardian_assessment_treats_bare_deny_as_high_risk() { } ); } - -#[test] -fn guardian_output_schema_requires_only_outcome_and_allows_optional_details() { - let schema = guardian_output_schema(); - - assert_eq!( - schema, - serde_json::json!({ - "type": "object", - "additionalProperties": false, - "properties": { - "risk_level": { - "type": "string", - "enum": ["low", "medium", "high", "critical"] - }, - "user_authorization": { - "type": "string", - "enum": ["unknown", "low", "medium", "high"] - }, - "outcome": { - "type": "string", - "enum": ["allow", "deny"] - }, - "rationale": { - "type": "string" - } - }, - "required": ["outcome"] - }) - ); -} 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 b7f0bc7263..f8ebb0aca6 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 @@ -2,7 +2,6 @@ use super::ContextInput; use codex_extension_api::ConversationHistorySnapshot; use codex_extension_api::ResponseItem; use codex_guardian_context::ContextTarget; -use codex_guardian_context::truncate_text as truncate_entry; use codex_protocol::AgentPath; use codex_protocol::models::AgentMessageInputContent; use codex_protocol::models::ContentItem; @@ -1029,84 +1028,6 @@ fn transcript_truncates_tool_results_using_standard_budget() { ); } -#[test] -fn transcript_preserves_outputs_with_call_ids_or_explicit_names() { - let mut items = vec![ResponseItem::FunctionCallOutput { - id: None, - call_id: None, - name: Some("notifications".to_owned()), - namespace: Some("slack".to_owned()), - output: FunctionCallOutputPayload::from_text("new message".to_owned()), - internal_chat_message_metadata_passthrough: None, - }]; - items.extend( - [ - (None, "anonymous output"), - (Some("missing-call"), "orphaned function output"), - ] - .map(|(call_id, text)| ResponseItem::FunctionCallOutput { - id: None, - call_id: call_id.map(str::to_string), - name: None, - namespace: None, - output: FunctionCallOutputPayload::from_text(text.to_string()), - internal_chat_message_metadata_passthrough: None, - }), - ); - - assert_eq!( - 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(), - vec![ - "[1] tool slack.notifications result: new message\n", - "[2] tool result: orphaned function output\n", - ] - ); - - if let ResponseItem::FunctionCallOutput { output, .. } = &mut items[0] { - *output = FunctionCallOutputPayload::from_content_items(vec![ - FunctionCallOutputContentItem::InputImage { - image: ImageReference::Inline { - image_url: "data:image/png;base64,image".to_owned(), - }, - detail: None, - }, - ]); - } - assert_eq!( - 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(), - vec![ - "[1] tool slack.notifications result: [non-text output]\n", - "[2] tool result: orphaned function output\n", - ] - ); -} - #[test] fn configured_reasoning_counts_against_message_budget() { for (repeats, include_reasoning) in [(200, true), (1_000, false)] { @@ -1157,17 +1078,6 @@ fn configured_reasoning_counts_against_message_budget() { } } -#[test] -fn truncate_entry_preserves_prefix_suffix_and_utf8_boundaries() { - let text = format!("prefix é{}é suffix", "🦀".repeat(2_000)); - let truncated = truncate_entry(&text, /*max_tokens*/ 200); - - assert!(truncated.starts_with("prefix é")); - assert!(truncated.contains(") -> Vec { .expect("trusted skill evidence should remain valid JSON") } -#[test] -fn renders_verified_skill_paths() { - assert_eq!( - rendered_paths(vec!["/home/user/.codex/skills/demo/SKILL.md".to_owned()]), - vec!["/home/user/.codex/skills/demo/SKILL.md"], - ); -} - #[test] fn bounds_escaped_skill_paths_without_corrupting_json_or_utf8() { let paths = (0..16)