mirror of
https://github.com/openai/codex.git
synced 2026-04-24 14:45:27 +00:00
fix invisible approvals
This commit is contained in:
@@ -3,10 +3,12 @@ use codex_core::config::Config;
|
||||
use codex_core::protocol::AgentMessageDeltaEvent;
|
||||
use codex_core::protocol::AgentMessageEvent;
|
||||
use codex_core::protocol::AgentReasoningDeltaEvent;
|
||||
use codex_core::protocol::ApplyPatchApprovalRequestEvent;
|
||||
use codex_core::protocol::BackgroundEventEvent;
|
||||
use codex_core::protocol::ErrorEvent;
|
||||
use codex_core::protocol::Event;
|
||||
use codex_core::protocol::EventMsg;
|
||||
use codex_core::protocol::ExecApprovalRequestEvent;
|
||||
use codex_core::protocol::ExecCommandBeginEvent;
|
||||
use codex_core::protocol::ExecCommandEndEvent;
|
||||
use codex_core::protocol::FileChange;
|
||||
@@ -474,11 +476,45 @@ impl EventProcessor for EventProcessorWithHumanOutput {
|
||||
println!("{}", line.style(self.dimmed));
|
||||
}
|
||||
}
|
||||
EventMsg::ExecApprovalRequest(_) => {
|
||||
// Should we exit?
|
||||
EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent {
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
..
|
||||
}) => {
|
||||
ts_println!(
|
||||
self,
|
||||
"{} {} in {}",
|
||||
"approval required for".style(self.magenta),
|
||||
escape_command(&command).style(self.bold),
|
||||
cwd.to_string_lossy(),
|
||||
);
|
||||
if let Some(r) = reason {
|
||||
ts_println!(self, "{r}");
|
||||
}
|
||||
return CodexStatus::InitiateShutdown;
|
||||
}
|
||||
EventMsg::ApplyPatchApprovalRequest(_) => {
|
||||
// Should we exit?
|
||||
EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent {
|
||||
changes,
|
||||
reason,
|
||||
..
|
||||
}) => {
|
||||
ts_println!(
|
||||
self,
|
||||
"{}:",
|
||||
"approval required for apply_patch".style(self.magenta),
|
||||
);
|
||||
for (path, change) in changes.iter() {
|
||||
println!(
|
||||
" {} {}",
|
||||
format_file_change(change).style(self.cyan),
|
||||
path.to_string_lossy(),
|
||||
);
|
||||
}
|
||||
if let Some(r) = reason {
|
||||
ts_println!(self, "{r}");
|
||||
}
|
||||
return CodexStatus::InitiateShutdown;
|
||||
}
|
||||
EventMsg::AgentReasoning(agent_reasoning_event) => {
|
||||
if self.show_agent_reasoning {
|
||||
@@ -538,3 +574,42 @@ fn format_file_change(change: &FileChange) -> &'static str {
|
||||
} => "M",
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
use codex_core::config::ConfigToml;
|
||||
use codex_core::protocol::Event;
|
||||
use codex_core::protocol::EventMsg;
|
||||
use codex_core::protocol::ExecApprovalRequestEvent;
|
||||
|
||||
fn test_config() -> Config {
|
||||
Config::load_from_base_config_with_overrides(
|
||||
ConfigToml::default(),
|
||||
ConfigOverrides::default(),
|
||||
std::env::temp_dir(),
|
||||
)
|
||||
.unwrap()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exec_approval_request_displays_command() {
|
||||
let config = test_config();
|
||||
let mut processor = EventProcessorWithHumanOutput::create_with_ansi(false, &config, None);
|
||||
|
||||
let event = Event {
|
||||
id: "1".into(),
|
||||
msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent {
|
||||
call_id: "c1".into(),
|
||||
command: vec!["rm".into(), "-rf".into(), "/".into()],
|
||||
cwd: PathBuf::from("/tmp"),
|
||||
reason: None,
|
||||
}),
|
||||
};
|
||||
|
||||
let status = processor.process_event(event);
|
||||
assert!(matches!(status, CodexStatus::InitiateShutdown));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -6,7 +6,7 @@ use ratatui::text::Line;
|
||||
use crate::slash_command::SlashCommand;
|
||||
|
||||
#[allow(clippy::large_enum_variant)]
|
||||
pub(crate) enum AppEvent {
|
||||
pub enum AppEvent {
|
||||
CodexEvent(Event),
|
||||
|
||||
/// Request a redraw which will be debounced by the [`App`].
|
||||
|
||||
@@ -3,18 +3,18 @@ use std::sync::mpsc::Sender;
|
||||
use crate::app_event::AppEvent;
|
||||
|
||||
#[derive(Clone, Debug)]
|
||||
pub(crate) struct AppEventSender {
|
||||
pub struct AppEventSender {
|
||||
app_event_tx: Sender<AppEvent>,
|
||||
}
|
||||
|
||||
impl AppEventSender {
|
||||
pub(crate) fn new(app_event_tx: Sender<AppEvent>) -> Self {
|
||||
pub 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(crate) fn send(&self, event: AppEvent) {
|
||||
pub fn send(&self, event: AppEvent) {
|
||||
if let Err(e) = self.app_event_tx.send(event) {
|
||||
tracing::error!("failed to send event: {e}");
|
||||
}
|
||||
|
||||
@@ -45,6 +45,11 @@ 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, UserApprovalWidget};
|
||||
|
||||
pub use cli::Cli;
|
||||
|
||||
pub fn run_main(
|
||||
|
||||
@@ -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(crate) enum ApprovalRequest {
|
||||
pub 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(crate) struct UserApprovalWidget<'a> {
|
||||
pub struct UserApprovalWidget<'a> {
|
||||
approval_request: ApprovalRequest,
|
||||
app_event_tx: AppEventSender,
|
||||
confirmation_prompt: Paragraph<'a>,
|
||||
@@ -117,7 +117,7 @@ pub(crate) struct UserApprovalWidget<'a> {
|
||||
}
|
||||
|
||||
impl UserApprovalWidget<'_> {
|
||||
pub(crate) fn new(approval_request: ApprovalRequest, app_event_tx: AppEventSender) -> Self {
|
||||
pub 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 don’t need to report whether the event
|
||||
/// was consumed—callers can assume it always is.
|
||||
pub(crate) fn handle_key_event(&mut self, key: KeyEvent) {
|
||||
pub 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(crate) fn is_complete(&self) -> bool {
|
||||
pub fn is_complete(&self) -> bool {
|
||||
self.done
|
||||
}
|
||||
}
|
||||
|
||||
79
codex-rs/tui/tests/approval_widget.rs
Normal file
79
codex-rs/tui/tests/approval_widget.rs
Normal file
@@ -0,0 +1,79 @@
|
||||
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, AppEventSender, ApprovalRequest, 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;
|
||||
}
|
||||
Reference in New Issue
Block a user