mirror of
https://github.com/openai/codex.git
synced 2026-10-21 07:42:01 +00:00
Preserve restored permission profiles in TUI sessions (#41192)
## Why Restored app-server threads can carry permission profiles that cannot be represented without loss as legacy sandbox-mode overrides. Sending those profiles as per-turn overrides can discard their original permission details. ## What changed - Distinguish restored permission profiles from legacy per-turn sandbox overrides. - Preserve restored profiles while routing turns and changing to compatible working directories. - Reject `/cd` when a restored profile cannot be represented safely by a legacy sandbox mode. ## Testing - Cover retaining a restored read-only profile across `/cd` without a turn override. - Verify that lossy sandbox-mode projections are detected. GitOrigin-RevId: 6143dca4b83ab4f59bab05f6a59b4fe841c56192
This commit is contained in:
@@ -642,6 +642,13 @@ struct RuntimePermissionProfileOverride {
|
||||
permission_profile: PermissionProfile,
|
||||
active_permission_profile: Option<ActivePermissionProfile>,
|
||||
network: Option<crate::legacy_core::config::NetworkProxySpec>,
|
||||
turn_override: RuntimePermissionProfileTurnOverride,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
enum RuntimePermissionProfileTurnOverride {
|
||||
Preserve,
|
||||
LegacySandbox,
|
||||
}
|
||||
|
||||
impl RuntimePermissionProfileOverride {
|
||||
@@ -650,8 +657,30 @@ impl RuntimePermissionProfileOverride {
|
||||
permission_profile: config.permissions.permission_profile().clone(),
|
||||
active_permission_profile: config.permissions.active_permission_profile(),
|
||||
network: config.permissions.network.clone(),
|
||||
turn_override: RuntimePermissionProfileTurnOverride::LegacySandbox,
|
||||
}
|
||||
}
|
||||
|
||||
fn from_restored_config(config: &Config) -> Self {
|
||||
Self {
|
||||
turn_override: RuntimePermissionProfileTurnOverride::Preserve,
|
||||
..Self::from_config(config)
|
||||
}
|
||||
}
|
||||
|
||||
fn matches_config(&self, config: &Config) -> bool {
|
||||
self.permission_profile == *config.permissions.permission_profile()
|
||||
&& self.active_permission_profile == config.permissions.active_permission_profile()
|
||||
&& self.network == config.permissions.network
|
||||
}
|
||||
|
||||
fn turn_permission_profile(&self) -> Option<&PermissionProfile> {
|
||||
matches!(
|
||||
self.turn_override,
|
||||
RuntimePermissionProfileTurnOverride::LegacySandbox
|
||||
)
|
||||
.then_some(&self.permission_profile)
|
||||
}
|
||||
}
|
||||
|
||||
fn active_turn_not_steerable_turn_error(error: &TypedRequestError) -> Option<AppServerTurnError> {
|
||||
|
||||
@@ -482,10 +482,10 @@ impl App {
|
||||
self.runtime_approval_policy_override =
|
||||
Some(self.config.permissions.approval_policy.value().into());
|
||||
}
|
||||
let destination_permissions =
|
||||
RuntimePermissionProfileOverride::from_config(&self.config);
|
||||
if destination_permissions != baseline_permissions {
|
||||
self.runtime_permission_profile_override = Some(destination_permissions);
|
||||
if !baseline_permissions.matches_config(&self.config) {
|
||||
self.runtime_permission_profile_override = Some(
|
||||
RuntimePermissionProfileOverride::from_restored_config(&self.config),
|
||||
);
|
||||
}
|
||||
if !self
|
||||
.backfill_loaded_subagent_threads(app_server)
|
||||
@@ -587,7 +587,7 @@ impl App {
|
||||
};
|
||||
if let Some(profile) = self.runtime_permission_profile_override.as_ref()
|
||||
&& profile.active_permission_profile.is_some()
|
||||
&& (RuntimePermissionProfileOverride::from_config(&config) != *profile
|
||||
&& (!profile.matches_config(&config)
|
||||
|| config.permissions.profile_workspace_roots()
|
||||
!= self.config.permissions.profile_workspace_roots())
|
||||
{
|
||||
|
||||
@@ -19,6 +19,7 @@ use codex_app_server_protocol::ThreadUnsubscribeStatus;
|
||||
use codex_config::types::KeybindingSpec;
|
||||
use codex_config::types::KeybindingsSpec;
|
||||
use codex_config::types::TuiKeymap;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::protocol::SubAgentSource;
|
||||
|
||||
static OVERVIEW_TIMESTAMP: std::sync::LazyLock<i64> =
|
||||
@@ -511,3 +512,76 @@ async fn root_switch_preserves_idle_root_with_running_subagent() -> Result<()> {
|
||||
app_server.shutdown().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn restored_server_permission_profile_survives_cd_without_turn_override() -> Result<()> {
|
||||
let mut app = make_test_app().await;
|
||||
let destination = app.config.codex_home.join("destination");
|
||||
std::fs::create_dir(&destination)?;
|
||||
crate::legacy_core::config::set_project_trust_level(
|
||||
app.config.codex_home.as_path(),
|
||||
&destination,
|
||||
codex_protocol::config_types::TrustLevel::Trusted,
|
||||
)
|
||||
.map_err(|error| color_eyre::eyre::eyre!(error.to_string()))?;
|
||||
let mut app_server =
|
||||
crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?;
|
||||
let previous = app_server.start_thread(&app.config).await?;
|
||||
app.enqueue_primary_thread_session(previous.session, previous.turns)
|
||||
.await?;
|
||||
let target = app_server.start_thread(&app.config).await?;
|
||||
let target_thread_id = target.session.thread_id;
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
|
||||
app.select_agents_overview_thread(&mut tui, &mut app_server, target_thread_id)
|
||||
.await?;
|
||||
app.config
|
||||
.permissions
|
||||
.set_permission_profile(PermissionProfile::read_only())?;
|
||||
app.chat_widget.set_permission_profile_with_active_profile(
|
||||
PermissionProfile::read_only(),
|
||||
/*active_permission_profile*/ None,
|
||||
)?;
|
||||
app.runtime_permission_profile_override = Some(
|
||||
RuntimePermissionProfileOverride::from_restored_config(app.chat_widget.config_ref()),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
app.chat_widget
|
||||
.config_ref()
|
||||
.permissions
|
||||
.effective_permission_profile(),
|
||||
PermissionProfile::read_only()
|
||||
);
|
||||
assert_eq!(
|
||||
app.runtime_permission_profile_override
|
||||
.as_ref()
|
||||
.map(|profile| profile.turn_override),
|
||||
Some(RuntimePermissionProfileTurnOverride::Preserve)
|
||||
);
|
||||
assert_eq!(
|
||||
App::turn_permissions_override_from_config(
|
||||
app.chat_widget.config_ref(),
|
||||
/*active_permission_profile*/ None,
|
||||
app.runtime_permission_profile_override
|
||||
.as_ref()
|
||||
.and_then(RuntimePermissionProfileOverride::turn_permission_profile),
|
||||
),
|
||||
TurnPermissionsOverride::Preserve
|
||||
);
|
||||
|
||||
app.change_working_directory(&mut tui, &mut app_server, destination.clone())
|
||||
.await;
|
||||
|
||||
assert_eq!(app.chat_widget.config_ref().cwd, destination);
|
||||
assert_eq!(
|
||||
app.chat_widget
|
||||
.config_ref()
|
||||
.permissions
|
||||
.effective_permission_profile(),
|
||||
PermissionProfile::read_only()
|
||||
);
|
||||
|
||||
app_server.shutdown().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1507,7 +1507,7 @@ impl App {
|
||||
config.permissions.active_permission_profile().as_ref(),
|
||||
self.runtime_permission_profile_override
|
||||
.as_ref()
|
||||
.map(|profile| &profile.permission_profile),
|
||||
.and_then(RuntimePermissionProfileOverride::turn_permission_profile),
|
||||
);
|
||||
if turn_permissions_overrides(permissions_override, config.cwd.as_path())
|
||||
.is_ok()
|
||||
|
||||
@@ -71,7 +71,7 @@ impl App {
|
||||
active_permission_profile.as_ref(),
|
||||
self.runtime_permission_profile_override
|
||||
.as_ref()
|
||||
.map(|profile| &profile.permission_profile),
|
||||
.and_then(RuntimePermissionProfileOverride::turn_permission_profile),
|
||||
);
|
||||
if let Err(err) = turn_permissions_overrides(permissions_override, cwd.as_path()) {
|
||||
self.chat_widget
|
||||
|
||||
@@ -2638,6 +2638,11 @@ async fn changing_directory_preserves_project_trust_permissions_history_and_hook
|
||||
("../trusted", "profile", "permission profile override"),
|
||||
("../trusted", "reviewer", "reviewer"),
|
||||
("../p", "named", "different settings"),
|
||||
(
|
||||
"../trusted",
|
||||
"restored",
|
||||
"Permission profile cannot be preserved",
|
||||
),
|
||||
("../p", "keymap", "open_transcript"),
|
||||
("../unknown", "local", "This directory is not trusted"),
|
||||
("../trusted", "main", "background terminals"),
|
||||
@@ -2659,8 +2664,17 @@ async fn changing_directory_preserves_project_trust_permissions_history_and_hook
|
||||
let mut profile = RuntimePermissionProfileOverride::from_config(&app.config);
|
||||
profile.active_permission_profile =
|
||||
(kind == "named").then(|| ActivePermissionProfile::new("dev"));
|
||||
if kind == "restored" {
|
||||
profile.permission_profile = PermissionProfile::workspace_write_with(
|
||||
&[failed.clone().abs()],
|
||||
codex_protocol::permissions::NetworkSandboxPolicy::Restricted,
|
||||
/*exclude_tmpdir_env_var*/ false,
|
||||
/*exclude_slash_tmp*/ false,
|
||||
);
|
||||
profile.turn_override = RuntimePermissionProfileTurnOverride::Preserve;
|
||||
}
|
||||
app.runtime_permission_profile_override =
|
||||
matches!(kind, "profile" | "reviewer" | "named").then_some(profile);
|
||||
matches!(kind, "profile" | "reviewer" | "named" | "restored").then_some(profile);
|
||||
app.app_server_target = crate::AppServerTarget::Embedded;
|
||||
if kind == "workspace" {
|
||||
let endpoint = crate::resolve_remote_addr("ws://127.0.0.1:8765")?;
|
||||
@@ -2691,6 +2705,8 @@ async fn changing_directory_preserves_project_trust_permissions_history_and_hook
|
||||
let output = history().join("");
|
||||
if kind == "mcp" {
|
||||
assert_snapshot!(output, @"■ MCP inventory is still loading.");
|
||||
} else if kind == "restored" {
|
||||
assert_snapshot!(output, @"■ Permission profile cannot be preserved by /cd.");
|
||||
}
|
||||
assert!(output.contains(expected), "{path}");
|
||||
app.clear_committed_mcp_inventory_loading();
|
||||
|
||||
@@ -739,7 +739,7 @@ impl App {
|
||||
active_permission_profile.as_ref(),
|
||||
self.runtime_permission_profile_override
|
||||
.as_ref()
|
||||
.map(|profile| &profile.permission_profile),
|
||||
.and_then(RuntimePermissionProfileOverride::turn_permission_profile),
|
||||
);
|
||||
let response = app_server
|
||||
.turn_start(
|
||||
|
||||
@@ -57,12 +57,22 @@ impl App {
|
||||
}
|
||||
if let Some(profile) = self.runtime_permission_profile_override.as_ref()
|
||||
&& profile.active_permission_profile.is_some()
|
||||
&& (RuntimePermissionProfileOverride::from_config(&config) != *profile
|
||||
&& (!profile.matches_config(&config)
|
||||
|| config.permissions.profile_workspace_roots()
|
||||
!= self.config.permissions.profile_workspace_roots())
|
||||
{
|
||||
return self.working_directory_error("Permission profile has different settings.");
|
||||
}
|
||||
if let Some(profile) = self.runtime_permission_profile_override.as_ref()
|
||||
&& profile.turn_override == RuntimePermissionProfileTurnOverride::Preserve
|
||||
&& profile.active_permission_profile.is_none()
|
||||
&& !crate::app_server_session::permission_profile_is_safely_represented_by_sandbox_mode(
|
||||
&profile.permission_profile,
|
||||
cwd.as_path(),
|
||||
)
|
||||
{
|
||||
return self.working_directory_error("Permission profile cannot be preserved by /cd.");
|
||||
}
|
||||
self.apply_runtime_policy_overrides(&mut config);
|
||||
if self.runtime_permission_profile_override.is_some() {
|
||||
let reviewer = self.config.approvals_reviewer;
|
||||
@@ -73,11 +83,10 @@ impl App {
|
||||
config.approvals_reviewer = reviewer;
|
||||
}
|
||||
let actual = config.permissions.approval_policy.value();
|
||||
let snapshot = RuntimePermissionProfileOverride::from_config(&config);
|
||||
let approval = self.runtime_approval_policy_override;
|
||||
let profile = self.runtime_permission_profile_override.as_ref();
|
||||
if approval.is_some_and(|p| actual != p.to_core())
|
||||
|| profile.is_some_and(|p| snapshot != *p)
|
||||
|| profile.is_some_and(|profile| !profile.matches_config(&config))
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -1789,6 +1789,29 @@ fn sandbox_mode_from_permission_profile(
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn permission_profile_is_safely_represented_by_sandbox_mode(
|
||||
permission_profile: &PermissionProfile,
|
||||
cwd: &std::path::Path,
|
||||
) -> bool {
|
||||
let Some(sandbox_mode) = sandbox_mode_from_permission_profile(permission_profile, cwd) else {
|
||||
return false;
|
||||
};
|
||||
let projected_profile = match sandbox_mode {
|
||||
codex_app_server_protocol::SandboxMode::ReadOnly => PermissionProfile::read_only(),
|
||||
codex_app_server_protocol::SandboxMode::WorkspaceWrite => {
|
||||
PermissionProfile::from_legacy_sandbox_policy_for_cwd(
|
||||
&codex_protocol::protocol::SandboxPolicy::new_workspace_write_policy(),
|
||||
cwd,
|
||||
)
|
||||
}
|
||||
codex_app_server_protocol::SandboxMode::DangerFullAccess => PermissionProfile::Disabled,
|
||||
};
|
||||
permission_profile.network_sandbox_policy() == projected_profile.network_sandbox_policy()
|
||||
&& permission_profile
|
||||
.file_system_sandbox_policy()
|
||||
.is_semantically_equivalent_to(&projected_profile.file_system_sandbox_policy(), cwd)
|
||||
}
|
||||
|
||||
fn permission_profile_id_from_active_profile(active: ActivePermissionProfile) -> String {
|
||||
active.id
|
||||
}
|
||||
@@ -3001,6 +3024,10 @@ mod tests {
|
||||
sandbox_mode_from_permission_profile(&permission_profile, cwd.as_path()),
|
||||
Some(codex_app_server_protocol::SandboxMode::ReadOnly)
|
||||
);
|
||||
assert!(!permission_profile_is_safely_represented_by_sandbox_mode(
|
||||
&permission_profile,
|
||||
cwd.as_path(),
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user