openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-03 19:53:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

Preserve repeated approval client integrity

Browse Source
This commit is contained in:
Chris Bookholt
2026-07-02 20:09:58 -07:00
parent 5e50079976
commit 9091ab403d
19 changed files with 433 additions and 365 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json
Generated
+1 -1
View File
@@ -521,7 +521,7 @@
},
"properties": {
"approvalId": {
"description": "Unique identifier for this specific approval callback.\n\nFor a command item with only one approval callback, this is absent.\n\nWhen multiple callbacks can belong to one parent `itemId` (for example, a zsh-exec-bridge subcommand or an unsandboxed retry), `approvalId` is a distinct opaque callback id (a UUID) used to disambiguate routing.",
"description": "Opaque identifier for this specific approval callback, when present.\n\nUse the JSON-RPC request ID for the response. When tracking callback UI or state outside that envelope, use this instead of `itemId` when it is present. Absence is a legacy/cacheable compatibility path.",
"type": [
"string",
"null"
codex-rs/app-server-protocol/schema/json/ServerRequest.json
Generated
+1 -1
View File
@@ -345,7 +345,7 @@
"CommandExecutionRequestApprovalParams": {
"properties": {
"approvalId": {
"description": "Unique identifier for this specific approval callback.\n\nFor a command item with only one approval callback, this is absent.\n\nWhen multiple callbacks can belong to one parent `itemId` (for example, a zsh-exec-bridge subcommand or an unsandboxed retry), `approvalId` is a distinct opaque callback id (a UUID) used to disambiguate routing.",
"description": "Opaque identifier for this specific approval callback, when present.\n\nUse the JSON-RPC request ID for the response. When tracking callback UI or state outside that envelope, use this instead of `itemId` when it is present. Absence is a legacy/cacheable compatibility path.",
"type": [
"string",
"null"
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json
Generated
+1 -1
View File
@@ -2375,7 +2375,7 @@
"$schema": "http://json-schema.org/draft-07/schema#",
"properties": {
"approvalId": {
"description": "Unique identifier for this specific approval callback.\n\nFor a command item with only one approval callback, this is absent.\n\nWhen multiple callbacks can belong to one parent `itemId` (for example, a zsh-exec-bridge subcommand or an unsandboxed retry), `approvalId` is a distinct opaque callback id (a UUID) used to disambiguate routing.",
"description": "Opaque identifier for this specific approval callback, when present.\n\nUse the JSON-RPC request ID for the response. When tracking callback UI or state outside that envelope, use this instead of `itemId` when it is present. Absence is a legacy/cacheable compatibility path.",
"type": [
"string",
"null"
codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts
Generated
+4 -6
View File
@@ -11,13 +11,11 @@ export type CommandExecutionRequestApprovalParams = {threadId: string, turnId: s
* Unix timestamp (in milliseconds) when this approval request started.
*/
startedAtMs: number, /**
* Unique identifier for this specific approval callback.
* Opaque identifier for this specific approval callback, when present.
*
* For a command item with only one approval callback, this is absent.
*
* When multiple callbacks can belong to one parent `itemId` (for example,
* a zsh-exec-bridge subcommand or an unsandboxed retry), `approvalId` is a
* distinct opaque callback id (a UUID) used to disambiguate routing.
* Use the JSON-RPC request ID for the response. When tracking callback UI
* or state outside that envelope, use this instead of `itemId` when it is
* present. Absence is a legacy/cacheable compatibility path.
*/
approvalId?: string | null, /**
* Environment in which the command will run.
codex-rs/app-server-protocol/src/protocol/v2/item.rs
+4 -6
View File
@@ -1444,13 +1444,11 @@ pub struct CommandExecutionRequestApprovalParams {
/// Unix timestamp (in milliseconds) when this approval request started.
#[ts(type = "number")]
pub started_at_ms: i64,
/// Unique identifier for this specific approval callback.
/// Opaque identifier for this specific approval callback, when present.
///
/// For a command item with only one approval callback, this is absent.
///
/// When multiple callbacks can belong to one parent `itemId` (for example,
/// a zsh-exec-bridge subcommand or an unsandboxed retry), `approvalId` is a
/// distinct opaque callback id (a UUID) used to disambiguate routing.
/// Use the JSON-RPC request ID for the response. When tracking callback UI
/// or state outside that envelope, use this instead of `itemId` when it is
/// present. Absence is a legacy/cacheable compatibility path.
#[serde(default, skip_serializing_if = "Option::is_none")]
#[ts(optional = nullable)]
pub approval_id: Option<String>,
codex-rs/app-server/README.md
+1 -1
View File
@@ -1453,7 +1453,7 @@ Certain actions (shell commands or modifying files) may require explicit user ap
Order of messages:
1. `item/started` — shows the pending `commandExecution` item with `command`, `cwd`, and other fields so you can render the proposed action.
2. `item/commandExecution/requestApproval` (request) — carries the same `itemId`, `threadId`, `turnId`, the nullable `environmentId` where the command will run, optionally `approvalId`, and `reason`. `itemId` remains the stable command-item identity. `approvalId` is a distinct opaque callback identity when one command can prompt more than once, such as an execve/subcommand callback or an unsandboxed retry; it is unique for each such callback and must not be rendered as a separate command item. A follow-up callback may reuse an active item without another `item/started`; if the prior attempt already completed, the server can restart the same `itemId` before requesting retry approval. New shell and unified-exec approvals set `environmentId`; older events that do not provide one are exposed as `null`. For normal command approvals, the request also includes `command`, `cwd`, and `commandActions` for friendly display. When `initialize.params.capabilities.experimentalApi = true`, it may also include experimental `additionalPermissions` describing requested per-command sandbox access; any filesystem paths in that payload are absolute on the wire, and network access is represented as `additionalPermissions.network.enabled`. For network-only approvals, those command fields may be omitted and `networkApprovalContext` is provided instead. Optional persistence hints may also be included via `proposedExecpolicyAmendment` and `proposedNetworkPolicyAmendments`. Clients can prefer `availableDecisions` when present to render the exact set of choices the server wants to expose, while still falling back to the older heuristics if it is omitted.
2. `item/commandExecution/requestApproval` (request) — carries the same `itemId`, `threadId`, `turnId`, the nullable `environmentId` where the command will run, optionally `approvalId`, and `reason`. `itemId` remains the stable command-item identity. Use the server-initiated JSON-RPC request ID for the response. When tracking approval UI or state outside that envelope, `approvalId`, when present, is the opaque identity of this callback and distinguishes it from other callbacks on the same `itemId`; compatibility flows may omit it. A follow-up callback may reuse an active item without another `item/started`; if the prior attempt already completed, the server can restart the same `itemId` before requesting retry approval. New shell and unified-exec approvals set `environmentId`; older events that do not provide one are exposed as `null`. For normal command approvals, the request also includes `command`, `cwd`, and `commandActions` for friendly display. When `initialize.params.capabilities.experimentalApi = true`, it may also include experimental `additionalPermissions` describing requested per-command sandbox access; any filesystem paths in that payload are absolute on the wire, and network access is represented as `additionalPermissions.network.enabled`. For network-only approvals, those command fields may be omitted and `networkApprovalContext` is provided instead. Optional persistence hints may also be included via `proposedExecpolicyAmendment` and `proposedNetworkPolicyAmendments`. Clients can prefer `availableDecisions` when present to render the exact set of choices the server wants to expose, while still falling back to the older heuristics if it is omitted.
3. Client response — for example `{ "decision": "accept" }`, `{ "decision": "acceptForSession" }`, `{ "decision": { "acceptWithExecpolicyAmendment": { "execpolicy_amendment": [...] } } }`, `{ "decision": { "applyNetworkPolicyAmendment": { "network_policy_amendment": { "host": "example.com", "action": "allow" } } } }`, `{ "decision": "decline" }`, or `{ "decision": "cancel" }`.
4. `serverRequest/resolved` — `{ threadId, requestId }` confirms the pending request has been resolved or cleared, including lifecycle cleanup on turn start/complete/interrupt.
5. `item/completed` — final `commandExecution` item with `status: "completed" | "failed" | "declined"` and execution output. Render this as the authoritative result.
codex-rs/app-server/src/bespoke_event_handling.rs
+165 -308
View File
@@ -6,6 +6,7 @@ use crate::request_processors::populate_thread_turns_from_history;
use crate::request_processors::thread_from_stored_thread;
use crate::request_processors::thread_settings_from_core_snapshot;
use crate::server_request_error::is_turn_transition_server_request_error;
use crate::thread_state::CommandExecutionItemMetadata;
use crate::thread_state::ThreadState;
use crate::thread_state::TurnSummary;
use crate::thread_state::resolve_server_request_on_thread_listener;
@@ -82,6 +83,7 @@ use codex_app_server_protocol::TurnPlanUpdatedNotification;
use codex_app_server_protocol::TurnStartedNotification;
use codex_app_server_protocol::TurnStatus;
use codex_app_server_protocol::WarningNotification;
use codex_app_server_protocol::build_command_execution_begin_item;
use codex_app_server_protocol::build_item_from_guardian_event;
use codex_app_server_protocol::guardian_auto_approval_review_notification;
use codex_app_server_protocol::item_event_to_server_notification;
@@ -128,12 +130,7 @@ enum CommandExecutionApprovalPresentation {
Command(CommandExecutionCompletionItem),
}
#[derive(Debug, PartialEq)]
struct CommandExecutionCompletionItem {
command: String,
cwd: LegacyAppPathString,
command_actions: Vec<V2ParsedCommand>,
}
type CommandExecutionCompletionItem = CommandExecutionItemMetadata;
#[allow(clippy::too_many_arguments)]
pub(crate) async fn apply_bespoke_event_handling(
@@ -305,17 +302,14 @@ pub(crate) async fn apply_bespoke_event_handling(
| codex_protocol::protocol::GuardianAssessmentStatus::Approved => None,
};
if let Some(completion_status) = completion_status
&& let Some((target_item_id, completion_item)) = pending_command_execution
&& let Some((target_item_id, _)) = pending_command_execution
{
complete_command_execution_item(
&conversation_id,
assessment_turn_id,
target_item_id,
completion_item.command,
completion_item.cwd,
/*process_id*/ None,
CommandExecutionSource::Agent,
completion_item.command_actions,
completion_status,
&outgoing,
&thread_state,
@@ -1123,13 +1117,25 @@ pub(crate) async fn apply_bespoke_event_handling(
return;
}
let item_id = exec_command_begin_event.call_id.clone();
let first_start = {
let mut state = thread_state.lock().await;
state
.turn_summary
.command_execution_started
.insert(item_id.clone())
let ThreadItem::CommandExecution {
command,
cwd,
command_actions,
..
} = build_command_execution_begin_item(&exec_command_begin_event)
else {
unreachable!("command begin must build a command execution item")
};
let first_start = note_started_command_execution_item(
&thread_state,
item_id,
CommandExecutionCompletionItem {
command,
cwd,
command_actions,
},
)
.await;
if first_start {
let notification = item_event_to_server_notification(
EventMsg::ExecCommandBegin(exec_command_begin_event),
@@ -1149,7 +1155,9 @@ pub(crate) async fn apply_bespoke_event_handling(
}
EventMsg::ExecCommandEnd(exec_command_end_event) => {
let call_id = exec_command_end_event.call_id.clone();
let should_emit = remove_started_command_execution_item(&thread_state, &call_id).await;
let should_emit = remove_started_command_execution_item(&thread_state, &call_id)
.await
.is_some();
if matches!(
exec_command_end_event.source,
codex_protocol::protocol::ExecCommandSource::UnifiedExecInteraction
@@ -1387,13 +1395,16 @@ async fn start_command_execution_item(
outgoing: &ThreadScopedOutgoingMessageSender,
thread_state: &Arc<Mutex<ThreadState>>,
) -> bool {
let first_start = {
let mut state = thread_state.lock().await;
state
.turn_summary
.command_execution_started
.insert(item_id.clone())
};
let first_start = note_started_command_execution_item(
thread_state,
item_id.clone(),
CommandExecutionCompletionItem {
command: command.clone(),
cwd: cwd.clone(),
command_actions: command_actions.clone(),
},
)
.await;
if first_start {
let notification = ItemStartedNotification {
thread_id: conversation_id.to_string(),
@@ -1424,28 +1435,25 @@ async fn complete_command_execution_item(
conversation_id: &ThreadId,
turn_id: String,
item_id: String,
command: String,
cwd: LegacyAppPathString,
process_id: Option<String>,
source: CommandExecutionSource,
command_actions: Vec<V2ParsedCommand>,
status: CommandExecutionStatus,
outgoing: &ThreadScopedOutgoingMessageSender,
thread_state: &Arc<Mutex<ThreadState>>,
) {
let should_emit = remove_started_command_execution_item(thread_state, &item_id).await;
if !should_emit {
let Some(completion_item) = remove_started_command_execution_item(thread_state, &item_id).await
else {
return;
}
};
let item = ThreadItem::CommandExecution {
id: item_id,
command,
cwd,
command: completion_item.command,
cwd: completion_item.cwd,
process_id,
source,
status,
command_actions,
command_actions: completion_item.command_actions,
aggregated_output: None,
exit_code: None,
duration_ms: None,
@@ -1461,10 +1469,26 @@ async fn complete_command_execution_item(
.await;
}
async fn note_started_command_execution_item(
thread_state: &Arc<Mutex<ThreadState>>,
item_id: String,
completion_item: CommandExecutionCompletionItem,
) -> bool {
let mut state = thread_state.lock().await;
if let std::collections::hash_map::Entry::Vacant(entry) =
state.turn_summary.command_execution_started.entry(item_id)
{
entry.insert(completion_item);
true
} else {
false
}
}
async fn remove_started_command_execution_item(
thread_state: &Arc<Mutex<ThreadState>>,
item_id: &str,
) -> bool {
) -> Option<CommandExecutionCompletionItem> {
thread_state
.lock()
.await
@@ -2163,17 +2187,14 @@ async fn on_command_execution_request_approval_response(
if let Some(status) = completion_status
&& !suppress_repeated_prompt_completion_item
&& let Some(completion_item) = completion_item
&& completion_item.is_some()
{
complete_command_execution_item(
&conversation_id,
event_turn_id.clone(),
item_id.clone(),
completion_item.command,
completion_item.cwd,
/*process_id*/ None,
CommandExecutionSource::Agent,
completion_item.command_actions,
status,
&outgoing,
&thread_state,
@@ -2224,7 +2245,6 @@ mod tests {
use codex_app_server_protocol::AutoReviewDecisionSource;
use codex_app_server_protocol::GuardianApprovalReviewStatus;
use codex_app_server_protocol::JSONRPCErrorError;
use codex_app_server_protocol::ServerRequest;
use codex_app_server_protocol::TurnPlanStepStatus;
use codex_login::CodexAuth;
use codex_protocol::AgentPath;
@@ -2387,100 +2407,6 @@ mod tests {
}
}
struct CommandApprovalTestContext {
_codex_home: TempDir,
conversation_id: ThreadId,
conversation: Arc<CodexThread>,
thread_manager: Arc<ThreadManager>,
outgoing: ThreadScopedOutgoingMessageSender,
raw_outgoing: Arc<OutgoingMessageSender>,
thread_state: Arc<Mutex<ThreadState>>,
thread_watch_manager: ThreadWatchManager,
rx: mpsc::Receiver<OutgoingEnvelope>,
}
impl CommandApprovalTestContext {
async fn new() -> Result<Self> {
let codex_home = TempDir::new()?;
let config = load_default_config_for_test(&codex_home).await;
let thread_manager = Arc::new(
codex_core::test_support::thread_manager_with_models_provider_and_home(
CodexAuth::create_dummy_chatgpt_auth_for_testing(),
config.model_provider.clone(),
config.codex_home.to_path_buf(),
Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()),
),
);
let codex_core::NewThread {
thread_id: conversation_id,
thread: conversation,
..
} = thread_manager.start_thread(config).await?;
let (tx, rx) = mpsc::channel(CHANNEL_CAPACITY);
let raw_outgoing = Arc::new(OutgoingMessageSender::new(
tx,
codex_analytics::AnalyticsEventsClient::disabled(),
));
let outgoing = ThreadScopedOutgoingMessageSender::new(
Arc::clone(&raw_outgoing),
vec![ConnectionId(1)],
conversation_id,
);
Ok(Self {
_codex_home: codex_home,
conversation_id,
conversation,
thread_manager,
outgoing,
raw_outgoing,
thread_state: new_thread_state(),
thread_watch_manager: ThreadWatchManager::new(),
rx,
})
}
async fn apply(&self, event: Event) {
apply_bespoke_event_handling(
event,
self.conversation_id,
Arc::clone(&self.conversation),
Arc::clone(&self.thread_manager),
self.outgoing.clone(),
Arc::clone(&self.thread_state),
self.thread_watch_manager.clone(),
Arc::new(tokio::sync::Semaphore::new(/*permits*/ 1)),
"test-provider".to_string(),
)
.await;
}
}
fn exec_approval_event(
call_id: &str,
approval_id: Option<&str>,
reason: Option<&str>,
) -> Event {
Event {
id: "turn-1".to_string(),
msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent {
call_id: call_id.to_string(),
approval_id: approval_id.map(str::to_string),
turn_id: "turn-1".to_string(),
environment_id: Some("local".to_string()),
started_at_ms: 1_000,
command: vec!["printf".to_string(), "hi".to_string()],
cwd: test_path_buf("/tmp").abs(),
reason: reason.map(str::to_string),
network_approval_context: None,
proposed_execpolicy_amendment: None,
proposed_network_policy_amendments: None,
additional_permissions: None,
available_decisions: Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]),
parsed_cmd: Vec::new(),
}),
}
}
fn guardian_command_assessment(
id: &str,
turn_id: &str,
@@ -2705,189 +2631,120 @@ mod tests {
}
#[tokio::test]
async fn repeated_approval_dispatch_preserves_item_and_callback_ids() -> Result<()> {
let mut context = CommandApprovalTestContext::new().await?;
context
.apply(exec_approval_event("cmd-1", None, Some("initial")))
.await;
let started = recv_broadcast_message(&mut context.rx).await?;
let OutgoingMessage::AppServerNotification(ServerNotification::ItemStarted(started)) =
started
else {
bail!("expected item/started");
};
assert!(matches!(
started.item,
ThreadItem::CommandExecution { ref id, .. } if id == "cmd-1"
));
let initial_request = recv_broadcast_message(&mut context.rx).await?;
let OutgoingMessage::Request(ServerRequest::CommandExecutionRequestApproval {
request_id,
params,
}) = initial_request
else {
bail!("expected initial command approval request");
};
assert_eq!(params.item_id, "cmd-1");
assert_eq!(params.approval_id, None);
context
.raw_outgoing
.notify_client_response(
request_id,
serde_json::to_value(CommandExecutionRequestApprovalResponse {
decision: CommandExecutionApprovalDecision::Accept,
})?,
)
.await;
context
.apply(exec_approval_event(
"cmd-1",
Some("retry-1"),
Some("sandbox denied"),
))
.await;
let retry_request = recv_broadcast_message(&mut context.rx).await?;
let OutgoingMessage::Request(ServerRequest::CommandExecutionRequestApproval {
request_id,
params,
}) = retry_request
else {
bail!("retry must reuse the active item without another item/started");
};
assert_eq!(params.item_id, "cmd-1");
assert_eq!(params.approval_id.as_deref(), Some("retry-1"));
assert_eq!(params.reason.as_deref(), Some("sandbox denied"));
assert_eq!(
params.available_decisions,
Some(vec![
CommandExecutionApprovalDecision::Accept,
CommandExecutionApprovalDecision::Cancel,
])
async fn repeated_start_retains_parent_metadata_for_completion() -> Result<()> {
let conversation_id = ThreadId::new();
let thread_state = new_thread_state();
let (tx, mut rx) = mpsc::channel(CHANNEL_CAPACITY);
let outgoing = ThreadScopedOutgoingMessageSender::new(
Arc::new(OutgoingMessageSender::new(
tx,
codex_analytics::AnalyticsEventsClient::disabled(),
)),
vec![ConnectionId(1)],
conversation_id,
);
context
.raw_outgoing
.notify_client_response(
request_id,
serde_json::to_value(CommandExecutionRequestApprovalResponse {
decision: CommandExecutionApprovalDecision::Decline,
})?,
let parent = command_execution_completion_item("parent-command");
let child = CommandExecutionCompletionItem {
command: "child-command".to_string(),
cwd: test_path_buf("/child").abs().into(),
command_actions: vec![],
};
assert!(
start_command_execution_item(
&conversation_id,
"turn-1".to_string(),
"cmd-1".to_string(),
parent.command.clone(),
parent.cwd.clone(),
parent.command_actions.clone(),
CommandExecutionSource::Agent,
&outgoing,
&thread_state,
)
.await;
let completed = recv_broadcast_message(&mut context.rx).await?;
assert!(matches!(
completed,
OutgoingMessage::AppServerNotification(ServerNotification::ItemCompleted(payload))
if matches!(payload.item, ThreadItem::CommandExecution { ref id, status: CommandExecutionStatus::Declined, .. } if id == "cmd-1")
));
.await
);
let _started = recv_broadcast_message(&mut rx).await?;
assert!(
!start_command_execution_item(
&conversation_id,
"turn-1".to_string(),
"cmd-1".to_string(),
child.command,
child.cwd,
child.command_actions,
CommandExecutionSource::Agent,
&outgoing,
&thread_state,
)
.await
);
complete_command_execution_item(
&conversation_id,
"turn-1".to_string(),
"cmd-1".to_string(),
/*process_id*/ None,
CommandExecutionSource::Agent,
CommandExecutionStatus::Declined,
&outgoing,
&thread_state,
)
.await;
let OutgoingMessage::AppServerNotification(ServerNotification::ItemCompleted(payload)) =
recv_broadcast_message(&mut rx).await?
else {
bail!("expected item/completed");
};
let ThreadItem::CommandExecution {
id,
command,
cwd,
command_actions,
status,
..
} = payload.item
else {
bail!("expected command execution item");
};
assert_eq!(
(id, command, cwd, command_actions, status),
(
"cmd-1".to_string(),
parent.command,
parent.cwd,
parent.command_actions,
CommandExecutionStatus::Declined,
)
);
assert!(
remove_started_command_execution_item(&thread_state, "cmd-1")
.await
.is_none()
);
Ok(())
}
#[tokio::test]
async fn repeated_approval_response_lifecycle_truth_table() -> Result<()> {
let mut context = CommandApprovalTestContext::new().await?;
#[test]
fn repeated_approval_response_lifecycle_truth_table() {
let cases = [
(false, true, CommandExecutionApprovalDecision::Decline, true),
(true, true, CommandExecutionApprovalDecision::Decline, true),
(true, true, CommandExecutionApprovalDecision::Cancel, true),
(true, true, CommandExecutionApprovalDecision::Accept, false),
(
true,
false,
CommandExecutionApprovalDecision::Decline,
false,
),
(true, false, CommandExecutionApprovalDecision::Cancel, true),
(None, false, false, false, false),
(Some("callback"), false, false, false, true),
(Some("callback"), false, false, true, false),
(Some("callback"), false, true, false, false),
(Some("callback"), true, false, false, false),
];
for (index, (has_active_item, is_sandbox_retry, decision, completes_on_response)) in
cases.into_iter().enumerate()
{
let item_id = format!("cmd-{index}");
let completion_item = command_execution_completion_item("printf hi");
if has_active_item {
assert!(
start_command_execution_item(
&context.conversation_id,
"turn-1".to_string(),
item_id.clone(),
completion_item.command.clone(),
completion_item.cwd.clone(),
completion_item.command_actions.clone(),
CommandExecutionSource::Agent,
&context.outgoing,
&context.thread_state,
)
.await
);
let _initial_started = recv_broadcast_message(&mut context.rx).await?;
}
let started_now = start_command_execution_item(
&context.conversation_id,
"turn-1".to_string(),
item_id.clone(),
completion_item.command.clone(),
completion_item.cwd.clone(),
completion_item.command_actions.clone(),
CommandExecutionSource::Agent,
&context.outgoing,
&context.thread_state,
)
.await;
assert_eq!(started_now, !has_active_item);
if started_now {
let _callback_started = recv_broadcast_message(&mut context.rx).await?;
}
let (response_tx, response_rx) = oneshot::channel();
response_tx
.send(Ok(serde_json::to_value(
CommandExecutionRequestApprovalResponse { decision },
)?))
.expect("response receiver should remain open");
let permission_guard = context
.thread_watch_manager
.note_permission_requested(&context.conversation_id.to_string())
.await;
on_command_execution_request_approval_response(
"turn-1".to_string(),
context.conversation_id,
Some(format!("callback-{index}")),
item_id.clone(),
Some(completion_item),
started_now,
is_sandbox_retry,
RequestId::Integer(index as i64),
response_rx,
Arc::clone(&context.conversation),
context.outgoing.clone(),
Arc::clone(&context.thread_state),
permission_guard,
)
.await;
if completes_on_response {
let completed = recv_broadcast_message(&mut context.rx).await?;
assert!(matches!(
completed,
OutgoingMessage::AppServerNotification(ServerNotification::ItemCompleted(payload))
if matches!(payload.item, ThreadItem::CommandExecution { ref id, status: CommandExecutionStatus::Declined, .. } if id == &item_id)
));
assert!(
!remove_started_command_execution_item(&context.thread_state, &item_id).await,
"a late runtime end must not complete {item_id} twice"
);
} else {
assert!(context.rx.try_recv().is_err());
assert!(
remove_started_command_execution_item(&context.thread_state, &item_id).await,
"the active parent retains completion ownership for {item_id}"
);
}
for (approval_id, started_now, is_sandbox_retry, cancel_requested, expected) in cases {
assert_eq!(
suppress_repeated_prompt_completion_item(
approval_id,
started_now,
is_sandbox_retry,
cancel_requested,
),
expected
);
}
Ok(())
}
#[tokio::test]
codex-rs/app-server/src/thread_state.rs
+9 -1
View File
@@ -1,5 +1,6 @@
use crate::outgoing_message::ConnectionId;
use crate::outgoing_message::ConnectionRequestId;
use codex_app_server_protocol::CommandAction;
use codex_app_server_protocol::RequestId;
use codex_app_server_protocol::ThreadGoal;
use codex_app_server_protocol::ThreadHistoryBuilder;
@@ -29,6 +30,13 @@ use tracing::error;
type PendingInterruptQueue = Vec<ConnectionRequestId>;
#[derive(Clone, Debug, PartialEq)]
pub(crate) struct CommandExecutionItemMetadata {
pub(crate) command: String,
pub(crate) cwd: LegacyAppPathString,
pub(crate) command_actions: Vec<CommandAction>,
}
pub(crate) struct PendingThreadResumeRequest {
pub(crate) request_id: ConnectionRequestId,
pub(crate) history_items: Vec<RolloutItem>,
@@ -70,7 +78,7 @@ pub(crate) enum ThreadListenerCommand {
#[derive(Default, Clone)]
pub(crate) struct TurnSummary {
pub(crate) started_at: Option<i64>,
pub(crate) command_execution_started: HashSet<String>,
pub(crate) command_execution_started: HashMap<String, CommandExecutionItemMetadata>,
pub(crate) last_error: Option<TurnError>,
}
codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs
+29
View File
@@ -640,6 +640,27 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2()
assert_eq!(approved_subcommand_strings.len(), 2);
assert!(approved_subcommand_strings[0].contains(&first_file.display().to_string()));
assert!(approved_subcommand_strings[1].contains(&second_file.display().to_string()));
let parent_started_metadata = timeout(DEFAULT_READ_TIMEOUT, async {
loop {
let started_notif = mcp
.read_stream_until_notification_message("item/started")
.await?;
let started: ItemStartedNotification =
serde_json::from_value(started_notif.params.expect("item/started params"))?;
if let ThreadItem::CommandExecution {
id,
command,
cwd,
command_actions,
..
} = started.item
&& id == "call-zsh-fork-subcommand-decline"
{
return Ok::<_, anyhow::Error>((command, cwd, command_actions));
}
}
})
.await??;
let parent_completed_command_execution = timeout(DEFAULT_READ_TIMEOUT, async {
loop {
let completed_notif = mcp
@@ -664,6 +685,9 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2()
Ok(Ok(parent_completed_command_execution)) => {
let ThreadItem::CommandExecution {
id,
command,
cwd,
command_actions,
status,
aggregated_output,
..
@@ -673,6 +697,11 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2()
};
assert_eq!(id, "call-zsh-fork-subcommand-decline");
assert_eq!(status, CommandExecutionStatus::Declined);
assert_eq!(
(command, cwd, command_actions),
parent_started_metadata,
"parent completion metadata was replaced by a child callback"
);
if let Some(output) = aggregated_output.as_deref() {
assert!(
output == "exec command rejected by user"
codex-rs/core/src/session/mod.rs
+13 -9
View File
@@ -2131,9 +2131,9 @@ impl Session {
/// cleared before a response arrives, treat it as an abort so interrupted
/// turns do not continue on a synthetic denial.
///
/// Note that if `available_decisions` is `None`, then the other fields will
/// be used to derive the available decisions via
/// [ExecApprovalRequestEvent::default_available_decisions].
/// If `available_decisions` is `None`, callback-scoped requests with an
/// `approval_id` offer only approve or abort. Other requests derive their
/// decisions via [ExecApprovalRequestEvent::default_available_decisions].
#[allow(clippy::too_many_arguments)]
#[expect(
clippy::await_holding_invalid_type,
@@ -2169,12 +2169,16 @@ impl Session {
]
});
let available_decisions = available_decisions.unwrap_or_else(|| {
ExecApprovalRequestEvent::default_available_decisions(
network_approval_context.as_ref(),
proposed_execpolicy_amendment.as_ref(),
proposed_network_policy_amendments.as_deref(),
additional_permissions.as_ref(),
)
if approval_id.is_some() {
vec![ReviewDecision::Approved, ReviewDecision::Abort]
} else {
ExecApprovalRequestEvent::default_available_decisions(
network_approval_context.as_ref(),
proposed_execpolicy_amendment.as_ref(),
proposed_network_policy_amendments.as_deref(),
additional_permissions.as_ref(),
)
}
});
let accepted_decisions = accepted_command_approval_decisions(
available_decisions.clone(),
codex-rs/core/src/session/tests.rs
+85
View File
@@ -793,6 +793,91 @@ fn accepted_command_decisions_add_only_exact_proposed_network_denials() {
}));
}
#[tokio::test]
async fn callback_without_explicit_decisions_is_one_shot() {
let (session, turn_context, rx_events) = make_session_and_context_with_rx().await;
*session.active_turn.lock().await = Some(ActiveTurn::default());
let request = tokio::spawn({
let session = Arc::clone(&session);
let turn_context = Arc::clone(&turn_context);
async move {
session
.request_command_approval(
turn_context.as_ref(),
"command-item".to_string(),
Some("callback".to_string()),
/*environment_id*/ None,
vec!["echo".to_string(), "hi".to_string()],
test_path_buf("/tmp").abs(),
/*reason*/ None,
/*network_approval_context*/ None,
/*proposed_execpolicy_amendment*/ None,
/*additional_permissions*/ None,
/*available_decisions*/ None,
)
.await
}
});
let event = tokio::time::timeout(std::time::Duration::from_secs(1), rx_events.recv())
.await
.expect("approval event timed out")
.expect("approval event missing");
let EventMsg::ExecApprovalRequest(event) = event.msg else {
panic!("expected exec approval event");
};
assert_eq!(
event.available_decisions,
Some(vec![ReviewDecision::Approved, ReviewDecision::Abort])
);
session
.notify_exec_approval("callback", ReviewDecision::Abort)
.await;
assert_eq!(
request.await.expect("approval request task"),
ReviewDecision::Abort
);
let explicit_request = tokio::spawn({
let session = Arc::clone(&session);
let turn_context = Arc::clone(&turn_context);
async move {
session
.request_command_approval(
turn_context.as_ref(),
"command-item".to_string(),
Some("explicit-callback".to_string()),
/*environment_id*/ None,
vec!["echo".to_string(), "hi".to_string()],
test_path_buf("/tmp").abs(),
/*reason*/ None,
/*network_approval_context*/ None,
/*proposed_execpolicy_amendment*/ None,
/*additional_permissions*/ None,
Some(vec![ReviewDecision::ApprovedForSession]),
)
.await
}
});
let event = tokio::time::timeout(std::time::Duration::from_secs(1), rx_events.recv())
.await
.expect("explicit approval event timed out")
.expect("explicit approval event missing");
let EventMsg::ExecApprovalRequest(event) = event.msg else {
panic!("expected exec approval event");
};
assert_eq!(
event.available_decisions,
Some(vec![ReviewDecision::ApprovedForSession])
);
session
.notify_exec_approval("explicit-callback", ReviewDecision::ApprovedForSession)
.await;
assert_eq!(
explicit_request.await.expect("explicit approval task"),
ReviewDecision::ApprovedForSession
);
}
#[derive(Clone, Copy, Debug)]
enum DuplicateApprovalRequestKind {
Exec,
codex-rs/protocol/src/approvals.rs
+31 -2
View File
@@ -220,8 +220,8 @@ pub struct ExecApprovalRequestEvent {
pub call_id: String,
/// Identifier for this specific approval callback.
///
/// When absent, the approval is for the command item itself (`call_id`).
/// This is present for subcommand approvals (via execve intercept).
/// When present, responses must use this ID instead of the command-item
/// `call_id`. Absence preserves legacy/cacheable approval routing.
#[serde(default, skip_serializing_if = "Option::is_none")]
#[ts(optional)]
pub approval_id: Option<String>,
@@ -286,6 +286,9 @@ impl ExecApprovalRequestEvent {
// senders, so we fall back to the legacy logic if it's not present.
match &self.available_decisions {
Some(decisions) => decisions.clone(),
None if self.approval_id.is_some() => {
vec![ReviewDecision::Approved, ReviewDecision::Abort]
}
None => Self::default_available_decisions(
self.network_approval_context.as_ref(),
self.proposed_execpolicy_amendment.as_ref(),
@@ -472,6 +475,32 @@ mod tests {
);
}
#[test]
fn callback_without_explicit_decisions_is_one_shot() {
let value = serde_json::json!({
"call_id": "command-item",
"approval_id": "callback",
"turn_id": "turn",
"started_at_ms": 0,
"command": ["echo", "hi"],
"cwd": test_path_buf("/tmp"),
"reason": null,
"parsed_cmd": [],
});
let mut event: ExecApprovalRequestEvent =
serde_json::from_value(value).expect("callback event");
assert_eq!(
event.effective_available_decisions(),
vec![ReviewDecision::Approved, ReviewDecision::Abort]
);
event.available_decisions = Some(vec![ReviewDecision::ApprovedForSession]);
assert_eq!(
event.effective_available_decisions(),
vec![ReviewDecision::ApprovedForSession]
);
}
#[test]
fn guardian_assessment_action_deserializes_command_shape() {
let action: GuardianAssessmentAction = serde_json::from_value(serde_json::json!({
codex-rs/tui/src/app.rs
+8
View File
@@ -303,6 +303,7 @@ fn collab_receiver_is_not_found(
}
fn default_exec_approval_decisions(
approval_id: Option<&str>,
network_approval_context: Option<&codex_app_server_protocol::NetworkApprovalContext>,
proposed_execpolicy_amendment: Option<&codex_app_server_protocol::ExecPolicyAmendment>,
proposed_network_policy_amendments: Option<
@@ -313,6 +314,13 @@ fn default_exec_approval_decisions(
use codex_app_server_protocol::CommandExecutionApprovalDecision;
use codex_app_server_protocol::NetworkPolicyRuleAction;
if approval_id.is_some() {
return vec![
CommandExecutionApprovalDecision::Accept,
CommandExecutionApprovalDecision::Cancel,
];
}
if network_approval_context.is_some() {
let mut decisions = vec![
CommandExecutionApprovalDecision::Accept,
codex-rs/tui/src/app/tests.rs
+43
View File
@@ -2524,6 +2524,49 @@ async fn inactive_thread_exec_approval_preserves_context() {
);
}
#[tokio::test]
async fn callback_exec_approval_falls_back_to_one_shot_decisions() -> std::io::Result<()> {
use codex_app_server_protocol::CommandExecutionApprovalDecision as Decision;
let app = make_test_app().await;
let thread_id = ThreadId::new();
let request = exec_approval_request(
thread_id,
"turn-approval",
"call-approval",
Some("callback"),
);
let Some(ThreadInteractiveRequest::Approval(ApprovalRequest::Exec {
available_decisions,
..
})) = app
.interactive_request_for_thread_request(thread_id, &request)
.await?
else {
panic!("expected exec approval request");
};
assert_eq!(
available_decisions,
vec![Decision::Accept, Decision::Cancel]
);
let mut request = request;
let ServerRequest::CommandExecutionRequestApproval { params, .. } = &mut request else {
unreachable!();
};
params.available_decisions = Some(vec![Decision::AcceptForSession]);
let Some(ThreadInteractiveRequest::Approval(ApprovalRequest::Exec {
available_decisions,
..
})) = app
.interactive_request_for_thread_request(thread_id, &request)
.await?
else {
panic!("expected exec approval request");
};
assert_eq!(available_decisions, vec![Decision::AcceptForSession]);
Ok(())
}
#[tokio::test]
async fn inactive_thread_exec_approval_splits_shell_wrapped_command() {
let app = make_test_app().await;
codex-rs/tui/src/app/thread_routing.rs
+1
View File
@@ -241,6 +241,7 @@ impl App {
reason: params.reason.clone(),
available_decisions: params.available_decisions.clone().unwrap_or_else(|| {
default_exec_approval_decisions(
params.approval_id.as_deref(),
network_approval_context.as_ref(),
proposed_execpolicy_amendment.as_ref(),
proposed_network_policy_amendments.as_deref(),
codex-rs/tui/src/approval_events.rs
+4
View File
@@ -53,6 +53,10 @@ impl ExecApprovalRequestEvent {
pub(crate) fn effective_available_decisions(&self) -> Vec<CommandExecutionApprovalDecision> {
match &self.available_decisions {
Some(decisions) => decisions.clone(),
None if self.approval_id.is_some() => vec![
CommandExecutionApprovalDecision::Accept,
CommandExecutionApprovalDecision::Cancel,
],
None => Self::default_available_decisions(
self.network_approval_context.as_ref(),
self.proposed_execpolicy_amendment.as_ref(),
codex-rs/tui/src/chatwidget/tests/approval_requests.rs
+3 -3
View File
@@ -11,7 +11,7 @@ async fn exec_approval_emits_proposed_command_and_decision_history() {
// Trigger an exec approval request with a short, single-line command.
let ev = ExecApprovalRequestEvent {
call_id: "call-short".into(),
approval_id: Some("call-short".into()),
approval_id: None,
turn_id: "turn-short".into(),
environment_id: Some("remote".to_string()),
command: vec!["bash".into(), "-lc".into(), "echo hello world".into()],
@@ -377,7 +377,7 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() {
let ev_multi = ExecApprovalRequestEvent {
call_id: "call-multi".into(),
approval_id: Some("call-multi".into()),
approval_id: None,
turn_id: "turn-multi".into(),
environment_id: None,
command: vec!["bash".into(), "-lc".into(), "echo line1\necho line2".into()],
@@ -429,7 +429,7 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() {
let long = format!("echo {}", "a".repeat(200));
let ev_long = ExecApprovalRequestEvent {
call_id: "call-long".into(),
approval_id: Some("call-long".into()),
approval_id: None,
turn_id: "turn-long".into(),
environment_id: None,
command: vec!["bash".into(), "-lc".into(), long],
codex-rs/tui/src/chatwidget/tests/exec_flow.rs
+29 -25
View File
@@ -8,7 +8,7 @@ async fn exec_approval_emits_proposed_command_and_decision_history() {
// Trigger an exec approval request with a short, single-line command
let ev = ExecApprovalRequestEvent {
call_id: "call-short".into(),
approval_id: Some("call-short".into()),
approval_id: None,
turn_id: "turn-short".into(),
environment_id: None,
command: vec!["bash".into(), "-lc".into(), "echo hello world".into()],
@@ -88,26 +88,30 @@ fn app_server_exec_approval_request_splits_shell_wrapped_command() {
async fn exec_approval_uses_approval_id_when_present() {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
handle_exec_approval_request(
&mut chat,
"sub-short",
ExecApprovalRequestEvent {
call_id: "call-parent".into(),
approval_id: Some("approval-subcommand".into()),
turn_id: "turn-short".into(),
environment_id: None,
command: vec!["bash".into(), "-lc".into(), "echo hello world".into()],
cwd: AbsolutePathBuf::current_dir().expect("current dir"),
reason: Some(
"this is a test reason such as one that would be produced by the model".into(),
),
network_approval_context: None,
proposed_execpolicy_amendment: None,
proposed_network_policy_amendments: None,
additional_permissions: None,
available_decisions: None,
},
let event = ExecApprovalRequestEvent {
call_id: "call-parent".into(),
approval_id: Some("approval-subcommand".into()),
turn_id: "turn-short".into(),
environment_id: None,
command: vec!["bash".into(), "-lc".into(), "echo hello world".into()],
cwd: AbsolutePathBuf::current_dir().expect("current dir"),
reason: Some(
"this is a test reason such as one that would be produced by the model".into(),
),
network_approval_context: None,
proposed_execpolicy_amendment: None,
proposed_network_policy_amendments: None,
additional_permissions: None,
available_decisions: None,
};
assert_eq!(
event.effective_available_decisions(),
vec![
codex_app_server_protocol::CommandExecutionApprovalDecision::Accept,
codex_app_server_protocol::CommandExecutionApprovalDecision::Cancel,
]
);
handle_exec_approval_request(&mut chat, "sub-short", event);
chat.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE));
@@ -137,7 +141,7 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() {
// Multiline command: modal should show full command, history records decision only
let ev_multi = ExecApprovalRequestEvent {
call_id: "call-multi".into(),
approval_id: Some("call-multi".into()),
approval_id: None,
turn_id: "turn-multi".into(),
environment_id: None,
command: vec!["bash".into(), "-lc".into(), "echo line1\necho line2".into()],
@@ -191,7 +195,7 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() {
let long = format!("echo {}", "a".repeat(200));
let ev_long = ExecApprovalRequestEvent {
call_id: "call-long".into(),
approval_id: Some("call-long".into()),
approval_id: None,
turn_id: "turn-long".into(),
environment_id: None,
command: vec!["bash".into(), "-lc".into(), long],
@@ -1116,7 +1120,7 @@ async fn approval_modal_exec_snapshot() -> anyhow::Result<()> {
// Inject an exec approval request to display the approval modal.
let ev = ExecApprovalRequestEvent {
call_id: "call-approve-cmd".into(),
approval_id: Some("call-approve-cmd".into()),
approval_id: None,
turn_id: "turn-approve-cmd".into(),
environment_id: None,
command: vec!["bash".into(), "-lc".into(), "echo hello world".into()],
@@ -1174,7 +1178,7 @@ async fn approval_modal_exec_without_reason_snapshot() -> anyhow::Result<()> {
let ev = ExecApprovalRequestEvent {
call_id: "call-approve-cmd-noreason".into(),
approval_id: Some("call-approve-cmd-noreason".into()),
approval_id: None,
turn_id: "turn-approve-cmd-noreason".into(),
environment_id: None,
command: vec!["bash".into(), "-lc".into(), "echo hello world".into()],
@@ -1221,7 +1225,7 @@ async fn approval_modal_exec_multiline_prefix_hides_execpolicy_option_snapshot()
let command = vec!["bash".into(), "-lc".into(), script];
let ev = ExecApprovalRequestEvent {
call_id: "call-approve-cmd-multiline-trunc".into(),
approval_id: Some("call-approve-cmd-multiline-trunc".into()),
approval_id: None,
turn_id: "turn-approve-cmd-multiline-trunc".into(),
environment_id: None,
command: command.clone(),
codex-rs/tui/src/chatwidget/tests/status_and_layout.rs
+1 -1
View File
@@ -2044,7 +2044,7 @@ async fn status_widget_and_approval_modal_snapshot() {
// Now show an approval modal (e.g. exec approval).
let ev = ExecApprovalRequestEvent {
call_id: "call-approve-exec".into(),
approval_id: Some("call-approve-exec".into()),
approval_id: None,
turn_id: "turn-approve-exec".into(),
environment_id: None,
command: vec!["echo".into(), "hello world".into()],