From 4f38432d8709bb5f46eca72e9f372cbe2967fefc Mon Sep 17 00:00:00 2001 From: rhan-oai Date: Thu, 20 Aug 2026 16:41:21 +0000 Subject: [PATCH] Use model-specific auto-review outcome instructions (#39741) ## What changed - Add `rejection_instructions` and `timeout_instructions` to catalog-provided auto-review messages. - Use the acting model's instructions for denied and timed-out reviews across tool approvals, shell escalation, and MCP elicitation responses. - Fall back to the existing instructions only when a catalog value is absent, while preserving explicit empty-string overrides. ## Testing - Cover catalog overrides, legacy fallbacks, empty values, and separation between acting-model and reviewer-model messages. GitOrigin-RevId: c5b2c2dbdaefd45d1d658651dd1abaeb6d8c93da --- codex-rs/core/src/guardian/review.rs | 20 ++- codex-rs/core/src/guardian/review_session.rs | 6 + codex-rs/core/src/guardian/tests.rs | 20 ++- codex-rs/core/src/mcp_tool_call.rs | 2 +- codex-rs/core/src/session/mcp.rs | 14 +- codex-rs/core/src/session/mcp_tests.rs | 42 ++++- codex-rs/core/src/tools/approvals.rs | 9 +- codex-rs/core/src/tools/approvals_tests.rs | 31 +++- .../tools/runtimes/shell/unix_escalation.rs | 4 +- codex-rs/core/tests/suite/guardian_review.rs | 161 +++++++++++++++++- .../models-manager/src/model_info_tests.rs | 2 + codex-rs/protocol/src/openai_models.rs | 14 +- 12 files changed, 292 insertions(+), 33 deletions(-) diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 93d36c52a8..46b04147e3 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -10,6 +10,7 @@ use codex_extension_api::ThreadIdleCause; use codex_features::Feature; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::openai_models::MODEL_SPECIALTY_CYBER; +use codex_protocol::openai_models::ModelInfo; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::CodexErrorInfo; use codex_protocol::protocol::EventMsg; @@ -101,8 +102,14 @@ pub(crate) fn new_guardian_review_id() -> String { uuid::Uuid::new_v4().to_string() } -pub(crate) fn guardian_timeout_message() -> String { - GUARDIAN_TIMEOUT_INSTRUCTIONS.to_string() +pub(crate) fn guardian_timeout_message(model_info: &ModelInfo) -> String { + model_info + .model_messages + .as_ref() + .and_then(|messages| messages.auto_review.as_ref()) + .and_then(|messages| messages.timeout_instructions.as_deref()) + .unwrap_or(GUARDIAN_TIMEOUT_INSTRUCTIONS) + .to_string() } #[derive(Debug)] @@ -665,8 +672,15 @@ async fn run_guardian_review( } else { assessment.rationale.trim() }; + let rejection_instructions = turn + .model_info + .model_messages + .as_ref() + .and_then(|messages| messages.auto_review.as_ref()) + .and_then(|messages| messages.rejection_instructions.as_deref()) + .unwrap_or(GUARDIAN_REJECTION_INSTRUCTIONS); ReviewDecision::denied(format!( - "This action was rejected due to unacceptable risk.\nReason: {rationale}\n{GUARDIAN_REJECTION_INSTRUCTIONS}" + "This action was rejected due to unacceptable risk.\nReason: {rationale}\n{rejection_instructions}" )) } } diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 3ed0584e35..94a3229697 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -1912,6 +1912,8 @@ mod tests { auto_review: Some(AutoReviewMessages { policy: Some("Use the catalog Guardian policy.".to_string()), policy_template: Some(catalog_template.to_string()), + rejection_instructions: None, + timeout_instructions: None, }), permissions: None, multi_agent: None, @@ -1948,6 +1950,8 @@ mod tests { auto_review: Some(AutoReviewMessages { policy: Some(String::new()), policy_template: None, + rejection_instructions: None, + timeout_instructions: None, }), permissions: None, multi_agent: None, @@ -1992,6 +1996,8 @@ mod tests { auto_review: Some(AutoReviewMessages { policy: Some(catalog_policy.to_string()), policy_template: Some(String::new()), + rejection_instructions: None, + timeout_instructions: None, }), permissions: None, multi_agent: None, diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 679f6f78d0..c0db8a93c0 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -1427,10 +1427,28 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() { #[test] fn guardian_timeout_message_distinguishes_timeout_from_policy_denial() { - let message = guardian_timeout_message(); + let mut model = codex_models_manager::model_info::model_info_from_slug("acting-model"); + model.model_messages = None; + let message = guardian_timeout_message(&model); assert!(message.contains("did not finish before its deadline")); assert!(message.contains("retry once")); assert!(!message.contains("unacceptable risk")); + + for timeout_instructions in [None, Some("Catalog timeout instructions."), Some("")] { + model.model_messages = Some( + serde_json::from_value(serde_json::json!({ + "auto_review": { + "policy": "review policy", + "timeout_instructions": timeout_instructions, + }, + })) + .expect("model messages should deserialize"), + ); + assert_eq!( + guardian_timeout_message(&model), + timeout_instructions.unwrap_or(&message), + ); + } } #[tokio::test] diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 58ecdac2e5..8259d623c5 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -283,7 +283,7 @@ pub(crate) async fn handle_mcp_tool_call( &call_id, invocation, item_metadata.clone(), - crate::guardian::guardian_timeout_message(), + crate::guardian::guardian_timeout_message(&turn_context.model_info), /*already_started*/ true, ) .await diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 4c8b13c9e8..6b25214762 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -29,6 +29,7 @@ use codex_protocol::mcp_approval_meta::TOOL_DESCRIPTION_KEY as MCP_ELICITATION_T use codex_protocol::mcp_approval_meta::TOOL_NAME_KEY as MCP_ELICITATION_TOOL_NAME_KEY; use codex_protocol::mcp_approval_meta::TOOL_PARAMS_KEY as MCP_ELICITATION_TOOL_PARAMS_KEY; use codex_protocol::mcp_approval_meta::TOOL_TITLE_KEY as MCP_ELICITATION_TOOL_TITLE_KEY; +use codex_protocol::openai_models::ModelInfo; use codex_rmcp_client::Elicitation; use rmcp::model::ElicitationAction; use rmcp::model::RequestMetaObject; @@ -784,8 +785,9 @@ async fn review_guardian_mcp_elicitation( ) .await; - return Ok(matches!(decision, ReviewDecision::Approved) - .then(|| mcp_elicitation_response_from_guardian_decision(decision))); + return Ok(matches!(decision, ReviewDecision::Approved).then(|| { + mcp_elicitation_response_from_guardian_decision(decision, &turn_context.model_info) + })); } let approval_policy = mcp_config.approval_policy.value(); @@ -855,6 +857,7 @@ async fn review_guardian_mcp_elicitation( .await; Ok(Some(mcp_elicitation_response_from_guardian_decision( decision, + &turn_context.model_info, ))) } @@ -1005,6 +1008,7 @@ fn mcp_elicitation_request_id(id: &RequestId) -> String { fn mcp_elicitation_response_from_guardian_decision( decision: ReviewDecision, + model_info: &ModelInfo, ) -> ElicitationResponse { match decision { ReviewDecision::Approved @@ -1017,9 +1021,9 @@ fn mcp_elicitation_response_from_guardian_decision( meta: Some(mcp_elicitation_auto_meta()), }, ReviewDecision::Denied { rejection } => mcp_elicitation_decline_with_message(rejection), - ReviewDecision::TimedOut => { - mcp_elicitation_decline_with_message(crate::guardian::guardian_timeout_message()) - } + ReviewDecision::TimedOut => mcp_elicitation_decline_with_message( + crate::guardian::guardian_timeout_message(model_info), + ), ReviewDecision::Abort => ElicitationResponse { action: ElicitationAction::Cancel, content: None, diff --git a/codex-rs/core/src/session/mcp_tests.rs b/codex-rs/core/src/session/mcp_tests.rs index ec4ee2ed22..781dc301d7 100644 --- a/codex-rs/core/src/session/mcp_tests.rs +++ b/codex-rs/core/src/session/mcp_tests.rs @@ -217,8 +217,9 @@ fn guardian_elicitation_review_request_declines_unsupported_opt_in_shapes() { #[test] fn guardian_decisions_map_to_elicitation_responses_without_session_state() { + let model = codex_models_manager::model_info::model_info_from_slug("acting-model"); assert_eq!( - mcp_elicitation_response_from_guardian_decision(ReviewDecision::Approved), + mcp_elicitation_response_from_guardian_decision(ReviewDecision::Approved, &model), ElicitationResponse { action: ElicitationAction::Accept, content: Some(json!({})), @@ -228,9 +229,10 @@ fn guardian_decisions_map_to_elicitation_responses_without_session_state() { } ); assert_eq!( - mcp_elicitation_response_from_guardian_decision(ReviewDecision::denied( - "Denied by Guardian", - )), + mcp_elicitation_response_from_guardian_decision( + ReviewDecision::denied("Denied by Guardian"), + &model, + ), ElicitationResponse { action: ElicitationAction::Decline, content: None, @@ -241,18 +243,18 @@ fn guardian_decisions_map_to_elicitation_responses_without_session_state() { } ); assert_eq!( - mcp_elicitation_response_from_guardian_decision(ReviewDecision::TimedOut), + mcp_elicitation_response_from_guardian_decision(ReviewDecision::TimedOut, &model), ElicitationResponse { action: ElicitationAction::Decline, content: None, meta: Some(json!({ "approvals_reviewer": ApprovalsReviewer::AutoReview, - "message": crate::guardian::guardian_timeout_message(), + "message": crate::guardian::guardian_timeout_message(&model), })), } ); assert_eq!( - mcp_elicitation_response_from_guardian_decision(ReviewDecision::Abort), + mcp_elicitation_response_from_guardian_decision(ReviewDecision::Abort, &model), ElicitationResponse { action: ElicitationAction::Cancel, content: None, @@ -262,3 +264,29 @@ fn guardian_decisions_map_to_elicitation_responses_without_session_state() { } ); } + +#[test] +fn guardian_elicitation_timeout_uses_acting_model_instructions() { + let mut model = codex_models_manager::model_info::model_info_from_slug("acting-model"); + for timeout_instructions in ["Catalog timeout instructions.", ""] { + model.model_messages = Some( + serde_json::from_value(json!({ + "auto_review": { + "timeout_instructions": timeout_instructions, + }, + })) + .expect("model messages should deserialize"), + ); + assert_eq!( + mcp_elicitation_response_from_guardian_decision(ReviewDecision::TimedOut, &model), + ElicitationResponse { + action: ElicitationAction::Decline, + content: None, + meta: Some(json!({ + "approvals_reviewer": ApprovalsReviewer::AutoReview, + "message": timeout_instructions, + })), + } + ); + } +} diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 37a3481fee..23382bd1fa 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -36,6 +36,7 @@ use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::error::CodexErr; use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::openai_models::ModelInfo; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::NetworkPolicyRuleAction; @@ -452,7 +453,7 @@ struct ApprovalResolution { } impl ApprovalResolution { - fn into_tool_result(self) -> Result { + fn into_tool_result(self, model_info: &ModelInfo) -> Result { let source = self.source; match self.decision { ReviewDecision::ApprovedMcpPolicyAmendment => { @@ -474,7 +475,9 @@ impl ApprovalResolution { Err(ToolError::Rejected(rejection.to_string())) } ReviewDecision::Denied { rejection } => Err(ToolError::Rejected(rejection)), - ReviewDecision::TimedOut => Err(ToolError::Rejected(guardian_timeout_message())), + ReviewDecision::TimedOut => { + Err(ToolError::Rejected(guardian_timeout_message(model_info))) + } ReviewDecision::Abort => Err(ToolError::Codex(CodexErr::TurnAborted)), decision => Ok(decision), } @@ -543,7 +546,7 @@ impl Session { _ => {} } } - resolution.into_tool_result() + resolution.into_tool_result(&ctx.review_context.turn().model_info) } async fn request_reviewer_approval( diff --git a/codex-rs/core/src/tools/approvals_tests.rs b/codex-rs/core/src/tools/approvals_tests.rs index 201aa02930..15a165dbb9 100644 --- a/codex-rs/core/src/tools/approvals_tests.rs +++ b/codex-rs/core/src/tools/approvals_tests.rs @@ -1,4 +1,5 @@ use super::*; +use codex_models_manager::model_info::model_info_from_slug; use codex_protocol::approvals::NetworkPolicyAmendment; use pretty_assertions::assert_eq; @@ -15,7 +16,7 @@ fn approval_resolution_rejects_denied_network_policy_amendment() { }; assert!(matches!( - resolution.into_tool_result(), + resolution.into_tool_result(&model_info_from_slug("acting-model")), Err(ToolError::Rejected(rejection)) if rejection == "rejected by user" )); } @@ -28,7 +29,7 @@ fn approval_resolution_rejects_mcp_policy_amendment() { }; assert!(matches!( - resolution.into_tool_result(), + resolution.into_tool_result(&model_info_from_slug("acting-model")), Err(ToolError::Rejected(rejection)) if rejection == "Error while requesting approval" )); } @@ -41,7 +42,7 @@ fn approval_resolution_aborts_turn_when_approval_is_aborted() { }; assert!(matches!( - resolution.into_tool_result(), + resolution.into_tool_result(&model_info_from_slug("acting-model")), Err(ToolError::Codex(error)) if matches!( error.details(), @@ -50,6 +51,30 @@ fn approval_resolution_aborts_turn_when_approval_is_aborted() { )); } +#[test] +fn approval_resolution_uses_acting_model_timeout_instructions() { + let mut model = model_info_from_slug("acting-model"); + for timeout_instructions in ["Catalog timeout instructions.", ""] { + model.model_messages = Some( + serde_json::from_value(serde_json::json!({ + "auto_review": { + "timeout_instructions": timeout_instructions, + }, + })) + .expect("model messages should deserialize"), + ); + let resolution = ApprovalResolution { + decision: ReviewDecision::TimedOut, + source: ApprovalResolutionSource::Guardian, + }; + + assert!(matches!( + resolution.into_tool_result(&model), + Err(ToolError::Rejected(rejection)) if rejection == timeout_instructions + )); + } +} + #[test] fn guardian_cwd_preserves_drive_shaped_local_posix_path() { let native_cwd = AbsolutePathBuf::try_from(std::path::PathBuf::from("/C:/workspace")) diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 206020d53c..41cf4020a4 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -551,7 +551,9 @@ impl CoreShellActionProvider { EscalationDecision::deny(Some(rejection)) } ReviewDecision::TimedOut => EscalationDecision::deny(Some( - crate::guardian::guardian_timeout_message(), + crate::guardian::guardian_timeout_message( + &self.review_context.turn().model_info, + ), )), ReviewDecision::ApprovedMcpPolicyAmendment => { error!("Shell escalation received ApprovedMcpPolicyAmendment"); diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index d758c8b1da..f021e719d2 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -27,6 +27,7 @@ use codex_protocol::ThreadId; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::PermissionProfile; use codex_protocol::models::PermissionProfileSnapshot; +use codex_protocol::openai_models::AutoReviewMessages; use codex_protocol::openai_models::MODEL_SPECIALTY_CYBER; use codex_protocol::openai_models::ModelsResponse; use codex_protocol::permissions::FileSystemAccessMode; @@ -38,6 +39,7 @@ use codex_protocol::protocol::EnvironmentConfig; use codex_protocol::protocol::EnvironmentConfigState; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::protocol::ThreadSettingsOverrides; use codex_protocol::protocol::TurnAbortReason; @@ -971,7 +973,12 @@ async fn interrupted_guardian_tool_review_aborts_without_executing_the_command() } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn guardian_denial_rejects_tool_call_with_rationale() -> Result<()> { +#[test_case(None; "legacy_fallback")] +#[test_case(Some("Acting model rejection instructions."); "catalog_override")] +#[test_case(Some(""); "empty_override")] +async fn guardian_denial_rejects_tool_call_with_rationale( + rejection_instructions: Option<&'static str>, +) -> Result<()> { skip_if_no_network!(Ok(())); skip_if_sandbox!(Ok(())); skip_if_wine_exec!( @@ -989,12 +996,38 @@ async fn guardian_denial_rejects_tool_call_with_rationale() -> Result<()> { }; let sandbox_policy_for_config = sandbox_policy.clone(); - let mut builder = test_codex().with_config(move |config| { - config.permissions.approval_policy = Constrained::allow_any(approval_policy); - config - .set_legacy_sandbox_policy(sandbox_policy_for_config) - .expect("set sandbox policy"); - }); + let mut builder = test_codex() + .with_model_info_override("gpt-5.6-luna", |model| { + model + .model_messages + .as_mut() + .expect("reviewer model messages") + .auto_review = Some(AutoReviewMessages { + policy: None, + policy_template: None, + rejection_instructions: Some("Reviewer-only rejection instructions.".to_string()), + timeout_instructions: None, + }); + }) + .with_model_info_override("gpt-5.5", move |model| { + model.auto_review_model_override = Some("gpt-5.6-luna".to_string()); + model + .model_messages + .as_mut() + .expect("acting model messages") + .auto_review = Some(AutoReviewMessages { + policy: None, + policy_template: None, + rejection_instructions: rejection_instructions.map(str::to_string), + timeout_instructions: None, + }); + }) + .with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config + .set_legacy_sandbox_policy(sandbox_policy_for_config) + .expect("set sandbox policy"); + }); let test = builder.build_with_auto_env(&server).await?; let output_file = test.cwd.path().join("guardian-denied.txt"); @@ -1065,6 +1098,7 @@ async fn guardian_denial_rejects_tool_call_with_rationale() -> Result<()> { .find(|request| request.body_contains_text("Exercise Guardian denial routing.")) .expect("expected Guardian review request"); assert!(guardian_request.body_contains_text(&command)); + assert_eq!(guardian_request.body_json()["model"], "gpt-5.6-luna"); let tool_output = requests .iter() @@ -1074,6 +1108,15 @@ async fn guardian_denial_rejects_tool_call_with_rationale() -> Result<()> { tool_output.contains("The requested write has unacceptable test risk."), "Guardian rationale missing from rejected tool output: {tool_output}" ); + assert_eq!( + tool_output.contains("The agent must not attempt to achieve the same outcome"), + rejection_instructions.is_none(), + "legacy rejection instructions should only be used when absent: {tool_output}" + ); + if let Some(rejection_instructions) = rejection_instructions { + assert!(tool_output.contains(rejection_instructions)); + } + assert!(!tool_output.contains("Reviewer-only rejection instructions.")); assert!( !output_file.exists(), "Guardian-denied command unexpectedly executed" @@ -1082,6 +1125,110 @@ async fn guardian_denial_rejects_tool_call_with_rationale() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[test_case(None; "legacy_fallback")] +#[test_case(Some("Acting model timeout instructions."); "catalog_override")] +#[test_case(Some(""); "empty_override")] +async fn guardian_timeout_rejects_tool_call_with_acting_model_instructions( + timeout_instructions: Option<&'static str>, +) -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + skip_if_wine_exec!( + Ok(()), + "Guardian approval actions require host-native paths" + ); + + struct TimedOutReviewContributor; + + impl codex_extension_api::ApprovalReviewContributor for TimedOutReviewContributor { + fn contribute<'a>( + &'a self, + _session_store: &'a codex_extension_api::ExtensionData, + _thread_store: &'a codex_extension_api::ExtensionData, + _prompt: &'a str, + _extension_metrics: Option>, + ) -> codex_extension_api::ExtensionFuture<'a, Option> { + Box::pin(async { Some(ReviewDecision::TimedOut) }) + } + } + + let server = start_mock_server().await; + let mut extensions = ExtensionRegistryBuilder::::new(); + extensions.approval_review_contributor(Arc::new(TimedOutReviewContributor)); + let mut builder = test_codex() + .with_extensions(Arc::new(extensions.build())) + .with_model_info_override("gpt-5.5", move |model| { + model + .model_messages + .as_mut() + .expect("acting model messages") + .auto_review = Some(AutoReviewMessages { + policy: None, + policy_template: None, + rejection_instructions: None, + timeout_instructions: timeout_instructions.map(str::to_string), + }); + }) + .with_config(|config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + }); + let test = builder.build_with_auto_env(&server).await?; + let output_file = test.cwd.path().join("guardian-timed-out.txt"); + let tool_args = json!({ + "cmd": format!("printf should-not-run > {}", output_file.display()), + "sandbox_permissions": SandboxPermissions::RequireEscalated, + "justification": "Exercise Guardian timeout routing.", + }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_function_call( + "exec-call-timed-out", + "exec_command", + &tool_args.to_string(), + ), + ev_completed("parent-tool"), + ]), + sse(vec![ev_completed("parent-complete")]), + ], + ) + .await; + + test.codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "run a command whose approval review will time out".into(), + text_elements: Vec::new(), + }])) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let requests = responses.requests(); + let tool_output = requests + .iter() + .find_map(|request| request.function_call_output_text("exec-call-timed-out")) + .expect("expected timed-out tool output to be returned to the parent model"); + assert_eq!( + tool_output.contains("did not finish before its deadline"), + timeout_instructions.is_none(), + "legacy timeout instructions should only be used when absent: {tool_output}" + ); + if let Some(timeout_instructions) = timeout_instructions { + assert!(tool_output.contains(timeout_instructions)); + } + assert!(!tool_output.contains("unacceptable risk")); + assert!( + !output_file.exists(), + "command whose approval timed out unexpectedly executed" + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn cyber_model_guardian_denial_interrupts_turn_immediately() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/models-manager/src/model_info_tests.rs b/codex-rs/models-manager/src/model_info_tests.rs index 579c4b0250..4dd87e605d 100644 --- a/codex-rs/models-manager/src/model_info_tests.rs +++ b/codex-rs/models-manager/src/model_info_tests.rs @@ -37,6 +37,8 @@ fn base_instruction_override_is_literal_and_preserves_catalog_messages() { let auto_review = AutoReviewMessages { policy: Some("review policy".to_string()), policy_template: Some("review policy template".to_string()), + rejection_instructions: Some("rejection instructions".to_string()), + timeout_instructions: Some(String::new()), }; let permissions = PermissionMessages { danger_full_access: Some("danger".to_string()), diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index 8192e50fcf..c758f5f4a5 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -572,6 +572,8 @@ pub struct CollaborationModeMessages { pub struct AutoReviewMessages { pub policy: Option, pub policy_template: Option, + pub rejection_instructions: Option, + pub timeout_instructions: Option, } #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] @@ -987,7 +989,7 @@ mod tests { } #[test] - fn auto_review_messages_preserve_missing_and_empty_template_values() { + fn auto_review_messages_preserve_missing_and_empty_values() { let missing_template: ModelMessages = from_str( r#"{ "instructions_template": null, @@ -1004,7 +1006,9 @@ mod tests { "instructions_variables": null, "auto_review": { "policy": "policy", - "policy_template": "" + "policy_template": "", + "rejection_instructions": "", + "timeout_instructions": "" } }"#, ) @@ -1015,6 +1019,8 @@ mod tests { Some(AutoReviewMessages { policy: Some("policy".to_string()), policy_template: None, + rejection_instructions: None, + timeout_instructions: None, }) ); assert_eq!( @@ -1022,6 +1028,8 @@ mod tests { Some(AutoReviewMessages { policy: Some("policy".to_string()), policy_template: Some(String::new()), + rejection_instructions: Some(String::new()), + timeout_instructions: Some(String::new()), }) ); } @@ -1385,6 +1393,8 @@ mod tests { auto_review: Some(AutoReviewMessages { policy: Some("policy".to_string()), policy_template: None, + rejection_instructions: Some("rejection instructions".to_string()), + timeout_instructions: Some("timeout instructions".to_string()), }), permissions: Some(PermissionMessages { danger_full_access: None,