mirror of
https://github.com/openai/codex.git
synced 2026-10-23 18:08:45 +00:00
Merge branch 'main' into dev/remote-mcp-http-wire-full-ci
This commit is contained in:
+7
@@ -311,6 +311,13 @@
|
||||
}
|
||||
],
|
||||
"default": "turn"
|
||||
},
|
||||
"strictAutoReview": {
|
||||
"description": "Review every subsequent command in this turn before normal sandboxed execution.",
|
||||
"type": [
|
||||
"boolean",
|
||||
"null"
|
||||
]
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
|
||||
+7
@@ -3501,6 +3501,13 @@
|
||||
}
|
||||
],
|
||||
"default": "turn"
|
||||
},
|
||||
"strictAutoReview": {
|
||||
"description": "Review every subsequent command in this turn before normal sandboxed execution.",
|
||||
"type": [
|
||||
"boolean",
|
||||
"null"
|
||||
]
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
|
||||
+5
-1
@@ -4,4 +4,8 @@
|
||||
import type { GrantedPermissionProfile } from "./GrantedPermissionProfile";
|
||||
import type { PermissionGrantScope } from "./PermissionGrantScope";
|
||||
|
||||
export type PermissionsRequestApprovalResponse = { permissions: GrantedPermissionProfile, scope: PermissionGrantScope, };
|
||||
export type PermissionsRequestApprovalResponse = { permissions: GrantedPermissionProfile, scope: PermissionGrantScope,
|
||||
/**
|
||||
* Review every subsequent command in this turn before normal sandboxed execution.
|
||||
*/
|
||||
strictAutoReview?: boolean, };
|
||||
|
||||
@@ -7052,6 +7052,10 @@ pub struct PermissionsRequestApprovalResponse {
|
||||
pub permissions: GrantedPermissionProfile,
|
||||
#[serde(default)]
|
||||
pub scope: PermissionGrantScope,
|
||||
/// Review every subsequent command in this turn before normal sandboxed execution.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub strict_auto_review: Option<bool>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
@@ -7801,6 +7805,18 @@ mod tests {
|
||||
.expect("response should deserialize");
|
||||
|
||||
assert_eq!(response.scope, PermissionGrantScope::Turn);
|
||||
assert_eq!(response.strict_auto_review, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn permissions_request_approval_response_accepts_strict_auto_review() {
|
||||
let response = serde_json::from_value::<PermissionsRequestApprovalResponse>(json!({
|
||||
"permissions": {},
|
||||
"strictAutoReview": true,
|
||||
}))
|
||||
.expect("response should deserialize");
|
||||
|
||||
assert_eq!(response.strict_auto_review, Some(true));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -961,6 +961,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
let empty = CoreRequestPermissionsResponse {
|
||||
permissions: Default::default(),
|
||||
scope: CorePermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
};
|
||||
if let Err(err) = conversation
|
||||
.submit(Op::RequestPermissionsResponse {
|
||||
@@ -2669,6 +2670,7 @@ fn request_permissions_response_from_client_result(
|
||||
return Some(CoreRequestPermissionsResponse {
|
||||
permissions: Default::default(),
|
||||
scope: CorePermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
});
|
||||
}
|
||||
Err(err) => {
|
||||
@@ -2676,6 +2678,7 @@ fn request_permissions_response_from_client_result(
|
||||
return Some(CoreRequestPermissionsResponse {
|
||||
permissions: Default::default(),
|
||||
scope: CorePermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
});
|
||||
}
|
||||
};
|
||||
@@ -2686,8 +2689,23 @@ fn request_permissions_response_from_client_result(
|
||||
PermissionsRequestApprovalResponse {
|
||||
permissions: V2GrantedPermissionProfile::default(),
|
||||
scope: codex_app_server_protocol::PermissionGrantScope::Turn,
|
||||
strict_auto_review: None,
|
||||
}
|
||||
});
|
||||
let strict_auto_review = response.strict_auto_review.unwrap_or(false);
|
||||
if strict_auto_review
|
||||
&& matches!(
|
||||
response.scope,
|
||||
codex_app_server_protocol::PermissionGrantScope::Session
|
||||
)
|
||||
{
|
||||
error!("strict auto review is only supported for turn-scoped permission grants");
|
||||
return Some(CoreRequestPermissionsResponse {
|
||||
permissions: Default::default(),
|
||||
scope: CorePermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
});
|
||||
}
|
||||
let granted_permissions: CorePermissionProfile = response.permissions.into();
|
||||
let permissions = if granted_permissions.is_empty() {
|
||||
CoreRequestPermissionProfile::default()
|
||||
@@ -2697,6 +2715,7 @@ fn request_permissions_response_from_client_result(
|
||||
Some(CoreRequestPermissionsResponse {
|
||||
permissions,
|
||||
scope: response.scope.to_core(),
|
||||
strict_auto_review,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -3858,6 +3877,7 @@ mod tests {
|
||||
CoreRequestPermissionsResponse {
|
||||
permissions: expected_permissions,
|
||||
scope: CorePermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
}
|
||||
);
|
||||
}
|
||||
@@ -3880,10 +3900,63 @@ mod tests {
|
||||
CoreRequestPermissionsResponse {
|
||||
permissions: CoreRequestPermissionProfile::default(),
|
||||
scope: CorePermissionGrantScope::Session,
|
||||
strict_auto_review: false,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn request_permissions_response_rejects_session_scoped_strict_auto_review() {
|
||||
let response = request_permissions_response_from_client_result(
|
||||
CoreRequestPermissionProfile::default(),
|
||||
Ok(Ok(serde_json::json!({
|
||||
"scope": "session",
|
||||
"strictAutoReview": true,
|
||||
"permissions": {
|
||||
"network": {
|
||||
"enabled": true,
|
||||
},
|
||||
},
|
||||
}))),
|
||||
std::env::current_dir().expect("current dir").as_path(),
|
||||
)
|
||||
.expect("response should be accepted");
|
||||
|
||||
assert_eq!(
|
||||
response,
|
||||
CoreRequestPermissionsResponse {
|
||||
permissions: CoreRequestPermissionProfile::default(),
|
||||
scope: CorePermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn request_permissions_response_preserves_turn_scoped_strict_auto_review() {
|
||||
let response = request_permissions_response_from_client_result(
|
||||
CoreRequestPermissionProfile {
|
||||
network: Some(codex_protocol::models::NetworkPermissions {
|
||||
enabled: Some(true),
|
||||
}),
|
||||
..Default::default()
|
||||
},
|
||||
Ok(Ok(serde_json::json!({
|
||||
"strictAutoReview": true,
|
||||
"permissions": {
|
||||
"network": {
|
||||
"enabled": true,
|
||||
},
|
||||
},
|
||||
}))),
|
||||
std::env::current_dir().expect("current dir").as_path(),
|
||||
)
|
||||
.expect("response should be accepted");
|
||||
|
||||
assert_eq!(response.scope, CorePermissionGrantScope::Turn);
|
||||
assert!(response.strict_auto_review);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn request_permissions_response_accepts_explicit_child_grant_for_requested_cwd_scope() {
|
||||
let temp_dir = TempDir::new().expect("temp dir");
|
||||
|
||||
@@ -5802,8 +5802,8 @@ impl CodexMessageProcessor {
|
||||
let environment_manager = self.thread_manager.environment_manager();
|
||||
let runtime_environment = match environment_manager.default_environment() {
|
||||
Some(environment) => {
|
||||
// Status listing has no turn cwd. This fallback is used only
|
||||
// by executor-backed stdio MCPs whose config omits `cwd`.
|
||||
// Status listing has no turn cwd. This fallback is used by
|
||||
// stdio MCPs whose config omits `cwd`.
|
||||
McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf())
|
||||
}
|
||||
None => McpRuntimeEnvironment::new(
|
||||
|
||||
@@ -99,6 +99,7 @@ async fn request_permissions_round_trip() -> Result<()> {
|
||||
}),
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: None,
|
||||
})?,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -668,8 +668,8 @@ pub struct McpConnectionManager {
|
||||
/// servers should run for the current caller. Keep it explicit at manager
|
||||
/// construction time so status/snapshot paths and real sessions make the same
|
||||
/// local-vs-remote decision. `fallback_cwd` is not a per-server override; it is
|
||||
/// used only when an executor-backed stdio server omits `cwd` and the executor
|
||||
/// API still needs a concrete process working directory.
|
||||
/// used when a stdio server omits `cwd` and the launcher needs a concrete
|
||||
/// process working directory.
|
||||
#[derive(Clone)]
|
||||
pub struct McpRuntimeEnvironment {
|
||||
environment: Arc<Environment>,
|
||||
@@ -1576,7 +1576,9 @@ async fn make_rmcp_client(
|
||||
runtime_environment.fallback_cwd(),
|
||||
))
|
||||
} else {
|
||||
Arc::new(LocalStdioServerLauncher) as Arc<dyn StdioServerLauncher>
|
||||
Arc::new(LocalStdioServerLauncher::new(
|
||||
runtime_environment.fallback_cwd(),
|
||||
)) as Arc<dyn StdioServerLauncher>
|
||||
};
|
||||
|
||||
// `RmcpClient` always sees a launched MCP stdio server. The
|
||||
|
||||
@@ -163,15 +163,14 @@ impl AgentControl {
|
||||
initial_operation: Op,
|
||||
session_source: Option<SessionSource>,
|
||||
) -> CodexResult<ThreadId> {
|
||||
Ok(self
|
||||
.spawn_agent_internal(
|
||||
config,
|
||||
initial_operation,
|
||||
session_source,
|
||||
SpawnAgentOptions::default(),
|
||||
)
|
||||
.await?
|
||||
.thread_id)
|
||||
let spawned_agent = Box::pin(self.spawn_agent_internal(
|
||||
config,
|
||||
initial_operation,
|
||||
session_source,
|
||||
SpawnAgentOptions::default(),
|
||||
))
|
||||
.await?;
|
||||
Ok(spawned_agent.thread_id)
|
||||
}
|
||||
|
||||
/// Spawn an agent thread with some metadata.
|
||||
@@ -182,7 +181,7 @@ impl AgentControl {
|
||||
session_source: Option<SessionSource>,
|
||||
options: SpawnAgentOptions, // TODO(jif) drop with new fork.
|
||||
) -> CodexResult<LiveAgent> {
|
||||
self.spawn_agent_internal(config, initial_operation, session_source, options)
|
||||
Box::pin(self.spawn_agent_internal(config, initial_operation, session_source, options))
|
||||
.await
|
||||
}
|
||||
|
||||
@@ -418,9 +417,12 @@ impl AgentControl {
|
||||
session_source: SessionSource,
|
||||
) -> CodexResult<ThreadId> {
|
||||
let root_depth = thread_spawn_depth(&session_source).unwrap_or(0);
|
||||
let resumed_thread_id = self
|
||||
.resume_single_agent_from_rollout(config.clone(), thread_id, session_source)
|
||||
.await?;
|
||||
let resumed_thread_id = Box::pin(self.resume_single_agent_from_rollout(
|
||||
config.clone(),
|
||||
thread_id,
|
||||
session_source,
|
||||
))
|
||||
.await?;
|
||||
let state = self.upgrade()?;
|
||||
let Ok(resumed_thread) = state.get_thread(resumed_thread_id).await else {
|
||||
return Ok(resumed_thread_id);
|
||||
@@ -698,7 +700,7 @@ impl AgentControl {
|
||||
{
|
||||
warn!("failed to persist thread-spawn edge status for {agent_id}: {err}");
|
||||
}
|
||||
self.shutdown_agent_tree(agent_id).await
|
||||
Box::pin(self.shutdown_agent_tree(agent_id)).await
|
||||
}
|
||||
|
||||
/// Shut down `agent_id` and any live descendants reachable from the in-memory spawn tree.
|
||||
|
||||
@@ -804,6 +804,7 @@ where
|
||||
let empty = RequestPermissionsResponse {
|
||||
permissions: Default::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
};
|
||||
parent_session
|
||||
.notify_request_permissions_response(call_id, empty.clone())
|
||||
@@ -813,6 +814,7 @@ where
|
||||
response = fut => response.unwrap_or_else(|| RequestPermissionsResponse {
|
||||
permissions: Default::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -179,6 +179,7 @@ async fn handle_request_permissions_uses_tool_call_id_for_round_trip() {
|
||||
..RequestPermissionProfile::default()
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
};
|
||||
let delegated_cwd = parent_ctx.cwd.join("delegated-cwd");
|
||||
let cancel_token = CancellationToken::new();
|
||||
|
||||
@@ -3628,8 +3628,7 @@ async fn set_feature_enabled_updates_profile() -> anyhow::Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn set_feature_enabled_persists_default_false_feature_disable_in_profile()
|
||||
-> anyhow::Result<()> {
|
||||
async fn set_feature_enabled_persists_feature_disable_in_profile() -> anyhow::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
ConfigEditsBuilder::new(codex_home.path())
|
||||
@@ -6881,7 +6880,7 @@ smart_approvals = true
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert!(!config.features.enabled(Feature::GuardianApproval));
|
||||
assert!(config.features.enabled(Feature::GuardianApproval));
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::User);
|
||||
|
||||
let serialized = tokio::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).await?;
|
||||
@@ -6910,7 +6909,7 @@ smart_approvals = true
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert!(!config.features.enabled(Feature::GuardianApproval));
|
||||
assert!(config.features.enabled(Feature::GuardianApproval));
|
||||
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::User);
|
||||
|
||||
let serialized = tokio::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).await?;
|
||||
|
||||
@@ -1893,6 +1893,7 @@ impl Session {
|
||||
return Some(RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
});
|
||||
}
|
||||
AskForApproval::Granular(granular_config)
|
||||
@@ -1901,6 +1902,7 @@ impl Session {
|
||||
return Some(RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
});
|
||||
}
|
||||
AskForApproval::OnFailure
|
||||
@@ -1944,11 +1946,13 @@ impl Session {
|
||||
RequestPermissionsResponse {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
}
|
||||
}
|
||||
ReviewDecision::ApprovedForSession => RequestPermissionsResponse {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
ReviewDecision::NetworkPolicyAmendment {
|
||||
network_policy_amendment,
|
||||
@@ -1956,16 +1960,19 @@ impl Session {
|
||||
NetworkPolicyRuleAction::Allow => RequestPermissionsResponse {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
NetworkPolicyRuleAction::Deny => RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
},
|
||||
ReviewDecision::Abort | ReviewDecision::Denied | ReviewDecision::TimedOut => {
|
||||
RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
}
|
||||
}
|
||||
};
|
||||
@@ -2137,6 +2144,14 @@ 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;
|
||||
}
|
||||
@@ -2149,6 +2164,7 @@ impl Session {
|
||||
)
|
||||
.into(),
|
||||
scope: response.scope,
|
||||
strict_auto_review: response.strict_auto_review,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2164,7 +2180,11 @@ impl Session {
|
||||
PermissionGrantScope::Turn => {
|
||||
if let Some(turn_state) = originating_turn_state {
|
||||
let mut ts = turn_state.lock().await;
|
||||
ts.record_granted_permissions(response.permissions.clone().into());
|
||||
let permissions: PermissionProfile = response.permissions.clone().into();
|
||||
ts.record_granted_permissions(permissions);
|
||||
if response.strict_auto_review {
|
||||
ts.enable_strict_auto_review();
|
||||
}
|
||||
}
|
||||
}
|
||||
PermissionGrantScope::Session => {
|
||||
@@ -2185,6 +2205,19 @@ impl Session {
|
||||
ts.granted_permissions()
|
||||
}
|
||||
|
||||
#[expect(
|
||||
clippy::await_holding_invalid_type,
|
||||
reason = "active turn reads must stay consistent with the matching turn state"
|
||||
)]
|
||||
pub(crate) async fn strict_auto_review_enabled_for_turn(&self) -> bool {
|
||||
let active = self.active_turn.lock().await;
|
||||
let Some(active) = active.as_ref() else {
|
||||
return false;
|
||||
};
|
||||
let ts = active.turn_state.lock().await;
|
||||
ts.strict_auto_review_enabled()
|
||||
}
|
||||
|
||||
pub(crate) async fn granted_session_permissions(&self) -> Option<PermissionProfile> {
|
||||
let state = self.state.lock().await;
|
||||
state.granted_permissions()
|
||||
|
||||
@@ -3352,6 +3352,7 @@ async fn notify_request_permissions_response_ignores_unmatched_call_id() {
|
||||
..RequestPermissionProfile::default()
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
@@ -3381,6 +3382,7 @@ async fn record_granted_request_permissions_for_turn_uses_originating_turn() {
|
||||
&codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
Some(&originating_turn_state),
|
||||
)
|
||||
@@ -3394,6 +3396,67 @@ async fn record_granted_request_permissions_for_turn_uses_originating_turn() {
|
||||
assert_eq!(session.granted_turn_permissions().await, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn enable_strict_auto_review_for_turn_uses_originating_turn() {
|
||||
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::Turn,
|
||||
strict_auto_review: true,
|
||||
},
|
||||
Some(&originating_turn_state),
|
||||
)
|
||||
.await;
|
||||
|
||||
assert!(
|
||||
originating_turn_state
|
||||
.lock()
|
||||
.await
|
||||
.strict_auto_review_enabled()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn strict_auto_review_session_scope_grants_no_permissions() {
|
||||
let requested_permissions = RequestPermissionProfile {
|
||||
network: Some(codex_protocol::models::NetworkPermissions {
|
||||
enabled: Some(true),
|
||||
}),
|
||||
..RequestPermissionProfile::default()
|
||||
};
|
||||
|
||||
let response = Session::normalize_request_permissions_response(
|
||||
requested_permissions.clone(),
|
||||
codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: requested_permissions,
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: true,
|
||||
},
|
||||
std::path::Path::new("/tmp"),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
response,
|
||||
codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[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;
|
||||
@@ -3421,6 +3484,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests()
|
||||
..RequestPermissionProfile::default()
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
};
|
||||
|
||||
let handle = tokio::spawn({
|
||||
@@ -3536,6 +3600,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec
|
||||
codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: request.permissions,
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
@@ -3550,6 +3615,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec
|
||||
let expected_response = codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: expected_permissions.clone(),
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: false,
|
||||
};
|
||||
|
||||
let response = tokio::time::timeout(StdDuration::from_secs(1), handle)
|
||||
@@ -3606,6 +3672,7 @@ async fn request_permissions_is_auto_denied_when_granular_policy_blocks_tool_req
|
||||
codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
}
|
||||
)
|
||||
);
|
||||
|
||||
@@ -131,6 +131,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() {
|
||||
Some(RequestPermissionsResponse {
|
||||
permissions: requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
})
|
||||
);
|
||||
assert_eq!(
|
||||
@@ -379,6 +380,119 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid
|
||||
assert!(exec_output.output.contains("hi"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn strict_auto_review_turn_grant_forces_guardian_for_shell_policy_skip() {
|
||||
let server = start_mock_server().await;
|
||||
let guardian_request_log = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-guardian"),
|
||||
ev_assistant_message(
|
||||
"msg-guardian",
|
||||
&serde_json::json!({
|
||||
"risk_level": "low",
|
||||
"user_authorization": "high",
|
||||
"outcome": "allow",
|
||||
"rationale": "The command stays within the strict turn permission grant.",
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
ev_completed("resp-guardian"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let (mut session, mut turn_context_raw) = make_session_and_context().await;
|
||||
let active_turn = crate::state::ActiveTurn::default();
|
||||
let originating_turn_state = Arc::clone(&active_turn.turn_state);
|
||||
*session.active_turn.lock().await = Some(active_turn);
|
||||
session
|
||||
.record_granted_request_permissions_for_turn(
|
||||
&RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile {
|
||||
network: Some(NetworkPermissions {
|
||||
enabled: Some(true),
|
||||
}),
|
||||
..Default::default()
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: true,
|
||||
},
|
||||
Some(&originating_turn_state),
|
||||
)
|
||||
.await;
|
||||
|
||||
turn_context_raw
|
||||
.approval_policy
|
||||
.set(AskForApproval::OnFailure)
|
||||
.expect("test setup should allow updating approval policy");
|
||||
turn_context_raw
|
||||
.sandbox_policy
|
||||
.set(SandboxPolicy::DangerFullAccess)
|
||||
.expect("test setup should allow updating sandbox policy");
|
||||
turn_context_raw.file_system_sandbox_policy =
|
||||
FileSystemSandboxPolicy::from(turn_context_raw.sandbox_policy.get());
|
||||
turn_context_raw.network_sandbox_policy =
|
||||
NetworkSandboxPolicy::from(turn_context_raw.sandbox_policy.get());
|
||||
let mut config = (*turn_context_raw.config).clone();
|
||||
config.approvals_reviewer = ApprovalsReviewer::User;
|
||||
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
|
||||
let config = Arc::new(config);
|
||||
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
|
||||
config.codex_home.to_path_buf(),
|
||||
Arc::clone(&session.services.auth_manager),
|
||||
config.model_provider.clone(),
|
||||
));
|
||||
session.services.models_manager = models_manager;
|
||||
turn_context_raw.config = Arc::clone(&config);
|
||||
turn_context_raw.provider = create_model_provider(
|
||||
config.model_provider.clone(),
|
||||
turn_context_raw.auth_manager.clone(),
|
||||
);
|
||||
let session = Arc::new(session);
|
||||
let turn_context = Arc::new(turn_context_raw);
|
||||
|
||||
let handler = ShellHandler;
|
||||
let command = if cfg!(windows) {
|
||||
vec![
|
||||
"cmd.exe".to_string(),
|
||||
"/Q".to_string(),
|
||||
"/D".to_string(),
|
||||
"/C".to_string(),
|
||||
"echo hi".to_string(),
|
||||
]
|
||||
} else {
|
||||
vec![
|
||||
"/bin/sh".to_string(),
|
||||
"-c".to_string(),
|
||||
"echo hi".to_string(),
|
||||
]
|
||||
};
|
||||
let resp = handler
|
||||
.handle(ToolInvocation {
|
||||
session: Arc::clone(&session),
|
||||
turn: Arc::clone(&turn_context),
|
||||
cancellation_token: CancellationToken::new(),
|
||||
tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())),
|
||||
call_id: "strict-shell-call".to_string(),
|
||||
tool_name: codex_tools::ToolName::plain("shell"),
|
||||
payload: ToolPayload::Function {
|
||||
arguments: serde_json::json!({
|
||||
"command": command,
|
||||
"workdir": Some(turn_context.cwd.to_string_lossy().to_string()),
|
||||
"timeout_ms": 1_000_u64,
|
||||
})
|
||||
.to_string(),
|
||||
},
|
||||
})
|
||||
.await;
|
||||
|
||||
let output = expect_text_output(&resp.expect("expected Ok result"));
|
||||
assert!(output.contains("hi"));
|
||||
let guardian_request = guardian_request_log.single_request();
|
||||
assert!(guardian_request.body_contains_text("echo hi"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_allows_unified_exec_additional_permissions_requests_past_policy_validation() {
|
||||
let (mut session, mut turn_context_raw) = make_session_and_context().await;
|
||||
|
||||
@@ -106,6 +106,7 @@ pub(crate) struct TurnState {
|
||||
pending_input: Vec<ResponseInputItem>,
|
||||
mailbox_delivery_phase: MailboxDeliveryPhase,
|
||||
granted_permissions: Option<PermissionProfile>,
|
||||
strict_auto_review_enabled: bool,
|
||||
pub(crate) tool_calls: u64,
|
||||
pub(crate) has_memory_citation: bool,
|
||||
pub(crate) token_usage_at_turn_start: TokenUsage,
|
||||
@@ -254,6 +255,14 @@ impl TurnState {
|
||||
pub(crate) fn granted_permissions(&self) -> Option<PermissionProfile> {
|
||||
self.granted_permissions.clone()
|
||||
}
|
||||
|
||||
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.strict_auto_review_enabled
|
||||
}
|
||||
}
|
||||
|
||||
impl ActiveTurn {
|
||||
|
||||
@@ -66,10 +66,7 @@ impl ToolHandler for Handler {
|
||||
return Err(collab_agent_error(agent_id, err));
|
||||
}
|
||||
};
|
||||
let result = session
|
||||
.services
|
||||
.agent_control
|
||||
.close_agent(agent_id)
|
||||
let result = Box::pin(session.services.agent_control.close_agent(agent_id))
|
||||
.await
|
||||
.map_err(|err| collab_agent_error(agent_id, err))
|
||||
.map(|_| ());
|
||||
|
||||
@@ -61,7 +61,14 @@ impl ToolHandler for Handler {
|
||||
.get_status(receiver_thread_id)
|
||||
.await;
|
||||
let (receiver_agent, error) = if matches!(status, AgentStatus::NotFound) {
|
||||
match try_resume_closed_agent(&session, &turn, receiver_thread_id, child_depth).await {
|
||||
match Box::pin(try_resume_closed_agent(
|
||||
&session,
|
||||
&turn,
|
||||
receiver_thread_id,
|
||||
child_depth,
|
||||
))
|
||||
.await
|
||||
{
|
||||
Ok(()) => {
|
||||
status = session
|
||||
.services
|
||||
@@ -149,21 +156,18 @@ async fn try_resume_closed_agent(
|
||||
child_depth: i32,
|
||||
) -> Result<(), FunctionCallError> {
|
||||
let config = build_agent_resume_config(turn.as_ref(), child_depth)?;
|
||||
session
|
||||
.services
|
||||
.agent_control
|
||||
.resume_agent_from_rollout(
|
||||
config,
|
||||
receiver_thread_id,
|
||||
thread_spawn_source(
|
||||
session.conversation_id,
|
||||
&turn.session_source,
|
||||
child_depth,
|
||||
/*agent_role*/ None,
|
||||
/*task_name*/ None,
|
||||
)?,
|
||||
)
|
||||
.await
|
||||
.map(|_| ())
|
||||
.map_err(|err| collab_agent_error(receiver_thread_id, err))
|
||||
Box::pin(session.services.agent_control.resume_agent_from_rollout(
|
||||
config,
|
||||
receiver_thread_id,
|
||||
thread_spawn_source(
|
||||
session.conversation_id,
|
||||
&turn.session_source,
|
||||
child_depth,
|
||||
/*agent_role*/ None,
|
||||
/*task_name*/ None,
|
||||
)?,
|
||||
))
|
||||
.await
|
||||
.map(|_| ())
|
||||
.map_err(|err| collab_agent_error(receiver_thread_id, err))
|
||||
}
|
||||
|
||||
@@ -82,26 +82,23 @@ impl ToolHandler for Handler {
|
||||
apply_spawn_agent_runtime_overrides(&mut config, turn.as_ref())?;
|
||||
apply_spawn_agent_overrides(&mut config, child_depth);
|
||||
|
||||
let result = session
|
||||
.services
|
||||
.agent_control
|
||||
.spawn_agent_with_metadata(
|
||||
config,
|
||||
input_items,
|
||||
Some(thread_spawn_source(
|
||||
session.conversation_id,
|
||||
&turn.session_source,
|
||||
child_depth,
|
||||
role_name,
|
||||
/*task_name*/ None,
|
||||
)?),
|
||||
SpawnAgentOptions {
|
||||
fork_parent_spawn_call_id: args.fork_context.then(|| call_id.clone()),
|
||||
fork_mode: args.fork_context.then_some(SpawnAgentForkMode::FullHistory),
|
||||
},
|
||||
)
|
||||
.await
|
||||
.map_err(collab_spawn_error);
|
||||
let result = Box::pin(session.services.agent_control.spawn_agent_with_metadata(
|
||||
config,
|
||||
input_items,
|
||||
Some(thread_spawn_source(
|
||||
session.conversation_id,
|
||||
&turn.session_source,
|
||||
child_depth,
|
||||
role_name,
|
||||
/*task_name*/ None,
|
||||
)?),
|
||||
SpawnAgentOptions {
|
||||
fork_parent_spawn_call_id: args.fork_context.then(|| call_id.clone()),
|
||||
fork_mode: args.fork_context.then_some(SpawnAgentForkMode::FullHistory),
|
||||
},
|
||||
))
|
||||
.await
|
||||
.map_err(collab_spawn_error);
|
||||
let (new_thread_id, new_agent_metadata, status) = match &result {
|
||||
Ok(spawned_agent) => (
|
||||
Some(spawned_agent.thread_id),
|
||||
|
||||
@@ -116,7 +116,8 @@ impl ToolOrchestrator {
|
||||
let otel = turn_ctx.session_telemetry.clone();
|
||||
let otel_tn = &tool_ctx.tool_name;
|
||||
let otel_ci = &tool_ctx.call_id;
|
||||
let use_guardian = routes_approval_to_guardian(turn_ctx);
|
||||
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;
|
||||
|
||||
// 1) Approval
|
||||
let mut already_approved = false;
|
||||
@@ -126,12 +127,37 @@ impl ToolOrchestrator {
|
||||
});
|
||||
match requirement {
|
||||
ExecApprovalRequirement::Skip { .. } => {
|
||||
otel.tool_decision(
|
||||
otel_tn,
|
||||
otel_ci,
|
||||
&ReviewDecision::Approved,
|
||||
ToolDecisionSource::Config,
|
||||
);
|
||||
if strict_auto_review {
|
||||
let guardian_review_id = Some(new_guardian_review_id());
|
||||
let approval_ctx = ApprovalCtx {
|
||||
session: &tool_ctx.session,
|
||||
turn: &tool_ctx.turn,
|
||||
call_id: &tool_ctx.call_id,
|
||||
guardian_review_id: guardian_review_id.clone(),
|
||||
retry_reason: None,
|
||||
network_approval_context: None,
|
||||
};
|
||||
let decision = Self::request_approval(
|
||||
tool,
|
||||
req,
|
||||
tool_ctx.call_id.as_str(),
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
/*evaluate_permission_request_hooks*/ false,
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
|
||||
.await?;
|
||||
already_approved = true;
|
||||
} else {
|
||||
otel.tool_decision(
|
||||
otel_tn,
|
||||
otel_ci,
|
||||
&ReviewDecision::Approved,
|
||||
ToolDecisionSource::Config,
|
||||
);
|
||||
}
|
||||
}
|
||||
ExecApprovalRequirement::Forbidden { reason } => {
|
||||
return Err(ToolError::Rejected(reason));
|
||||
@@ -152,35 +178,13 @@ impl ToolOrchestrator {
|
||||
tool_ctx.call_id.as_str(),
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
use_guardian,
|
||||
/*evaluate_permission_request_hooks*/ !strict_auto_review,
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
let reason = if let Some(review_id) = guardian_review_id.as_deref() {
|
||||
guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await
|
||||
} else {
|
||||
"rejected by user".to_string()
|
||||
};
|
||||
return Err(ToolError::Rejected(reason));
|
||||
}
|
||||
ReviewDecision::TimedOut => {
|
||||
return Err(ToolError::Rejected(guardian_timeout_message()));
|
||||
}
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::ApprovedForSession => {}
|
||||
ReviewDecision::NetworkPolicyAmendment {
|
||||
network_policy_amendment,
|
||||
} => match network_policy_amendment.action {
|
||||
NetworkPolicyRuleAction::Allow => {}
|
||||
NetworkPolicyRuleAction::Deny => {
|
||||
return Err(ToolError::Rejected("rejected by user".to_string()));
|
||||
}
|
||||
},
|
||||
}
|
||||
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
|
||||
.await?;
|
||||
already_approved = true;
|
||||
}
|
||||
}
|
||||
@@ -287,9 +291,10 @@ impl ToolOrchestrator {
|
||||
build_denial_reason_from_output(output.as_ref())
|
||||
};
|
||||
|
||||
// Ask for approval before retrying with the escalated sandbox.
|
||||
let bypass_retry_approval = tool
|
||||
.should_bypass_approval(approval_policy, already_approved)
|
||||
// Strict auto-review approval covers the sandboxed attempt only;
|
||||
// retrying without the sandbox requires a fresh guardian review.
|
||||
let bypass_retry_approval = !strict_auto_review
|
||||
&& tool.should_bypass_approval(approval_policy, already_approved)
|
||||
&& network_approval_context.is_none();
|
||||
if !bypass_retry_approval {
|
||||
let guardian_review_id = use_guardian.then(new_guardian_review_id);
|
||||
@@ -309,36 +314,13 @@ impl ToolOrchestrator {
|
||||
&permission_request_run_id,
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
use_guardian,
|
||||
/*evaluate_permission_request_hooks*/ !strict_auto_review,
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
let reason = if let Some(review_id) = guardian_review_id.as_deref() {
|
||||
guardian_rejection_message(tool_ctx.session.as_ref(), review_id)
|
||||
.await
|
||||
} else {
|
||||
"rejected by user".to_string()
|
||||
};
|
||||
return Err(ToolError::Rejected(reason));
|
||||
}
|
||||
ReviewDecision::TimedOut => {
|
||||
return Err(ToolError::Rejected(guardian_timeout_message()));
|
||||
}
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::ApprovedForSession => {}
|
||||
ReviewDecision::NetworkPolicyAmendment {
|
||||
network_policy_amendment,
|
||||
} => match network_policy_amendment.action {
|
||||
NetworkPolicyRuleAction::Allow => {}
|
||||
NetworkPolicyRuleAction::Deny => {
|
||||
return Err(ToolError::Rejected("rejected by user".to_string()));
|
||||
}
|
||||
},
|
||||
}
|
||||
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
|
||||
.await?;
|
||||
}
|
||||
|
||||
let escalated_attempt = SandboxAttempt {
|
||||
@@ -385,13 +367,15 @@ impl ToolOrchestrator {
|
||||
permission_request_run_id: &str,
|
||||
approval_ctx: ApprovalCtx<'_>,
|
||||
tool_ctx: &ToolCtx,
|
||||
use_guardian: bool,
|
||||
evaluate_permission_request_hooks: bool,
|
||||
otel: &codex_otel::SessionTelemetry,
|
||||
) -> Result<ReviewDecision, ToolError>
|
||||
where
|
||||
T: ToolRuntime<Rq, Out>,
|
||||
{
|
||||
if let Some(permission_request) = tool.permission_request_payload(req) {
|
||||
if evaluate_permission_request_hooks
|
||||
&& let Some(permission_request) = tool.permission_request_payload(req)
|
||||
{
|
||||
match run_permission_request_hooks(
|
||||
approval_ctx.session,
|
||||
approval_ctx.turn,
|
||||
@@ -424,12 +408,12 @@ impl ToolOrchestrator {
|
||||
}
|
||||
}
|
||||
|
||||
let decision = tool.start_approval_async(req, approval_ctx).await;
|
||||
let otel_source = if use_guardian {
|
||||
let otel_source = if approval_ctx.guardian_review_id.is_some() {
|
||||
ToolDecisionSource::AutomatedReviewer
|
||||
} else {
|
||||
ToolDecisionSource::User
|
||||
};
|
||||
let decision = tool.start_approval_async(req, approval_ctx).await;
|
||||
otel.tool_decision(
|
||||
&tool_ctx.tool_name,
|
||||
&tool_ctx.call_id,
|
||||
@@ -438,6 +422,35 @@ impl ToolOrchestrator {
|
||||
);
|
||||
Ok(decision)
|
||||
}
|
||||
|
||||
async fn reject_if_not_approved(
|
||||
tool_ctx: &ToolCtx,
|
||||
guardian_review_id: Option<&str>,
|
||||
decision: ReviewDecision,
|
||||
) -> Result<(), ToolError> {
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
let reason = if let Some(review_id) = guardian_review_id {
|
||||
guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await
|
||||
} else {
|
||||
"rejected by user".to_string()
|
||||
};
|
||||
Err(ToolError::Rejected(reason))
|
||||
}
|
||||
ReviewDecision::TimedOut => Err(ToolError::Rejected(guardian_timeout_message())),
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::ApprovedForSession => Ok(()),
|
||||
ReviewDecision::NetworkPolicyAmendment {
|
||||
network_policy_amendment,
|
||||
} => match network_policy_amendment.action {
|
||||
NetworkPolicyRuleAction::Allow => Ok(()),
|
||||
NetworkPolicyRuleAction::Deny => {
|
||||
Err(ToolError::Rejected("rejected by user".to_string()))
|
||||
}
|
||||
},
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String {
|
||||
|
||||
@@ -139,14 +139,14 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
|
||||
let changes = req.changes.clone();
|
||||
let guardian_review_id = ctx.guardian_review_id.clone();
|
||||
Box::pin(async move {
|
||||
if req.permissions_preapproved && retry_reason.is_none() {
|
||||
return ReviewDecision::Approved;
|
||||
}
|
||||
if let Some(review_id) = guardian_review_id {
|
||||
let action = ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id);
|
||||
return review_approval_request(session, turn, review_id, action, retry_reason)
|
||||
.await;
|
||||
}
|
||||
if req.permissions_preapproved && retry_reason.is_none() {
|
||||
return ReviewDecision::Approved;
|
||||
}
|
||||
if let Some(reason) = retry_reason {
|
||||
let rx_approve = session
|
||||
.request_patch_approval(
|
||||
|
||||
@@ -483,6 +483,7 @@ async fn request_permissions_tool_is_auto_denied_when_granular_request_permissio
|
||||
RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
}
|
||||
);
|
||||
|
||||
@@ -1089,6 +1090,7 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls() -> Resul
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1203,6 +1205,7 @@ async fn request_permissions_preapprove_explicit_exec_permissions_outside_on_req
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1316,6 +1319,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls() -> Resu
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1425,6 +1429,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls_without_i
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1571,6 +1576,7 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions()
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: granted_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1692,6 +1698,7 @@ async fn request_permissions_grants_do_not_carry_across_turns() -> Result<()> {
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -1809,6 +1816,7 @@ async fn request_permissions_session_grants_carry_across_turns() -> Result<()> {
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
@@ -4,6 +4,7 @@
|
||||
use anyhow::Result;
|
||||
use codex_core::config::Constrained;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::models::FileSystemPermissions;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
@@ -134,6 +135,7 @@ async fn submit_turn(
|
||||
prompt: &str,
|
||||
approval_policy: AskForApproval,
|
||||
sandbox_policy: SandboxPolicy,
|
||||
approvals_reviewer: Option<ApprovalsReviewer>,
|
||||
) -> Result<()> {
|
||||
let session_model = test.session_configured.model.clone();
|
||||
test.codex
|
||||
@@ -146,7 +148,7 @@ async fn submit_turn(
|
||||
final_output_json_schema: None,
|
||||
cwd: test.cwd.path().to_path_buf(),
|
||||
approval_policy,
|
||||
approvals_reviewer: None,
|
||||
approvals_reviewer,
|
||||
sandbox_policy,
|
||||
model: session_model,
|
||||
effort: None,
|
||||
@@ -159,6 +161,13 @@ async fn submit_turn(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn wait_for_completion(test: &TestCodex) {
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
})
|
||||
.await;
|
||||
}
|
||||
|
||||
async fn expect_request_permissions_event(
|
||||
test: &TestCodex,
|
||||
expected_call_id: &str,
|
||||
@@ -248,6 +257,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s
|
||||
"write outside the workspace",
|
||||
approval_policy,
|
||||
sandbox_policy,
|
||||
/*approvals_reviewer*/ None,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -262,6 +272,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
@@ -305,11 +316,17 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
#[cfg(target_os = "macos")]
|
||||
async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_without_prompt()
|
||||
-> Result<()> {
|
||||
async fn approved_folder_write_request_permissions_unblocks_later_apply_patch() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_sandbox!(Ok(()));
|
||||
|
||||
apply_patch_after_request_permissions(/*strict_auto_review*/ false).await?;
|
||||
apply_patch_after_request_permissions(/*strict_auto_review*/ true).await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn apply_patch_after_request_permissions(strict_auto_review: bool) -> Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
let approval_policy = AskForApproval::OnRequest;
|
||||
let sandbox_policy = workspace_write_excluding_tmp();
|
||||
@@ -330,43 +347,72 @@ async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_wi
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
let requested_dir = tempfile::tempdir()?;
|
||||
let requested_file = requested_dir.path().join("allowed-patch.txt");
|
||||
let requested_file_name = if strict_auto_review {
|
||||
"strict-allowed-patch.txt"
|
||||
} else {
|
||||
"allowed-patch.txt"
|
||||
};
|
||||
let patch_content = if strict_auto_review {
|
||||
"patched-after-strict-review"
|
||||
} else {
|
||||
"patched-via-request-permissions"
|
||||
};
|
||||
let requested_file = requested_dir.path().join(requested_file_name);
|
||||
let requested_permissions = requested_directory_write_permissions(requested_dir.path());
|
||||
let normalized_requested_permissions =
|
||||
normalized_directory_write_permissions(requested_dir.path())?;
|
||||
let patch = build_add_file_patch(&requested_file, "patched-via-request-permissions");
|
||||
let patch = build_add_file_patch(&requested_file, patch_content);
|
||||
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-request-permissions-patch-1"),
|
||||
request_permissions_tool_event(
|
||||
"permissions-call",
|
||||
"Allow patching outside the workspace",
|
||||
&requested_permissions,
|
||||
)?,
|
||||
ev_completed("resp-request-permissions-patch-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-request-permissions-patch-2"),
|
||||
ev_apply_patch_function_call("apply-patch-call", &patch),
|
||||
ev_completed("resp-request-permissions-patch-2"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-request-permissions-patch-3"),
|
||||
ev_assistant_message("msg-request-permissions-patch-1", "done"),
|
||||
ev_completed("resp-request-permissions-patch-3"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
let response_prefix = if strict_auto_review {
|
||||
"resp-strict-request-permissions-patch"
|
||||
} else {
|
||||
"resp-request-permissions-patch"
|
||||
};
|
||||
let mut sse_sequence = vec![
|
||||
sse(vec![
|
||||
ev_response_created(&format!("{response_prefix}-1")),
|
||||
request_permissions_tool_event(
|
||||
"permissions-call",
|
||||
"Allow patching outside the workspace",
|
||||
&requested_permissions,
|
||||
)?,
|
||||
ev_completed(&format!("{response_prefix}-1")),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created(&format!("{response_prefix}-2")),
|
||||
ev_apply_patch_function_call("apply-patch-call", &patch),
|
||||
ev_completed(&format!("{response_prefix}-2")),
|
||||
]),
|
||||
];
|
||||
if strict_auto_review {
|
||||
sse_sequence.push(sse(vec![
|
||||
ev_response_created(&format!("{response_prefix}-guardian")),
|
||||
ev_assistant_message(
|
||||
"msg-strict-request-permissions-patch-guardian",
|
||||
&serde_json::json!({
|
||||
"risk_level": "low",
|
||||
"user_authorization": "high",
|
||||
"outcome": "allow",
|
||||
"rationale": "The patch stays within the strict turn grant.",
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
ev_completed(&format!("{response_prefix}-guardian")),
|
||||
]));
|
||||
}
|
||||
sse_sequence.push(sse(vec![
|
||||
ev_response_created(&format!("{response_prefix}-3")),
|
||||
ev_assistant_message("msg-request-permissions-patch-1", "done"),
|
||||
ev_completed(&format!("{response_prefix}-3")),
|
||||
]));
|
||||
let responses = mount_sse_sequence(&server, sse_sequence).await;
|
||||
|
||||
submit_turn(
|
||||
&test,
|
||||
"patch outside the workspace",
|
||||
approval_policy,
|
||||
sandbox_policy,
|
||||
strict_auto_review.then_some(ApprovalsReviewer::User),
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -381,26 +427,38 @@ async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_wi
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
let event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ApplyPatchApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
match event {
|
||||
EventMsg::TurnComplete(_) => {}
|
||||
EventMsg::ApplyPatchApprovalRequest(approval) => {
|
||||
panic!(
|
||||
"unexpected apply_patch approval request after granted permissions: {:?}",
|
||||
approval.call_id
|
||||
if strict_auto_review {
|
||||
wait_for_completion(&test).await;
|
||||
let guardian_request = responses
|
||||
.requests()
|
||||
.into_iter()
|
||||
.find(|request| request.body_contains_text(requested_file_name))
|
||||
.expect("expected guardian request for strict apply_patch");
|
||||
assert!(guardian_request.body_contains_text(requested_file_name));
|
||||
assert!(guardian_request.body_contains_text(patch_content));
|
||||
} else {
|
||||
let event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ApplyPatchApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
match event {
|
||||
EventMsg::TurnComplete(_) => {}
|
||||
EventMsg::ApplyPatchApprovalRequest(approval) => {
|
||||
panic!(
|
||||
"unexpected apply_patch approval request after granted permissions: {:?}",
|
||||
approval.call_id
|
||||
)
|
||||
}
|
||||
other => panic!("unexpected event: {other:?}"),
|
||||
}
|
||||
other => panic!("unexpected event: {other:?}"),
|
||||
}
|
||||
|
||||
let patch_output = responses
|
||||
@@ -415,7 +473,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_wi
|
||||
);
|
||||
assert_eq!(
|
||||
fs::read_to_string(&requested_file)?,
|
||||
"patched-via-request-permissions\n"
|
||||
format!("{patch_content}\n")
|
||||
);
|
||||
|
||||
Ok(())
|
||||
|
||||
@@ -664,6 +664,69 @@ async fn remote_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() -
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
|
||||
#[serial(mcp_cwd)]
|
||||
async fn local_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() -> anyhow::Result<()>
|
||||
{
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = responses::start_mock_server().await;
|
||||
let server_name = "rmcp_local_fallback_cwd";
|
||||
let expected_cwd = Arc::new(Mutex::new(None::<PathBuf>));
|
||||
let expected_cwd_for_config = Arc::clone(&expected_cwd);
|
||||
let rmcp_test_server_bin = cargo_bin("test_stdio_server")?;
|
||||
let relative_server_path = PathBuf::from("mcp-bin").join(
|
||||
rmcp_test_server_bin
|
||||
.file_name()
|
||||
.expect("test stdio server binary should have a file name"),
|
||||
);
|
||||
let relative_command = relative_server_path.to_string_lossy().into_owned();
|
||||
|
||||
let fixture = test_codex()
|
||||
.with_config(move |config| {
|
||||
*expected_cwd_for_config
|
||||
.lock()
|
||||
.expect("expected cwd lock should not be poisoned") =
|
||||
Some(config.cwd.to_path_buf());
|
||||
|
||||
let target_bin = config.cwd.join(&relative_server_path).into_path_buf();
|
||||
let target_dir = target_bin
|
||||
.parent()
|
||||
.expect("relative test server path should include a parent");
|
||||
fs::create_dir_all(target_dir).expect("create relative MCP bin directory");
|
||||
fs::copy(&rmcp_test_server_bin, &target_bin).expect("copy test stdio server");
|
||||
|
||||
insert_mcp_server(
|
||||
config,
|
||||
server_name,
|
||||
stdio_transport(
|
||||
relative_command,
|
||||
Some(HashMap::from([(
|
||||
"MCP_TEST_VALUE".to_string(),
|
||||
"local-fallback-cwd".to_string(),
|
||||
)])),
|
||||
Vec::new(),
|
||||
),
|
||||
TestMcpServerOptions::default(),
|
||||
);
|
||||
})
|
||||
.build(&server)
|
||||
.await?;
|
||||
|
||||
let expected_cwd = expected_cwd
|
||||
.lock()
|
||||
.expect("expected cwd lock should not be poisoned")
|
||||
.clone()
|
||||
.expect("test config should record runtime fallback cwd");
|
||||
let structured =
|
||||
call_cwd_tool(&server, &fixture, server_name, "call-local-fallback-cwd").await?;
|
||||
|
||||
assert_cwd_tool_output(&structured, &expected_cwd);
|
||||
server.verify().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
|
||||
async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
@@ -917,12 +917,8 @@ pub const FEATURES: &[FeatureSpec] = &[
|
||||
FeatureSpec {
|
||||
id: Feature::GuardianApproval,
|
||||
key: "guardian_approval",
|
||||
stage: Stage::Experimental {
|
||||
name: "Auto-review",
|
||||
menu_description: "When Codex needs approval for higher-risk actions (e.g. sandbox escapes or blocked network access), route eligible approval requests to a carefully-prompted security reviewer subagent rather than blocking the agent on your input. This can consume significantly more tokens because it runs a subagent on every approval request.",
|
||||
announcement: "",
|
||||
},
|
||||
default_enabled: false,
|
||||
stage: Stage::Stable,
|
||||
default_enabled: true,
|
||||
},
|
||||
FeatureSpec {
|
||||
id: Feature::CollaborationModes,
|
||||
|
||||
@@ -87,20 +87,11 @@ fn code_mode_only_requires_code_mode() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_approval_is_experimental_and_user_toggleable() {
|
||||
fn guardian_approval_is_stable_and_enabled_by_default() {
|
||||
let spec = Feature::GuardianApproval.info();
|
||||
let stage = spec.stage;
|
||||
|
||||
assert!(matches!(stage, Stage::Experimental { .. }));
|
||||
assert_eq!(stage.experimental_menu_name(), Some("Auto-review"));
|
||||
assert_eq!(
|
||||
stage.experimental_menu_description().map(str::to_owned),
|
||||
Some(
|
||||
"When Codex needs approval for higher-risk actions (e.g. sandbox escapes or blocked network access), route eligible approval requests to a carefully-prompted security reviewer subagent rather than blocking the agent on your input. This can consume significantly more tokens because it runs a subagent on every approval request.".to_string()
|
||||
)
|
||||
);
|
||||
assert_eq!(stage.experimental_announcement(), None);
|
||||
assert_eq!(Feature::GuardianApproval.default_enabled(), false);
|
||||
assert_eq!(spec.stage, Stage::Stable);
|
||||
assert_eq!(Feature::GuardianApproval.default_enabled(), true);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -58,6 +58,9 @@ pub struct RequestPermissionsResponse {
|
||||
pub permissions: RequestPermissionProfile,
|
||||
#[serde(default)]
|
||||
pub scope: PermissionGrantScope,
|
||||
/// Review every subsequent command in this turn before normal sandboxed execution.
|
||||
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
|
||||
pub strict_auto_review: bool,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)]
|
||||
|
||||
@@ -12,6 +12,7 @@
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::ffi::OsString;
|
||||
use std::path::Path;
|
||||
|
||||
/// Resolves a program to its executable path on Unix systems.
|
||||
///
|
||||
@@ -19,7 +20,11 @@ use std::ffi::OsString;
|
||||
/// the kernel's shebang (`#!`) mechanism, so this function simply returns
|
||||
/// the program name unchanged.
|
||||
#[cfg(unix)]
|
||||
pub fn resolve(program: OsString, _env: &HashMap<OsString, OsString>) -> std::io::Result<OsString> {
|
||||
pub fn resolve(
|
||||
program: OsString,
|
||||
_env: &HashMap<OsString, OsString>,
|
||||
_cwd: &Path,
|
||||
) -> std::io::Result<OsString> {
|
||||
Ok(program)
|
||||
}
|
||||
|
||||
@@ -33,16 +38,16 @@ pub fn resolve(program: OsString, _env: &HashMap<OsString, OsString>) -> std::io
|
||||
/// This enables tools like `npx`, `pnpm`, and `yarn` to work correctly on Windows
|
||||
/// without requiring users to specify full paths or extensions in their configuration.
|
||||
#[cfg(windows)]
|
||||
pub fn resolve(program: OsString, env: &HashMap<OsString, OsString>) -> std::io::Result<OsString> {
|
||||
// Get current directory for relative path resolution
|
||||
let cwd = std::env::current_dir()
|
||||
.map_err(|e| std::io::Error::other(format!("Failed to get current directory: {e}")))?;
|
||||
|
||||
pub fn resolve(
|
||||
program: OsString,
|
||||
env: &HashMap<OsString, OsString>,
|
||||
cwd: &Path,
|
||||
) -> std::io::Result<OsString> {
|
||||
// Extract PATH from environment for search locations
|
||||
let search_path = env.get(std::ffi::OsStr::new("PATH"));
|
||||
|
||||
// Attempt resolution via which crate
|
||||
match which::which_in(&program, search_path, &cwd) {
|
||||
match which::which_in(&program, search_path, cwd) {
|
||||
Ok(resolved) => {
|
||||
tracing::debug!("Resolved {program:?} to {resolved:?}");
|
||||
Ok(resolved.into_os_string())
|
||||
@@ -119,7 +124,7 @@ mod tests {
|
||||
let program = OsString::from(&env.program_name);
|
||||
|
||||
// Apply platform-specific resolution
|
||||
let resolved = resolve(program, &env.mcp_env)?;
|
||||
let resolved = resolve(program, &env.mcp_env, std::env::current_dir()?.as_path())?;
|
||||
|
||||
// Verify resolved path executes successfully
|
||||
let mut cmd = Command::new(resolved);
|
||||
|
||||
@@ -162,14 +162,27 @@ impl StdioServerCommand {
|
||||
/// process spawns the configured command and rmcp talks to the child's local
|
||||
/// stdin/stdout pipes directly.
|
||||
#[derive(Clone)]
|
||||
pub struct LocalStdioServerLauncher;
|
||||
pub struct LocalStdioServerLauncher {
|
||||
fallback_cwd: PathBuf,
|
||||
}
|
||||
|
||||
impl LocalStdioServerLauncher {
|
||||
/// Creates a local stdio launcher.
|
||||
///
|
||||
/// `fallback_cwd` is used when the MCP server config omits `cwd`, so
|
||||
/// relative commands resolve from the caller's runtime working directory.
|
||||
pub fn new(fallback_cwd: PathBuf) -> Self {
|
||||
Self { fallback_cwd }
|
||||
}
|
||||
}
|
||||
|
||||
impl StdioServerLauncher for LocalStdioServerLauncher {
|
||||
fn launch(
|
||||
&self,
|
||||
command: StdioServerCommand,
|
||||
) -> BoxFuture<'static, io::Result<StdioServerTransport>> {
|
||||
async move { Self::launch_server(command) }.boxed()
|
||||
let fallback_cwd = self.fallback_cwd.clone();
|
||||
async move { Self::launch_server(command, fallback_cwd) }.boxed()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -193,7 +206,10 @@ mod private {
|
||||
impl private::Sealed for LocalStdioServerLauncher {}
|
||||
|
||||
impl LocalStdioServerLauncher {
|
||||
fn launch_server(command: StdioServerCommand) -> io::Result<StdioServerTransport> {
|
||||
fn launch_server(
|
||||
command: StdioServerCommand,
|
||||
fallback_cwd: PathBuf,
|
||||
) -> io::Result<StdioServerTransport> {
|
||||
let StdioServerCommand {
|
||||
program,
|
||||
args,
|
||||
@@ -203,22 +219,21 @@ impl LocalStdioServerLauncher {
|
||||
} = command;
|
||||
let program_name = program.to_string_lossy().into_owned();
|
||||
let envs = create_env_for_mcp_server(env, &env_vars).map_err(io::Error::other)?;
|
||||
let cwd = cwd.unwrap_or(fallback_cwd);
|
||||
let resolved_program =
|
||||
program_resolver::resolve(program, &envs).map_err(io::Error::other)?;
|
||||
program_resolver::resolve(program, &envs, &cwd).map_err(io::Error::other)?;
|
||||
|
||||
let mut command = Command::new(resolved_program);
|
||||
command
|
||||
.kill_on_drop(true)
|
||||
.stdin(Stdio::piped())
|
||||
.stdout(Stdio::piped())
|
||||
.current_dir(cwd)
|
||||
.env_clear()
|
||||
.envs(envs)
|
||||
.args(args);
|
||||
#[cfg(unix)]
|
||||
command.process_group(0);
|
||||
if let Some(cwd) = cwd {
|
||||
command.current_dir(cwd);
|
||||
}
|
||||
|
||||
let (transport, stderr) = TokioChildProcess::builder(command)
|
||||
.stderr(Stdio::piped())
|
||||
|
||||
@@ -80,7 +80,7 @@ async fn drop_kills_wrapper_process_group() -> Result<()> {
|
||||
)])),
|
||||
&[],
|
||||
/*cwd*/ None,
|
||||
Arc::new(LocalStdioServerLauncher),
|
||||
Arc::new(LocalStdioServerLauncher::new(std::env::current_dir()?)),
|
||||
)
|
||||
.await?;
|
||||
|
||||
|
||||
@@ -63,7 +63,7 @@ async fn rmcp_client_can_list_and_read_resources() -> anyhow::Result<()> {
|
||||
/*env*/ None,
|
||||
&[],
|
||||
/*cwd*/ None,
|
||||
Arc::new(LocalStdioServerLauncher),
|
||||
Arc::new(LocalStdioServerLauncher::new(std::env::current_dir()?)),
|
||||
)
|
||||
.await?;
|
||||
|
||||
|
||||
@@ -183,6 +183,7 @@ impl PendingAppServerRequests {
|
||||
response.permissions.clone(),
|
||||
),
|
||||
scope: response.scope.into(),
|
||||
strict_auto_review: response.strict_auto_review.then_some(true),
|
||||
})
|
||||
.map_err(|err| {
|
||||
format!("failed to serialize permissions approval response: {err}")
|
||||
@@ -543,6 +544,7 @@ mod tests {
|
||||
)),
|
||||
},
|
||||
scope: codex_protocol::request_permissions::PermissionGrantScope::Session,
|
||||
strict_auto_review: false,
|
||||
},
|
||||
})
|
||||
.expect("permissions response should serialize")
|
||||
@@ -564,6 +566,7 @@ mod tests {
|
||||
}),
|
||||
},
|
||||
scope: PermissionGrantScope::Session,
|
||||
strict_auto_review: None,
|
||||
}
|
||||
);
|
||||
|
||||
|
||||
@@ -278,8 +278,8 @@ impl ApprovalOverlay {
|
||||
permissions,
|
||||
..
|
||||
},
|
||||
ApprovalDecision::Review(decision),
|
||||
) => self.handle_permissions_decision(call_id, permissions, decision.clone()),
|
||||
ApprovalDecision::Permissions(decision),
|
||||
) => self.handle_permissions_decision(call_id, permissions, *decision),
|
||||
(ApprovalRequest::ApplyPatch { id, .. }, ApprovalDecision::Review(decision)) => {
|
||||
self.handle_patch_decision(id, decision.clone());
|
||||
}
|
||||
@@ -322,27 +322,31 @@ impl ApprovalOverlay {
|
||||
&self,
|
||||
call_id: &str,
|
||||
permissions: &RequestPermissionProfile,
|
||||
decision: ReviewDecision,
|
||||
decision: PermissionsDecision,
|
||||
) {
|
||||
let Some(request) = self.current_request.as_ref() else {
|
||||
return;
|
||||
};
|
||||
let granted_permissions = match decision {
|
||||
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => permissions.clone(),
|
||||
ReviewDecision::Denied | ReviewDecision::TimedOut | ReviewDecision::Abort => {
|
||||
Default::default()
|
||||
}
|
||||
ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::NetworkPolicyAmendment { .. } => Default::default(),
|
||||
PermissionsDecision::GrantForTurn
|
||||
| PermissionsDecision::GrantForTurnWithStrictAutoReview
|
||||
| PermissionsDecision::GrantForSession => permissions.clone(),
|
||||
PermissionsDecision::Deny => Default::default(),
|
||||
};
|
||||
let scope = if matches!(decision, ReviewDecision::ApprovedForSession) {
|
||||
let scope = if matches!(decision, PermissionsDecision::GrantForSession) {
|
||||
PermissionGrantScope::Session
|
||||
} else {
|
||||
PermissionGrantScope::Turn
|
||||
};
|
||||
let strict_auto_review = matches!(
|
||||
decision,
|
||||
PermissionsDecision::GrantForTurnWithStrictAutoReview
|
||||
);
|
||||
if request.thread_label().is_none() {
|
||||
let message = if granted_permissions.is_empty() {
|
||||
"You did not grant additional permissions"
|
||||
} else if strict_auto_review {
|
||||
"You granted additional permissions with strict auto review"
|
||||
} else if matches!(scope, PermissionGrantScope::Session) {
|
||||
"You granted additional permissions for this session"
|
||||
} else {
|
||||
@@ -359,6 +363,7 @@ impl ApprovalOverlay {
|
||||
codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: granted_permissions,
|
||||
scope,
|
||||
strict_auto_review,
|
||||
},
|
||||
);
|
||||
}
|
||||
@@ -482,7 +487,11 @@ impl BottomPaneView for ApprovalOverlay {
|
||||
permissions,
|
||||
..
|
||||
} => {
|
||||
self.handle_permissions_decision(call_id, permissions, ReviewDecision::Abort);
|
||||
self.handle_permissions_decision(
|
||||
call_id,
|
||||
permissions,
|
||||
PermissionsDecision::Deny,
|
||||
);
|
||||
}
|
||||
ApprovalRequest::ApplyPatch { id, .. } => {
|
||||
self.handle_patch_decision(id, ReviewDecision::Abort);
|
||||
@@ -679,9 +688,18 @@ fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
|
||||
#[derive(Clone)]
|
||||
enum ApprovalDecision {
|
||||
Review(ReviewDecision),
|
||||
Permissions(PermissionsDecision),
|
||||
McpElicitation(ElicitationAction),
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
enum PermissionsDecision {
|
||||
GrantForTurn,
|
||||
GrantForTurnWithStrictAutoReview,
|
||||
GrantForSession,
|
||||
Deny,
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
struct ApprovalOption {
|
||||
label: String,
|
||||
@@ -901,20 +919,28 @@ fn patch_options() -> Vec<ApprovalOption> {
|
||||
fn permissions_options() -> Vec<ApprovalOption> {
|
||||
vec![
|
||||
ApprovalOption {
|
||||
label: "Yes, grant these permissions".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::Approved),
|
||||
label: "Yes, grant these permissions for this turn".to_string(),
|
||||
decision: ApprovalDecision::Permissions(PermissionsDecision::GrantForTurn),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
|
||||
},
|
||||
ApprovalOption {
|
||||
label: "Yes, grant for this turn with strict auto review".to_string(),
|
||||
decision: ApprovalDecision::Permissions(
|
||||
PermissionsDecision::GrantForTurnWithStrictAutoReview,
|
||||
),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('r'))],
|
||||
},
|
||||
ApprovalOption {
|
||||
label: "Yes, grant these permissions for this session".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession),
|
||||
decision: ApprovalDecision::Permissions(PermissionsDecision::GrantForSession),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))],
|
||||
},
|
||||
ApprovalOption {
|
||||
label: "No, continue without permissions".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::Denied),
|
||||
decision: ApprovalDecision::Permissions(PermissionsDecision::Deny),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
|
||||
},
|
||||
@@ -1347,7 +1373,8 @@ mod tests {
|
||||
assert_eq!(
|
||||
labels,
|
||||
vec![
|
||||
"Yes, grant these permissions".to_string(),
|
||||
"Yes, grant these permissions for this turn".to_string(),
|
||||
"Yes, grant for this turn with strict auto review".to_string(),
|
||||
"Yes, grant these permissions for this session".to_string(),
|
||||
"No, continue without permissions".to_string(),
|
||||
]
|
||||
@@ -1410,6 +1437,34 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn permissions_strict_auto_review_shortcut_submits_turn_scope_with_strict_review() {
|
||||
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('r'), 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::Turn);
|
||||
assert!(response.strict_auto_review);
|
||||
saw_op = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
assert!(
|
||||
saw_op,
|
||||
"expected permission approval decision to emit a strict auto review response"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn additional_permissions_prompt_shows_permission_rule_line() {
|
||||
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
||||
|
||||
+4
-3
@@ -9,8 +9,9 @@ expression: "normalize_snapshot_paths(render_overlay_lines(&view, 120))"
|
||||
|
||||
Permission rule: network; read `/tmp/readme.txt`; write `/tmp/out.txt`
|
||||
|
||||
› 1. Yes, grant these permissions (y)
|
||||
2. Yes, grant these permissions for this session (a)
|
||||
3. No, continue without permissions (n)
|
||||
› 1. Yes, grant these permissions for this turn (y)
|
||||
2. Yes, grant for this turn with strict auto review (r)
|
||||
3. Yes, grant these permissions for this session (a)
|
||||
4. No, continue without permissions (n)
|
||||
|
||||
Press enter to confirm or esc to cancel
|
||||
|
||||
@@ -401,7 +401,7 @@ async fn permissions_selection_hides_auto_review_when_feature_disabled() {
|
||||
|
||||
assert!(
|
||||
!popup.contains("Auto-review"),
|
||||
"expected Auto-review to stay hidden until the experimental feature is enabled: {popup}"
|
||||
"expected Auto-review to stay hidden until the feature is enabled: {popup}"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -433,7 +433,7 @@ async fn permissions_selection_hides_auto_review_when_feature_disabled_even_if_a
|
||||
|
||||
assert!(
|
||||
!popup.contains("Auto-review"),
|
||||
"expected Auto-review to stay hidden when the experimental feature is disabled: {popup}"
|
||||
"expected Auto-review to stay hidden when the feature is disabled: {popup}"
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use super::*;
|
||||
use codex_app_server_protocol::AppInfo;
|
||||
use codex_features::Stage;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[tokio::test]
|
||||
@@ -1747,31 +1748,22 @@ async fn experimental_popup_shows_js_repl_node_requirement() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn experimental_popup_includes_guardian_approval() {
|
||||
async fn experimental_popup_omits_stable_guardian_approval() {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let guardian_stage = FEATURES
|
||||
.iter()
|
||||
.find(|spec| spec.id == Feature::GuardianApproval)
|
||||
.map(|spec| spec.stage)
|
||||
.expect("expected guardian approval feature metadata");
|
||||
let guardian_name = guardian_stage
|
||||
.experimental_menu_name()
|
||||
.expect("expected guardian approval experimental menu name");
|
||||
let guardian_description = guardian_stage
|
||||
.experimental_menu_description()
|
||||
.expect("expected guardian approval experimental description");
|
||||
|
||||
assert_eq!(guardian_stage, Stage::Stable);
|
||||
|
||||
chat.open_experimental_popup();
|
||||
|
||||
let popup = render_bottom_popup(&chat, /*width*/ 120);
|
||||
let normalized_popup = popup.split_whitespace().collect::<Vec<_>>().join(" ");
|
||||
assert!(
|
||||
popup.contains(guardian_name),
|
||||
"expected auto-review entry in experimental popup, got:\n{popup}"
|
||||
);
|
||||
assert!(
|
||||
normalized_popup.contains(guardian_description),
|
||||
"expected auto-review description in experimental popup, got:\n{popup}"
|
||||
!popup.contains("Auto-review"),
|
||||
"expected stable auto-review feature to be omitted from experimental popup, got:\n{popup}"
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user