mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Surface TUI config write error causes (#26537)
## Summary TUI config writes currently wrap app-server failures with local context like `config/batchWrite failed in TUI`, but several user-visible paths only render the outer error. That hides the actionable app-server message, such as validation constraints or read-only `CODEX_HOME` failures, leaving users with a dead-end diagnostic. This change adds a small formatter next to the TUI config write helpers that renders the error source chain, then uses it for model persistence, feature persistence, project trust, status line writes, hook trust, and hook enablement. Fixes #26077
This commit is contained in:
@@ -7,6 +7,7 @@
|
||||
use super::plugin_mentions::fetch_plugin_mentions;
|
||||
use super::*;
|
||||
use crate::app_event::ConnectorsSnapshot;
|
||||
use crate::config_update::format_config_error;
|
||||
use codex_app_server_protocol::AppsListParams;
|
||||
use codex_app_server_protocol::AppsListResponse;
|
||||
use codex_app_server_protocol::MarketplaceAddParams;
|
||||
@@ -357,7 +358,12 @@ impl App {
|
||||
let result = write_hook_enabled(request_handle, key, enabled)
|
||||
.await
|
||||
.map(|_| ())
|
||||
.map_err(|err| format!("Failed to update hook config: {err}"));
|
||||
.map_err(|err| {
|
||||
format!(
|
||||
"Failed to update hook config: {}",
|
||||
format_config_error(&err)
|
||||
)
|
||||
});
|
||||
app_event_tx.send(AppEvent::HookEnabledSet {
|
||||
key: key_for_event,
|
||||
enabled,
|
||||
@@ -378,7 +384,7 @@ impl App {
|
||||
let result = write_hook_trust(request_handle, key, current_hash)
|
||||
.await
|
||||
.map(|_| ())
|
||||
.map_err(|err| format!("Failed to trust hook: {err}"));
|
||||
.map_err(|err| format!("Failed to trust hook: {}", format_config_error(&err)));
|
||||
app_event_tx.send(AppEvent::HookTrusted { result });
|
||||
});
|
||||
}
|
||||
@@ -394,7 +400,7 @@ impl App {
|
||||
let result = write_hook_trusts(request_handle, updates)
|
||||
.await
|
||||
.map(|_| ())
|
||||
.map_err(|err| format!("Failed to trust hooks: {err}"));
|
||||
.map_err(|err| format!("Failed to trust hooks: {}", format_config_error(&err)));
|
||||
app_event_tx.send(AppEvent::HookTrusted { result });
|
||||
});
|
||||
}
|
||||
|
||||
@@ -482,9 +482,10 @@ impl App {
|
||||
{
|
||||
Ok(response) => response,
|
||||
Err(err) => {
|
||||
tracing::error!(error = %err, "failed to persist feature flags");
|
||||
let error = crate::config_update::format_config_error(&err);
|
||||
tracing::error!(error = %error, "failed to persist feature flags");
|
||||
self.chat_widget
|
||||
.add_error_message(format!("Failed to update experimental features: {err}"));
|
||||
.add_error_message(format!("Failed to update experimental features: {error}"));
|
||||
return;
|
||||
}
|
||||
};
|
||||
|
||||
@@ -5,6 +5,7 @@
|
||||
|
||||
use super::resize_reflow::trailing_run_start;
|
||||
use super::*;
|
||||
use crate::config_update::format_config_error;
|
||||
#[cfg(target_os = "windows")]
|
||||
use codex_config::types::WindowsSandboxModeToml;
|
||||
|
||||
@@ -1321,12 +1322,13 @@ impl App {
|
||||
self.chat_widget.add_info_message(message, /*hint*/ None);
|
||||
}
|
||||
Err(err) => {
|
||||
let error = format_config_error(&err);
|
||||
tracing::error!(
|
||||
error = %err,
|
||||
error = %error,
|
||||
"failed to persist model selection"
|
||||
);
|
||||
self.chat_widget
|
||||
.add_error_message(format!("Failed to save default model: {err}"));
|
||||
.add_error_message(format!("Failed to save default model: {error}"));
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1945,9 +1947,10 @@ impl App {
|
||||
self.chat_widget.setup_status_line(items, use_theme_colors);
|
||||
}
|
||||
Err(err) => {
|
||||
tracing::error!(error = %err, "failed to persist status line settings; keeping previous selection");
|
||||
let error = format_config_error(&err);
|
||||
tracing::error!(error = %error, "failed to persist status line settings; keeping previous selection");
|
||||
self.chat_widget.add_error_message(format!(
|
||||
"Failed to save status line settings: {err}"
|
||||
"Failed to save status line settings: {error}"
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
+7
@@ -0,0 +1,7 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/config_errors_tests.rs
|
||||
expression: normalize_snapshot_paths(term.backend().vt100().screen().contents())
|
||||
---
|
||||
■ Failed to save default model: config/batchWrite failed
|
||||
in TUI: Invalid configuration: features.fast_mode=true
|
||||
is not supported; allowed set [fast_mode=false]
|
||||
@@ -219,6 +219,8 @@ macro_rules! assert_chatwidget_snapshot {
|
||||
mod app_server;
|
||||
mod approval_requests;
|
||||
mod composer_submission;
|
||||
#[path = "tests/config_errors_tests.rs"]
|
||||
mod config_errors;
|
||||
mod exec_flow;
|
||||
mod goal_menu;
|
||||
mod goal_validation;
|
||||
|
||||
@@ -0,0 +1,25 @@
|
||||
use super::*;
|
||||
|
||||
#[tokio::test]
|
||||
async fn chained_config_error_wraps_in_history_snapshot() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
chat.add_error_message(
|
||||
"Failed to save default model: config/batchWrite failed in TUI: Invalid configuration: features.fast_mode=true is not supported; allowed set [fast_mode=false]"
|
||||
.to_string(),
|
||||
);
|
||||
|
||||
let width = 56;
|
||||
let height = 8;
|
||||
let backend = VT100Backend::new(width, height);
|
||||
let mut term = crate::custom_terminal::Terminal::with_options(backend).expect("terminal");
|
||||
term.set_viewport_area(ratatui::layout::Rect::new(0, 0, width, height));
|
||||
for lines in drain_insert_history(&mut rx) {
|
||||
crate::insert_history::insert_history_lines(&mut term, lines)
|
||||
.expect("insert history lines");
|
||||
}
|
||||
|
||||
assert_chatwidget_snapshot!(
|
||||
"chained_config_error_wraps_in_history_snapshot",
|
||||
normalize_snapshot_paths(term.backend().vt100().screen().contents())
|
||||
);
|
||||
}
|
||||
@@ -23,6 +23,7 @@ use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use color_eyre::eyre::Result;
|
||||
use color_eyre::eyre::WrapErr;
|
||||
use serde_json::Value as JsonValue;
|
||||
use std::fmt::Display;
|
||||
use std::path::Path;
|
||||
use uuid::Uuid;
|
||||
|
||||
@@ -43,6 +44,10 @@ pub(crate) fn app_scoped_key_path(app_id: &str, key_path: &str) -> String {
|
||||
format!("apps.{app_id}.{key_path}")
|
||||
}
|
||||
|
||||
pub(crate) fn format_config_error(err: &impl Display) -> String {
|
||||
format!("{err:#}")
|
||||
}
|
||||
|
||||
fn trusted_project_edit(project_path: &Path) -> ConfigEdit {
|
||||
let project_key = project_trust_key(project_path)
|
||||
.replace('\\', "\\\\")
|
||||
@@ -203,27 +208,5 @@ pub(crate) async fn write_skill_enabled(
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn app_scoped_key_path_quotes_dotted_app_ids() {
|
||||
assert_eq!(
|
||||
app_scoped_key_path("plugin.linear", "enabled"),
|
||||
"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,
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
#[path = "config_update_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -0,0 +1,40 @@
|
||||
use super::*;
|
||||
use color_eyre::eyre::WrapErr;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::path::Path;
|
||||
|
||||
#[test]
|
||||
fn app_scoped_key_path_quotes_dotted_app_ids() {
|
||||
assert_eq!(
|
||||
app_scoped_key_path("plugin.linear", "enabled"),
|
||||
"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,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn format_config_error_preserves_server_validation_message() {
|
||||
let err = Err::<(), _>(color_eyre::eyre::eyre!(
|
||||
"config/batchWrite failed: Invalid configuration: features.fast_mode=true violates \
|
||||
managed requirements; allowed set [fast_mode=false]"
|
||||
))
|
||||
.wrap_err("config/batchWrite failed in TUI")
|
||||
.unwrap_err();
|
||||
|
||||
assert_eq!(
|
||||
format_config_error(&err),
|
||||
"config/batchWrite failed in TUI: config/batchWrite failed: Invalid configuration: \
|
||||
features.fast_mode=true violates managed requirements; allowed set [fast_mode=false]"
|
||||
);
|
||||
}
|
||||
@@ -32,6 +32,7 @@ use codex_protocol::config_types::ForcedLoginMethod;
|
||||
|
||||
use crate::LoginStatus;
|
||||
use crate::app_server_session::AppServerSession;
|
||||
use crate::config_update::format_config_error;
|
||||
use crate::config_update::write_trusted_project;
|
||||
use crate::key_hint::KeyBindingListExt;
|
||||
use crate::legacy_core::config::Config;
|
||||
@@ -605,8 +606,9 @@ async fn persist_selected_trust(
|
||||
match result {
|
||||
Ok(()) => true,
|
||||
Err(error) => {
|
||||
let error = format_config_error(&error);
|
||||
tracing::error!(
|
||||
"failed to persist trusted project state for {}: {error:?}",
|
||||
"failed to persist trusted project state for {}: {error}",
|
||||
trust_target.display()
|
||||
);
|
||||
if let Step::TrustDirectory(widget) = &mut onboarding_screen.steps[trust_step_index] {
|
||||
|
||||
+19
@@ -0,0 +1,19 @@
|
||||
---
|
||||
source: tui/src/onboarding/trust_directory.rs
|
||||
expression: terminal.backend()
|
||||
---
|
||||
> You are in /workspace/project
|
||||
|
||||
Do you trust the contents of this directory? Working with untrusted
|
||||
contents comes with higher risk of prompt injection. Trusting the
|
||||
directory allows project-local config, hooks, and exec policies to
|
||||
load.
|
||||
|
||||
› 1. Yes, continue
|
||||
2. No, quit
|
||||
|
||||
Failed to set trust for /workspace/project: config/batchWrite failed
|
||||
in TUI: Invalid configuration: features.fast_mode=true is not
|
||||
supported; allowed set [fast_mode=false]
|
||||
|
||||
Press enter to continue
|
||||
@@ -191,6 +191,18 @@ mod tests {
|
||||
use ratatui::Terminal;
|
||||
use std::path::PathBuf;
|
||||
|
||||
fn widget(error: Option<String>) -> TrustDirectoryWidget {
|
||||
TrustDirectoryWidget {
|
||||
cwd: PathBuf::from("/workspace/project"),
|
||||
trust_target: PathBuf::from("/workspace/project"),
|
||||
show_windows_create_sandbox_hint: false,
|
||||
should_quit: false,
|
||||
selection: None,
|
||||
highlighted: TrustDirectorySelection::Trust,
|
||||
error,
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn release_event_does_not_change_selection() {
|
||||
let mut widget = TrustDirectoryWidget {
|
||||
@@ -217,15 +229,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn renders_snapshot_for_git_repo() {
|
||||
let widget = TrustDirectoryWidget {
|
||||
cwd: PathBuf::from("/workspace/project"),
|
||||
trust_target: PathBuf::from("/workspace/project"),
|
||||
show_windows_create_sandbox_hint: false,
|
||||
should_quit: false,
|
||||
selection: None,
|
||||
highlighted: TrustDirectorySelection::Trust,
|
||||
error: None,
|
||||
};
|
||||
let widget = widget(/*error*/ None);
|
||||
|
||||
let mut terminal =
|
||||
Terminal::new(VT100Backend::new(/*width*/ 70, /*height*/ 14)).expect("terminal");
|
||||
@@ -235,4 +239,20 @@ mod tests {
|
||||
|
||||
insta::assert_snapshot!(terminal.backend());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn renders_snapshot_for_trust_error() {
|
||||
let widget = widget(Some(
|
||||
"Failed to set trust for /workspace/project: config/batchWrite failed in TUI: Invalid configuration: features.fast_mode=true is not supported; allowed set [fast_mode=false]"
|
||||
.to_string(),
|
||||
));
|
||||
|
||||
let mut terminal =
|
||||
Terminal::new(VT100Backend::new(/*width*/ 70, /*height*/ 18)).expect("terminal");
|
||||
terminal
|
||||
.draw(|f| (&widget).render_ref(f.area(), f.buffer_mut()))
|
||||
.expect("draw");
|
||||
|
||||
insta::assert_snapshot!(terminal.backend());
|
||||
}
|
||||
}
|
||||
|
||||
+9
-9
@@ -2,13 +2,13 @@
|
||||
source: tui/src/startup_hooks_review.rs
|
||||
expression: "render_lines(&view, 80)"
|
||||
---
|
||||
|
||||
Hooks need review
|
||||
2 hooks are new or changed.
|
||||
Hooks can run outside the sandbox after you trust them.
|
||||
|
||||
› 1. Review hooks
|
||||
2. Trust all and continue
|
||||
3. Continue without trusting (hooks won't run)
|
||||
|
||||
|
||||
Hooks need review
|
||||
2 hooks are new or changed.
|
||||
Hooks can run outside the sandbox after you trust them.
|
||||
|
||||
› 1. Review hooks
|
||||
2. Trust all and continue
|
||||
3. Continue without trusting (hooks won't run)
|
||||
|
||||
Press enter to confirm or esc to go back
|
||||
|
||||
+13
-11
@@ -1,15 +1,17 @@
|
||||
---
|
||||
source: tui/src/startup_hooks_review.rs
|
||||
expression: "render_lines(&view, 80)"
|
||||
expression: "render_lines(&view, 62)"
|
||||
---
|
||||
|
||||
Hooks need review
|
||||
2 hooks are new or changed.
|
||||
Hooks can run outside the sandbox after you trust them.
|
||||
Failed to trust hooks: disk full
|
||||
|
||||
› 1. Review hooks
|
||||
2. Trust all and continue
|
||||
3. Continue without trusting (hooks won't run)
|
||||
|
||||
|
||||
Hooks need review
|
||||
2 hooks are new or changed.
|
||||
Hooks can run outside the sandbox after you trust them.
|
||||
Failed to trust hooks: config/batchWrite failed in TUI:
|
||||
Invalid configuration: features.fast_mode=true is not
|
||||
supported; allowed set [fast_mode=false]
|
||||
|
||||
› 1. Review hooks
|
||||
2. Trust all and continue
|
||||
3. Continue without trusting (hooks won't run)
|
||||
|
||||
Press enter to confirm or esc to go back
|
||||
|
||||
@@ -5,7 +5,9 @@ use ratatui::layout::Rect;
|
||||
use ratatui::style::Stylize;
|
||||
use ratatui::text::Line;
|
||||
use ratatui::widgets::Clear;
|
||||
use ratatui::widgets::Paragraph;
|
||||
use ratatui::widgets::WidgetRef;
|
||||
use ratatui::widgets::Wrap;
|
||||
use tokio::sync::mpsc::unbounded_channel;
|
||||
use tokio_stream::StreamExt;
|
||||
|
||||
@@ -17,6 +19,7 @@ use crate::bottom_pane::ListSelectionView;
|
||||
use crate::bottom_pane::SelectionItem;
|
||||
use crate::bottom_pane::SelectionViewParams;
|
||||
use crate::bottom_pane::popup_consts::standard_popup_hint_line_for_keymap;
|
||||
use crate::config_update::format_config_error;
|
||||
use crate::hooks_rpc::HookTrustUpdate;
|
||||
use crate::hooks_rpc::fetch_hooks_list;
|
||||
use crate::hooks_rpc::hook_needs_review;
|
||||
@@ -130,7 +133,9 @@ async fn run_startup_hooks_review_app(
|
||||
)
|
||||
.await
|
||||
.map(|_| ())
|
||||
.map_err(|err| format!("Failed to trust hooks: {err}"));
|
||||
.map_err(|err| {
|
||||
format!("Failed to trust hooks: {}", format_config_error(&err))
|
||||
});
|
||||
match result {
|
||||
Ok(()) => return Ok(StartupHooksReviewOutcome::Continue),
|
||||
Err(err) => {
|
||||
@@ -199,7 +204,7 @@ fn selection_view_params(
|
||||
"Hooks can run outside the sandbox after you trust them.".dim(),
|
||||
));
|
||||
if let Some(error) = trust_all_error {
|
||||
header.push(Line::from(error.to_string()).red());
|
||||
header.push(Paragraph::new(Line::from(error.to_string()).red()).wrap(Wrap { trim: false }));
|
||||
} else if trusting_all {
|
||||
header.push(Line::from("Trusting hooks...".dim()));
|
||||
}
|
||||
@@ -333,7 +338,7 @@ mod tests {
|
||||
}
|
||||
})
|
||||
.collect::<String>();
|
||||
format!("{rendered:width$}", width = area.width as usize)
|
||||
rendered.trim_end().to_string()
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n")
|
||||
@@ -373,7 +378,9 @@ mod tests {
|
||||
let keymap = RuntimeKeymap::defaults();
|
||||
let view = selection_view(
|
||||
&entry(),
|
||||
Some("Failed to trust hooks: disk full"),
|
||||
Some(
|
||||
"Failed to trust hooks: config/batchWrite failed in TUI: Invalid configuration: features.fast_mode=true is not supported; allowed set [fast_mode=false]",
|
||||
),
|
||||
/*trusting_all*/ false,
|
||||
AppEventSender::new(tx_raw),
|
||||
&keymap,
|
||||
@@ -381,7 +388,7 @@ mod tests {
|
||||
|
||||
assert_snapshot!(
|
||||
"startup_hooks_review_prompt_with_trust_error",
|
||||
render_lines(&view, /*width*/ 80)
|
||||
render_lines(&view, /*width*/ 62)
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user