mirror of
https://github.com/openai/codex.git
synced 2026-11-01 19:33:35 +00:00
Preserve session config when switching thread permission profiles (#45981)
## Why Permission profile changes reloaded configuration without the thread's session overrides, so profiles defined at thread start could be unavailable to `thread/settings/update`. ## What changed Reload permission configuration with the thread's merged, enabled `SessionFlags` layers, the effective working directory, and the requested profile. Preserve sandbox executable paths through `ConfigManager`. ## Testing Add regression coverage for switching away from and back to session-defined and disk-defined profiles, including top-level profile selection. Add a config manager test for merging session layers, excluding disabled layers, and retaining filesystem access to the exec wrapper directory. GitOrigin-RevId: b0271e28a90cd55d3faf5fdf5f044d768395b0b6
This commit is contained in:
@@ -281,6 +281,45 @@ impl ConfigManager {
|
||||
.await
|
||||
}
|
||||
|
||||
/// Reload sources using the task's session flags before materializing config.
|
||||
pub(crate) async fn load_permission_config_for_thread(
|
||||
&self,
|
||||
thread_config: &Config,
|
||||
cwd: AbsolutePathBuf,
|
||||
permission_profile: String,
|
||||
) -> std::io::Result<Config> {
|
||||
let mut session_flags = TomlValue::Table(Default::default());
|
||||
for layer in thread_config.config_layer_stack.layers_low_to_high() {
|
||||
if matches!(layer.name, codex_config::ConfigLayerSource::SessionFlags) {
|
||||
codex_config::merge_toml_values(&mut session_flags, &layer.config);
|
||||
}
|
||||
}
|
||||
let overrides = session_flags
|
||||
.as_table()
|
||||
.ok_or_else(|| {
|
||||
std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidData,
|
||||
"session flags must be a table",
|
||||
)
|
||||
})?
|
||||
.iter()
|
||||
.map(|(key, value)| (key.clone(), value.clone()))
|
||||
.collect::<Vec<_>>();
|
||||
self.load_with_cli_overrides(
|
||||
&overrides,
|
||||
/*request_overrides*/ None,
|
||||
ConfigOverrides {
|
||||
cwd: Some(cwd.to_path_buf()),
|
||||
default_permissions: Some(permission_profile),
|
||||
codex_linux_sandbox_exe: self.arg0_paths.codex_linux_sandbox_exe.clone(),
|
||||
main_execve_wrapper_exe: self.arg0_paths.main_execve_wrapper_exe.clone(),
|
||||
..Default::default()
|
||||
},
|
||||
Some(cwd.to_path_buf()),
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
#[instrument(level = "trace", skip_all)]
|
||||
pub(crate) async fn load_with_cli_overrides(
|
||||
&self,
|
||||
|
||||
@@ -2674,3 +2674,81 @@ async fn allowed_login_methods_follow_current_forced_workspaces() -> Result<()>
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn permission_config_reload_merges_session_layers() -> Result<()> {
|
||||
use codex_config::ConfigLayerEntry;
|
||||
use codex_config::ConfigLayerSource;
|
||||
use codex_config::ConfigLayerStack;
|
||||
let tmp = tempdir()?;
|
||||
let wrapper_dir = tmp.path().join("tmp/arg0/session");
|
||||
std::fs::create_dir_all(&wrapper_dir)?;
|
||||
let wrapper = wrapper_dir.join("codex-execve-wrapper");
|
||||
std::fs::write(&wrapper, "")?;
|
||||
let service = ConfigManager::new(
|
||||
tmp.path().to_path_buf(),
|
||||
Vec::new(),
|
||||
LoaderOverrides::without_managed_config_for_tests(),
|
||||
/*strict_config*/ false,
|
||||
CloudConfigBundleLoader::default(),
|
||||
codex_arg0::Arg0DispatchPaths {
|
||||
main_execve_wrapper_exe: Some(wrapper),
|
||||
..Default::default()
|
||||
},
|
||||
std::sync::Arc::new(codex_config::NoopThreadConfigLoader),
|
||||
);
|
||||
|
||||
let mut config = service
|
||||
.load_with_overrides(
|
||||
/*request_overrides*/ None,
|
||||
codex_core::config::ConfigOverrides {
|
||||
cwd: Some(tmp.path().to_path_buf()),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
let mut layers = config
|
||||
.config_layer_stack
|
||||
.all_layers_low_to_high()
|
||||
.cloned()
|
||||
.collect::<Vec<_>>();
|
||||
for contents in [
|
||||
"[permissions.first.filesystem]\n",
|
||||
"[permissions.second]\nextends = ':workspace'\n",
|
||||
] {
|
||||
layers.push(ConfigLayerEntry::new(
|
||||
ConfigLayerSource::SessionFlags,
|
||||
toml::from_str(contents)?,
|
||||
));
|
||||
}
|
||||
layers.push(ConfigLayerEntry::new_disabled(
|
||||
ConfigLayerSource::SessionFlags,
|
||||
toml::from_str("[permissions.disabled]\nextends = ':read-only'\n")?,
|
||||
"disabled for test",
|
||||
));
|
||||
config.config_layer_stack =
|
||||
ConfigLayerStack::new(layers, Default::default(), Default::default())?;
|
||||
let loaded = service
|
||||
.load_permission_config_for_thread(&config, config.cwd.clone(), "first".to_string())
|
||||
.await?;
|
||||
assert_eq!(
|
||||
loaded
|
||||
.custom_permission_profiles
|
||||
.iter()
|
||||
.map(|profile| profile.id.as_str())
|
||||
.collect::<Vec<_>>(),
|
||||
vec!["first", "second"]
|
||||
);
|
||||
let policy = loaded.permissions.file_system_sandbox_policy();
|
||||
assert_eq!(
|
||||
(
|
||||
policy.can_read_local_path_with_cwd(&wrapper_dir, loaded.cwd.as_path()),
|
||||
policy.can_read_local_path_with_cwd(
|
||||
&tmp.path().join("tmp/arg0/other"),
|
||||
loaded.cwd.as_path()
|
||||
),
|
||||
),
|
||||
(true, false),
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -520,7 +520,6 @@ impl MessageProcessor {
|
||||
Arc::clone(&thread_manager),
|
||||
outgoing.clone(),
|
||||
analytics_events_client.clone(),
|
||||
arg0_paths.clone(),
|
||||
Arc::clone(&config),
|
||||
config_manager.clone(),
|
||||
pending_thread_unloads,
|
||||
|
||||
@@ -84,7 +84,6 @@ pub(crate) struct TurnRequestProcessor {
|
||||
thread_manager: Arc<ThreadManager>,
|
||||
outgoing: Arc<OutgoingMessageSender>,
|
||||
analytics_events_client: AnalyticsEventsClient,
|
||||
arg0_paths: Arg0DispatchPaths,
|
||||
config: Arc<Config>,
|
||||
config_manager: ConfigManager,
|
||||
pending_thread_unloads: Arc<Mutex<HashSet<ThreadId>>>,
|
||||
@@ -147,7 +146,6 @@ impl TurnRequestProcessor {
|
||||
thread_manager: Arc<ThreadManager>,
|
||||
outgoing: Arc<OutgoingMessageSender>,
|
||||
analytics_events_client: AnalyticsEventsClient,
|
||||
arg0_paths: Arg0DispatchPaths,
|
||||
config: Arc<Config>,
|
||||
config_manager: ConfigManager,
|
||||
pending_thread_unloads: Arc<Mutex<HashSet<ThreadId>>>,
|
||||
@@ -163,7 +161,6 @@ impl TurnRequestProcessor {
|
||||
thread_manager,
|
||||
outgoing,
|
||||
analytics_events_client,
|
||||
arg0_paths,
|
||||
config,
|
||||
config_manager,
|
||||
pending_thread_unloads,
|
||||
@@ -843,24 +840,16 @@ impl TurnRequestProcessor {
|
||||
"{method} permission selection missing thread snapshot"
|
||||
)));
|
||||
};
|
||||
let overrides = ConfigOverrides {
|
||||
cwd: environments
|
||||
.as_ref()
|
||||
.map(|environments| environments.legacy_fallback_cwd.to_path_buf()),
|
||||
default_permissions: Some(permissions),
|
||||
codex_linux_sandbox_exe: self.arg0_paths.codex_linux_sandbox_exe.clone(),
|
||||
main_execve_wrapper_exe: self.arg0_paths.main_execve_wrapper_exe.clone(),
|
||||
..Default::default()
|
||||
};
|
||||
let thread_config = thread.config().await;
|
||||
let cwd = environments.as_ref().map_or_else(
|
||||
|| snapshot.cwd().clone(),
|
||||
|environments| environments.legacy_fallback_cwd.clone(),
|
||||
);
|
||||
let config = self
|
||||
.config_manager
|
||||
.load_for_cwd(
|
||||
/*request_overrides*/ None,
|
||||
overrides,
|
||||
Some(snapshot.cwd().to_path_buf()),
|
||||
)
|
||||
.load_permission_config_for_thread(&thread_config, cwd, permissions)
|
||||
.await
|
||||
.map_err(|err| config_load_error(&err))?;
|
||||
.map_err(|error| config_load_error(&error))?;
|
||||
// Startup config is allowed to fall back when requirements
|
||||
// disallow a configured profile. An explicit settings update
|
||||
// is different: reject it before accepting the request.
|
||||
|
||||
@@ -702,3 +702,70 @@ fn create_config_toml(codex_home: &std::path::Path, server_uri: &str) -> std::io
|
||||
.with_provider_config("supports_websockets = false")
|
||||
.write(codex_home)
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_settings_update_preserves_session_profiles() -> Result<()> {
|
||||
for (profile_on_disk, top_level_selection) in [(false, false), (true, false), (true, true)] {
|
||||
let home = TempDir::new()?;
|
||||
let profile = json!({"extends": ":read-only"});
|
||||
std::fs::write(
|
||||
home.path().join("config.toml"),
|
||||
if profile_on_disk {
|
||||
"default_permissions = ':read-only'\n[permissions.audit]\nextends = ':read-only'\n"
|
||||
} else {
|
||||
""
|
||||
},
|
||||
)?;
|
||||
write_models_cache(home.path()).await?;
|
||||
let mut server = TestAppServer::builder()
|
||||
.with_codex_home(home.path())
|
||||
.build_initialized_with_timeout(DEFAULT_TIMEOUT)
|
||||
.await?;
|
||||
let mut config = std::collections::HashMap::from([
|
||||
("default_permissions".to_string(), json!("audit")),
|
||||
("features.guardian_approval".to_string(), json!(true)),
|
||||
]);
|
||||
if !profile_on_disk {
|
||||
config.insert("permissions.audit".to_string(), profile);
|
||||
}
|
||||
if top_level_selection {
|
||||
config.remove("default_permissions");
|
||||
}
|
||||
let id = server
|
||||
.send_thread_start_request_with_auto_env(ThreadStartParams {
|
||||
model: Some("mock-model".to_string()),
|
||||
config: Some(config),
|
||||
permissions: top_level_selection.then(|| "audit".to_string()),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let started: ThreadStartResponse =
|
||||
timeout(DEFAULT_TIMEOUT, server.read_response(id)).await??;
|
||||
let thread_id = started.thread.id;
|
||||
for profile_id in [":workspace", "audit"] {
|
||||
let id = server
|
||||
.send_thread_settings_update_request(ThreadSettingsUpdateParams {
|
||||
thread_id: thread_id.clone(),
|
||||
permissions: Some(profile_id.to_string()),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let _: ThreadSettingsUpdateResponse =
|
||||
timeout(DEFAULT_TIMEOUT, server.read_response(id)).await??;
|
||||
let applied: ThreadSettingsUpdatedNotification = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
server.read_notification("thread/settings/updated"),
|
||||
)
|
||||
.await??;
|
||||
assert_eq!(
|
||||
applied
|
||||
.thread_settings
|
||||
.active_permission_profile
|
||||
.unwrap()
|
||||
.id,
|
||||
profile_id
|
||||
);
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user