address comments

This commit is contained in:
Dylan Hurd
2026-03-16 23:36:02 -07:00
parent 9bf61f70aa
commit 26e9c745dc
11 changed files with 107 additions and 30 deletions
+17
View File
@@ -776,6 +776,14 @@
],
"type": "string"
},
"MacOsContactsPermission": {
"enum": [
"none",
"read_only",
"read_write"
],
"type": "string"
},
"MacOsPermissionsToml": {
"additionalProperties": false,
"properties": {
@@ -788,8 +796,17 @@
"calendar": {
"type": "boolean"
},
"contacts": {
"$ref": "#/definitions/MacOsContactsPermission"
},
"launch_services": {
"type": "boolean"
},
"preferences": {
"$ref": "#/definitions/MacOsPreferencesPermission"
},
"reminders": {
"type": "boolean"
}
},
"type": "object"
+1 -1
View File
@@ -3187,7 +3187,7 @@ impl Session {
if let Err(err) = persist_granted_permission_profile(
self.codex_home().await.as_path(),
&config,
&permissions,
&permissions.clone().into(),
)
.await
{
+11
View File
@@ -43,6 +43,7 @@ use crate::guardian::GuardianApprovalRequest;
use crate::guardian::review_approval_request_with_cancel;
use crate::guardian::routes_approval_to_guardian;
use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT;
use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER;
use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION;
use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC;
use crate::mcp_tool_call::build_guardian_mcp_tool_review_request;
@@ -687,6 +688,16 @@ async fn maybe_auto_review_mcp_request_user_input(
)
.await;
let selected_label = match decision {
ReviewDecision::ApprovedForAlways => question
.options
.as_ref()
.and_then(|options| {
options
.iter()
.find(|option| option.label == MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER)
})
.map(|option| option.label.clone())
.unwrap_or_else(|| MCP_TOOL_APPROVAL_ACCEPT.to_string()),
ReviewDecision::ApprovedForSession => question
.options
.as_ref()
+2 -18
View File
@@ -19,12 +19,7 @@ use codex_protocol::ThreadId;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::models::FunctionCallOutputBody;
use codex_protocol::models::FunctionCallOutputPayload;
use codex_protocol::models::MacOsAutomationPermission;
use codex_protocol::models::MacOsContactsPermission;
use codex_protocol::models::MacOsPreferencesPermission;
use codex_protocol::models::MacOsSeatbeltProfileExtensions;
use codex_protocol::models::NetworkPermissions;
use codex_protocol::models::PermissionProfile;
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath;
use codex_protocol::permissions::FileSystemSandboxEntry;
@@ -2629,7 +2624,7 @@ async fn notify_request_permissions_response_persists_always_allow_permissions()
.insert_pending_request_permissions("call-1".to_string(), tx);
*session.active_turn.lock().await = Some(active_turn);
let permissions = PermissionProfile {
let permissions = RequestPermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
@@ -2640,17 +2635,6 @@ async fn notify_request_permissions_response_persists_always_allow_permissions()
.expect("absolute path"),
]),
}),
macos: Some(MacOsSeatbeltProfileExtensions {
macos_preferences: MacOsPreferencesPermission::ReadWrite,
macos_automation: MacOsAutomationPermission::BundleIds(vec![
"com.apple.Calendar".to_string(),
]),
macos_launch_services: false,
macos_accessibility: true,
macos_calendar: false,
macos_reminders: false,
macos_contacts: MacOsContactsPermission::None,
}),
};
session
@@ -2666,7 +2650,7 @@ async fn notify_request_permissions_response_persists_always_allow_permissions()
assert_eq!(session.granted_turn_permissions().await, None);
assert_eq!(
session.granted_session_permissions().await,
Some(permissions)
Some(permissions.clone().into())
);
assert_eq!(
rx.await.expect("response should be delivered").scope,
+3 -3
View File
@@ -6122,11 +6122,11 @@ calendar = true
automations: Some(MacOsAutomationPermission::BundleIds(vec![
"com.apple.Calendar".to_string(),
])),
launch_services: Some(false),
launch_services: None,
accessibility: Some(true),
calendar: Some(true),
reminders: Some(false),
contacts: Some(MacOsContactsPermission::None),
reminders: None,
contacts: None,
})
);
}
+3
View File
@@ -633,8 +633,11 @@ fn network_toml_has_values(network: &NetworkToml) -> bool {
fn macos_permissions_toml_has_values(permissions: &MacOsPermissionsToml) -> bool {
permissions.preferences.is_some()
|| permissions.automations.is_some()
|| permissions.launch_services.is_some()
|| permissions.accessibility.is_some()
|| permissions.calendar.is_some()
|| permissions.reminders.is_some()
|| permissions.contacts.is_some()
}
fn compile_network_sandbox_policy(network: Option<&NetworkToml>) -> NetworkSandboxPolicy {
+1 -1
View File
@@ -428,7 +428,7 @@ pub(crate) const MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION: &str = "Allow for this se
// real "Decline" answer, so this lets guardian denials round-trip distinctly from user cancel.
// This is not a user-facing option.
pub(crate) const MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC: &str = "__codex_mcp_decline__";
const MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER: &str = "Allow and don't ask me again";
pub(crate) const MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER: &str = "Allow and don't ask me again";
const MCP_TOOL_APPROVAL_CANCEL: &str = "Cancel";
const MCP_TOOL_APPROVAL_KIND_KEY: &str = "codex_approval_kind";
const MCP_TOOL_APPROVAL_KIND_MCP_TOOL_CALL: &str = "mcp_tool_call";
@@ -350,7 +350,9 @@ struct McpLegacyRequestKey {
fn file_change_decision(decision: &ReviewDecision) -> Result<FileChangeApprovalDecision, String> {
match decision {
ReviewDecision::Approved => Ok(FileChangeApprovalDecision::Accept),
ReviewDecision::ApprovedForSession => Ok(FileChangeApprovalDecision::AcceptForSession),
ReviewDecision::ApprovedForSession | ReviewDecision::ApprovedForAlways => {
Ok(FileChangeApprovalDecision::AcceptForSession)
}
ReviewDecision::Denied => Ok(FileChangeApprovalDecision::Decline),
ReviewDecision::Abort => Ok(FileChangeApprovalDecision::Cancel),
ReviewDecision::ApprovedExecpolicyAmendment { .. } => {
@@ -279,19 +279,23 @@ impl ApprovalOverlay {
return;
};
let granted_permissions = match decision {
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => permissions.clone(),
ReviewDecision::Approved
| ReviewDecision::ApprovedForSession
| ReviewDecision::ApprovedForAlways => permissions.clone(),
ReviewDecision::Denied | ReviewDecision::Abort => Default::default(),
ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::NetworkPolicyAmendment { .. } => Default::default(),
};
let scope = if matches!(decision, ReviewDecision::ApprovedForSession) {
PermissionGrantScope::Session
} else {
PermissionGrantScope::Turn
let scope = match decision {
ReviewDecision::ApprovedForSession => PermissionGrantScope::Session,
ReviewDecision::ApprovedForAlways => PermissionGrantScope::AlwaysAllow,
_ => PermissionGrantScope::Turn,
};
if request.thread_label().is_none() {
let message = if granted_permissions.is_empty() {
"You did not grant additional permissions"
} else if matches!(scope, PermissionGrantScope::AlwaysAllow) {
"You granted additional permissions and saved them for future sessions"
} else if matches!(scope, PermissionGrantScope::Session) {
"You granted additional permissions for this session"
} else {
@@ -695,6 +699,14 @@ fn exec_options(
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))],
}),
ReviewDecision::ApprovedForAlways => {
additional_permissions.as_ref().map(|_| ApprovalOption {
label: "Yes, and always allow these permissions".to_string(),
decision: ApprovalDecision::Review(ReviewDecision::ApprovedForAlways),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))],
})
}
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
} => {
@@ -855,6 +867,12 @@ fn permissions_options() -> Vec<ApprovalOption> {
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))],
},
ApprovalOption {
label: "Yes, always allow these permissions".to_string(),
decision: ApprovalDecision::Review(ReviewDecision::ApprovedForAlways),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))],
},
ApprovalOption {
label: "No, continue without permissions".to_string(),
decision: ApprovalDecision::Review(ReviewDecision::Denied),
@@ -1268,6 +1286,7 @@ mod tests {
vec![
"Yes, grant these permissions".to_string(),
"Yes, grant these permissions for this session".to_string(),
"Yes, always allow these permissions".to_string(),
"No, continue without permissions".to_string(),
]
);
@@ -1300,6 +1319,33 @@ mod tests {
);
}
#[test]
fn permissions_always_shortcut_submits_always_allow_scope() {
let (tx, mut rx) = unbounded_channel::<AppEvent>();
let tx = AppEventSender::new(tx);
let mut view =
ApprovalOverlay::new(make_permissions_request(), tx, Features::with_defaults());
view.handle_key_event(KeyEvent::new(KeyCode::Char('p'), KeyModifiers::NONE));
let mut saw_op = false;
while let Ok(ev) = rx.try_recv() {
if let AppEvent::SubmitThreadOp {
op: Op::RequestPermissionsResponse { response, .. },
..
} = ev
{
assert_eq!(response.scope, PermissionGrantScope::AlwaysAllow);
saw_op = true;
break;
}
}
assert!(
saw_op,
"expected permission approval decision to emit an always-allow response"
);
}
#[test]
fn additional_permissions_prompt_shows_permission_rule_line() {
let (tx, _rx) = unbounded_channel::<AppEvent>();
@@ -11,6 +11,7 @@ expression: "normalize_snapshot_paths(render_overlay_lines(&view, 120))"
› 1. Yes, grant these permissions (y)
2. Yes, grant these permissions for this session (a)
3. No, continue without permissions (n)
3. Yes, always allow these permissions (p)
4. No, continue without permissions (n)
Press enter to confirm or esc to cancel
@@ -840,6 +840,19 @@ pub fn new_approval_decision_cell(
],
)
}
ApprovedForAlways => {
let snippet = Span::from(exec_snippet(&command)).dim();
(
"✔ ".green(),
vec![
"You ".into(),
"approved".bold(),
" codex to run ".into(),
snippet,
" every time and saved it".bold(),
],
)
}
NetworkPolicyAmendment {
network_policy_amendment,
} => match network_policy_amendment.action {