From 2be7d3bcd9d1aec2780f0a71fe79cbb5afd877a1 Mon Sep 17 00:00:00 2001 From: rhan-oai Date: Tue, 21 Jul 2026 00:17:58 +0000 Subject: [PATCH] Support catalog messages for non-request approval policies (#34434) ## What changed - Add model-catalog approval message variants for `never` and `unless_trusted`. - Select the catalog message that matches the active approval policy, while retaining the existing built-in text when that variant is absent. - Treat an explicitly empty variant as an instruction to suppress the built-in approval text, consistent with `on_request` messages. ## Testing - Cover variant selection, fallback and empty-message behavior, catalog deserialization, and the initial permissions message sent to the model. GitOrigin-RevId: a0f8d41a08645f39b80093be53f200eeee18ca25 --- .../context/world_state/permissions_tests.rs | 2 + .../core/tests/suite/permissions_messages.rs | 65 +++++++++++++++++++ .../models-manager/src/model_info_tests.rs | 4 ++ .../prompts/src/permissions_instructions.rs | 15 +++-- .../src/permissions_instructions_tests.rs | 53 +++++++++++++-- codex-rs/protocol/src/openai_models.rs | 7 +- 6 files changed, 135 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/src/context/world_state/permissions_tests.rs b/codex-rs/core/src/context/world_state/permissions_tests.rs index 769b9a472a25..dedeee09037c 100644 --- a/codex-rs/core/src/context/world_state/permissions_tests.rs +++ b/codex-rs/core/src/context/world_state/permissions_tests.rs @@ -72,6 +72,8 @@ fn permissions_state( let approval_messages = ApprovalMessages { on_request: Some("Ask for approval.".to_string()), on_request_auto_review: None, + never: None, + unless_trusted: None, }; let permission_messages = PermissionMessages { danger_full_access: Some("Full access.".to_string()), diff --git a/codex-rs/core/tests/suite/permissions_messages.rs b/codex-rs/core/tests/suite/permissions_messages.rs index 4a9e9ea3a28f..e9fa9f40dd11 100644 --- a/codex-rs/core/tests/suite/permissions_messages.rs +++ b/codex-rs/core/tests/suite/permissions_messages.rs @@ -51,6 +51,8 @@ fn model_with_approval_messages( approvals: Some(ApprovalMessages { on_request: Some(on_request.to_string()), on_request_auto_review: Some(on_request_auto_review.to_string()), + never: None, + unless_trusted: None, }), auto_review: None, permissions: None, @@ -179,6 +181,69 @@ async fn model_change_appends_new_catalog_approval_message() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn catalog_non_on_request_approval_messages_are_sent_in_initial_permissions() -> Result<()> { + skip_if_no_network!(Ok(())); + + for (approval_policy, approvals, expected, unexpected) in [ + ( + AskForApproval::Never, + ApprovalMessages { + on_request: None, + on_request_auto_review: None, + never: Some("catalog never approval instructions".to_string()), + unless_trusted: None, + }, + "catalog never approval instructions", + "Approval policy is currently never", + ), + ( + AskForApproval::UnlessTrusted, + ApprovalMessages { + on_request: None, + on_request_auto_review: None, + never: None, + unless_trusted: Some("catalog unless-trusted approval instructions".to_string()), + }, + "catalog unless-trusted approval instructions", + "`approval_policy` is `unless-trusted`", + ), + ] { + let server = start_mock_server().await; + let req = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + ) + .await; + let model_slug = "catalog-non-on-request-approvals-model"; + let mut model = model_info_from_slug(model_slug); + model.model_messages = Some(ModelMessages { + instructions_template: None, + instructions_variables: None, + approvals: Some(approvals), + auto_review: None, + permissions: None, + }); + let mut builder = test_codex() + .with_model(model_slug) + .with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config.model_catalog = Some(ModelsResponse { + models: vec![model], + }); + }); + let test = builder.build(&server).await?; + + submit_text_turn(&test, "hello").await?; + + let permissions = permissions_texts(&req.single_request()); + assert_eq!(permissions.len(), 1); + assert!(permissions[0].contains(expected)); + assert!(!permissions[0].contains(unexpected)); + } + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn catalog_permission_message_is_sent_initially_and_after_model_change() -> 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 838cdcefc0c0..9cc3f3cecb99 100644 --- a/codex-rs/models-manager/src/model_info_tests.rs +++ b/codex-rs/models-manager/src/model_info_tests.rs @@ -20,6 +20,8 @@ fn base_instruction_override_preserves_catalog_approval_messages() { let approvals = ApprovalMessages { on_request: Some("user approvals".to_string()), on_request_auto_review: Some("auto approvals".to_string()), + never: None, + unless_trusted: None, }; model.model_messages = Some(ModelMessages { instructions_template: Some("template".to_string()), @@ -57,6 +59,8 @@ fn disabled_personality_preserves_catalog_approval_messages() { let approvals = ApprovalMessages { on_request: Some("user approvals".to_string()), on_request_auto_review: None, + never: None, + unless_trusted: None, }; model.model_messages = Some(ModelMessages { instructions_template: Some("template".to_string()), diff --git a/codex-rs/prompts/src/permissions_instructions.rs b/codex-rs/prompts/src/permissions_instructions.rs index a438812c22a4..82f1ffc400e5 100644 --- a/codex-rs/prompts/src/permissions_instructions.rs +++ b/codex-rs/prompts/src/permissions_instructions.rs @@ -229,12 +229,15 @@ fn approval_text( exec_permission_approvals_enabled: bool, request_permissions_tool_enabled: bool, ) -> String { - if approval_policy == AskForApproval::OnRequest - && let Some(approval_messages) = approval_messages - { - let selected = match approvals_reviewer { - ApprovalsReviewer::User => approval_messages.on_request.as_ref(), - ApprovalsReviewer::AutoReview => approval_messages.on_request_auto_review.as_ref(), + if let Some(approval_messages) = approval_messages { + let selected = match &approval_policy { + AskForApproval::OnRequest => match approvals_reviewer { + ApprovalsReviewer::User => approval_messages.on_request.as_ref(), + ApprovalsReviewer::AutoReview => approval_messages.on_request_auto_review.as_ref(), + }, + AskForApproval::Never => approval_messages.never.as_ref(), + AskForApproval::UnlessTrusted => approval_messages.unless_trusted.as_ref(), + AskForApproval::Granular(_) => None, }; if let Some(selected) = selected { return selected.clone(); diff --git a/codex-rs/prompts/src/permissions_instructions_tests.rs b/codex-rs/prompts/src/permissions_instructions_tests.rs index 5a82cd5da908..eb1c2bfa3aa9 100644 --- a/codex-rs/prompts/src/permissions_instructions_tests.rs +++ b/codex-rs/prompts/src/permissions_instructions_tests.rs @@ -387,18 +387,35 @@ fn catalog_approval_messages_select_reviewer_variant() { let messages = ApprovalMessages { on_request: Some("user catalog approvals".to_string()), on_request_auto_review: Some("auto-review catalog approvals".to_string()), + never: Some("never catalog approvals".to_string()), + unless_trusted: Some("unless-trusted catalog approvals".to_string()), }; - for (reviewer, expected) in [ - (ApprovalsReviewer::User, "user catalog approvals"), + for (approval_policy, reviewer, expected) in [ ( + AskForApproval::OnRequest, + ApprovalsReviewer::User, + "user catalog approvals", + ), + ( + AskForApproval::OnRequest, ApprovalsReviewer::AutoReview, "auto-review catalog approvals", ), + ( + AskForApproval::Never, + ApprovalsReviewer::AutoReview, + "never catalog approvals", + ), + ( + AskForApproval::UnlessTrusted, + ApprovalsReviewer::AutoReview, + "unless-trusted catalog approvals", + ), ] { assert_eq!( approval_text( - AskForApproval::OnRequest, + approval_policy, reviewer, Some(&messages), &Policy::empty(), @@ -415,6 +432,8 @@ fn empty_catalog_approval_message_suppresses_legacy_approval_section() { let messages = ApprovalMessages { on_request: Some(String::new()), on_request_auto_review: None, + never: None, + unless_trusted: None, }; let mut exec_policy = Policy::empty(); exec_policy @@ -452,10 +471,12 @@ fn empty_catalog_approval_message_suppresses_legacy_approval_section() { } #[test] -fn missing_catalog_key_and_non_on_request_policy_use_legacy_approval_text() { +fn missing_catalog_key_uses_legacy_approval_text() { let messages = ApprovalMessages { on_request: None, on_request_auto_review: Some("unused catalog approvals".to_string()), + never: None, + unless_trusted: None, }; let on_request = approval_text( @@ -479,6 +500,30 @@ fn missing_catalog_key_and_non_on_request_policy_use_legacy_approval_text() { assert_eq!(never, APPROVAL_POLICY_NEVER); } +#[test] +fn empty_catalog_non_on_request_approval_messages_suppress_legacy_approval_text() { + let messages = ApprovalMessages { + on_request: None, + on_request_auto_review: None, + never: Some(String::new()), + unless_trusted: Some(String::new()), + }; + + for approval_policy in [AskForApproval::Never, AskForApproval::UnlessTrusted] { + assert_eq!( + approval_text( + approval_policy, + ApprovalsReviewer::AutoReview, + Some(&messages), + &Policy::empty(), + /*exec_permission_approvals_enabled*/ true, + /*request_permissions_tool_enabled*/ true, + ), + "" + ); + } +} + #[test] fn auto_review_approvals_append_auto_review_specific_guidance() { let text = approval_text( diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index b3523be33ee8..5f53bab0378b 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -515,6 +515,8 @@ pub struct ModelMessages { pub struct ApprovalMessages { pub on_request: Option, pub on_request_auto_review: Option, + pub never: Option, + pub unless_trusted: Option, } #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] @@ -765,7 +767,8 @@ mod tests { "instructions_template": null, "instructions_variables": null, "approvals": { - "on_request": "" + "on_request": "", + "never": "" } }"#, ) @@ -776,6 +779,8 @@ mod tests { Some(ApprovalMessages { on_request: Some(String::new()), on_request_auto_review: None, + never: Some(String::new()), + unless_trusted: None, }) ); }