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 4a912ac4bf..6d898123fe 100644 --- a/codex-rs/core/src/context/world_state/permissions_tests.rs +++ b/codex-rs/core/src/context/world_state/permissions_tests.rs @@ -269,6 +269,7 @@ fn permissions_state( workspace_write: Some("Workspace write.".to_string()), read_only: Some("Read only.".to_string()), approved_prefixes: None, + ..PermissionMessages::default() }; let mut model = model_info_from_slug("test-model"); model.model_messages = Some(ModelMessages { diff --git a/codex-rs/core/src/session/world_state.rs b/codex-rs/core/src/session/world_state.rs index 550fd7e4db..41f9faf607 100644 --- a/codex-rs/core/src/session/world_state.rs +++ b/codex-rs/core/src/session/world_state.rs @@ -236,10 +236,12 @@ impl Session { let cwd = environment .and_then(|environment| environment.cwd().to_abs_path().ok()) .unwrap_or_else(|| turn_context.cwd.clone()); + let approval_context = + ApprovalPromptContext::new(settings.approvals_reviewer(), model_messages); world_state.add_section(PermissionsState::new( &permission_profile, settings.approval_policy(), - ApprovalPromptContext::new(settings.approvals_reviewer(), model_messages), + approval_context, exec_policy.as_ref(), &cwd, paths.as_ref(), diff --git a/codex-rs/core/tests/suite/catalog_permission_messages.rs b/codex-rs/core/tests/suite/catalog_permission_messages.rs index 77db765b63..bf1a613c45 100644 --- a/codex-rs/core/tests/suite/catalog_permission_messages.rs +++ b/codex-rs/core/tests/suite/catalog_permission_messages.rs @@ -57,6 +57,7 @@ async fn catalog_permission_message_loaded_from_remote_models_is_sent( workspace_write: None, read_only: Some("remote catalog permissions: {{ network_access }}".to_string()), approved_prefixes: Some(prefix_template.to_string()), + ..PermissionMessages::default() }), multi_agent: None, token_budget: None, diff --git a/codex-rs/core/tests/suite/permissions_messages.rs b/codex-rs/core/tests/suite/permissions_messages.rs index 604fbc1ae0..5fc67ec512 100644 --- a/codex-rs/core/tests/suite/permissions_messages.rs +++ b/codex-rs/core/tests/suite/permissions_messages.rs @@ -9,6 +9,8 @@ use codex_protocol::openai_models::ApprovalMessages; use codex_protocol::openai_models::ModelMessages; use codex_protocol::openai_models::ModelsResponse; use codex_protocol::openai_models::PermissionMessages; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; @@ -25,14 +27,17 @@ 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 serde_json::json; use std::collections::HashSet; use tempfile::TempDir; +use test_case::test_case; fn permissions_texts(request: &ResponsesRequest) -> Vec { request .message_input_texts("developer") .into_iter() .filter(|text| text.contains("")) + .map(|text| text.replace("\r\n", "\n")) .collect() } @@ -254,6 +259,8 @@ async fn catalog_permission_message_is_sent_initially_and_after_model_change() - workspace_write: None, read_only: Some("model A permissions: {{ network_access }}".to_string()), approved_prefixes: None, + writable_root: None, + writable_roots: None, }, ); let second = model_with_permission_messages( @@ -263,6 +270,8 @@ async fn catalog_permission_message_is_sent_initially_and_after_model_change() - workspace_write: None, read_only: Some("model B permissions: {{ network_access }}".to_string()), approved_prefixes: None, + writable_root: None, + writable_roots: None, }, ); let mut builder = test_codex() @@ -329,6 +338,8 @@ async fn empty_catalog_permission_message_preserves_approval_instructions() -> R workspace_write: None, read_only: Some(String::new()), approved_prefixes: None, + writable_root: None, + writable_roots: None, }, ); let mut builder = test_codex() @@ -738,8 +749,50 @@ async fn resume_and_fork_append_permissions_messages() -> Result<()> { Ok(()) } +const WORKSPACE_PERMISSIONS_PREFIX: &str = concat!( + "\n", + "Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `workspace-write`: The sandbox permits reading files, and editing files in `cwd` and `writable_roots`. Editing files in other directories requires approval. Network access is restricted.\n", + "Approval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected.\n", +); + +#[test_case( + 1, + PermissionMessages::default(), + " The writable root is {roots}."; + "default singular" +)] +#[test_case( + 2, + PermissionMessages::default(), + " The writable roots are {roots}."; + "default plural" +)] +#[test_case( + 1, + PermissionMessages { + writable_root: Some("Catalog single: {{ roots }}!".to_string()), + writable_roots: Some("Catalog plural: {{ roots }}!".to_string()), + ..Default::default() + }, + "Catalog single: {roots}!"; + "override singular" +)] +#[test_case( + 2, + PermissionMessages { + writable_root: Some("Catalog single: {{ roots }}!".to_string()), + writable_roots: Some("Catalog plural: {{ roots }}!".to_string()), + ..Default::default() + }, + "Catalog plural: {roots}!"; + "override plural" +)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn permissions_message_includes_writable_roots() -> Result<()> { +async fn permissions_message_includes_writable_roots( + root_count: usize, + permissions: PermissionMessages, + expected_root_sentence: &str, +) -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -750,53 +803,218 @@ async fn permissions_message_includes_writable_roots() -> Result<()> { .await; let writable = TempDir::new()?; let writable_root = AbsolutePathBuf::try_from(writable.path().canonicalize()?)?; - let writable_root_for_config = writable_root.clone(); - let permission_profile = PermissionProfile::workspace_write_with( - std::slice::from_ref(&writable_root), + let extra_roots = if root_count == 2 { + vec![writable_root.clone()] + } else { + vec![] + }; + let denied_root = writable_root.join("blocked"); + let mut file_system = PermissionProfile::workspace_write_with( + &extra_roots, NetworkSandboxPolicy::Restricted, /*exclude_tmpdir_env_var*/ true, /*exclude_slash_tmp*/ true, - ); - - let mut builder = test_codex().with_config(move |config| { - config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); - config - .permissions - .set_permission_profile(permission_profile) - .expect("test permission profile should be allowed"); - let workspace_roots = vec![config.cwd.clone(), writable_root_for_config]; - config.workspace_roots = workspace_roots.clone(); - config.permissions.set_workspace_roots(workspace_roots); - config.config_layer_stack = ConfigLayerStack::default(); - }); + ) + .file_system_sandbox_policy(); + // Windows restricted-token sandboxes cannot enforce deny-read policies. + // Keep the writable-root cases enabled there without the extra denial. + let denied_reads = if cfg!(windows) { + String::new() + } else { + file_system.entries.push(FileSystemSandboxEntry::new( + denied_root.clone().into(), + FileSystemAccessMode::Deny, + )); + let denied_root = denied_root.to_string_lossy(); + format!( + "\n## Denied filesystem reads\nThe active permission profile denies reading these paths/globs. Do not request escalation or additional permissions to read them; these denials are policy restrictions.\n- path `{denied_root}`" + ) + }; + let permission_profile = + PermissionProfile::from_runtime_permissions(&file_system, NetworkSandboxPolicy::Restricted); + let model_slug = "catalog-writable-roots-model"; + let mut model = model_with_permission_messages(model_slug, permissions); + 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::Never); + config + .permissions + .set_permission_profile(permission_profile) + .expect("test permission profile should be allowed"); + let mut workspace_roots = vec![config.cwd.clone()]; + workspace_roots.extend(extra_roots); + config.workspace_roots = workspace_roots.clone(); + config.permissions.set_workspace_roots(workspace_roots); + config.config_layer_stack = ConfigLayerStack::default(); + config.model_catalog = Some(ModelsResponse { + models: vec![model], + }); + }); + // This case intentionally exercises host-local workspace and extra roots. let test = builder.build(&server).await?; + submit_text_turn(&test, "hello").await?; - test.codex - .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { - text: "hello".into(), - text_elements: Vec::new(), - }])) - .await?; - wait_for_event(&test.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; - - let permissions = permissions_texts(&req.single_request()); - assert_eq!(permissions.len(), 1); - let mut writable_roots = [ - AbsolutePathBuf::try_from(test.config.cwd.as_path().canonicalize()?)?, - writable_root, - ]; - writable_roots.sort(); - let writable_roots = writable_roots + let mut roots = vec![AbsolutePathBuf::try_from( + test.config.cwd.as_path().canonicalize()?, + )?]; + if root_count == 2 { + roots.push(writable_root); + } + roots.sort(); + let roots = roots .iter() .map(|root| format!("`{}`", root.to_string_lossy())) .collect::>() .join(", "); - let expected = format!(" The writable roots are {writable_roots}."); - let actual = permissions[0] - .lines() - .filter(|line| line.starts_with(" The writable root")) + let root_sentence = expected_root_sentence.replace("{roots}", &roots); + let request = req.single_request(); + let expected = format!( + "{WORKSPACE_PERMISSIONS_PREFIX}{root_sentence}{denied_reads}\n" + ); + assert_eq!(permissions_texts(&request), vec![expected.clone()]); + let annotated_permissions = request + .input() + .into_iter() + .flat_map(|item| { + item["content"] + .as_array() + .into_iter() + .flatten() + .enumerate() + .filter(|(_, content)| { + content["text"] + .as_str() + .is_some_and(|text| text.replace("\r\n", "\n") == expected) + }) + .map(|(index, _)| { + ( + item["role"].clone(), + item["internal_chat_message_metadata_passthrough"]["content_item_kinds"] + [index] + .clone(), + item["internal_chat_message_metadata_passthrough"]["content_item_metadata"] + [index] + .clone(), + ) + }) + .collect::>() + }) .collect::>(); - assert_eq!(actual, vec![expected.as_str()]); - + assert_eq!( + annotated_permissions, + vec![( + json!("developer"), + json!("permissions.instructions"), + json!({"provenance": {"type": "harness", "hook": false}}), + )] + ); Ok(()) } + +pub(super) async fn catalog_writable_roots_model_change_requests() -> Result> +{ + let server = start_mock_server().await; + let req1 = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + ) + .await; + let first_slug = "catalog-writable-roots-a"; + let second_slug = "catalog-writable-roots-b"; + let models = [ + ( + first_slug, + "The writable root for model A is {{ roots }}.", + "Wrong A plural: {{ roots }}.", + ), + ( + second_slug, + "The writable root for model B is {{ roots }}.", + "Wrong B plural: {{ roots }}.", + ), + ] + .into_iter() + .map(|(slug, single, plural)| { + model_with_permission_messages( + slug, + serde_json::from_value(json!({"writable_root": single, "writable_roots": plural})) + .expect("valid catalog permission messages"), + ) + }) + .collect(); + let mut builder = test_codex() + .with_model(first_slug) + .with_config(move |config| { + config.base_instructions = Some("Help with the workspace.".to_string()); + // Match the canonical writable root so snapshot normalization recognizes the cwd. + if !core_test_support::is_remote_test_environment() { + config.cwd = dunce::canonicalize(config.cwd.as_path()) + .expect("test workspace should be canonicalizable") + .try_into() + .expect("canonical workspace path should be absolute"); + } + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::Never); + config + .permissions + .set_permission_profile(PermissionProfile::workspace_write_with( + &[], + NetworkSandboxPolicy::Restricted, + /*exclude_tmpdir_env_var*/ true, + /*exclude_slash_tmp*/ true, + )) + .expect("workspace-write profile should be allowed"); + config.workspace_roots = vec![config.cwd.clone()]; + config + .permissions + .set_workspace_roots(config.workspace_roots.clone()); + config.config_layer_stack = ConfigLayerStack::default(); + config.model_catalog = Some(ModelsResponse { models }); + }); + let test = builder.build_with_auto_env(&server).await?; + submit_text_turn(&test, "Inspect the writable workspace.").await?; + core_test_support::submit_thread_settings( + &test.codex, + ThreadSettingsOverrides { + model: Some(second_slug.to_string()), + ..Default::default() + }, + ) + .await?; + let req2 = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-2"), ev_completed("resp-2")]), + ) + .await; + submit_text_turn(&test, "Continue with the selected model.").await?; + let req3 = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-3"), ev_completed("resp-3")]), + ) + .await; + submit_text_turn(&test, "Continue without changing permissions.").await?; + let writable_root = test + .fs() + .canonicalize(&test.workspace_path_uri("")?, /*sandbox*/ None) + .await? + .inferred_native_path_string(); + let requests = vec![ + req1.single_request(), + req2.single_request(), + req3.single_request(), + ]; + let first = format!( + "{WORKSPACE_PERMISSIONS_PREFIX}The writable root for model A is `{writable_root}`.\n" + ); + let second = format!( + "{WORKSPACE_PERMISSIONS_PREFIX}The writable root for model B is `{writable_root}`.\n" + ); + assert_eq!(permissions_texts(&requests[0]), vec![first.clone()]); + assert_eq!(permissions_texts(&requests[1]), vec![first, second]); + assert_eq!( + permissions_texts(&requests[2]), + permissions_texts(&requests[1]) + ); + Ok(requests) +} diff --git a/codex-rs/core/tests/suite/scenarios.rs b/codex-rs/core/tests/suite/scenarios.rs index dfef49d007..51e89ca463 100644 --- a/codex-rs/core/tests/suite/scenarios.rs +++ b/codex-rs/core/tests/suite/scenarios.rs @@ -2674,6 +2674,20 @@ async fn model_catalog_refresh_preserves_tools_and_history() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn catalog_writable_roots_follow_selected_model() -> Result<()> { + skip_if_no_network!(Ok(())); + let requests = + super::permissions_messages::catalog_writable_roots_model_change_requests().await?; + let snapshot = context_snapshot::format_request_history_snapshot( + "Catalog writable-root wording follows the selected model, appending changed permissions once while preserving prior input.", + &requests, + &ContextSnapshotOptions::default().include_request_settings(), + ); + insta::assert_snapshot!(snapshot); + Ok(()) +} + fn compaction_chunk( gate: Option>, events: Vec, diff --git a/codex-rs/core/tests/suite/settings_constraints.rs b/codex-rs/core/tests/suite/settings_constraints.rs index 4cee6608ce..f5d35c64c7 100644 --- a/codex-rs/core/tests/suite/settings_constraints.rs +++ b/codex-rs/core/tests/suite/settings_constraints.rs @@ -143,6 +143,7 @@ async fn protected_model_settings_use_the_proposed_permissions( workspace_write: None, read_only: Some(PERMISSION_INSTRUCTIONS.to_string()), approved_prefixes: None, + ..PermissionMessages::default() }); let test = test_codex() .with_model(INITIAL_MODEL) diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__catalog_writable_roots_follow_selected_model.snap b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__catalog_writable_roots_follow_selected_model.snap new file mode 100644 index 0000000000..165d622cec --- /dev/null +++ b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__catalog_writable_roots_follow_selected_model.snap @@ -0,0 +1,87 @@ +--- +source: core/tests/suite/scenarios.rs +expression: snapshot +--- +Scenario: Catalog writable-root wording follows the selected model, appending changed permissions once while preserving prior input. + +## Window 1 +Settings: + include: ["reasoning.encrypted_content"] + model: "catalog-writable-roots-a" + parallel_tool_calls: true + prompt_cache_key: "" + reasoning: {"summary":"auto"} + store: false + stream: true + tool_choice: "auto" + tools (6; hash=3201044466fc72a5): + - function/exec_command: Runs a command in a PTY, returning output or a session ID for ongoing interaction.; args=[cmd, justification, login, max_output_tokens, prefix_rule, sandbox_permissions, shell, tty, workdir, yield_time_ms]; hash=c04108e51a8548c5 + - function/write_stdin: Writes characters to an existing unified exec session and returns recent output.; args=[chars, max_output_tokens, session_id, yield_time_ms]; hash=7fb62d8a3e2df4e2 + - function/request_user_input: Request user input for one to three short questions and wait for the response. This tool is only ...; args=[questions]; hash=64a729b25c424fc6 + - function/view_image: View a local image file from the filesystem when visual inspection is needed. Use this for images...; args=[path]; hash=1908de9c0be7118a + - 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 + - web_search; hash=b24567c220fd75ba +-- request 1 (turn) -- +00:message/developer: + Help with the workspace. +01:message/developer: + + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `workspace-write`: T...ries requires approval. Network access is restricted. [hash=99963de5837f6638] + Approval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected. + The writable root for model A is ``. + +02:message/user: + + + + + + [hash=8d38f7686fcb5f7d] + +03:message/user: + Inspect the writable workspace. + +## Window 2 (after request 1) +Settings: relative to window 1 + model: "catalog-writable-roots-b" +-- request 2 (turn) -- +00:message/developer: + Help with the workspace. +01:message/developer: + + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `workspace-write`: T...ries requires approval. Network access is restricted. [hash=99963de5837f6638] + Approval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected. + The writable root for model A is ``. + +02:message/user: + + + + + + [hash=8d38f7686fcb5f7d] + +03:message/user: + Inspect the writable workspace. +04:message/developer: + + The user was previously using a different model. Please continue the conversation according to the following instructions: + + Help with the workspace. + +05:message/developer: + + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `workspace-write`: T...ries requires approval. Network access is restricted. [hash=99963de5837f6638] + Approval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected. + The writable root for model B is ``. + +06:message/user: + Continue with the selected model. +-- request 3 (turn) -- +07:message/user: + Continue without changing permissions. diff --git a/codex-rs/models-manager/src/model_info_tests.rs b/codex-rs/models-manager/src/model_info_tests.rs index 44f5462a99..d7c7ab73c8 100644 --- a/codex-rs/models-manager/src/model_info_tests.rs +++ b/codex-rs/models-manager/src/model_info_tests.rs @@ -52,6 +52,7 @@ fn base_instruction_override_is_literal_and_preserves_catalog_messages() { workspace_write: Some(String::new()), read_only: None, approved_prefixes: None, + ..PermissionMessages::default() }; 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 264752c160..32e0134bc1 100644 --- a/codex-rs/prompts/src/model_messages/permissions.rs +++ b/codex-rs/prompts/src/model_messages/permissions.rs @@ -19,6 +19,8 @@ pub(crate) const WORKSPACE_WRITE_TEMPLATE: &str = include_str!("../../templates/permissions/sandbox_mode/workspace_write.md"); pub(crate) const READ_ONLY_TEMPLATE: &str = include_str!("../../templates/permissions/sandbox_mode/read_only.md"); +pub(crate) const WRITABLE_ROOT_TEMPLATE: &str = " The writable root is {{ roots }}."; +pub(crate) const WRITABLE_ROOTS_TEMPLATE: &str = " The writable roots are {{ roots }}."; /// Resolved approval-policy text; policy and tool selection remain with the consumer. #[derive(Debug, Clone, Copy)] @@ -59,6 +61,8 @@ pub(crate) struct ResolvedPermissionMessages<'a> { pub(crate) danger_full_access: ResolvedMessage<'a>, pub(crate) workspace_write: ResolvedMessage<'a>, pub(crate) read_only: ResolvedMessage<'a>, + pub(crate) writable_root: ResolvedMessage<'a>, + pub(crate) writable_roots: ResolvedMessage<'a>, } impl<'a> ResolvedPermissionMessages<'a> { @@ -80,6 +84,14 @@ impl<'a> ResolvedPermissionMessages<'a> { messages.and_then(|m| m.read_only.as_deref()), READ_ONLY_TEMPLATE, ), + writable_root: ResolvedMessage::new( + messages.and_then(|m| m.writable_root.as_deref()), + WRITABLE_ROOT_TEMPLATE, + ), + writable_roots: ResolvedMessage::new( + messages.and_then(|m| m.writable_roots.as_deref()), + WRITABLE_ROOTS_TEMPLATE, + ), } } } diff --git a/codex-rs/prompts/src/permissions_instructions.rs b/codex-rs/prompts/src/permissions_instructions.rs index b827006901..eb6653973c 100644 --- a/codex-rs/prompts/src/permissions_instructions.rs +++ b/codex-rs/prompts/src/permissions_instructions.rs @@ -27,6 +27,9 @@ use codex_utils_template::Template; use std::collections::HashSet; use std::path::Path; use std::sync::LazyLock; +use writable_roots::writable_roots_text; + +mod writable_roots; const REQUEST_PERMISSION_RULE: &str = include_str!("../templates/permissions/approval_policy/on_request_rule_request_permission.md"); @@ -139,7 +142,9 @@ impl PermissionsInstructions { context.request_permissions_tool_enabled, ), ); - if let Some(writable_roots) = writable_roots_text(&writable_roots) { + if let Some(writable_roots) = + writable_roots_text(&writable_roots, approval_context.permission_messages) + { append_section(&mut text, &writable_roots); } if let Some(denied_reads) = denied_reads_text(&denied_read_paths, &denied_read_globs) { @@ -377,22 +382,6 @@ fn sandbox_text( } } -fn writable_roots_text(writable_roots: &[String]) -> Option { - if writable_roots.is_empty() { - return None; - } - - let roots_list: Vec = writable_roots - .iter() - .map(|root| format!("`{root}`")) - .collect(); - Some(if roots_list.len() == 1 { - format!(" The writable root is {}.", roots_list[0]) - } else { - format!(" The writable roots are {}.", roots_list.join(", ")) - }) -} - fn denied_reads_text(paths: &[String], globs: &[String]) -> Option { let mut entries = paths .iter() diff --git a/codex-rs/prompts/src/permissions_instructions/writable_roots.rs b/codex-rs/prompts/src/permissions_instructions/writable_roots.rs new file mode 100644 index 0000000000..0da8e47e30 --- /dev/null +++ b/codex-rs/prompts/src/permissions_instructions/writable_roots.rs @@ -0,0 +1,72 @@ +//! Renders bounded catalog wording around an already bounded list of writable roots. +//! Validate template text before inserting paths once; unusable overrides retain legacy bytes. + +use super::MAX_PERMISSION_PATH_BYTES; +use super::PermissionsInstructions; +use crate::model_messages::permissions::ResolvedPermissionMessages; +use crate::model_messages::permissions::WRITABLE_ROOT_TEMPLATE; +use crate::model_messages::permissions::WRITABLE_ROOTS_TEMPLATE; +use codex_context_fragments::ContextualUserFragment; + +const ROOTS_PLACEHOLDER: &str = "{{ roots }}"; +// Replacing the 11-byte marker leaves at most 989 bytes of catalog wording, +// bounding it below 1K tokens even with one byte-fallback token per byte. +const MAX_ROOTS_TEMPLATE_BYTES: usize = 1_000; +const MAX_ROOTS_SENTENCE_BYTES: usize = MAX_PERMISSION_PATH_BYTES + MAX_ROOTS_TEMPLATE_BYTES; + +// The caller bounds paths at whole-entry boundaries before reaching this renderer. +pub(super) fn writable_roots_text( + roots: &[String], + messages: ResolvedPermissionMessages<'_>, +) -> Option { + let (selected, fallback) = match roots.len() { + 0 => return None, + 1 => (messages.writable_root, WRITABLE_ROOT_TEMPLATE), + _ => (messages.writable_roots, WRITABLE_ROOTS_TEMPLATE), + }; + let roots_list = roots + .iter() + .map(|root| format!("`{root}`")) + .collect::>() + .join(", "); + Some( + selected + .catalog_override() + .and_then(|template| render_roots_override(template, &roots_list)) + .unwrap_or_else(|| fallback.replace(ROOTS_PLACEHOLDER, &roots_list)), + ) +} + +fn render_roots_override(template: &str, roots: &str) -> Option { + if template.len() > MAX_ROOTS_TEMPLATE_BYTES + || template + .chars() + .any(|c| c.is_control() || matches!(c, '\u{2028}' | '\u{2029}')) + { + return None; + } + let (before, after) = template.split_once(ROOTS_PLACEHOLDER)?; + // Reject duplicate, unknown and malformed placeholders, without parsing runtime paths. + if [before, after] + .iter() + .any(|part| part.chars().any(|c| matches!(c, '{' | '}'))) + { + return None; + } + let (open, close) = PermissionsInstructions::type_markers(); + if template.contains(open) || template.contains(close) { + return None; + } + let rendered_len = before + .len() + .checked_add(roots.len())? + .checked_add(after.len())?; + if rendered_len > MAX_ROOTS_SENTENCE_BYTES { + return None; + } + Some(format!("{before}{roots}{after}")) +} + +#[cfg(test)] +#[path = "writable_roots_tests.rs"] +mod tests; diff --git a/codex-rs/prompts/src/permissions_instructions/writable_roots_tests.rs b/codex-rs/prompts/src/permissions_instructions/writable_roots_tests.rs new file mode 100644 index 0000000000..bae36abc1e --- /dev/null +++ b/codex-rs/prompts/src/permissions_instructions/writable_roots_tests.rs @@ -0,0 +1,80 @@ +//! Checks bounded catalog wording without interpreting or truncating runtime paths. + +use super::*; +use codex_protocol::openai_models::PermissionMessages; +use pretty_assertions::assert_eq; + +#[test] +fn unusable_templates_fall_back_without_losing_roots() { + let roots = vec!["/a".to_string(), "/b".to_string()]; + for template in [ + "", + " ", + "No placeholder", + "{{roots}}", + "{{ roots }} {{ roots }}", + "{{ roots }} {{ other }}", + "{{{ roots }}}", + "X { {{ roots }}", + "X {{ roots }} }", + "X\n{{ roots }}", + "X\t{{ roots }}", + "X\0{{ roots }}", + "X\u{2028}{{ roots }}", + "X\u{2029}{{ roots }}", + "{{ roots }}", + "{{ roots }}", + ] { + let messages = PermissionMessages { + writable_root: Some(template.into()), + writable_roots: Some(template.into()), + ..PermissionMessages::default() + }; + let resolved = ResolvedPermissionMessages::new(Some(&messages)); + assert_eq!( + writable_roots_text(&roots, resolved), + Some(" The writable roots are `/a`, `/b`.".into()) + ); + assert_eq!( + writable_roots_text(&roots[..1], resolved), + Some(" The writable root is `/a`.".into()) + ); + assert_eq!(writable_roots_text(&[], resolved), None); + } +} + +#[test] +fn substitution_preserves_whitespace_and_path_contents() { + assert_eq!( + render_roots_override( + " Paths = {{ roots }} ", + "`/a/{{ roots }}`, `/é\n{literal}`" + ), + Some(" Paths = `/a/{{ roots }}`, `/é\n{literal}` ".into()), + ); +} + +#[test] +fn bounds_template_bytes_and_rendered_output_before_allocation() { + let exact = format!( + "{}{{{{ roots }}}}", + "x".repeat(1_000 - ROOTS_PLACEHOLDER.len()) + ); + assert!(render_roots_override(&exact, "`/a`").is_some()); + for oversized in [exact + "x", format!("{}{{{{ roots }}}}", "é".repeat(495))] { + let messages = PermissionMessages { + writable_root: Some(oversized), + ..PermissionMessages::default() + }; + assert_eq!( + writable_roots_text( + &["/a".into()], + ResolvedPermissionMessages::new(Some(&messages)) + ), + Some(" The writable root is `/a`.".into()), + ); + } + let roots = "x".repeat(MAX_ROOTS_SENTENCE_BYTES); + assert_eq!(render_roots_override("a{{ roots }}", &roots), None); + assert_eq!(render_roots_override("{{ roots }}", &roots), Some(roots)); +} diff --git a/codex-rs/prompts/src/permissions_instructions_tests.rs b/codex-rs/prompts/src/permissions_instructions_tests.rs index eb5585688d..88e075c3e0 100644 --- a/codex-rs/prompts/src/permissions_instructions_tests.rs +++ b/codex-rs/prompts/src/permissions_instructions_tests.rs @@ -194,6 +194,7 @@ fn catalog_permission_messages_select_sandbox_mode_and_render_network_access() { workspace_write: Some("catalog workspace {{ network_access }}".to_string()), read_only: Some("catalog read only {{ network_access }}".to_string()), approved_prefixes: None, + ..PermissionMessages::default() }; for (mode, expected) in [ @@ -219,6 +220,7 @@ fn catalog_permission_text_equal_to_bundled_template_preserves_literal_whitespac workspace_write: Some(WORKSPACE_WRITE_TEMPLATE.to_string()), read_only: None, approved_prefixes: None, + ..PermissionMessages::default() }; let catalog_text = sandbox_text( SandboxMode::WorkspaceWrite, @@ -251,6 +253,7 @@ fn missing_catalog_permission_message_uses_legacy_sandbox_text() { workspace_write: None, read_only: Some("unused".to_string()), approved_prefixes: None, + ..PermissionMessages::default() }; assert_eq!( @@ -271,6 +274,7 @@ fn invalid_catalog_permission_message_is_preserved_verbatim() { workspace_write: Some(workspace_write.to_string()), read_only: None, approved_prefixes: None, + ..PermissionMessages::default() }; assert_eq!( sandbox_text( @@ -291,6 +295,7 @@ fn catalog_permission_message_renders_network_access_and_preserves_other_placeho workspace_write: Some(source.to_string()), read_only: None, approved_prefixes: None, + ..PermissionMessages::default() }; assert_eq!( @@ -310,6 +315,7 @@ fn empty_catalog_permission_message_preserves_non_sandbox_sections() { workspace_write: Some(String::new()), read_only: None, approved_prefixes: None, + ..PermissionMessages::default() }; let writable_root = "/tmp/repo".to_string(); let text = PermissionsInstructions::from_resolved( @@ -734,6 +740,7 @@ fn preserves_supplied_path_spellings_and_order() { workspace_write: Some(String::new()), read_only: None, approved_prefixes: None, + ..PermissionMessages::default() }; let instructions = PermissionsInstructions::from_resolved( @@ -798,3 +805,72 @@ fn bounds_permission_paths_at_whole_entry_boundaries() { let expected = render(&[writable], &[denied], &["/short/**".into()]); assert_eq!(text, format!("{expected}{OMITTED_PERMISSION_PATHS}\n")); } + +#[test] +fn catalog_writable_roots_preserve_fragment_facts() { + let messages = PermissionMessages { + writable_root: Some("Catalog root: {{ roots }}.".into()), + writable_roots: Some("Catalog roots: {{ roots }}.".into()), + ..PermissionMessages::default() + }; + let catalog_context = ApprovalPromptContext { + permission_messages: ResolvedPermissionMessages::new(Some(&messages)), + ..user_approval_context() + }; + let denied = vec!["/private".to_string()]; + for roots in [ + vec![], + vec!["/a/{{ roots }}".to_string()], + vec![r"C:\work\z".to_string(), "/a/é\n{literal}".to_string()], + ] { + let context = PermissionsRenderContext { + writable_roots: &roots, + denied_read_paths: &denied, + ..WORKSPACE_CONTEXT + }; + let original = PermissionsInstructions::from_resolved(context, user_approval_context()); + let expected = PermissionsInstructions { + text: original + .text + .replace(" The writable root is ", "Catalog root: ") + .replace(" The writable roots are ", "Catalog roots: "), + }; + assert_eq!( + PermissionsInstructions::from_resolved(context, catalog_context,).render_fragment(), + expected.render_fragment(), + ); + } +} + +#[test] +fn catalog_writable_roots_select_displayed_count_after_truncation() { + let messages = PermissionMessages { + writable_root: Some("Catalog root: {{ roots }}.".into()), + writable_roots: Some("Catalog roots: {{ roots }}.".into()), + ..PermissionMessages::default() + }; + let roots = vec!["x".repeat(MAX_PERMISSION_PATH_BYTES), "/short".to_string()]; + let context = PermissionsRenderContext { + writable_roots: &roots, + ..WORKSPACE_CONTEXT + }; + let original = PermissionsInstructions::from_resolved(context, user_approval_context()); + let expected = PermissionsInstructions { + text: original + .text + .replace(" The writable root is ", "Catalog root: "), + }; + let actual = PermissionsInstructions::from_resolved( + context, + ApprovalPromptContext { + permission_messages: ResolvedPermissionMessages::new(Some(&messages)), + ..user_approval_context() + }, + ); + assert_eq!(actual.render_fragment(), expected.render_fragment()); + assert!( + actual + .text + .ends_with(&format!("{OMITTED_PERMISSION_PATHS}\n")) + ); +} diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index d3d3dd8930..797c3f881b 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -779,7 +779,7 @@ pub struct AutoReviewMessages { pub timeout_instructions: Option, } -#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] +#[derive(Debug, Default, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] pub struct PermissionMessages { pub danger_full_access: Option, pub workspace_write: Option, @@ -788,6 +788,14 @@ pub struct PermissionMessages { /// an empty string suppresses the approved-prefix section. #[serde(default, skip_serializing_if = "Option::is_none")] pub approved_prefixes: Option, + /// Single writable-root sentence with exactly one `{{ roots }}` placeholder. + /// Missing, null, empty, invalid, or oversized templates use the bundled sentence. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub writable_root: Option, + /// Multiple writable-roots sentence with exactly one `{{ roots }}` placeholder. + /// Missing, null, empty, invalid, or oversized templates use the bundled sentence. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub writable_roots: Option, } #[derive(Debug, Default, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] @@ -1315,10 +1323,33 @@ mod tests { workspace_write: Some(String::new()), read_only: None, approved_prefixes: None, + ..PermissionMessages::default() }) ); } + #[test] + fn writable_root_messages_preserve_sparse_wire_values() { + for input in [ + serde_json::json!({}), + serde_json::json!({"writable_root": null, "writable_roots": null}), + serde_json::json!({"writable_root": "", "writable_roots": "Roots: {{ roots }}."}), + ] { + let messages: PermissionMessages = serde_json::from_value(input.clone()).unwrap(); + let mut expected = serde_json::json!({ + "danger_full_access": null, + "workspace_write": null, + "read_only": null, + }); + for key in ["writable_root", "writable_roots"] { + if let Some(value) = input.get(key).filter(|value| !value.is_null()) { + expected[key] = value.clone(); + } + } + assert_eq!(serde_json::to_value(messages).unwrap(), expected); + } + } + #[test] fn multi_agent_messages_preserve_missing_and_empty_values() { let messages: ModelMessages = from_str( @@ -1560,6 +1591,7 @@ mod tests { workspace_write: Some("workspace".to_string()), read_only: None, approved_prefixes: None, + ..PermissionMessages::default() }), multi_agent: None, token_budget: None,