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 f7622f9593..4a912ac4bf 100644 --- a/codex-rs/core/src/context/world_state/permissions_tests.rs +++ b/codex-rs/core/src/context/world_state/permissions_tests.rs @@ -268,6 +268,7 @@ fn permissions_state( danger_full_access: Some("Full access.".to_string()), workspace_write: Some("Workspace write.".to_string()), read_only: Some("Read only.".to_string()), + approved_prefixes: None, }; let mut model = model_info_from_slug("test-model"); model.model_messages = Some(ModelMessages { diff --git a/codex-rs/core/tests/suite/catalog_permission_messages.rs b/codex-rs/core/tests/suite/catalog_permission_messages.rs index ad985d46bf..77db765b63 100644 --- a/codex-rs/core/tests/suite/catalog_permission_messages.rs +++ b/codex-rs/core/tests/suite/catalog_permission_messages.rs @@ -5,6 +5,7 @@ use codex_login::CodexAuth; use codex_models_manager::manager::RefreshStrategy; use codex_models_manager::model_info::model_info_from_slug; use codex_protocol::models::PermissionProfile; +use codex_protocol::openai_models::ApprovalMessages; use codex_protocol::openai_models::ModelMessages; use codex_protocol::openai_models::ModelsResponse; use codex_protocol::openai_models::PermissionMessages; @@ -21,10 +22,18 @@ use core_test_support::skip_if_no_network; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use pretty_assertions::assert_eq; +use std::fs; +use test_case::test_case; use wiremock::MockServer; +#[test_case(Some("Catalog approval instructions."), "Catalog approved prefixes: {{ approved_prefixes }}", Some(r#"Catalog approved prefixes: - ["echo"]"#); "catalog_prefixes")] +#[test_case(None, "", None; "empty_catalog_prefixes")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn catalog_permission_message_loaded_from_remote_models_is_sent() -> Result<()> { +async fn catalog_permission_message_loaded_from_remote_models_is_sent( + catalog_approval: Option<&str>, + prefix_template: &str, + expected_prefix_line: Option<&str>, +) -> Result<()> { skip_if_no_network!(Ok(())); let server = MockServer::start().await; @@ -35,13 +44,19 @@ async fn catalog_permission_message_loaded_from_remote_models_is_sent() -> Resul persistent_instructions: None, tools: None, instructions_template: None, - approvals: None, + approvals: catalog_approval.map(|on_request| ApprovalMessages { + on_request: Some(on_request.to_string()), + on_request_auto_review: None, + never: None, + unless_trusted: None, + }), collaboration_modes: None, auto_review: None, permissions: Some(PermissionMessages { danger_full_access: None, workspace_write: None, read_only: Some("remote catalog permissions: {{ network_access }}".to_string()), + approved_prefixes: Some(prefix_template.to_string()), }), multi_agent: None, token_budget: None, @@ -63,7 +78,14 @@ async fn catalog_permission_message_loaded_from_remote_models_is_sent() -> Resul let mut builder = test_codex() .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) .with_config(|config| { - config.permissions.approval_policy = Constrained::allow_any(AskForApproval::Never); + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + let rules_dir = config.codex_home.join("rules"); + fs::create_dir_all(&rules_dir).expect("create rules directory"); + fs::write( + rules_dir.join("approved.rules"), + r#"prefix_rule(pattern=["echo"], decision="allow")"#, + ) + .expect("write approved prefix"); config .permissions .set_permission_profile(PermissionProfile::read_only()) @@ -105,12 +127,18 @@ async fn catalog_permission_message_loaded_from_remote_models_is_sent() -> Resul .filter(|text| text.contains("")) .map(|text| text.replace("\r\n", "\n")) .collect::>(); + let [permissions]: [String; 1] = permissions.try_into().expect("one permission fragment"); + assert!( + permissions + .starts_with("\nremote catalog permissions: restricted\n") + ); + assert!(permissions.contains(catalog_approval.unwrap_or("prefix_rule"))); assert_eq!( - permissions, - vec![ - "\nremote catalog permissions: restricted\nApproval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected.\n" - .to_string() - ] + permissions + .lines() + .filter(|line| line.contains(r#"["echo"]"#)) + .collect::>(), + expected_prefix_line.into_iter().collect::>() ); Ok(()) } diff --git a/codex-rs/core/tests/suite/permissions_messages.rs b/codex-rs/core/tests/suite/permissions_messages.rs index f3b0b0fab7..604fbc1ae0 100644 --- a/codex-rs/core/tests/suite/permissions_messages.rs +++ b/codex-rs/core/tests/suite/permissions_messages.rs @@ -253,6 +253,7 @@ async fn catalog_permission_message_is_sent_initially_and_after_model_change() - danger_full_access: None, workspace_write: None, read_only: Some("model A permissions: {{ network_access }}".to_string()), + approved_prefixes: None, }, ); let second = model_with_permission_messages( @@ -261,6 +262,7 @@ async fn catalog_permission_message_is_sent_initially_and_after_model_change() - danger_full_access: None, workspace_write: None, read_only: Some("model B permissions: {{ network_access }}".to_string()), + approved_prefixes: None, }, ); let mut builder = test_codex() @@ -326,6 +328,7 @@ async fn empty_catalog_permission_message_preserves_approval_instructions() -> R danger_full_access: None, workspace_write: None, read_only: Some(String::new()), + approved_prefixes: None, }, ); let mut builder = test_codex() diff --git a/codex-rs/core/tests/suite/settings_constraints.rs b/codex-rs/core/tests/suite/settings_constraints.rs index 3f4c32bf28..4cee6608ce 100644 --- a/codex-rs/core/tests/suite/settings_constraints.rs +++ b/codex-rs/core/tests/suite/settings_constraints.rs @@ -142,6 +142,7 @@ async fn protected_model_settings_use_the_proposed_permissions( danger_full_access: None, workspace_write: None, read_only: Some(PERMISSION_INSTRUCTIONS.to_string()), + approved_prefixes: None, }); let test = test_codex() .with_model(INITIAL_MODEL) diff --git a/codex-rs/models-manager/src/model_info_tests.rs b/codex-rs/models-manager/src/model_info_tests.rs index 9550d69615..44f5462a99 100644 --- a/codex-rs/models-manager/src/model_info_tests.rs +++ b/codex-rs/models-manager/src/model_info_tests.rs @@ -51,6 +51,7 @@ fn base_instruction_override_is_literal_and_preserves_catalog_messages() { danger_full_access: Some("danger".to_string()), workspace_write: Some(String::new()), read_only: None, + approved_prefixes: None, }; let multi_agent = MultiAgentMessages { role: Some(MultiAgentRoleMessages { diff --git a/codex-rs/prompts/src/model_messages/permissions.rs b/codex-rs/prompts/src/model_messages/permissions.rs index f3e6c1d776..264752c160 100644 --- a/codex-rs/prompts/src/model_messages/permissions.rs +++ b/codex-rs/prompts/src/model_messages/permissions.rs @@ -55,6 +55,7 @@ impl<'a> ResolvedApprovalMessages<'a> { /// Permission text retains its source so renderers can distinguish catalog text from templates. #[derive(Debug, Clone, Copy)] pub(crate) struct ResolvedPermissionMessages<'a> { + pub(crate) approved_prefixes: ResolvedMessage<'a>, pub(crate) danger_full_access: ResolvedMessage<'a>, pub(crate) workspace_write: ResolvedMessage<'a>, pub(crate) read_only: ResolvedMessage<'a>, @@ -63,6 +64,10 @@ pub(crate) struct ResolvedPermissionMessages<'a> { impl<'a> ResolvedPermissionMessages<'a> { pub(crate) fn new(messages: Option<&'a PermissionMessages>) -> Self { Self { + approved_prefixes: ResolvedMessage::new( + messages.and_then(|m| m.approved_prefixes.as_deref()), + include_str!("../../templates/permissions/approved_prefixes.md").trim_end(), + ), danger_full_access: ResolvedMessage::new( messages.and_then(|m| m.danger_full_access.as_deref()), DANGER_FULL_ACCESS_TEMPLATE, diff --git a/codex-rs/prompts/src/permissions_instructions.rs b/codex-rs/prompts/src/permissions_instructions.rs index de3be91884..b827006901 100644 --- a/codex-rs/prompts/src/permissions_instructions.rs +++ b/codex-rs/prompts/src/permissions_instructions.rs @@ -32,8 +32,6 @@ const REQUEST_PERMISSION_RULE: &str = include_str!("../templates/permissions/approval_policy/on_request_rule_request_permission.md"); const REQUEST_PERMISSIONS_TOOL: &str = "# request_permissions Tool\n\nThe built-in `request_permissions` tool is available in this session. Invoke it when you need to request additional `network` or `file_system` permissions before later shell-like commands need them. Request only the specific permissions required for the task."; const AUTO_REVIEW_SUFFIX: &str = "`approvals_reviewer` is `auto_review`: Sandbox escalations with require_escalated will be reviewed for compliance with the policy. If a rejection happens, you should proceed only with a materially safer alternative, or inform the user of the risk and send a final message to ask for approval."; -const APPROVED_PREFIXES: &str = - "## Approved command prefixes\nThe following prefix rules have already been approved: "; const GRANULAR_INTRO: &str = "# Approval Requests\n\nApproval policy is `granular`. Categories set to `false` are automatically rejected instead of prompting the user."; const GRANULAR_PROMPTED_CATEGORIES: &str = "These approval categories may still prompt the user when needed:"; @@ -135,8 +133,7 @@ impl PermissionsInstructions { &mut text, &approval_text( context.approval_policy, - approval_context.reviewer, - approval_context.messages, + approval_context, context.approved_command_prefixes, context.exec_permission_approvals_enabled, context.request_permissions_tool_enabled, @@ -288,14 +285,14 @@ fn append_section(text: &mut String, section: &str) { fn approval_text( approval_policy: AskForApproval, - approvals_reviewer: ApprovalsReviewer, - messages: ResolvedApprovalMessages<'_>, + approval_context: ApprovalPromptContext<'_>, approved_command_prefixes: &[Vec], exec_permission_approvals_enabled: bool, request_permissions_tool_enabled: bool, ) -> String { + let messages = approval_context.messages; let selected = match approval_policy { - AskForApproval::OnRequest => match approvals_reviewer { + AskForApproval::OnRequest => match approval_context.reviewer { ApprovalsReviewer::User => messages.on_request, ApprovalsReviewer::AutoReview => messages.on_request_auto_review, }, @@ -303,8 +300,25 @@ fn approval_text( AskForApproval::UnlessTrusted => messages.unless_trusted, AskForApproval::Granular(_) => ResolvedMessage::Bundled(GRANULAR_INTRO), }; + let prefix_template = approval_context.permission_messages.approved_prefixes; + let render_prefixes = + || approved_command_prefixes_text(approved_command_prefixes, prefix_template.text()); let base = match selected { - ResolvedMessage::Catalog(text) => return text.to_string(), + ResolvedMessage::Catalog(text) => { + // Catalog approval prose replaces the bundled section, but not an explicit + // approved-prefixes template supplied alongside it. + if approval_policy == AskForApproval::OnRequest + && matches!(prefix_template, ResolvedMessage::Catalog(_)) + && let Some(prefixes) = render_prefixes() + { + return if text.is_empty() { + prefixes + } else { + format!("{text}\n\n{prefixes}") + }; + } + return text.to_string(); + } ResolvedMessage::Bundled(text) => text, }; let text = match approval_policy { @@ -326,20 +340,18 @@ fn approval_text( if request_permissions_tool_enabled { sections.push(REQUEST_PERMISSIONS_TOOL.to_string()); } - if let Some(prefixes) = approved_command_prefixes_text(approved_command_prefixes) { - sections.push(format!("{APPROVED_PREFIXES}{prefixes}")); - } + sections.extend(render_prefixes()); sections.join("\n\n") } AskForApproval::Granular(granular_config) => granular_instructions( granular_config, - approved_command_prefixes, + render_prefixes(), exec_permission_approvals_enabled, request_permissions_tool_enabled, ), }; - if approvals_reviewer == ApprovalsReviewer::AutoReview { + if approval_context.reviewer == ApprovalsReviewer::AutoReview { format!("{text}\n\n{AUTO_REVIEW_SUFFIX}") } else { text @@ -397,14 +409,17 @@ fn denied_reads_text(paths: &[String], globs: &[String]) -> Option { )) } -fn approved_command_prefixes_text(approved_command_prefixes: &[Vec]) -> Option { - format_allow_prefixes(approved_command_prefixes.to_vec()) - .filter(|prefixes| !prefixes.is_empty()) +fn approved_command_prefixes_text(prefixes: &[Vec], template: &str) -> Option { + if template.is_empty() || prefixes.is_empty() { + return None; + } + let prefixes = format_allow_prefixes(prefixes.to_vec())?; + Some(template.replace("{{ approved_prefixes }}", &prefixes)) } fn granular_instructions( granular_config: GranularApprovalConfig, - approved_command_prefixes: &[Vec], + approved_command_prefixes: Option, exec_permission_approvals_enabled: bool, request_permissions_tool_enabled: bool, ) -> String { @@ -465,9 +480,7 @@ fn granular_instructions( sections.push(REQUEST_PERMISSIONS_TOOL.to_string()); } - if let Some(prefixes) = approved_command_prefixes_text(approved_command_prefixes) { - sections.push(format!("{APPROVED_PREFIXES}{prefixes}")); - } + sections.extend(approved_command_prefixes); sections.join("\n\n") } diff --git a/codex-rs/prompts/src/permissions_instructions_tests.rs b/codex-rs/prompts/src/permissions_instructions_tests.rs index 4c3c5070b0..eb5585688d 100644 --- a/codex-rs/prompts/src/permissions_instructions_tests.rs +++ b/codex-rs/prompts/src/permissions_instructions_tests.rs @@ -193,6 +193,7 @@ fn catalog_permission_messages_select_sandbox_mode_and_render_network_access() { danger_full_access: Some("catalog danger".to_string()), workspace_write: Some("catalog workspace {{ network_access }}".to_string()), read_only: Some("catalog read only {{ network_access }}".to_string()), + approved_prefixes: None, }; for (mode, expected) in [ @@ -217,6 +218,7 @@ fn catalog_permission_text_equal_to_bundled_template_preserves_literal_whitespac danger_full_access: None, workspace_write: Some(WORKSPACE_WRITE_TEMPLATE.to_string()), read_only: None, + approved_prefixes: None, }; let catalog_text = sandbox_text( SandboxMode::WorkspaceWrite, @@ -248,6 +250,7 @@ fn missing_catalog_permission_message_uses_legacy_sandbox_text() { danger_full_access: None, workspace_write: None, read_only: Some("unused".to_string()), + approved_prefixes: None, }; assert_eq!( @@ -267,6 +270,7 @@ fn invalid_catalog_permission_message_is_preserved_verbatim() { danger_full_access: None, workspace_write: Some(workspace_write.to_string()), read_only: None, + approved_prefixes: None, }; assert_eq!( sandbox_text( @@ -286,6 +290,7 @@ fn catalog_permission_message_renders_network_access_and_preserves_other_placeho danger_full_access: None, workspace_write: Some(source.to_string()), read_only: None, + approved_prefixes: None, }; assert_eq!( @@ -304,6 +309,7 @@ fn empty_catalog_permission_message_preserves_non_sandbox_sections() { danger_full_access: None, workspace_write: Some(String::new()), read_only: None, + approved_prefixes: None, }; let writable_root = "/tmp/repo".to_string(); let text = PermissionsInstructions::from_resolved( @@ -441,8 +447,11 @@ fn catalog_approval_messages_select_reviewer_variant() { assert_eq!( approval_text( approval_policy, - reviewer, - ResolvedApprovalMessages::new(Some(&messages)), + ApprovalPromptContext { + reviewer, + messages: ResolvedApprovalMessages::new(Some(&messages)), + ..user_approval_context() + }, &[], /*exec_permission_approvals_enabled*/ true, /*request_permissions_tool_enabled*/ true, @@ -511,19 +520,21 @@ fn missing_catalog_key_uses_legacy_approval_text() { }, ), ] { - let messages = ResolvedApprovalMessages::new(Some(&messages)); + let approval_context = ApprovalPromptContext { + reviewer, + messages: ResolvedApprovalMessages::new(Some(&messages)), + ..user_approval_context() + }; let on_request = approval_text( AskForApproval::OnRequest, - reviewer, - messages, + approval_context, &[], /*exec_permission_approvals_enabled*/ false, /*request_permissions_tool_enabled*/ false, ); let never = approval_text( AskForApproval::Never, - reviewer, - messages, + approval_context, &[], /*exec_permission_approvals_enabled*/ false, /*request_permissions_tool_enabled*/ false, @@ -555,8 +566,11 @@ fn empty_catalog_non_on_request_approval_messages_suppress_legacy_approval_text( assert_eq!( approval_text( approval_policy, - ApprovalsReviewer::AutoReview, - ResolvedApprovalMessages::new(Some(&messages)), + ApprovalPromptContext { + reviewer: ApprovalsReviewer::AutoReview, + messages: ResolvedApprovalMessages::new(Some(&messages)), + ..user_approval_context() + }, &[], /*exec_permission_approvals_enabled*/ true, /*request_permissions_tool_enabled*/ true, @@ -570,8 +584,10 @@ fn empty_catalog_non_on_request_approval_messages_suppress_legacy_approval_text( fn auto_review_approvals_append_auto_review_specific_guidance() { let text = approval_text( AskForApproval::OnRequest, - ApprovalsReviewer::AutoReview, - ResolvedApprovalMessages::new(/*messages*/ None), + ApprovalPromptContext::new( + ApprovalsReviewer::AutoReview, + ResolvedModelMessages::bundled(), + ), &[], /*exec_permission_approvals_enabled*/ false, /*request_permissions_tool_enabled*/ false, @@ -586,8 +602,10 @@ fn auto_review_approvals_append_auto_review_specific_guidance() { fn auto_review_approvals_omit_auto_review_specific_guidance_when_approval_is_never() { let text = approval_text( AskForApproval::Never, - ApprovalsReviewer::AutoReview, - ResolvedApprovalMessages::new(/*messages*/ None), + ApprovalPromptContext::new( + ApprovalsReviewer::AutoReview, + ResolvedModelMessages::bundled(), + ), &[], /*exec_permission_approvals_enabled*/ false, /*request_permissions_tool_enabled*/ false, @@ -622,8 +640,7 @@ fn granular_policy_lists_prompted_and_rejected_categories_separately() { request_permissions: true, mcp_elicitations: false, }), - ApprovalsReviewer::User, - ResolvedApprovalMessages::new(/*messages*/ None), + user_approval_context(), &[], /*exec_permission_approvals_enabled*/ true, /*request_permissions_tool_enabled*/ false, @@ -644,8 +661,7 @@ fn granular_policy_lists_prompted_and_rejected_categories_separately() { fn granular_policy_includes_command_permission_instructions_when_sandbox_approval_can_prompt() { let text = approval_text( AskForApproval::Granular(ALL_APPROVALS_ENABLED), - ApprovalsReviewer::User, - ResolvedApprovalMessages::new(/*messages*/ None), + user_approval_context(), &[], /*exec_permission_approvals_enabled*/ true, /*request_permissions_tool_enabled*/ false, @@ -666,8 +682,7 @@ fn granular_policy_includes_command_permission_instructions_when_sandbox_approva fn granular_policy_omits_shell_permission_instructions_when_inline_requests_are_disabled() { let text = approval_text( AskForApproval::Granular(ALL_APPROVALS_ENABLED), - ApprovalsReviewer::User, - ResolvedApprovalMessages::new(/*messages*/ None), + user_approval_context(), &[], /*exec_permission_approvals_enabled*/ false, /*request_permissions_tool_enabled*/ false, @@ -683,8 +698,7 @@ fn granular_policy_omits_shell_permission_instructions_when_inline_requests_are_ fn granular_policy_includes_request_permissions_tool_only_when_that_prompt_can_still_fire() { let allowed = approval_text( AskForApproval::Granular(ALL_APPROVALS_ENABLED), - ApprovalsReviewer::User, - ResolvedApprovalMessages::new(/*messages*/ None), + user_approval_context(), &[], /*exec_permission_approvals_enabled*/ true, /*request_permissions_tool_enabled*/ true, @@ -696,8 +710,7 @@ fn granular_policy_includes_request_permissions_tool_only_when_that_prompt_can_s request_permissions: false, ..ALL_APPROVALS_ENABLED }), - ApprovalsReviewer::User, - ResolvedApprovalMessages::new(/*messages*/ None), + user_approval_context(), &[], /*exec_permission_approvals_enabled*/ true, /*request_permissions_tool_enabled*/ true, @@ -720,6 +733,7 @@ fn preserves_supplied_path_spellings_and_order() { danger_full_access: None, workspace_write: Some(String::new()), read_only: None, + approved_prefixes: None, }; let instructions = PermissionsInstructions::from_resolved( diff --git a/codex-rs/prompts/templates/permissions/approved_prefixes.md b/codex-rs/prompts/templates/permissions/approved_prefixes.md new file mode 100644 index 0000000000..6693ed5fa2 --- /dev/null +++ b/codex-rs/prompts/templates/permissions/approved_prefixes.md @@ -0,0 +1,2 @@ +## Approved command prefixes +The following prefix rules have already been approved: {{ approved_prefixes }} diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index 45e3d5c096..d3d3dd8930 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -784,6 +784,10 @@ pub struct PermissionMessages { pub danger_full_access: Option, pub workspace_write: Option, pub read_only: Option, + /// Supports `{{ approved_prefixes }}`. Missing or null uses the bundled text; + /// an empty string suppresses the approved-prefix section. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub approved_prefixes: Option, } #[derive(Debug, Default, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] @@ -1310,6 +1314,7 @@ mod tests { danger_full_access: None, workspace_write: Some(String::new()), read_only: None, + approved_prefixes: None, }) ); } @@ -1554,6 +1559,7 @@ mod tests { danger_full_access: None, workspace_write: Some("workspace".to_string()), read_only: None, + approved_prefixes: None, }), multi_agent: None, token_budget: None,