fix approvals

This commit is contained in:
Eason Goodale
2025-07-26 12:15:05 -07:00
parent 6a64165349
commit 2358af7803
6 changed files with 109 additions and 98 deletions

View File

@@ -591,7 +591,7 @@ mod tests {
ConfigOverrides::default(),
std::env::temp_dir(),
)
.unwrap()
.unwrap_or_else(|e| panic!("failed to load test configuration: {e}"))
}
#[test]

View File

@@ -6,7 +6,7 @@ use ratatui::text::Line;
use crate::slash_command::SlashCommand;
#[allow(clippy::large_enum_variant)]
pub enum AppEvent {
pub(crate) enum AppEvent {
CodexEvent(Event),
/// Request a redraw which will be debounced by the [`App`].

View File

@@ -3,18 +3,18 @@ use std::sync::mpsc::Sender;
use crate::app_event::AppEvent;
#[derive(Clone, Debug)]
pub struct AppEventSender {
pub(crate) struct AppEventSender {
app_event_tx: Sender<AppEvent>,
}
impl AppEventSender {
pub fn new(app_event_tx: Sender<AppEvent>) -> Self {
pub(crate) fn new(app_event_tx: Sender<AppEvent>) -> Self {
Self { app_event_tx }
}
/// Send an event to the app event channel. If it fails, we swallow the
/// error and log it.
pub fn send(&self, event: AppEvent) {
pub(crate) fn send(&self, event: AppEvent) {
if let Err(e) = self.app_event_tx.send(event) {
tracing::error!("failed to send event: {e}");
}

View File

@@ -45,12 +45,6 @@ mod text_formatting;
mod tui;
mod user_approval_widget;
// Re-export a small subset of internal types used by integration tests.
pub use app_event::AppEvent;
pub use app_event_sender::AppEventSender;
pub use user_approval_widget::ApprovalRequest;
pub use user_approval_widget::UserApprovalWidget;
pub use cli::Cli;
pub fn run_main(

View File

@@ -33,7 +33,7 @@ use crate::exec_command::relativize_to_home;
use crate::exec_command::strip_bash_lc_and_escape;
/// Request coming from the agent that needs user approval.
pub enum ApprovalRequest {
pub(crate) enum ApprovalRequest {
Exec {
id: String,
command: Vec<String>,
@@ -97,7 +97,7 @@ enum Mode {
}
/// A modal prompting the user to approve or deny the pending request.
pub struct UserApprovalWidget<'a> {
pub(crate) struct UserApprovalWidget<'a> {
approval_request: ApprovalRequest,
app_event_tx: AppEventSender,
confirmation_prompt: Paragraph<'a>,
@@ -117,7 +117,7 @@ pub struct UserApprovalWidget<'a> {
}
impl UserApprovalWidget<'_> {
pub fn new(approval_request: ApprovalRequest, app_event_tx: AppEventSender) -> Self {
pub(crate) fn new(approval_request: ApprovalRequest, app_event_tx: AppEventSender) -> Self {
let input = Input::default();
let confirmation_prompt = match &approval_request {
ApprovalRequest::Exec {
@@ -196,7 +196,7 @@ impl UserApprovalWidget<'_> {
/// Process a key event originating from crossterm. As the modal fully
/// captures input while visible, we dont need to report whether the event
/// was consumed—callers can assume it always is.
pub fn handle_key_event(&mut self, key: KeyEvent) {
pub(crate) fn handle_key_event(&mut self, key: KeyEvent) {
match self.mode {
Mode::Select => self.handle_select_key(key),
Mode::Input => self.handle_input_key(key),
@@ -289,7 +289,7 @@ impl UserApprovalWidget<'_> {
/// Returns `true` once the user has made a decision and the widget no
/// longer needs to be displayed.
pub fn is_complete(&self) -> bool {
pub(crate) fn is_complete(&self) -> bool {
self.done
}
}
@@ -368,3 +368,102 @@ impl WidgetRef for &UserApprovalWidget<'_> {
Widget::render(List::new(lines), response_chunk, buf);
}
}
#[cfg(test)]
mod tests {
use super::*;
use crate::app_event::AppEvent;
use crate::app_event_sender::AppEventSender;
use ratatui::buffer::Buffer;
use ratatui::layout::Rect;
use ratatui::widgets::WidgetRef;
use std::path::PathBuf;
use std::sync::mpsc::channel;
#[test]
fn exec_command_is_visible_in_small_viewport() {
let long_reason = "This is a very long explanatory reason that would normally occupy many lines in the confirmation prompt. \
It should not cause the actual command or the response options to be scrolled out of the visible area.";
let (tx, _rx) = channel::<AppEvent>();
let app_tx = AppEventSender::new(tx);
let cwd = PathBuf::from("/home/alice/project");
let command = vec![
"bash".to_string(),
"-lc".to_string(),
"echo 123 && printf 'hello'".to_string(),
];
let widget = UserApprovalWidget::new(
ApprovalRequest::Exec {
id: "test-id".to_string(),
command: command.clone(),
cwd: cwd.clone(),
reason: Some(long_reason.to_string()),
},
app_tx,
);
let area = Rect::new(0, 0, 50, 12);
let mut buf = Buffer::empty(area);
(&widget).render_ref(area, &mut buf);
let mut rendered = String::new();
for y in 0..area.height {
for x in 0..area.width {
let cell = &buf[(x, y)];
rendered.push(cell.symbol().chars().next().unwrap_or('\0'));
}
rendered.push('\n');
}
assert!(
rendered.contains("echo 123 && printf 'hello'"),
"rendered buffer did not contain the command.\n--- buffer ---\n{rendered}\n----------------"
);
assert!(rendered.contains("Yes (y)"));
}
#[test]
fn all_options_visible_in_reasonable_viewport() {
let (tx, _rx) = channel::<AppEvent>();
let app_tx = AppEventSender::new(tx);
let widget = UserApprovalWidget::new(
ApprovalRequest::Exec {
id: "test-id".to_string(),
command: vec![
"bash".into(),
"-lc".into(),
"echo 123 && printf 'hello'".into(),
],
cwd: PathBuf::from("/home/alice/project"),
reason: Some("short reason".into()),
},
app_tx,
);
// Use a generous area to avoid truncation of either the prompt or the options.
let area = Rect::new(0, 0, 100, 30);
let mut buf = Buffer::empty(area);
(&widget).render_ref(area, &mut buf);
let mut rendered = String::new();
for y in 0..area.height {
for x in 0..area.width {
let cell = &buf[(x, y)];
rendered.push(cell.symbol().chars().next().unwrap_or('\0'));
}
rendered.push('\n');
}
for opt in super::SELECT_OPTIONS {
assert!(
rendered.contains(opt.label),
"expected option label to be visible: {}\n--- buffer ---\n{rendered}\n----------------",
opt.label
);
}
}
}

View File

@@ -1,82 +0,0 @@
use std::path::PathBuf;
use std::sync::mpsc::channel;
use codex_core::protocol::ReviewDecision; // keep this import to ensure the enum is linked
use ratatui::buffer::Buffer;
use ratatui::layout::Rect;
use ratatui::widgets::WidgetRef;
use codex_tui::AppEvent;
use codex_tui::AppEventSender;
use codex_tui::ApprovalRequest;
use codex_tui::UserApprovalWidget;
/// Regression test: ensure that the exec command line (cwd + command) is always
/// visible when rendering the approval modal, even when the available viewport
/// is very small and the confirmation prompt needs to be truncated.
#[test]
fn exec_command_is_visible_in_small_viewport() {
// Construct a long reason to inflate the confirmation prompt, so we can
// verify that it gets truncated rather than pushing the command or the
// response options out of view.
let long_reason = "This is a very long explanatory reason that would normally occupy many lines in the confirmation prompt. \
It should not cause the actual command or the response options to be scrolled out of the visible area.".to_string();
// Minimal AppEventSender the widget will send an event when a decision is
// made, but we will not interact with it in this rendering test.
let (tx, _rx) = channel::<AppEvent>();
let app_tx = AppEventSender::new(tx);
let cwd = PathBuf::from("/home/alice/project");
let command = vec![
"bash".to_string(),
"-lc".to_string(),
"echo 123 && printf 'hello'".to_string(),
];
let widget = UserApprovalWidget::new(
ApprovalRequest::Exec {
id: "test-id".to_string(),
command: command.clone(),
cwd: cwd.clone(),
reason: Some(long_reason),
},
app_tx,
);
// Render into a deliberately small area to force truncation of the prompt.
// The outer border consumes one row/column on each side, leaving very
// little space inside this is fine, we just want to see that the command
// line is still present in the buffer.
let area = Rect::new(0, 0, 50, 12);
let mut buf = Buffer::empty(area);
(&widget).render_ref(area, &mut buf);
// Extract the contents of the buffer into a single string for searching.
let mut rendered = String::new();
for y in 0..area.height {
for x in 0..area.width {
let cell = buf.get(x, y);
rendered.push(cell.symbol().chars().next().unwrap_or('\0'));
}
rendered.push('\n');
}
// The command is displayed after stripping the leading "bash -lc" wrapper
// and shell-escaping. For our input this should result in the literal
// command text below. The cwd is printed as a prefix ending with a '$', but
// to keep the assertion robust we only verify that the command itself is
// present in the rendered buffer.
assert!(
rendered.contains("echo 123 && printf 'hello'"),
"rendered buffer did not contain the command.\n--- buffer ---\n{rendered}\n----------------"
);
// Additionally assert that at least one of the response options is visible
// to ensure the interactive section has not been pushed out of view.
assert!(rendered.contains("Yes (y)"));
// Silence unused import warning for ReviewDecision (ensures the crate is
// linked with the protocol module and avoids accidental dead-code removal).
let _ = ReviewDecision::Approved;
}