mirror of
https://github.com/openai/codex.git
synced 2026-11-07 20:30:33 +00:00
## Why Tool actions and approval retries can outlive the step that issued them. If approval handling reads newer turn settings, a delayed action can use the wrong approval policy or reviewer. ## What changed - Capture the approval policy and reviewer in `GuardianReviewContext` from the issuing step. - Use step settings for command, patch, permission, sandbox-retry, and remote-network approval decisions. - Use the active task's published settings for background network approvals and long-lived Unix shell interception. - Preserve explicit MCP reviewer overrides. ## Testing Added regression coverage for permission requests, same-turn network policy changes, delayed commands after a new turn starts, sandbox retries, patch policy selection, and MCP reviewer overrides. GitOrigin-RevId: b7e84a2e1e79a71502272b15b5ef5b0e5643f8b0
155 lines
5.3 KiB
Rust
155 lines
5.3 KiB
Rust
use super::*;
|
|
use crate::session::tests::make_session_and_context_with_rx;
|
|
use codex_models_manager::model_info::model_info_from_slug;
|
|
use codex_protocol::approvals::NetworkPolicyAmendment;
|
|
use pretty_assertions::assert_eq;
|
|
|
|
#[test]
|
|
fn approval_resolution_rejects_denied_network_policy_amendment() {
|
|
let resolution = ApprovalResolution {
|
|
decision: ReviewDecision::NetworkPolicyAmendment {
|
|
network_policy_amendment: NetworkPolicyAmendment {
|
|
host: "denied.example.com".to_string(),
|
|
action: NetworkPolicyRuleAction::Deny,
|
|
},
|
|
},
|
|
source: ApprovalResolutionSource::User,
|
|
};
|
|
|
|
assert!(matches!(
|
|
resolution.into_tool_result(&model_info_from_slug("acting-model")),
|
|
Err(ToolError::Rejected(rejection)) if rejection == "rejected by user"
|
|
));
|
|
}
|
|
|
|
#[test]
|
|
fn approval_resolution_rejects_mcp_policy_amendment() {
|
|
let resolution = ApprovalResolution {
|
|
decision: ReviewDecision::ApprovedMcpPolicyAmendment,
|
|
source: ApprovalResolutionSource::User,
|
|
};
|
|
|
|
assert!(matches!(
|
|
resolution.into_tool_result(&model_info_from_slug("acting-model")),
|
|
Err(ToolError::Rejected(rejection)) if rejection == "Error while requesting approval"
|
|
));
|
|
}
|
|
|
|
#[test]
|
|
fn approval_resolution_aborts_turn_when_approval_is_aborted() {
|
|
let resolution = ApprovalResolution {
|
|
decision: ReviewDecision::Abort,
|
|
source: ApprovalResolutionSource::User,
|
|
};
|
|
|
|
assert!(matches!(
|
|
resolution.into_tool_result(&model_info_from_slug("acting-model")),
|
|
Err(ToolError::Codex(error))
|
|
if matches!(
|
|
error.details(),
|
|
codex_protocol::error::CodexErrorDetails::TurnAborted
|
|
)
|
|
));
|
|
}
|
|
|
|
#[test]
|
|
fn approval_resolution_uses_acting_model_timeout_instructions() {
|
|
let mut model = model_info_from_slug("acting-model");
|
|
for timeout_instructions in ["Catalog timeout instructions.", ""] {
|
|
model.model_messages = Some(
|
|
serde_json::from_value(serde_json::json!({
|
|
"auto_review": {
|
|
"timeout_instructions": timeout_instructions,
|
|
},
|
|
}))
|
|
.expect("model messages should deserialize"),
|
|
);
|
|
let resolution = ApprovalResolution {
|
|
decision: ReviewDecision::TimedOut,
|
|
source: ApprovalResolutionSource::Guardian,
|
|
};
|
|
|
|
assert!(matches!(
|
|
resolution.into_tool_result(&model),
|
|
Err(ToolError::Rejected(rejection)) if rejection == timeout_instructions
|
|
));
|
|
}
|
|
}
|
|
|
|
#[test]
|
|
fn guardian_cwd_preserves_drive_shaped_local_posix_path() {
|
|
let native_cwd = AbsolutePathBuf::try_from(std::path::PathBuf::from("/C:/workspace"))
|
|
.expect("drive-shaped POSIX path should be absolute");
|
|
let cwd = PathUri::from_abs_path(&native_cwd);
|
|
|
|
assert_eq!(
|
|
guardian_cwd(codex_exec_server::LOCAL_ENVIRONMENT_ID, cwd)
|
|
.expect("local cwd should retain the host path convention"),
|
|
native_cwd
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn guardian_cwd_rejects_foreign_remote_path() {
|
|
let cwd = PathUri::parse("file:///C:/workspace").expect("valid Windows path URI");
|
|
|
|
assert!(guardian_cwd(codex_exec_server::REMOTE_ENVIRONMENT_ID, cwd).is_err());
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn explicit_mcp_reviewer_override_takes_precedence_over_action_context() {
|
|
let (session, turn, events) = make_session_and_context_with_rx().await;
|
|
let action = ApprovalAction::McpToolCall {
|
|
id: "mcp-override".to_string(),
|
|
server: "example".to_string(),
|
|
tool_name: "dangerous".to_string(),
|
|
arguments: None,
|
|
connector_id: None,
|
|
connector_name: None,
|
|
connector_description: None,
|
|
connected_account_email: None,
|
|
tool_title: None,
|
|
tool_description: None,
|
|
annotations: None,
|
|
hook_tool_name: HookToolName::new("mcp__example__dangerous"),
|
|
approval_policy: AskForApproval::OnRequest,
|
|
reviewer: ApprovalsReviewer::User,
|
|
approval_mode: AppToolApproval::Prompt,
|
|
allow_session_remember: false,
|
|
allow_persistent_approval: false,
|
|
};
|
|
let mut review_context = GuardianReviewContext::from(&turn);
|
|
review_context.approval_policy = AskForApproval::OnRequest;
|
|
review_context.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
|
let context = ApprovalContext {
|
|
review_context,
|
|
cancellation_token: None,
|
|
call_id: "mcp-override".to_string(),
|
|
tool_name: ToolName::plain("dangerous"),
|
|
strict_auto_review: false,
|
|
approval_reason: None,
|
|
retry_reason: None,
|
|
network_approval_context: None,
|
|
};
|
|
|
|
tokio::select! {
|
|
resolution = session.request_reviewer_approval(action, &context) => {
|
|
panic!("expected a user approval request, got {resolution:?}");
|
|
}
|
|
event = events.recv() => {
|
|
let codex_protocol::protocol::EventMsg::ElicitationRequest(request) =
|
|
event.expect("receive user approval request").msg
|
|
else {
|
|
panic!("expected an MCP user approval request");
|
|
};
|
|
assert_eq!(request.server_name, "example");
|
|
assert_eq!(
|
|
request.id,
|
|
codex_protocol::mcp::RequestId::String(
|
|
"mcp_tool_call_approval_mcp-override".to_string()
|
|
)
|
|
);
|
|
}
|
|
}
|
|
}
|