mirror of
https://github.com/openai/codex.git
synced 2026-10-22 17:58:46 +00:00
Keep step settings and approval environments consistent (#46556)
## Why Background processes can outlive their launching turn. Their approval reviews need the originating environment's filesystem restrictions, even when the active turn uses a different environment. Remote restrictions must also be resolved using executor paths rather than the local filesystem. ## What changed - Capture settings and environment selections together in `StepInputs`. Preserve the captured selection during active model updates while allowing environment startup to finish. - Carry environment IDs into Guardian network and `execve` requests, and use the owning process's environment with current review settings. Refresh environment readiness for network requests that fall back to the active turn. - Resolve remote denied paths and globs using the executor's policy context. Decline automatic approval when the request's environment is unavailable or an explicitly denied temporary directory cannot be resolved. ## Testing Add regression coverage for active model updates retaining their environment while the next turn adopts a new selection, background approvals using their owning environment, newly ready network environments, and remote denied paths and globs, including Windows paths and missing temporary-directory metadata. GitOrigin-RevId: b620d4b030c597a4f430fe0500864fa5469db3d4
This commit is contained in:
@@ -333,6 +333,7 @@ impl ApprovalAction {
|
||||
#[cfg(unix)]
|
||||
Self::Execve {
|
||||
id,
|
||||
environment_id,
|
||||
source,
|
||||
program,
|
||||
argv,
|
||||
@@ -341,6 +342,7 @@ impl ApprovalAction {
|
||||
..
|
||||
} => crate::guardian::GuardianApprovalRequest::Execve {
|
||||
id,
|
||||
environment_id,
|
||||
source,
|
||||
program: program.to_string_lossy().into_owned(),
|
||||
argv,
|
||||
@@ -388,6 +390,7 @@ impl ApprovalAction {
|
||||
Self::NetworkAccess {
|
||||
id,
|
||||
turn_id,
|
||||
environment_id,
|
||||
target,
|
||||
host,
|
||||
protocol,
|
||||
@@ -397,6 +400,7 @@ impl ApprovalAction {
|
||||
} => crate::guardian::GuardianApprovalRequest::NetworkAccess {
|
||||
id,
|
||||
turn_id,
|
||||
environment_id,
|
||||
target,
|
||||
host,
|
||||
protocol,
|
||||
|
||||
@@ -430,7 +430,7 @@ mod tests {
|
||||
let model = turn.model_info().slug.clone();
|
||||
let truncation_policy = turn.model_info().truncation_policy.into();
|
||||
let expected_sandbox_cwds = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.turn_environments()
|
||||
.map(|environment| environment.cwd().clone())
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
@@ -2216,7 +2216,7 @@ async fn spawn_agent_reapplies_runtime_sandbox_after_role_config() {
|
||||
.await
|
||||
.expect("non-empty role config should apply");
|
||||
let TurnEnvironmentState::Ready(environment) = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.environments
|
||||
.first_mut()
|
||||
.expect("parent environment should exist")
|
||||
@@ -4203,7 +4203,7 @@ async fn tool_handlers_cascade_close_and_resume_and_keep_explicitly_closed_subtr
|
||||
let parent_session = parent.thread.session.clone();
|
||||
let parent_turn = parent_session.new_default_turn().await;
|
||||
let mut owner_config = parent_turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.primary()
|
||||
.expect("parent should have an environment")
|
||||
.config()
|
||||
@@ -4478,7 +4478,7 @@ async fn build_agent_spawn_config_uses_captured_step_settings_and_turn_context_v
|
||||
&file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
);
|
||||
turn.environments.environments.clear();
|
||||
turn.initial_environments.environments.clear();
|
||||
Arc::make_mut(&mut turn.config)
|
||||
.permissions
|
||||
.set_permission_profile(permission_profile)
|
||||
@@ -4566,7 +4566,7 @@ async fn build_agent_resume_config_clears_base_instructions() {
|
||||
PermissionProfile::read_only()
|
||||
};
|
||||
let TurnEnvironmentState::Ready(environment) = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.environments
|
||||
.first_mut()
|
||||
.expect("parent environment should exist")
|
||||
|
||||
@@ -180,7 +180,7 @@ async fn exec_command_rejects_login_when_selected_environment_disallows_it() {
|
||||
let (session, mut turn) = make_session_and_context().await;
|
||||
assert!(turn.config.permissions.allow_login_shell);
|
||||
let TurnEnvironmentState::Ready(environment) = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.environments
|
||||
.first_mut()
|
||||
.expect("primary environment")
|
||||
@@ -325,7 +325,7 @@ async fn exec_command_reuses_foreign_windows_grant() {
|
||||
{
|
||||
let turn = Arc::get_mut(&mut turn).expect("turn should be uniquely owned");
|
||||
let TurnEnvironmentState::Ready(environment) = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.environments
|
||||
.first_mut()
|
||||
.expect("primary environment")
|
||||
|
||||
@@ -297,7 +297,7 @@ mod tests {
|
||||
|
||||
fn replace_primary_environment_cwd(turn: &mut crate::TurnContext, cwd: AbsolutePathBuf) {
|
||||
let mut current = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.turn_environments()
|
||||
.next()
|
||||
.cloned()
|
||||
@@ -306,12 +306,13 @@ mod tests {
|
||||
let mut selection = current.selection;
|
||||
selection.cwd = PathUri::from_abs_path(&cwd);
|
||||
selection.workspace_roots.clear();
|
||||
turn.environments.environments[0] = TurnEnvironmentState::Ready(TurnEnvironment::new(
|
||||
selection,
|
||||
current.config_origin,
|
||||
current.environment,
|
||||
current.shell,
|
||||
));
|
||||
turn.initial_environments.environments[0] =
|
||||
TurnEnvironmentState::Ready(TurnEnvironment::new(
|
||||
selection,
|
||||
current.config_origin,
|
||||
current.environment,
|
||||
current.shell,
|
||||
));
|
||||
}
|
||||
|
||||
fn tiny_png() -> Vec<u8> {
|
||||
@@ -372,7 +373,8 @@ mod tests {
|
||||
.permissions
|
||||
.set_permission_profile(PermissionProfile::Disabled)
|
||||
.expect("set thread permission profile");
|
||||
let TurnEnvironmentState::Ready(environment) = &mut turn.environments.environments[0]
|
||||
let TurnEnvironmentState::Ready(environment) =
|
||||
&mut turn.initial_environments.environments[0]
|
||||
else {
|
||||
panic!("primary environment should be ready");
|
||||
};
|
||||
@@ -450,7 +452,8 @@ mod tests {
|
||||
replace_primary_environment_cwd(&mut turn, image_cwd.clone());
|
||||
let image_path = image_cwd.join("image.png");
|
||||
std::fs::write(image_path.as_path(), tiny_png()).expect("write test image");
|
||||
let TurnEnvironmentState::Ready(environment) = &mut turn.environments.environments[0]
|
||||
let TurnEnvironmentState::Ready(environment) =
|
||||
&mut turn.initial_environments.environments[0]
|
||||
else {
|
||||
panic!("primary environment should be ready");
|
||||
};
|
||||
@@ -487,7 +490,8 @@ mod tests {
|
||||
let image_path = image_cwd.join("not-an-image.txt");
|
||||
std::fs::write(image_path.as_path(), b"arbitrary file contents")
|
||||
.expect("write invalid image");
|
||||
let TurnEnvironmentState::Ready(environment) = &mut turn.environments.environments[0]
|
||||
let TurnEnvironmentState::Ready(environment) =
|
||||
&mut turn.initial_environments.environments[0]
|
||||
else {
|
||||
panic!("primary environment should be ready");
|
||||
};
|
||||
|
||||
@@ -648,11 +648,17 @@ impl NetworkApprovalService {
|
||||
{
|
||||
return NetworkDecision::deny(REASON_NOT_ALLOWED);
|
||||
}
|
||||
let active_turn = session.active_turn_context_and_strict_auto_review().await;
|
||||
let active_turn = session
|
||||
.active_turn_context_and_strict_auto_review()
|
||||
.await
|
||||
.map(|(turn, inputs, strict)| {
|
||||
let environments = inputs.environments.refresh_readiness();
|
||||
(turn, inputs, environments, strict)
|
||||
});
|
||||
let Some(environment_id) = active_environment_id.or_else(|| {
|
||||
active_turn
|
||||
.as_ref()
|
||||
.and_then(|(turn_context, _, _)| turn_context.environments.primary())
|
||||
.and_then(|(_, _, environments, _)| environments.primary())
|
||||
.map(|environment| environment.selection.environment_id.clone())
|
||||
}) else {
|
||||
return NetworkDecision::deny(REASON_NOT_ALLOWED);
|
||||
@@ -679,7 +685,9 @@ impl NetworkApprovalService {
|
||||
format!("Network access to \"{target}\" was blocked by policy.");
|
||||
let prompt_reason = format!("{} is not in the allowed_domains", request.host);
|
||||
|
||||
let Some((turn_context, step_settings, strict_auto_review)) = active_turn else {
|
||||
let Some((turn_context, step_inputs, active_environments, strict_auto_review)) =
|
||||
active_turn
|
||||
else {
|
||||
if let Some(owner_call) = owner_call.as_ref() {
|
||||
self.record_call_outcome(&owner_call.registration_id, policy_denial_message);
|
||||
}
|
||||
@@ -709,12 +717,15 @@ impl NetworkApprovalService {
|
||||
pending_owner.disconnect = request.disconnect.clone();
|
||||
pending_owner.cancellation = request.cancellation.clone();
|
||||
|
||||
// Review under current settings, but an owned process keeps the environments it started in.
|
||||
let review_environments = owner_call
|
||||
.as_ref()
|
||||
.map_or(&active_environments, |call| &call.environments);
|
||||
let permission_profile = owner_call
|
||||
.as_ref()
|
||||
.map(|call| &call.permission_profile)
|
||||
.or_else(|| {
|
||||
turn_context
|
||||
.environments
|
||||
review_environments
|
||||
.turn_environments()
|
||||
.find(|environment| environment.selection.environment_id == environment_id)
|
||||
.map(TurnEnvironment::permission_profile)
|
||||
@@ -728,12 +739,8 @@ impl NetworkApprovalService {
|
||||
}
|
||||
let review_context = GuardianReviewContext::from_resolved_settings(
|
||||
Arc::clone(&turn_context),
|
||||
&step_settings,
|
||||
// Review this new request under current settings, in its originating execution's
|
||||
// environments. Completing the original turn does not transfer process ownership.
|
||||
owner_call
|
||||
.as_ref()
|
||||
.map_or(&turn_context.environments, |call| &call.environments),
|
||||
&step_inputs.settings,
|
||||
review_environments,
|
||||
);
|
||||
if !allows_network_approval_flow(review_context.approval_policy) {
|
||||
if let Some(owner_call) = owner_call.as_ref() {
|
||||
@@ -762,8 +769,7 @@ impl NetworkApprovalService {
|
||||
{
|
||||
cwd
|
||||
} else {
|
||||
turn_context
|
||||
.environments
|
||||
review_environments
|
||||
.turn_environments()
|
||||
.find(|environment| environment.selection.environment_id == environment_id)
|
||||
.and_then(|environment| environment.cwd().to_abs_path().ok())
|
||||
|
||||
@@ -282,7 +282,7 @@ impl CoreShellActionProvider {
|
||||
};
|
||||
match stopwatch
|
||||
.pause_for(async {
|
||||
let (turn_context, step_settings, strict_auto_review) = self
|
||||
let (turn_context, step_inputs, strict_auto_review) = self
|
||||
.session
|
||||
.active_turn_context_and_strict_auto_review()
|
||||
.await
|
||||
@@ -295,8 +295,8 @@ impl CoreShellActionProvider {
|
||||
let approval_ctx = ApprovalContext {
|
||||
review_context: GuardianReviewContext::from_resolved_settings(
|
||||
Arc::clone(&turn_context),
|
||||
&step_settings,
|
||||
&turn_context.environments,
|
||||
&step_inputs.settings,
|
||||
self.review_context.environments(),
|
||||
),
|
||||
// The running process can outlive its launching tool or code-mode cell.
|
||||
cancellation_token: None,
|
||||
|
||||
@@ -230,7 +230,7 @@ fn plan_with_model(
|
||||
let mut registry = build_core_tool_registry(
|
||||
turn,
|
||||
model_info,
|
||||
&turn.environments,
|
||||
&turn.initial_environments,
|
||||
&mcp,
|
||||
inputs.tool_suggest_candidates.as_ref(),
|
||||
inputs.wait_for_environment_tool_config.as_ref(),
|
||||
@@ -402,12 +402,12 @@ impl<'call> ToolExecutor<ExtensionToolCall<'call>> for DeferredExtensionTool {
|
||||
|
||||
fn duplicate_primary_environment(turn: &mut TurnContext) {
|
||||
let mut second_environment = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.primary()
|
||||
.expect("primary environment")
|
||||
.clone();
|
||||
second_environment.selection.environment_id = "secondary".to_string();
|
||||
turn.environments
|
||||
turn.initial_environments
|
||||
.environments
|
||||
.push(TurnEnvironmentState::Ready(second_environment));
|
||||
}
|
||||
@@ -740,7 +740,7 @@ async fn internal_guardian_sessions_require_managed_secondary_environments() {
|
||||
session.allowed_tools = Some(Arc::new(codex_guardian_reviewer::reviewer_allowed_tools()));
|
||||
set_feature(&mut turn, Feature::ViewImage, /*enabled*/ true);
|
||||
let TurnEnvironmentState::Ready(primary) = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.environments
|
||||
.first_mut()
|
||||
.expect("primary environment")
|
||||
@@ -755,7 +755,7 @@ async fn internal_guardian_sessions_require_managed_secondary_environments() {
|
||||
let secondary_workspace_root =
|
||||
codex_utils_path_uri::PathUri::from_abs_path(&turn.config.cwd.join("secondary"));
|
||||
let TurnEnvironmentState::Ready(secondary) = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.environments
|
||||
.get_mut(1)
|
||||
.expect("secondary environment")
|
||||
@@ -1008,7 +1008,7 @@ async fn exec_command_guidance_follows_executor_platform_and_fallbacks() {
|
||||
ConfigShellToolType::UnifiedExec;
|
||||
});
|
||||
let TurnEnvironmentState::Ready(environment) = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.environments
|
||||
.first_mut()
|
||||
.expect("primary environment")
|
||||
@@ -1045,7 +1045,7 @@ async fn login_shell_parameter_follows_selected_environment() {
|
||||
config.permissions.allow_login_shell = !allow_login_shell;
|
||||
});
|
||||
let TurnEnvironmentState::Ready(environment) = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.environments
|
||||
.first_mut()
|
||||
.expect("primary environment")
|
||||
@@ -1081,7 +1081,12 @@ async fn login_shell_parameter_is_available_when_any_environment_allows_it() {
|
||||
config.permissions.allow_login_shell = false;
|
||||
});
|
||||
duplicate_primary_environment(turn);
|
||||
for (index, environment) in turn.environments.environments.iter_mut().enumerate() {
|
||||
for (index, environment) in turn
|
||||
.initial_environments
|
||||
.environments
|
||||
.iter_mut()
|
||||
.enumerate()
|
||||
{
|
||||
let TurnEnvironmentState::Ready(environment) = environment else {
|
||||
panic!("environment should be ready");
|
||||
};
|
||||
@@ -1102,7 +1107,7 @@ async fn disabling_shell_tools_disables_command_tools_for_all_environments() {
|
||||
});
|
||||
|
||||
let TurnEnvironmentState::Ready(environment) = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.environments
|
||||
.first_mut()
|
||||
.expect("primary environment")
|
||||
@@ -1238,12 +1243,12 @@ async fn zsh_fork_unified_exec_keeps_shell_parameter_when_remote_environment_ava
|
||||
turn.unified_exec_shell_mode =
|
||||
codex_tools::UnifiedExecShellMode::ZshFork(zsh_fork_config_for_spec_plan_tests());
|
||||
let remote_cwd = turn
|
||||
.environments
|
||||
.initial_environments
|
||||
.primary()
|
||||
.expect("primary environment")
|
||||
.cwd()
|
||||
.clone();
|
||||
turn.environments
|
||||
turn.initial_environments
|
||||
.environments
|
||||
.push(TurnEnvironmentState::Ready(
|
||||
crate::session::turn_context::TurnEnvironment::new(
|
||||
@@ -1295,7 +1300,7 @@ async fn zsh_fork_unified_exec_keeps_shell_parameter_when_remote_environment_ava
|
||||
#[tokio::test]
|
||||
async fn environment_count_controls_environment_backed_tools() {
|
||||
let no_environment = probe(|turn| {
|
||||
turn.environments.environments.clear();
|
||||
turn.initial_environments.environments.clear();
|
||||
set_feature(turn, Feature::ShellTool, /*enabled*/ true);
|
||||
set_feature(turn, Feature::RequestPermissionsTool, /*enabled*/ true);
|
||||
update_turn_settings_for_test(turn, |settings| {
|
||||
@@ -1358,8 +1363,8 @@ async fn environment_tools_follow_the_step_context() {
|
||||
Some(ApplyPatchToolType::Freeform);
|
||||
});
|
||||
|
||||
let environments = turn.environments.clone();
|
||||
turn.environments.environments.clear();
|
||||
let environments = turn.initial_environments.clone();
|
||||
turn.initial_environments.environments.clear();
|
||||
let turn = Arc::new(turn);
|
||||
let mcp = Arc::new(codex_mcp::McpBinding::empty(mcp_config_for_test(
|
||||
&turn.config,
|
||||
|
||||
Reference in New Issue
Block a user