From bb557369068fec5707edbae6013c090e05b1d1f6 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 25 May 2026 09:54:05 -0700 Subject: [PATCH] TUI config cleanup: trusted projects (#24255) ## Why TUI onboarding trusted-project persistence should go through the same app-server config write path as other config mutations. Writing `config.toml` directly from the trust widget bypasses that layer and can let onboarding proceed even when the trust decision was not actually persisted. ## What changed - Added a TUI config helper that writes the existing project trust structure through `config/batchWrite`. - Persists trust decisions as `projects..trust_level = "trusted"` using the existing project trust key helper. - Changed the trust directory widget to only record the user selection; onboarding performs the app-server write before reporting success. - Keeps the user on the trust screen and shows an error if app-server persistence fails. ## Verification - `cargo test -p codex-tui --lib trust_persistence_failure_keeps_trust_step_in_progress` - `cargo test -p codex-tui --lib trusted_project_edit_targets_project_trust_level` - Manual: built the local `codex-cli`, accepted the trust prompt in a temp project, confirmed `projects..trust_level = "trusted"`, and simulated an unwritable config to verify onboarding stays on the trust screen without writing trust. --- codex-rs/tui/src/config_update.rs | 32 +++++ codex-rs/tui/src/lib.rs | 4 +- .../tui/src/onboarding/onboarding_screen.rs | 118 +++++++++++++++--- .../tui/src/onboarding/trust_directory.rs | 16 +-- 4 files changed, 137 insertions(+), 33 deletions(-) diff --git a/codex-rs/tui/src/config_update.rs b/codex-rs/tui/src/config_update.rs index 16ade0400..a882b2123 100644 --- a/codex-rs/tui/src/config_update.rs +++ b/codex-rs/tui/src/config_update.rs @@ -15,12 +15,15 @@ use codex_app_server_protocol::MergeStrategy; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::SkillsConfigWriteParams; use codex_app_server_protocol::SkillsConfigWriteResponse; +use codex_config::loader::project_trust_key; use codex_features::FEATURES; use codex_protocol::config_types::SERVICE_TIER_DEFAULT_REQUEST_VALUE; +use codex_protocol::config_types::TrustLevel; use codex_utils_absolute_path::AbsolutePathBuf; use color_eyre::eyre::Result; use color_eyre::eyre::WrapErr; use serde_json::Value as JsonValue; +use std::path::Path; use uuid::Uuid; pub(crate) fn replace_config_value(key_path: impl Into, value: JsonValue) -> ConfigEdit { @@ -40,6 +43,16 @@ pub(crate) fn app_scoped_key_path(app_id: &str, key_path: &str) -> String { format!("apps.{app_id}.{key_path}") } +fn trusted_project_edit(project_path: &Path) -> ConfigEdit { + let project_key = project_trust_key(project_path) + .replace('\\', "\\\\") + .replace('"', "\\\""); + replace_config_value( + format!("projects.\"{project_key}\".trust_level"), + serde_json::json!(TrustLevel::Trusted.to_string()), + ) +} + pub(crate) fn build_model_selection_edits( model: &str, effort: Option, @@ -145,6 +158,13 @@ pub(crate) async fn write_config_batch( .wrap_err("config/batchWrite failed in TUI") } +pub(crate) async fn write_trusted_project( + request_handle: AppServerRequestHandle, + project_path: &Path, +) -> Result { + write_config_batch(request_handle, vec![trusted_project_edit(project_path)]).await +} + pub(crate) async fn read_effective_config( request_handle: AppServerRequestHandle, cwd: String, @@ -194,4 +214,16 @@ mod tests { "apps.\"plugin.linear\".enabled" ); } + + #[test] + fn trusted_project_edit_targets_project_trust_level() { + assert_eq!( + trusted_project_edit(Path::new("/workspace/team.project")), + ConfigEdit { + key_path: "projects.\"/workspace/team.project\".trust_level".to_string(), + value: serde_json::json!("trusted"), + merge_strategy: MergeStrategy::Replace, + } + ); + } } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 33b79b278..d7a605c73 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -1425,7 +1425,7 @@ async fn run_ratatui_app( exit_reason: ExitReason::UserRequested, }); } - trust_decision_was_made = onboarding_result.directory_trust_decision.is_some(); + trust_decision_was_made = onboarding_result.directory_trust_persisted; // If this onboarding run included the login step, always refresh cloud requirements and // rebuild config. This avoids missing newly available cloud requirements due to login // status detection edge cases. @@ -1441,7 +1441,7 @@ async fn run_ratatui_app( // If the user made an explicit trust decision, or we showed the login flow, reload config // so current process state reflects persisted trust/auth changes. - if onboarding_result.directory_trust_decision.is_some() + if onboarding_result.directory_trust_persisted || (show_login_screen && !uses_remote_workspace) { load_config_or_exit( diff --git a/codex-rs/tui/src/onboarding/onboarding_screen.rs b/codex-rs/tui/src/onboarding/onboarding_screen.rs index 6edb6a6d8..5291934a3 100644 --- a/codex-rs/tui/src/onboarding/onboarding_screen.rs +++ b/codex-rs/tui/src/onboarding/onboarding_screen.rs @@ -32,6 +32,7 @@ use codex_protocol::config_types::ForcedLoginMethod; use crate::LoginStatus; use crate::app_server_session::AppServerSession; +use crate::config_update::write_trusted_project; use crate::key_hint::KeyBindingListExt; use crate::legacy_core::config::Config; #[cfg(target_os = "windows")] @@ -89,7 +90,7 @@ pub(crate) struct OnboardingScreenArgs { } pub(crate) struct OnboardingResult { - pub directory_trust_decision: Option, + pub directory_trust_persisted: bool, pub should_exit: bool, } @@ -111,7 +112,6 @@ impl OnboardingScreen { config, } = args; let cwd = config.cwd.to_path_buf(); - let codex_home = config.codex_home.to_path_buf(); let forced_login_method = config.forced_login_method; let mut steps: Vec = Vec::new(); steps.push(Step::Welcome(WelcomeWidget::new( @@ -154,7 +154,6 @@ impl OnboardingScreen { steps.push(Step::TrustDirectory(TrustDirectoryWidget { cwd, trust_target, - codex_home, show_windows_create_sandbox_hint, should_quit: false, selection: None, @@ -223,19 +222,6 @@ impl OnboardingScreen { .any(|step| matches!(step.get_step_state(), StepState::InProgress)) } - pub fn directory_trust_decision(&self) -> Option { - self.steps - .iter() - .find_map(|step| { - if let Step::TrustDirectory(TrustDirectoryWidget { selection, .. }) = step { - Some(*selection) - } else { - None - } - }) - .flatten() - } - pub fn should_exit(&self) -> bool { self.should_exit } @@ -493,7 +479,9 @@ pub(crate) async fn run_onboarding_app( ) -> Result { use tokio_stream::StreamExt; + let app_server_request_handle = args.app_server_request_handle.clone(); let mut onboarding_screen = OnboardingScreen::new(tui, args).await; + let mut directory_trust_persisted = false; // One-time guard to fully clear the screen after ChatGPT login success message is shown let mut did_full_clear_after_success = false; @@ -511,6 +499,13 @@ pub(crate) async fn run_onboarding_app( match event { TuiEvent::Key(key_event) => { onboarding_screen.handle_key_event(key_event); + if !directory_trust_persisted { + directory_trust_persisted = persist_selected_trust( + &mut onboarding_screen, + app_server_request_handle.clone(), + ) + .await; + } } TuiEvent::Paste(text) => { onboarding_screen.handle_paste(text); @@ -575,18 +570,73 @@ pub(crate) async fn run_onboarding_app( } } Ok(OnboardingResult { - directory_trust_decision: onboarding_screen.directory_trust_decision(), + directory_trust_persisted, should_exit: onboarding_screen.should_exit(), }) } +async fn persist_selected_trust( + onboarding_screen: &mut OnboardingScreen, + request_handle: Option, +) -> bool { + let Some((trust_step_index, trust_target)) = onboarding_screen + .steps + .iter() + .enumerate() + .find_map(|(index, step)| { + if let Step::TrustDirectory(widget) = step + && widget.selection == Some(TrustDirectorySelection::Trust) + { + return Some((index, widget.trust_target.clone())); + } + None + }) + else { + return false; + }; + + let result = match request_handle { + Some(request_handle) => write_trusted_project(request_handle, &trust_target) + .await + .map(|_| ()), + None => Err(color_eyre::eyre::eyre!("app server unavailable")), + }; + + match result { + Ok(()) => true, + Err(error) => { + tracing::error!( + "failed to persist trusted project state for {}: {error:?}", + trust_target.display() + ); + if let Step::TrustDirectory(widget) = &mut onboarding_screen.steps[trust_step_index] { + widget.selection = None; + widget.error = Some(format!( + "Failed to set trust for {}: {error}", + trust_target.display() + )); + } + false + } + } +} + #[cfg(test)] mod tests { use super::ApiKeyEntryContext; + use super::OnboardingScreen; + use super::Step; + use super::StepStateProvider; + use super::persist_selected_trust; use super::suppress_quit_while_typing_api_key; + use crate::onboarding::trust_directory::TrustDirectorySelection; + use crate::onboarding::trust_directory::TrustDirectoryWidget; + use crate::tui::FrameRequester; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; + use pretty_assertions::assert_eq; + use std::path::PathBuf; #[test] fn suppresses_printable_quit_key_during_api_key_entry() { @@ -635,4 +685,38 @@ mod tests { ); assert!(!suppressed); } + + #[tokio::test] + async fn trust_persistence_failure_keeps_trust_step_in_progress() { + let mut onboarding_screen = OnboardingScreen { + request_frame: FrameRequester::test_dummy(), + steps: vec![Step::TrustDirectory(TrustDirectoryWidget { + cwd: PathBuf::from("/workspace/project"), + trust_target: PathBuf::from("/workspace/project"), + show_windows_create_sandbox_hint: false, + should_quit: false, + selection: Some(TrustDirectorySelection::Trust), + highlighted: TrustDirectorySelection::Trust, + error: None, + })], + is_done: false, + should_exit: false, + }; + + let persisted = + persist_selected_trust(&mut onboarding_screen, /*request_handle*/ None).await; + + assert!(!persisted); + let Step::TrustDirectory(widget) = &onboarding_screen.steps[0] else { + panic!("trust step should remain present"); + }; + assert_eq!(widget.selection, None); + assert_eq!(widget.get_step_state(), super::StepState::InProgress); + assert!( + widget + .error + .as_deref() + .is_some_and(|error| error.contains("app server unavailable")) + ); + } } diff --git a/codex-rs/tui/src/onboarding/trust_directory.rs b/codex-rs/tui/src/onboarding/trust_directory.rs index ffc0c040a..36d13e5a5 100644 --- a/codex-rs/tui/src/onboarding/trust_directory.rs +++ b/codex-rs/tui/src/onboarding/trust_directory.rs @@ -1,7 +1,5 @@ use std::path::PathBuf; -use crate::legacy_core::config::set_project_trust_level; -use codex_protocol::config_types::TrustLevel; use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; use ratatui::buffer::Buffer; @@ -24,7 +22,6 @@ use crate::selection_list::selection_option_row; use super::onboarding_screen::StepState; pub(crate) struct TrustDirectoryWidget { - pub codex_home: PathBuf, pub cwd: PathBuf, pub trust_target: PathBuf, pub show_windows_create_sandbox_hint: bool, @@ -166,12 +163,8 @@ impl StepStateProvider for TrustDirectoryWidget { impl TrustDirectoryWidget { fn handle_trust(&mut self) { - let target = self.trust_target.clone(); - if let Err(e) = set_project_trust_level(&self.codex_home, &target, TrustLevel::Trusted) { - tracing::error!("Failed to set project trusted: {e:?}"); - self.error = Some(format!("Failed to set trust for {}: {e}", target.display())); - } - + self.highlighted = TrustDirectorySelection::Trust; + self.error = None; self.selection = Some(TrustDirectorySelection::Trust); } @@ -197,13 +190,10 @@ mod tests { use pretty_assertions::assert_eq; use ratatui::Terminal; use std::path::PathBuf; - use tempfile::TempDir; #[test] fn release_event_does_not_change_selection() { - let codex_home = TempDir::new().expect("temp home"); let mut widget = TrustDirectoryWidget { - codex_home: codex_home.path().to_path_buf(), cwd: PathBuf::from("."), trust_target: PathBuf::from("."), show_windows_create_sandbox_hint: false, @@ -227,9 +217,7 @@ mod tests { #[test] fn renders_snapshot_for_git_repo() { - let codex_home = TempDir::new().expect("temp home"); let widget = TrustDirectoryWidget { - codex_home: codex_home.path().to_path_buf(), cwd: PathBuf::from("/workspace/project"), trust_target: PathBuf::from("/workspace/project"), show_windows_create_sandbox_hint: false,