mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
permissions: apply reviewer overrides in core
This commit is contained in:
@@ -33,7 +33,6 @@ use tokio_util::sync::CancellationToken;
|
||||
use crate::config::Config;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::guardian::routes_approval_to_guardian_with_reviewer;
|
||||
use crate::guardian::spawn_approval_request_review;
|
||||
use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT;
|
||||
@@ -460,7 +459,10 @@ async fn handle_exec_approval(
|
||||
available_decisions,
|
||||
..
|
||||
} = event;
|
||||
let decision = if routes_approval_to_guardian(parent_ctx) {
|
||||
let approvals_reviewer = parent_session
|
||||
.approvals_reviewer_for_turn(parent_ctx.config.approvals_reviewer)
|
||||
.await;
|
||||
let decision = if routes_approval_to_guardian_with_reviewer(parent_ctx, approvals_reviewer) {
|
||||
let review_cancel = cancel_token.child_token();
|
||||
let review_rx = spawn_approval_request_review(
|
||||
Arc::clone(parent_session),
|
||||
@@ -538,70 +540,74 @@ async fn handle_patch_approval(
|
||||
..
|
||||
} = event;
|
||||
let approval_id = call_id.clone();
|
||||
let guardian_decision = if routes_approval_to_guardian(parent_ctx) {
|
||||
let files = changes
|
||||
.keys()
|
||||
.map(|path| {
|
||||
#[allow(deprecated)]
|
||||
parent_ctx.cwd.join(path)
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
let review_cancel = cancel_token.child_token();
|
||||
let patch = changes
|
||||
.iter()
|
||||
.map(|(path, change)| match change {
|
||||
codex_protocol::protocol::FileChange::Add { content } => {
|
||||
format!("*** Add File: {}\n{}", path.display(), content)
|
||||
}
|
||||
codex_protocol::protocol::FileChange::Delete { content } => {
|
||||
format!("*** Delete File: {}\n{}", path.display(), content)
|
||||
}
|
||||
codex_protocol::protocol::FileChange::Update {
|
||||
unified_diff,
|
||||
move_path,
|
||||
} => {
|
||||
if let Some(move_path) = move_path {
|
||||
format!(
|
||||
"*** Update File: {}\n*** Move to: {}\n{}",
|
||||
path.display(),
|
||||
move_path.display(),
|
||||
unified_diff
|
||||
)
|
||||
} else {
|
||||
format!("*** Update File: {}\n{}", path.display(), unified_diff)
|
||||
let approvals_reviewer = parent_session
|
||||
.approvals_reviewer_for_turn(parent_ctx.config.approvals_reviewer)
|
||||
.await;
|
||||
let guardian_decision =
|
||||
if routes_approval_to_guardian_with_reviewer(parent_ctx, approvals_reviewer) {
|
||||
let files = changes
|
||||
.keys()
|
||||
.map(|path| {
|
||||
#[allow(deprecated)]
|
||||
parent_ctx.cwd.join(path)
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
let review_cancel = cancel_token.child_token();
|
||||
let patch = changes
|
||||
.iter()
|
||||
.map(|(path, change)| match change {
|
||||
codex_protocol::protocol::FileChange::Add { content } => {
|
||||
format!("*** Add File: {}\n{}", path.display(), content)
|
||||
}
|
||||
}
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n");
|
||||
let review_rx = spawn_approval_request_review(
|
||||
Arc::clone(parent_session),
|
||||
Arc::clone(parent_ctx),
|
||||
new_guardian_review_id(),
|
||||
GuardianApprovalRequest::ApplyPatch {
|
||||
id: approval_id.clone(),
|
||||
#[allow(deprecated)]
|
||||
cwd: parent_ctx.cwd.clone(),
|
||||
files,
|
||||
patch,
|
||||
},
|
||||
reason.clone(),
|
||||
GuardianApprovalRequestSource::DelegatedSubagent,
|
||||
review_cancel.clone(),
|
||||
);
|
||||
Some(
|
||||
await_approval_with_cancel(
|
||||
async move { review_rx.await.unwrap_or_default() },
|
||||
parent_session,
|
||||
&approval_id,
|
||||
cancel_token,
|
||||
Some(&review_cancel),
|
||||
codex_protocol::protocol::FileChange::Delete { content } => {
|
||||
format!("*** Delete File: {}\n{}", path.display(), content)
|
||||
}
|
||||
codex_protocol::protocol::FileChange::Update {
|
||||
unified_diff,
|
||||
move_path,
|
||||
} => {
|
||||
if let Some(move_path) = move_path {
|
||||
format!(
|
||||
"*** Update File: {}\n*** Move to: {}\n{}",
|
||||
path.display(),
|
||||
move_path.display(),
|
||||
unified_diff
|
||||
)
|
||||
} else {
|
||||
format!("*** Update File: {}\n{}", path.display(), unified_diff)
|
||||
}
|
||||
}
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n");
|
||||
let review_rx = spawn_approval_request_review(
|
||||
Arc::clone(parent_session),
|
||||
Arc::clone(parent_ctx),
|
||||
new_guardian_review_id(),
|
||||
GuardianApprovalRequest::ApplyPatch {
|
||||
id: approval_id.clone(),
|
||||
#[allow(deprecated)]
|
||||
cwd: parent_ctx.cwd.clone(),
|
||||
files,
|
||||
patch,
|
||||
},
|
||||
reason.clone(),
|
||||
GuardianApprovalRequestSource::DelegatedSubagent,
|
||||
review_cancel.clone(),
|
||||
);
|
||||
Some(
|
||||
await_approval_with_cancel(
|
||||
async move { review_rx.await.unwrap_or_default() },
|
||||
parent_session,
|
||||
&approval_id,
|
||||
cancel_token,
|
||||
Some(&review_cancel),
|
||||
)
|
||||
.await,
|
||||
)
|
||||
.await,
|
||||
)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let decision = if let Some(decision) = guardian_decision {
|
||||
decision
|
||||
} else {
|
||||
@@ -823,6 +829,7 @@ where
|
||||
permissions: Default::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
};
|
||||
parent_session
|
||||
.notify_request_permissions_response(call_id, empty.clone())
|
||||
@@ -833,6 +840,7 @@ where
|
||||
permissions: Default::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -210,6 +210,7 @@ async fn handle_request_permissions_uses_tool_call_id_for_round_trip() {
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
};
|
||||
#[allow(deprecated)]
|
||||
let delegated_cwd = parent_ctx.cwd.join("delegated-cwd");
|
||||
|
||||
@@ -38,7 +38,6 @@ pub(crate) use review::record_guardian_denial_for_test;
|
||||
pub(crate) use review::review_approval_request;
|
||||
#[cfg(test)]
|
||||
pub(crate) use review::review_approval_request_with_cancel;
|
||||
pub(crate) use review::routes_approval_to_guardian;
|
||||
pub(crate) use review::routes_approval_to_guardian_with_reviewer;
|
||||
pub(crate) use review::spawn_approval_request_review;
|
||||
pub(crate) use review_session::GuardianReviewSessionManager;
|
||||
|
||||
@@ -162,13 +162,6 @@ fn guardian_risk_level_str(level: GuardianRiskLevel) -> &'static str {
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether this turn should route allowed approval prompts through the guardian
|
||||
/// reviewer instead of surfacing them to the user. ARC may still block actions
|
||||
/// earlier in the flow.
|
||||
pub(crate) fn routes_approval_to_guardian(turn: &TurnContext) -> bool {
|
||||
routes_approval_to_guardian_with_reviewer(turn, turn.config.approvals_reviewer)
|
||||
}
|
||||
|
||||
/// Whether an approval with its own reviewer selection should be routed through guardian.
|
||||
pub(crate) fn routes_approval_to_guardian_with_reviewer(
|
||||
turn: &TurnContext,
|
||||
|
||||
@@ -1137,12 +1137,18 @@ async fn routes_approval_to_guardian_requires_guardian_reviewer() {
|
||||
config.approvals_reviewer = ApprovalsReviewer::User;
|
||||
turn.config = Arc::new(config.clone());
|
||||
|
||||
assert!(!routes_approval_to_guardian(&turn));
|
||||
assert!(!routes_approval_to_guardian_with_reviewer(
|
||||
&turn,
|
||||
turn.config.approvals_reviewer
|
||||
));
|
||||
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
turn.config = Arc::new(config);
|
||||
|
||||
assert!(routes_approval_to_guardian(&turn));
|
||||
assert!(routes_approval_to_guardian_with_reviewer(
|
||||
&turn,
|
||||
turn.config.approvals_reviewer
|
||||
));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -1175,7 +1181,10 @@ async fn routes_approval_to_guardian_allows_granular_review_policy() {
|
||||
}))
|
||||
.expect("test setup should allow updating approval policy");
|
||||
|
||||
assert!(routes_approval_to_guardian(&turn));
|
||||
assert!(routes_approval_to_guardian_with_reviewer(
|
||||
&turn,
|
||||
turn.config.approvals_reviewer
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -2151,6 +2151,7 @@ impl Session {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
});
|
||||
}
|
||||
AskForApproval::Granular(granular_config)
|
||||
@@ -2160,6 +2161,7 @@ impl Session {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
});
|
||||
}
|
||||
AskForApproval::OnFailure
|
||||
@@ -2170,7 +2172,13 @@ impl Session {
|
||||
|
||||
let requested_permissions = args.permissions;
|
||||
|
||||
if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) {
|
||||
let approvals_reviewer = self
|
||||
.approvals_reviewer_for_turn(turn_context.config.approvals_reviewer)
|
||||
.await;
|
||||
if crate::guardian::routes_approval_to_guardian_with_reviewer(
|
||||
turn_context.as_ref(),
|
||||
approvals_reviewer,
|
||||
) {
|
||||
let originating_turn_state = {
|
||||
let active = self.active_turn.lock().await;
|
||||
active.as_ref().map(|active| Arc::clone(&active.turn_state))
|
||||
@@ -2204,12 +2212,14 @@ impl Session {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
}
|
||||
}
|
||||
ReviewDecision::ApprovedForSession => RequestPermissionsResponse {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
ReviewDecision::NetworkPolicyAmendment {
|
||||
network_policy_amendment,
|
||||
@@ -2218,11 +2228,13 @@ impl Session {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
NetworkPolicyRuleAction::Deny => RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
},
|
||||
ReviewDecision::Abort | ReviewDecision::Denied | ReviewDecision::TimedOut => {
|
||||
@@ -2230,6 +2242,7 @@ impl Session {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
}
|
||||
}
|
||||
};
|
||||
@@ -2314,6 +2327,7 @@ impl Session {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
});
|
||||
};
|
||||
let mut environment = turn_environment.selection();
|
||||
@@ -2443,14 +2457,6 @@ impl Session {
|
||||
response: RequestPermissionsResponse,
|
||||
cwd: &Path,
|
||||
) -> RequestPermissionsResponse {
|
||||
if response.strict_auto_review && matches!(response.scope, PermissionGrantScope::Session) {
|
||||
return RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
};
|
||||
}
|
||||
|
||||
if response.permissions.is_empty() {
|
||||
return response;
|
||||
}
|
||||
@@ -2464,6 +2470,7 @@ impl Session {
|
||||
.into(),
|
||||
scope: response.scope,
|
||||
strict_auto_review: response.strict_auto_review,
|
||||
approvals_reviewer: response.approvals_reviewer,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2473,27 +2480,37 @@ impl Session {
|
||||
environment_id: &str,
|
||||
originating_turn_state: Option<&Arc<Mutex<crate::state::TurnState>>>,
|
||||
) {
|
||||
if response.permissions.is_empty() {
|
||||
return;
|
||||
}
|
||||
let approvals_reviewer = response.approvals_reviewer_override();
|
||||
match response.scope {
|
||||
PermissionGrantScope::Turn => {
|
||||
if let Some(turn_state) = originating_turn_state {
|
||||
let mut ts = turn_state.lock().await;
|
||||
let permissions: AdditionalPermissionProfile =
|
||||
response.permissions.clone().into();
|
||||
ts.record_granted_permissions(environment_id, permissions);
|
||||
if response.strict_auto_review {
|
||||
ts.enable_strict_auto_review();
|
||||
if !response.permissions.is_empty() {
|
||||
let permissions: AdditionalPermissionProfile =
|
||||
response.permissions.clone().into();
|
||||
ts.record_granted_permissions(environment_id, permissions);
|
||||
}
|
||||
if let Some(approvals_reviewer) = approvals_reviewer {
|
||||
ts.set_approvals_reviewer_override(approvals_reviewer);
|
||||
}
|
||||
}
|
||||
}
|
||||
PermissionGrantScope::Session => {
|
||||
let mut state = self.state.lock().await;
|
||||
state.record_granted_permissions(
|
||||
environment_id,
|
||||
response.permissions.clone().into(),
|
||||
);
|
||||
if !response.permissions.is_empty() {
|
||||
let mut state = self.state.lock().await;
|
||||
state.record_granted_permissions(
|
||||
environment_id,
|
||||
response.permissions.clone().into(),
|
||||
);
|
||||
}
|
||||
if let (Some(turn_state), Some(approvals_reviewer)) =
|
||||
(originating_turn_state, approvals_reviewer)
|
||||
{
|
||||
turn_state
|
||||
.lock()
|
||||
.await
|
||||
.set_approvals_reviewer_override(approvals_reviewer);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -2525,6 +2542,22 @@ impl Session {
|
||||
ts.strict_auto_review_enabled()
|
||||
}
|
||||
|
||||
#[expect(
|
||||
clippy::await_holding_invalid_type,
|
||||
reason = "active turn reads must stay consistent with the matching turn state"
|
||||
)]
|
||||
pub(crate) async fn approvals_reviewer_for_turn(
|
||||
&self,
|
||||
fallback: ApprovalsReviewer,
|
||||
) -> ApprovalsReviewer {
|
||||
let active = self.active_turn.lock().await;
|
||||
let Some(active) = active.as_ref() else {
|
||||
return fallback;
|
||||
};
|
||||
let ts = active.turn_state.lock().await;
|
||||
ts.approvals_reviewer_override().unwrap_or(fallback)
|
||||
}
|
||||
|
||||
pub(crate) async fn granted_session_permissions(
|
||||
&self,
|
||||
environment_id: &str,
|
||||
|
||||
@@ -5258,6 +5258,7 @@ async fn notify_request_permissions_response_ignores_unmatched_call_id() {
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
@@ -5293,6 +5294,7 @@ async fn record_granted_request_permissions_for_turn_uses_originating_turn() {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
codex_exec_server::LOCAL_ENVIRONMENT_ID,
|
||||
Some(&originating_turn_state),
|
||||
@@ -5340,6 +5342,7 @@ async fn request_permission_grants_are_environment_keyed() {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
"remote",
|
||||
Some(&originating_turn_state),
|
||||
@@ -5361,6 +5364,7 @@ async fn request_permission_grants_are_environment_keyed() {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
"remote",
|
||||
/*originating_turn_state*/ None,
|
||||
@@ -5393,6 +5397,7 @@ async fn enable_strict_auto_review_for_turn_uses_originating_turn() {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: true,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
codex_exec_server::LOCAL_ENVIRONMENT_ID,
|
||||
Some(&originating_turn_state),
|
||||
@@ -5408,7 +5413,7 @@ async fn enable_strict_auto_review_for_turn_uses_originating_turn() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn strict_auto_review_session_scope_grants_no_permissions() {
|
||||
fn strict_auto_review_session_scope_preserves_session_grant() {
|
||||
let requested_permissions = RequestPermissionProfile {
|
||||
network: Some(codex_protocol::models::NetworkPermissions {
|
||||
enabled: Some(true),
|
||||
@@ -5422,6 +5427,7 @@ fn strict_auto_review_session_scope_grants_no_permissions() {
|
||||
permissions: requested_permissions,
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: true,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
std::path::Path::new("/tmp"),
|
||||
);
|
||||
@@ -5429,13 +5435,59 @@ fn strict_auto_review_session_scope_grants_no_permissions() {
|
||||
assert_eq!(
|
||||
response,
|
||||
codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
permissions: RequestPermissionProfile {
|
||||
network: Some(codex_protocol::models::NetworkPermissions {
|
||||
enabled: Some(true),
|
||||
}),
|
||||
..RequestPermissionProfile::default()
|
||||
},
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: true,
|
||||
approvals_reviewer: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn session_scope_auto_review_records_session_grant_and_turn_reviewer_override() {
|
||||
let (session, _turn_context) = make_session_and_context().await;
|
||||
let originating_active_turn = ActiveTurn::default();
|
||||
let originating_turn_state = Arc::clone(&originating_active_turn.turn_state);
|
||||
*session.active_turn.lock().await = Some(originating_active_turn);
|
||||
|
||||
let requested_permissions = RequestPermissionProfile {
|
||||
network: Some(codex_protocol::models::NetworkPermissions {
|
||||
enabled: Some(true),
|
||||
}),
|
||||
..RequestPermissionProfile::default()
|
||||
};
|
||||
session
|
||||
.record_granted_request_permissions_for_turn(
|
||||
&codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: true,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
codex_exec_server::LOCAL_ENVIRONMENT_ID,
|
||||
Some(&originating_turn_state),
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
session
|
||||
.granted_session_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID)
|
||||
.await,
|
||||
Some(requested_permissions.into())
|
||||
);
|
||||
assert!(
|
||||
originating_turn_state
|
||||
.lock()
|
||||
.await
|
||||
.strict_auto_review_enabled()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn request_permissions_emits_event_when_granular_policy_allows_requests() {
|
||||
let (session, mut turn_context, rx) = make_session_and_context_with_rx().await;
|
||||
@@ -5464,6 +5516,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests()
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
};
|
||||
|
||||
let handle = tokio::spawn({
|
||||
@@ -5619,6 +5672,7 @@ async fn request_permissions_tool_resolves_relative_paths_against_selected_envir
|
||||
permissions: request.permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
@@ -5740,6 +5794,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec
|
||||
permissions: request.permissions,
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
@@ -5755,6 +5810,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec
|
||||
permissions: expected_permissions.clone(),
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
};
|
||||
|
||||
let response = tokio::time::timeout(StdDuration::from_secs(1), handle)
|
||||
@@ -5821,6 +5877,7 @@ async fn request_permissions_is_auto_denied_when_granular_policy_blocks_tool_req
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
}
|
||||
)
|
||||
);
|
||||
|
||||
@@ -152,6 +152,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
})
|
||||
);
|
||||
assert_eq!(
|
||||
@@ -400,6 +401,7 @@ async fn strict_auto_review_turn_grant_forces_guardian_for_shell_command_policy_
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: true,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
codex_exec_server::LOCAL_ENVIRONMENT_ID,
|
||||
Some(&originating_turn_state),
|
||||
|
||||
@@ -8,6 +8,7 @@ use tokio_util::sync::CancellationToken;
|
||||
use tokio_util::task::AbortOnDropHandle;
|
||||
|
||||
use codex_extension_api::ExtensionData;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::dynamic_tools::DynamicToolResponse;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelection;
|
||||
use codex_protocol::request_permissions::RequestPermissionProfile;
|
||||
@@ -93,7 +94,7 @@ pub(crate) struct TurnState {
|
||||
pub(crate) pending_input: TurnInputQueue,
|
||||
mailbox_delivery_phase: MailboxDeliveryPhase,
|
||||
granted_permissions_by_environment_id: HashMap<String, AdditionalPermissionProfile>,
|
||||
strict_auto_review_enabled: bool,
|
||||
approvals_reviewer_override: Option<ApprovalsReviewer>,
|
||||
pub(crate) tool_calls: u64,
|
||||
pub(crate) has_memory_citation: bool,
|
||||
pub(crate) token_usage_at_turn_start: TokenUsage,
|
||||
@@ -231,11 +232,15 @@ impl TurnState {
|
||||
.cloned()
|
||||
}
|
||||
|
||||
pub(crate) fn enable_strict_auto_review(&mut self) {
|
||||
self.strict_auto_review_enabled = true;
|
||||
pub(crate) fn strict_auto_review_enabled(&self) -> bool {
|
||||
self.approvals_reviewer_override == Some(ApprovalsReviewer::AutoReview)
|
||||
}
|
||||
|
||||
pub(crate) fn strict_auto_review_enabled(&self) -> bool {
|
||||
self.strict_auto_review_enabled
|
||||
pub(crate) fn set_approvals_reviewer_override(&mut self, reviewer: ApprovalsReviewer) {
|
||||
self.approvals_reviewer_override = Some(reviewer);
|
||||
}
|
||||
|
||||
pub(crate) fn approvals_reviewer_override(&self) -> Option<ApprovalsReviewer> {
|
||||
self.approvals_reviewer_override
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,7 +4,7 @@ use crate::guardian::guardian_rejection_message;
|
||||
use crate::guardian::guardian_timeout_message;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::guardian::routes_approval_to_guardian_with_reviewer;
|
||||
use crate::hook_runtime::run_permission_request_hooks;
|
||||
use crate::network_policy_decision::denied_network_policy_message;
|
||||
use crate::session::session::Session;
|
||||
@@ -496,7 +496,11 @@ impl NetworkApprovalService {
|
||||
}
|
||||
}
|
||||
}
|
||||
let use_guardian = routes_approval_to_guardian(&turn_context);
|
||||
let approvals_reviewer = session
|
||||
.approvals_reviewer_for_turn(turn_context.config.approvals_reviewer)
|
||||
.await;
|
||||
let use_guardian =
|
||||
routes_approval_to_guardian_with_reviewer(&turn_context, approvals_reviewer);
|
||||
let guardian_review_id = use_guardian.then(new_guardian_review_id);
|
||||
let approval_decision = if let Some(review_id) = guardian_review_id.clone() {
|
||||
review_approval_request(
|
||||
|
||||
@@ -9,7 +9,7 @@ caching).
|
||||
use crate::guardian::guardian_rejection_message;
|
||||
use crate::guardian::guardian_timeout_message;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::guardian::routes_approval_to_guardian_with_reviewer;
|
||||
use crate::hook_runtime::run_permission_request_hooks;
|
||||
use crate::network_policy_decision::network_approval_context_from_payload;
|
||||
use crate::tools::flat_tool_name;
|
||||
@@ -142,8 +142,13 @@ impl ToolOrchestrator {
|
||||
let otel = turn_ctx.session_telemetry.clone();
|
||||
let otel_tn = flat_tool_name(&tool_ctx.tool_name).into_owned();
|
||||
let otel_ci = &tool_ctx.call_id;
|
||||
let approvals_reviewer = tool_ctx
|
||||
.session
|
||||
.approvals_reviewer_for_turn(turn_ctx.config.approvals_reviewer)
|
||||
.await;
|
||||
let strict_auto_review = tool_ctx.session.strict_auto_review_enabled_for_turn().await;
|
||||
let use_guardian = routes_approval_to_guardian(turn_ctx) || strict_auto_review;
|
||||
let use_guardian = routes_approval_to_guardian_with_reviewer(turn_ctx, approvals_reviewer)
|
||||
|| strict_auto_review;
|
||||
|
||||
// 1) Approval
|
||||
let mut already_approved = false;
|
||||
|
||||
@@ -8,7 +8,7 @@ use crate::guardian::guardian_rejection_message;
|
||||
use crate::guardian::guardian_timeout_message;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::guardian::routes_approval_to_guardian_with_reviewer;
|
||||
use crate::hook_runtime::run_permission_request_hooks;
|
||||
use crate::sandboxing::ExecOptions;
|
||||
use crate::sandboxing::ExecRequest;
|
||||
@@ -422,7 +422,6 @@ impl CoreShellActionProvider {
|
||||
let call_id = self.call_id.clone();
|
||||
let approval_id = Some(Uuid::new_v4().to_string());
|
||||
let source = self.tool_name;
|
||||
let guardian_review_id = routes_approval_to_guardian(&turn).then(new_guardian_review_id);
|
||||
Ok(stopwatch
|
||||
.pause_for(async move {
|
||||
// 1) Run PermissionRequest hooks
|
||||
@@ -457,6 +456,12 @@ impl CoreShellActionProvider {
|
||||
}
|
||||
|
||||
// 2) Route to Guardian if configured
|
||||
let approvals_reviewer = session
|
||||
.approvals_reviewer_for_turn(turn.config.approvals_reviewer)
|
||||
.await;
|
||||
let guardian_review_id =
|
||||
routes_approval_to_guardian_with_reviewer(&turn, approvals_reviewer)
|
||||
.then(new_guardian_review_id);
|
||||
if let Some(review_id) = guardian_review_id.clone() {
|
||||
let decision = review_approval_request(
|
||||
&session,
|
||||
|
||||
@@ -192,6 +192,7 @@ async fn remote_model_override_uses_catalog_model_for_strict_auto_review() -> Re
|
||||
permissions: permissions_request.permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: true,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
@@ -409,6 +409,7 @@ async fn remote_request_permissions_grant_unblocks_later_remote_exec() -> Result
|
||||
permissions: expected_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
};
|
||||
let command = format!(
|
||||
"printf 'remote-request-permissions-ok' > {relative_target_path} && cat {relative_target_path}"
|
||||
|
||||
@@ -501,6 +501,7 @@ async fn request_permissions_tool_is_auto_denied_when_granular_request_permissio
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
}
|
||||
);
|
||||
|
||||
@@ -1126,6 +1127,7 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls() -> Resul
|
||||
permissions: normalized_requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1244,6 +1246,7 @@ async fn request_permissions_preapprove_explicit_exec_permissions_outside_on_req
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1361,6 +1364,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls() -> Resu
|
||||
permissions: normalized_requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1474,6 +1478,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls_without_i
|
||||
permissions: normalized_requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1624,6 +1629,7 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions()
|
||||
permissions: granted_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1749,6 +1755,7 @@ async fn request_permissions_grants_do_not_carry_across_turns() -> Result<()> {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1870,6 +1877,7 @@ async fn request_permissions_session_grants_carry_across_turns() -> Result<()> {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: false,
|
||||
approvals_reviewer: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
Reference in New Issue
Block a user