mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Fix quoted command rendering in tui_app_server (#15825)
When `tui_app_server` is enabled, shell commands in the transcript render as fully quoted invocations like `/bin/zsh -lc "..."`. The non-app-server TUI correctly shows the parsed command body. Root cause: The app-server stores `ThreadItem::CommandExecution.command` as a shell-quoted string. When `tui_app_server` bridges that item back into the exec renderer, it was passing `vec![command]` unchanged instead of splitting the string back into argv. That prevented `strip_bash_lc_and_escape()` from recognizing the shell wrapper, so the renderer displayed the wrapper literally. Solution: Add a shared command-string splitter that round-trips shell-quoted commands back into argv when it is safe to do so, while preserving non-roundtrippable inputs as a single string. Use that helper everywhere `tui_app_server` reconstructs exec commands from app-server payloads, including live command-execution items, replayed thread items, and exec approval requests. This restores the same command display behavior as the direct TUI path without breaking Windows-style commands that cannot be safely round-tripped.
This commit is contained in:
committed by
GitHub
Unverified
parent
4b50446ffa
commit
b565f05d79
@@ -22,6 +22,7 @@ use crate::chatwidget::ReplayKind;
|
||||
use crate::chatwidget::ThreadInputState;
|
||||
use crate::cwd_prompt::CwdPromptAction;
|
||||
use crate::diff_render::DiffSummary;
|
||||
use crate::exec_command::split_command_string;
|
||||
use crate::exec_command::strip_bash_lc_and_escape;
|
||||
use crate::external_editor;
|
||||
use crate::file_search::FileSearchManager;
|
||||
@@ -1701,7 +1702,11 @@ impl App {
|
||||
.approval_id
|
||||
.clone()
|
||||
.unwrap_or_else(|| params.item_id.clone()),
|
||||
command: params.command.clone().into_iter().collect(),
|
||||
command: params
|
||||
.command
|
||||
.as_deref()
|
||||
.map(split_command_string)
|
||||
.unwrap_or_default(),
|
||||
reason: params.reason.clone(),
|
||||
available_decisions: params
|
||||
.available_decisions
|
||||
@@ -7554,6 +7559,36 @@ guardian_approval = true
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn inactive_thread_exec_approval_splits_shell_wrapped_command() {
|
||||
let app = make_test_app().await;
|
||||
let thread_id = ThreadId::new();
|
||||
let script = r#"python3 -c 'print("Hello, world!")'"#;
|
||||
let mut request = exec_approval_request(thread_id, "turn-approval", "call-approval", None);
|
||||
let ServerRequest::CommandExecutionRequestApproval { params, .. } = &mut request else {
|
||||
panic!("expected exec approval request");
|
||||
};
|
||||
params.command = Some(
|
||||
shlex::try_join(["/bin/zsh", "-lc", script]).expect("round-trippable shell wrapper"),
|
||||
);
|
||||
|
||||
let Some(ThreadInteractiveRequest::Approval(ApprovalRequest::Exec { command, .. })) = app
|
||||
.interactive_request_for_thread_request(thread_id, &request)
|
||||
.await
|
||||
else {
|
||||
panic!("expected exec approval request");
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
command,
|
||||
vec![
|
||||
"/bin/zsh".to_string(),
|
||||
"-lc".to_string(),
|
||||
script.to_string(),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn inactive_thread_approval_badge_clears_after_turn_completion_notification() -> Result<()>
|
||||
{
|
||||
|
||||
@@ -16,6 +16,8 @@ use crate::app_event::AppEvent;
|
||||
use crate::app_server_session::AppServerSession;
|
||||
use crate::app_server_session::app_server_rate_limit_snapshot_to_core;
|
||||
use crate::app_server_session::status_account_display_from_auth_mode;
|
||||
#[cfg(test)]
|
||||
use crate::exec_command::split_command_string;
|
||||
use codex_app_server_client::AppServerEvent;
|
||||
use codex_app_server_protocol::AuthMode;
|
||||
use codex_app_server_protocol::JSONRPCErrorError;
|
||||
@@ -956,23 +958,6 @@ fn command_execution_snapshot_events(turn_id: &str, item: &ThreadItem) -> Option
|
||||
Some(events)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
fn split_command_string(command: &str) -> Vec<String> {
|
||||
let Some(parts) = shlex::split(command) else {
|
||||
return vec![command.to_string()];
|
||||
};
|
||||
match shlex::try_join(parts.iter().map(String::as_str)) {
|
||||
Ok(round_trip)
|
||||
if round_trip == command
|
||||
|| (!command.contains(":\\")
|
||||
&& shlex::split(&round_trip).as_ref() == Some(&parts)) =>
|
||||
{
|
||||
parts
|
||||
}
|
||||
_ => vec![command.to_string()],
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
fn app_server_web_search_action_to_core(
|
||||
action: codex_app_server_protocol::WebSearchAction,
|
||||
|
||||
@@ -311,6 +311,7 @@ use crate::diff_render::display_path_for;
|
||||
use crate::exec_cell::CommandOutput;
|
||||
use crate::exec_cell::ExecCell;
|
||||
use crate::exec_cell::new_active_exec_command;
|
||||
use crate::exec_command::split_command_string;
|
||||
use crate::exec_command::strip_bash_lc_and_escape;
|
||||
use crate::get_git_diff::get_git_diff;
|
||||
use crate::history_cell;
|
||||
@@ -1261,7 +1262,11 @@ fn exec_approval_request_from_params(
|
||||
) -> ExecApprovalRequestEvent {
|
||||
ExecApprovalRequestEvent {
|
||||
call_id: params.item_id,
|
||||
command: params.command.into_iter().collect(),
|
||||
command: params
|
||||
.command
|
||||
.as_deref()
|
||||
.map(split_command_string)
|
||||
.unwrap_or_default(),
|
||||
cwd: params.cwd.unwrap_or_default(),
|
||||
reason: params.reason,
|
||||
network_approval_context: params
|
||||
@@ -5809,7 +5814,7 @@ impl ChatWidget {
|
||||
call_id: id,
|
||||
process_id,
|
||||
turn_id: turn_id.clone(),
|
||||
command: vec![command],
|
||||
command: split_command_string(&command),
|
||||
cwd,
|
||||
parsed_cmd: command_actions
|
||||
.into_iter()
|
||||
@@ -5824,7 +5829,7 @@ impl ChatWidget {
|
||||
call_id: id,
|
||||
process_id,
|
||||
turn_id: turn_id.clone(),
|
||||
command: vec![command],
|
||||
command: split_command_string(&command),
|
||||
cwd,
|
||||
parsed_cmd: command_actions
|
||||
.into_iter()
|
||||
@@ -6391,7 +6396,7 @@ impl ChatWidget {
|
||||
call_id: id,
|
||||
process_id,
|
||||
turn_id: notification.turn_id,
|
||||
command: vec![command],
|
||||
command: split_command_string(&command),
|
||||
cwd,
|
||||
parsed_cmd: command_actions
|
||||
.into_iter()
|
||||
|
||||
+6
@@ -0,0 +1,6 @@
|
||||
---
|
||||
source: tui_app_server/src/chatwidget/tests.rs
|
||||
expression: blob
|
||||
---
|
||||
• You ran python3 -c 'print("Hello, world!")'
|
||||
└ Hello, world!
|
||||
@@ -26,6 +26,10 @@ use codex_app_server_protocol::CollabAgentState as AppServerCollabAgentState;
|
||||
use codex_app_server_protocol::CollabAgentStatus as AppServerCollabAgentStatus;
|
||||
use codex_app_server_protocol::CollabAgentTool as AppServerCollabAgentTool;
|
||||
use codex_app_server_protocol::CollabAgentToolCallStatus as AppServerCollabAgentToolCallStatus;
|
||||
use codex_app_server_protocol::CommandAction as AppServerCommandAction;
|
||||
use codex_app_server_protocol::CommandExecutionRequestApprovalParams as AppServerCommandExecutionRequestApprovalParams;
|
||||
use codex_app_server_protocol::CommandExecutionSource as AppServerCommandExecutionSource;
|
||||
use codex_app_server_protocol::CommandExecutionStatus as AppServerCommandExecutionStatus;
|
||||
use codex_app_server_protocol::ErrorNotification;
|
||||
use codex_app_server_protocol::FileUpdateChange;
|
||||
use codex_app_server_protocol::GuardianApprovalReview;
|
||||
@@ -3503,6 +3507,40 @@ async fn exec_approval_emits_proposed_command_and_decision_history() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn app_server_exec_approval_request_splits_shell_wrapped_command() {
|
||||
let script = r#"python3 -c 'print("Hello, world!")'"#;
|
||||
let request =
|
||||
exec_approval_request_from_params(AppServerCommandExecutionRequestApprovalParams {
|
||||
thread_id: "thread-1".to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
item_id: "item-1".to_string(),
|
||||
approval_id: Some("approval-1".to_string()),
|
||||
reason: None,
|
||||
network_approval_context: None,
|
||||
command: Some(
|
||||
shlex::try_join(["/bin/zsh", "-lc", script])
|
||||
.expect("round-trippable shell wrapper"),
|
||||
),
|
||||
cwd: Some(PathBuf::from("/tmp")),
|
||||
command_actions: None,
|
||||
additional_permissions: None,
|
||||
skill_metadata: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
proposed_network_policy_amendments: None,
|
||||
available_decisions: None,
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
request.command,
|
||||
vec![
|
||||
"/bin/zsh".to_string(),
|
||||
"-lc".to_string(),
|
||||
script.to_string(),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn exec_approval_uses_approval_id_when_present() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await;
|
||||
@@ -4650,6 +4688,69 @@ async fn live_app_server_file_change_item_started_preserves_changes() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn live_app_server_command_execution_strips_shell_wrapper() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await;
|
||||
let script = r#"python3 -c 'print("Hello, world!")'"#;
|
||||
let command =
|
||||
shlex::try_join(["/bin/zsh", "-lc", script]).expect("round-trippable shell wrapper");
|
||||
|
||||
chat.handle_server_notification(
|
||||
ServerNotification::ItemStarted(ItemStartedNotification {
|
||||
thread_id: "thread-1".to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
item: AppServerThreadItem::CommandExecution {
|
||||
id: "cmd-1".to_string(),
|
||||
command: command.clone(),
|
||||
cwd: PathBuf::from("/tmp"),
|
||||
process_id: None,
|
||||
source: AppServerCommandExecutionSource::UserShell,
|
||||
status: AppServerCommandExecutionStatus::InProgress,
|
||||
command_actions: vec![AppServerCommandAction::Unknown {
|
||||
command: script.to_string(),
|
||||
}],
|
||||
aggregated_output: None,
|
||||
exit_code: None,
|
||||
duration_ms: None,
|
||||
},
|
||||
}),
|
||||
None,
|
||||
);
|
||||
chat.handle_server_notification(
|
||||
ServerNotification::ItemCompleted(ItemCompletedNotification {
|
||||
thread_id: "thread-1".to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
item: AppServerThreadItem::CommandExecution {
|
||||
id: "cmd-1".to_string(),
|
||||
command,
|
||||
cwd: PathBuf::from("/tmp"),
|
||||
process_id: None,
|
||||
source: AppServerCommandExecutionSource::UserShell,
|
||||
status: AppServerCommandExecutionStatus::Completed,
|
||||
command_actions: vec![AppServerCommandAction::Unknown {
|
||||
command: script.to_string(),
|
||||
}],
|
||||
aggregated_output: Some("Hello, world!\n".to_string()),
|
||||
exit_code: Some(0),
|
||||
duration_ms: Some(5),
|
||||
},
|
||||
}),
|
||||
None,
|
||||
);
|
||||
|
||||
let cells = drain_insert_history(&mut rx);
|
||||
assert_eq!(
|
||||
cells.len(),
|
||||
1,
|
||||
"expected one completed command history cell"
|
||||
);
|
||||
let blob = lines_to_single_string(cells.first().expect("command cell"));
|
||||
assert_snapshot!(
|
||||
"live_app_server_command_execution_strips_shell_wrapper",
|
||||
blob
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn app_server_patch_changes_to_core_preserves_diffs() {
|
||||
let changes = app_server_patch_changes_to_core(vec![FileUpdateChange {
|
||||
|
||||
@@ -16,6 +16,22 @@ pub(crate) fn strip_bash_lc_and_escape(command: &[String]) -> String {
|
||||
escape_command(command)
|
||||
}
|
||||
|
||||
pub(crate) fn split_command_string(command: &str) -> Vec<String> {
|
||||
let Some(parts) = shlex::split(command) else {
|
||||
return vec![command.to_string()];
|
||||
};
|
||||
match shlex::try_join(parts.iter().map(String::as_str)) {
|
||||
Ok(round_trip)
|
||||
if round_trip == command
|
||||
|| (!command.contains(":\\")
|
||||
&& shlex::split(&round_trip).as_ref() == Some(&parts)) =>
|
||||
{
|
||||
parts
|
||||
}
|
||||
_ => vec![command.to_string()],
|
||||
}
|
||||
}
|
||||
|
||||
/// If `path` is absolute and inside $HOME, return the part *after* the home
|
||||
/// directory; otherwise, return the path as-is. Note if `path` is the homedir,
|
||||
/// this will return and empty path.
|
||||
@@ -67,4 +83,25 @@ mod tests {
|
||||
let cmdline = strip_bash_lc_and_escape(&args);
|
||||
assert_eq!(cmdline, "echo hello");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn split_command_string_round_trips_shell_wrappers() {
|
||||
let command =
|
||||
shlex::try_join(["/bin/zsh", "-lc", r#"python3 -c 'print("Hello, world!")'"#])
|
||||
.expect("round-trippable command");
|
||||
assert_eq!(
|
||||
split_command_string(&command),
|
||||
vec![
|
||||
"/bin/zsh".to_string(),
|
||||
"-lc".to_string(),
|
||||
r#"python3 -c 'print("Hello, world!")'"#.to_string(),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn split_command_string_preserves_non_roundtrippable_windows_commands() {
|
||||
let command = r#"C:\Program Files\Git\bin\bash.exe -lc "echo hi""#;
|
||||
assert_eq!(split_command_string(command), vec![command.to_string()]);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user