diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 019d041511..f982373d56 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3669,6 +3669,7 @@ dependencies = [ "codex-extension-api", "codex-features", "codex-feedback", + "codex-file-system", "codex-guardian-context", "codex-history", "codex-mcp", diff --git a/codex-rs/codex-mcp/src/binding.rs b/codex-rs/codex-mcp/src/binding.rs index 70a2fa9d68..6b07723b25 100644 --- a/codex-rs/codex-mcp/src/binding.rs +++ b/codex-rs/codex-mcp/src/binding.rs @@ -197,7 +197,7 @@ pub struct PreparedMcpCall { pub enum McpPermissionSource<'a> { Environment { environment_id: &'a str, - sandbox: &'a FileSystemSandboxContext, + sandbox: &'a Arc, }, Thread { profile: &'a PermissionProfile, @@ -205,6 +205,15 @@ pub enum McpPermissionSource<'a> { }, } +impl<'a> McpPermissionSource<'a> { + pub fn profile(&self) -> &'a PermissionProfile { + match self { + Self::Environment { sandbox, .. } => &sandbox.permissions, + Self::Thread { profile, .. } => profile, + } + } +} + #[derive(Clone)] pub(crate) struct PreparedToolInfo { catalog: Arc<[ToolInfo]>, @@ -291,11 +300,7 @@ impl PreparedMcpCall { /// Returns the owner permissions validated when this immutable call was prepared. pub fn permission_profile(&self) -> &PermissionProfile { - let Some(permission_profile) = self.config.permission_profile_for_server(&self.server_name) - else { - unreachable!("prepared MCP calls retain their immutable permission authority"); - }; - permission_profile + self.permission_source().profile() } /// Attach policy aliases and the server-host launcher, replacing stale authority. diff --git a/codex-rs/core/src/guardian/permissions.rs b/codex-rs/core/src/guardian/permissions.rs index 30c70fa7e3..cd7d1db73f 100644 --- a/codex-rs/core/src/guardian/permissions.rs +++ b/codex-rs/core/src/guardian/permissions.rs @@ -1,7 +1,11 @@ -//! Resolves Guardian permission evidence from the host's captured environment context. -//! Sync review and async scoring use the same rules and environment identity. +//! Fresh reviews of environment-owned MCP calls use the prepared environment's rules. +//! Other reviews and early scoring use the issuing context. use codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID; +use codex_file_system::FileSystemSandboxContext; +use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemSandboxPolicyContext; +use codex_utils_absolute_path::AbsolutePathBuf; use super::GuardianReviewContext; use crate::context::GuardianPermissionContext; @@ -9,6 +13,39 @@ use crate::session::turn_context::TurnEnvironment; use crate::tools::context::ToolEnvironments; use crate::tools::context::ToolInvocation; +// Bound both JSON and permission evidence without restricting manual approvals. +const MAX_GUARDIAN_ENVIRONMENT_ID_BYTES: usize = 256; + +pub(super) fn check_environment_id(id: Option<&str>) -> anyhow::Result<()> { + if id.is_some_and(|id| id.len() > MAX_GUARDIAN_ENVIRONMENT_ID_BYTES) { + anyhow::bail!( + "approval environment id exceeds Guardian's {MAX_GUARDIAN_ENVIRONMENT_ID_BYTES}-byte limit" + ); + } + Ok(()) +} + +pub(super) fn for_mcp_environment( + environment_id: &str, + sandbox: &FileSystemSandboxContext, +) -> anyhow::Result { + check_environment_id(Some(environment_id))?; + if environment_id == DEFAULT_MCP_SERVER_ENVIRONMENT_ID + && let Ok(cwd) = sandbox.cwd.to_abs_path() + { + return Ok(host_permissions( + Some(environment_id.to_owned()), + &sandbox.permissions, + &cwd, + )); + } + executor_permissions( + environment_id, + &sandbox.permissions, + sandbox.policy_context(), + ) +} + pub(crate) fn for_tool(invocation: &ToolInvocation) -> anyhow::Result { let context = GuardianReviewContext::from(&invocation.step_context); let environment_id = if matches!(invocation.environments, ToolEnvironments::Mcp { .. }) @@ -40,48 +77,71 @@ pub(super) fn for_environment( ), None => context.environments().primary(), }; - let environment_id = - environment.map(|environment| environment.selection.environment_id.clone()); let native_cwd = environment .filter(|environment| !environment.environment.is_remote()) .and_then(|environment| environment.cwd().to_abs_path().ok()); let permission_profile = environment .map(TurnEnvironment::permission_profile_with_workspace_roots) .unwrap_or_else(|| turn.permission_profile_for_environments(context.environments())); - let file_system_policy = permission_profile.file_system_sandbox_policy(); - // Remote restrictions must not be interpreted using the filesystem running Guardian. - // Older executors may not report their temp folders. If a rule explicitly denies those - // folders, decline automatic approval rather than guess. Default rules do not deny them. if let Some(environment) = environment && native_cwd.is_none() { - let sandbox = environment.sandbox_context(/*additional_permissions*/ None); - let paths = sandbox.policy_context(); - let mut denied_globs = file_system_policy - .get_unreadable_globs_with_context(&paths) - .map_err(anyhow::Error::msg)?; - denied_globs.sort(); - denied_globs.dedup(); - return Ok(GuardianPermissionContext { - environment_id, - denied_paths: file_system_policy - .get_unreadable_roots_with_context(&paths) - .map_err(anyhow::Error::msg)? - .into_iter() - .map(|path| path.inferred_native_path_string()) - .collect(), - denied_globs, - }); + return executor_permissions( + &environment.selection.environment_id, + &permission_profile, + environment + .sandbox_context(/*additional_permissions*/ None) + .policy_context(), + ); } #[allow(deprecated)] - let cwd = native_cwd.unwrap_or_else(|| turn.cwd.clone()); + let cwd = native_cwd.as_ref().unwrap_or(&turn.cwd); + Ok(host_permissions( + environment.map(|env| env.selection.environment_id.clone()), + &permission_profile, + cwd, + )) +} + +fn executor_permissions( + environment_id: &str, + profile: &PermissionProfile, + paths: FileSystemSandboxPolicyContext<'_>, +) -> anyhow::Result { + // Remote restrictions must not be interpreted using the filesystem running Guardian. + // Older executors may not report their temp folders. If a rule explicitly denies those + // folders, decline automatic approval rather than guess. Default rules do not deny them. + let file_system_policy = profile.file_system_sandbox_policy(); + let mut denied_globs = file_system_policy + .get_unreadable_globs_with_context(&paths) + .map_err(anyhow::Error::msg)?; + denied_globs.sort(); + denied_globs.dedup(); Ok(GuardianPermissionContext { + environment_id: Some(environment_id.to_owned()), + denied_paths: file_system_policy + .get_unreadable_roots_with_context(&paths) + .map_err(anyhow::Error::msg)? + .into_iter() + .map(|path| path.inferred_native_path_string()) + .collect(), + denied_globs, + }) +} + +fn host_permissions( + environment_id: Option, + profile: &PermissionProfile, + cwd: &AbsolutePathBuf, +) -> GuardianPermissionContext { + let file_system_policy = profile.file_system_sandbox_policy(); + GuardianPermissionContext { environment_id, denied_paths: file_system_policy - .get_unreadable_roots_with_cwd(&cwd) + .get_unreadable_roots_with_cwd(cwd) .into_iter() .map(|root| root.to_string_lossy().into_owned()) .collect(), - denied_globs: file_system_policy.get_unreadable_globs_with_cwd(&cwd), - }) + denied_globs: file_system_policy.get_unreadable_globs_with_cwd(cwd), + } } diff --git a/codex-rs/core/src/guardian/prompt.rs b/codex-rs/core/src/guardian/prompt.rs index e2a82e65ae..b9b6879cb6 100644 --- a/codex-rs/core/src/guardian/prompt.rs +++ b/codex-rs/core/src/guardian/prompt.rs @@ -32,8 +32,6 @@ use super::GUARDIAN_MAX_TOOL_ENTRY_TOKENS; use super::GuardianApprovalRequest; use super::GuardianReviewContext; -// Bound both JSON and permission evidence without restricting manual approvals. -const MAX_GUARDIAN_ENVIRONMENT_ID_BYTES: usize = 256; pub(super) use codex_guardian_reviewer::GUARDIAN_TRANSCRIPT_START; /// Selects transcript evidence using the checkpoint already loaded by the reviewer. @@ -103,12 +101,7 @@ pub(super) async fn collect_guardian_prompt_items_with_parent_turn( GuardianTranscriptHistory::Retained(history) => (history, false), GuardianTranscriptHistory::LoadedParentCheckpoint(history) => (history, true), }; - if request - .target_environment_id() - .is_some_and(|id| id.len() > MAX_GUARDIAN_ENVIRONMENT_ID_BYTES) - { - anyhow::bail!("approval environment id exceeds Guardian's 256-byte limit"); - } + super::permissions::check_environment_id(request.target_environment_id())?; let evidence_mode = parent_context .map(|context| node_repl_review_evidence_mode(context.turn())) .unwrap_or(NodeReplReviewEvidenceMode::Disabled); @@ -133,11 +126,20 @@ pub(super) async fn collect_guardian_prompt_items_with_parent_turn( &request, reasons.retry.or(reasons.approval), )?; - let permissions = parent_context - .map(|context| { - super::permissions::for_environment(context, request.target_environment_id()) - }) - .transpose()?; + let permissions = match request { + GuardianApprovalRequest::McpToolCall { + prompt_permissions: Some((environment_id, sandbox)), + .. + } => Some(super::permissions::for_mcp_environment( + &environment_id, + &sandbox, + )?), + _ => parent_context + .map(|context| { + super::permissions::for_environment(context, request.target_environment_id()) + }) + .transpose()?, + }; let node_repl_snapshot = if node_repl_transcripts_enabled { session .services diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 239a965fcb..a7e24e6d28 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -278,6 +278,7 @@ fn guardian_mcp_request(server: &str, tool_name: &str) -> GuardianApprovalReques open_world_hint: Some(true), read_only_hint: None, }), + prompt_permissions: None, } } diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 453bc96c4f..a9cff9a429 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -40,6 +40,7 @@ use codex_features::Feature; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY; use codex_mcp::McpPermissionPromptAutoApproveContext; +use codex_mcp::McpPermissionSource; use codex_mcp::McpToolRequestMetaHook; use codex_mcp::PreparedMcpCall; use codex_mcp::SandboxState; @@ -78,7 +79,6 @@ use codex_protocol::mcp_approval_meta::TOOL_NAME_KEY as MCP_TOOL_APPROVAL_TOOL_N use codex_protocol::mcp_approval_meta::TOOL_PARAMS_DISPLAY_KEY as MCP_TOOL_APPROVAL_TOOL_PARAMS_DISPLAY_KEY; use codex_protocol::mcp_approval_meta::TOOL_PARAMS_KEY as MCP_TOOL_APPROVAL_TOOL_PARAMS_KEY; use codex_protocol::mcp_approval_meta::TOOL_TITLE_KEY as MCP_TOOL_APPROVAL_TOOL_TITLE_KEY; -use codex_protocol::models::PermissionProfile; use codex_protocol::openai_models::InputModality; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::McpInvocation; @@ -292,7 +292,7 @@ pub(crate) async fn handle_mcp_tool_call( &hook_tool_name, &metadata, prepared_call.config(), - prepared_call.permission_profile(), + prepared_call.permission_source(), prepared_call.server_environment_id(), approval_policy, ) @@ -1557,7 +1557,7 @@ async fn maybe_request_mcp_tool_approval( hook_tool_name: &HookToolName, metadata: &McpToolApprovalMetadata, config: &codex_mcp::McpConfig, - permission_profile: &PermissionProfile, + permission_source: McpPermissionSource<'_>, environment_id: &str, policy: McpToolApprovalPolicy, ) -> Option { @@ -1577,7 +1577,7 @@ async fn maybe_request_mcp_tool_approval( if !strict_auto_review && mcp_permission_prompt_is_auto_approved( config.approval_policy.value(), - permission_profile, + permission_source.profile(), McpPermissionPromptAutoApproveContext { tool_approval_mode: Some(policy.mode), }, @@ -1637,6 +1637,13 @@ async fn maybe_request_mcp_tool_approval( approval_mode: policy.mode, allow_session_remember: session_approval_key.is_some(), allow_persistent_approval: persistent_approval_key.is_some(), + prompt_permissions: match permission_source { + McpPermissionSource::Environment { + environment_id, + sandbox, + } => Some((environment_id.to_owned(), Arc::clone(sandbox))), + McpPermissionSource::Thread { .. } => None, + }, }; let approval_context = ApprovalContext { exec_requirement: None, @@ -1898,6 +1905,7 @@ pub(crate) fn build_guardian_mcp_tool_review_request( open_world_hint: annotations.open_world_hint, read_only_hint: annotations.read_only_hint, }), + prompt_permissions: None, } } diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index c0ac2d401b..72ba73536e 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -107,6 +107,13 @@ fn approval_config(turn_context: &TurnContext) -> codex_mcp::McpConfig { (*mcp_config_for_test(&turn_context.config)).clone() } +fn thread_permission_source(turn_context: &TurnContext) -> McpPermissionSource<'_> { + McpPermissionSource::Thread { + profile: turn_context.config.permissions.permission_profile(), + host_cwd: &turn_context.config.cwd, + } +} + #[test_case::test_case(false, None, Ok(Some("default_link")); "catalog_account")] #[test_case::test_case(true, None, Err(McpToolAccountError::InvalidSelector); "required_selector_omitted")] #[test_case::test_case(true, Some(serde_json::json!(null)), Err(McpToolAccountError::InvalidSelector); "null_selector")] @@ -2054,6 +2061,7 @@ fn guardian_mcp_review_request_includes_invocation_metadata() { tool_title: Some("Navigate".to_string()), tool_description: Some("Open a page".to_string()), annotations: None, + prompt_permissions: None, } ); } @@ -2101,6 +2109,7 @@ fn guardian_mcp_review_request_includes_annotations_when_present() { open_world_hint: Some(true), read_only_hint: Some(false), }), + prompt_permissions: None, } ); } @@ -2135,6 +2144,7 @@ fn guardian_mcp_review_request_ignores_untrusted_connected_account_email() { tool_title: None, tool_description: None, annotations: None, + prompt_permissions: None, } ); } @@ -2372,6 +2382,7 @@ async fn dispatched_mcp_approval_with_closed_response_is_classified_as_approval( approval_mode: AppToolApproval::Auto, allow_session_remember: true, allow_persistent_approval: true, + prompt_permissions: None, }; let approval = tokio::spawn({ let session = Arc::clone(&session); @@ -2906,7 +2917,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), - turn_context.config.permissions.permission_profile(), + thread_permission_source(&turn_context), test_environment_id(&turn_context), McpToolApprovalPolicy::for_server(AppToolApproval::Approve), ) @@ -2989,7 +3000,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), - turn_context.config.permissions.permission_profile(), + thread_permission_source(&turn_context), test_environment_id(&turn_context), McpToolApprovalPolicy::for_server(AppToolApproval::Auto), ) @@ -3054,7 +3065,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { &HookToolName::new("mcp__memory__create_entities"), &metadata, &approval_config(&turn_context), - turn_context.config.permissions.permission_profile(), + thread_permission_source(&turn_context), test_environment_id(&turn_context), McpToolApprovalPolicy::for_server(AppToolApproval::Auto), ) @@ -3133,7 +3144,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { &HookToolName::new("mcp__memory__create_entities"), &metadata, &approval_config(&turn_context), - turn_context.config.permissions.permission_profile(), + thread_permission_source(&turn_context), test_environment_id(&turn_context), McpToolApprovalPolicy::for_server(AppToolApproval::Auto), ) @@ -3228,7 +3239,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { &HookToolName::new("mcp__memory__create_entities"), &metadata, &approval_config(&turn_context), - turn_context.config.permissions.permission_profile(), + thread_permission_source(&turn_context), test_environment_id(&turn_context), McpToolApprovalPolicy::for_server(AppToolApproval::Auto), ) @@ -3336,7 +3347,10 @@ async fn strict_auto_review_forces_guardian_for_mcp_policy_skip() { &HookToolName::new("mcp__test__tool"), &metadata, &captured_mcp_config, - &captured_mcp_config.permission_profile, + McpPermissionSource::Thread { + profile: &captured_mcp_config.permission_profile, + host_cwd: &captured_mcp_config.host_cwd, + }, test_environment_id(&turn_context), McpToolApprovalPolicy::for_server(AppToolApproval::Approve), ) @@ -3421,7 +3435,7 @@ async fn assert_mcp_user_approval_persistence( &HookToolName::new("mcp__memory__create_entities"), &metadata, &approval_config(&turn_context), - turn_context.config.permissions.permission_profile(), + thread_permission_source(&turn_context), test_environment_id(&turn_context), McpToolApprovalPolicy::for_server(AppToolApproval::Auto), ) @@ -3513,7 +3527,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), - turn_context.config.permissions.permission_profile(), + thread_permission_source(&turn_context), test_environment_id(&turn_context), McpToolApprovalPolicy::for_server(AppToolApproval::Prompt), ) @@ -3581,7 +3595,7 @@ async fn full_access_mode_skips_mcp_tool_approval_for_all_approval_modes() { &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), - turn_context.config.permissions.permission_profile(), + thread_permission_source(&turn_context), test_environment_id(&turn_context), McpToolApprovalPolicy::for_server(approval_mode), ) @@ -3677,7 +3691,7 @@ async fn approve_mode_skips_guardian_in_every_permission_mode() { &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), - turn_context.config.permissions.permission_profile(), + thread_permission_source(&turn_context), test_environment_id(&turn_context), McpToolApprovalPolicy::for_server(AppToolApproval::Approve), ) diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 31280867b3..2f63fe7070 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -136,6 +136,8 @@ pub(crate) enum ApprovalAction { approval_mode: AppToolApproval, allow_session_remember: bool, allow_persistent_approval: bool, + #[serde(skip_serializing)] + prompt_permissions: Option<(String, Arc)>, }, NetworkAccess { id: String, @@ -399,6 +401,7 @@ impl ApprovalAction { tool_title, tool_description, annotations, + prompt_permissions, .. } => crate::guardian::GuardianApprovalRequest::McpToolCall { id, @@ -412,6 +415,7 @@ impl ApprovalAction { tool_title, tool_description, annotations, + prompt_permissions, }, Self::NetworkAccess { id, diff --git a/codex-rs/core/src/tools/approvals_tests.rs b/codex-rs/core/src/tools/approvals_tests.rs index 7a87b9f90d..cadbfade84 100644 --- a/codex-rs/core/src/tools/approvals_tests.rs +++ b/codex-rs/core/src/tools/approvals_tests.rs @@ -210,6 +210,7 @@ async fn explicit_mcp_reviewer_override_takes_precedence_over_action_context() { approval_mode: AppToolApproval::Prompt, allow_session_remember: false, allow_persistent_approval: false, + prompt_permissions: None, }; let mut review_context = GuardianReviewContext::from(&turn); review_context.approval_policy = AskForApproval::OnRequest; diff --git a/codex-rs/core/tests/suite/guardian_environments_tests.rs b/codex-rs/core/tests/suite/guardian_environments_tests.rs index 3c7edea82a..99660838e9 100644 --- a/codex-rs/core/tests/suite/guardian_environments_tests.rs +++ b/codex-rs/core/tests/suite/guardian_environments_tests.rs @@ -3,8 +3,12 @@ use super::*; use codex_core::context::UserGoalUpdate; use codex_history::RolloutItem; +use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::protocol::GuardianAssessmentStatus; use codex_protocol::protocol::TurnEnvironmentRequests; +use codex_utils_cargo_bin::cargo_bin; +use codex_utils_path_uri::PathUri; +use core_test_support::exec_server::ExecServerProcess; use core_test_support::streaming_sse::StreamingSseChunk; use core_test_support::streaming_sse::start_streaming_sse_server; use pretty_assertions::assert_eq; @@ -16,6 +20,123 @@ use wiremock::matchers::body_partial_json; use wiremock::matchers::method; use wiremock::matchers::path; +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn direct_mcp_approval_uses_the_server_owners_denied_paths() -> Result<()> { + skip_if_no_network!(Ok(())); + let server = start_mock_server().await; + let workspace = TempDir::new()?; + let owner_temp = TempDir::new()?; + let denied = PathUri::from_abs_path(&owner_temp.path().abs().canonicalize()?) + .inferred_native_path_string(); + // Give the remote executor its own temp directory on both Unix and Windows. + let executor = ExecServerProcess::start_with_env(&[ + ("TMPDIR", &denied), + ("TEMP", &denied), + ("TMP", &denied), + ]) + .await?; + let mcp_servers = serde_json::from_value(json!({ + "files": { + "command": cargo_bin("test_stdio_server")?, + "cwd": workspace.path(), + "environment_id": REMOTE_ENVIRONMENT_ID, + "default_tools_approval_mode": "prompt", + } + }))?; + let test = test_codex() + .with_exec_server_url(executor.websocket_url.clone()) + .with_config(move |config| { + config.mcp_servers.set(mcp_servers).expect("set MCP server"); + config.mcp_optional_startup_grace = Duration::ZERO; + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config + .features + .disable(Feature::GuardianV2) + .expect("use synchronous Guardian"); + }) + .build_with_remote_and_local_env(&server) + .await?; + + let primary = local(workspace.path().abs()); + let mut owner = primary.clone(); + owner.environment_id = REMOTE_ENVIRONMENT_ID.to_string(); + let mut owner_config = environment_config_for_selection(&test.config, &owner); + owner.config = EnvironmentConfigState::Ready(owner_config.clone()); + submit_thread_settings( + &test.codex, + ThreadSettingsOverrides { + environments: Some(TurnEnvironmentRequests::new( + test.config.cwd.clone(), + vec![primary.into_request(), owner.clone().into_request()], + )), + ..Default::default() + }, + ) + .await?; + + let (release, gate) = tokio::sync::oneshot::channel(); + let (parent, _) = start_streaming_sse_server(vec![vec![StreamingSseChunk { + gate: Some(gate), + body: sse(vec![ + ev_function_call_with_namespace("call", "mcp__files", "echo", r#"{"message":"hi"}"#), + ev_completed("parent"), + ]), + }]]) + .await; + Mock::given(method("POST")) + .and(path("/v1/responses")) + .respond_with( + ResponseTemplate::new(307) + .insert_header("location", format!("{}/v1/responses", parent.uri())), + ) + .up_to_n_times(1) + .with_priority(1) + .mount(&server) + .await; + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_assistant_message("review", r#"{"outcome":"deny"}"#), + ev_completed("review"), + ]), + sse(vec![ev_completed("done")]), + ], + ) + .await; + let mut policy = FileSystemSandboxPolicy::read_only(); + policy.entries.push(FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Tmpdir, + }, + FileSystemAccessMode::Deny, + )); + owner_config.permission_profile = PermissionProfileSnapshot::legacy( + PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted), + ); + // The step didn't see the deny rule. MCP receives it before preparing the call. + tokio::try_join!(test.submit_text_turn("Call the files tool."), async { + parent.wait_for_request_count(1).await; + test.codex.environment_ready(&owner, owner_config).await?; + release.send(()).expect("let the model call the tool"); + Ok(()) + })?; + let review = responses + .requests() + .into_iter() + .find(|request| request.body_json()["client_metadata"]["x-openai-subagent"] == "guardian") + .context("expected a Guardian review")?; + let owner_label = format!("permission profile for environment {REMOTE_ENVIRONMENT_ID:?}"); + let permissions = review + .message_input_texts("user") + .into_iter() + .find(|text| text.contains(&owner_label)) + .context("expected the MCP server owner's permission rules")?; + assert!(permissions.contains(&denied), "{permissions}"); + Ok(()) +} + #[test_case("exec_command")] #[test_case("apply_patch")] #[test_case("request_permissions")] diff --git a/codex-rs/ext/guardian-reviewer/Cargo.toml b/codex-rs/ext/guardian-reviewer/Cargo.toml index 26e73f0f6f..c6adf40e75 100644 --- a/codex-rs/ext/guardian-reviewer/Cargo.toml +++ b/codex-rs/ext/guardian-reviewer/Cargo.toml @@ -19,6 +19,7 @@ codex-context-fragments = { workspace = true } codex-extension-api = { workspace = true } codex-features = { workspace = true } codex-feedback = { workspace = true } +codex-file-system = { workspace = true } codex-guardian-context = { workspace = true } codex-history = { workspace = true } codex-mcp = { workspace = true } diff --git a/codex-rs/ext/guardian-reviewer/src/approval_request.rs b/codex-rs/ext/guardian-reviewer/src/approval_request.rs index 076eb9b877..85e09cad57 100644 --- a/codex-rs/ext/guardian-reviewer/src/approval_request.rs +++ b/codex-rs/ext/guardian-reviewer/src/approval_request.rs @@ -13,6 +13,7 @@ use codex_utils_path_uri::LegacyAppPathString; use codex_utils_path_uri::PathUri; use serde::Serialize; use serde_json::Value; +use std::sync::Arc; #[derive(Clone, PartialEq)] pub enum GuardianApprovalRequest { @@ -74,6 +75,9 @@ pub enum GuardianApprovalRequest { tool_title: Option, tool_description: Option, annotations: Option, + /// Only direct calls to environment-owned servers supply their prepared rules. + /// Thread-owned calls and server-initiated approvals use the issuing context. + prompt_permissions: Option<(String, Arc)>, }, RequestPermissions { id: String, diff --git a/codex-rs/ext/guardian-reviewer/src/approval_request_json.rs b/codex-rs/ext/guardian-reviewer/src/approval_request_json.rs index 893e16f15d..603cbc3618 100644 --- a/codex-rs/ext/guardian-reviewer/src/approval_request_json.rs +++ b/codex-rs/ext/guardian-reviewer/src/approval_request_json.rs @@ -205,6 +205,7 @@ pub fn guardian_approval_request_to_json( tool_title, tool_description, annotations, + prompt_permissions: _, } => serialize_guardian_action(McpToolCallApprovalAction { tool: "mcp_tool_call", server, diff --git a/codex-rs/ext/guardian-reviewer/src/approval_request_json_tests.rs b/codex-rs/ext/guardian-reviewer/src/approval_request_json_tests.rs index 7138cab39d..9fef11051e 100644 --- a/codex-rs/ext/guardian-reviewer/src/approval_request_json_tests.rs +++ b/codex-rs/ext/guardian-reviewer/src/approval_request_json_tests.rs @@ -24,6 +24,15 @@ fn guardian_approval_request_to_json_renders_mcp_tool_call_shape() -> serde_json open_world_hint: None, read_only_hint: Some(false), }), + prompt_permissions: Some(( + "owner".to_string(), + std::sync::Arc::new( + codex_file_system::FileSystemSandboxContext::from_permission_profile( + codex_protocol::models::PermissionProfile::read_only(), + PathUri::parse("file:///workspace").expect("valid executor path"), + ), + ), + )), }; assert_eq!( diff --git a/codex-rs/ext/guardian-reviewer/src/elicitation.rs b/codex-rs/ext/guardian-reviewer/src/elicitation.rs index 26399d564a..8a4fdac4cb 100644 --- a/codex-rs/ext/guardian-reviewer/src/elicitation.rs +++ b/codex-rs/ext/guardian-reviewer/src/elicitation.rs @@ -130,6 +130,7 @@ pub fn guardian_elicitation_review_request( tool_title: metadata_owned_string(meta, MCP_ELICITATION_TOOL_TITLE_KEY), tool_description: metadata_owned_string(meta, MCP_ELICITATION_TOOL_DESCRIPTION_KEY), annotations: None, + prompt_permissions: None, }), } } diff --git a/codex-rs/ext/guardian-reviewer/src/elicitation_tests.rs b/codex-rs/ext/guardian-reviewer/src/elicitation_tests.rs index a1f72f53be..a53f6c4557 100644 --- a/codex-rs/ext/guardian-reviewer/src/elicitation_tests.rs +++ b/codex-rs/ext/guardian-reviewer/src/elicitation_tests.rs @@ -69,6 +69,7 @@ fn guardian_elicitation_review_request_builds_mcp_tool_call( tool_title: Some("Access browser origin".to_owned()), tool_description: None, annotations: None, + prompt_permissions: None, }), } );