mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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.<project>.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.<project>.trust_level = "trusted"`, and simulated an unwritable config to verify onboarding stays on the trust screen without writing trust.
This commit is contained in:
committed by
GitHub
Unverified
parent
f05fd0e661
commit
bb55736906
@@ -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<String>, 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<impl ToString>,
|
||||
@@ -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<ConfigWriteResponse> {
|
||||
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,
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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<TrustDirectorySelection>,
|
||||
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<Step> = 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<TrustDirectorySelection> {
|
||||
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<OnboardingResult> {
|
||||
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<AppServerRequestHandle>,
|
||||
) -> 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"))
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user