Compare commits

...

3 Commits

Author SHA1 Message Date
Ahmed Ibrahim
249648d7b4 core: stabilize unicode shell command test 2026-03-09 00:54:10 -07:00
Ahmed Ibrahim
2bc3e52a91 Stabilize app list update ordering test (#14052)
## Summary
- make
`list_apps_waits_for_accessible_data_before_emitting_directory_updates`
accept the two valid notification paths the server can emit
- keep rejecting the real bug this test is meant to catch: a
directory-only `app/list/updated` notification before accessible app
data is available

## Why this fixes the flake
The old test used a fixed `150ms` silence window and assumed the first
notification after that window had to be the fully merged final update.
In CI, scheduling occasionally lets accessible app data arrive before
directory data, so the first valid notification can be an
accessible-only interim update. That made the test fail even though the
server behavior was correct.

This change makes the test deterministic by reading notifications until
the final merged payload arrives. Any interim update is only accepted if
it contains accessible apps only; if the server ever emits inaccessible
directory data before accessible data is ready, the test still fails
immediately.

## Change type
- test-only; no production app-list logic changes
2026-03-09 00:16:13 -07:00
Dylan Hurd
06f82c123c feat(tui) render request_permissions calls (#14004)
## Summary
Adds support for tui rendering of request_permission calls

<img width="724" height="245" alt="Screenshot 2026-03-08 at 9 04 07 PM"
src="https://github.com/user-attachments/assets/e1997825-a496-4bfb-bbda-43d0006460a5"
/>


## Testing
- [x] Added snapshot test
2026-03-09 04:24:04 +00:00
10 changed files with 317 additions and 16 deletions

View File

@@ -501,16 +501,6 @@ async fn list_apps_waits_for_accessible_data_before_emitting_directory_updates()
})
.await?;
let maybe_update = timeout(
Duration::from_millis(150),
read_app_list_updated_notification(&mut mcp),
)
.await;
assert!(
maybe_update.is_err(),
"unexpected directory-only app/list update before accessible apps loaded"
);
let expected = vec![
AppInfo {
id: "beta".to_string(),
@@ -544,8 +534,17 @@ async fn list_apps_waits_for_accessible_data_before_emitting_directory_updates()
},
];
let update = read_app_list_updated_notification(&mut mcp).await?;
assert_eq!(update.data, expected);
loop {
let update = read_app_list_updated_notification(&mut mcp).await?;
if update.data == expected {
break;
}
assert!(
!update.data.is_empty() && update.data.iter().all(|connector| connector.is_accessible),
"unexpected directory-only app/list update before accessible apps loaded"
);
}
let response: JSONRPCResponse = timeout(
DEFAULT_TIMEOUT,

View File

@@ -266,7 +266,7 @@ async fn unicode_output(login: bool) -> anyhow::Result<()> {
mount_shell_responses_with_timeout(
&harness,
call_id,
"git -c alias.say='!printf \"%s\" \"naïve_café\"' say",
"echo 'naïve_café'",
Some(login),
MEDIUM_TIMEOUT,
)

View File

@@ -1171,6 +1171,15 @@ impl App {
))
}
}
EventMsg::RequestPermissions(ev) => Some(ThreadInteractiveRequest::Approval(
ApprovalRequest::Permissions {
thread_id,
thread_label,
call_id: ev.call_id.clone(),
reason: ev.reason.clone(),
permissions: ev.permissions.clone(),
},
)),
_ => None,
}
}
@@ -3237,6 +3246,30 @@ impl App {
"E X E C".to_string(),
));
}
ApprovalRequest::Permissions {
permissions,
reason,
..
} => {
let _ = tui.enter_alt_screen();
let mut lines = Vec::new();
if let Some(reason) = reason {
lines.push(Line::from(vec!["Reason: ".into(), reason.italic()]));
lines.push(Line::from(""));
}
if let Some(rule_line) =
crate::bottom_pane::format_additional_permissions_rule(&permissions)
{
lines.push(Line::from(vec![
"Permission rule: ".into(),
rule_line.cyan(),
]));
}
self.overlay = Some(Overlay::new_static_with_renderables(
vec![Box::new(Paragraph::new(lines).wrap(Wrap { trim: false }))],
"P E R M I S S I O N S".to_string(),
));
}
ApprovalRequest::McpElicitation {
server_name,
message,

View File

@@ -39,6 +39,8 @@ pub(super) struct PendingInteractiveReplayState {
patch_approval_call_ids: HashSet<String>,
patch_approval_call_ids_by_turn_id: HashMap<String, Vec<String>>,
elicitation_requests: HashSet<ElicitationRequestKey>,
request_permissions_call_ids: HashSet<String>,
request_permissions_call_ids_by_turn_id: HashMap<String, Vec<String>>,
request_user_input_call_ids: HashSet<String>,
request_user_input_call_ids_by_turn_id: HashMap<String, Vec<String>>,
}
@@ -50,6 +52,7 @@ impl PendingInteractiveReplayState {
EventMsg::ExecApprovalRequest(_)
| EventMsg::ApplyPatchApprovalRequest(_)
| EventMsg::ElicitationRequest(_)
| EventMsg::RequestPermissions(_)
| EventMsg::ExecCommandBegin(_)
| EventMsg::PatchApplyBegin(_)
| EventMsg::TurnComplete(_)
@@ -64,6 +67,7 @@ impl PendingInteractiveReplayState {
Op::ExecApproval { .. }
| Op::PatchApproval { .. }
| Op::ResolveElicitation { .. }
| Op::RequestPermissionsResponse { .. }
| Op::UserInputAnswer { .. }
| Op::Shutdown
)
@@ -99,6 +103,13 @@ impl PendingInteractiveReplayState {
request_id.clone(),
));
}
Op::RequestPermissionsResponse { id, .. } => {
self.request_permissions_call_ids.remove(id);
Self::remove_call_id_from_turn_map(
&mut self.request_permissions_call_ids_by_turn_id,
id,
);
}
// `Op::UserInputAnswer` identifies the turn, not the prompt call_id. The UI
// answers queued prompts for the same turn in FIFO order, so remove the oldest
// queued call_id for that turn.
@@ -166,17 +177,26 @@ impl PendingInteractiveReplayState {
.or_default()
.push(ev.call_id.clone());
}
EventMsg::RequestPermissions(ev) => {
self.request_permissions_call_ids.insert(ev.call_id.clone());
self.request_permissions_call_ids_by_turn_id
.entry(ev.turn_id.clone())
.or_default()
.push(ev.call_id.clone());
}
// A turn ending (normally or aborted/replaced) invalidates any unresolved
// turn-scoped approvals and request_user_input prompts from that turn.
// turn-scoped approvals, permission prompts, and request_user_input prompts.
EventMsg::TurnComplete(ev) => {
self.clear_exec_approval_turn(&ev.turn_id);
self.clear_patch_approval_turn(&ev.turn_id);
self.clear_request_permissions_turn(&ev.turn_id);
self.clear_request_user_input_turn(&ev.turn_id);
}
EventMsg::TurnAborted(ev) => {
if let Some(turn_id) = &ev.turn_id {
self.clear_exec_approval_turn(turn_id);
self.clear_patch_approval_turn(turn_id);
self.clear_request_permissions_turn(turn_id);
self.clear_request_user_input_turn(turn_id);
}
}
@@ -228,6 +248,23 @@ impl PendingInteractiveReplayState {
.remove(&ev.turn_id);
}
}
EventMsg::RequestPermissions(ev) => {
self.request_permissions_call_ids.remove(&ev.call_id);
let mut remove_turn_entry = false;
if let Some(call_ids) = self
.request_permissions_call_ids_by_turn_id
.get_mut(&ev.turn_id)
{
call_ids.retain(|call_id| call_id != &ev.call_id);
if call_ids.is_empty() {
remove_turn_entry = true;
}
}
if remove_turn_entry {
self.request_permissions_call_ids_by_turn_id
.remove(&ev.turn_id);
}
}
_ => {}
}
}
@@ -250,6 +287,9 @@ impl PendingInteractiveReplayState {
EventMsg::RequestUserInput(ev) => {
self.request_user_input_call_ids.contains(&ev.call_id)
}
EventMsg::RequestPermissions(ev) => {
self.request_permissions_call_ids.contains(&ev.call_id)
}
_ => true,
}
}
@@ -258,6 +298,7 @@ impl PendingInteractiveReplayState {
!self.exec_approval_call_ids.is_empty()
|| !self.patch_approval_call_ids.is_empty()
|| !self.elicitation_requests.is_empty()
|| !self.request_permissions_call_ids.is_empty()
}
fn clear_request_user_input_turn(&mut self, turn_id: &str) {
@@ -268,6 +309,14 @@ impl PendingInteractiveReplayState {
}
}
fn clear_request_permissions_turn(&mut self, turn_id: &str) {
if let Some(call_ids) = self.request_permissions_call_ids_by_turn_id.remove(turn_id) {
for call_id in call_ids {
self.request_permissions_call_ids.remove(&call_id);
}
}
}
fn clear_exec_approval_turn(&mut self, turn_id: &str) {
if let Some(call_ids) = self.exec_approval_call_ids_by_turn_id.remove(turn_id) {
for call_id in call_ids {
@@ -317,6 +366,8 @@ impl PendingInteractiveReplayState {
self.patch_approval_call_ids.clear();
self.patch_approval_call_ids_by_turn_id.clear();
self.elicitation_requests.clear();
self.request_permissions_call_ids.clear();
self.request_permissions_call_ids_by_turn_id.clear();
self.request_user_input_call_ids.clear();
self.request_user_input_call_ids_by_turn_id.clear();
}

View File

@@ -53,6 +53,13 @@ pub(crate) enum ApprovalRequest {
network_approval_context: Option<NetworkApprovalContext>,
additional_permissions: Option<PermissionProfile>,
},
Permissions {
thread_id: ThreadId,
thread_label: Option<String>,
call_id: String,
reason: Option<String>,
permissions: PermissionProfile,
},
ApplyPatch {
thread_id: ThreadId,
thread_label: Option<String>,
@@ -74,6 +81,7 @@ impl ApprovalRequest {
fn thread_id(&self) -> ThreadId {
match self {
ApprovalRequest::Exec { thread_id, .. }
| ApprovalRequest::Permissions { thread_id, .. }
| ApprovalRequest::ApplyPatch { thread_id, .. }
| ApprovalRequest::McpElicitation { thread_id, .. } => *thread_id,
}
@@ -82,6 +90,7 @@ impl ApprovalRequest {
fn thread_label(&self) -> Option<&str> {
match self {
ApprovalRequest::Exec { thread_label, .. }
| ApprovalRequest::Permissions { thread_label, .. }
| ApprovalRequest::ApplyPatch { thread_label, .. }
| ApprovalRequest::McpElicitation { thread_label, .. } => thread_label.as_deref(),
}
@@ -156,6 +165,10 @@ impl ApprovalOverlay {
},
),
),
ApprovalRequest::Permissions { .. } => (
permissions_options(),
"Would you like to grant these permissions?".to_string(),
),
ApprovalRequest::ApplyPatch { .. } => (
patch_options(),
"Would you like to make the following edits?".to_string(),
@@ -206,6 +219,14 @@ impl ApprovalOverlay {
(ApprovalRequest::Exec { id, command, .. }, ApprovalDecision::Review(decision)) => {
self.handle_exec_decision(id, command, decision.clone());
}
(
ApprovalRequest::Permissions {
call_id,
permissions,
..
},
ApprovalDecision::Review(decision),
) => self.handle_permissions_decision(call_id, permissions, decision.clone()),
(ApprovalRequest::ApplyPatch { id, .. }, ApprovalDecision::Review(decision)) => {
self.handle_patch_decision(id, decision.clone());
}
@@ -246,6 +267,43 @@ impl ApprovalOverlay {
});
}
fn handle_permissions_decision(
&self,
call_id: &str,
permissions: &PermissionProfile,
decision: ReviewDecision,
) {
let Some(request) = self.current_request.as_ref() else {
return;
};
let granted_permissions = match decision {
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => permissions.clone(),
ReviewDecision::Denied | ReviewDecision::Abort => Default::default(),
ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::NetworkPolicyAmendment { .. } => Default::default(),
};
if request.thread_label().is_none() {
let message = if granted_permissions.is_empty() {
"You did not grant additional permissions"
} else {
"You granted additional permissions"
};
self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new(
crate::history_cell::PlainHistoryCell::new(vec![message.into()]),
)));
}
let thread_id = request.thread_id();
self.app_event_tx.send(AppEvent::SubmitThreadOp {
thread_id,
op: Op::RequestPermissionsResponse {
id: call_id.to_string(),
response: codex_protocol::request_permissions::RequestPermissionsResponse {
permissions: granted_permissions,
},
},
});
}
fn handle_patch_decision(&self, id: &str, decision: ReviewDecision) {
let Some(thread_id) = self
.current_request
@@ -367,6 +425,13 @@ impl BottomPaneView for ApprovalOverlay {
ApprovalRequest::Exec { id, command, .. } => {
self.handle_exec_decision(id, command, ReviewDecision::Abort);
}
ApprovalRequest::Permissions {
call_id,
permissions,
..
} => {
self.handle_permissions_decision(call_id, permissions, ReviewDecision::Abort);
}
ApprovalRequest::ApplyPatch { id, .. } => {
self.handle_patch_decision(id, ReviewDecision::Abort);
}
@@ -474,6 +539,32 @@ fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
}
Box::new(Paragraph::new(header).wrap(Wrap { trim: false }))
}
ApprovalRequest::Permissions {
thread_label,
reason,
permissions,
..
} => {
let mut header: Vec<Line<'static>> = Vec::new();
if let Some(thread_label) = thread_label {
header.push(Line::from(vec![
"Thread: ".into(),
thread_label.clone().bold(),
]));
header.push(Line::from(""));
}
if let Some(reason) = reason {
header.push(Line::from(vec!["Reason: ".into(), reason.clone().italic()]));
header.push(Line::from(""));
}
if let Some(rule_line) = format_additional_permissions_rule(permissions) {
header.push(Line::from(vec![
"Permission rule: ".into(),
rule_line.cyan(),
]));
}
Box::new(Paragraph::new(header).wrap(Wrap { trim: false }))
}
ApprovalRequest::ApplyPatch {
thread_label,
reason,
@@ -641,7 +732,7 @@ fn exec_options(
.collect()
}
fn format_additional_permissions_rule(
pub(crate) fn format_additional_permissions_rule(
additional_permissions: &PermissionProfile,
) -> Option<String> {
let mut parts = Vec::new();
@@ -732,6 +823,23 @@ fn patch_options() -> Vec<ApprovalOption> {
]
}
fn permissions_options() -> Vec<ApprovalOption> {
vec![
ApprovalOption {
label: "Yes, grant these permissions".to_string(),
decision: ApprovalDecision::Review(ReviewDecision::Approved),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
},
ApprovalOption {
label: "No, continue without permissions".to_string(),
decision: ApprovalDecision::Review(ReviewDecision::Denied),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
},
]
}
fn elicitation_options() -> Vec<ApprovalOption> {
vec![
ApprovalOption {
@@ -816,6 +924,25 @@ mod tests {
}
}
fn make_permissions_request() -> ApprovalRequest {
ApprovalRequest::Permissions {
thread_id: ThreadId::new(),
thread_label: None,
call_id: "test".to_string(),
reason: Some("need workspace access".to_string()),
permissions: PermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
file_system: Some(FileSystemPermissions {
read: Some(vec![absolute_path("/tmp/readme.txt")]),
write: Some(vec![absolute_path("/tmp/out.txt")]),
}),
..Default::default()
},
}
}
#[test]
fn ctrl_c_aborts_and_clears_queue() {
let (tx, _rx) = unbounded_channel::<AppEvent>();
@@ -1107,6 +1234,21 @@ mod tests {
);
}
#[test]
fn permissions_options_use_expected_labels() {
let labels: Vec<String> = permissions_options()
.into_iter()
.map(|option| option.label)
.collect();
assert_eq!(
labels,
vec![
"Yes, grant these permissions".to_string(),
"No, continue without permissions".to_string(),
]
);
}
#[test]
fn additional_permissions_prompt_shows_permission_rule_line() {
let (tx, _rx) = unbounded_channel::<AppEvent>();
@@ -1186,6 +1328,17 @@ mod tests {
);
}
#[test]
fn permissions_prompt_snapshot() {
let (tx, _rx) = unbounded_channel::<AppEvent>();
let tx = AppEventSender::new(tx);
let view = ApprovalOverlay::new(make_permissions_request(), tx, Features::with_defaults());
assert_snapshot!(
"approval_overlay_permissions_prompt",
normalize_snapshot_paths(render_overlay_lines(&view, 120))
);
}
#[test]
fn additional_permissions_macos_prompt_snapshot() {
let (tx, _rx) = unbounded_channel::<AppEvent>();

View File

@@ -51,6 +51,7 @@ pub(crate) use app_link_view::AppLinkView;
pub(crate) use app_link_view::AppLinkViewParams;
pub(crate) use approval_overlay::ApprovalOverlay;
pub(crate) use approval_overlay::ApprovalRequest;
pub(crate) use approval_overlay::format_additional_permissions_rule;
pub(crate) use mcp_server_elicitation::McpServerElicitationFormRequest;
pub(crate) use mcp_server_elicitation::McpServerElicitationOverlay;
pub(crate) use request_user_input::RequestUserInputOverlay;

View File

@@ -0,0 +1,15 @@
---
source: tui/src/bottom_pane/approval_overlay.rs
expression: "normalize_snapshot_paths(render_overlay_lines(&view, 120))"
---
Would you like to grant these permissions?
Reason: need workspace access
Permission rule: network; read `/tmp/readme.txt`; write `/tmp/out.txt`
1. Yes, grant these permissions (y)
2. No, continue without permissions (n)
Press enter to confirm or esc to cancel

View File

@@ -142,6 +142,7 @@ use codex_protocol::protocol::ViewImageToolCallEvent;
use codex_protocol::protocol::WarningEvent;
use codex_protocol::protocol::WebSearchBeginEvent;
use codex_protocol::protocol::WebSearchEndEvent;
use codex_protocol::request_permissions::RequestPermissionsEvent;
use codex_protocol::request_user_input::RequestUserInputEvent;
use codex_protocol::user_input::TextElement;
use codex_protocol::user_input::UserInput;
@@ -2233,6 +2234,14 @@ impl ChatWidget {
);
}
fn on_request_permissions(&mut self, ev: RequestPermissionsEvent) {
let ev2 = ev.clone();
self.defer_or_handle(
|q| q.push_request_permissions(ev),
|s| s.handle_request_permissions_now(ev2),
);
}
fn on_exec_command_begin(&mut self, ev: ExecCommandBeginEvent) {
self.flush_answer_stream_with_separator();
if is_unified_exec_source(ev.source) {
@@ -2968,6 +2977,20 @@ impl ChatWidget {
self.request_redraw();
}
pub(crate) fn handle_request_permissions_now(&mut self, ev: RequestPermissionsEvent) {
self.flush_answer_stream_with_separator();
let request = ApprovalRequest::Permissions {
thread_id: self.thread_id.unwrap_or_default(),
thread_label: None,
call_id: ev.call_id,
reason: ev.reason,
permissions: ev.permissions,
};
self.bottom_pane
.push_approval_request(request, &self.config.features);
self.request_redraw();
}
pub(crate) fn handle_exec_begin_now(&mut self, ev: ExecCommandBeginEvent) {
// Ensure the status indicator is visible while the command runs.
self.bottom_pane.ensure_status_indicator();
@@ -4867,7 +4890,9 @@ impl ChatWidget {
EventMsg::RequestUserInput(ev) => {
self.on_request_user_input(ev);
}
EventMsg::RequestPermissions(_) => {}
EventMsg::RequestPermissions(ev) => {
self.on_request_permissions(ev);
}
EventMsg::ExecCommandBegin(ev) => self.on_exec_command_begin(ev),
EventMsg::TerminalInteraction(delta) => self.on_terminal_interaction(delta),
EventMsg::ExecCommandOutputDelta(delta) => self.on_exec_command_output_delta(delta),

View File

@@ -8,6 +8,7 @@ use codex_protocol::protocol::ExecCommandEndEvent;
use codex_protocol::protocol::McpToolCallBeginEvent;
use codex_protocol::protocol::McpToolCallEndEvent;
use codex_protocol::protocol::PatchApplyEndEvent;
use codex_protocol::request_permissions::RequestPermissionsEvent;
use codex_protocol::request_user_input::RequestUserInputEvent;
use super::ChatWidget;
@@ -17,6 +18,7 @@ pub(crate) enum QueuedInterrupt {
ExecApproval(ExecApprovalRequestEvent),
ApplyPatchApproval(ApplyPatchApprovalRequestEvent),
Elicitation(ElicitationRequestEvent),
RequestPermissions(RequestPermissionsEvent),
RequestUserInput(RequestUserInputEvent),
ExecBegin(ExecCommandBeginEvent),
ExecEnd(ExecCommandEndEvent),
@@ -55,6 +57,11 @@ impl InterruptManager {
self.queue.push_back(QueuedInterrupt::Elicitation(ev));
}
pub(crate) fn push_request_permissions(&mut self, ev: RequestPermissionsEvent) {
self.queue
.push_back(QueuedInterrupt::RequestPermissions(ev));
}
pub(crate) fn push_user_input(&mut self, ev: RequestUserInputEvent) {
self.queue.push_back(QueuedInterrupt::RequestUserInput(ev));
}
@@ -85,6 +92,7 @@ impl InterruptManager {
QueuedInterrupt::ExecApproval(ev) => chat.handle_exec_approval_now(ev),
QueuedInterrupt::ApplyPatchApproval(ev) => chat.handle_apply_patch_approval_now(ev),
QueuedInterrupt::Elicitation(ev) => chat.handle_elicitation_request_now(ev),
QueuedInterrupt::RequestPermissions(ev) => chat.handle_request_permissions_now(ev),
QueuedInterrupt::RequestUserInput(ev) => chat.handle_request_user_input_now(ev),
QueuedInterrupt::ExecBegin(ev) => chat.handle_exec_begin_now(ev),
QueuedInterrupt::ExecEnd(ev) => chat.handle_exec_end_now(ev),

View File

@@ -47,6 +47,7 @@ use codex_protocol::items::PlanItem;
use codex_protocol::items::TurnItem;
use codex_protocol::items::UserMessageItem;
use codex_protocol::models::MessagePhase;
use codex_protocol::models::PermissionProfile;
use codex_protocol::openai_models::ModelPreset;
use codex_protocol::openai_models::ReasoningEffortPreset;
use codex_protocol::openai_models::default_input_modalities;
@@ -98,6 +99,7 @@ use codex_protocol::protocol::UndoCompletedEvent;
use codex_protocol::protocol::UndoStartedEvent;
use codex_protocol::protocol::ViewImageToolCallEvent;
use codex_protocol::protocol::WarningEvent;
use codex_protocol::request_permissions::RequestPermissionsEvent;
use codex_protocol::request_user_input::RequestUserInputEvent;
use codex_protocol::request_user_input::RequestUserInputQuestion;
use codex_protocol::request_user_input::RequestUserInputQuestionOption;
@@ -2746,6 +2748,20 @@ async fn handle_request_user_input_sets_pending_notification() {
);
}
#[tokio::test]
async fn handle_request_permissions_opens_approval_modal() {
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(Some("gpt-5.1-codex-max")).await;
chat.handle_request_permissions_now(RequestPermissionsEvent {
call_id: "call-1".to_string(),
turn_id: "turn-1".to_string(),
reason: Some("need workspace access".to_string()),
permissions: PermissionProfile::default(),
});
assert!(chat.bottom_pane.has_active_view());
}
#[tokio::test]
async fn plan_reasoning_scope_popup_mentions_selected_reasoning() {
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(Some("gpt-5.1-codex-max")).await;