From 1f52d407041d933e490ca3033729bce6319d07fa Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Thu, 1 Oct 2026 05:22:48 +0000 Subject: [PATCH] Bind permission grants to the originating turn (#49880) ## Why Background code-mode cells can continue running after a new turn starts. Looking up grants and strict review requirements through the active turn can make those cells lose their approved access or use another turn's permissions. ## What changed - Keep turn-scoped grants and strict review state in `TurnContext`, retained by captured steps and background cells. - Record permission responses against the requesting turn, including requests made after a later turn starts. - Share live session-scoped grants across turns while keeping turn-scoped grants isolated. - Use the originating turn's permissions and review requirements for tool execution, patches, MCP calls, file uploads, and terminal input. ## Testing Add regression coverage for background cells retaining grants across turn changes, turn versus session scope, permission requests after the next turn starts, and strict review enabled while `write_stdin` is queued. GitOrigin-RevId: 641502fbfd43553c2c42424d4aaa53ae8020c850 --- .../tests/suite/v2/request_permissions.rs | 494 ++++++++++++++++++ codex-rs/core/src/mcp_openai_file.rs | 11 +- codex-rs/core/src/mcp_tool_call.rs | 11 +- codex-rs/core/src/mcp_tool_call_tests.rs | 15 +- codex-rs/core/src/session/mod.rs | 85 +-- codex-rs/core/src/session/review.rs | 57 +- codex-rs/core/src/session/session.rs | 1 + codex-rs/core/src/session/step_context.rs | 2 +- codex-rs/core/src/session/tests.rs | 179 +++---- .../core/src/session/tests/guardian_tests.rs | 83 ++- codex-rs/core/src/session/turn_context.rs | 144 +++++ codex-rs/core/src/state/service.rs | 29 + codex-rs/core/src/state/session.rs | 30 -- codex-rs/core/src/state/turn.rs | 39 +- .../core/src/tools/handlers/apply_patch.rs | 22 +- .../src/tools/handlers/extension_tools.rs | 3 +- codex-rs/core/src/tools/handlers/mod.rs | 13 +- .../handlers/unified_exec/exec_command.rs | 5 +- .../src/tools/handlers/unified_exec_tests.rs | 18 +- codex-rs/core/src/tools/orchestrator.rs | 2 +- codex-rs/core/src/unified_exec/mod_tests.rs | 84 ++- .../core/src/unified_exec/process_manager.rs | 2 +- 22 files changed, 914 insertions(+), 415 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/request_permissions.rs b/codex-rs/app-server/tests/suite/v2/request_permissions.rs index fbd21f6c5a..3a061e93e7 100644 --- a/codex-rs/app-server/tests/suite/v2/request_permissions.rs +++ b/codex-rs/app-server/tests/suite/v2/request_permissions.rs @@ -9,6 +9,8 @@ use codex_app_server_protocol::AdditionalPermissionProfile; use codex_app_server_protocol::ClientRequest; use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; +use codex_app_server_protocol::FileChangeApprovalDecision; +use codex_app_server_protocol::FileChangeRequestApprovalResponse; use codex_app_server_protocol::JSONRPCMessage; use codex_app_server_protocol::PermissionGrantScope; use codex_app_server_protocol::PermissionsRequestApprovalResponse; @@ -25,6 +27,7 @@ use codex_exec_server::ReadFileOptions; use codex_features::Feature; use codex_models_manager::model_info::model_info_from_slug; use codex_protocol::models::FileSystemPermissions; +use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::request_permissions::PermissionGrantScope as CorePermissionGrantScope; use codex_protocol::request_permissions::RequestPermissionProfile; use codex_protocol::request_permissions::RequestPermissionsResponse; @@ -239,6 +242,497 @@ text(await tools.exec_command({{cmd: "{command}"}}));"#, Ok(()) } +/// A step captures earlier RPC grants and keeps them after the active turn changes. +/// The new turn inherits session grants but must request approval for turn-scoped grants. +#[test_case(PermissionGrantScope::Turn; "turn")] +#[test_case(PermissionGrantScope::Session; "session")] +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn steps_capture_permission_grants_and_preserve_their_scope( + scope: PermissionGrantScope, +) -> Result<()> { + skip_if_wine_exec!( + Ok(()), + "Wine does not emulate Windows restricted-token and ACL sandbox semantics" + ); + let codex_home = tempfile::TempDir::new()?; + let server = responses::start_mock_server().await; + MockResponsesConfig::new(&server.uri()) + .with_model("gpt-5.4") + .with_approval_policy("on-request") + .with_sandbox_mode("read-only") + .with_root_config(r#"approvals_reviewer = "user""#) + .enable_feature(Feature::RequestPermissionsTool) + .enable_feature(Feature::CodeModeOnly) + .write(codex_home.path())?; + let mut model = model_info_from_slug("gpt-5.4"); + model.apply_patch_tool_type = Some(ApplyPatchToolType::Freeform); + model.experimental_supported_tools = vec!["test_sync_tool".to_string()]; + write_models_cache_with_models(codex_home.path(), vec![model]).await?; + let mut app = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized() + .await?; + let cwd = app.auto_env()?.selection().cwd.clone(); + let fs = app.auto_env()?.environment().get_filesystem(); + let ThreadStartResponse { thread, .. } = app.start_thread(ThreadStartParams::default()).await?; + + // The old cell cannot resume until a tool is executing in the next turn. + let barrier = r#"await tools.test_sync_tool({barrier: { + id: "permission-grant-turns", participants: 2, timeout_ms: 60000 + }});"#; + let mut first_cell_id = None; + for turn_index in 0..2 { + let events = match &first_cell_id { + None => vec![ + // Request approval in an earlier step so the file-writing cell captures it. + responses::sse(vec![ + responses::ev_response_created("request-grant"), + responses::ev_custom_tool_call( + "request-grant-cell", + "exec", + r#"// @exec: {"yield_time_ms": 60000} +await tools.request_permissions({permissions: {file_system: {write: ["."]}}});"#, + ), + responses::ev_completed("request-grant"), + ]), + responses::sse(vec![ + responses::ev_response_created("first-turn"), + responses::ev_custom_tool_call( + "first-cell", + "exec", + &format!( + r#"// @exec: {{"yield_time_ms": 60000}} +text(await tools.apply_patch("*** Begin Patch\n*** Add File: first.txt\n+first turn\n*** End Patch")); +yield_control(); +{barrier} +text(await tools.apply_patch("*** Begin Patch\n*** Add File: original.txt\n+original turn\n*** End Patch"));"#, + ), + ), + responses::ev_completed("first-turn"), + ]), + create_final_assistant_message_sse_response("first turn done")?, + ], + Some(cell_id) => vec![ + responses::sse(vec![ + responses::ev_response_created("next-turn"), + responses::ev_custom_tool_call( + "next-cell", + "exec", + &format!( + r#"// @exec: {{"yield_time_ms": 60000}} +{barrier} +text(await tools.apply_patch("*** Begin Patch\n*** Add File: next.txt\n+next turn\n*** End Patch"));"#, + ), + ), + responses::ev_completed("next-turn"), + ]), + responses::sse(vec![ + responses::ev_response_created("wait-original"), + responses::ev_function_call( + "wait-original", + "wait", + &json!({ + "cell_id": cell_id, "yield_time_ms": 60000, + }) + .to_string(), + ), + responses::ev_completed("wait-original"), + ]), + create_final_assistant_message_sse_response("next turn done")?, + ], + }; + let mock = responses::mount_sse_sequence(&server, events).await; + let TurnStartResponse { turn } = app + .request(|request_id| ClientRequest::TurnStart { + request_id, + params: TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "write the file".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }, + }) + .await?; + let mut saw_approval = false; + loop { + match timeout(DEFAULT_READ_TIMEOUT, app.read_next_message()).await?? { + JSONRPCMessage::Request(request) => { + let request: ServerRequest = + serde_json::from_value(serde_json::to_value(request)?)?; + match request { + ServerRequest::PermissionsRequestApproval { request_id, params } => { + assert_eq!((turn_index, saw_approval), (0, false)); + assert_eq!( + (¶ms.thread_id, ¶ms.turn_id), + (&thread.id, &turn.id) + ); + app.send_response( + request_id, + json!({"permissions": params.permissions, "scope": scope}), + ) + .await?; + } + ServerRequest::FileChangeRequestApproval { request_id, params } => { + assert_eq!( + (turn_index, scope, saw_approval), + (1, PermissionGrantScope::Turn, false) + ); + assert_eq!( + (¶ms.thread_id, ¶ms.turn_id), + (&thread.id, &turn.id) + ); + app.send_response( + request_id, + serde_json::to_value(FileChangeRequestApprovalResponse { + decision: FileChangeApprovalDecision::Decline, + })?, + ) + .await?; + } + request => anyhow::bail!("unexpected server request: {request:?}"), + } + saw_approval = true; + } + JSONRPCMessage::Notification(notification) + if notification.method == "turn/completed" => + { + let completed: TurnCompletedNotification = serde_json::from_value( + notification.params.expect("turn/completed params"), + )?; + assert_eq!( + ( + completed.thread_id, + completed.turn.id, + completed.turn.status + ), + (thread.id.clone(), turn.id, TurnStatus::Completed), + ); + break; + } + _ => {} + } + } + assert_eq!( + saw_approval, + turn_index == 0 || scope == PermissionGrantScope::Turn + ); + if turn_index == 0 { + assert_eq!( + fs.read_file( + &cwd.join("first.txt")?, + ReadFileOptions::default(), + /*sandbox*/ None + ) + .await?, + b"first turn\n", + ); + let requests = mock.requests(); + assert_eq!(requests.len(), 3); + let output = requests[2].custom_tool_call_output("first-cell"); + let header = output["output"][0]["text"].as_str().expect("cell status"); + first_cell_id = Some( + header + .strip_prefix("Script running with cell ID ") + .and_then(|rest| rest.lines().next()) + .expect("first cell must yield before the next turn") + .to_string(), + ); + } else { + let requests = mock.requests(); + assert_eq!(requests.len(), 3); + let output = requests[2].function_call_output("wait-original"); + assert!( + output["output"][0]["text"] + .as_str() + .expect("cell status") + .starts_with("Script completed") + ); + } + } + + // The first cell writes with its own grant after the active turn has changed. + assert_eq!( + fs.read_file( + &cwd.join("original.txt")?, + ReadFileOptions::default(), + /*sandbox*/ None + ) + .await?, + b"original turn\n" + ); + match scope { + PermissionGrantScope::Session => { + assert_eq!( + fs.read_file( + &cwd.join("next.txt")?, + ReadFileOptions::default(), + /*sandbox*/ None + ) + .await?, + b"next turn\n" + ); + } + PermissionGrantScope::Turn => { + assert_eq!( + fs.read_file( + &cwd.join("next.txt")?, + ReadFileOptions::default(), + /*sandbox*/ None + ) + .await + .expect_err("the declined write must not create a file") + .kind(), + std::io::ErrorKind::NotFound, + ); + } + } + Ok(()) +} + +/// Permission requests from a background cell still belong to its original turn. +/// Its next operation must observe both the approved access and any strict-review requirement. +#[test_case(false; "turn_grant")] +#[test_case(true; "strict_review")] +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn background_cell_uses_permissions_requested_after_next_turn_starts( + strict_auto_review: bool, +) -> Result<()> { + skip_if_wine_exec!( + Ok(()), + "Wine does not emulate Windows restricted-token and ACL sandbox semantics" + ); + let codex_home = tempfile::TempDir::new()?; + let server = responses::start_mock_server().await; + MockResponsesConfig::new(&server.uri()) + .with_model("gpt-5.4") + .with_approval_policy("on-request") + .with_sandbox_mode("read-only") + .with_root_config(r#"approvals_reviewer = "user""#) + .enable_feature(Feature::RequestPermissionsTool) + .enable_feature(Feature::CodeModeOnly) + .enable_feature(Feature::GuardianApproval) + .write(codex_home.path())?; + let mut model = model_info_from_slug("gpt-5.4"); + model.apply_patch_tool_type = Some(ApplyPatchToolType::Freeform); + model.experimental_supported_tools = vec!["test_sync_tool".to_string()]; + write_models_cache_with_models(codex_home.path(), vec![model]).await?; + let mut app = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized() + .await?; + let cwd = app.auto_env()?.selection().cwd.clone(); + let fs = app.auto_env()?.environment().get_filesystem(); + let ThreadStartResponse { thread, .. } = app.start_thread(ThreadStartParams::default()).await?; + let operation = if strict_auto_review { + r#"text(await tools.exec_command({cmd: "echo background-cell-review"}));"# + } else { + r#"text(await tools.apply_patch("*** Begin Patch\n*** Add File: granted.txt\n+background-cell-grant\n*** End Patch"));"# + }; + let started = r#"await tools.test_sync_tool({barrier: { + id: "next-turn-started", participants: 2, timeout_ms: 60000 + }});"#; + let first_mock = responses::mount_sse_sequence( + &server, + vec![ + responses::sse(vec![ + responses::ev_response_created("first-turn"), + responses::ev_custom_tool_call( + "background-cell", + "exec", + &format!( + r#"// @exec: {{"yield_time_ms": 60000}} +yield_control(); +{started} +await tools.request_permissions({{permissions: {{file_system: {{write: ["."]}}}}}}); +{operation}"#, + ), + ), + responses::ev_completed("first-turn"), + ]), + create_final_assistant_message_sse_response("first turn done")?, + ], + ) + .await; + let TurnStartResponse { turn: first_turn } = app + .request(|request_id| ClientRequest::TurnStart { + request_id, + params: TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "start a background cell".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }, + }) + .await?; + let completed: TurnCompletedNotification = timeout( + DEFAULT_READ_TIMEOUT, + app.read_notification("turn/completed"), + ) + .await??; + assert_eq!(completed.turn.id, first_turn.id); + assert_eq!(completed.turn.status, TurnStatus::Completed); + let requests = first_mock.requests(); + let (output, _) = requests + .last() + .expect("first turn continuation") + .custom_tool_call_output_content_and_success("background-cell") + .context("background cell output")?; + let output = output.context("background cell status")?; + let cell_id = output + .strip_prefix("Script running with cell ID ") + .and_then(|rest| rest.lines().next()) + .with_context(|| format!("background cell must yield: {output}"))?; + + // Guardian and the parent may sample concurrently after the barrier releases. + let guardian_mock = responses::mount_sse_once_match( + &server, + wiremock::matchers::body_partial_json(json!({ + "client_metadata": {"x-openai-subagent": "guardian"}, + })), + responses::sse(vec![ + responses::ev_response_created("guardian-review"), + responses::ev_assistant_message( + "guardian-review", + r#"{"outcome":"deny","rationale":"background command requires review"}"#, + ), + responses::ev_completed("guardian-review"), + ]), + ) + .await; + let mut events = vec![responses::sse(vec![ + responses::ev_response_created("next-turn"), + responses::ev_custom_tool_call( + "next-cell", + "exec", + &format!("// @exec: {{\"yield_time_ms\": 60000}}\n{started}"), + ), + responses::ev_completed("next-turn"), + ])]; + events.push(responses::sse(vec![ + responses::ev_response_created("wait-background"), + responses::ev_function_call( + "wait-background", + "wait", + &json!({"cell_id": cell_id, "yield_time_ms": 60000}).to_string(), + ), + responses::ev_completed("wait-background"), + ])); + events.push(create_final_assistant_message_sse_response( + "next turn done", + )?); + let mock = responses::mount_sse_sequence(&server, events).await; + let TurnStartResponse { turn } = app + .request(|request_id| ClientRequest::TurnStart { + request_id, + params: TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "resume the background cell".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }, + }) + .await?; + let mut saw_permission_request = false; + let mut unexpected_approvals = Vec::new(); + loop { + match timeout(DEFAULT_READ_TIMEOUT, app.read_next_message()).await?? { + JSONRPCMessage::Request(request) => match ServerRequest::try_from(request)? { + ServerRequest::PermissionsRequestApproval { request_id, params } => { + assert!(!saw_permission_request); + assert_eq!( + (¶ms.thread_id, ¶ms.turn_id), + (&thread.id, &first_turn.id) + ); + app.send_response( + request_id, + json!({ + "permissions": params.permissions, + "scope": PermissionGrantScope::Turn, + "strictAutoReview": strict_auto_review, + }), + ) + .await?; + saw_permission_request = true; + } + ServerRequest::FileChangeRequestApproval { request_id, params } => { + unexpected_approvals.push(params.turn_id); + app.send_response( + request_id, + serde_json::to_value(FileChangeRequestApprovalResponse { + decision: FileChangeApprovalDecision::Decline, + })?, + ) + .await?; + } + request => anyhow::bail!("unexpected server request: {request:?}"), + }, + JSONRPCMessage::Notification(notification) + if notification.method == "turn/completed" => + { + let completed: TurnCompletedNotification = + serde_json::from_value(notification.params.expect("turn/completed params"))?; + assert_eq!( + ( + completed.thread_id, + completed.turn.id, + completed.turn.status + ), + (thread.id.clone(), turn.id, TurnStatus::Completed) + ); + break; + } + _ => {} + } + } + assert!(saw_permission_request); + assert_eq!( + unexpected_approvals, + Vec::::new(), + "A's approved write must not need a second approval after B starts" + ); + let requests = mock.requests(); + if strict_auto_review { + assert_eq!( + guardian_mock + .requests() + .iter() + .filter( + |request| request.body_json()["client_metadata"]["x-openai-subagent"] + == "guardian" + ) + .count(), + 1, + "strict review must apply to A's next command after B starts" + ); + let output = requests + .last() + .expect("next turn continuation") + .function_call_output("wait-background"); + assert!( + output + .to_string() + .contains("background command requires review") + ); + } else { + assert_eq!( + fs.read_file( + &cwd.join("granted.txt")?, + ReadFileOptions::default(), + /*sandbox*/ None + ) + .await?, + b"background-cell-grant\n" + ); + } + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn request_permissions_round_trip() -> Result<()> { let codex_home = tempfile::TempDir::new()?; diff --git a/codex-rs/core/src/mcp_openai_file.rs b/codex-rs/core/src/mcp_openai_file.rs index 110a585fce..521b9148dc 100644 --- a/codex-rs/core/src/mcp_openai_file.rs +++ b/codex-rs/core/src/mcp_openai_file.rs @@ -20,7 +20,6 @@ use codex_exec_server::GetMetadataOptions; use codex_login::CodexAuth; use codex_protocol::permissions::FileSystemAccessMode; use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; -use codex_sandboxing::policy_transforms::merge_permission_profiles; use serde_json::Value as JsonValue; use std::collections::HashMap; use std::sync::Arc; @@ -162,14 +161,8 @@ async fn build_uploaded_argument_value( .cwd() .join(file_path) .map_err(|error| contextualize_error(error.to_string()))?; - let additional_permissions = merge_permission_profiles( - sess.granted_session_permissions(&turn_environment.selection.environment_id) - .await - .as_ref(), - sess.granted_turn_permissions(&turn_environment.selection.environment_id) - .await - .as_ref(), - ); + let additional_permissions = + turn_context.granted_permissions(&turn_environment.selection.environment_id); let file_system_policy = effective_file_system_sandbox_policy( &turn_environment .permission_profile() diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index c2066132b8..26d31b12ce 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -1497,16 +1497,7 @@ async fn maybe_request_mcp_tool_approval( policy: McpToolApprovalPolicy, ) -> Option { let turn_context = &step_context.turn; - let turn_state = sess - .active_turn - .lock() - .await - .as_ref() - .map(|active| Arc::clone(&active.turn_state)); - let strict_auto_review = match turn_state { - Some(turn_state) => turn_state.lock().await.strict_auto_review_enabled(), - None => false, - }; + let strict_auto_review = turn_context.strict_auto_review_enabled(); let approvals_reviewer = connectors::mcp_approvals_reviewer_from_layers( &config.config_layer_stack, step_context diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 12b2ab9a18..a1c0549575 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -3126,13 +3126,6 @@ async fn strict_auto_review_forces_guardian_for_mcp_policy_skip() { turn_context.auth_manager.clone(), ); - let active_turn = ActiveTurn::default(); - active_turn - .turn_state - .lock() - .await - .enable_strict_auto_review(); - *session.active_turn.lock().await = Some(active_turn); let session = Arc::new(session); let turn_context = Arc::new(turn_context); let invocation = McpInvocation { @@ -3161,9 +3154,15 @@ async fn strict_auto_review_forces_guardian_for_mcp_policy_skip() { .set(AskForApproval::OnRequest) .expect("captured MCP policy should allow updating approval policy"); + turn_context.record_granted_permissions( + codex_exec_server::LOCAL_ENVIRONMENT_ID, + Default::default(), + /*strict_auto_review*/ true, + ); + let step_context = StepContext::for_test(Arc::clone(&turn_context)); let decision = maybe_request_mcp_tool_approval( &session, - &StepContext::for_test(Arc::clone(&turn_context)), + &step_context, &CancellationToken::new(), "call-guardian-deny", &invocation, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 1697cd0e83..f24e68ab7f 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2937,10 +2937,6 @@ impl Session { let sandbox_context = environment.sandbox_context(/*additional_permissions*/ None); let context = sandbox_context.policy_context(); { - let originating_turn_state = { - let active = self.active_turn.lock().await; - active.as_ref().map(|active| Arc::clone(&active.turn_state)) - }; let action = ApprovalAction::RequestPermissions { id: call_id.clone(), environment_id: environment_selection.environment_id.clone(), @@ -3008,9 +3004,8 @@ impl Session { self.record_granted_request_permissions_for_turn( &response, &environment.selection.environment_id, - originating_turn_state.as_ref(), - ) - .await; + turn_context, + ); return Some(response); } } @@ -3028,6 +3023,7 @@ impl Session { tx_response, requested_permissions: requested_permissions.clone(), environment: environment.clone(), + turn_context: Arc::clone(turn_context), }, ) } @@ -3150,16 +3146,14 @@ impl Session { call_id: &str, response: RequestPermissionsResponse, ) { - let (entry, originating_turn_state) = { + let entry = { let mut active = self.active_turn.lock().await; match active.as_mut() { Some(at) => { let mut ts = at.turn_state.lock().await; - let entry = ts.remove_pending_request_permissions(call_id); - let originating_turn_state = entry.as_ref().map(|_| Arc::clone(&at.turn_state)); - (entry, originating_turn_state) + ts.remove_pending_request_permissions(call_id) } - None => (None, None), + None => None, } }; match entry { @@ -3175,9 +3169,8 @@ impl Session { self.record_granted_request_permissions_for_turn( &response, &entry.environment.selection.environment_id, - originating_turn_state.as_ref(), - ) - .await; + &entry.turn_context, + ); entry.tx_response.send(response).ok(); } None => { @@ -3215,30 +3208,26 @@ impl Session { } } - async fn record_granted_request_permissions_for_turn( + fn record_granted_request_permissions_for_turn( &self, response: &RequestPermissionsResponse, environment_id: &str, - originating_turn_state: Option<&Arc>>, + turn_context: &TurnContext, ) { if response.permissions.is_empty() { return; } match response.scope { PermissionGrantScope::Turn => { - if let Some(turn_state) = originating_turn_state { - let mut ts = turn_state.lock().await; - let permissions: AdditionalPermissionProfile = - response.permissions.clone().into(); - ts.record_granted_permissions(environment_id, permissions); - if response.strict_auto_review { - ts.enable_strict_auto_review(); - } - } + let permissions: AdditionalPermissionProfile = response.permissions.clone().into(); + turn_context.record_granted_permissions( + environment_id, + permissions, + response.strict_auto_review, + ); } PermissionGrantScope::Session => { - let mut state = self.state.lock().await; - state.record_granted_permissions( + self.services.record_granted_permissions( environment_id, response.permissions.clone().into(), ); @@ -3246,24 +3235,6 @@ impl Session { } } - #[expect( - clippy::await_holding_invalid_type, - reason = "active turn reads must stay consistent with the matching turn state" - )] - pub(crate) async fn granted_turn_permissions( - &self, - environment_id: &str, - ) -> Option { - let active = self.active_turn.lock().await; - let active = active.as_ref()?; - let ts = active.turn_state.lock().await; - ts.granted_permissions(environment_id) - } - - #[expect( - clippy::await_holding_invalid_type, - reason = "active turn reads must stay consistent with the matching turn state" - )] pub(crate) async fn active_turn_context_and_strict_auto_review( &self, ) -> Option<( @@ -3277,31 +3248,11 @@ impl Session { let task = active.task.as_ref()?; let turn_context = Arc::clone(&task.turn_context); let settings = turn_context.next_step_settings.load_full(); - let strict_auto_review = active.turn_state.lock().await.strict_auto_review_enabled(); + let strict_auto_review = turn_context.strict_auto_review_enabled(); let environments = self.services.turn_environments.snapshot_now(); Some((turn_context, settings, environments, strict_auto_review)) } - #[expect( - clippy::await_holding_invalid_type, - reason = "active turn reads must stay consistent with the matching turn state" - )] - pub(crate) async fn strict_auto_review_enabled(&self) -> bool { - let active = self.active_turn.lock().await; - let Some(active) = active.as_ref().filter(|active| active.task.is_some()) else { - return false; - }; - active.turn_state.lock().await.strict_auto_review_enabled() - } - - pub(crate) async fn granted_session_permissions( - &self, - environment_id: &str, - ) -> Option { - let state = self.state.lock().await; - state.granted_permissions(environment_id) - } - #[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/review.rs b/codex-rs/core/src/session/review.rs index 291eb1280a..e7f90646e2 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -1,7 +1,5 @@ use super::step_settings::ResolvedStepSettings; use super::*; -use arc_swap::ArcSwap; -use std::sync::atomic::AtomicBool; /// Spawn a review thread using the given prompt. pub(super) async fn spawn_review_thread( @@ -42,8 +40,6 @@ pub(super) async fn spawn_review_thread( ); let review_prompt = resolved.prompt.clone(); - let provider = parent_turn_context.provider.clone(); - let auth_manager = parent_turn_context.auth_manager.clone(); let model_info = review_model_info.clone(); let mut selected = parent_turn_context.initial_settings.selected().clone(); let mut reasoning_effort = selected.collaboration_mode.reasoning_effort(); @@ -77,8 +73,6 @@ pub(super) async fn spawn_review_thread( ); } - let auth_manager_for_context = auth_manager.clone(); - let provider_for_context = provider.clone(); let session_source = parent_turn_context.session_source.clone(); let (forked_from_thread_id, thread_source, service_tier) = { let state = sess.state.lock().await; @@ -110,8 +104,6 @@ pub(super) async fn spawn_review_thread( per_turn_config.model = Some(model); per_turn_config.model_reasoning_effort = reasoning_effort; per_turn_config.service_tier = step_settings.service_tier.clone(); - let session_telemetry_for_context = - step_settings.telemetry(&parent_turn_context.session_telemetry); let per_turn_config = Arc::new(per_turn_config); let review_turn_id = sub_id.to_string(); #[allow(deprecated)] @@ -145,53 +137,14 @@ pub(super) async fn spawn_review_thread( &model_info, )); - let extension_data = Arc::new(codex_extension_api::ExtensionData::new( - review_turn_id.clone(), - )); - extension_data.insert(parent_turn_context.skills_snapshot().as_ref().clone()); - - let review_turn_context = TurnContext { - sub_id: review_turn_id.clone(), - trace_id: current_span_trace_id(), - realtime_active: parent_turn_context.realtime_active, - code_mode_available: parent_turn_context.code_mode_available, - configured_token_budget: per_turn_config.token_budget.clone(), - use_model_token_budget_defaults: per_turn_config.features.enabled(Feature::TokenBudget) - && !super::token_budget::has_explicit_settings(&per_turn_config), - config: per_turn_config, - auth_manager: auth_manager_for_context, - initial_settings: Arc::clone(&step_settings), - disabled_plugin_ids: parent_turn_context.disabled_plugin_ids.clone(), - active_host_plugin_identities: None, - next_step_settings: ArcSwap::from(step_settings), - session_telemetry: session_telemetry_for_context, - provider: provider_for_context, - session_source, - history_mode: parent_turn_context.history_mode, - parent_thread_id: parent_turn_context.parent_thread_id, - originator: parent_turn_context.originator.clone(), - initial_environments: parent_turn_context.initial_environments.clone(), + let review_turn_context = parent_turn_context.for_review( + review_turn_id, + per_turn_config, + step_settings, available_models, unified_exec_shell_mode, - current_date: parent_turn_context.current_date.clone(), - timezone: parent_turn_context.timezone.clone(), - app_server_client_name: parent_turn_context.app_server_client_name.clone(), - developer_instructions: None, - multi_agent_version: MultiAgentVersion::Disabled, - network: parent_turn_context.network.clone(), - windows_sandbox_level: parent_turn_context.windows_sandbox_level, - #[allow(deprecated)] - cwd: parent_turn_context.cwd.clone(), - final_output_json_schema: None, - dynamic_tools: parent_turn_context.dynamic_tools.clone(), turn_metadata_state, - extension_data, - turn_timing_state: Arc::new(TurnTimingState::default()), - terminal_error: Arc::new(Mutex::new(None)), - server_model_warning_emitted: AtomicBool::new(false), - model_verification_emitted: AtomicBool::new(false), - cyber_access_program: None, - }; + ); // Seed the child task with the review prompt as the initial user message. let input = vec![TurnInput::UserInput { diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 07ecc2f1db..9452040178 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1734,6 +1734,7 @@ impl Session { models_manager: Arc::clone(&models_manager), git_root_discovery, tool_approvals: Mutex::new(ApprovalStore::default()), + granted_permissions_by_environment_id: Arc::default(), runtime_handle: tokio::runtime::Handle::current(), skills_service, agents_md_manager, diff --git a/codex-rs/core/src/session/step_context.rs b/codex-rs/core/src/session/step_context.rs index d0f6d77c90..5c1006c900 100644 --- a/codex-rs/core/src/session/step_context.rs +++ b/codex-rs/core/src/session/step_context.rs @@ -1,4 +1,4 @@ -//! Request-scoped settings and capabilities, including the durable context snapshot. +//! Request-scoped settings and capabilities, with live grants bound to the originating turn. use std::sync::Arc; diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 4e5ae1ae94..f7a3cc26cc 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -6907,6 +6907,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { models_manager: Arc::clone(&models_manager), git_root_discovery: Arc::default(), tool_approvals: Mutex::new(ApprovalStore::default()), + granted_permissions_by_environment_id: Arc::default(), runtime_handle: tokio::runtime::Handle::current(), skills_service, agents_md_manager: Arc::new(AgentsMdManager::new(SessionInstructions::default())), @@ -7032,6 +7033,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { let turn_context = Session::make_turn_context( thread_id, SessionId::from(thread_id), + Arc::clone(&session.services.granted_permissions_by_environment_id), Some(Arc::clone(&auth_manager)), &session_telemetry, session_configuration.provider.clone(), @@ -7466,8 +7468,9 @@ async fn resumed_copied_fork_ignores_source_history_base() { #[tokio::test] async fn notify_request_permissions_response_ignores_unmatched_call_id() { - let (session, _turn_context) = make_session_and_context().await; - *session.active_turn.lock().await = Some(ActiveTurn::default()); + let (session, turn_context) = make_session_and_context().await; + let active_turn = ActiveTurn::default(); + *session.active_turn.lock().await = Some(active_turn); session .notify_request_permissions_response( @@ -7486,23 +7489,16 @@ async fn notify_request_permissions_response_ignores_unmatched_call_id() { .await; assert_eq!( - session - .granted_turn_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID) - .await, + turn_context.granted_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID), None ); } #[tokio::test] async fn record_granted_request_permissions_for_turn_uses_originating_turn() { - let (session, _turn_context) = make_session_and_context().await; - let originating_active_turn = ActiveTurn::default(); - let originating_turn_state = Arc::clone(&originating_active_turn.turn_state); - *session.active_turn.lock().await = Some(originating_active_turn); + let (session, turn_context) = make_session_and_context().await; - let current_active_turn = ActiveTurn::default(); - let current_turn_state = Arc::clone(¤t_active_turn.turn_state); - *session.active_turn.lock().await = Some(current_active_turn); + let (_other_session, current_turn_context) = make_session_and_context().await; let requested_permissions = RequestPermissionProfile { network: Some(codex_protocol::models::NetworkPermissions { @@ -7510,46 +7506,29 @@ async fn record_granted_request_permissions_for_turn_uses_originating_turn() { }), ..RequestPermissionProfile::default() }; - session - .record_granted_request_permissions_for_turn( - &codex_protocol::request_permissions::RequestPermissionsResponse { - permissions: requested_permissions.clone(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }, - codex_exec_server::LOCAL_ENVIRONMENT_ID, - Some(&originating_turn_state), - ) - .await; + session.record_granted_request_permissions_for_turn( + &codex_protocol::request_permissions::RequestPermissionsResponse { + permissions: requested_permissions.clone(), + scope: PermissionGrantScope::Turn, + strict_auto_review: false, + }, + codex_exec_server::LOCAL_ENVIRONMENT_ID, + &turn_context, + ); assert_eq!( - originating_turn_state - .lock() - .await - .granted_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID), + turn_context.granted_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID), Some(requested_permissions.into()) ); assert_eq!( - current_turn_state - .lock() - .await - .granted_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID), - None - ); - assert_eq!( - session - .granted_turn_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID) - .await, + current_turn_context.granted_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID), None ); } #[tokio::test] async fn request_permission_grants_are_environment_keyed() { - let (session, _turn_context) = make_session_and_context().await; - let originating_active_turn = ActiveTurn::default(); - let originating_turn_state = Arc::clone(&originating_active_turn.turn_state); - *session.active_turn.lock().await = Some(originating_active_turn); + let (session, turn_context) = make_session_and_context().await; let requested_permissions = RequestPermissionProfile { network: Some(codex_protocol::models::NetworkPermissions { @@ -7557,52 +7536,57 @@ async fn request_permission_grants_are_environment_keyed() { }), ..RequestPermissionProfile::default() }; - session - .record_granted_request_permissions_for_turn( - &codex_protocol::request_permissions::RequestPermissionsResponse { - permissions: requested_permissions.clone(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }, - "remote", - Some(&originating_turn_state), - ) - .await; - - { - let turn_state = originating_turn_state.lock().await; - assert_eq!( - turn_state.granted_permissions("remote"), - Some(requested_permissions.clone().into()) - ); - assert_eq!(turn_state.granted_permissions("local"), None); - } - - session - .record_granted_request_permissions_for_turn( - &codex_protocol::request_permissions::RequestPermissionsResponse { - permissions: requested_permissions.clone(), - scope: PermissionGrantScope::Session, - strict_auto_review: false, - }, - "remote", - /*originating_turn_state*/ None, - ) - .await; + session.record_granted_request_permissions_for_turn( + &codex_protocol::request_permissions::RequestPermissionsResponse { + permissions: requested_permissions.clone(), + scope: PermissionGrantScope::Turn, + strict_auto_review: false, + }, + "remote", + &turn_context, + ); assert_eq!( - session.granted_session_permissions("remote").await, + turn_context.granted_permissions("remote"), + Some(requested_permissions.clone().into()) + ); + assert_eq!(turn_context.granted_permissions("local"), None); + + session.record_granted_request_permissions_for_turn( + &codex_protocol::request_permissions::RequestPermissionsResponse { + permissions: requested_permissions.clone(), + scope: PermissionGrantScope::Session, + strict_auto_review: false, + }, + "remote", + &turn_context, + ); + + assert_eq!( + session + .services + .granted_permissions_by_environment_id + .lock() + .expect("session permission grants lock poisoned") + .get("remote") + .cloned(), Some(requested_permissions.into()) ); - assert_eq!(session.granted_session_permissions("local").await, None); + assert_eq!( + session + .services + .granted_permissions_by_environment_id + .lock() + .expect("session permission grants lock poisoned") + .get("local") + .cloned(), + None + ); } #[tokio::test] async fn enable_strict_auto_review_for_turn_uses_originating_turn() { - let (session, _turn_context) = make_session_and_context().await; - let originating_active_turn = ActiveTurn::default(); - let originating_turn_state = Arc::clone(&originating_active_turn.turn_state); - *session.active_turn.lock().await = Some(originating_active_turn); + let (session, turn_context) = make_session_and_context().await; let requested_permissions = RequestPermissionProfile { network: Some(codex_protocol::models::NetworkPermissions { @@ -7610,24 +7594,17 @@ async fn enable_strict_auto_review_for_turn_uses_originating_turn() { }), ..RequestPermissionProfile::default() }; - session - .record_granted_request_permissions_for_turn( - &codex_protocol::request_permissions::RequestPermissionsResponse { - permissions: requested_permissions.clone(), - scope: PermissionGrantScope::Turn, - strict_auto_review: true, - }, - codex_exec_server::LOCAL_ENVIRONMENT_ID, - Some(&originating_turn_state), - ) - .await; - - assert!( - originating_turn_state - .lock() - .await - .strict_auto_review_enabled() + session.record_granted_request_permissions_for_turn( + &codex_protocol::request_permissions::RequestPermissionsResponse { + permissions: requested_permissions, + scope: PermissionGrantScope::Turn, + strict_auto_review: true, + }, + codex_exec_server::LOCAL_ENVIRONMENT_ID, + &turn_context, ); + + assert!(turn_context.strict_auto_review_enabled()); } #[test] @@ -8034,8 +8011,12 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec assert_eq!(response, Some(expected_response)); assert_eq!( session - .granted_session_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID) - .await, + .services + .granted_permissions_by_environment_id + .lock() + .expect("session permission grants lock poisoned") + .get(codex_exec_server::LOCAL_ENVIRONMENT_ID) + .cloned(), Some(expected_permissions.into()) ); } @@ -9192,6 +9173,7 @@ where models_manager: Arc::clone(&models_manager), git_root_discovery: Arc::default(), tool_approvals: Mutex::new(ApprovalStore::default()), + granted_permissions_by_environment_id: Arc::default(), runtime_handle: tokio::runtime::Handle::current(), skills_service, agents_md_manager: Arc::new(AgentsMdManager::new(SessionInstructions::default())), @@ -9317,6 +9299,7 @@ where let turn_context = Arc::new(Session::make_turn_context( thread_id, SessionId::from(thread_id), + Arc::clone(&session.services.granted_permissions_by_environment_id), Some(Arc::clone(&auth_manager)), &session_telemetry, session_configuration.provider.clone(), diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index 00c6d882b4..59a65e10a6 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -204,7 +204,6 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { update_turn_settings_for_test(&mut turn_context_raw, |settings| { Arc::make_mut(&mut settings.model_info).node_repl_auto_review_required = true; }); - *session.active_turn.lock().await = Some(ActiveTurn::default()); Arc::make_mut(&mut turn_context_raw.config) .permissions .approval_policy @@ -244,6 +243,16 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { evidence.record("js", "cell", "image", vec![image]); let session = Arc::new(session); let turn_context = Arc::new(turn_context_raw); + session + .start_task( + Arc::clone(&turn_context), + Vec::new(), + super::NeverEndingTask { + kind: crate::state::TaskKind::Regular, + listen_to_cancellation_token: true, + }, + ) + .await; let step_context = StepContext::for_test(Arc::clone(&turn_context)); let requested_permissions = RequestPermissionProfile { @@ -297,9 +306,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { .await; assert_eq!(second_response, response); assert_eq!( - session - .granted_turn_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID) - .await, + turn_context.granted_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID), Some(requested_permissions.into()) ); @@ -414,7 +421,8 @@ async fn request_permissions_guardian_review_stops_when_cancelled( .await; let (mut session, mut turn_context, rx_event) = make_session_and_context_with_rx().await; - *session.active_turn.lock().await = Some(ActiveTurn::default()); + let active_turn = ActiveTurn::default(); + *session.active_turn.lock().await = Some(active_turn); let turn_context_raw = Arc::get_mut(&mut turn_context).expect("single turn context ref"); Arc::make_mut(&mut turn_context_raw.config) .permissions @@ -523,9 +531,7 @@ async fn request_permissions_guardian_review_stops_when_cancelled( .expect("parent shutdown must finish reviewer cleanup"); } assert_eq!( - session - .granted_turn_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID) - .await, + turn_context.granted_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID), None ); } @@ -658,25 +664,20 @@ async fn strict_auto_review_turn_grant_forces_guardian_for_exec_command_policy_s .await; let (mut session, mut turn_context_raw) = make_session_and_context().await; - let active_turn = crate::state::ActiveTurn::default(); - let originating_turn_state = Arc::clone(&active_turn.turn_state); - *session.active_turn.lock().await = Some(active_turn); - session - .record_granted_request_permissions_for_turn( - &RequestPermissionsResponse { - permissions: RequestPermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - ..Default::default() - }, - scope: PermissionGrantScope::Turn, - strict_auto_review: true, + session.record_granted_request_permissions_for_turn( + &RequestPermissionsResponse { + permissions: RequestPermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + ..Default::default() }, - codex_exec_server::LOCAL_ENVIRONMENT_ID, - Some(&originating_turn_state), - ) - .await; + scope: PermissionGrantScope::Turn, + strict_auto_review: true, + }, + codex_exec_server::LOCAL_ENVIRONMENT_ID, + &turn_context_raw, + ); Arc::make_mut(&mut turn_context_raw.config) .permissions @@ -1162,32 +1163,22 @@ async fn compaction_initial_context_preserves_separate_guardian_developer_messag #[tokio::test] #[cfg(unix)] -#[expect( - clippy::await_holding_invalid_type, - reason = "test mutates active turn state directly to seed granted permissions" -)] async fn exec_command_allows_sticky_turn_permissions_without_inline_request_permissions_feature() { let (mut session, turn_context_raw) = make_session_and_context().await; session .features .enable(Feature::RequestPermissionsTool) .expect("test setup should allow enabling request permissions tool"); - *session.active_turn.lock().await = Some(ActiveTurn::default()); - { - let mut active_turn = session.active_turn.lock().await; - let active_turn = active_turn.as_mut().expect("active turn"); - let mut turn_state = active_turn.turn_state.lock().await; - turn_state.record_granted_permissions( - codex_exec_server::LOCAL_ENVIRONMENT_ID, - PermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - ..Default::default() - }, - ); - } - + turn_context_raw.record_granted_permissions( + codex_exec_server::LOCAL_ENVIRONMENT_ID, + PermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + ..Default::default() + }, + /*strict_auto_review*/ false, + ); let session = Arc::new(session); let turn_context = Arc::new(turn_context_raw); diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 652da73c3c..5fd52bb823 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -34,6 +34,7 @@ use codex_protocol::protocol::ThreadHistoryMode; use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::turn_input::CyberAccessProgram; use codex_sandboxing::policy_transforms::effective_permission_profile; +use codex_sandboxing::policy_transforms::merge_permission_profiles; use codex_skills_extension::HostSkillsSnapshot; use codex_skills_extension::SkillLoadOutcome; use codex_utils_path_uri::PathUri; @@ -42,6 +43,7 @@ use futures::FutureExt; use futures::future::BoxFuture; use futures::future::Shared; use std::path::PathBuf; +use std::sync::Mutex as StdMutex; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; use tracing::instrument; @@ -356,6 +358,10 @@ pub struct TurnContext { pub(crate) extension_data: Arc, pub(crate) turn_timing_state: Arc, pub(crate) terminal_error: Arc>>, + /// Shares the session-owned store; session approvals remain live across turns. + session_grants: Arc>>, + /// Retained by background cells so a later active turn cannot replace their grants. + turn_grants: Arc>, pub(crate) server_model_warning_emitted: AtomicBool, pub(crate) model_verification_emitted: AtomicBool, /// Effective cyber treatment for this turn, including any child-agent inheritance. @@ -379,6 +385,130 @@ enum TurnContextBuildMode { } impl TurnContext { + /// Builds a review turn with shared session grants and fresh turn-local state. + pub(super) fn for_review( + &self, + sub_id: String, + config: Arc, + step_settings: Arc, + available_models: Vec, + unified_exec_shell_mode: UnifiedExecShellMode, + turn_metadata_state: Arc, + ) -> Self { + let session_telemetry = step_settings.telemetry(&self.session_telemetry); + let extension_data = Arc::new(codex_extension_api::ExtensionData::new(sub_id.clone())); + extension_data.insert(self.skills_snapshot().as_ref().clone()); + + Self { + sub_id, + trace_id: current_span_trace_id(), + realtime_active: self.realtime_active, + code_mode_available: self.code_mode_available, + configured_token_budget: config.token_budget.clone(), + use_model_token_budget_defaults: config.features.enabled(Feature::TokenBudget) + && !has_explicit_settings(&config), + config, + auth_manager: self.auth_manager.clone(), + initial_settings: Arc::clone(&step_settings), + disabled_plugin_ids: self.disabled_plugin_ids.clone(), + active_host_plugin_identities: None, + next_step_settings: ArcSwap::from(step_settings), + session_telemetry, + provider: self.provider.clone(), + session_source: self.session_source.clone(), + history_mode: self.history_mode, + parent_thread_id: self.parent_thread_id, + originator: self.originator.clone(), + initial_environments: self.initial_environments.clone(), + available_models, + unified_exec_shell_mode, + current_date: self.current_date.clone(), + timezone: self.timezone.clone(), + app_server_client_name: self.app_server_client_name.clone(), + developer_instructions: None, + multi_agent_version: MultiAgentVersion::Disabled, + network: self.network.clone(), + windows_sandbox_level: self.windows_sandbox_level, + #[allow(deprecated)] + cwd: self.cwd.clone(), + final_output_json_schema: None, + dynamic_tools: self.dynamic_tools.clone(), + turn_metadata_state, + extension_data, + turn_timing_state: Arc::new(TurnTimingState::default()), + terminal_error: Arc::new(Mutex::new(None)), + session_grants: Arc::clone(&self.session_grants), + turn_grants: Arc::default(), + server_model_warning_emitted: AtomicBool::new(false), + model_verification_emitted: AtomicBool::new(false), + cyber_access_program: None, + } + } + + /// Returns current session and turn grants for this environment. + #[expect( + clippy::expect_used, + reason = "poisoned grant state must not authorize further operations" + )] + pub(crate) fn granted_permissions( + &self, + environment_id: &str, + ) -> Option { + let session_grants = self + .session_grants + .lock() + .expect("session permission grants lock poisoned") + .get(environment_id) + .cloned(); + let turn_grants = self + .turn_grants + .lock() + .expect("turn permission grants lock poisoned") + .granted_permissions_by_environment_id + .get(environment_id) + .cloned(); + merge_permission_profiles(session_grants.as_ref(), turn_grants.as_ref()) + } + + #[expect( + clippy::expect_used, + reason = "poisoned grant state must not authorize further operations" + )] + pub(crate) fn record_granted_permissions( + &self, + environment_id: &str, + permissions: AdditionalPermissionProfile, + strict_auto_review: bool, + ) { + let mut grants = self + .turn_grants + .lock() + .expect("turn permission grants lock poisoned"); + let granted_permissions = merge_permission_profiles( + grants + .granted_permissions_by_environment_id + .get(environment_id), + Some(&permissions), + ); + if let Some(granted_permissions) = granted_permissions { + grants + .granted_permissions_by_environment_id + .insert(environment_id.to_string(), granted_permissions); + } + grants.strict_auto_review_enabled |= strict_auto_review; + } + + #[expect( + clippy::expect_used, + reason = "poisoned grant state must not authorize further operations" + )] + pub(crate) fn strict_auto_review_enabled(&self) -> bool { + self.turn_grants + .lock() + .expect("turn permission grants lock poisoned") + .strict_auto_review_enabled + } + /// Captures current model metadata without preparing a step. pub(crate) fn capture_current_model_info(&self) -> Arc { Arc::clone(&self.next_step_settings.load().model_info) @@ -717,6 +847,8 @@ impl TurnContext { extension_data: Arc::clone(&self.extension_data), turn_timing_state: Arc::clone(&self.turn_timing_state), terminal_error: Arc::clone(&self.terminal_error), + session_grants: Arc::clone(&self.session_grants), + turn_grants: Arc::clone(&self.turn_grants), server_model_warning_emitted: AtomicBool::new( self.server_model_warning_emitted.load(Ordering::Relaxed), ), @@ -827,6 +959,14 @@ impl TurnContext { } } +/// Grants and their review requirement share a lock so readers cannot see a new grant before +/// its strict-review requirement. +#[derive(Debug, Default)] +struct TurnGrants { + granted_permissions_by_environment_id: HashMap, + strict_auto_review_enabled: bool, +} + fn local_time_context() -> (String, String) { match iana_time_zone::get_timezone() { Ok(timezone) => (Local::now().format("%Y-%m-%d").to_string(), timezone), @@ -916,6 +1056,7 @@ impl Session { pub(crate) fn make_turn_context( thread_id: ThreadId, session_id: SessionId, + session_grants: Arc>>, auth_manager: Option>, session_telemetry: &SessionTelemetry, provider: SharedModelProvider, @@ -1039,6 +1180,8 @@ impl Session { extension_data, turn_timing_state: Arc::new(TurnTimingState::default()), terminal_error: Arc::new(Mutex::new(None)), + session_grants, + turn_grants: Arc::default(), server_model_warning_emitted: AtomicBool::new(false), model_verification_emitted: AtomicBool::new(false), cyber_access_program: None, @@ -1240,6 +1383,7 @@ impl Session { let mut turn_context: TurnContext = Self::make_turn_context( self.thread_id(), self.session_id(), + Arc::clone(&self.services.granted_permissions_by_environment_id), Some(Arc::clone(&self.services.auth_manager)), &self.services.session_telemetry, session_configuration.provider.clone(), diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index ded1f878e8..4688178c96 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -1,5 +1,7 @@ +use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; +use std::sync::Mutex as StdMutex; use crate::agent::api::AgentControl; use crate::agent::control::LocalAgentRuntime; @@ -36,8 +38,10 @@ use codex_models_manager::manager::SharedModelsManager; use codex_otel::SessionTelemetry; use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_protocol::mcp::ClientMcpExtensions; +use codex_protocol::models::AdditionalPermissionProfile; use codex_rollout::state_db::StateDbHandle; use codex_rollout_trace::ThreadTraceContext; +use codex_sandboxing::policy_transforms::merge_permission_profiles; use codex_skills_extension::HostSkillsService; use codex_thread_store::LiveThread; use codex_thread_store::ThreadStore; @@ -69,6 +73,9 @@ pub(crate) struct SessionServices { pub(crate) git_root_discovery: Arc, pub(crate) session_telemetry: SessionTelemetry, pub(crate) tool_approvals: Mutex, + /// Shared with captured steps so later calls observe newly approved permissions. + pub(crate) granted_permissions_by_environment_id: + Arc>>, pub(crate) runtime_handle: Handle, pub(crate) skills_service: Arc, pub(crate) agents_md_manager: Arc, @@ -102,3 +109,25 @@ pub(crate) struct SessionServices { pub(crate) tool_search_handler_cache: ToolSearchHandlerCache, pub(crate) turn_environments: Arc, } + +impl SessionServices { + #[expect( + clippy::expect_used, + reason = "poisoned grant state must not authorize further operations" + )] + pub(crate) fn record_granted_permissions( + &self, + environment_id: &str, + permissions: AdditionalPermissionProfile, + ) { + let mut grants = self + .granted_permissions_by_environment_id + .lock() + .expect("session permission grants lock poisoned"); + let granted_permissions = + merge_permission_profiles(grants.get(environment_id), Some(&permissions)); + if let Some(granted_permissions) = granted_permissions { + grants.insert(environment_id.to_string(), granted_permissions); + } + } +} diff --git a/codex-rs/core/src/state/session.rs b/codex-rs/core/src/state/session.rs index 5f8d2d8814..6a93e160a4 100644 --- a/codex-rs/core/src/state/session.rs +++ b/codex-rs/core/src/state/session.rs @@ -1,12 +1,9 @@ //! Session-wide mutable state. -use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::models::BaseInstructionsProvenance; #[cfg(test)] use codex_protocol::models::ResponseItem; use codex_protocol::openai_models::ReasoningEffort; -use codex_sandboxing::policy_transforms::merge_permission_profiles; -use std::collections::HashMap; use std::collections::HashSet; use std::collections::VecDeque; @@ -100,7 +97,6 @@ pub(crate) struct SessionState { pub(crate) current_time_reminder: CurrentTimeReminderState, pub(crate) active_connector_selection: HashSet, pub(crate) pending_session_start_sources: VecDeque, - granted_permissions_by_environment_id: HashMap, next_turn_is_first: bool, } @@ -141,7 +137,6 @@ impl SessionState { current_time_reminder: CurrentTimeReminderState::default(), active_connector_selection: HashSet::new(), pending_session_start_sources: VecDeque::new(), - granted_permissions_by_environment_id: HashMap::new(), next_turn_is_first: true, } } @@ -412,31 +407,6 @@ impl SessionState { ) -> Option { self.pending_session_start_sources.pop_front() } - - pub(crate) fn record_granted_permissions( - &mut self, - environment_id: &str, - permissions: AdditionalPermissionProfile, - ) { - let granted_permissions = merge_permission_profiles( - self.granted_permissions_by_environment_id - .get(environment_id), - Some(&permissions), - ); - if let Some(granted_permissions) = granted_permissions { - self.granted_permissions_by_environment_id - .insert(environment_id.to_string(), granted_permissions); - } - } - - pub(crate) fn granted_permissions( - &self, - environment_id: &str, - ) -> Option { - self.granted_permissions_by_environment_id - .get(environment_id) - .cloned() - } } // Sometimes new snapshots don't include credits or plan information. diff --git a/codex-rs/core/src/state/turn.rs b/codex-rs/core/src/state/turn.rs index 334372019f..292e0a21fb 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -13,7 +13,6 @@ use codex_protocol::request_permissions::RequestPermissionProfile; use codex_protocol::request_permissions::RequestPermissionsResponse; use codex_protocol::request_user_input::RequestUserInputResponse; use codex_rmcp_client::ElicitationResponse; -use codex_sandboxing::policy_transforms::merge_permission_profiles; use rmcp::model::RequestId; use tokio::sync::oneshot; @@ -24,7 +23,6 @@ use crate::session::step_context::StepContext; use crate::session::turn_context::TurnContext; use crate::session::turn_context::TurnEnvironment; use crate::tasks::AnySessionTask; -use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::TokenUsage; @@ -94,8 +92,6 @@ pub(crate) struct TurnState { pending_dynamic_tools: HashMap>, pub(crate) pending_input: TurnInputQueue, mailbox_delivery_phase: MailboxDeliveryPhase, - granted_permissions_by_environment_id: HashMap, - strict_auto_review_enabled: bool, pub(crate) tool_calls: u64, pub(crate) has_memory_citation: bool, pub(crate) token_usage_at_turn_start: TokenUsage, @@ -116,6 +112,8 @@ pub(crate) struct PendingRequestPermissions { pub(crate) tx_response: oneshot::Sender, pub(crate) requested_permissions: RequestPermissionProfile, pub(crate) environment: TurnEnvironment, + /// Background cells can request permissions after a different turn becomes active. + pub(crate) turn_context: Arc, } impl TurnState { @@ -218,37 +216,4 @@ impl TurnState { pub(crate) fn set_mailbox_delivery_phase(&mut self, phase: MailboxDeliveryPhase) { self.mailbox_delivery_phase = phase; } - - pub(crate) fn record_granted_permissions( - &mut self, - environment_id: &str, - permissions: AdditionalPermissionProfile, - ) { - let granted_permissions = merge_permission_profiles( - self.granted_permissions_by_environment_id - .get(environment_id), - Some(&permissions), - ); - if let Some(granted_permissions) = granted_permissions { - self.granted_permissions_by_environment_id - .insert(environment_id.to_string(), granted_permissions); - } - } - - pub(crate) fn granted_permissions( - &self, - environment_id: &str, - ) -> Option { - self.granted_permissions_by_environment_id - .get(environment_id) - .cloned() - } - - pub(crate) fn enable_strict_auto_review(&mut self) { - self.strict_auto_review_enabled = true; - } - - pub(crate) fn strict_auto_review_enabled(&self) -> bool { - self.strict_auto_review_enabled - } } diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 1c9d2c22f0..390e62dd2b 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -53,7 +53,6 @@ use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::PatchApplyUpdatedEvent; use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; -use codex_sandboxing::policy_transforms::merge_permission_profiles; use codex_sandboxing::policy_transforms::normalize_additional_permissions; use codex_sandboxing::policy_transforms::normalize_additional_permissions_with_context; use codex_tools::ToolName; @@ -520,18 +519,10 @@ async fn execute_verified_patch( }; let environment_id = turn_environment.selection.environment_id.as_str(); let file_paths = file_paths_for_action(&action); - let granted_permissions = merge_permission_profiles( - tool_ctx - .session - .granted_session_permissions(environment_id) - .await - .as_ref(), - tool_ctx - .session - .granted_turn_permissions(environment_id) - .await - .as_ref(), - ); + let granted_permissions = tool_ctx + .step_context + .turn + .granted_permissions(environment_id); let base_file_system_sandbox_policy = turn_environment .permission_profile() .file_system_sandbox_policy(); @@ -551,13 +542,12 @@ async fn execute_verified_patch( FunctionCallError::RespondToModel(format!("failed to check patch permissions: {error}")) })?; let effective_additional_permissions = apply_granted_turn_permissions( - tool_ctx.session.as_ref(), + &tool_ctx.step_context, &turn_environment, &cwd, crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions, - ) - .await; + ); let apply = apply_patch::prepare_apply_patch( &tool_ctx.step_context, &turn_environment, diff --git a/codex-rs/core/src/tools/handlers/extension_tools.rs b/codex-rs/core/src/tools/handlers/extension_tools.rs index 927e9b3d83..58434b2ee1 100644 --- a/codex-rs/core/src/tools/handlers/extension_tools.rs +++ b/codex-rs/core/src/tools/handlers/extension_tools.rs @@ -187,13 +187,12 @@ async fn to_extension_call(invocation: &ToolInvocation) -> ExtensionToolCall<'_> let mut environments = Vec::new(); for environment in invocation.step_context.environments.turn_environments() { let additional_permissions = apply_granted_turn_permissions( - invocation.session.as_ref(), + &invocation.step_context, environment, environment.cwd(), SandboxPermissions::UseDefault, /*additional_permissions*/ None, ) - .await .additional_permissions; let file_system_sandbox_context = environment.sandbox_context(additional_permissions); environments.push(ToolEnvironment { diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 2d0b6177e1..bd58aeda59 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -50,7 +50,7 @@ use serde_json::Value; use crate::environment_selection::TurnEnvironmentSnapshot; use crate::function_tool::FunctionCallError; use crate::sandboxing::SandboxPermissions; -use crate::session::session::Session; +use crate::session::step_context::StepContext; use crate::session::turn_context::TurnEnvironment; pub(crate) use crate::tools::code_mode::CodeModeExecuteHandler; pub(crate) use crate::tools::code_mode::CodeModeWaitHandler; @@ -267,8 +267,8 @@ pub(super) fn implicit_granted_permissions( } } -pub(super) async fn apply_granted_turn_permissions( - session: &Session, +pub(super) fn apply_granted_turn_permissions( + step_context: &StepContext, environment: &TurnEnvironment, cwd: &PathUri, sandbox_permissions: SandboxPermissions, @@ -283,12 +283,7 @@ pub(super) async fn apply_granted_turn_permissions( } let environment_id = &environment.selection.environment_id; - let granted_session_permissions = session.granted_session_permissions(environment_id).await; - let granted_turn_permissions = session.granted_turn_permissions(environment_id).await; - let granted_permissions = merge_permission_profiles( - granted_session_permissions.as_ref(), - granted_turn_permissions.as_ref(), - ); + let granted_permissions = step_context.turn.granted_permissions(environment_id); let effective_permissions = merge_permission_profiles( additional_permissions.as_ref(), granted_permissions.as_ref(), diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index ba5dd79efb..7358f1ef72 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -325,13 +325,12 @@ impl ExecCommandHandler { turn_environment.sandbox_context(/*additional_permissions*/ None); let permission_context = file_system_sandbox_policy_context_for_cwd(&sandbox_context, &cwd); let effective_additional_permissions = apply_granted_turn_permissions( - context.session.as_ref(), + &context.step_context, turn_environment, &cwd, sandbox_permissions, additional_permissions, - ) - .await; + ); let additional_permissions_allowed = exec_permission_approvals_enabled || (session.features().enabled(Feature::RequestPermissionsTool) && effective_additional_permissions.permissions_preapproved); diff --git a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs index 98b8795839..cf75c3a611 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -489,16 +489,6 @@ async fn exec_command_reuses_foreign_windows_grant() { )), ..Default::default() }; - *session.active_turn.lock().await = Some(crate::state::ActiveTurn::default()); - let turn_state = { - let active_turn = session.active_turn.lock().await; - Arc::clone(&active_turn.as_ref().expect("active turn").turn_state) - }; - turn_state.lock().await.record_granted_permissions( - codex_exec_server::REMOTE_ENVIRONMENT_ID, - granted_permissions.clone(), - ); - { let turn = Arc::get_mut(&mut turn).expect("turn should be uniquely owned"); let TurnEnvironmentState::Ready(environment) = turn @@ -519,10 +509,16 @@ async fn exec_command_reuses_foreign_windows_grant() { ); } + let step_context = StepContext::for_test(Arc::clone(&turn)); + step_context.turn.record_granted_permissions( + codex_exec_server::REMOTE_ENVIRONMENT_ID, + granted_permissions.clone(), + /*strict_auto_review*/ false, + ); let response = ExecCommandHandler::default() .handle(ToolInvocation { session: Arc::clone(&session), - step_context: StepContext::for_test(Arc::clone(&turn)), + step_context, turn, cancellation_token: tokio_util::sync::CancellationToken::new(), tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 2d94f27d86..3d7e2b62c5 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -136,7 +136,7 @@ impl ToolOrchestrator { let otel = turn_ctx.session_telemetry.clone(); let otel_tn = flat_tool_name(&tool_ctx.tool_name).into_owned(); let otel_ci = &tool_ctx.call_id; - let strict_auto_review = tool_ctx.session.strict_auto_review_enabled().await; + let strict_auto_review = turn_ctx.strict_auto_review_enabled(); // 1) Approval let mut already_approved = false; diff --git a/codex-rs/core/src/unified_exec/mod_tests.rs b/codex-rs/core/src/unified_exec/mod_tests.rs index 6c09065f36..1b8af5bc0d 100644 --- a/codex-rs/core/src/unified_exec/mod_tests.rs +++ b/codex-rs/core/src/unified_exec/mod_tests.rs @@ -949,20 +949,6 @@ async fn stdin_approval_preserves_the_reviewed_terminal() -> anyhow::Result<()> entry.cwd = cwd.clone(); Arc::clone(&entry.process) }; - // A queued write must acquire the terminal lock before reading active strict - // mode: code-mode calls can enable it while another interaction is draining. - { - let interaction = original.interaction_lock().lock_owned().await; - let _active_turn = session.active_turn.lock().await; - let mut queued = Box::pin(write_stdin( - &session, &turn, process_id, "queued\n", /*yield_time_ms*/ 250, - )); - let mut task_context = std::task::Context::from_waker(futures::task::noop_waker_ref()); - assert!(queued.as_mut().poll(&mut task_context).is_pending()); - drop(interaction); - assert!(queued.as_mut().poll(&mut task_context).is_pending()); - assert!(original.interaction_lock().try_lock_owned().is_err()); - } // Empty polling must complete without an approval response. // The test deadline must allow the minimum empty-poll wait. tokio::time::timeout( @@ -1060,3 +1046,73 @@ async fn stdin_approval_preserves_the_reviewed_terminal() -> anyhow::Result<()> assert!(session.terminate_background_terminal(process_id).await); Ok(()) } + +/// A queued write observes strict review enabled on its captured step while waiting +/// for another terminal interaction to finish. +#[tokio::test] +async fn stdin_approval_observes_strict_review_enabled_while_queued() -> anyhow::Result<()> { + use crate::session::step_context::StepContext; + use crate::session::tests::make_session_and_context_with_auth_and_config_and_rx; + use crate::tools::sandboxing::ToolError; + use codex_features::Feature; + + skip_if_sandbox!(Ok(())); + let (session, turn, _events) = make_session_and_context_with_auth_and_config_and_rx( + codex_login::CodexAuth::from_api_key("Test API Key"), + Vec::new(), + |config| { + config.features.enable(Feature::WriteStdinApproval).unwrap(); + }, + ) + .await; + let manager = &session.services.unified_exec_manager; + let opened = exec_command( + &session, &turn, "cat", /*yield_time_ms*/ 250, /*workdir*/ None, + ) + .await?; + let process_id = opened.process_id.expect("running terminal"); + let process = Arc::clone( + &manager + .process_store + .lock() + .await + .processes + .get(&process_id) + .unwrap() + .process, + ); + let context = UnifiedExecContext::new( + Arc::clone(&session), + StepContext::for_test(turn), + tokio_util::sync::CancellationToken::new(), + "write".to_string(), + ); + let interaction = process.interaction_lock().lock_owned().await; + let mut queued = Box::pin(manager.write_stdin( + &context, + WriteStdinRequest { + process_id, + // NUL input must be rejected when strict review becomes required. + input: "\0", + yield_time_ms: 250, + max_output_tokens: None, + truncation_policy: TruncationPolicy::Tokens(10_000), + interaction_event: None, + }, + )); + let mut task_context = std::task::Context::from_waker(futures::task::noop_waker_ref()); + assert!(queued.as_mut().poll(&mut task_context).is_pending()); + context.step_context.turn.record_granted_permissions( + codex_exec_server::LOCAL_ENVIRONMENT_ID, + Default::default(), + /*strict_auto_review*/ true, + ); + drop(interaction); + assert!(matches!( + queued.await, + Err(UnifiedExecError::StdinApproval(ToolError::Rejected(reason))) + if reason.contains("NUL byte") + )); + assert!(session.terminate_background_terminal(process_id).await); + Ok(()) +} diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 565194441f..8c0cfcfd04 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -899,7 +899,7 @@ impl UnifiedExecProcessManager { }; let _interaction_guard = locked_process.interaction_lock().lock_owned().await; // A queued write must observe strict review enabled while it was waiting. - let strict_auto_review = context.session.strict_auto_review_enabled().await; + let strict_auto_review = context.step_context.turn.strict_auto_review_enabled(); let approval = { let store = self.process_store.lock().await; let entry = store