mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat(approvals) RejectConfig for request_permissions (#14118)
## Summary We need to support allowing request_permissions calls when using `Reject` policy <img width="1133" height="588" alt="Screenshot 2026-03-09 at 12 06 40 PM" src="https://github.com/user-attachments/assets/a8df987f-c225-4866-b8ab-5590960daec5" /> Note that this is a backwards-incompatible change for Reject policy. I'm not sure if we need to add a default based on our current use/setup ## Testing - [x] Added tests - [x] Tested locally
This commit is contained in:
committed by
GitHub
Unverified
parent
c1defcc98c
commit
6da84efed8
@@ -2831,6 +2831,25 @@ impl Session {
|
||||
call_id: String,
|
||||
args: RequestPermissionsArgs,
|
||||
) -> Option<RequestPermissionsResponse> {
|
||||
match turn_context.approval_policy.value() {
|
||||
AskForApproval::Never => {
|
||||
return Some(RequestPermissionsResponse {
|
||||
permissions: PermissionProfile::default(),
|
||||
});
|
||||
}
|
||||
AskForApproval::Reject(reject_config)
|
||||
if reject_config.rejects_request_permissions() =>
|
||||
{
|
||||
return Some(RequestPermissionsResponse {
|
||||
permissions: PermissionProfile::default(),
|
||||
});
|
||||
}
|
||||
AskForApproval::OnFailure
|
||||
| AskForApproval::OnRequest
|
||||
| AskForApproval::UnlessTrusted
|
||||
| AskForApproval::Reject(_) => {}
|
||||
}
|
||||
|
||||
let (tx_response, rx_response) = oneshot::channel();
|
||||
let prev_entry = {
|
||||
let mut active = self.active_turn.lock().await;
|
||||
|
||||
@@ -2165,6 +2165,128 @@ async fn notify_request_permissions_response_ignores_unmatched_call_id() {
|
||||
assert_eq!(session.granted_turn_permissions().await, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn request_permissions_emits_event_when_reject_policy_allows_requests() {
|
||||
let (session, mut turn_context, rx) = make_session_and_context_with_rx().await;
|
||||
*session.active_turn.lock().await = Some(ActiveTurn::default());
|
||||
Arc::get_mut(&mut turn_context)
|
||||
.expect("single turn context ref")
|
||||
.approval_policy
|
||||
.set(crate::protocol::AskForApproval::Reject(
|
||||
crate::protocol::RejectConfig {
|
||||
sandbox_approval: true,
|
||||
rules: true,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: true,
|
||||
},
|
||||
))
|
||||
.expect("test setup should allow updating approval policy");
|
||||
|
||||
let session = Arc::new(session);
|
||||
let turn_context = Arc::new(turn_context);
|
||||
let call_id = "call-1".to_string();
|
||||
let expected_response = codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: codex_protocol::models::PermissionProfile {
|
||||
network: Some(codex_protocol::models::NetworkPermissions {
|
||||
enabled: Some(true),
|
||||
}),
|
||||
..Default::default()
|
||||
},
|
||||
};
|
||||
|
||||
let handle = tokio::spawn({
|
||||
let session = Arc::clone(&session);
|
||||
let turn_context = Arc::clone(&turn_context);
|
||||
let call_id = call_id.clone();
|
||||
async move {
|
||||
session
|
||||
.request_permissions(
|
||||
turn_context.as_ref(),
|
||||
call_id,
|
||||
codex_protocol::request_permissions::RequestPermissionsArgs {
|
||||
reason: Some("need network".to_string()),
|
||||
permissions: codex_protocol::models::PermissionProfile {
|
||||
network: Some(codex_protocol::models::NetworkPermissions {
|
||||
enabled: Some(true),
|
||||
}),
|
||||
..Default::default()
|
||||
},
|
||||
},
|
||||
)
|
||||
.await
|
||||
}
|
||||
});
|
||||
|
||||
let request_event = tokio::time::timeout(StdDuration::from_secs(1), rx.recv())
|
||||
.await
|
||||
.expect("request_permissions event timed out")
|
||||
.expect("request_permissions event missing");
|
||||
let EventMsg::RequestPermissions(request) = request_event.msg else {
|
||||
panic!("expected request_permissions event");
|
||||
};
|
||||
assert_eq!(request.call_id, call_id);
|
||||
|
||||
session
|
||||
.notify_request_permissions_response(&request.call_id, expected_response.clone())
|
||||
.await;
|
||||
|
||||
let response = tokio::time::timeout(StdDuration::from_secs(1), handle)
|
||||
.await
|
||||
.expect("request_permissions future timed out")
|
||||
.expect("request_permissions join error");
|
||||
|
||||
assert_eq!(response, Some(expected_response));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn request_permissions_returns_empty_grant_when_reject_policy_blocks_requests() {
|
||||
let (session, mut turn_context, rx) = make_session_and_context_with_rx().await;
|
||||
*session.active_turn.lock().await = Some(ActiveTurn::default());
|
||||
Arc::get_mut(&mut turn_context)
|
||||
.expect("single turn context ref")
|
||||
.approval_policy
|
||||
.set(crate::protocol::AskForApproval::Reject(
|
||||
crate::protocol::RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
request_permissions: true,
|
||||
mcp_elicitations: false,
|
||||
},
|
||||
))
|
||||
.expect("test setup should allow updating approval policy");
|
||||
|
||||
let response = session
|
||||
.request_permissions(
|
||||
&turn_context,
|
||||
"call-1".to_string(),
|
||||
codex_protocol::request_permissions::RequestPermissionsArgs {
|
||||
reason: Some("need network".to_string()),
|
||||
permissions: codex_protocol::models::PermissionProfile {
|
||||
network: Some(codex_protocol::models::NetworkPermissions {
|
||||
enabled: Some(true),
|
||||
}),
|
||||
..Default::default()
|
||||
},
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
response,
|
||||
Some(
|
||||
codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: codex_protocol::models::PermissionProfile::default(),
|
||||
}
|
||||
)
|
||||
);
|
||||
assert!(
|
||||
tokio::time::timeout(StdDuration::from_millis(50), rx.recv())
|
||||
.await
|
||||
.is_err(),
|
||||
"unexpected request_permissions event emitted",
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn submit_with_id_captures_current_span_trace_context() {
|
||||
let (session, _turn_context) = make_session_and_context().await;
|
||||
|
||||
@@ -1569,6 +1569,7 @@ prefix_rule(pattern=["git"], decision="prompt")
|
||||
AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
&SandboxPolicy::new_read_only_policy(),
|
||||
@@ -1590,6 +1591,7 @@ prefix_rule(pattern=["git"], decision="prompt")
|
||||
approval_policy: AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: true,
|
||||
rules: false,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
sandbox_policy: &SandboxPolicy::new_read_only_policy(),
|
||||
@@ -1626,6 +1628,7 @@ prefix_rule(pattern=["git"], decision="prompt")
|
||||
approval_policy: AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: true,
|
||||
rules: false,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
sandbox_policy: &SandboxPolicy::new_read_only_policy(),
|
||||
@@ -1660,6 +1663,7 @@ prefix_rule(pattern=["git"], decision="prompt")
|
||||
approval_policy: AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: true,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
sandbox_policy: &SandboxPolicy::new_read_only_policy(),
|
||||
|
||||
@@ -1739,6 +1739,7 @@ mod tests {
|
||||
RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: false,
|
||||
}
|
||||
)));
|
||||
@@ -1751,6 +1752,7 @@ mod tests {
|
||||
RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: true,
|
||||
}
|
||||
)));
|
||||
|
||||
@@ -316,6 +316,7 @@ mod tests {
|
||||
AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
&policy_workspace_only,
|
||||
@@ -348,6 +349,7 @@ mod tests {
|
||||
AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: true,
|
||||
rules: false,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
&policy_workspace_only,
|
||||
|
||||
@@ -218,6 +218,7 @@ mod tests {
|
||||
!runtime.wants_no_sandbox_approval(AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: true,
|
||||
rules: false,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: false,
|
||||
}))
|
||||
);
|
||||
@@ -225,6 +226,7 @@ mod tests {
|
||||
runtime.wants_no_sandbox_approval(AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: false,
|
||||
}))
|
||||
);
|
||||
|
||||
@@ -398,6 +398,7 @@ mod tests {
|
||||
let policy = AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: true,
|
||||
rules: false,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: false,
|
||||
});
|
||||
|
||||
@@ -417,6 +418,7 @@ mod tests {
|
||||
let policy = AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: true,
|
||||
request_permissions: false,
|
||||
mcp_elicitations: true,
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user