mirror of
https://github.com/openai/codex.git
synced 2026-11-03 19:53:25 +00:00
Prepare clients for repeated command approvals
This commit is contained in:
@@ -969,15 +969,12 @@ impl AnalyticsReducer {
|
||||
.and_then(|network| network.enabled)
|
||||
.unwrap_or(false);
|
||||
let requested_additional_permissions = params.additional_permissions.is_some();
|
||||
let trigger = if params.approval_id.is_some() {
|
||||
ReviewTrigger::ExecveIntercept
|
||||
} else if requested_network_access {
|
||||
ReviewTrigger::NetworkPolicyDenial
|
||||
} else if requested_additional_permissions {
|
||||
ReviewTrigger::SandboxDenial
|
||||
} else {
|
||||
ReviewTrigger::Initial
|
||||
};
|
||||
let trigger = command_execution_review_trigger(
|
||||
params.approval_id.as_deref(),
|
||||
params.reason.as_deref(),
|
||||
requested_additional_permissions,
|
||||
requested_network_access,
|
||||
);
|
||||
let Some(started_at_ms) = option_i64_to_u64(Some(params.started_at_ms)) else {
|
||||
return;
|
||||
};
|
||||
@@ -2320,6 +2317,33 @@ fn guardian_review_subject_metadata(
|
||||
}
|
||||
}
|
||||
|
||||
fn command_execution_review_trigger(
|
||||
approval_id: Option<&str>,
|
||||
reason: Option<&str>,
|
||||
requested_additional_permissions: bool,
|
||||
requested_network_access: bool,
|
||||
) -> ReviewTrigger {
|
||||
if requested_network_access {
|
||||
return ReviewTrigger::NetworkPolicyDenial;
|
||||
}
|
||||
|
||||
// Both execve interception and a command's unsandboxed retry use a
|
||||
// callback-specific approval ID. A retry also carries the sandbox denial
|
||||
// reason, while an execve interception does not. Initial command prompts
|
||||
// can carry a justification, but do not have a callback-specific ID.
|
||||
if approval_id.is_some() {
|
||||
if reason.is_some() {
|
||||
ReviewTrigger::SandboxDenial
|
||||
} else {
|
||||
ReviewTrigger::ExecveIntercept
|
||||
}
|
||||
} else if requested_additional_permissions {
|
||||
ReviewTrigger::SandboxDenial
|
||||
} else {
|
||||
ReviewTrigger::Initial
|
||||
}
|
||||
}
|
||||
|
||||
fn guardian_review_requested_additional_permissions(action: &GuardianApprovalReviewAction) -> bool {
|
||||
match action {
|
||||
GuardianApprovalReviewAction::ApplyPatch { .. }
|
||||
@@ -2843,4 +2867,50 @@ mod tests {
|
||||
Some((ReviewStatus::TimedOut, ReviewResolution::None))
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn command_execution_review_trigger_distinguishes_retry_from_execve() {
|
||||
assert!(matches!(
|
||||
command_execution_review_trigger(
|
||||
Some("retry-id"),
|
||||
Some("sandbox denied"),
|
||||
false,
|
||||
false
|
||||
),
|
||||
ReviewTrigger::SandboxDenial
|
||||
));
|
||||
assert!(matches!(
|
||||
command_execution_review_trigger(Some("execve-id"), None, false, false),
|
||||
ReviewTrigger::ExecveIntercept
|
||||
));
|
||||
assert!(matches!(
|
||||
command_execution_review_trigger(None, Some("initial justification"), false, false),
|
||||
ReviewTrigger::Initial
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn command_execution_review_trigger_uses_semantic_precedence() {
|
||||
assert!(matches!(
|
||||
command_execution_review_trigger(
|
||||
Some("network-id"),
|
||||
Some("network denied"),
|
||||
true,
|
||||
true,
|
||||
),
|
||||
ReviewTrigger::NetworkPolicyDenial
|
||||
));
|
||||
assert!(matches!(
|
||||
command_execution_review_trigger(Some("permissions-id"), None, true, false),
|
||||
ReviewTrigger::ExecveIntercept
|
||||
));
|
||||
assert!(matches!(
|
||||
command_execution_review_trigger(None, None, true, false),
|
||||
ReviewTrigger::SandboxDenial
|
||||
));
|
||||
assert!(matches!(
|
||||
command_execution_review_trigger(None, None, false, false),
|
||||
ReviewTrigger::Initial
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
+1
-1
@@ -521,7 +521,7 @@
|
||||
},
|
||||
"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.",
|
||||
"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.",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
|
||||
+1
-1
@@ -345,7 +345,7 @@
|
||||
"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.",
|
||||
"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.",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
|
||||
+1
-1
@@ -2375,7 +2375,7 @@
|
||||
"$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.",
|
||||
"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.",
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
|
||||
Generated
+4
-4
@@ -13,11 +13,11 @@ export type CommandExecutionRequestApprovalParams = {threadId: string, turnId: s
|
||||
startedAtMs: number, /**
|
||||
* Unique identifier for this specific approval callback.
|
||||
*
|
||||
* For regular shell/unified_exec approvals, this is null.
|
||||
* For a command item with only one approval callback, this is absent.
|
||||
*
|
||||
* 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.
|
||||
* 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.
|
||||
*/
|
||||
approvalId?: string | null, /**
|
||||
* Environment in which the command will run.
|
||||
|
||||
@@ -1446,11 +1446,11 @@ pub struct CommandExecutionRequestApprovalParams {
|
||||
pub started_at_ms: i64,
|
||||
/// Unique identifier for this specific approval callback.
|
||||
///
|
||||
/// For regular shell/unified_exec approvals, this is null.
|
||||
/// For a command item with only one approval callback, this is absent.
|
||||
///
|
||||
/// 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.
|
||||
/// 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.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional = nullable)]
|
||||
pub approval_id: Option<String>,
|
||||
|
||||
@@ -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` (for subcommand callbacks), and `reason`. 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. `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.
|
||||
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.
|
||||
|
||||
@@ -614,9 +614,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
Some(completion_item),
|
||||
),
|
||||
};
|
||||
if approval_id.is_none()
|
||||
&& let Some(completion_item) = completion_item.as_ref()
|
||||
{
|
||||
let started_now = if let Some(completion_item) = completion_item.as_ref() {
|
||||
start_command_execution_item(
|
||||
&conversation_id,
|
||||
event_turn_id.clone(),
|
||||
@@ -628,8 +626,10 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
&outgoing,
|
||||
&thread_state,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
.await
|
||||
} else {
|
||||
false
|
||||
};
|
||||
let proposed_execpolicy_amendment_v2 =
|
||||
proposed_execpolicy_amendment.map(V2ExecPolicyAmendment::from);
|
||||
let proposed_network_policy_amendments_v2 =
|
||||
@@ -641,6 +641,10 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
});
|
||||
let additional_permissions =
|
||||
additional_permissions.map(V2AdditionalPermissionProfile::from);
|
||||
// Today execve callbacks omit `reason`, while sandbox retries carry
|
||||
// one. If execve starts carrying a reason, replace this narrow
|
||||
// compatibility discriminator with an explicit callback purpose.
|
||||
let is_sandbox_retry = approval_id.is_some() && reason.is_some();
|
||||
|
||||
let params = CommandExecutionRequestApprovalParams {
|
||||
thread_id: conversation_id.to_string(),
|
||||
@@ -671,6 +675,8 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
approval_id,
|
||||
call_id,
|
||||
completion_item,
|
||||
started_now,
|
||||
is_sandbox_retry,
|
||||
pending_request_id,
|
||||
rx,
|
||||
conversation,
|
||||
@@ -1143,13 +1149,7 @@ 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 mut state = thread_state.lock().await;
|
||||
state
|
||||
.turn_summary
|
||||
.command_execution_started
|
||||
.remove(&call_id);
|
||||
}
|
||||
let should_emit = remove_started_command_execution_item(&thread_state, &call_id).await;
|
||||
if matches!(
|
||||
exec_command_end_event.source,
|
||||
codex_protocol::protocol::ExecCommandSource::UnifiedExecInteraction
|
||||
@@ -1159,6 +1159,12 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
// emitted for unified exec interactions.
|
||||
return;
|
||||
}
|
||||
if !should_emit {
|
||||
// An approval response may have completed the command item
|
||||
// before a late process-end event arrives. Do not emit a
|
||||
// second completion for an item that is no longer active.
|
||||
return;
|
||||
}
|
||||
let notification = item_event_to_server_notification(
|
||||
EventMsg::ExecCommandEnd(exec_command_end_event),
|
||||
&conversation_id.to_string(),
|
||||
@@ -1427,12 +1433,7 @@ async fn complete_command_execution_item(
|
||||
outgoing: &ThreadScopedOutgoingMessageSender,
|
||||
thread_state: &Arc<Mutex<ThreadState>>,
|
||||
) {
|
||||
let should_emit = thread_state
|
||||
.lock()
|
||||
.await
|
||||
.turn_summary
|
||||
.command_execution_started
|
||||
.remove(&item_id);
|
||||
let should_emit = remove_started_command_execution_item(thread_state, &item_id).await;
|
||||
if !should_emit {
|
||||
return;
|
||||
}
|
||||
@@ -1460,6 +1461,18 @@ async fn complete_command_execution_item(
|
||||
.await;
|
||||
}
|
||||
|
||||
async fn remove_started_command_execution_item(
|
||||
thread_state: &Arc<Mutex<ThreadState>>,
|
||||
item_id: &str,
|
||||
) -> bool {
|
||||
thread_state
|
||||
.lock()
|
||||
.await
|
||||
.turn_summary
|
||||
.command_execution_started
|
||||
.remove(item_id)
|
||||
}
|
||||
|
||||
async fn maybe_emit_raw_response_item_completed(
|
||||
conversation_id: ThreadId,
|
||||
turn_id: &str,
|
||||
@@ -2050,6 +2063,8 @@ async fn on_command_execution_request_approval_response(
|
||||
approval_id: Option<String>,
|
||||
item_id: String,
|
||||
completion_item: Option<CommandExecutionCompletionItem>,
|
||||
started_now: bool,
|
||||
is_sandbox_retry: bool,
|
||||
pending_request_id: RequestId,
|
||||
receiver: oneshot::Receiver<ClientRequestResult>,
|
||||
conversation: Arc<CodexThread>,
|
||||
@@ -2060,7 +2075,7 @@ async fn on_command_execution_request_approval_response(
|
||||
let response = receiver.await;
|
||||
resolve_server_request_on_thread_listener(&thread_state, pending_request_id).await;
|
||||
drop(permission_guard);
|
||||
let (decision, completion_status) = match response {
|
||||
let (decision, completion_status, cancel_requested) = match response {
|
||||
Ok(Ok(value)) => {
|
||||
let response = serde_json::from_value::<CommandExecutionRequestApprovalResponse>(value)
|
||||
.unwrap_or_else(|err| {
|
||||
@@ -2072,10 +2087,10 @@ async fn on_command_execution_request_approval_response(
|
||||
|
||||
let decision = response.decision;
|
||||
|
||||
let (decision, completion_status) = match decision {
|
||||
CommandExecutionApprovalDecision::Accept => (ReviewDecision::Approved, None),
|
||||
let (decision, completion_status, cancel_requested) = match decision {
|
||||
CommandExecutionApprovalDecision::Accept => (ReviewDecision::Approved, None, false),
|
||||
CommandExecutionApprovalDecision::AcceptForSession => {
|
||||
(ReviewDecision::ApprovedForSession, None)
|
||||
(ReviewDecision::ApprovedForSession, None, false)
|
||||
}
|
||||
CommandExecutionApprovalDecision::AcceptWithExecpolicyAmendment {
|
||||
execpolicy_amendment,
|
||||
@@ -2084,6 +2099,7 @@ async fn on_command_execution_request_approval_response(
|
||||
proposed_execpolicy_amendment: execpolicy_amendment.into_core(),
|
||||
},
|
||||
None,
|
||||
false,
|
||||
),
|
||||
CommandExecutionApprovalDecision::ApplyNetworkPolicyAmendment {
|
||||
network_policy_amendment,
|
||||
@@ -2097,47 +2113,56 @@ async fn on_command_execution_request_approval_response(
|
||||
network_policy_amendment: network_policy_amendment.into_core(),
|
||||
},
|
||||
completion_status,
|
||||
false,
|
||||
)
|
||||
}
|
||||
CommandExecutionApprovalDecision::Decline => (
|
||||
ReviewDecision::Denied,
|
||||
Some(CommandExecutionStatus::Declined),
|
||||
false,
|
||||
),
|
||||
CommandExecutionApprovalDecision::Cancel => (
|
||||
ReviewDecision::Abort,
|
||||
Some(CommandExecutionStatus::Declined),
|
||||
true,
|
||||
),
|
||||
};
|
||||
(decision, completion_status)
|
||||
(decision, completion_status, cancel_requested)
|
||||
}
|
||||
Ok(Err(err)) if is_turn_transition_server_request_error(&err) => return,
|
||||
Ok(Err(err)) => {
|
||||
error!("request failed with client error: {err:?}");
|
||||
(ReviewDecision::Denied, Some(CommandExecutionStatus::Failed))
|
||||
(
|
||||
ReviewDecision::Denied,
|
||||
Some(CommandExecutionStatus::Failed),
|
||||
false,
|
||||
)
|
||||
}
|
||||
Err(err) => {
|
||||
error!("request failed: {err:?}");
|
||||
(ReviewDecision::Denied, Some(CommandExecutionStatus::Failed))
|
||||
(
|
||||
ReviewDecision::Denied,
|
||||
Some(CommandExecutionStatus::Failed),
|
||||
false,
|
||||
)
|
||||
}
|
||||
};
|
||||
|
||||
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
|
||||
}
|
||||
};
|
||||
// A non-null approval_id can identify either an execve callback for a
|
||||
// command item that is still active (for example, zsh-exec-bridge) or an
|
||||
// unsandboxed retry. Suppress duplicate completion only for a non-canceling
|
||||
// callback of the former kind. A declined retry must complete the item even
|
||||
// when its initial approval already started it and no begin/end event ran
|
||||
// before the sandbox denial; cancel must also close the item before aborting.
|
||||
let suppress_repeated_prompt_completion_item = suppress_repeated_prompt_completion_item(
|
||||
approval_id.as_deref(),
|
||||
started_now,
|
||||
is_sandbox_retry,
|
||||
cancel_requested,
|
||||
);
|
||||
|
||||
if let Some(status) = completion_status
|
||||
&& !suppress_subcommand_completion_item
|
||||
&& !suppress_repeated_prompt_completion_item
|
||||
&& let Some(completion_item) = completion_item
|
||||
{
|
||||
complete_command_execution_item(
|
||||
@@ -2168,6 +2193,15 @@ async fn on_command_execution_request_approval_response(
|
||||
}
|
||||
}
|
||||
|
||||
fn suppress_repeated_prompt_completion_item(
|
||||
approval_id: Option<&str>,
|
||||
started_now: bool,
|
||||
is_sandbox_retry: bool,
|
||||
cancel_requested: bool,
|
||||
) -> bool {
|
||||
approval_id.is_some() && !started_now && !is_sandbox_retry && !cancel_requested
|
||||
}
|
||||
|
||||
fn now_unix_timestamp_ms() -> i64 {
|
||||
SystemTime::now()
|
||||
.duration_since(UNIX_EPOCH)
|
||||
@@ -2647,8 +2681,133 @@ mod tests {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn complete_command_execution_item_emits_declined_once_for_pending_command() -> Result<()>
|
||||
{
|
||||
async fn active_item_retry_decline_completes_but_execve_decline_is_suppressed() -> Result<()> {
|
||||
let conversation_id = ThreadId::new();
|
||||
let thread_state = new_thread_state();
|
||||
let (tx, mut rx) = mpsc::channel(CHANNEL_CAPACITY);
|
||||
let outgoing = Arc::new(OutgoingMessageSender::new(
|
||||
tx,
|
||||
codex_analytics::AnalyticsEventsClient::disabled(),
|
||||
));
|
||||
let outgoing = ThreadScopedOutgoingMessageSender::new(
|
||||
outgoing,
|
||||
vec![ConnectionId(1)],
|
||||
ThreadId::new(),
|
||||
);
|
||||
let completion_item = command_execution_completion_item("printf hi");
|
||||
|
||||
let initial_started = start_command_execution_item(
|
||||
&conversation_id,
|
||||
"turn-1".to_string(),
|
||||
"cmd-1".to_string(),
|
||||
completion_item.command.clone(),
|
||||
completion_item.cwd.clone(),
|
||||
completion_item.command_actions.clone(),
|
||||
CommandExecutionSource::Agent,
|
||||
&outgoing,
|
||||
&thread_state,
|
||||
)
|
||||
.await;
|
||||
assert!(initial_started);
|
||||
let _initial_started = recv_broadcast_message(&mut rx).await?;
|
||||
|
||||
let retry_started = start_command_execution_item(
|
||||
&conversation_id,
|
||||
"turn-1".to_string(),
|
||||
"cmd-1".to_string(),
|
||||
completion_item.command.clone(),
|
||||
completion_item.cwd.clone(),
|
||||
completion_item.command_actions.clone(),
|
||||
CommandExecutionSource::Agent,
|
||||
&outgoing,
|
||||
&thread_state,
|
||||
)
|
||||
.await;
|
||||
assert!(!retry_started);
|
||||
assert!(!suppress_repeated_prompt_completion_item(
|
||||
Some("retry-approval-id"),
|
||||
retry_started,
|
||||
/*is_sandbox_retry*/ true,
|
||||
/*cancel_requested*/ false,
|
||||
));
|
||||
assert!(!suppress_repeated_prompt_completion_item(
|
||||
Some("retry-cancel-approval-id"),
|
||||
retry_started,
|
||||
/*is_sandbox_retry*/ true,
|
||||
/*cancel_requested*/ true,
|
||||
));
|
||||
|
||||
complete_command_execution_item(
|
||||
&conversation_id,
|
||||
"turn-1".to_string(),
|
||||
"cmd-1".to_string(),
|
||||
completion_item.command.clone(),
|
||||
completion_item.cwd.clone(),
|
||||
/*process_id*/ None,
|
||||
CommandExecutionSource::Agent,
|
||||
completion_item.command_actions.clone(),
|
||||
CommandExecutionStatus::Declined,
|
||||
&outgoing,
|
||||
&thread_state,
|
||||
)
|
||||
.await;
|
||||
|
||||
let retry_completed = recv_broadcast_message(&mut rx).await?;
|
||||
match retry_completed {
|
||||
OutgoingMessage::AppServerNotification(ServerNotification::ItemCompleted(payload)) => {
|
||||
let ThreadItem::CommandExecution { id, status, .. } = payload.item else {
|
||||
bail!("expected command execution completion");
|
||||
};
|
||||
assert_eq!(id, "cmd-1");
|
||||
assert_eq!(status, CommandExecutionStatus::Declined);
|
||||
}
|
||||
other => bail!("unexpected message: {other:?}"),
|
||||
}
|
||||
|
||||
let active_started = start_command_execution_item(
|
||||
&conversation_id,
|
||||
"turn-1".to_string(),
|
||||
"cmd-2".to_string(),
|
||||
completion_item.command.clone(),
|
||||
completion_item.cwd.clone(),
|
||||
completion_item.command_actions.clone(),
|
||||
CommandExecutionSource::Agent,
|
||||
&outgoing,
|
||||
&thread_state,
|
||||
)
|
||||
.await;
|
||||
assert!(active_started);
|
||||
let _active_started = recv_broadcast_message(&mut rx).await?;
|
||||
let repeated_started = start_command_execution_item(
|
||||
&conversation_id,
|
||||
"turn-1".to_string(),
|
||||
"cmd-2".to_string(),
|
||||
completion_item.command,
|
||||
completion_item.cwd,
|
||||
completion_item.command_actions,
|
||||
CommandExecutionSource::Agent,
|
||||
&outgoing,
|
||||
&thread_state,
|
||||
)
|
||||
.await;
|
||||
assert!(!repeated_started);
|
||||
assert!(suppress_repeated_prompt_completion_item(
|
||||
Some("zsh-subcommand-approval-id"),
|
||||
repeated_started,
|
||||
/*is_sandbox_retry*/ false,
|
||||
/*cancel_requested*/ false,
|
||||
));
|
||||
assert!(!suppress_repeated_prompt_completion_item(
|
||||
Some("zsh-subcommand-cancel-id"),
|
||||
repeated_started,
|
||||
/*is_sandbox_retry*/ false,
|
||||
/*cancel_requested*/ true,
|
||||
));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn response_completion_suppresses_late_exec_command_end() -> Result<()> {
|
||||
let conversation_id = ThreadId::new();
|
||||
let thread_state = new_thread_state();
|
||||
let (tx, mut rx) = mpsc::channel(CHANNEL_CAPACITY);
|
||||
@@ -2704,23 +2863,12 @@ mod tests {
|
||||
other => bail!("unexpected message: {other:?}"),
|
||||
}
|
||||
|
||||
complete_command_execution_item(
|
||||
&conversation_id,
|
||||
"turn-1".to_string(),
|
||||
"cmd-1".to_string(),
|
||||
completion_item.command,
|
||||
completion_item.cwd,
|
||||
/*process_id*/ None,
|
||||
CommandExecutionSource::Agent,
|
||||
completion_item.command_actions,
|
||||
CommandExecutionStatus::Declined,
|
||||
&outgoing,
|
||||
&thread_state,
|
||||
)
|
||||
.await;
|
||||
let late_end_should_emit =
|
||||
remove_started_command_execution_item(&thread_state, "cmd-1").await;
|
||||
assert!(!late_end_should_emit);
|
||||
assert!(
|
||||
rx.try_recv().is_err(),
|
||||
"completion should not emit after the pending item is cleared"
|
||||
"a late ExecCommandEnd should not emit after response completion"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -340,7 +340,10 @@ fn default_exec_approval_decisions(
|
||||
];
|
||||
}
|
||||
|
||||
let mut decisions = vec![CommandExecutionApprovalDecision::Accept];
|
||||
let mut decisions = vec![
|
||||
CommandExecutionApprovalDecision::Accept,
|
||||
CommandExecutionApprovalDecision::AcceptForSession,
|
||||
];
|
||||
if let Some(execpolicy_amendment) = proposed_execpolicy_amendment {
|
||||
decisions.push(
|
||||
CommandExecutionApprovalDecision::AcceptWithExecpolicyAmendment {
|
||||
|
||||
@@ -769,6 +769,32 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn thread_event_snapshot_keeps_retry_callback_after_initial_item_approval() {
|
||||
let mut store = ThreadEventStore::new(/*capacity*/ 8);
|
||||
store.push_request(exec_approval_request("call-1", None, "turn-1"));
|
||||
store.note_outbound_op(&Op::ExecApproval {
|
||||
id: "call-1".to_string(),
|
||||
turn_id: Some("turn-1".to_string()),
|
||||
decision: CommandExecutionApprovalDecision::Accept,
|
||||
});
|
||||
store.push_request(exec_approval_request(
|
||||
"call-1",
|
||||
Some("retry-approval-1"),
|
||||
"turn-1",
|
||||
));
|
||||
|
||||
let snapshot = store.snapshot();
|
||||
assert_eq!(snapshot.events.len(), 1);
|
||||
assert!(matches!(
|
||||
snapshot.events.first(),
|
||||
Some(ThreadBufferedEvent::Request(
|
||||
ServerRequest::CommandExecutionRequestApproval { params, .. }
|
||||
)) if params.item_id == "call-1"
|
||||
&& params.approval_id.as_deref() == Some("retry-approval-1")
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn thread_event_snapshot_drops_resolved_exec_approval_after_server_resolution() {
|
||||
let mut store = ThreadEventStore::new(/*capacity*/ 8);
|
||||
|
||||
@@ -95,7 +95,10 @@ impl ExecApprovalRequestEvent {
|
||||
];
|
||||
}
|
||||
|
||||
let mut decisions = vec![CommandExecutionApprovalDecision::Accept];
|
||||
let mut decisions = vec![
|
||||
CommandExecutionApprovalDecision::Accept,
|
||||
CommandExecutionApprovalDecision::AcceptForSession,
|
||||
];
|
||||
if let Some(prefix) = proposed_execpolicy_amendment {
|
||||
decisions.push(
|
||||
CommandExecutionApprovalDecision::AcceptWithExecpolicyAmendment {
|
||||
|
||||
+4
-2
@@ -1,5 +1,6 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/exec_flow.rs
|
||||
assertion_line: 1157
|
||||
expression: terminal.backend().vt100().screen().contents()
|
||||
---
|
||||
|
||||
@@ -11,7 +12,8 @@ expression: terminal.backend().vt100().screen().contents()
|
||||
$ echo hello world
|
||||
|
||||
› 1. Yes, proceed (y)
|
||||
2. Yes, and don't ask again for commands that start with `echo hello world` (p)
|
||||
3. No, and tell Codex what to do differently (esc)
|
||||
2. Yes, and don't ask again for this command in this session (a)
|
||||
3. Yes, and don't ask again for commands that start with `echo hello world` (p)
|
||||
4. No, and tell Codex what to do differently (esc)
|
||||
|
||||
Press enter to confirm or esc to cancel
|
||||
|
||||
+3
-1
@@ -1,5 +1,6 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/exec_flow.rs
|
||||
assertion_line: 1249
|
||||
expression: contents
|
||||
---
|
||||
|
||||
@@ -11,6 +12,7 @@ expression: contents
|
||||
PY
|
||||
|
||||
› 1. Yes, proceed (y)
|
||||
2. No, and tell Codex what to do differently (esc)
|
||||
2. Yes, and don't ask again for this command in this session (a)
|
||||
3. No, and tell Codex what to do differently (esc)
|
||||
|
||||
Press enter to confirm or esc to cancel
|
||||
|
||||
+4
-2
@@ -1,5 +1,6 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/exec_flow.rs
|
||||
assertion_line: 1201
|
||||
expression: terminal.backend().vt100().screen().contents()
|
||||
---
|
||||
|
||||
@@ -9,7 +10,8 @@ expression: terminal.backend().vt100().screen().contents()
|
||||
$ echo hello world
|
||||
|
||||
› 1. Yes, proceed (y)
|
||||
2. Yes, and don't ask again for commands that start with `echo hello world` (p)
|
||||
3. No, and tell Codex what to do differently (esc)
|
||||
2. Yes, and don't ask again for this command in this session (a)
|
||||
3. Yes, and don't ask again for commands that start with `echo hello world` (p)
|
||||
4. No, and tell Codex what to do differently (esc)
|
||||
|
||||
Press enter to confirm or esc to cancel
|
||||
|
||||
+9
-6
@@ -1,10 +1,10 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/exec_flow.rs
|
||||
assertion_line: 40
|
||||
assertion_line: 37
|
||||
expression: "format!(\"{buf:?}\")"
|
||||
---
|
||||
Buffer {
|
||||
area: Rect { x: 0, y: 0, width: 80, height: 13 },
|
||||
area: Rect { x: 0, y: 0, width: 80, height: 14 },
|
||||
content: [
|
||||
" ",
|
||||
" ",
|
||||
@@ -16,7 +16,8 @@ Buffer {
|
||||
" $ echo hello world ",
|
||||
" ",
|
||||
"› 1. Yes, proceed (y) ",
|
||||
" 2. No, and tell Codex what to do differently (esc) ",
|
||||
" 2. Yes, and don't ask again for this command in this session (a) ",
|
||||
" 3. No, and tell Codex what to do differently (esc) ",
|
||||
" ",
|
||||
" Press enter to confirm or esc to cancel ",
|
||||
],
|
||||
@@ -33,8 +34,10 @@ Buffer {
|
||||
x: 20, y: 7, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 9, fg: Cyan, bg: Reset, underline: Reset, modifier: BOLD,
|
||||
x: 21, y: 9, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 48, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 51, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 2, y: 12, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 64, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 65, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 48, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 51, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 2, y: 13, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
]
|
||||
}
|
||||
|
||||
+4
-2
@@ -1,5 +1,6 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/status_and_layout.rs
|
||||
assertion_line: 2074
|
||||
expression: normalized_backend_snapshot(terminal.backend())
|
||||
---
|
||||
" "
|
||||
@@ -11,7 +12,8 @@ expression: normalized_backend_snapshot(terminal.backend())
|
||||
" $ echo 'hello world' "
|
||||
" "
|
||||
"› 1. Yes, proceed (y) "
|
||||
" 2. Yes, and don't ask again for commands that start with `echo 'hello world'` (p) "
|
||||
" 3. No, and tell Codex what to do differently (esc) "
|
||||
" 2. Yes, and don't ask again for this command in this session (a) "
|
||||
" 3. Yes, and don't ask again for commands that start with `echo 'hello world'` (p) "
|
||||
" 4. No, and tell Codex what to do differently (esc) "
|
||||
" "
|
||||
" Press enter to confirm or esc to cancel "
|
||||
|
||||
@@ -1244,7 +1244,8 @@ async fn approval_modal_exec_multiline_prefix_hides_execpolicy_option_snapshot()
|
||||
.draw(|f| chat.render(f.area(), f.buffer_mut()))
|
||||
.expect("draw approval modal (multiline prefix)");
|
||||
let contents = terminal.backend().vt100().screen().contents();
|
||||
assert!(!contents.contains("don't ask again"));
|
||||
assert!(contents.contains("don't ask again for this command in this session"));
|
||||
assert!(!contents.contains("don't ask again for commands that start with"));
|
||||
assert_chatwidget_snapshot!(
|
||||
"approval_modal_exec_multiline_prefix_no_execpolicy",
|
||||
contents
|
||||
|
||||
+9
-5
@@ -1,9 +1,10 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/approval_requests.rs
|
||||
assertion_line: 39
|
||||
expression: "format!(\"{buf:?}\")"
|
||||
---
|
||||
Buffer {
|
||||
area: Rect { x: 0, y: 0, width: 80, height: 15 },
|
||||
area: Rect { x: 0, y: 0, width: 80, height: 16 },
|
||||
content: [
|
||||
" ",
|
||||
" ",
|
||||
@@ -17,7 +18,8 @@ Buffer {
|
||||
" $ echo hello world ",
|
||||
" ",
|
||||
"› 1. Yes, proceed (y) ",
|
||||
" 2. No, and tell Codex what to do differently (esc) ",
|
||||
" 2. Yes, and don't ask again for this command in this session (a) ",
|
||||
" 3. No, and tell Codex what to do differently (esc) ",
|
||||
" ",
|
||||
" Press enter to confirm or esc to cancel ",
|
||||
],
|
||||
@@ -36,8 +38,10 @@ Buffer {
|
||||
x: 20, y: 9, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 11, fg: Cyan, bg: Reset, underline: Reset, modifier: BOLD,
|
||||
x: 21, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 48, y: 12, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 51, y: 12, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 2, y: 14, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 64, y: 12, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 65, y: 12, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 48, y: 13, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 51, y: 13, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 2, y: 15, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
]
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user