From 9b3a7c9570dffb88bf02f7dc87654fcf84ca6905 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 21 May 2026 13:22:52 -0700 Subject: [PATCH] fix: surface denied filesystem reads in prompts Expose denied-read entries from the active permission profile to model-visible context and automatic approval review prompts so escalation decisions can account for policy-restricted paths. --- .../core/src/context/environment_context.rs | 91 ++++++++++++++++++- .../src/context/environment_context_tests.rs | 30 ++++++ .../src/context/permissions_instructions.rs | 78 ++++++++++++---- .../context/permissions_instructions_tests.rs | 45 +++++++++ codex-rs/core/src/guardian/mod.rs | 2 + codex-rs/core/src/guardian/prompt.rs | 49 ++++++++++ codex-rs/core/src/guardian/review_session.rs | 5 +- codex-rs/core/src/guardian/tests.rs | 62 +++++++++++++ 8 files changed, 336 insertions(+), 26 deletions(-) diff --git a/codex-rs/core/src/context/environment_context.rs b/codex-rs/core/src/context/environment_context.rs index a3052b6642..4077944c98 100644 --- a/codex-rs/core/src/context/environment_context.rs +++ b/codex-rs/core/src/context/environment_context.rs @@ -1,9 +1,11 @@ use crate::session::turn_context::TurnContext; use crate::session::turn_context::TurnEnvironment; use crate::shell::Shell; +use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::TurnContextItem; use codex_protocol::protocol::TurnContextNetworkItem; use codex_utils_absolute_path::AbsolutePathBuf; +use std::path::Path; use super::ContextualUserFragment; @@ -13,6 +15,7 @@ pub(crate) struct EnvironmentContext { pub(crate) current_date: Option, pub(crate) timezone: Option, pub(crate) network: Option, + pub(crate) filesystem: Option, pub(crate) subagents: Option, } @@ -83,6 +86,55 @@ impl EnvironmentContextEnvironments { } } +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub(crate) struct FileSystemContext { + unreadable_roots: Vec, + unreadable_globs: Vec, +} + +impl FileSystemContext { + fn new(unreadable_roots: Vec, unreadable_globs: Vec) -> Option { + if unreadable_roots.is_empty() && unreadable_globs.is_empty() { + None + } else { + Some(Self { + unreadable_roots, + unreadable_globs, + }) + } + } + + fn from_permission_profile(permission_profile: &PermissionProfile, cwd: &Path) -> Option { + let file_system_sandbox_policy = permission_profile.file_system_sandbox_policy(); + Self::new( + file_system_sandbox_policy + .get_unreadable_roots_with_cwd(cwd) + .into_iter() + .map(|root| root.to_string_lossy().into_owned()) + .collect(), + file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd), + ) + } + + fn render(&self) -> String { + let mut rendered = "".to_string(); + Self::push_rendered_element(&mut rendered, "paths", &self.unreadable_roots); + Self::push_rendered_element(&mut rendered, "globs", &self.unreadable_globs); + rendered.push_str(""); + rendered + } + + fn push_rendered_element(rendered_filesystem: &mut String, name: &str, values: &[String]) { + if values.is_empty() { + return; + } + + rendered_filesystem.push_str(&format!("<{name}>")); + rendered_filesystem.push_str(&values.join(",")); + rendered_filesystem.push_str(&format!("")); + } +} + #[derive(Debug, Clone, PartialEq, Eq, Default)] pub(crate) struct NetworkContext { allowed_domains: Vec, @@ -129,6 +181,7 @@ impl EnvironmentContext { current_date, timezone, network, + filesystem: None, subagents, } } @@ -138,6 +191,7 @@ impl EnvironmentContext { current_date: Option, timezone: Option, network: Option, + filesystem: Option, subagents: Option, ) -> Self { Self { @@ -145,6 +199,7 @@ impl EnvironmentContext { current_date, timezone, network, + filesystem, subagents, } } @@ -157,6 +212,7 @@ impl EnvironmentContext { && self.current_date == other.current_date && self.timezone == other.timezone && self.network == other.network + && self.filesystem == other.filesystem && self.subagents == other.subagents } @@ -165,6 +221,7 @@ impl EnvironmentContext { after: &EnvironmentContext, ) -> Self { let before_network = Self::network_from_turn_context_item(before); + let before_filesystem = Self::filesystem_from_turn_context_item(before); let environments = match &after.environments { EnvironmentContextEnvironments::Single(environment) => { if before.cwd.as_path() != environment.cwd.as_path() { @@ -186,17 +243,25 @@ impl EnvironmentContext { } else { before_network }; + let filesystem = if before_filesystem != after.filesystem { + after.filesystem.clone() + } else { + before_filesystem + }; EnvironmentContext::new_with_environments( environments, after.current_date.clone(), after.timezone.clone(), network, + filesystem, /*subagents*/ None, ) } pub(crate) fn from_turn_context(turn_context: &TurnContext, shell: &Shell) -> Self { - Self::new( + #[allow(deprecated)] + let cwd = &turn_context.cwd; + let mut context = Self::new( EnvironmentContextEnvironment::from_turn_environments( &turn_context.environments.turn_environments, shell, @@ -205,7 +270,10 @@ impl EnvironmentContext { turn_context.timezone.clone(), Self::network_from_turn_context(turn_context), /*subagents*/ None, - ) + ); + context.filesystem = + FileSystemContext::from_permission_profile(&turn_context.permission_profile, cwd); + context } pub(crate) fn from_turn_context_item( @@ -216,11 +284,14 @@ impl EnvironmentContext { Ok(cwd) => cwd, Err(_) => AbsolutePathBuf::resolve_path_against_base(&turn_context_item.cwd, "/"), }; - Self::new( - vec![EnvironmentContextEnvironment::legacy(cwd, shell)], + Self::new_with_environments( + EnvironmentContextEnvironments::from_vec(vec![EnvironmentContextEnvironment::legacy( + cwd, shell, + )]), turn_context_item.current_date.clone(), turn_context_item.timezone.clone(), Self::network_from_turn_context_item(turn_context_item), + Self::filesystem_from_turn_context_item(turn_context_item), /*subagents*/ None, ) } @@ -266,6 +337,15 @@ impl EnvironmentContext { denied_domains.clone(), )) } + + fn filesystem_from_turn_context_item( + turn_context_item: &TurnContextItem, + ) -> Option { + FileSystemContext::from_permission_profile( + &turn_context_item.permission_profile(), + &turn_context_item.cwd, + ) + } } impl ContextualUserFragment for EnvironmentContext { @@ -324,6 +404,9 @@ impl ContextualUserFragment for EnvironmentContext { // lines.push(" ".to_string()); } } + if let Some(filesystem) = &self.filesystem { + lines.push(format!(" {}", filesystem.render())); + } if let Some(subagents) = &self.subagents { lines.push(" ".to_string()); lines.extend(subagents.lines().map(|line| format!(" {line}"))); diff --git a/codex-rs/core/src/context/environment_context_tests.rs b/codex-rs/core/src/context/environment_context_tests.rs index 68ff7c9d44..2d0855b326 100644 --- a/codex-rs/core/src/context/environment_context_tests.rs +++ b/codex-rs/core/src/context/environment_context_tests.rs @@ -79,6 +79,36 @@ fn serialize_environment_context_with_network() { assert_eq!(context.render(), expected); } +#[test] +fn serialize_environment_context_with_filesystem_denied_reads() { + let mut context = EnvironmentContext::new( + vec![EnvironmentContextEnvironment { + id: "local".to_string(), + cwd: test_path_buf("/repo").abs(), + shell: fake_shell_name(), + }], + /*current_date*/ None, + /*timezone*/ None, + /*network*/ None, + /*subagents*/ None, + ); + context.filesystem = FileSystemContext::new( + vec!["/repo/private".to_string()], + vec!["/repo/private/**".to_string()], + ); + + let expected = format!( + r#" + {} + bash + /repo/private/repo/private/** +"#, + test_path_buf("/repo").display() + ); + + assert_eq!(context.render(), expected); +} + #[test] fn serialize_read_only_environment_context() { let context = EnvironmentContext::new( diff --git a/codex-rs/core/src/context/permissions_instructions.rs b/codex-rs/core/src/context/permissions_instructions.rs index db8f983c16..e87dceec01 100644 --- a/codex-rs/core/src/context/permissions_instructions.rs +++ b/codex-rs/core/src/context/permissions_instructions.rs @@ -4,6 +4,7 @@ use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::SandboxMode; use codex_protocol::models::PermissionProfile; use codex_protocol::models::format_allow_prefixes; +use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::GranularApprovalConfig; @@ -68,9 +69,11 @@ impl PermissionsInstructions { exec_permission_approvals_enabled: bool, request_permissions_tool_enabled: bool, ) -> Self { - let (sandbox_mode, writable_roots) = sandbox_prompt_from_profile(permission_profile, cwd); + let file_system_sandbox_policy = permission_profile.file_system_sandbox_policy(); + let (sandbox_mode, writable_roots) = + sandbox_prompt_from_policy(&file_system_sandbox_policy, cwd); - Self::from_permissions_with_network( + Self::from_permissions_with_network_and_denied_reads( sandbox_mode, network_access_from_policy(permission_profile.network_sandbox_policy()), PermissionsPromptConfig { @@ -81,14 +84,32 @@ impl PermissionsInstructions { request_permissions_tool_enabled, }, writable_roots, + denied_reads_text(&file_system_sandbox_policy, cwd), ) } + #[cfg(test)] fn from_permissions_with_network( sandbox_mode: SandboxMode, network_access: NetworkAccess, config: PermissionsPromptConfig<'_>, writable_roots: Option>, + ) -> Self { + Self::from_permissions_with_network_and_denied_reads( + sandbox_mode, + network_access, + config, + writable_roots, + /*denied_reads*/ None, + ) + } + + fn from_permissions_with_network_and_denied_reads( + sandbox_mode: SandboxMode, + network_access: NetworkAccess, + config: PermissionsPromptConfig<'_>, + writable_roots: Option>, + denied_reads: Option, ) -> Self { let mut text = String::new(); append_section(&mut text, &sandbox_text(sandbox_mode, network_access)); @@ -105,6 +126,9 @@ impl PermissionsInstructions { if let Some(writable_roots) = writable_roots_text(writable_roots) { append_section(&mut text, &writable_roots); } + if let Some(denied_reads) = denied_reads { + append_section(&mut text, &denied_reads); + } if !text.ends_with('\n') { text.push('\n'); } @@ -112,27 +136,19 @@ impl PermissionsInstructions { } } -fn sandbox_prompt_from_profile( - permission_profile: &PermissionProfile, +fn sandbox_prompt_from_policy( + file_system_policy: &FileSystemSandboxPolicy, cwd: &Path, ) -> (SandboxMode, Option>) { - match permission_profile { - PermissionProfile::Disabled | PermissionProfile::External { .. } => { - (SandboxMode::DangerFullAccess, None) - } - PermissionProfile::Managed { .. } => { - let file_system_policy = permission_profile.file_system_sandbox_policy(); - if file_system_policy.has_full_disk_write_access() { - return (SandboxMode::DangerFullAccess, None); - } + if file_system_policy.has_full_disk_write_access() { + return (SandboxMode::DangerFullAccess, None); + } - let writable_roots = file_system_policy.get_writable_roots_with_cwd(cwd); - if writable_roots.is_empty() { - (SandboxMode::ReadOnly, None) - } else { - (SandboxMode::WorkspaceWrite, Some(writable_roots)) - } - } + let writable_roots = file_system_policy.get_writable_roots_with_cwd(cwd); + if writable_roots.is_empty() { + (SandboxMode::ReadOnly, None) + } else { + (SandboxMode::WorkspaceWrite, Some(writable_roots)) } } @@ -254,6 +270,28 @@ fn writable_roots_text(writable_roots: Option>) -> Option Option { + let mut entries = file_system_policy + .get_unreadable_roots_with_cwd(cwd) + .into_iter() + .map(|root| format!("- path `{}`", root.to_string_lossy())) + .collect::>(); + entries.extend( + file_system_policy + .get_unreadable_globs_with_cwd(cwd) + .into_iter() + .map(|glob| format!("- glob `{glob}`")), + ); + if entries.is_empty() { + return None; + } + + Some(format!( + "## 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{}", + entries.join("\n") + )) +} + fn approved_command_prefixes_text(exec_policy: &Policy) -> Option { format_allow_prefixes(exec_policy.get_allowed_prefixes()) .filter(|prefixes| !prefixes.is_empty()) diff --git a/codex-rs/core/src/context/permissions_instructions_tests.rs b/codex-rs/core/src/context/permissions_instructions_tests.rs index 6d1aa5d886..466b8a35d7 100644 --- a/codex-rs/core/src/context/permissions_instructions_tests.rs +++ b/codex-rs/core/src/context/permissions_instructions_tests.rs @@ -83,6 +83,51 @@ fn builds_permissions_from_profile() { assert!(text.contains(writable_root.to_string_lossy().as_ref())); } +#[test] +fn builds_permissions_from_profile_with_denied_reads() { + let cwd = PathBuf::from("/tmp"); + let denied_root = + AbsolutePathBuf::from_absolute_path(cwd.join("blocked")).expect("absolute path"); + let permission_profile = PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: denied_root.clone(), + }, + access: FileSystemAccessMode::Deny, + }, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "/tmp/blocked/**".to_string(), + }, + access: FileSystemAccessMode::Deny, + }, + ]), + NetworkSandboxPolicy::Restricted, + ); + + let instructions = PermissionsInstructions::from_permission_profile( + &permission_profile, + AskForApproval::OnRequest, + ApprovalsReviewer::AutoReview, + &Policy::empty(), + &cwd, + /*exec_permission_approvals_enabled*/ false, + /*request_permissions_tool_enabled*/ false, + ); + let text = instructions.body(); + assert!(text.contains("## Denied filesystem reads")); + assert!(text.contains("Do not request escalation or additional permissions")); + assert!(text.contains(denied_root.to_string_lossy().as_ref())); + assert!(text.contains("glob `/tmp/blocked/**`")); +} + #[test] fn includes_request_rule_instructions_for_on_request() { let mut exec_policy = Policy::empty(); diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 058c19d008..17a7e3c99f 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -147,6 +147,8 @@ use prompt::GuardianTranscriptEntryKind; #[cfg(test)] use prompt::build_guardian_prompt_items; #[cfg(test)] +use prompt::build_guardian_prompt_items_with_parent_turn; +#[cfg(test)] use prompt::collect_guardian_transcript_entries; #[cfg(test)] use prompt::guardian_output_schema; diff --git a/codex-rs/core/src/guardian/prompt.rs b/codex-rs/core/src/guardian/prompt.rs index b1b132a984..062a21deef 100644 --- a/codex-rs/core/src/guardian/prompt.rs +++ b/codex-rs/core/src/guardian/prompt.rs @@ -10,6 +10,7 @@ use serde_json::Value; use crate::compact::content_items_to_text; use crate::event_mapping::is_contextual_user_message_content; use crate::session::session::Session; +use crate::session::turn_context::TurnContext; use codex_utils_output_truncation::approx_bytes_for_tokens; use codex_utils_output_truncation::approx_token_count; use codex_utils_output_truncation::approx_tokens_from_byte_count; @@ -86,11 +87,29 @@ pub(crate) enum GuardianPromptMode { /// Split the variable request into separate user content items so the /// Responses request snapshot shows clear boundaries while preserving exact /// prompt text through trailing newlines. +#[cfg(test)] pub(crate) async fn build_guardian_prompt_items( session: &Session, retry_reason: Option, request: GuardianApprovalRequest, mode: GuardianPromptMode, +) -> serde_json::Result { + build_guardian_prompt_items_with_parent_turn( + session, + /*parent_turn*/ None, + retry_reason, + request, + mode, + ) + .await +} + +pub(crate) async fn build_guardian_prompt_items_with_parent_turn( + session: &Session, + parent_turn: Option<&TurnContext>, + retry_reason: Option, + request: GuardianApprovalRequest, + mode: GuardianPromptMode, ) -> serde_json::Result { let history = session.clone_history().await; let transcript_entries = collect_guardian_transcript_entries(history.raw_items()); @@ -172,6 +191,11 @@ pub(crate) async fn build_guardian_prompt_items( if let Some(note) = omission_note { push_text(format!("\n{note}\n")); } + if let Some(denied_reads_context) = parent_turn.and_then(parent_turn_denied_reads_context) { + push_text("\n>>> PARENT TURN PERMISSION CONTEXT START\n".to_string()); + push_text(denied_reads_context); + push_text(">>> PARENT TURN PERMISSION CONTEXT END\n".to_string()); + } match &request { GuardianApprovalRequest::NetworkAccess { trigger, .. } => { push_text(">>> APPROVAL REQUEST START\n".to_string()); @@ -216,6 +240,31 @@ pub(crate) async fn build_guardian_prompt_items( }) } +fn parent_turn_denied_reads_context(turn: &TurnContext) -> Option { + #[allow(deprecated)] + let cwd = &turn.cwd; + let file_system_policy = turn.permission_profile.file_system_sandbox_policy(); + let mut entries = file_system_policy + .get_unreadable_roots_with_cwd(cwd) + .into_iter() + .map(|root| format!("- path `{}`", root.to_string_lossy())) + .collect::>(); + entries.extend( + file_system_policy + .get_unreadable_globs_with_cwd(cwd) + .into_iter() + .map(|glob| format!("- glob `{glob}`")), + ); + if entries.is_empty() { + return None; + } + + Some(format!( + "The parent turn's active permission profile denies reading these paths/globs. These are policy restrictions; do not approve escalation whose purpose is to read them.\n{}\n", + entries.join("\n") + )) +} + enum GuardianPromptShape { Full, Delta { already_seen_entry_count: usize }, diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 15d6c142ca..9ae8d73a33 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -49,7 +49,7 @@ use super::GUARDIAN_REVIEWER_NAME; use super::GuardianApprovalRequest; use super::prompt::GuardianPromptMode; use super::prompt::GuardianTranscriptCursor; -use super::prompt::build_guardian_prompt_items; +use super::prompt::build_guardian_prompt_items_with_parent_turn; use super::prompt::guardian_policy_prompt; use super::prompt::guardian_policy_prompt_with_config; @@ -670,8 +670,9 @@ async fn run_review_on_session( ) .await; - build_guardian_prompt_items( + build_guardian_prompt_items_with_parent_turn( params.parent_session.as_ref(), + Some(params.parent_turn.as_ref()), params.retry_reason.clone(), params.request.clone(), prompt_mode, diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 0544b27770..44d1f91a38 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -33,6 +33,11 @@ use codex_protocol::models::ContentItem; use codex_protocol::models::PermissionProfile; use codex_protocol::models::ResponseItem; use codex_protocol::openai_models::ReasoningEffort; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; @@ -357,6 +362,63 @@ async fn build_guardian_prompt_full_mode_preserves_initial_review_format() -> an Ok(()) } +#[tokio::test(flavor = "current_thread")] +async fn build_guardian_prompt_includes_parent_turn_denied_reads() -> anyhow::Result<()> { + let (mut session, mut turn) = crate::session::tests::make_session_and_context().await; + session.conversation_id = fixed_guardian_parent_session_id(); + let denied_root = test_path_buf("/repo/private").abs(); + turn.permission_profile = PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: denied_root.clone(), + }, + access: FileSystemAccessMode::Deny, + }, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "/repo/private/**".to_string(), + }, + access: FileSystemAccessMode::Deny, + }, + ]), + NetworkSandboxPolicy::Restricted, + ); + let session = Arc::new(session); + let turn = Arc::new(turn); + seed_guardian_parent_history(&session, &turn).await; + + let prompt = build_guardian_prompt_items_with_parent_turn( + session.as_ref(), + Some(turn.as_ref()), + Some("Sandbox denied reading /repo/private/secret.txt.".to_string()), + GuardianApprovalRequest::Shell { + id: "shell-1".to_string(), + command: vec!["cat".to_string(), "/repo/private/secret.txt".to_string()], + cwd: test_path_buf("/repo").abs(), + sandbox_permissions: crate::sandboxing::SandboxPermissions::RequireEscalated, + additional_permissions: None, + justification: Some("Need to inspect the secret file.".to_string()), + }, + GuardianPromptMode::Full, + ) + .await?; + + let text = guardian_prompt_text(&prompt.items); + assert!(text.contains("PARENT TURN PERMISSION CONTEXT START")); + assert!(text.contains("do not approve escalation whose purpose is to read them")); + assert!(text.contains(denied_root.to_string_lossy().as_ref())); + assert!(text.contains("glob `/repo/private/**`")); + + Ok(()) +} + #[tokio::test(flavor = "current_thread")] async fn build_guardian_prompt_delta_mode_preserves_original_numbering() -> anyhow::Result<()> { let (session, turn) = guardian_test_session_and_turn_with_base_url("http://localhost").await;