From e29071e4c9bf724f9679b7bcf2038d74cc44acd1 Mon Sep 17 00:00:00 2001 From: jif Date: Tue, 2 Jun 2026 20:51:25 +0200 Subject: [PATCH] Add environmentId to request_permissions (#25858) ## Stack 1. #25850 - Key request-permission grants by environment: stores and applies sticky permission grants per environment id. 2. This PR (#25858) - Add `environmentId` to `request_permissions`: lets the model target a selected environment and resolves relative permission paths against it. 3. #25862 - Propagate permission approval environment id: carries the selected environment id through approval events, app-server requests, TUI prompts, and delegate forwarding. 4. #25867 - Add remote request permissions integration coverage: verifies the selected remote environment across request, approval, grant reuse, and exec. This PR is stacked on #25850; #25862 and #25867 are stacked on this PR. ## Why PR1 made request-permission grants internally environment-keyed, but the model-facing `request_permissions` tool could still only target the primary environment. For CCA and multi-environment turns, the tool needs an explicit way to bind a permission request to a selected attached environment before resolving relative paths. ## What Changed - Added optional `environmentId` to `RequestPermissionsArgs`, with `environment_id` accepted as an alias. - Exposed `environmentId` in the `request_permissions` tool schema and description. - Resolve the selected environment before parsing filesystem permission paths, so relative paths bind to the selected environment cwd. - Route validated tool calls through `request_permissions_for_environment` directly instead of duplicating environment lookup in `Session::request_permissions`. - Reject unknown environment ids with a model-facing error. - Updated focused request-permissions and Guardian call sites for the new optional field. ## Testing Not run locally per instruction. --- codex-rs/core/src/codex_delegate.rs | 1 + codex-rs/core/src/session/mod.rs | 24 ------- codex-rs/core/src/session/tests.rs | 63 +++++++++++++++++-- .../core/src/session/tests/guardian_tests.rs | 18 +++++- .../src/tools/handlers/request_permissions.rs | 22 ++++++- .../core/src/tools/handlers/shell_spec.rs | 9 ++- .../src/tools/handlers/shell_spec_tests.rs | 7 +++ codex-rs/protocol/src/request_permissions.rs | 8 +++ 8 files changed, 119 insertions(+), 33 deletions(-) diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 3132420e4..f5f04d2db 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -755,6 +755,7 @@ async fn handle_request_permissions( ) { let call_id = event.call_id; let args = RequestPermissionsArgs { + environment_id: None, reason: event.reason, permissions: event.permissions, }; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 7c49cd742..0abd3c6ec 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2112,30 +2112,6 @@ impl Session { rx_approve } - pub async fn request_permissions( - self: &Arc, - turn_context: &Arc, - call_id: String, - args: RequestPermissionsArgs, - cancellation_token: CancellationToken, - ) -> Option { - let Some(turn_environment) = turn_context.environments.primary() else { - return Some(RequestPermissionsResponse { - permissions: RequestPermissionProfile::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }); - }; - self.request_permissions_for_environment( - turn_context, - call_id, - args, - turn_environment.selection(), - cancellation_token, - ) - .await - } - #[expect( clippy::await_holding_invalid_type, reason = "active turn checks and turn state updates must remain atomic" diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 260e91c09..6f54e54b1 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -5365,11 +5365,17 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests() let turn_context = Arc::clone(&turn_context); let call_id = call_id.clone(); async move { + let environment = turn_context + .environments + .primary() + .expect("primary environment") + .selection(); session - .request_permissions( + .request_permissions_for_environment( &turn_context, call_id, codex_protocol::request_permissions::RequestPermissionsArgs { + environment_id: None, reason: Some("need network".to_string()), permissions: RequestPermissionProfile { network: Some(codex_protocol::models::NetworkPermissions { @@ -5378,6 +5384,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests() ..RequestPermissionProfile::default() }, }, + environment, CancellationToken::new(), ) .await @@ -5409,7 +5416,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests() } #[tokio::test] -async fn request_permissions_tool_resolves_relative_paths_against_primary_environment() { +async fn request_permissions_tool_resolves_relative_paths_against_selected_environment() { let (session, mut turn_context, rx) = make_session_and_context_with_rx().await; *session.active_turn.lock().await = Some(ActiveTurn::default()); let environment_cwd = { @@ -5429,6 +5436,7 @@ async fn request_permissions_tool_resolves_relative_paths_against_primary_enviro mcp_elicitations: true, })) .expect("test setup should allow updating approval policy"); + turn_context_mut.environments.turn_environments[0].environment_id = "remote".to_string(); turn_context_mut.environments.turn_environments[0].cwd = environment_cwd.clone(); let call_id = "call-1".to_string(); @@ -5451,6 +5459,7 @@ async fn request_permissions_tool_resolves_relative_paths_against_primary_enviro source: ToolCallSource::Direct, payload: ToolPayload::Function { arguments: json!({ + "environment_id": "remote", "reason": "need write", "permissions": { "file_system": { @@ -5509,6 +5518,38 @@ async fn request_permissions_tool_resolves_relative_paths_against_primary_enviro .expect("request_permissions handler should succeed"); } +#[tokio::test] +async fn request_permissions_tool_rejects_unknown_environment_id() { + let (session, turn_context) = make_session_and_context().await; + let result = RequestPermissionsHandler + .handle(ToolInvocation { + session: Arc::new(session), + turn: Arc::new(turn_context), + cancellation_token: CancellationToken::new(), + tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())), + call_id: "call-1".to_string(), + tool_name: codex_tools::ToolName::plain("request_permissions"), + source: ToolCallSource::Direct, + payload: ToolPayload::Function { + arguments: json!({ + "environment_id": "missing", + "permissions": { + "network": { + "enabled": true, + }, + }, + }) + .to_string(), + }, + }) + .await; + + let Err(FunctionCallError::RespondToModel(output)) = result else { + panic!("expected unknown environment id to be rejected"); + }; + assert_eq!(output, "unknown turn environment id `missing`"); +} + #[tokio::test] async fn request_permissions_response_materializes_session_cwd_grants_before_recording() { let (session, mut turn_context, rx) = make_session_and_context_with_rx().await; @@ -5547,14 +5588,21 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec let call_id = call_id.clone(); let requested_permissions = requested_permissions.clone(); async move { + let environment = turn_context + .environments + .primary() + .expect("primary environment") + .selection(); session - .request_permissions( + .request_permissions_for_environment( &turn_context, call_id, codex_protocol::request_permissions::RequestPermissionsArgs { + environment_id: None, reason: Some("need cwd write".to_string()), permissions: requested_permissions, }, + environment, CancellationToken::new(), ) .await @@ -5627,11 +5675,17 @@ async fn request_permissions_is_auto_denied_when_granular_policy_blocks_tool_req let session = Arc::new(session); let turn_context = Arc::new(turn_context); let call_id = "call-1".to_string(); + let environment = turn_context + .environments + .primary() + .expect("primary environment") + .selection(); let response = session - .request_permissions( + .request_permissions_for_environment( &turn_context, call_id, codex_protocol::request_permissions::RequestPermissionsArgs { + environment_id: None, reason: Some("need network".to_string()), permissions: RequestPermissionProfile { network: Some(codex_protocol::models::NetworkPermissions { @@ -5640,6 +5694,7 @@ async fn request_permissions_is_auto_denied_when_granular_policy_blocks_tool_req ..RequestPermissionProfile::default() }, }, + environment, CancellationToken::new(), ) .await; diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index e8fccd7d7..d8b46c999 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -124,15 +124,22 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { }), ..RequestPermissionProfile::default() }; + let environment = turn_context + .environments + .primary() + .expect("primary environment") + .selection(); let response = tokio::time::timeout( Duration::from_secs(45), - session.request_permissions( + session.request_permissions_for_environment( &turn_context, "perm-call-1".to_string(), RequestPermissionsArgs { + environment_id: None, reason: Some("need network".to_string()), permissions: requested_permissions.clone(), }, + environment, CancellationToken::new(), ), ) @@ -213,14 +220,21 @@ async fn request_permissions_guardian_review_stops_when_cancelled() { let requested_permissions = requested_permissions.clone(); let cancellation_token = cancellation_token.clone(); async move { + let environment = turn_context + .environments + .primary() + .expect("primary environment") + .selection(); session - .request_permissions( + .request_permissions_for_environment( &turn_context, "perm-call-cancelled".to_string(), RequestPermissionsArgs { + environment_id: None, reason: Some("need network".to_string()), permissions: requested_permissions, }, + environment, cancellation_token, ) .await diff --git a/codex-rs/core/src/tools/handlers/request_permissions.rs b/codex-rs/core/src/tools/handlers/request_permissions.rs index f4a89fe0c..a20ff2b66 100644 --- a/codex-rs/core/src/tools/handlers/request_permissions.rs +++ b/codex-rs/core/src/tools/handlers/request_permissions.rs @@ -6,16 +6,25 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; +use crate::tools::handlers::parse_arguments; use crate::tools::handlers::parse_arguments_with_base_path; +use crate::tools::handlers::resolve_tool_environment; use crate::tools::handlers::shell_spec::create_request_permissions_tool; use crate::tools::handlers::shell_spec::request_permissions_tool_description; use crate::tools::registry::CoreToolRuntime; use crate::tools::registry::ToolExecutor; use codex_tools::ToolName; use codex_tools::ToolSpec; +use serde::Deserialize; pub struct RequestPermissionsHandler; +#[derive(Deserialize)] +struct RequestPermissionsEnvironmentArgs { + #[serde(default, rename = "environment_id", alias = "environmentId")] + environment_id: Option, +} + #[async_trait::async_trait] impl ToolExecutor for RequestPermissionsHandler { fn tool_name(&self) -> ToolName { @@ -48,7 +57,10 @@ impl ToolExecutor for RequestPermissionsHandler { } }; - let Some(turn_environment) = turn.environments.primary() else { + let environment_args: RequestPermissionsEnvironmentArgs = parse_arguments(&arguments)?; + let Some(turn_environment) = + resolve_tool_environment(turn.as_ref(), environment_args.environment_id.as_deref())? + else { return Err(FunctionCallError::RespondToModel( "request_permissions requires a primary environment".to_string(), )); @@ -65,7 +77,13 @@ impl ToolExecutor for RequestPermissionsHandler { } let response = session - .request_permissions(&turn, call_id, args, cancellation_token) + .request_permissions_for_environment( + &turn, + call_id, + args, + turn_environment.selection(), + cancellation_token, + ) .await .ok_or_else(|| { FunctionCallError::RespondToModel( diff --git a/codex-rs/core/src/tools/handlers/shell_spec.rs b/codex-rs/core/src/tools/handlers/shell_spec.rs index 13c531eab..3fb563b8d 100644 --- a/codex-rs/core/src/tools/handlers/shell_spec.rs +++ b/codex-rs/core/src/tools/handlers/shell_spec.rs @@ -229,6 +229,13 @@ pub fn create_request_permissions_tool(description: String) -> ToolSpec { "Optional short explanation for why additional permissions are needed.".to_string(), )), ), + ( + "environment_id".to_string(), + JsonSchema::string(Some( + "Environment id from . Omit to use the primary environment." + .to_string(), + )), + ), ("permissions".to_string(), permission_profile_schema()), ]); @@ -247,7 +254,7 @@ pub fn create_request_permissions_tool(description: String) -> ToolSpec { } pub fn request_permissions_tool_description() -> String { - "Request additional filesystem or network permissions from the user and wait for the client to grant a subset of the requested permission profile. Granted permissions apply automatically to later shell-like commands in the current turn, or for the rest of the session if the client approves them at session scope." + "Request additional filesystem or network permissions from the user and wait for the client to grant a subset of the requested permission profile. Use environment_id to target a specific attached environment; omit it to use the primary environment. Relative filesystem paths resolve against the selected environment cwd. Granted permissions apply automatically to later shell-like commands in the current turn, or for the rest of the session if the client approves them at session scope." .to_string() } diff --git a/codex-rs/core/src/tools/handlers/shell_spec_tests.rs b/codex-rs/core/src/tools/handlers/shell_spec_tests.rs index b7c0db724..b7bbfa54a 100644 --- a/codex-rs/core/src/tools/handlers/shell_spec_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_spec_tests.rs @@ -172,6 +172,13 @@ fn request_permissions_tool_includes_full_permission_schema() { "Optional short explanation for why additional permissions are needed.".to_string(), )), ), + ( + "environment_id".to_string(), + JsonSchema::string(Some( + "Environment id from . Omit to use the primary environment." + .to_string(), + )), + ), ("permissions".to_string(), permission_profile_schema()), ]); diff --git a/codex-rs/protocol/src/request_permissions.rs b/codex-rs/protocol/src/request_permissions.rs index be6b88ef5..43e8b8ee9 100644 --- a/codex-rs/protocol/src/request_permissions.rs +++ b/codex-rs/protocol/src/request_permissions.rs @@ -48,6 +48,14 @@ impl From for RequestPermissionProfile { #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] pub struct RequestPermissionsArgs { + #[serde( + default, + rename = "environment_id", + alias = "environmentId", + skip_serializing_if = "Option::is_none" + )] + #[ts(optional)] + pub environment_id: Option, #[serde(skip_serializing_if = "Option::is_none")] pub reason: Option, pub permissions: RequestPermissionProfile,