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

refactor(app-server): simplify permission confirmation flow

Browse Source 
Replace the per-method v1 app-server capability with a single
`permissionConfirmations` opt-in. Route both permission request
flows through that capability.

Collapse the separate preset tool feature into
`request_permissions_tool`, and make core apply accepted preset
responses so clients only confirm the user decision.
This commit is contained in:
Felipe Coury
2026-04-13 19:50:22 -03:00
parent 0fbcf6600c
commit 39474c7eab
47 changed files with 209 additions and 1118 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/protocol/src/models.rs
+15 -83
View File
@@ -391,7 +391,6 @@ struct PermissionsPromptConfig<'a> {
exec_policy: &'a Policy,
exec_permission_approvals_enabled: bool,
request_permissions_tool_enabled: bool,
request_permission_preset_tool_enabled: bool,
}
impl DeveloperInstructions {
@@ -405,14 +404,11 @@ impl DeveloperInstructions {
exec_policy: &Policy,
exec_permission_approvals_enabled: bool,
request_permissions_tool_enabled: bool,
request_permission_preset_tool_enabled: bool,
) -> DeveloperInstructions {
let append_permission_tool_sections = |text: &str| {
let mut sections = vec![text.to_string()];
if request_permissions_tool_enabled {
sections.push(request_permissions_tool_prompt_section().to_string());
}
if request_permission_preset_tool_enabled {
sections.push(request_permission_preset_tool_prompt_section().to_string());
}
sections.join("\n\n")
@@ -426,8 +422,6 @@ impl DeveloperInstructions {
let mut sections = vec![on_request_rule];
if request_permissions_tool_enabled {
sections.push(request_permissions_tool_prompt_section().to_string());
}
if request_permission_preset_tool_enabled {
sections.push(request_permission_preset_tool_prompt_section().to_string());
}
if let Some(prefixes) = approved_command_prefixes_text(exec_policy) {
@@ -451,7 +445,6 @@ impl DeveloperInstructions {
exec_policy,
exec_permission_approvals_enabled,
request_permissions_tool_enabled,
request_permission_preset_tool_enabled,
),
};
@@ -518,7 +511,6 @@ impl DeveloperInstructions {
cwd: &Path,
exec_permission_approvals_enabled: bool,
request_permissions_tool_enabled: bool,
request_permission_preset_tool_enabled: bool,
) -> Self {
let network_access = if sandbox_policy.has_full_network_access() {
NetworkAccess::Enabled
@@ -545,7 +537,6 @@ impl DeveloperInstructions {
exec_policy,
exec_permission_approvals_enabled,
request_permissions_tool_enabled,
request_permission_preset_tool_enabled,
},
writable_roots,
)
@@ -584,7 +575,6 @@ impl DeveloperInstructions {
config.exec_policy,
config.exec_permission_approvals_enabled,
config.request_permissions_tool_enabled,
config.request_permission_preset_tool_enabled,
))
.concat(DeveloperInstructions::from_writable_roots(writable_roots))
.concat(end_tag)
@@ -648,15 +638,12 @@ fn granular_instructions(
exec_policy: &Policy,
exec_permission_approvals_enabled: bool,
request_permissions_tool_enabled: bool,
request_permission_preset_tool_enabled: bool,
) -> String {
let sandbox_approval_prompts_allowed = granular_config.allows_sandbox_approval();
let shell_permission_requests_available =
exec_permission_approvals_enabled && sandbox_approval_prompts_allowed;
let request_permissions_tool_prompts_allowed =
request_permissions_tool_enabled && granular_config.allows_request_permissions();
let request_permission_preset_tool_prompts_allowed =
request_permission_preset_tool_enabled && granular_config.allows_request_permissions();
let categories = [
Some((
granular_config.allows_sandbox_approval(),
@@ -668,7 +655,7 @@ fn granular_instructions(
granular_config.allows_request_permissions(),
"`request_permissions`",
)),
request_permission_preset_tool_enabled.then_some((
request_permissions_tool_enabled.then_some((
granular_config.allows_request_permissions(),
"`request_permission_preset`",
)),
@@ -711,8 +698,6 @@ fn granular_instructions(
if request_permissions_tool_prompts_allowed {
sections.push(request_permissions_tool_prompt_section().to_string());
}
if request_permission_preset_tool_prompts_allowed {
sections.push(request_permission_preset_tool_prompt_section().to_string());
}
@@ -1722,7 +1707,6 @@ mod tests {
exec_policy: &Policy::empty(),
exec_permission_approvals_enabled: false,
request_permissions_tool_enabled: false,
request_permission_preset_tool_enabled: false,
},
/*writable_roots*/ None,
);
@@ -1756,7 +1740,6 @@ mod tests {
&PathBuf::from("/tmp"),
/*exec_permission_approvals_enabled*/ false,
/*request_permissions_tool_enabled*/ false,
/*request_permission_preset_tool_enabled*/ false,
);
let text = instructions.into_text();
assert!(text.contains("Network access is enabled."));
@@ -1781,7 +1764,6 @@ mod tests {
exec_policy: &exec_policy,
exec_permission_approvals_enabled: false,
request_permissions_tool_enabled: false,
request_permission_preset_tool_enabled: false,
},
/*writable_roots*/ None,
);
@@ -1803,7 +1785,6 @@ mod tests {
exec_policy: &Policy::empty(),
exec_permission_approvals_enabled: false,
request_permissions_tool_enabled: true,
request_permission_preset_tool_enabled: false,
},
/*writable_roots*/ None,
);
@@ -1811,6 +1792,7 @@ mod tests {
let text = instructions.into_text();
assert!(text.contains("`approval_policy` is `unless-trusted`"));
assert!(text.contains("# request_permissions Tool"));
assert!(text.contains("# request_permission_preset Tool"));
assert!(text.contains("After this tool returns, the user has already approved or denied"));
assert!(text.contains("do not say \"once you approve\""));
}
@@ -1826,7 +1808,6 @@ mod tests {
exec_policy: &Policy::empty(),
exec_permission_approvals_enabled: false,
request_permissions_tool_enabled: true,
request_permission_preset_tool_enabled: false,
},
/*writable_roots*/ None,
);
@@ -1834,6 +1815,7 @@ mod tests {
let text = instructions.into_text();
assert!(text.contains("`approval_policy` is `on-failure`"));
assert!(text.contains("# request_permissions Tool"));
assert!(text.contains("# request_permission_preset Tool"));
}
#[test]
@@ -1847,7 +1829,6 @@ mod tests {
exec_policy: &Policy::empty(),
exec_permission_approvals_enabled: true,
request_permissions_tool_enabled: false,
request_permission_preset_tool_enabled: false,
},
/*writable_roots*/ None,
);
@@ -1868,7 +1849,6 @@ mod tests {
exec_policy: &Policy::empty(),
exec_permission_approvals_enabled: false,
request_permissions_tool_enabled: true,
request_permission_preset_tool_enabled: false,
},
/*writable_roots*/ None,
);
@@ -1880,10 +1860,16 @@ mod tests {
);
assert!(text.contains("allow writing to ~/Downloads in this session"));
assert!(text.contains("Set `scope` to `session`"));
assert!(text.contains("# request_permission_preset Tool"));
assert!(text.contains(
"asks conversationally to change the broad sandboxing, approval, or permission mode"
));
assert!(text.contains("Use `request_permissions` instead for named paths"));
assert!(text.contains("never use the preset picker"));
}
#[test]
fn includes_request_permission_preset_tool_instructions_when_enabled() {
fn omits_permission_tool_instructions_when_disabled() {
let instructions = DeveloperInstructions::from_permissions_with_network(
SandboxMode::WorkspaceWrite,
NetworkAccess::Enabled,
@@ -1893,18 +1879,13 @@ mod tests {
exec_policy: &Policy::empty(),
exec_permission_approvals_enabled: false,
request_permissions_tool_enabled: false,
request_permission_preset_tool_enabled: true,
},
/*writable_roots*/ None,
);
let text = instructions.into_text();
assert!(text.contains("# request_permission_preset Tool"));
assert!(text.contains(
"asks conversationally to change the broad sandboxing, approval, or permission mode"
));
assert!(text.contains("Use `request_permissions` instead for named paths"));
assert!(text.contains("never use the preset picker"));
assert!(!text.contains("# request_permissions Tool"));
assert!(!text.contains("# request_permission_preset Tool"));
}
#[test]
@@ -1918,7 +1899,6 @@ mod tests {
exec_policy: &Policy::empty(),
exec_permission_approvals_enabled: true,
request_permissions_tool_enabled: true,
request_permission_preset_tool_enabled: false,
},
/*writable_roots*/ None,
);
@@ -1926,6 +1906,7 @@ mod tests {
let text = instructions.into_text();
assert!(text.contains("with_additional_permissions"));
assert!(text.contains("# request_permissions Tool"));
assert!(text.contains("# request_permission_preset Tool"));
}
#[test]
@@ -1936,7 +1917,6 @@ mod tests {
&Policy::empty(),
/*exec_permission_approvals_enabled*/ false,
/*request_permissions_tool_enabled*/ false,
/*request_permission_preset_tool_enabled*/ false,
)
.into_text();
@@ -1952,7 +1932,6 @@ mod tests {
&Policy::empty(),
/*exec_permission_approvals_enabled*/ false,
/*request_permissions_tool_enabled*/ false,
/*request_permission_preset_tool_enabled*/ false,
)
.into_text();
@@ -1968,7 +1947,6 @@ mod tests {
rejected_categories: &[&str],
include_shell_permission_request_instructions: bool,
include_request_permissions_tool_section: bool,
include_request_permission_preset_tool_section: bool,
) -> String {
let mut sections = vec![granular_prompt_intro_text().to_string()];
if !prompted_categories.is_empty() {
@@ -1988,8 +1966,6 @@ mod tests {
}
if include_request_permissions_tool_section {
sections.push(request_permissions_tool_prompt_section().to_string());
}
if include_request_permission_preset_tool_section {
sections.push(request_permission_preset_tool_prompt_section().to_string());
}
sections.join("\n\n")
@@ -2009,7 +1985,6 @@ mod tests {
&Policy::empty(),
/*exec_permission_approvals_enabled*/ true,
/*request_permissions_tool_enabled*/ false,
/*request_permission_preset_tool_enabled*/ false,
)
.into_text();
@@ -2044,7 +2019,6 @@ mod tests {
&Policy::empty(),
/*exec_permission_approvals_enabled*/ true,
/*request_permissions_tool_enabled*/ false,
/*request_permission_preset_tool_enabled*/ false,
)
.into_text();
@@ -2060,7 +2034,6 @@ mod tests {
&[],
/*include_shell_permission_request_instructions*/ true,
/*include_request_permissions_tool_section*/ false,
/*include_request_permission_preset_tool_section*/ false,
)
);
}
@@ -2079,7 +2052,6 @@ mod tests {
&Policy::empty(),
/*exec_permission_approvals_enabled*/ false,
/*request_permissions_tool_enabled*/ false,
/*request_permission_preset_tool_enabled*/ false,
)
.into_text();
@@ -2095,7 +2067,6 @@ mod tests {
&[],
/*include_shell_permission_request_instructions*/ false,
/*include_request_permissions_tool_section*/ false,
/*include_request_permission_preset_tool_section*/ false,
)
);
}
@@ -2114,47 +2085,9 @@ mod tests {
&Policy::empty(),
/*exec_permission_approvals_enabled*/ true,
/*request_permissions_tool_enabled*/ true,
/*request_permission_preset_tool_enabled*/ false,
)
.into_text();
assert!(allowed.contains("# request_permissions Tool"));
let rejected = DeveloperInstructions::from(
AskForApproval::Granular(GranularApprovalConfig {
sandbox_approval: true,
rules: true,
skill_approval: true,
request_permissions: false,
mcp_elicitations: true,
}),
ApprovalsReviewer::User,
&Policy::empty(),
/*exec_permission_approvals_enabled*/ true,
/*request_permissions_tool_enabled*/ true,
/*request_permission_preset_tool_enabled*/ false,
)
.into_text();
assert!(!rejected.contains("# request_permissions Tool"));
}
#[test]
fn granular_policy_includes_request_permission_preset_tool_only_when_that_prompt_can_still_fire()
{
let allowed = DeveloperInstructions::from(
AskForApproval::Granular(GranularApprovalConfig {
sandbox_approval: true,
rules: true,
skill_approval: true,
request_permissions: true,
mcp_elicitations: true,
}),
ApprovalsReviewer::User,
&Policy::empty(),
/*exec_permission_approvals_enabled*/ true,
/*request_permissions_tool_enabled*/ false,
/*request_permission_preset_tool_enabled*/ true,
)
.into_text();
assert!(allowed.contains("# request_permission_preset Tool"));
let rejected = DeveloperInstructions::from(
@@ -2168,10 +2101,10 @@ mod tests {
ApprovalsReviewer::User,
&Policy::empty(),
/*exec_permission_approvals_enabled*/ true,
/*request_permissions_tool_enabled*/ false,
/*request_permission_preset_tool_enabled*/ true,
/*request_permissions_tool_enabled*/ true,
)
.into_text();
assert!(!rejected.contains("# request_permissions Tool"));
assert!(!rejected.contains("# request_permission_preset Tool"));
assert!(rejected.contains("- `request_permission_preset`"));
}
@@ -2191,7 +2124,6 @@ mod tests {
&Policy::empty(),
/*exec_permission_approvals_enabled*/ true,
/*request_permissions_tool_enabled*/ false,
/*request_permission_preset_tool_enabled*/ false,
)
.into_text();
codex-rs/protocol/src/request_permission_preset.rs
+3 -11
View File
@@ -1,13 +1,10 @@
//! Protocol types for conversational permission-mode preset requests.
//!
//! These messages represent the narrow bridge between a model tool call and a
//! client-owned confirmation UI. Core resolves a requested preset into concrete
//! sandbox, approval, and reviewer settings before emitting the event, and the
//! settings only become active after the client returns an accepted response.
//! client-owned confirmation UI. Core validates the requested preset before
//! emitting the event, and the settings only become active after the client
//! returns an accepted response.
use crate::config_types::ApprovalsReviewer;
use crate::protocol::AskForApproval;
use crate::protocol::SandboxPolicy;
use schemars::JsonSchema;
use serde::Deserialize;
use serde::Serialize;
@@ -81,11 +78,6 @@ pub struct RequestPermissionPresetEvent {
#[serde(default)]
pub turn_id: String,
pub preset: PermissionPresetId,
pub label: String,
pub description: String,
pub approval_policy: AskForApproval,
pub approvals_reviewer: ApprovalsReviewer,
pub sandbox_policy: SandboxPolicy,
#[serde(skip_serializing_if = "Option::is_none")]
pub reason: Option<String>,
}