From 133c11db436b89fe93ddfed56e00a46319abc864 Mon Sep 17 00:00:00 2001 From: goodman-oai Date: Tue, 27 Oct 2026 22:15:14 +0000 Subject: [PATCH] Allow model catalogs to customize approval instruction headings (#57581) ## What changed Add optional `request_permissions` and `auto_review_suffix` fields to catalog approval messages. Use them to customize the permission tool heading and introduce automatic review guidance while preserving the bundled instruction bodies and existing policy gates. Accept `request_permissions_tool` as a deserialization alias. Limit headings to 500 UTF-8 bytes on a single line and fall back to bundled text for missing or invalid values. Complete catalog approval messages retain precedence. ## Testing Add coverage for heading validation, serialization aliases, policy gates, fallback behavior, and complete-message precedence. Integration tests verify that model switches append the selected model's headings without rewriting history or repeating unchanged instructions. GitOrigin-RevId: 3311591a464cc086783fe25271a352dd5a229790 --- codex-rs/core/BUILD.bazel | 4 +- .../context/world_state/permissions_tests.rs | 1 + .../suite/catalog_permission_messages.rs | 1 + .../core/tests/suite/permissions_messages.rs | 138 +++++++++++++++-- codex-rs/core/tests/suite/scenarios.rs | 3 + ...ios_catalog_approval_instructions_tests.rs | 144 ++++++++++++++++++ .../core/tests/suite/settings_constraints.rs | 1 + ...proval_headings_follow_selected_model.snap | 140 +++++++++++++++++ codex-rs/core/tests/suite/step_settings.rs | 1 + .../models-manager/src/model_info_tests.rs | 2 + codex-rs/prompts/BUILD.bazel | 5 + .../prompts/src/model_messages/permissions.rs | 2 + .../prompts/src/permissions_instructions.rs | 23 +-- .../approval_instructions.rs | 59 +++++++ .../approval_instructions_tests.rs | 34 +++++ .../src/permissions_instructions_tests.rs | 79 ++++++++++ codex-rs/protocol/src/openai_models.rs | 44 +++++- 17 files changed, 658 insertions(+), 23 deletions(-) create mode 100644 codex-rs/core/tests/suite/scenarios_catalog_approval_instructions_tests.rs create mode 100644 codex-rs/core/tests/suite/snapshots/all__suite__scenarios__catalog_approval_instructions__catalog_approval_headings_follow_selected_model.snap create mode 100644 codex-rs/prompts/src/permissions_instructions/approval_instructions.rs create mode 100644 codex-rs/prompts/src/permissions_instructions/approval_instructions_tests.rs diff --git a/codex-rs/core/BUILD.bazel b/codex-rs/core/BUILD.bazel index 662d643ba0..54f8376a79 100644 --- a/codex-rs/core/BUILD.bazel +++ b/codex-rs/core/BUILD.bazel @@ -18,7 +18,9 @@ codex_rust_crate( "//codex-rs/windows-sandbox-rs:codex-windows-managed-deny-probe", "//codex-rs/windows-sandbox-rs:codex-windows-sandbox-setup", ], - integration_compile_data_extra = glob(["assets/**"]), + integration_compile_data_extra = glob(["assets/**"]) + [ + "//codex-rs/prompts:templates/permissions/approval_policy/on_request_rule_request_permission.md", + ], integration_test_timeout = "long", run_tests_with_wine_exec = True, rustc_env = { 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 0ea35c1b94..de9f1949c4 100644 --- a/codex-rs/core/src/context/world_state/permissions_tests.rs +++ b/codex-rs/core/src/context/world_state/permissions_tests.rs @@ -264,6 +264,7 @@ fn permissions_state( never: None, unless_trusted: None, granular: None, + ..Default::default() }; let permission_messages = PermissionMessages { danger_full_access: Some("Full access.".to_string()), diff --git a/codex-rs/core/tests/suite/catalog_permission_messages.rs b/codex-rs/core/tests/suite/catalog_permission_messages.rs index 5d007aeada..7e6d8adb66 100644 --- a/codex-rs/core/tests/suite/catalog_permission_messages.rs +++ b/codex-rs/core/tests/suite/catalog_permission_messages.rs @@ -50,6 +50,7 @@ async fn catalog_permission_message_loaded_from_remote_models_is_sent( never: None, unless_trusted: None, granular: None, + ..Default::default() }), collaboration_modes: None, auto_review: None, diff --git a/codex-rs/core/tests/suite/permissions_messages.rs b/codex-rs/core/tests/suite/permissions_messages.rs index 75cba3b93e..620630c429 100644 --- a/codex-rs/core/tests/suite/permissions_messages.rs +++ b/codex-rs/core/tests/suite/permissions_messages.rs @@ -4,7 +4,9 @@ use codex_core::ForkSnapshot; use codex_core::TurnInputRequest; use codex_core::config::Constrained; use codex_exec_server::CreateDirectoryOptions; +use codex_features::Feature; use codex_models_manager::model_info::model_info_from_slug; +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::PermissionProfile; use codex_protocol::openai_models::ApprovalMessages; use codex_protocol::openai_models::ModelMessages; @@ -35,6 +37,9 @@ use std::collections::HashSet; use tempfile::TempDir; use test_case::test_case; +const REQUEST_TOOL_BODY: &str = "The 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_BODY: &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."; + fn permissions_texts(request: &ResponsesRequest) -> Vec { request .message_input_texts("developer") @@ -87,9 +92,7 @@ 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, - granular: None, + ..Default::default() }), ..Default::default() }); @@ -162,6 +165,125 @@ async fn catalog_approval_message_is_sent_in_initial_permissions() -> Result<()> Ok(()) } +#[derive(Clone, Copy)] +enum ApprovalInstructionCase { + Present, + Absent, + Complete, +} + +#[test_case(ApprovalInstructionCase::Present; "present_fields_use_model_wording")] +#[test_case(ApprovalInstructionCase::Absent; "absent_fields_preserve_bundled_instructions")] +#[test_case(ApprovalInstructionCase::Complete; "complete_approval_message_takes_precedence")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn catalog_approval_instruction_parts_are_sent_in_initial_permissions( + case: ApprovalInstructionCase, +) -> Result<()> { + skip_if_no_network!(Ok(())); + + 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-approvals-model"; + let heading = + |value: &str| (!matches!(case, ApprovalInstructionCase::Absent)).then(|| value.to_string()); + let sandbox = PermissionMessages { + read_only: Some(String::new()), + ..Default::default() + }; + let mut model = model_info_from_slug(model_slug); + model.use_responses_lite = true; + model.model_messages = Some(ModelMessages { + approvals: Some(ApprovalMessages { + on_request_auto_review: matches!(case, ApprovalInstructionCase::Complete) + .then(|| "Complete catalog approval instructions".to_string()), + request_permissions: heading("Catalog permission tool"), + auto_review_suffix: heading("Catalog automatic review"), + ..Default::default() + }), + permissions: Some(sandbox.clone()), + ..Default::default() + }); + let fallback_slug = "catalog-approvals-fallback-model"; + let mut fallback_model = model_with_permission_messages(fallback_slug, sandbox); + fallback_model.use_responses_lite = true; + let mut builder = test_codex() + .with_model(model_slug) + .with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config + .permissions + .set_permission_profile(PermissionProfile::read_only()) + .expect("read-only permissions should be allowed"); + config.config_layer_stack = ConfigLayerStack::default(); + for (feature, value) in [ + (Feature::ExecPermissionApprovals, true), + (Feature::RequestPermissionsTool, true), + ] { + config + .features + .set_enabled(feature, value) + .expect("test feature setting should be allowed"); + } + config.model_catalog = Some(ModelsResponse { + models: vec![model, fallback_model], + }); + }); + let test = builder.build_with_auto_env(&server).await?; + submit_text_turn(&test, "hello").await?; + + let request = req.single_request(); + let bundled_rule = include_str!("../../../prompts/templates/permissions/approval_policy/on_request_rule_request_permission.md").replace("\r\n", "\n"); + let bundled_approval = format!( + "{bundled_rule}\n\n# request_permissions Tool\n\n{REQUEST_TOOL_BODY}\n\n{AUTO_REVIEW_BODY}" + ); + let expected = match case { + ApprovalInstructionCase::Complete => "Complete catalog approval instructions".to_string(), + ApprovalInstructionCase::Present => { + format!( + "{bundled_rule}\n\nCatalog permission tool\n\n{REQUEST_TOOL_BODY}\n\nCatalog automatic review\n\n{AUTO_REVIEW_BODY}" + ) + } + ApprovalInstructionCase::Absent => bundled_approval.clone(), + }; + let first = format!("\n{expected}\n"); + let annotation = ( + json!("developer"), + json!("permissions.instructions"), + json!({"provenance": {"type": "harness", "hook": false}}), + ); + assert_eq!(permissions_texts(&request), vec![first.clone()]); + assert_eq!(permissions_annotations(&request), vec![annotation.clone()]); + + if matches!(case, ApprovalInstructionCase::Present) { + let next = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-2"), ev_completed("resp-2")]), + ) + .await; + core_test_support::submit_thread_settings( + &test.codex, + ThreadSettingsOverrides { + model: Some(fallback_slug.to_string()), + ..Default::default() + }, + ) + .await?; + submit_text_turn(&test, "Continue with the fallback model.").await?; + let next = next.single_request(); + let fallback = + format!("\n{bundled_approval}\n"); + assert!(next.input().starts_with(&request.input())); + assert_eq!(permissions_texts(&next), vec![first, fallback]); + assert_eq!(permissions_annotations(&next), vec![annotation; 2]); + } + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn model_change_appends_new_catalog_approval_message() -> Result<()> { skip_if_no_network!(Ok(())); @@ -220,11 +342,8 @@ async fn catalog_non_on_request_approval_messages_are_sent_in_initial_permission ( AskForApproval::Never, ApprovalMessages { - on_request: None, - on_request_auto_review: None, never: Some("catalog never approval instructions".to_string()), - unless_trusted: None, - granular: None, + ..Default::default() }, "catalog never approval instructions", "Approval policy is currently never", @@ -232,11 +351,8 @@ async fn catalog_non_on_request_approval_messages_are_sent_in_initial_permission ( AskForApproval::UnlessTrusted, ApprovalMessages { - on_request: None, - on_request_auto_review: None, - never: None, unless_trusted: Some("catalog unless-trusted approval instructions".to_string()), - granular: None, + ..Default::default() }, "catalog unless-trusted approval instructions", "`approval_policy` is `unless-trusted`", diff --git a/codex-rs/core/tests/suite/scenarios.rs b/codex-rs/core/tests/suite/scenarios.rs index 3d7ec19cf9..dd78ae2725 100644 --- a/codex-rs/core/tests/suite/scenarios.rs +++ b/codex-rs/core/tests/suite/scenarios.rs @@ -116,6 +116,9 @@ mod incremental_tools_resume; #[path = "scenarios_model_instructions.rs"] mod model_instructions; +#[path = "scenarios_catalog_approval_instructions_tests.rs"] +mod catalog_approval_instructions; + #[path = "scenarios_context_management_tools.rs"] mod context_management_tools; diff --git a/codex-rs/core/tests/suite/scenarios_catalog_approval_instructions_tests.rs b/codex-rs/core/tests/suite/scenarios_catalog_approval_instructions_tests.rs new file mode 100644 index 0000000000..774fd899a5 --- /dev/null +++ b/codex-rs/core/tests/suite/scenarios_catalog_approval_instructions_tests.rs @@ -0,0 +1,144 @@ +//! Catalog approval headings follow model changes without rewriting or repeating context. + +#![cfg(test)] + +use super::*; +use codex_core::config::Constrained; +use codex_models_manager::model_info::model_info_from_slug; +use codex_protocol::config_types::ApprovalsReviewer; +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::protocol::AskForApproval; +use codex_protocol::protocol::ThreadSettingsOverrides; +use pretty_assertions::assert_eq; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn catalog_approval_headings_follow_selected_model() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let mock = mount_sse_sequence( + &server, + [ + ( + "response-1", + "I can inspect the workspace with the current permissions.", + ), + ( + "response-2", + "The selected model's approval instructions are now active.", + ), + ("response-3", "The permissions are unchanged."), + ] + .into_iter() + .map(|(id, text)| sse(vec![ev_assistant_message(id, text), ev_completed(id)])) + .collect(), + ) + .await; + let first_slug = "catalog-approval-headings-a"; + let second_slug = "catalog-approval-headings-b"; + let models = [(first_slug, "Model A"), (second_slug, "Model B")] + .into_iter() + .map(|(slug, label)| { + let mut model = model_info_from_slug(slug); + model.use_responses_lite = true; + model.model_messages = Some(ModelMessages { + approvals: Some(ApprovalMessages { + request_permissions: Some(format!("# {label} permission requests")), + auto_review_suffix: Some(format!("# {label} automatic review")), + ..Default::default() + }), + ..Default::default() + }); + model + }) + .collect(); + let test = test_codex() + .with_model(first_slug) + .with_config(move |config| { + configure_scenario_catalog(config); + config.workspace_roots = vec![config.cwd.clone()]; + config.base_instructions = Some("Help with the workspace.".to_string()); + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config + .permissions + .set_permission_profile(PermissionProfile::read_only()) + .expect("read-only permissions should be allowed"); + for feature in [ + Feature::ExecPermissionApprovals, + Feature::RequestPermissionsTool, + ] { + config + .features + .enable(feature) + .expect("enable permission requests"); + } + config.model_catalog = Some(ModelsResponse { models }); + }) + .build_with_auto_env(&server) + .await?; + + test.submit_text_turn("Inspect the workspace permissions.") + .await?; + core_test_support::submit_thread_settings( + &test.codex, + ThreadSettingsOverrides { + model: Some(second_slug.to_string()), + ..Default::default() + }, + ) + .await?; + test.submit_text_turn("Continue with the selected model.") + .await?; + test.submit_text_turn("Continue without changing permissions.") + .await?; + + let requests = mock.requests(); + assert_eq!(requests.len(), 3); + let headings = requests + .iter() + .map(|request| { + request + .message_input_texts("developer") + .into_iter() + .filter(|text| text.contains("")) + .map(|text| { + text.lines() + .filter(|line| line.starts_with("# Model ")) + .map(str::to_string) + .collect::>() + }) + .collect::>() + }) + .collect::>(); + let first_headings = vec![ + "# Model A permission requests", + "# Model A automatic review", + ]; + let second_headings = vec![ + "# Model B permission requests", + "# Model B automatic review", + ]; + assert_eq!( + headings, + vec![ + vec![first_headings.clone()], + vec![first_headings.clone(), second_headings.clone()], + vec![first_headings, second_headings], + ] + ); + for pair in requests.windows(2) { + let previous = pair[0].input(); + let next = pair[1].input(); + assert_eq!(&next[..previous.len()], previous); + } + insta::assert_snapshot!(context_snapshot::format_request_history_snapshot( + "Catalog permission-request and automatic-review headings follow the selected model. The switch appends new developer instructions after the prior reply and before the next user message; an unchanged follow-up preserves the full history without repeating instructions.", + &requests, + &ContextSnapshotOptions::default().include_request_settings(), + )); + Ok(()) +} diff --git a/codex-rs/core/tests/suite/settings_constraints.rs b/codex-rs/core/tests/suite/settings_constraints.rs index a45c00416d..f32e21888c 100644 --- a/codex-rs/core/tests/suite/settings_constraints.rs +++ b/codex-rs/core/tests/suite/settings_constraints.rs @@ -138,6 +138,7 @@ async fn protected_model_settings_use_the_proposed_permissions( never: None, unless_trusted: None, granular: None, + ..Default::default() }); messages.permissions = Some(PermissionMessages { danger_full_access: None, diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__catalog_approval_instructions__catalog_approval_headings_follow_selected_model.snap b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__catalog_approval_instructions__catalog_approval_headings_follow_selected_model.snap new file mode 100644 index 0000000000..6d8a3b8ae0 --- /dev/null +++ b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__catalog_approval_instructions__catalog_approval_headings_follow_selected_model.snap @@ -0,0 +1,140 @@ +--- +source: core/tests/suite/scenarios_catalog_approval_instructions_tests.rs +assertion_line: 138 +expression: "context_snapshot::format_request_history_snapshot(\"Catalog permission-request and automatic-review headings follow the selected model. The switch appends new developer instructions after the prior reply and before the next user message; an unchanged follow-up preserves the full history without repeating instructions.\",\n&requests, &ContextSnapshotOptions::default().include_request_settings(),)" +--- +Scenario: Catalog permission-request and automatic-review headings follow the selected model. The switch appends new developer instructions after the prior reply and before the next user message; an unchanged follow-up preserves the full history without repeating instructions. + +## Window 1 +Settings: + include: ["reasoning.encrypted_content"] + model: "catalog-approval-headings-a" + parallel_tool_calls: false + prompt_cache_key: "" + reasoning: {"context":"all_turns","summary":"auto"} + store: false + stream: true + tool_choice: "auto" +-- request 1 (turn) -- +00:additional_tools/developer (2; hash=23179252be1abe5f): + - namespace/functions; hash=9962be1e4c2d492b + - function/exec_command + - function/write_stdin + - function/request_user_input + - function/request_permissions + - function/view_image + - namespace/multi_agent_v1: Tools for spawning and managing sub-agents.; hash=c826e5b07fd2820f + - function/close_agent + - function/resume_agent + - function/send_input + - function/spawn_agent + - function/wait_agent +01:message/developer: + Help with the workspace. +02:message/developer: + + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `read-only`: The san... permits reading files. Network access is restricted. [hash=be7da4425f89053c] + # Permission Requests + + Commands may require user approval before execution. Prefer requesting sandboxed additional permissions instead of asking to run fully outside the sandbox. + + ## Preferred request mode + + + # Model A permission requests + + The built-in `request_permissions` tool is available in this session. Invoke it when you need to request... only the specific permissions required for the task. [hash=a29255aad0722385] + + # Model A automatic review + + `approvals_reviewer` is `auto_review`: Sandbox escalations with require_escalated will be reviewed for c...he risk and send a final message to ask for approval. [hash=c592d119d24cd2d6] + +03:message/user: + + + + + + [hash=e8c1de60307d0a3d] + +04:message/user: + Inspect the workspace permissions. + +## Window 2 (after request 1) +Settings: relative to window 1 + model: "catalog-approval-headings-b" +-- request 2 (turn) -- +00:additional_tools/developer (2; hash=23179252be1abe5f): + - namespace/functions; hash=9962be1e4c2d492b + - function/exec_command + - function/write_stdin + - function/request_user_input + - function/request_permissions + - function/view_image + - namespace/multi_agent_v1: Tools for spawning and managing sub-agents.; hash=c826e5b07fd2820f + - function/close_agent + - function/resume_agent + - function/send_input + - function/spawn_agent + - function/wait_agent +01:message/developer: + Help with the workspace. +02:message/developer: + + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `read-only`: The san... permits reading files. Network access is restricted. [hash=be7da4425f89053c] + # Permission Requests + + Commands may require user approval before execution. Prefer requesting sandboxed additional permissions instead of asking to run fully outside the sandbox. + + ## Preferred request mode + + + # Model A permission requests + + The built-in `request_permissions` tool is available in this session. Invoke it when you need to request... only the specific permissions required for the task. [hash=a29255aad0722385] + + # Model A automatic review + + `approvals_reviewer` is `auto_review`: Sandbox escalations with require_escalated will be reviewed for c...he risk and send a final message to ask for approval. [hash=c592d119d24cd2d6] + +03:message/user: + + + + + + [hash=e8c1de60307d0a3d] + +04:message/user: + Inspect the workspace permissions. +05:message/assistant: + I can inspect the workspace with the current permissions. +06:message/developer: + + The current model is catalog-approval-headings-b. + +07:message/developer: + + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `read-only`: The san... permits reading files. Network access is restricted. [hash=be7da4425f89053c] + # Permission Requests + + Commands may require user approval before execution. Prefer requesting sandboxed additional permissions instead of asking to run fully outside the sandbox. + + ## Preferred request mode + + + # Model B permission requests + + The built-in `request_permissions` tool is available in this session. Invoke it when you need to request... only the specific permissions required for the task. [hash=a29255aad0722385] + + # Model B automatic review + + `approvals_reviewer` is `auto_review`: Sandbox escalations with require_escalated will be reviewed for c...he risk and send a final message to ask for approval. [hash=c592d119d24cd2d6] + +08:message/user: + Continue with the selected model. +-- request 3 (turn) -- +09:message/assistant: + The selected model's approval instructions are now active. +10:message/user: + Continue without changing permissions. diff --git a/codex-rs/core/tests/suite/step_settings.rs b/codex-rs/core/tests/suite/step_settings.rs index a6303fdec3..15394819dc 100644 --- a/codex-rs/core/tests/suite/step_settings.rs +++ b/codex-rs/core/tests/suite/step_settings.rs @@ -1104,6 +1104,7 @@ async fn active_model_switch_updates_core_context_from_captured_settings( never: None, unless_trusted: None, granular: None, + ..Default::default() }); messages.token_budget = (initial_model || destination_has_guidance).then(|| ModelTokenBudgetConfig { diff --git a/codex-rs/models-manager/src/model_info_tests.rs b/codex-rs/models-manager/src/model_info_tests.rs index 0f83d82539..658a0d41bc 100644 --- a/codex-rs/models-manager/src/model_info_tests.rs +++ b/codex-rs/models-manager/src/model_info_tests.rs @@ -41,6 +41,7 @@ fn base_instruction_override_is_literal_and_preserves_catalog_messages() { prompted_categories: Some("Allowed: {{ categories }}".to_string()), rejected_categories: Some("Denied: {{ categories }}".to_string()), }), + ..Default::default() }; let collaboration_modes = CollaborationModeMessages { default: Some("default instructions".to_string()), @@ -174,6 +175,7 @@ fn personality_none_strips_catalog_instruction_sources_through_the_next_h1() { never: None, unless_trusted: None, granular: None, + ..Default::default() }), ..Default::default() }; diff --git a/codex-rs/prompts/BUILD.bazel b/codex-rs/prompts/BUILD.bazel index 4689baafab..aeead87aeb 100644 --- a/codex-rs/prompts/BUILD.bazel +++ b/codex-rs/prompts/BUILD.bazel @@ -1,5 +1,10 @@ load("//:defs.bzl", "codex_rust_crate") +exports_files( + ["templates/permissions/approval_policy/on_request_rule_request_permission.md"], + visibility = ["//codex-rs/core:__pkg__"], +) + codex_rust_crate( name = "prompts", compile_data = glob(["templates/**"]), diff --git a/codex-rs/prompts/src/model_messages/permissions.rs b/codex-rs/prompts/src/model_messages/permissions.rs index 559f5b18ef..edb664d7ea 100644 --- a/codex-rs/prompts/src/model_messages/permissions.rs +++ b/codex-rs/prompts/src/model_messages/permissions.rs @@ -26,6 +26,7 @@ pub(crate) const DENIED_READS_TEMPLATE: &str = "## Denied filesystem reads\n{{ d /// Resolved approval-policy text; policy and tool selection remain with the consumer. #[derive(Debug, Clone, Copy)] pub(crate) struct ResolvedApprovalMessages<'a> { + pub(crate) catalog: Option<&'a ApprovalMessages>, pub(crate) never: ResolvedMessage<'a>, pub(crate) on_request: ResolvedMessage<'a>, pub(crate) on_request_auto_review: ResolvedMessage<'a>, @@ -35,6 +36,7 @@ pub(crate) struct ResolvedApprovalMessages<'a> { impl<'a> ResolvedApprovalMessages<'a> { pub(crate) fn new(messages: Option<&'a ApprovalMessages>) -> Self { Self { + catalog: messages, never: ResolvedMessage::new( messages.and_then(|m| m.never.as_deref()), APPROVAL_POLICY_NEVER, diff --git a/codex-rs/prompts/src/permissions_instructions.rs b/codex-rs/prompts/src/permissions_instructions.rs index 81f8c2bfb8..54fd9e4406 100644 --- a/codex-rs/prompts/src/permissions_instructions.rs +++ b/codex-rs/prompts/src/permissions_instructions.rs @@ -10,6 +10,7 @@ use crate::model_messages::permissions::READ_ONLY_TEMPLATE; use crate::model_messages::permissions::ResolvedApprovalMessages; use crate::model_messages::permissions::ResolvedPermissionMessages; use crate::model_messages::permissions::WORKSPACE_WRITE_TEMPLATE; +use approval_instructions::ApprovalInstructions; use codex_context_fragments::ContextualUserFragment; use codex_execpolicy::Policy; use codex_protocol::config_types::ApprovalsReviewer; @@ -32,6 +33,7 @@ use std::path::Path; use std::sync::LazyLock; use writable_roots::writable_roots_text; +mod approval_instructions; mod path_warnings; mod writable_roots; @@ -332,30 +334,31 @@ fn approval_text( } ResolvedMessage::Bundled(text) => text, }; + let instructions = ApprovalInstructions::new(approval_context); let text = match approval_policy { AskForApproval::Never => return base.to_string(), AskForApproval::UnlessTrusted => { if request_permissions_tool_enabled { - format!("{base}\n\n{REQUEST_PERMISSIONS_TOOL}") + format!("{base}\n\n{}", instructions.tool()) } else { base.to_string() } } AskForApproval::OnRequest => { - let rule = if exec_permission_approvals_enabled { - REQUEST_PERMISSION_RULE + let mut sections = vec![if exec_permission_approvals_enabled { + REQUEST_PERMISSION_RULE.to_string() } else { - base - }; - let mut sections = vec![rule.to_string()]; + base.to_string() + }]; if request_permissions_tool_enabled { - sections.push(REQUEST_PERMISSIONS_TOOL.to_string()); + sections.push(instructions.tool()); } sections.extend(render_prefixes()); sections.join("\n\n") } AskForApproval::Granular(granular_config) => granular_instructions( granular_config, + approval_context, render_prefixes(), exec_permission_approvals_enabled, request_permissions_tool_enabled, @@ -363,7 +366,7 @@ fn approval_text( }; if approval_context.reviewer == ApprovalsReviewer::AutoReview { - format!("{text}\n\n{AUTO_REVIEW_SUFFIX}") + format!("{text}\n\n{}", instructions.auto_review()) } else { text } @@ -398,10 +401,12 @@ fn approved_command_prefixes_text(prefixes: &[Vec], template: &str) -> O fn granular_instructions( granular_config: GranularApprovalConfig, + approval_context: ApprovalPromptContext<'_>, approved_command_prefixes: Option, exec_permission_approvals_enabled: bool, request_permissions_tool_enabled: bool, ) -> String { + let instructions = ApprovalInstructions::new(approval_context); let sandbox_approval_prompts_allowed = granular_config.allows_sandbox_approval(); let shell_permission_requests_available = exec_permission_approvals_enabled && sandbox_approval_prompts_allowed; @@ -456,7 +461,7 @@ fn granular_instructions( } if request_permissions_tool_prompts_allowed { - sections.push(REQUEST_PERMISSIONS_TOOL.to_string()); + sections.push(instructions.tool()); } sections.extend(approved_command_prefixes); diff --git a/codex-rs/prompts/src/permissions_instructions/approval_instructions.rs b/codex-rs/prompts/src/permissions_instructions/approval_instructions.rs new file mode 100644 index 0000000000..6de0dd000e --- /dev/null +++ b/codex-rs/prompts/src/permissions_instructions/approval_instructions.rs @@ -0,0 +1,59 @@ +//! Selects bounded catalog headings while preserving all permission and review guidance. + +use super::AUTO_REVIEW_SUFFIX; +use super::ApprovalPromptContext; +use super::PermissionsInstructions; +use super::REQUEST_PERMISSIONS_TOOL; +use codex_context_fragments::ContextualUserFragment; +use codex_protocol::openai_models::ApprovalMessages; + +pub(super) struct ApprovalInstructions<'a>(Option<&'a ApprovalMessages>); + +impl<'a> ApprovalInstructions<'a> { + pub(super) fn new(context: ApprovalPromptContext<'a>) -> Self { + Self(context.messages.catalog) + } + + pub(super) fn tool(&self) -> String { + with_heading( + self.0.and_then(|m| m.request_permissions.as_deref()), + REQUEST_PERMISSIONS_TOOL, + ) + } + + pub(super) fn auto_review(&self) -> String { + match self + .0 + .and_then(|m| m.auto_review_suffix.as_deref()) + .filter(|s| valid_heading(s)) + { + Some(heading) => format!("{heading}\n\n{AUTO_REVIEW_SUFFIX}"), + None => AUTO_REVIEW_SUFFIX.to_owned(), + } + } +} + +fn with_heading(heading: Option<&str>, bundled: &str) -> String { + match ( + heading.filter(|s| valid_heading(s)), + bundled.find(['\r', '\n']), + ) { + (Some(heading), Some(end)) => format!("{heading}{}", &bundled[end..]), + _ => bundled.to_owned(), + } +} + +// One single-line heading can add at most 500 UTF-8 bytes plus its paragraph separator. +fn valid_heading(text: &str) -> bool { + let (open, close) = PermissionsInstructions::type_markers(); + text.len() <= 500 + && !text.trim().is_empty() + && !text.chars().any(char::is_control) + && !text.contains(['{', '}', '\u{2028}', '\u{2029}']) + && !text.contains(open) + && !text.contains(close) +} + +#[cfg(test)] +#[path = "approval_instructions_tests.rs"] +mod tests; diff --git a/codex-rs/prompts/src/permissions_instructions/approval_instructions_tests.rs b/codex-rs/prompts/src/permissions_instructions/approval_instructions_tests.rs new file mode 100644 index 0000000000..9999951d8a --- /dev/null +++ b/codex-rs/prompts/src/permissions_instructions/approval_instructions_tests.rs @@ -0,0 +1,34 @@ +//! Verifies catalog heading bounds and preservation of bundled instruction line endings. + +#![cfg(test)] + +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn replacing_heading_preserves_the_bundled_line_endings() { + let bundled = "Heading\r\n\r\nRequired instructions.\r\n"; + assert_eq!( + with_heading(Some("Catalog"), bundled), + "Catalog\r\n\r\nRequired instructions.\r\n" + ); +} + +#[test] +fn accepts_bounded_heading_and_rejects_malformed_presentation() { + let boundary = "é".repeat(250); + assert!(valid_heading(&boundary)); + let (open, close) = PermissionsInstructions::type_markers(); + for invalid in [ + "", + " ", + &format!("{boundary}a"), + open, + close, + "{{ policy }}", + "title\nbody", + "title\u{2028}body", + ] { + assert!(!valid_heading(invalid), "accepted {invalid:?}"); + } +} diff --git a/codex-rs/prompts/src/permissions_instructions_tests.rs b/codex-rs/prompts/src/permissions_instructions_tests.rs index 218ae8c15d..5a55533023 100644 --- a/codex-rs/prompts/src/permissions_instructions_tests.rs +++ b/codex-rs/prompts/src/permissions_instructions_tests.rs @@ -427,6 +427,7 @@ fn catalog_approval_messages_select_reviewer_variant() { never: Some("never catalog approvals".to_string()), unless_trusted: Some("unless-trusted catalog approvals".to_string()), granular: None, + ..Default::default() }; for (approval_policy, reviewer, expected) in [ @@ -476,6 +477,7 @@ fn empty_catalog_approval_message_suppresses_legacy_approval_section() { never: None, unless_trusted: None, granular: None, + ..Default::default() }; let approved_command_prefixes = vec![vec!["git".to_string(), "pull".to_string()]]; let writable_root = "/tmp/repo".to_string(); @@ -517,6 +519,7 @@ fn missing_catalog_key_uses_legacy_approval_text() { never: None, unless_trusted: None, granular: None, + ..Default::default() }, ), ( @@ -527,6 +530,7 @@ fn missing_catalog_key_uses_legacy_approval_text() { never: None, unless_trusted: None, granular: None, + ..Default::default() }, ), ] { @@ -571,6 +575,7 @@ fn empty_catalog_non_on_request_approval_messages_suppress_legacy_approval_text( never: Some(String::new()), unless_trusted: Some(String::new()), granular: None, + ..Default::default() }; for approval_policy in [AskForApproval::Never, AskForApproval::UnlessTrusted] { @@ -641,6 +646,79 @@ const ALL_PROMPTED_CATEGORIES: &str = - `skill_approval` - `mcp_elicitations`"; +#[test] +fn granular_approval_headings_follow_independent_tool_and_sandbox_gates() { + let messages = ApprovalMessages { + request_permissions: Some("Catalog permission tool".into()), + auto_review_suffix: Some("Catalog reviewer".into()), + ..Default::default() + }; + let context = ApprovalPromptContext { + messages: ResolvedApprovalMessages::new(Some(&messages)), + ..user_approval_context() + }; + for (sandbox_allowed, tool_allowed) in + [(true, false), (false, true), (true, true), (false, false)] + { + let text = approval_text( + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: sandbox_allowed, + request_permissions: tool_allowed, + ..ALL_APPROVALS_ENABLED + }), + context, + &[], + /*exec_permission_approvals_enabled*/ true, + /*request_permissions_tool_enabled*/ true, + ); + assert_eq!(text.contains(REQUEST_PERMISSION_RULE), sandbox_allowed); + assert_eq!(text.contains("Catalog permission tool"), tool_allowed); + assert!(!text.contains("Catalog reviewer")); + } +} + +#[test] +fn catalog_heading_fallback_and_policy_selection_are_independent() { + let messages = ApprovalMessages { + request_permissions: Some("Catalog permission tool".into()), + auto_review_suffix: Some("{{ unknown }}".into()), + ..Default::default() + }; + let bundled = ApprovalPromptContext::new( + ApprovalsReviewer::AutoReview, + ResolvedModelMessages::bundled(), + ); + let catalog = ApprovalPromptContext { + messages: ResolvedApprovalMessages::new(Some(&messages)), + ..bundled + }; + for policy in [ + AskForApproval::Never, + AskForApproval::UnlessTrusted, + AskForApproval::OnRequest, + ] { + for tool_enabled in [false, true] { + let render = |context| { + approval_text( + policy, + context, + &[], + /*exec_permission_approvals_enabled*/ true, + tool_enabled, + ) + }; + let default = render(bundled); + let expected = if tool_enabled && policy != AskForApproval::Never { + assert!(default.contains("# request_permissions Tool")); + default.replace("# request_permissions Tool", "Catalog permission tool") + } else { + default + }; + assert_eq!(render(catalog), expected); + } + } +} + #[test] fn granular_policy_lists_prompted_and_rejected_categories_separately() { let text = approval_text( @@ -740,6 +818,7 @@ fn preserves_supplied_path_spellings_and_order() { never: None, unless_trusted: None, granular: None, + ..Default::default() }; let permission_messages = PermissionMessages { danger_full_access: None, diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index 3a2616934a..8d6dffac09 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -769,12 +769,22 @@ pub struct ModelTokenBudgetConfig { pub auto_compact_fallback_buffer_tokens: i64, } -#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] +#[derive(Debug, Default, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] pub struct ApprovalMessages { pub on_request: Option, pub on_request_auto_review: Option, pub never: Option, pub unless_trusted: Option, + /// Optional replacement heading for the request_permissions tool instructions. + #[serde( + default, + alias = "request_permissions_tool", + skip_serializing_if = "Option::is_none" + )] + pub request_permissions: Option, + /// Optional introduction to the harness-owned automatic escalation review policy. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub auto_review_suffix: Option, /// Optional catalog templates for harness-owned granular approval facts. #[serde(default, skip_serializing_if = "Option::is_none")] pub granular: Option, @@ -1285,7 +1295,9 @@ mod tests { "instructions_template": null, "approvals": { "on_request": "", - "never": "" + "never": "", + "request_permissions": "", + "auto_review_suffix": null } }"#, ) @@ -1295,13 +1307,40 @@ mod tests { messages.approvals, Some(ApprovalMessages { on_request: Some(String::new()), + request_permissions: Some(String::new()), on_request_auto_review: None, never: Some(String::new()), unless_trusted: None, granular: None, + ..Default::default() }) ); + for field in ["request_permissions", "request_permissions_tool"] { + for heading in [ + serde_json::Value::Null, + serde_json::json!(""), + serde_json::json!("Permission requests"), + ] { + let approvals: ApprovalMessages = + serde_json::from_value(serde_json::json!({ field: heading })) + .expect("permission heading should deserialize with either field name"); + let mut expected = serde_json::json!({ + "on_request": null, + "on_request_auto_review": null, + "never": null, + "unless_trusted": null, + }); + if !heading.is_null() { + expected["request_permissions"] = heading; + } + assert_eq!( + serde_json::to_value(approvals).expect("serialize approvals"), + expected + ); + } + } + for (granular, expected) in [ (serde_json::json!(null), None), (serde_json::json!({}), Some(serde_json::json!({}))), @@ -1674,6 +1713,7 @@ mod tests { prompted_categories: Some("Allowed: {{ categories }}".to_string()), rejected_categories: Some("Denied: {{ categories }}".to_string()), }), + ..Default::default() }), collaboration_modes: Some(CollaborationModeMessages { default: Some("default collaboration".to_string()),