mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat(core): plumb distinct approval ids for command approvals (#12051)
zsh fork PR stack: - https://github.com/openai/codex/pull/12051 👈 - https://github.com/openai/codex/pull/12052 With upcoming support for a fork of zsh that allows us to intercept `execve` and run execpolicy checks for each subcommand as part of a `CommandExecution`, it will be possible for there to be multiple approval requests for a shell command like `/path/to/zsh -lc 'git status && rg \"TODO\" src && make test'`. To support that, this PR introduces a new `approval_id` field across core, protocol, and app-server so that we can associate approvals properly for subcommands.
This commit is contained in:
committed by
GitHub
Unverified
parent
b3a8571219
commit
db4d2599b5
@@ -113,6 +113,13 @@
|
||||
}
|
||||
},
|
||||
"properties": {
|
||||
"approvalId": {
|
||||
"description": "Unique identifier for this specific approval callback.\n\nFor regular shell/unified_exec approvals, this is null.\n\nFor zsh-exec-bridge subcommand approvals, multiple callbacks can belong to one parent `itemId`, so `approvalId` is a distinct opaque callback id (a UUID) used to disambiguate routing.",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"command": {
|
||||
"description": "The command to be executed.",
|
||||
"type": [
|
||||
|
||||
@@ -1452,8 +1452,15 @@
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"approval_id": {
|
||||
"description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"call_id": {
|
||||
"description": "Identifier for the associated exec call, if available.",
|
||||
"description": "Identifier for the associated command execution item.",
|
||||
"type": "string"
|
||||
},
|
||||
"command": {
|
||||
@@ -6392,8 +6399,15 @@
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"approval_id": {
|
||||
"description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"call_id": {
|
||||
"description": "Identifier for the associated exec call, if available.",
|
||||
"description": "Identifier for the associated command execution item.",
|
||||
"type": "string"
|
||||
},
|
||||
"command": {
|
||||
|
||||
@@ -117,6 +117,13 @@
|
||||
}
|
||||
},
|
||||
"properties": {
|
||||
"approvalId": {
|
||||
"description": "Identifier for this specific approval callback.",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"callId": {
|
||||
"description": "Use to correlate this with [codex_core::protocol::ExecCommandBeginEvent] and [codex_core::protocol::ExecCommandEndEvent].",
|
||||
"type": "string"
|
||||
|
||||
@@ -2268,8 +2268,15 @@
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"approval_id": {
|
||||
"description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"call_id": {
|
||||
"description": "Identifier for the associated exec call, if available.",
|
||||
"description": "Identifier for the associated command execution item.",
|
||||
"type": "string"
|
||||
},
|
||||
"command": {
|
||||
|
||||
@@ -179,6 +179,13 @@
|
||||
},
|
||||
"CommandExecutionRequestApprovalParams": {
|
||||
"properties": {
|
||||
"approvalId": {
|
||||
"description": "Unique identifier for this specific approval callback.\n\nFor regular shell/unified_exec approvals, this is null.\n\nFor zsh-exec-bridge subcommand approvals, multiple callbacks can belong to one parent `itemId`, so `approvalId` is a distinct opaque callback id (a UUID) used to disambiguate routing.",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"command": {
|
||||
"description": "The command to be executed.",
|
||||
"type": [
|
||||
@@ -264,6 +271,13 @@
|
||||
},
|
||||
"ExecCommandApprovalParams": {
|
||||
"properties": {
|
||||
"approvalId": {
|
||||
"description": "Identifier for this specific approval callback.",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"callId": {
|
||||
"description": "Use to correlate this with [codex_core::protocol::ExecCommandBeginEvent] and [codex_core::protocol::ExecCommandEndEvent].",
|
||||
"type": "string"
|
||||
|
||||
@@ -2051,6 +2051,13 @@
|
||||
"CommandExecutionRequestApprovalParams": {
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"properties": {
|
||||
"approvalId": {
|
||||
"description": "Unique identifier for this specific approval callback.\n\nFor regular shell/unified_exec approvals, this is null.\n\nFor zsh-exec-bridge subcommand approvals, multiple callbacks can belong to one parent `itemId`, so `approvalId` is a distinct opaque callback id (a UUID) used to disambiguate routing.",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"command": {
|
||||
"description": "The command to be executed.",
|
||||
"type": [
|
||||
@@ -3465,8 +3472,15 @@
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"approval_id": {
|
||||
"description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"call_id": {
|
||||
"description": "Identifier for the associated exec call, if available.",
|
||||
"description": "Identifier for the associated command execution item.",
|
||||
"type": "string"
|
||||
},
|
||||
"command": {
|
||||
@@ -4939,6 +4953,13 @@
|
||||
"ExecCommandApprovalParams": {
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"properties": {
|
||||
"approvalId": {
|
||||
"description": "Identifier for this specific approval callback.",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"callId": {
|
||||
"description": "Use to correlate this with [codex_core::protocol::ExecCommandBeginEvent] and [codex_core::protocol::ExecCommandEndEvent].",
|
||||
"type": "string"
|
||||
|
||||
@@ -1452,8 +1452,15 @@
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"approval_id": {
|
||||
"description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"call_id": {
|
||||
"description": "Identifier for the associated exec call, if available.",
|
||||
"description": "Identifier for the associated command execution item.",
|
||||
"type": "string"
|
||||
},
|
||||
"command": {
|
||||
|
||||
@@ -1452,8 +1452,15 @@
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"approval_id": {
|
||||
"description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"call_id": {
|
||||
"description": "Identifier for the associated exec call, if available.",
|
||||
"description": "Identifier for the associated command execution item.",
|
||||
"type": "string"
|
||||
},
|
||||
"command": {
|
||||
|
||||
@@ -1452,8 +1452,15 @@
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"approval_id": {
|
||||
"description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"call_id": {
|
||||
"description": "Identifier for the associated exec call, if available.",
|
||||
"description": "Identifier for the associated command execution item.",
|
||||
"type": "string"
|
||||
},
|
||||
"command": {
|
||||
|
||||
@@ -7,9 +7,16 @@ import type { ParsedCommand } from "./ParsedCommand";
|
||||
|
||||
export type ExecApprovalRequestEvent = {
|
||||
/**
|
||||
* Identifier for the associated exec call, if available.
|
||||
* Identifier for the associated command execution item.
|
||||
*/
|
||||
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).
|
||||
*/
|
||||
approval_id?: string,
|
||||
/**
|
||||
* Turn ID that this command belongs to.
|
||||
* Uses `#[serde(default)]` for backwards compatibility.
|
||||
|
||||
@@ -9,4 +9,8 @@ export type ExecCommandApprovalParams = { conversationId: ThreadId,
|
||||
* Use to correlate this with [codex_core::protocol::ExecCommandBeginEvent]
|
||||
* and [codex_core::protocol::ExecCommandEndEvent].
|
||||
*/
|
||||
callId: string, command: Array<string>, cwd: string, reason: string | null, parsedCmd: Array<ParsedCommand>, };
|
||||
callId: string,
|
||||
/**
|
||||
* Identifier for this specific approval callback.
|
||||
*/
|
||||
approvalId: string | null, command: Array<string>, cwd: string, reason: string | null, parsedCmd: Array<ParsedCommand>, };
|
||||
|
||||
+10
@@ -5,6 +5,16 @@ import type { CommandAction } from "./CommandAction";
|
||||
import type { ExecPolicyAmendment } from "./ExecPolicyAmendment";
|
||||
|
||||
export type CommandExecutionRequestApprovalParams = { threadId: string, turnId: string, itemId: string,
|
||||
/**
|
||||
* Unique identifier for this specific approval callback.
|
||||
*
|
||||
* For regular shell/unified_exec approvals, this is null.
|
||||
*
|
||||
* For zsh-exec-bridge subcommand approvals, multiple callbacks can belong to
|
||||
* one parent `itemId`, so `approvalId` is a distinct opaque callback id
|
||||
* (a UUID) used to disambiguate routing.
|
||||
*/
|
||||
approvalId?: string | null,
|
||||
/**
|
||||
* Optional explanatory reason (e.g. request for network access).
|
||||
*/
|
||||
|
||||
@@ -1003,6 +1003,7 @@ mod tests {
|
||||
let params = v1::ExecCommandApprovalParams {
|
||||
conversation_id,
|
||||
call_id: "call-42".to_string(),
|
||||
approval_id: Some("approval-42".to_string()),
|
||||
command: vec!["echo".to_string(), "hello".to_string()],
|
||||
cwd: PathBuf::from("/tmp"),
|
||||
reason: Some("because tests".to_string()),
|
||||
@@ -1022,6 +1023,7 @@ mod tests {
|
||||
"params": {
|
||||
"conversationId": "67e55044-10b1-426f-9247-bb680e5fe0c8",
|
||||
"callId": "call-42",
|
||||
"approvalId": "approval-42",
|
||||
"command": ["echo", "hello"],
|
||||
"cwd": "/tmp",
|
||||
"reason": "because tests",
|
||||
|
||||
@@ -258,6 +258,8 @@ pub struct ExecCommandApprovalParams {
|
||||
/// Use to correlate this with [codex_core::protocol::ExecCommandBeginEvent]
|
||||
/// and [codex_core::protocol::ExecCommandEndEvent].
|
||||
pub call_id: String,
|
||||
/// Identifier for this specific approval callback.
|
||||
pub approval_id: Option<String>,
|
||||
pub command: Vec<String>,
|
||||
pub cwd: PathBuf,
|
||||
pub reason: Option<String>,
|
||||
|
||||
@@ -3181,7 +3181,18 @@ pub struct CommandExecutionRequestApprovalParams {
|
||||
pub thread_id: String,
|
||||
pub turn_id: String,
|
||||
pub item_id: String,
|
||||
/// Unique identifier for this specific approval callback.
|
||||
///
|
||||
/// For regular shell/unified_exec approvals, this is null.
|
||||
///
|
||||
/// For zsh-exec-bridge subcommand approvals, multiple callbacks can belong to
|
||||
/// one parent `itemId`, so `approvalId` is a distinct opaque callback id
|
||||
/// (a UUID) used to disambiguate routing.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional = nullable)]
|
||||
pub approval_id: Option<String>,
|
||||
/// Optional explanatory reason (e.g. request for network access).
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional = nullable)]
|
||||
pub reason: Option<String>,
|
||||
/// The command to be executed.
|
||||
@@ -3197,6 +3208,7 @@ pub struct CommandExecutionRequestApprovalParams {
|
||||
#[ts(optional = nullable)]
|
||||
pub command_actions: Option<Vec<CommandAction>>,
|
||||
/// Optional proposed execpolicy amendment to allow similar commands without prompting.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional = nullable)]
|
||||
pub proposed_execpolicy_amendment: Option<ExecPolicyAmendment>,
|
||||
}
|
||||
|
||||
@@ -1302,6 +1302,7 @@ impl CodexClient {
|
||||
thread_id,
|
||||
turn_id,
|
||||
item_id,
|
||||
approval_id,
|
||||
reason,
|
||||
command,
|
||||
cwd,
|
||||
@@ -1310,7 +1311,8 @@ impl CodexClient {
|
||||
} = params;
|
||||
|
||||
println!(
|
||||
"\n< commandExecution approval requested for thread {thread_id}, turn {turn_id}, item {item_id}"
|
||||
"\n< commandExecution approval requested for thread {thread_id}, turn {turn_id}, item {item_id}, approval {}",
|
||||
approval_id.as_deref().unwrap_or("<none>")
|
||||
);
|
||||
if let Some(reason) = reason.as_deref() {
|
||||
println!("< reason: {reason}");
|
||||
|
||||
@@ -637,7 +637,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`, optionally `reason`, plus `command`, `cwd`, and `commandActions` for friendly display.
|
||||
2. `item/commandExecution/requestApproval` (request) — carries the same `itemId`, `threadId`, `turnId`, optionally `approvalId` (for subcommand callbacks), `reason`, plus `command`, `cwd`, and `commandActions` for friendly display.
|
||||
3. Client response — `{ "decision": "accept", "acceptSettings": { "forSession": false } }` or `{ "decision": "decline" }`.
|
||||
4. `item/completed` — final `commandExecution` item with `status: "completed" | "failed" | "declined"` and execution output. Render this as the authoritative result.
|
||||
|
||||
|
||||
@@ -214,76 +214,88 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
});
|
||||
}
|
||||
},
|
||||
EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent {
|
||||
call_id,
|
||||
turn_id,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
proposed_execpolicy_amendment,
|
||||
parsed_cmd,
|
||||
..
|
||||
}) => match api_version {
|
||||
ApiVersion::V1 => {
|
||||
let params = ExecCommandApprovalParams {
|
||||
conversation_id,
|
||||
call_id: call_id.clone(),
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
parsed_cmd,
|
||||
};
|
||||
let rx = outgoing
|
||||
.send_request(ServerRequestPayload::ExecCommandApproval(params))
|
||||
.await;
|
||||
tokio::spawn(async move {
|
||||
on_exec_approval_response(call_id, event_turn_id, rx, conversation).await;
|
||||
});
|
||||
}
|
||||
ApiVersion::V2 => {
|
||||
let item_id = call_id.clone();
|
||||
let command_actions = parsed_cmd
|
||||
.iter()
|
||||
.cloned()
|
||||
.map(V2ParsedCommand::from)
|
||||
.collect::<Vec<_>>();
|
||||
let command_string = shlex_join(&command);
|
||||
let proposed_execpolicy_amendment_v2 =
|
||||
proposed_execpolicy_amendment.map(V2ExecPolicyAmendment::from);
|
||||
|
||||
let params = CommandExecutionRequestApprovalParams {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: turn_id.clone(),
|
||||
// Until we migrate the core to be aware of a first class CommandExecutionItem
|
||||
// and emit the corresponding EventMsg, we repurpose the call_id as the item_id.
|
||||
item_id: item_id.clone(),
|
||||
reason,
|
||||
command: Some(command_string.clone()),
|
||||
cwd: Some(cwd.clone()),
|
||||
command_actions: Some(command_actions.clone()),
|
||||
proposed_execpolicy_amendment: proposed_execpolicy_amendment_v2,
|
||||
};
|
||||
let rx = outgoing
|
||||
.send_request(ServerRequestPayload::CommandExecutionRequestApproval(
|
||||
params,
|
||||
))
|
||||
.await;
|
||||
tokio::spawn(async move {
|
||||
on_command_execution_request_approval_response(
|
||||
event_turn_id,
|
||||
EventMsg::ExecApprovalRequest(ev) => {
|
||||
let approval_id_for_op = ev.effective_approval_id();
|
||||
let ExecApprovalRequestEvent {
|
||||
call_id,
|
||||
approval_id,
|
||||
turn_id,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
proposed_execpolicy_amendment,
|
||||
parsed_cmd,
|
||||
..
|
||||
} = ev;
|
||||
match api_version {
|
||||
ApiVersion::V1 => {
|
||||
let params = ExecCommandApprovalParams {
|
||||
conversation_id,
|
||||
item_id,
|
||||
command_string,
|
||||
call_id: call_id.clone(),
|
||||
approval_id,
|
||||
command,
|
||||
cwd,
|
||||
command_actions,
|
||||
rx,
|
||||
conversation,
|
||||
outgoing,
|
||||
)
|
||||
.await;
|
||||
});
|
||||
reason,
|
||||
parsed_cmd,
|
||||
};
|
||||
let rx = outgoing
|
||||
.send_request(ServerRequestPayload::ExecCommandApproval(params))
|
||||
.await;
|
||||
tokio::spawn(async move {
|
||||
on_exec_approval_response(
|
||||
approval_id_for_op,
|
||||
event_turn_id,
|
||||
rx,
|
||||
conversation,
|
||||
)
|
||||
.await;
|
||||
});
|
||||
}
|
||||
ApiVersion::V2 => {
|
||||
let command_actions = parsed_cmd
|
||||
.iter()
|
||||
.cloned()
|
||||
.map(V2ParsedCommand::from)
|
||||
.collect::<Vec<_>>();
|
||||
let command_string = shlex_join(&command);
|
||||
let proposed_execpolicy_amendment_v2 =
|
||||
proposed_execpolicy_amendment.map(V2ExecPolicyAmendment::from);
|
||||
|
||||
let params = CommandExecutionRequestApprovalParams {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: turn_id.clone(),
|
||||
item_id: call_id.clone(),
|
||||
approval_id: approval_id.clone(),
|
||||
reason,
|
||||
command: Some(command_string.clone()),
|
||||
cwd: Some(cwd.clone()),
|
||||
command_actions: Some(command_actions.clone()),
|
||||
proposed_execpolicy_amendment: proposed_execpolicy_amendment_v2,
|
||||
};
|
||||
let rx = outgoing
|
||||
.send_request(ServerRequestPayload::CommandExecutionRequestApproval(
|
||||
params,
|
||||
))
|
||||
.await;
|
||||
tokio::spawn(async move {
|
||||
on_command_execution_request_approval_response(
|
||||
event_turn_id,
|
||||
conversation_id,
|
||||
approval_id,
|
||||
call_id,
|
||||
command_string,
|
||||
cwd,
|
||||
command_actions,
|
||||
rx,
|
||||
conversation,
|
||||
outgoing,
|
||||
thread_state.clone(),
|
||||
)
|
||||
.await;
|
||||
});
|
||||
}
|
||||
}
|
||||
},
|
||||
}
|
||||
EventMsg::RequestUserInput(request) => {
|
||||
if matches!(api_version, ApiVersion::V2) {
|
||||
let questions = request
|
||||
@@ -933,6 +945,14 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
let cwd = exec_command_begin_event.cwd;
|
||||
let process_id = exec_command_begin_event.process_id;
|
||||
|
||||
{
|
||||
let mut state = thread_state.lock().await;
|
||||
state
|
||||
.turn_summary
|
||||
.command_execution_started
|
||||
.insert(item_id.clone());
|
||||
}
|
||||
|
||||
let item = ThreadItem::CommandExecution {
|
||||
id: item_id,
|
||||
command,
|
||||
@@ -1020,6 +1040,14 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
..
|
||||
} = exec_command_end_event;
|
||||
|
||||
{
|
||||
let mut state = thread_state.lock().await;
|
||||
state
|
||||
.turn_summary
|
||||
.command_execution_started
|
||||
.remove(&call_id);
|
||||
}
|
||||
|
||||
let status: CommandExecutionStatus = (&status).into();
|
||||
let command_actions = parsed_cmd
|
||||
.into_iter()
|
||||
@@ -1739,6 +1767,7 @@ async fn on_file_change_request_approval_response(
|
||||
async fn on_command_execution_request_approval_response(
|
||||
event_turn_id: String,
|
||||
conversation_id: ThreadId,
|
||||
approval_id: Option<String>,
|
||||
item_id: String,
|
||||
command: String,
|
||||
cwd: PathBuf,
|
||||
@@ -1746,6 +1775,7 @@ async fn on_command_execution_request_approval_response(
|
||||
receiver: oneshot::Receiver<ClientRequestResult>,
|
||||
conversation: Arc<CodexThread>,
|
||||
outgoing: ThreadScopedOutgoingMessageSender,
|
||||
thread_state: Arc<Mutex<ThreadState>>,
|
||||
) {
|
||||
let response = receiver.await;
|
||||
let (decision, completion_status) = match response {
|
||||
@@ -1794,7 +1824,24 @@ async fn on_command_execution_request_approval_response(
|
||||
}
|
||||
};
|
||||
|
||||
if let Some(status) = completion_status {
|
||||
let suppress_subcommand_completion_item = {
|
||||
// For regular shell/unified_exec approvals, approval_id is null.
|
||||
// For zsh-fork subcommand approvals, approval_id is present and
|
||||
// item_id points to the parent command item.
|
||||
if approval_id.is_some() {
|
||||
let state = thread_state.lock().await;
|
||||
state
|
||||
.turn_summary
|
||||
.command_execution_started
|
||||
.contains(&item_id)
|
||||
} else {
|
||||
false
|
||||
}
|
||||
};
|
||||
|
||||
if let Some(status) = completion_status
|
||||
&& !suppress_subcommand_completion_item
|
||||
{
|
||||
complete_command_execution_item(
|
||||
conversation_id,
|
||||
event_turn_id.clone(),
|
||||
@@ -1811,7 +1858,7 @@ async fn on_command_execution_request_approval_response(
|
||||
|
||||
if let Err(err) = conversation
|
||||
.submit(Op::ExecApproval {
|
||||
id: item_id,
|
||||
id: approval_id.unwrap_or_else(|| item_id.clone()),
|
||||
turn_id: Some(event_turn_id),
|
||||
decision,
|
||||
})
|
||||
|
||||
@@ -20,6 +20,7 @@ type PendingInterruptQueue = Vec<(
|
||||
#[derive(Default, Clone)]
|
||||
pub(crate) struct TurnSummary {
|
||||
pub(crate) file_change_started: HashSet<String>,
|
||||
pub(crate) command_execution_started: HashSet<String>,
|
||||
pub(crate) last_error: Option<TurnError>,
|
||||
}
|
||||
|
||||
|
||||
@@ -298,6 +298,7 @@ async fn test_send_user_turn_changes_approval_policy_behavior() -> Result<()> {
|
||||
ExecCommandApprovalParams {
|
||||
conversation_id,
|
||||
call_id: "call1".to_string(),
|
||||
approval_id: None,
|
||||
command: format_with_current_shell("python3 -c 'print(42)'"),
|
||||
cwd: working_directory.clone(),
|
||||
reason: None,
|
||||
|
||||
@@ -2155,7 +2155,7 @@ impl Session {
|
||||
|
||||
/// Emit an exec approval request event and await the user's decision.
|
||||
///
|
||||
/// The request is keyed by `call_id` so matching responses are delivered
|
||||
/// The request is keyed by `call_id` + `approval_id` so matching responses are delivered
|
||||
/// to the correct in-flight turn. If the task is aborted, this returns the
|
||||
/// default `ReviewDecision` (`Denied`).
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
@@ -2163,32 +2163,36 @@ impl Session {
|
||||
&self,
|
||||
turn_context: &TurnContext,
|
||||
call_id: String,
|
||||
approval_id: Option<String>,
|
||||
command: Vec<String>,
|
||||
cwd: PathBuf,
|
||||
reason: Option<String>,
|
||||
network_approval_context: Option<NetworkApprovalContext>,
|
||||
proposed_execpolicy_amendment: Option<ExecPolicyAmendment>,
|
||||
) -> ReviewDecision {
|
||||
// command-level approvals use `call_id`.
|
||||
// `approval_id` is only present for subcommand callbacks (execve intercept)
|
||||
let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone());
|
||||
// Add the tx_approve callback to the map before sending the request.
|
||||
let (tx_approve, rx_approve) = oneshot::channel();
|
||||
let approval_id = call_id.clone();
|
||||
let prev_entry = {
|
||||
let mut active = self.active_turn.lock().await;
|
||||
match active.as_mut() {
|
||||
Some(at) => {
|
||||
let mut ts = at.turn_state.lock().await;
|
||||
ts.insert_pending_approval(approval_id.clone(), tx_approve)
|
||||
ts.insert_pending_approval(effective_approval_id.clone(), tx_approve)
|
||||
}
|
||||
None => None,
|
||||
}
|
||||
};
|
||||
if prev_entry.is_some() {
|
||||
warn!("Overwriting existing pending approval for call_id: {approval_id}");
|
||||
warn!("Overwriting existing pending approval for call_id: {effective_approval_id}");
|
||||
}
|
||||
|
||||
let parsed_cmd = parse_command(&command);
|
||||
let event = EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent {
|
||||
call_id,
|
||||
approval_id,
|
||||
turn_id: turn_context.sub_id.clone(),
|
||||
command,
|
||||
cwd,
|
||||
|
||||
@@ -311,8 +311,10 @@ async fn handle_exec_approval(
|
||||
event: ExecApprovalRequestEvent,
|
||||
cancel_token: &CancellationToken,
|
||||
) {
|
||||
let approval_id_for_op = event.effective_approval_id();
|
||||
let ExecApprovalRequestEvent {
|
||||
call_id,
|
||||
approval_id,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
@@ -320,23 +322,28 @@ async fn handle_exec_approval(
|
||||
proposed_execpolicy_amendment,
|
||||
..
|
||||
} = event;
|
||||
let approval_id = call_id.clone();
|
||||
// Race approval with cancellation and timeout to avoid hangs.
|
||||
let approval_fut = parent_session.request_command_approval(
|
||||
parent_ctx,
|
||||
call_id,
|
||||
approval_id,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
network_approval_context,
|
||||
proposed_execpolicy_amendment,
|
||||
);
|
||||
let decision =
|
||||
await_approval_with_cancel(approval_fut, parent_session, &approval_id, cancel_token).await;
|
||||
let decision = await_approval_with_cancel(
|
||||
approval_fut,
|
||||
parent_session,
|
||||
&approval_id_for_op,
|
||||
cancel_token,
|
||||
)
|
||||
.await;
|
||||
|
||||
let _ = codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval_id,
|
||||
id: approval_id_for_op,
|
||||
turn_id: Some(turn_id),
|
||||
decision,
|
||||
})
|
||||
|
||||
@@ -247,6 +247,7 @@ impl NetworkApprovalService {
|
||||
.request_command_approval(
|
||||
turn_context.as_ref(),
|
||||
attempt.call_id.clone(),
|
||||
None,
|
||||
attempt.command.clone(),
|
||||
attempt.cwd.clone(),
|
||||
Some(format!(
|
||||
|
||||
@@ -109,6 +109,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
.request_command_approval(
|
||||
turn,
|
||||
call_id,
|
||||
None,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
|
||||
@@ -110,6 +110,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
.request_command_approval(
|
||||
turn,
|
||||
call_id,
|
||||
None,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
|
||||
@@ -1603,7 +1603,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
|
||||
}
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.call_id,
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: decision.clone(),
|
||||
})
|
||||
@@ -1823,7 +1823,7 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts
|
||||
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.call_id,
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::ApprovedExecpolicyAmendment {
|
||||
proposed_execpolicy_amendment: expected_execpolicy_amendment.clone(),
|
||||
@@ -1988,7 +1988,7 @@ async fn compound_command_with_one_safe_command_still_requires_approval() -> Res
|
||||
let approval = expect_exec_approval(&test, expected_command.as_str()).await;
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.call_id,
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Denied,
|
||||
})
|
||||
|
||||
@@ -100,7 +100,7 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() {
|
||||
// Approve via parent using the emitted approval call ID.
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.call_id,
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Approved,
|
||||
})
|
||||
|
||||
@@ -1042,7 +1042,7 @@ async fn handle_container_exec_user_approved_records_tool_decision() {
|
||||
|
||||
codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.call_id,
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Approved,
|
||||
})
|
||||
@@ -1108,7 +1108,7 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision()
|
||||
|
||||
codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.call_id,
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::ApprovedForSession,
|
||||
})
|
||||
@@ -1174,7 +1174,7 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() {
|
||||
|
||||
codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.call_id,
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Approved,
|
||||
})
|
||||
@@ -1240,7 +1240,7 @@ async fn handle_container_exec_user_denies_records_tool_decision() {
|
||||
|
||||
codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.call_id,
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Denied,
|
||||
})
|
||||
@@ -1306,7 +1306,7 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision()
|
||||
|
||||
codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.call_id,
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::ApprovedForSession,
|
||||
})
|
||||
@@ -1373,7 +1373,7 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() {
|
||||
|
||||
codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.call_id,
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Denied,
|
||||
})
|
||||
|
||||
@@ -146,7 +146,7 @@ Example:
|
||||
When Codex needs approval to apply changes or run commands, the server issues JSON‑RPC requests to the client:
|
||||
|
||||
- `applyPatchApproval { conversationId, callId, fileChanges, reason?, grantRoot? }`
|
||||
- `execCommandApproval { conversationId, callId, command, cwd, reason? }`
|
||||
- `execCommandApproval { conversationId, callId, approvalId?, command, cwd, reason? }`
|
||||
|
||||
The client must reply with `{ decision: "allow" | "deny" }` for each request.
|
||||
|
||||
|
||||
@@ -213,16 +213,19 @@ async fn run_codex_tool_session_inner(
|
||||
.await;
|
||||
|
||||
match event.msg {
|
||||
EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent {
|
||||
turn_id: _,
|
||||
command,
|
||||
cwd,
|
||||
call_id,
|
||||
reason: _,
|
||||
proposed_execpolicy_amendment: _,
|
||||
parsed_cmd,
|
||||
network_approval_context: _,
|
||||
}) => {
|
||||
EventMsg::ExecApprovalRequest(ev) => {
|
||||
let approval_id = ev.effective_approval_id();
|
||||
let ExecApprovalRequestEvent {
|
||||
turn_id: _,
|
||||
command,
|
||||
cwd,
|
||||
call_id,
|
||||
approval_id: _,
|
||||
reason: _,
|
||||
proposed_execpolicy_amendment: _,
|
||||
parsed_cmd,
|
||||
network_approval_context: _,
|
||||
} = ev;
|
||||
handle_exec_approval_request(
|
||||
command,
|
||||
cwd,
|
||||
@@ -232,6 +235,7 @@ async fn run_codex_tool_session_inner(
|
||||
request_id_str.clone(),
|
||||
event.id.clone(),
|
||||
call_id,
|
||||
approval_id,
|
||||
parsed_cmd,
|
||||
thread_id,
|
||||
)
|
||||
@@ -254,9 +258,6 @@ async fn run_codex_tool_session_inner(
|
||||
EventMsg::Warning(_) => {
|
||||
continue;
|
||||
}
|
||||
EventMsg::ModelReroute(_) => {
|
||||
continue;
|
||||
}
|
||||
EventMsg::ElicitationRequest(_) => {
|
||||
// TODO: forward elicitation requests to the client?
|
||||
continue;
|
||||
@@ -361,6 +362,7 @@ async fn run_codex_tool_session_inner(
|
||||
| EventMsg::RequestUserInput(_)
|
||||
| EventMsg::DynamicToolCallRequest(_)
|
||||
| EventMsg::ContextCompacted(_)
|
||||
| EventMsg::ModelReroute(_)
|
||||
| EventMsg::ThreadRolledBack(_)
|
||||
| EventMsg::CollabAgentSpawnBegin(_)
|
||||
| EventMsg::CollabAgentSpawnEnd(_)
|
||||
|
||||
@@ -57,10 +57,10 @@ pub(crate) async fn handle_exec_approval_request(
|
||||
tool_call_id: String,
|
||||
event_id: String,
|
||||
call_id: String,
|
||||
approval_id: String,
|
||||
codex_parsed_cmd: Vec<ParsedCommand>,
|
||||
thread_id: ThreadId,
|
||||
) {
|
||||
let approval_id = call_id.clone();
|
||||
let escaped_command =
|
||||
shlex::try_join(command.iter().map(String::as_str)).unwrap_or_else(|_| command.join(" "));
|
||||
let message = format!(
|
||||
|
||||
@@ -57,8 +57,15 @@ pub struct NetworkApprovalContext {
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)]
|
||||
pub struct ExecApprovalRequestEvent {
|
||||
/// Identifier for the associated exec call, if available.
|
||||
/// Identifier for the associated command execution item.
|
||||
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).
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub approval_id: Option<String>,
|
||||
/// Turn ID that this command belongs to.
|
||||
/// Uses `#[serde(default)]` for backwards compatibility.
|
||||
#[serde(default)]
|
||||
@@ -81,6 +88,14 @@ pub struct ExecApprovalRequestEvent {
|
||||
pub parsed_cmd: Vec<ParsedCommand>,
|
||||
}
|
||||
|
||||
impl ExecApprovalRequestEvent {
|
||||
pub fn effective_approval_id(&self) -> String {
|
||||
self.approval_id
|
||||
.clone()
|
||||
.unwrap_or_else(|| self.call_id.clone())
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)]
|
||||
pub struct ElicitationRequestEvent {
|
||||
pub server_name: String,
|
||||
|
||||
@@ -2472,6 +2472,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()),
|
||||
turn_id: "turn-short".into(),
|
||||
command: vec!["bash".into(), "-lc".into(), "echo hello world".into()],
|
||||
cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
|
||||
@@ -2517,6 +2518,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()),
|
||||
turn_id: "turn-multi".into(),
|
||||
command: vec!["bash".into(), "-lc".into(), "echo line1\necho line2".into()],
|
||||
cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
|
||||
@@ -2570,6 +2572,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()),
|
||||
turn_id: "turn-long".into(),
|
||||
command: vec!["bash".into(), "-lc".into(), long],
|
||||
cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
|
||||
@@ -5684,6 +5687,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()),
|
||||
turn_id: "turn-approve-cmd".into(),
|
||||
command: vec!["bash".into(), "-lc".into(), "echo hello world".into()],
|
||||
cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
|
||||
@@ -5743,6 +5747,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()),
|
||||
turn_id: "turn-approve-cmd-noreason".into(),
|
||||
command: vec!["bash".into(), "-lc".into(), "echo hello world".into()],
|
||||
cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
|
||||
@@ -5791,6 +5796,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()),
|
||||
turn_id: "turn-approve-cmd-multiline-trunc".into(),
|
||||
command: command.clone(),
|
||||
cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
|
||||
@@ -6149,6 +6155,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()),
|
||||
turn_id: "turn-approve-exec".into(),
|
||||
command: vec!["echo".into(), "hello world".into()],
|
||||
cwd: PathBuf::from("/tmp"),
|
||||
|
||||
Reference in New Issue
Block a user