mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Preserve cloud requirements across TUI thread resets (#25177)
Fixes a TUI regression where thread transitions such as `/new` and `/clear` could rebuild config without the cloud requirements loader, allowing users to fall back to non-cloud-managed settings. The config refresh path now preserves cloud requirements during thread reinitialization, and config loading is moved off the deep TUI event stack to avoid stack-overflow crashes during those reloads. - Passes the cloud requirements loader through TUI config rebuild paths. - Keeps cloud requirements applied for `/new`, `/clear`, `/fork`, side conversations, and session picker transitions. - Runs config building on a Tokio task so reloads do not occur on the deep TUI caller stack. - Adds regression coverage that cloud requirements survive thread-transition config refreshes. ## Test/Repro: - Start Codex with a cloud requirement applied. - Use `/new` or `/clear`. - The refreshed/fresh-session config should still include the cloud requirements This can be tested with any config item, at this moment for oai staff the easiest item to test is the `mentions_v2` feature. This is currently enabled in cloud requirements, but is not enabled by default. As a result, prior to these changes that feature is disabled after `/new` or `/clear`. Testing the same steps with a binary from this branch should not drop the feature enablement.
This commit is contained in:
committed by
GitHub
Unverified
parent
c656cc4a83
commit
0beb5c7f32
@@ -130,6 +130,7 @@ use codex_app_server_protocol::Turn;
|
||||
use codex_app_server_protocol::TurnError as AppServerTurnError;
|
||||
use codex_app_server_protocol::TurnStatus;
|
||||
use codex_app_server_protocol::WriteStatus;
|
||||
use codex_config::CloudConfigBundleLoader;
|
||||
use codex_config::ConfigLayerStackOrdering;
|
||||
use codex_config::LoaderOverrides;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
@@ -490,6 +491,7 @@ pub(crate) struct App {
|
||||
cli_kv_overrides: Vec<(String, TomlValue)>,
|
||||
harness_overrides: ConfigOverrides,
|
||||
loader_overrides: LoaderOverrides,
|
||||
cloud_config_bundle: CloudConfigBundleLoader,
|
||||
runtime_approval_policy_override: Option<AskForApproval>,
|
||||
runtime_permission_profile_override: Option<RuntimePermissionProfileOverride>,
|
||||
|
||||
@@ -720,6 +722,7 @@ impl App {
|
||||
cli_kv_overrides: Vec<(String, TomlValue)>,
|
||||
harness_overrides: ConfigOverrides,
|
||||
loader_overrides: LoaderOverrides,
|
||||
cloud_config_bundle: CloudConfigBundleLoader,
|
||||
initial_prompt: Option<String>,
|
||||
initial_images: Vec<PathBuf>,
|
||||
session_selection: SessionSelection,
|
||||
@@ -754,6 +757,7 @@ impl App {
|
||||
&mut config,
|
||||
&cli_kv_overrides,
|
||||
&harness_overrides,
|
||||
&cloud_config_bundle,
|
||||
entered_trust_nux,
|
||||
)
|
||||
.await?;
|
||||
@@ -1003,6 +1007,7 @@ See the Codex keymap documentation for supported actions and examples."
|
||||
cli_kv_overrides,
|
||||
harness_overrides,
|
||||
loader_overrides,
|
||||
cloud_config_bundle,
|
||||
runtime_approval_policy_override: None,
|
||||
runtime_permission_profile_override: None,
|
||||
file_search,
|
||||
|
||||
@@ -34,7 +34,8 @@ impl App {
|
||||
.codex_home(self.config.codex_home.to_path_buf())
|
||||
.cli_overrides(self.cli_kv_overrides.clone())
|
||||
.harness_overrides(overrides)
|
||||
.loader_overrides(self.loader_overrides.clone());
|
||||
.loader_overrides(self.loader_overrides.clone())
|
||||
.cloud_config_bundle(self.cloud_config_bundle.clone());
|
||||
build_config_on_runtime_worker(
|
||||
builder,
|
||||
format!("Failed to rebuild config for cwd {cwd_display}"),
|
||||
@@ -55,7 +56,8 @@ impl App {
|
||||
.codex_home(self.config.codex_home.to_path_buf())
|
||||
.cli_overrides(self.cli_kv_overrides.clone())
|
||||
.harness_overrides(overrides)
|
||||
.loader_overrides(self.loader_overrides.clone());
|
||||
.loader_overrides(self.loader_overrides.clone())
|
||||
.cloud_config_bundle(self.cloud_config_bundle.clone());
|
||||
build_config_on_runtime_worker(
|
||||
builder,
|
||||
format!("Failed to rebuild config for permission profile {profile_id}"),
|
||||
@@ -1123,6 +1125,66 @@ mod tests {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// Regression coverage for `/new` and `/clear`: cloud requirements
|
||||
// must survive the config refresh that runs before thread transitions.
|
||||
#[tokio::test]
|
||||
async fn refresh_in_memory_config_from_disk_keeps_cloud_requirements_for_thread_transitions()
|
||||
-> Result<()> {
|
||||
let mut app = make_test_app().await;
|
||||
let codex_home = tempdir()?;
|
||||
let required_policy = codex_protocol::protocol::AskForApproval::Never;
|
||||
let cloud_config_bundle =
|
||||
codex_config::test_support::CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||
r#"allowed_approval_policies = ["never"]"#,
|
||||
);
|
||||
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.loader_overrides(LoaderOverrides::without_managed_config_for_tests())
|
||||
.cloud_config_bundle(cloud_config_bundle.clone())
|
||||
.build()
|
||||
.await?;
|
||||
app.config = config;
|
||||
app.cloud_config_bundle = cloud_config_bundle;
|
||||
let app_id = "unit_test_cloud_requirements_reload_marker";
|
||||
std::fs::write(
|
||||
codex_home.path().join("config.toml"),
|
||||
format!(
|
||||
r#"
|
||||
[apps.{app_id}]
|
||||
enabled = false
|
||||
"#
|
||||
),
|
||||
)?;
|
||||
|
||||
let assert_cloud_requirements = |app: &App| {
|
||||
let config = app.fresh_session_config();
|
||||
assert_eq!(
|
||||
config
|
||||
.config_layer_stack
|
||||
.requirements_toml()
|
||||
.allowed_approval_policies
|
||||
.clone(),
|
||||
Some(vec![required_policy])
|
||||
);
|
||||
assert_eq!(config.permissions.approval_policy.value(), required_policy);
|
||||
};
|
||||
|
||||
assert_cloud_requirements(&app);
|
||||
assert_eq!(app_enabled_in_effective_config(&app.config, app_id), None);
|
||||
|
||||
// This is the fallible reload that the best-effort `/new`, `/clear`,
|
||||
// `/fork`, side-conversation, and session-picker paths wrap.
|
||||
app.refresh_in_memory_config_from_disk().await?;
|
||||
|
||||
assert_eq!(
|
||||
app_enabled_in_effective_config(&app.config, app_id),
|
||||
Some(false)
|
||||
);
|
||||
assert_cloud_requirements(&app);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn refresh_in_memory_config_from_disk_best_effort_keeps_current_config_on_error()
|
||||
-> Result<()> {
|
||||
|
||||
@@ -25,6 +25,7 @@ pub(super) async fn make_test_app() -> App {
|
||||
cli_kv_overrides: Vec::new(),
|
||||
harness_overrides: ConfigOverrides::default(),
|
||||
loader_overrides: LoaderOverrides::without_managed_config_for_tests(),
|
||||
cloud_config_bundle: CloudConfigBundleLoader::default(),
|
||||
runtime_approval_policy_override: None,
|
||||
runtime_permission_profile_override: None,
|
||||
file_search,
|
||||
|
||||
@@ -3945,6 +3945,7 @@ async fn make_test_app() -> App {
|
||||
cli_kv_overrides: Vec::new(),
|
||||
harness_overrides: ConfigOverrides::default(),
|
||||
loader_overrides: LoaderOverrides::without_managed_config_for_tests(),
|
||||
cloud_config_bundle: CloudConfigBundleLoader::default(),
|
||||
runtime_approval_policy_override: None,
|
||||
runtime_permission_profile_override: None,
|
||||
file_search,
|
||||
@@ -4009,6 +4010,7 @@ async fn make_test_app_with_channels() -> (
|
||||
cli_kv_overrides: Vec::new(),
|
||||
harness_overrides: ConfigOverrides::default(),
|
||||
loader_overrides: LoaderOverrides::without_managed_config_for_tests(),
|
||||
cloud_config_bundle: CloudConfigBundleLoader::default(),
|
||||
runtime_approval_policy_override: None,
|
||||
runtime_permission_profile_override: None,
|
||||
file_search,
|
||||
|
||||
@@ -9,6 +9,7 @@ use crate::legacy_core::config::edit::ConfigEditsBuilder;
|
||||
use crate::tui;
|
||||
use codex_app_server_protocol::ExternalAgentConfigDetectParams;
|
||||
use codex_app_server_protocol::ExternalAgentConfigMigrationItem;
|
||||
use codex_config::CloudConfigBundleLoader;
|
||||
use codex_features::Feature;
|
||||
use color_eyre::eyre::Result;
|
||||
use color_eyre::eyre::WrapErr;
|
||||
@@ -248,6 +249,7 @@ pub(crate) async fn handle_external_agent_config_migration_prompt_if_needed(
|
||||
config: &mut Config,
|
||||
cli_kv_overrides: &[(String, TomlValue)],
|
||||
harness_overrides: &ConfigOverrides,
|
||||
cloud_config_bundle: &CloudConfigBundleLoader,
|
||||
entered_trust_nux: bool,
|
||||
) -> Result<ExternalAgentConfigMigrationStartupOutcome> {
|
||||
if !should_show_external_agent_config_migration_prompt(config, entered_trust_nux) {
|
||||
@@ -321,6 +323,7 @@ pub(crate) async fn handle_external_agent_config_migration_prompt_if_needed(
|
||||
.codex_home(config.codex_home.to_path_buf())
|
||||
.cli_overrides(cli_kv_overrides.to_vec())
|
||||
.harness_overrides(harness_overrides.clone())
|
||||
.cloud_config_bundle(cloud_config_bundle.clone())
|
||||
.build()
|
||||
.await
|
||||
.wrap_err("Failed to reload config after external agent migration")?;
|
||||
|
||||
@@ -1849,6 +1849,7 @@ async fn run_ratatui_app(
|
||||
cli_kv_overrides.clone(),
|
||||
overrides.clone(),
|
||||
loader_overrides.clone(),
|
||||
cloud_config_bundle,
|
||||
prompt,
|
||||
images,
|
||||
session_selection,
|
||||
|
||||
Reference in New Issue
Block a user