Skip Guardian reviews in Full Access (#42147)

## Why

Full Access already combines `approvalPolicy: "never"` with unrestricted
permissions, so confirmation-only actions do not require a model review.

## What changed

- Detect Full Access consistently across the thread and every selected
  environment. Pending, failed, or restricted environments are not treated as
  Full Access.
- Approve confirmation-only Guardian and MCP requests without synchronous
  review, sampler prewarming, or background scoring. Cancellation, explicit
  denials, and forms that require user input keep their existing behavior.
- Re-evaluate the active permission state on each turn so an existing thread
  can enter or leave Full Access safely.

## Testing

Added coverage for switching approval modes, strict sensitive MCP
confirmations, failed and pending environments, and suppression of Guardian
requests and background-scoring connections.

GitOrigin-RevId: 089ab4296dde473b8e33ab8324be79c5446c46f6
This commit is contained in:
jif
2026-09-01 22:16:54 +00:00
committed by copyberry
parent cd8bd62c6e
commit e576993911
14 changed files with 295 additions and 22 deletions
+7
View File
@@ -1909,6 +1909,13 @@ persisted rollout errors, so unavailable details after a restart remain a termin
## Approvals
Full Access (`approvalPolicy: "never"` with unrestricted selected environments)
skips Guardian, including background scoring. Confirmation-only MCP approvals,
including strict or sensitive CUA requests, are accepted. Strict responses retain
`approvals_reviewer: "auto_review"` for client compatibility, without a model review.
Restricted or unresolved environments, explicit client denials, and forms requiring
user input keep their existing behavior. Cancellation still stops the request.
Certain actions (shell commands or modifying files) may require explicit user approval depending on the user's config. When `turn/start` is used, the app-server drives an approval flow by sending a server-initiated JSON-RPC request to the client. The client must respond to tell Codex whether to proceed. UIs should present these requests inline with the active turn so users can review the proposed command or diff before choosing.
- Requests include `threadId` and `turnId`—use them to scope UI state to the active conversation.
@@ -683,6 +683,7 @@ mod thread_processor_behavior_tests {
approval_policy: codex_protocol::protocol::AskForApproval::OnRequest,
approvals_reviewer: codex_protocol::config_types::ApprovalsReviewer::User,
permission_profile: codex_protocol::models::PermissionProfile::Disabled,
full_access: false,
active_permission_profile: None,
environments: TurnEnvironmentSelections::new(cwd, Vec::new()),
workspace_roots: Vec::new(),
@@ -23,12 +23,16 @@ use axum::routing::get;
use axum::routing::post;
use codex_app_server_protocol::ApprovalsReviewer;
use codex_app_server_protocol::AskForApproval;
use codex_app_server_protocol::ItemCompletedNotification;
use codex_app_server_protocol::ItemGuardianApprovalReviewStartedNotification;
use codex_app_server_protocol::McpToolCallStatus;
use codex_app_server_protocol::SandboxMode;
use codex_app_server_protocol::ServerRequest;
use codex_app_server_protocol::StrictReviewRequiredNotification;
use codex_app_server_protocol::ThreadForkParams;
use codex_app_server_protocol::ThreadForkResponse;
use codex_app_server_protocol::ThreadHistoryMode;
use codex_app_server_protocol::ThreadItem;
use codex_app_server_protocol::ThreadResumeParams;
use codex_app_server_protocol::ThreadResumeResponse;
use codex_app_server_protocol::ThreadRollbackParams;
@@ -37,6 +41,7 @@ use codex_app_server_protocol::ThreadStartParams;
use codex_app_server_protocol::TurnCompletedNotification;
use codex_app_server_protocol::TurnStartParams;
use codex_app_server_protocol::TurnStartResponse;
use codex_app_server_protocol::TurnStatus;
use codex_app_server_protocol::UserInput;
use codex_features::Feature;
use codex_state::StateRuntime;
@@ -144,6 +149,7 @@ struct MockResponsesState {
guardian_reviews: AtomicUsize,
guardian_requests: Mutex<Vec<Value>>,
luna_requests: Mutex<Vec<Value>>,
luna_connections: AtomicUsize,
root_thread_id: Mutex<Option<String>>,
allow_luna: Notify,
allow_guardian_review: Notify,
@@ -486,6 +492,7 @@ async fn luna_websocket(
State(state): State<Arc<MockResponsesState>>,
websocket: WebSocketUpgrade,
) -> impl IntoResponse {
state.luna_connections.fetch_add(1, Ordering::SeqCst);
websocket.on_upgrade(move |mut socket| async move {
while let Some(Ok(message)) = socket.recv().await {
let Message::Text(text) = message else {
@@ -1809,6 +1816,113 @@ async fn first_cua_review_does_not_wait_for_initial_score(
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn full_access_skips_guardian_until_review_is_enabled() -> Result<()> {
skip_if_no_network!(Ok(()));
let state = Arc::new(MockResponsesState {
mcp_server_name: Some("node_repl"),
mcp_tool_sequence: Some(&["js", "js"]),
..Default::default()
});
let listener = TcpListener::bind("127.0.0.1:0").await?;
let responses_url = format!("http://{}", listener.local_addr()?);
let router = Router::new()
.route("/v1/responses", get(luna_websocket).post(parent_response))
.with_state(Arc::clone(&state));
let responses_server = tokio::spawn(async move {
let _ = axum::serve(listener, router).await;
});
// Exercise strict, sensitive CUA confirmations using the existing MCP server.
let (mcp_url, mcp_server) =
start_mcp_server_with_tools(&["js"], /*sensitive_action*/ Some(true)).await?;
let codex_home = TempDir::new()?;
MockResponsesConfig::new(&responses_url)
.with_model(MODEL)
.with_provider_config("supports_websockets = false")
.with_approval_policy("never")
.with_root_config("approvals_reviewer = \"auto_review\"")
.with_extra_config(&format!(
"[mcp_servers.node_repl]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"auto\"\n\n[features.guardianv2]\nenabled = true"
))
.enable_feature(Feature::GuardianApproval)
.write(codex_home.path())?;
let config = load_default_config_for_test(&codex_home).await;
let mut model_info = codex_core::test_support::construct_model_info_offline(MODEL, &config);
model_info.node_repl_auto_review_required = true;
write_models_cache_with_models(codex_home.path(), vec![model_info])?;
let mut app_server = TestAppServer::builder()
.with_codex_home(codex_home.path())
.build_initialized_with_timeout(TIMEOUT)
.await?;
let thread = app_server
.start_thread(ThreadStartParams {
approval_policy: Some(AskForApproval::Never),
sandbox: Some(SandboxMode::DangerFullAccess),
..Default::default()
})
.await?
.thread;
// Leave async scoring pending so reviewed mode must use synchronous Guardian.
state.allow_guardian_review.notify_one();
for approval_policy in [AskForApproval::Never, AskForApproval::OnRequest] {
state.parent_requests.store(0, Ordering::SeqCst);
app_server.clear_message_buffer();
let completed = timeout(
TIMEOUT,
app_server.start_turn_and_wait_for_completion(TurnStartParams {
thread_id: thread.id.clone(),
input: vec![UserInput::Text {
text: USER_CONTEXT.to_owned(),
text_elements: Vec::new(),
}],
approval_policy: Some(approval_policy),
..Default::default()
}),
)
.await??;
assert_eq!(completed.turn.status, TurnStatus::Completed);
let mut tool_results = Vec::new();
while tool_results.len() < 2 {
let completed: ItemCompletedNotification =
timeout(TIMEOUT, app_server.read_notification("item/completed")).await??;
if let ThreadItem::McpToolCall {
status,
result,
error,
..
} = completed.item
{
tool_results.push((status, result.map(|result| result.content), error));
}
}
assert_eq!(
tool_results,
(0..2)
.map(|index| (
McpToolCallStatus::Completed,
Some(vec![
json!({ "type": "text", "text": format!("echo: guardian-{index}") })
]),
None,
))
.collect::<Vec<_>>()
);
if approval_policy == AskForApproval::Never {
assert_eq!(state.guardian_reviews.load(Ordering::SeqCst), 0);
assert_eq!(state.luna_connections.load(Ordering::SeqCst), 0);
} else {
wait_for_luna_request(&state, /*index*/ 0).await?;
assert_eq!(state.guardian_reviews.load(Ordering::SeqCst), 2);
}
}
app_server.shutdown_gracefully().await?;
mcp_server.abort();
responses_server.abort();
Ok(())
}
#[test_case("node_repl", GuardianRisk::Low, None; "browser low risk")]
#[test_case("cua_repl", GuardianRisk::Low, None; "computer use low risk")]
#[test_case("node_repl", GuardianRisk::Low, Some(false); "browser low risk sensitive action false")]
@@ -142,9 +142,10 @@ impl StrictReviewScenario {
fn review_outcomes(self) -> &'static [bool] {
match self {
Self::Approved | Self::ApproveForMe | Self::Never | Self::FullAccess => &[true],
Self::Approved | Self::ApproveForMe | Self::Never => &[true],
Self::DeniedBurst => &[false, false, false],
Self::GuardianDisabled
Self::FullAccess
| Self::GuardianDisabled
| Self::ManagedGuardianDisabled
| Self::ManagedReviewerForbidden
| Self::AppReviewerUser
@@ -1148,7 +1149,12 @@ impl ServerHandler for ElicitationAppsMcpServer {
.map_err(|err| {
rmcp::ErrorData::internal_error(err.to_string(), None)
})?;
let expected = match strict.review_outcomes().get(index) {
let expected = match strict
.review_outcomes()
.get(index)
.copied()
.or_else(|| (strict == Review::FullAccess).then_some(true))
{
Some(true) => json!({
"action": "accept",
"content": {},
+2
View File
@@ -86,6 +86,8 @@ pub struct ThreadConfigSnapshot {
pub approval_policy: AskForApproval,
pub approvals_reviewer: ApprovalsReviewer,
pub permission_profile: PermissionProfile,
/// Resolved Full Access across the thread and every selected environment.
pub full_access: bool,
pub active_permission_profile: Option<ActivePermissionProfile>,
pub environments: TurnEnvironmentSelections,
pub workspace_roots: Vec<AbsolutePathBuf>,
+52 -13
View File
@@ -16,6 +16,7 @@ use codex_exec_server::SelectedCapabilityRootsStatus;
use codex_protocol::capabilities::CapabilityRootLocation;
use codex_protocol::capabilities::SelectedCapabilityRoot;
use codex_protocol::models::PermissionProfile;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::EnvironmentConfig;
use codex_protocol::protocol::EnvironmentConfigState;
use codex_protocol::protocol::EnvironmentConnectionEvent;
@@ -701,6 +702,7 @@ impl ThreadEnvironments {
environment.selection.config,
EnvironmentConfigState::Failed(_)
) {
environments.push(TurnEnvironmentState::Failed);
continue;
}
let pending = matches!(
@@ -720,9 +722,7 @@ impl ThreadEnvironments {
Err(error) => Err(error),
})
};
if let Some(environment) = TurnEnvironmentState::from_resolution(starting, resolved) {
environments.push(environment);
}
environments.push(TurnEnvironmentState::from_resolution(starting, resolved));
}
TurnEnvironmentSnapshot { environments }
}
@@ -736,18 +736,23 @@ impl ThreadEnvironments {
pub(crate) enum TurnEnvironmentState {
Ready(TurnEnvironment),
Starting(StartingTurnEnvironment),
/// Unavailable for execution, but still selected when evaluating permissions.
Failed,
}
impl TurnEnvironmentState {
fn from_resolution(
starting: StartingTurnEnvironment,
resolved: Option<TurnEnvironmentResult>,
) -> Option<Self> {
) -> Self {
match resolved {
Some(Ok(environment)) => {
let mut selection = starting.selection;
if matches!(selection.config, EnvironmentConfigState::Pending) {
selection.config = EnvironmentConfigState::Ready(environment.installed_config?);
let Some(config) = environment.installed_config else {
return Self::Failed;
};
selection.config = EnvironmentConfigState::Ready(config);
}
let mut turn_environment = TurnEnvironment::new(
selection,
@@ -761,35 +766,55 @@ impl TurnEnvironmentState {
environment.shell_snapshot_v2_supported;
turn_environment.user_home_dir = environment.user_home_dir;
turn_environment.temporary_directories = environment.temporary_directories;
Some(Self::Ready(turn_environment))
Self::Ready(turn_environment)
}
Some(Err(err)) => {
tracing::debug!(
environment_id = %starting.selection.environment_id,
"skipping failed turn environment: {err}"
);
None
Self::Failed
}
None => Some(Self::Starting(starting)),
None => Self::Starting(starting),
}
}
}
#[derive(Clone, Debug, Default)]
pub(crate) struct TurnEnvironmentSnapshot {
// Keep ready and starting environments in their original selection order.
// Keep every selected environment, including failures, in its original order.
pub(crate) environments: Vec<TurnEnvironmentState>,
}
impl TurnEnvironmentSnapshot {
pub(crate) fn has_full_access(
&self,
approval_policy: AskForApproval,
thread_profile: &PermissionProfile,
) -> bool {
codex_protocol::protocol::has_full_access(
approval_policy,
thread_profile,
self.refresh_readiness()
.environments
.iter()
.map(|environment| match environment {
TurnEnvironmentState::Ready(environment) => &environment.selection.config,
TurnEnvironmentState::Starting(_) | TurnEnvironmentState::Failed => {
&EnvironmentConfigState::Pending
}
}),
)
}
/// Promotes completed startup work without adopting newer thread selections.
pub(crate) fn refresh_readiness(&self) -> Self {
let environments = self
.environments
.iter()
.filter_map(|environment| match environment {
.map(|environment| match environment {
TurnEnvironmentState::Ready(environment) => {
Some(TurnEnvironmentState::Ready(environment.clone()))
TurnEnvironmentState::Ready(environment.clone())
}
TurnEnvironmentState::Starting(environment) => {
TurnEnvironmentState::from_resolution(
@@ -797,6 +822,7 @@ impl TurnEnvironmentSnapshot {
environment.resolution.clone().now_or_never(),
)
}
TurnEnvironmentState::Failed => TurnEnvironmentState::Failed,
})
.collect();
Self { environments }
@@ -871,7 +897,7 @@ impl TurnEnvironmentSnapshot {
{
environment.selection.cwd.to_abs_path().ok()
}
TurnEnvironmentState::Starting(_) => None,
TurnEnvironmentState::Starting(_) | TurnEnvironmentState::Failed => None,
})
}
@@ -1458,9 +1484,15 @@ url = "ws://127.0.0.1:8765"
assert_eq!(starting.to_selections(), vec![local.clone()]);
assert!(starting.single_local_environment().is_none());
let next_config = test_environment_config();
let next_config = EnvironmentConfig {
permission_profile: PermissionProfileSnapshot::legacy(PermissionProfile::Disabled),
..test_environment_config()
};
turn_environments.update_thread_config(&next_config);
let next_starting = turn_environments.snapshot().await;
assert!(
!next_starting.has_full_access(AskForApproval::Never, &PermissionProfile::Disabled)
);
let server = tokio::spawn(serve_environment_info(listener));
timeout(
@@ -1475,6 +1507,8 @@ url = "ws://127.0.0.1:8765"
.await
.expect("environment resolution should finish")
.expect("environment resolution should succeed");
assert!(next_starting.has_full_access(AskForApproval::Never, &PermissionProfile::Disabled));
assert!(!starting.has_full_access(AskForApproval::Never, &PermissionProfile::Disabled));
let attached = starting.refresh_readiness();
assert!(attached.starting().next().is_none());
@@ -1552,12 +1586,17 @@ url = "ws://127.0.0.1:8765"
);
environments
.update_selections(std::slice::from_ref(&selection), &test_environment_config());
let starting = environments.snapshot().await;
let failed_resolution = environments.environments.load()[0].resolution.clone();
let error = failed_resolution
.clone()
.await
.err()
.expect("environment should fail to start");
// Failed selections must not turn an empty executable snapshot into Full Access.
for snapshot in [starting.refresh_readiness(), environments.snapshot().await] {
assert!(!snapshot.has_full_access(AskForApproval::Never, &PermissionProfile::Disabled));
}
let selected_root = SelectedCapabilityRoot {
id: "failed-root".to_string(),
location: CapabilityRootLocation::Environment {
+1
View File
@@ -1,5 +1,6 @@
//! Guardian review decides whether an `on-request` approval should be granted
//! automatically instead of shown to the user.
//! Full Access (`never` approvals with a disabled sandbox) approves without review.
//!
//! High-level approach:
//! 1. Reconstruct a compact transcript that preserves user intent plus the most
+15 -1
View File
@@ -315,7 +315,7 @@ pub(crate) async fn record_guardian_denial_for_test(
record_guardian_denial(session, turn, turn_id).await;
}
/// Runs Guardian unless an installed extension explicitly claims the review.
/// Runs Guardian unless Full Access or an installed extension resolves the review.
/// Guardian timeouts, review-session failures, and parse failures all block
/// execution, with timeouts surfaced separately from explicit denials.
async fn run_guardian_review(
@@ -327,6 +327,20 @@ async fn run_guardian_review(
options: GuardianReviewOptions,
) -> ReviewDecision {
let turn = Arc::clone(context.turn());
if context.environments().has_full_access(
context.approval_policy,
&turn.config.permissions.effective_permission_profile(),
) {
return if options
.external_cancel
.as_ref()
.is_some_and(CancellationToken::is_cancelled)
{
ReviewDecision::Abort
} else {
ReviewDecision::Approved
};
}
let requires_synchronous_review = options.require_synchronous_review
|| reasons.retry.is_some()
|| matches!(
+24
View File
@@ -726,6 +726,30 @@ async fn review_guardian_mcp_elicitation(
};
let step_settings = turn_context.current_settings.load_full();
// Full Access skips inference, not the active-turn and cancellation checks.
if turn_context.environments.has_full_access(
turn_context.approval_policy(),
&turn_context
.config
.permissions
.effective_permission_profile(),
) && matches!(
&request.elicitation,
Elicitation::Mcp(rmcp::model::ElicitRequestParams::FormElicitationParams {
requested_schema, ..
}) if requested_schema.properties.is_empty()
) {
let decision = if cancellation_token.is_cancelled() {
ReviewDecision::Abort
} else {
ReviewDecision::Approved
};
return Ok(Some(mcp_elicitation_response_from_guardian_decision(
decision,
turn_context.model_info(),
)));
}
// The invocation identifies the tool event, but a nested elicitation can
// review a different action and connector than the enclosing JavaScript.
let originating_call_id = if is_node_repl_backed_server(&request.server_name)
+7
View File
@@ -250,6 +250,13 @@ impl SessionConfiguration {
approval_policy: self.step_settings.approval_policy.value(),
approvals_reviewer: self.step_settings.approvals_reviewer,
permission_profile: self.effective_permission_profile(&environment_selections),
full_access: codex_protocol::protocol::has_full_access(
self.step_settings.approval_policy.value(),
&self.permission_profile(),
environment_selections
.iter()
.map(|environment| &environment.config),
),
active_permission_profile: permission_profile.active_permission_profile(),
environments: TurnEnvironmentSelections::new(
self.legacy_fallback_cwd.clone(),
@@ -647,10 +647,13 @@ async fn strict_auto_review_turn_grant_forces_guardian_for_exec_command_policy_s
.set(AskForApproval::Never)
.expect("test setup should allow updating approval policy");
let mut config = (*turn_context_raw.config).clone();
// Keep Never outside Full Access without requiring an OS sandbox for this routing test.
config
.permissions
.set_permission_profile(codex_protocol::models::PermissionProfile::Disabled)
.expect("test setup should allow disabling the permission profile");
.set_permission_profile(codex_protocol::models::PermissionProfile::External {
network: NetworkSandboxPolicy::Restricted,
})
.expect("test setup should allow external sandbox permissions");
let TurnEnvironmentState::Ready(environment) =
&mut turn_context_raw.environments.environments[0]
else {
@@ -45,6 +45,7 @@ use codex_protocol::mcp::is_node_repl_backed_tool;
use codex_protocol::openai_models::ModelInfo;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::TruncationPolicy;
use codex_protocol::protocol::has_full_access;
use codex_protocol::security_risk::SecurityRiskScore;
use super::action::GuardianAction;
@@ -213,9 +214,20 @@ impl ThreadLifecycleContributor<Config> for GuardianV2Extension {
.insert(TrustedSkillRoots::from_config(input.config));
input.thread_store.insert(guardian_v2_enabled);
tokio::spawn(async move {
sampler.prewarm().await;
});
// Keep the sampler available if a later turn leaves Full Access, but do
// not open Guardian connections while Full Access is selected.
if !has_full_access(
input.config.permissions.approval_policy.value(),
&input.config.permissions.effective_permission_profile(),
input
.environments
.iter()
.map(|environment| &environment.config),
) {
tokio::spawn(async move {
sampler.prewarm().await;
});
}
})
}
}
@@ -481,6 +493,16 @@ impl GuardianV2Extension {
return;
}
};
// Read current permissions, not the startup config: existing threads can
// enter Full Access after the sampler has already been initialized.
let snapshot = thread.config_snapshot().await;
if snapshot.full_access {
// A skipped call invalidates older scores, including ones still in flight.
score_progress
.latest_failed_tool_call
.fetch_max(tool_call_index, Ordering::Release);
return;
}
let parent_model = input.thread_store.get::<ModelInfo>();
// Computer-use-only scores cannot approve other tools for required models.
if guardian_config.review_scope != GuardianV2ReviewScope::ComputerUseOnly
+32
View File
@@ -1,8 +1,12 @@
//! Environment attachment authority, including the shared Full Access decision.
use crate::capabilities::SelectedCapabilityRoot;
use crate::config_types::ShellEnvironmentPolicy;
use crate::config_types::WindowsSandboxLevel;
use crate::mcp_policy::EnvironmentMcpPolicy;
use crate::models::PermissionProfile;
use crate::models::PermissionProfileSnapshot;
use crate::protocol::AskForApproval;
use codex_execpolicy::RequirementsExecPolicy;
use codex_network_proxy::EnvironmentNetworkPolicy;
use codex_utils_path_uri::PathUri;
@@ -21,6 +25,34 @@ pub enum EnvironmentConfigState {
Failed(String),
}
/// Full Access requires no approvals and unrestricted permissions everywhere selected.
/// Thread-owned attachments inherit the fallback profile; unresolved owner authority
/// is never Full Access. All approval and background-review paths use this decision.
pub fn has_full_access<'a>(
approval_policy: AskForApproval,
thread_profile: &PermissionProfile,
environments: impl IntoIterator<Item = &'a EnvironmentConfigState>,
) -> bool {
let mut environments = environments.into_iter().peekable();
approval_policy == AskForApproval::Never
&& if environments.peek().is_none() {
matches!(thread_profile, PermissionProfile::Disabled)
} else {
environments.all(|environment| match environment {
EnvironmentConfigState::FromThread => {
matches!(thread_profile, PermissionProfile::Disabled)
}
EnvironmentConfigState::Ready(config) => {
matches!(
config.permission_profile.permission_profile(),
PermissionProfile::Disabled
)
}
EnvironmentConfigState::Pending | EnvironmentConfigState::Failed(_) => false,
})
}
}
/// Resolved configuration for a thread/environment attachment.
#[derive(Clone, PartialEq)]
pub struct EnvironmentConfig {
+1
View File
@@ -97,6 +97,7 @@ pub use crate::approvals::NetworkPolicyAmendment;
pub use crate::approvals::NetworkPolicyRuleAction;
pub use crate::environment::EnvironmentConfig;
pub use crate::environment::EnvironmentConfigState;
pub use crate::environment::has_full_access;
pub use crate::legacy_events::HasLegacyEvent;
pub use crate::permissions::FileSystemAccessMode;
pub use crate::permissions::FileSystemPath;