fix(tui): render network approval history by target (#22229)

## Why

Network approval prompts are rendered without a command string on the
app-server path. After the user approves one of those prompts, the TUI
history cell previously fell back to command-oriented copy and produced
malformed lines such as:

```text
You approved codex to run  every time this session
```

That hid the network target the user actually approved and left a
visibly broken transcript entry.

## What changed

- Preserve the approval subject as either a command or a network target
when recording TUI approval decisions.
- Render target-aware history copy for network approval outcomes:
  - approve once
  - approve for the current session
  - cancel
- Include the approval protocol and preserve the managed-proxy
`network-access` target when present, including non-default ports such
as `https://example.com:8443`.
- Fall back to formatting the network approval context as
`protocol://host` when no generated target command is available.
- Keep ordinary command approval history, Guardian approval history, and
persisted network-rule history behavior unchanged.
- Add focused regression coverage and snapshots for the three
network-history cases.

## How to Test

1. Start Codex in a flow that triggers a network approval prompt.
2. Approve network access only for the current conversation.
3. Confirm the transcript records the approved network target, for
example:
- `You approved codex network access to https://example.com:8443 every
time this session`
4. Trigger the prompt again and verify the one-time approval and cancel
paths also record target-specific history text instead of an empty
command gap.

Targeted automated coverage:
- `cargo test -p codex-tui network_exec_approval_history`

## Additional verification

- `cargo insta pending-snapshots`
- `git diff --check`
- `just fix -p codex-tui`
- `just argument-comment-lint`

## Known unrelated local test noise

A full `cargo test -p codex-tui` run still hits a pre-existing stack
overflow outside this change:
- `tests::fork_last_filters_latest_session_by_cwd_unless_show_all`
aborts with a stack overflow
This commit is contained in:
Felipe Coury
2026-05-14 14:33:54 -03:00
committed by GitHub
Unverified
parent 6ec8c4a6ec
commit 5a02962519
8 changed files with 449 additions and 71 deletions
@@ -47,6 +47,7 @@ use codex_app_server_protocol::FileSystemSandboxEntry;
use codex_app_server_protocol::FileSystemSpecialPath;
use codex_app_server_protocol::McpServerElicitationAction;
use codex_app_server_protocol::NetworkApprovalContext;
use codex_app_server_protocol::NetworkApprovalProtocol;
use codex_app_server_protocol::NetworkPolicyRuleAction;
use codex_app_server_protocol::RequestId;
use codex_features::Features;
@@ -354,8 +355,25 @@ impl ApprovalOverlay {
return;
};
if request.thread_label().is_none() {
let subject = match request {
ApprovalRequest::Exec {
network_approval_context: Some(network_approval_context),
..
} => history_cell::ApprovalDecisionSubject::NetworkAccess {
target: network_approval_target(network_approval_context, command),
},
_ => {
if let Some(target) = network_approval_command_target(command) {
history_cell::ApprovalDecisionSubject::NetworkAccess {
target: target.to_string(),
}
} else {
history_cell::ApprovalDecisionSubject::Command(command.to_vec())
}
}
};
let cell = history_cell::new_approval_decision_cell(
command.to_vec(),
subject,
command_decision_to_review_decision(&decision),
history_cell::ApprovalDecisionActor::User,
);
@@ -623,6 +641,35 @@ fn approval_footer_hint(
Line::from(spans)
}
fn network_approval_target(
network_approval_context: &NetworkApprovalContext,
command: &[String],
) -> String {
if let Some(target) = network_approval_command_target(command) {
return target.to_string();
}
let scheme = match network_approval_context.protocol {
NetworkApprovalProtocol::Http => "http",
NetworkApprovalProtocol::Https => "https",
NetworkApprovalProtocol::Socks5Tcp => "socks5-tcp",
NetworkApprovalProtocol::Socks5Udp => "socks5-udp",
};
format!("{scheme}://{}", network_approval_context.host)
}
fn network_approval_command_target(command: &[String]) -> Option<&str> {
match command {
[program, target] if program == "network-access" && !target.is_empty() => {
Some(target.as_str())
}
[command] => command
.strip_prefix("network-access ")
.filter(|target| !target.is_empty()),
_ => None,
}
}
fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
match request {
ApprovalRequest::Exec {
@@ -1102,6 +1149,21 @@ mod tests {
.join("\n")
}
fn render_history_cell_lines(
cell: &dyn crate::history_cell::HistoryCell,
width: u16,
) -> Vec<String> {
cell.display_lines(width)
.iter()
.map(|line| {
line.spans
.iter()
.map(|span| span.content.as_ref())
.collect::<String>()
})
.collect()
}
fn normalize_snapshot_paths(rendered: String) -> String {
[
(absolute_path("/tmp/readme.txt"), "/tmp/readme.txt"),
@@ -2111,7 +2173,7 @@ mod tests {
"git add tui/src/render/mod.rs tui/src/render/renderable.rs".into(),
];
let cell = history_cell::new_approval_decision_cell(
command,
history_cell::ApprovalDecisionSubject::Command(command),
ReviewDecision::Approved,
history_cell::ApprovalDecisionActor::User,
);
@@ -2134,6 +2196,73 @@ mod tests {
assert_eq!(rendered, expected);
}
#[test]
fn exec_history_cell_does_not_render_blank_action_for_empty_command() {
let approved = history_cell::new_approval_decision_cell(
history_cell::ApprovalDecisionSubject::Command(Vec::new()),
ReviewDecision::Approved,
history_cell::ApprovalDecisionActor::User,
);
assert_eq!(
render_history_cell_lines(approved.as_ref(), /*width*/ 80),
vec!["✔ You approved this request this time".to_string()]
);
let approved_for_session = history_cell::new_approval_decision_cell(
history_cell::ApprovalDecisionSubject::Command(Vec::new()),
ReviewDecision::ApprovedForSession,
history_cell::ApprovalDecisionActor::User,
);
assert_eq!(
render_history_cell_lines(approved_for_session.as_ref(), /*width*/ 80),
vec!["✔ You approved this request every time this session".to_string()]
);
}
#[test]
fn network_access_command_history_uses_target_without_structured_context() {
let (tx_raw, mut rx) = unbounded_channel::<AppEvent>();
let tx = AppEventSender::new(tx_raw);
let mut view = make_overlay(
ApprovalRequest::Exec {
thread_id: ThreadId::new(),
thread_label: None,
id: "test".into(),
command: vec![
"network-access".to_string(),
"https://example.com:8443".to_string(),
],
reason: None,
available_decisions: vec![
CommandExecutionApprovalDecision::Accept,
CommandExecutionApprovalDecision::Cancel,
],
network_approval_context: None,
additional_permissions: None,
},
tx,
Features::with_defaults(),
);
view.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE));
let mut decision = None;
while let Ok(event) = rx.try_recv() {
if let AppEvent::InsertHistoryCell(cell) = event {
decision = Some(cell);
break;
}
}
let decision = decision.expect("expected decision cell in history");
assert_eq!(
render_history_cell_lines(decision.as_ref(), /*width*/ 80),
vec![
"✔ You approved codex network access to https://example.com:8443 this time"
.to_string(),
]
);
}
#[test]
fn esc_cancels_mcp_elicitation() {
let (tx_raw, mut rx) = unbounded_channel::<AppEvent>();
@@ -145,6 +145,129 @@ fn app_server_exec_approval_request_preserves_permissions_context() {
);
}
#[tokio::test]
async fn network_exec_approval_history_describes_session_host_allowance() {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
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(),
started_at_ms: 0,
approval_id: Some("approval-1".to_string()),
reason: None,
network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext {
host: "example.com".to_string(),
protocol: codex_app_server_protocol::NetworkApprovalProtocol::Https,
}),
command: Some("network-access https://example.com:8443".to_string()),
cwd: None,
command_actions: None,
additional_permissions: None,
proposed_execpolicy_amendment: None,
proposed_network_policy_amendments: None,
available_decisions: Some(vec![
codex_app_server_protocol::CommandExecutionApprovalDecision::AcceptForSession,
codex_app_server_protocol::CommandExecutionApprovalDecision::Cancel,
]),
},
&test_path_buf("/tmp").abs(),
);
handle_exec_approval_request(&mut chat, "sub-network", request);
chat.handle_key_event(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::NONE));
let decision = drain_insert_history(&mut rx)
.pop()
.expect("expected decision cell in history");
assert_snapshot!(
"network_exec_approval_history_session_host_allowance",
lines_to_single_string(&decision)
);
}
#[tokio::test]
async fn network_exec_approval_history_describes_one_time_host_allowance() {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
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(),
started_at_ms: 0,
approval_id: Some("approval-1".to_string()),
reason: None,
network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext {
host: "example.com".to_string(),
protocol: codex_app_server_protocol::NetworkApprovalProtocol::Http,
}),
command: None,
cwd: None,
command_actions: None,
additional_permissions: None,
proposed_execpolicy_amendment: None,
proposed_network_policy_amendments: None,
available_decisions: Some(vec![
codex_app_server_protocol::CommandExecutionApprovalDecision::Accept,
codex_app_server_protocol::CommandExecutionApprovalDecision::Cancel,
]),
},
&test_path_buf("/tmp").abs(),
);
handle_exec_approval_request(&mut chat, "sub-network", request);
chat.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE));
let decision = drain_insert_history(&mut rx)
.pop()
.expect("expected decision cell in history");
assert_snapshot!(
"network_exec_approval_history_one_time_host_allowance",
lines_to_single_string(&decision)
);
}
#[tokio::test]
async fn network_exec_approval_history_describes_canceled_host_request() {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
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(),
started_at_ms: 0,
approval_id: Some("approval-1".to_string()),
reason: None,
network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext {
host: "example.com".to_string(),
protocol: codex_app_server_protocol::NetworkApprovalProtocol::Socks5Tcp,
}),
command: Some("network-access socks5-tcp://example.com:1080".to_string()),
cwd: None,
command_actions: None,
additional_permissions: None,
proposed_execpolicy_amendment: None,
proposed_network_policy_amendments: None,
available_decisions: Some(vec![
codex_app_server_protocol::CommandExecutionApprovalDecision::Accept,
codex_app_server_protocol::CommandExecutionApprovalDecision::Cancel,
]),
},
&test_path_buf("/tmp").abs(),
);
handle_exec_approval_request(&mut chat, "sub-network", request);
chat.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE));
let decision = drain_insert_history(&mut rx)
.pop()
.expect("expected decision cell in history");
assert_snapshot!(
"network_exec_approval_history_canceled_host_request",
lines_to_single_string(&decision)
);
}
#[test]
fn app_server_request_permissions_preserves_file_system_permissions() {
let read_path = AbsolutePathBuf::try_from(PathBuf::from(test_path_display("/tmp/read-only")))
@@ -0,0 +1,6 @@
---
source: tui/src/chatwidget/tests/approval_requests.rs
expression: lines_to_single_string(&decision)
---
✗ You canceled the request for codex network access to
socks5-tcp://example.com:1080
@@ -0,0 +1,5 @@
---
source: tui/src/chatwidget/tests/approval_requests.rs
expression: lines_to_single_string(&decision)
---
✔ You approved codex network access to http://example.com this time
@@ -0,0 +1,6 @@
---
source: tui/src/chatwidget/tests/approval_requests.rs
expression: lines_to_single_string(&decision)
---
✔ You approved codex network access to https://example.com:8443 every time this
session
+3 -3
View File
@@ -149,7 +149,7 @@ impl ChatWidget {
if ev.status == GuardianAssessmentStatus::Approved {
let cell = if let Some(command) = guardian_command(&ev.action) {
history_cell::new_approval_decision_cell(
command,
history_cell::ApprovalDecisionSubject::Command(command),
crate::history_cell::ReviewDecision::Approved,
history_cell::ApprovalDecisionActor::Guardian,
)
@@ -169,7 +169,7 @@ impl ChatWidget {
if ev.status == GuardianAssessmentStatus::TimedOut {
let cell = if let Some(command) = guardian_command(&ev.action) {
history_cell::new_approval_decision_cell(
command,
history_cell::ApprovalDecisionSubject::Command(command),
crate::history_cell::ReviewDecision::TimedOut,
history_cell::ApprovalDecisionActor::Guardian,
)
@@ -213,7 +213,7 @@ impl ChatWidget {
self.review.recent_auto_review_denials.push(ev.clone());
let cell = if let Some(command) = guardian_command(&ev.action) {
history_cell::new_approval_decision_cell(
command,
history_cell::ApprovalDecisionSubject::Command(command),
crate::history_cell::ReviewDecision::Denied,
history_cell::ApprovalDecisionActor::Guardian,
)
+174 -65
View File
@@ -1089,6 +1089,11 @@ fn exec_snippet(command: &[String]) -> String {
truncate_exec_snippet(&full_cmd)
}
fn non_empty_exec_snippet(command: &[String]) -> Option<String> {
let snippet = exec_snippet(command);
(!snippet.is_empty()).then_some(snippet)
}
#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) enum ReviewDecision {
Approved,
@@ -1104,8 +1109,14 @@ pub(crate) enum ReviewDecision {
Abort,
}
#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) enum ApprovalDecisionSubject {
Command(Vec<String>),
NetworkAccess { target: String },
}
pub fn new_approval_decision_cell(
command: Vec<String>,
subject: ApprovalDecisionSubject,
decision: ReviewDecision,
actor: ApprovalDecisionActor,
) -> Box<dyn HistoryCell> {
@@ -1113,19 +1124,37 @@ pub fn new_approval_decision_cell(
use codex_protocol::approvals::NetworkPolicyRuleAction;
let (symbol, summary): (Span<'static>, Vec<Span<'static>>) = match decision {
Approved => {
let snippet = Span::from(exec_snippet(&command)).dim();
(
Approved => match subject {
ApprovalDecisionSubject::Command(command) => {
let summary = if let Some(snippet) = non_empty_exec_snippet(&command) {
vec![
actor.subject().into(),
"approved".bold(),
" codex to run ".into(),
Span::from(snippet).dim(),
" this time".bold(),
]
} else {
vec![
actor.subject().into(),
"approved".bold(),
" this request".into(),
" this time".bold(),
]
};
("".green(), summary)
}
ApprovalDecisionSubject::NetworkAccess { target } => (
"".green(),
vec![
actor.subject().into(),
"approved".bold(),
" codex to run ".into(),
snippet,
" codex network access to ".into(),
Span::from(target).dim(),
" this time".bold(),
],
)
}
),
},
ApprovedExecpolicyAmendment {
proposed_execpolicy_amendment,
} => {
@@ -1140,84 +1169,164 @@ pub fn new_approval_decision_cell(
],
)
}
ApprovedForSession => {
let snippet = Span::from(exec_snippet(&command)).dim();
(
ApprovedForSession => match subject {
ApprovalDecisionSubject::Command(command) => {
let summary = if let Some(snippet) = non_empty_exec_snippet(&command) {
vec![
actor.subject().into(),
"approved".bold(),
" codex to run ".into(),
Span::from(snippet).dim(),
" every time this session".bold(),
]
} else {
vec![
actor.subject().into(),
"approved".bold(),
" this request".into(),
" every time this session".bold(),
]
};
("".green(), summary)
}
ApprovalDecisionSubject::NetworkAccess { target } => (
"".green(),
vec![
actor.subject().into(),
"approved".bold(),
" codex to run ".into(),
snippet,
" every time this session".bold(),
],
)
}
NetworkPolicyAmendment {
network_policy_amendment,
} => match network_policy_amendment.action {
NetworkPolicyRuleAction::Allow => (
"".green(),
vec![
actor.subject().into(),
"persisted".bold(),
" Codex network access to ".into(),
Span::from(network_policy_amendment.host).dim(),
],
),
NetworkPolicyRuleAction::Deny => (
"".red(),
vec![
actor.subject().into(),
"denied".bold(),
" codex network access to ".into(),
Span::from(network_policy_amendment.host).dim(),
" and saved that rule".into(),
Span::from(target).dim(),
" every time this session".bold(),
],
),
},
Denied => {
let snippet = Span::from(exec_snippet(&command)).dim();
let summary = match actor {
ApprovalDecisionActor::User => vec![
NetworkPolicyAmendment {
network_policy_amendment,
} => {
let target = match subject {
ApprovalDecisionSubject::NetworkAccess { target } => target,
ApprovalDecisionSubject::Command(_) => network_policy_amendment.host,
};
match network_policy_amendment.action {
NetworkPolicyRuleAction::Allow => (
"".green(),
vec![
actor.subject().into(),
"persisted".bold(),
" Codex network access to ".into(),
Span::from(target).dim(),
],
),
NetworkPolicyRuleAction::Deny => (
"".red(),
vec![
actor.subject().into(),
"denied".bold(),
" codex network access to ".into(),
Span::from(target).dim(),
" and saved that rule".into(),
],
),
}
}
Denied => match subject {
ApprovalDecisionSubject::Command(command) => {
let summary = if let Some(snippet) = non_empty_exec_snippet(&command) {
let snippet = Span::from(snippet).dim();
match actor {
ApprovalDecisionActor::User => vec![
actor.subject().into(),
"did not approve".bold(),
" codex to run ".into(),
snippet,
],
ApprovalDecisionActor::Guardian => vec![
"Request ".into(),
"denied".bold(),
" for codex to run ".into(),
snippet,
],
}
} else {
match actor {
ApprovalDecisionActor::User => vec![
actor.subject().into(),
"did not approve".bold(),
" this request".into(),
],
ApprovalDecisionActor::Guardian => {
vec!["Request ".into(), "denied".bold()]
}
}
};
("".red(), summary)
}
ApprovalDecisionSubject::NetworkAccess { target } => (
"".red(),
vec![
actor.subject().into(),
"did not approve".bold(),
" codex to run ".into(),
snippet,
" codex network access to ".into(),
Span::from(target).dim(),
],
ApprovalDecisionActor::Guardian => vec![
"Request ".into(),
"denied".bold(),
" for codex to run ".into(),
snippet,
],
};
("".red(), summary)
}
TimedOut => {
let snippet = Span::from(exec_snippet(&command)).dim();
(
),
},
TimedOut => match subject {
ApprovalDecisionSubject::Command(command) => {
let summary = if let Some(snippet) = non_empty_exec_snippet(&command) {
vec![
"Review ".into(),
"timed out".bold(),
" before codex could run ".into(),
Span::from(snippet).dim(),
]
} else {
vec![
"Review ".into(),
"timed out".bold(),
" before this request could be approved".into(),
]
};
("".red(), summary)
}
ApprovalDecisionSubject::NetworkAccess { target } => (
"".red(),
vec![
"Review ".into(),
"timed out".bold(),
" before codex could run ".into(),
snippet,
" before codex could access ".into(),
Span::from(target).dim(),
],
)
}
Abort => {
let snippet = Span::from(exec_snippet(&command)).dim();
(
),
},
Abort => match subject {
ApprovalDecisionSubject::Command(command) => {
let summary = if let Some(snippet) = non_empty_exec_snippet(&command) {
vec![
actor.subject().into(),
"canceled".bold(),
" the request to run ".into(),
Span::from(snippet).dim(),
]
} else {
vec![
actor.subject().into(),
"canceled".bold(),
" this request".into(),
]
};
("".red(), summary)
}
ApprovalDecisionSubject::NetworkAccess { target } => (
"".red(),
vec![
actor.subject().into(),
"canceled".bold(),
" the request to run ".into(),
snippet,
" the request for codex network access to ".into(),
Span::from(target).dim(),
],
)
}
),
},
};
Box::new(PrefixedWrappedHistoryCell::new(
+1 -1
View File
@@ -1133,7 +1133,7 @@ mod tests {
cells.push(apply_begin_cell);
let apply_end_cell: Arc<dyn HistoryCell> = history_cell::new_approval_decision_cell(
vec!["ls".into()],
history_cell::ApprovalDecisionSubject::Command(vec!["ls".into()]),
ReviewDecision::Approved,
history_cell::ApprovalDecisionActor::User,
)