openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-29 19:08:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

Add opt-in synchronous Guardian review for native tool calls (#57433)

Browse Source 
## What changed

- Add `CoreToolRuntime::requires_guardian_review` to review opted-in function calls before dispatch and stop execution when review rejects the call.
- Deliver proposed calls as native review evidence, preserving encrypted argument metadata. When hooks rewrite arguments, review the updated arguments and discard the original encrypted replay content.
- Deduplicate native call evidence against reviewer history and include paired evidence outputs in input budgets.
- Keep review accounting and metrics for native calls without emitting action-specific client assessment events, and omit approval payloads from debug output.

## Testing

Extend regression coverage for rewritten native calls across review retries, denied calls that never execute their handlers, oversized native evidence, and review metrics without client events.

GitOrigin-RevId: c3b22c0220344581c2b38f233001abaed49cba34
This commit is contained in:
jif
2026-10-27 11:37:24 +00:00
committed by copyberry
parent b6ad8ea1fc
commit ed1b92fa45
34 changed files with 669 additions and 184 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/guardian/approval_request.rs
+26 -7
View File
@@ -9,8 +9,12 @@ use codex_utils_path_uri::PathUri;
use serde::Serialize;
use serde_json::Value;
#[derive(Debug, Clone, PartialEq)]
#[derive(Clone, PartialEq)]
pub(crate) enum GuardianApprovalRequest {
NativeToolCall {
id: String,
call: Box<codex_protocol::models::ResponseItem>,
},
ExecCommand {
id: String,
environment_id: String,
@@ -73,6 +77,16 @@ pub(crate) enum GuardianApprovalRequest {
},
}
// Approval arguments may contain credentials or encrypted tool messages.
impl std::fmt::Debug for GuardianApprovalRequest {
fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
formatter
.debug_struct("GuardianApprovalRequest")
.field("kind", &std::mem::discriminant(self))
.finish_non_exhaustive()
}
}
impl GuardianApprovalRequest {
/// The environment owning this action, independent of the reviewer's default environment.
pub(super) fn target_environment_id(&self) -> Option<&str> {
@@ -82,7 +96,7 @@ impl GuardianApprovalRequest {
| Self::ApplyPatch { environment_id, .. }
| Self::RequestPermissions { environment_id, .. }
| Self::NetworkAccess { environment_id, .. } => Some(environment_id),
Self::McpToolCall { .. } => None,
Self::McpToolCall { .. } | Self::NativeToolCall { .. } => None,
}
}
}
@@ -231,6 +245,7 @@ pub(crate) fn guardian_approval_request_to_json(
action: &GuardianApprovalRequest,
) -> serde_json::Result<Value> {
let mut value = match action {
GuardianApprovalRequest::NativeToolCall { call, .. } => serde_json::to_value(call.as_ref()),
GuardianApprovalRequest::ExecCommand {
id: _,
environment_id: _,
@@ -348,8 +363,9 @@ pub(crate) fn guardian_approval_request_to_json(
pub(crate) fn guardian_assessment_action(
action: &GuardianApprovalRequest,
) -> GuardianAssessmentAction {
match action {
) -> Option<GuardianAssessmentAction> {
Some(match action {
GuardianApprovalRequest::NativeToolCall { .. } => return None,
GuardianApprovalRequest::ExecCommand {
command,
guardian_cwd,
@@ -410,13 +426,14 @@ pub(crate) fn guardian_assessment_action(
reason: reason.clone(),
permissions: permissions.clone(),
},
}
})
}
pub(crate) fn guardian_reviewed_action(
request: &GuardianApprovalRequest,
) -> GuardianReviewedAction {
match request {
GuardianApprovalRequest::NativeToolCall { .. } => GuardianReviewedAction::NativeToolCall {},
GuardianApprovalRequest::ExecCommand {
sandbox_permissions,
additional_permissions,
@@ -463,7 +480,8 @@ pub(crate) fn guardian_request_target_item_id(request: &GuardianApprovalRequest)
| GuardianApprovalRequest::WriteStdin { id, .. }
| GuardianApprovalRequest::ApplyPatch { id, .. }
| GuardianApprovalRequest::McpToolCall { id, .. }
| GuardianApprovalRequest::RequestPermissions { id, .. } => Some(id),
| GuardianApprovalRequest::RequestPermissions { id, .. }
| GuardianApprovalRequest::NativeToolCall { id, .. } => Some(id),
GuardianApprovalRequest::NetworkAccess { .. } => None,
}
}
@@ -478,7 +496,8 @@ pub(crate) fn guardian_request_turn_id<'a>(
GuardianApprovalRequest::ExecCommand { .. }
| GuardianApprovalRequest::WriteStdin { .. }
| GuardianApprovalRequest::ApplyPatch { .. }
| GuardianApprovalRequest::McpToolCall { .. } => default_turn_id,
| GuardianApprovalRequest::McpToolCall { .. }
| GuardianApprovalRequest::NativeToolCall { .. } => default_turn_id,
}
}
codex-rs/core/src/guardian/coverage.rs
+1
View File
@@ -12,6 +12,7 @@ impl GuardianApprovalRequest {
Self::McpToolCall { server, .. } => GuardianScope::for_mcp_server(server),
Self::NetworkAccess { .. } => GuardianScope::Network,
Self::RequestPermissions { .. } => GuardianScope::Permissions,
Self::NativeToolCall { .. } => GuardianScope::NativeTool,
}
}
}
codex-rs/core/src/guardian/decision.rs
+1
View File
@@ -71,6 +71,7 @@ pub(crate) fn decide_approval(
| GuardianApprovalRequest::WriteStdin { .. }
| GuardianApprovalRequest::ApplyPatch { .. }
| GuardianApprovalRequest::McpToolCall { .. }
| GuardianApprovalRequest::NativeToolCall { .. }
| GuardianApprovalRequest::RequestPermissions { .. },
)
| Err(_) => request.tool_call_id.as_deref(),
codex-rs/core/src/guardian/input_budget.rs
+4 -4
View File
@@ -91,7 +91,7 @@ pub(crate) async fn check_pending(session: &Session, turn: &TurnContext) -> Code
.for_prompt_annotated(&turn.model_info().input_modalities);
context.retain_new_instructions(&history);
context
.retain_new_agent_messages(&history)
.retain_new_native_messages(&history)
.map_err(|error| CodexErr::InvalidRequest(error.to_string()))?;
if context.estimated_tokens().saturating_add(minimum_prefix) > maximum {
context
@@ -156,7 +156,7 @@ pub(crate) async fn finalize(
// compaction retry; evidence removed by compaction must be delivered again.
context.retain_new_instructions(&history);
context
.retain_new_agent_messages(&history)
.retain_new_native_messages(&history)
.map_err(|error| CodexErr::InvalidRequest(error.to_string()))?;
let prompt = build_prompt(
history
@@ -285,14 +285,14 @@ pub(crate) async fn finalize(
}
}
Err(codex_guardian_context::SectionError::UnsupportedDelivery {
section: "conversation_transcript",
section: "conversation_transcript" | "planned_action",
}) => {
*input = context
.into_annotated_messages()
.into_iter()
.map(|mut envelope| {
// Preserve the ordinary user-content annotations on each text segment;
// native messages retain the original author, recipient and ciphertext.
// native evidence retains its original identity and ciphertext.
if let ResponseItem::Message { role, content, .. } = &envelope.item
&& role == "user"
{
codex-rs/core/src/guardian/input_budget_tests.rs
+1 -2
View File
@@ -144,8 +144,7 @@ async fn finalization_exports_loss_totals_only_for_accepted_input(window: i64) {
fn required_context(text: String) -> ComposedContext {
let action = PlannedAction {
tool_descriptions: None,
json: text,
kind: PlannedActionKind::Command,
kind: PlannedActionKind::Command { json: text },
reason: None,
};
let context = super::super::prompt::collect_guardian_context(
codex-rs/core/src/guardian/prompt.rs
+10 -4
View File
@@ -118,9 +118,7 @@ pub(crate) async fn build_guardian_prompt_items_with_parent_turn(
.retained_context()
.map(codex_guardian_context::collect_verified_answer_candidates)
.unwrap_or_default();
let planned_action_json = format_guardian_action_pretty(&request)?;
let planned_action = PlannedAction {
json: planned_action_json,
tool_descriptions: if let GuardianApprovalRequest::McpToolCall {
tool_description,
connector_description,
@@ -135,14 +133,22 @@ pub(crate) async fn build_guardian_prompt_items_with_parent_turn(
None
},
kind: match &request {
GuardianApprovalRequest::NativeToolCall { call, .. } => {
PlannedActionKind::ToolCall(call.clone())
}
GuardianApprovalRequest::NetworkAccess { trigger, .. } => PlannedActionKind::Network {
json: format_guardian_action_pretty(&request)?,
has_trigger: trigger.is_some(),
},
GuardianApprovalRequest::WriteStdin { .. } => PlannedActionKind::TerminalInput,
GuardianApprovalRequest::WriteStdin { .. } => PlannedActionKind::TerminalInput {
json: format_guardian_action_pretty(&request)?,
},
GuardianApprovalRequest::ExecCommand { .. }
| GuardianApprovalRequest::ApplyPatch { .. }
| GuardianApprovalRequest::McpToolCall { .. }
| GuardianApprovalRequest::RequestPermissions { .. } => PlannedActionKind::Command,
| GuardianApprovalRequest::RequestPermissions { .. } => PlannedActionKind::Command {
json: format_guardian_action_pretty(&request)?,
},
},
reason: reasons.retry.or(reasons.approval),
};
codex-rs/core/src/guardian/review_session.rs
+2 -2
View File
@@ -650,10 +650,10 @@ async fn run_review_on_session(
let items = match prompt_items.context.clone().into_user_inputs() {
Ok(items) => items,
Err(codex_guardian_context::SectionError::UnsupportedDelivery {
section: "conversation_transcript",
section: "conversation_transcript" | "planned_action",
}) => {
// Final admission replaces this turn-start marker with the complete,
// budgeted context, including native encrypted agent-message evidence.
// budgeted context, including native encrypted messages and tool calls.
vec![codex_protocol::user_input::UserInput::Text {
text: super::prompt::GUARDIAN_TRANSCRIPT_START.to_owned(),
text_elements: Vec::new(),
codex-rs/core/src/guardian/runtime.rs
+2 -1
View File
@@ -32,7 +32,8 @@ impl From<GuardianApprovalRequest> for ReviewAction {
| GuardianApprovalRequest::ApplyPatch { .. }
| GuardianApprovalRequest::NetworkAccess { .. }
| GuardianApprovalRequest::McpToolCall { .. }
| GuardianApprovalRequest::RequestPermissions { .. } => {
| GuardianApprovalRequest::RequestPermissions { .. }
| GuardianApprovalRequest::NativeToolCall { .. } => {
super::approval_request::guardian_request_target_item_id(&request)
.map(str::to_owned)
}
codex-rs/core/src/guardian/tests.rs
+62 -24
View File
@@ -1543,7 +1543,7 @@ fn guardian_write_stdin_preserves_input_and_foreign_cwd(
}
assert_eq!(guardian_approval_request_to_json(&action)?, expected);
assert_eq!(
guardian_assessment_action(&action),
guardian_assessment_action(&action).expect("reported approval action"),
GuardianAssessmentAction::WriteStdin {
approval_id: "terminal-write".to_string(),
process_id: "1000".to_string(),
@@ -1599,7 +1599,7 @@ fn guardian_exec_command_uses_executor_cwd_convention(
}),
);
assert_eq!(
guardian_assessment_action(&action),
guardian_assessment_action(&action).expect("reported approval action"),
GuardianAssessmentAction::Command {
source: codex_protocol::approvals::GuardianCommandSource::UnifiedExec,
command: "git status".to_string(),
@@ -3314,30 +3314,68 @@ async fn guardian_review_retries_two_parse_failures_then_approves() -> anyhow::R
],
)
.await;
let (session, turn) = guardian_test_session_and_turn(&server).await;
let (session, mut turn) = guardian_test_session_and_turn(&server).await;
let config = Arc::make_mut(&mut Arc::get_mut(&mut turn).unwrap().config);
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
seed_guardian_parent_history(&session, &turn).await;
let (outcome, metadata) = run_guardian_review_session_for_test(
Arc::clone(&session),
Arc::clone(&turn),
guardian_exec_command_request("shell-parse-retry"),
ApprovalRequestReasons::default(),
/*external_cancel*/ None,
/*max_attempts*/ 3,
)
.await;
let GuardianReviewOutcome::Completed(assessment) = outcome else {
panic!("expected guardian assessment");
let original = serde_json::json!({
"type": "function_call", "id": "fc_action", "call_id": "action",
"namespace": "team", "name": "send_message",
"arguments": r#"{"target":"worker","message":"opaque-message"}"#,
"encrypted_content": "opaque-replay", "encrypted_function_args": ["message"],
});
session
.record_conversation_items(
&turn,
turn.model_info(),
&[serde_json::from_value(original.clone())?],
)
.await;
let mut expected = original;
expected
.as_object_mut()
.unwrap()
.remove("encrypted_content");
expected["arguments"] = serde_json::json!(r#"{"target":"/root","message":"opaque-message"}"#);
let invocation = crate::tools::context::ToolInvocation {
session: Arc::clone(&session),
step_context: crate::session::step_context::StepContext::for_test(Arc::clone(&turn)),
turn,
cancellation_token: CancellationToken::new(),
tracker: Arc::new(tokio::sync::Mutex::new(
crate::turn_diff_tracker::TurnDiffTracker::new(),
)),
call_id: "action".to_owned(),
tool_name: codex_tools::ToolName::namespaced("team", "send_message"),
source: crate::tools::context::ToolCallSource::Direct,
environments: Default::default(),
payload: crate::tools::context::ToolPayload::Function {
arguments: expected["arguments"].as_str().unwrap().to_owned(),
},
};
assert_eq!(assessment.outcome, GuardianAssessmentOutcome::Allow);
assert_eq!(assessment.rationale, "retry succeeded");
assert_eq!(metadata.attempt_count, 3);
assert!(matches!(
metadata.guardian_session_kind,
Some(codex_analytics::GuardianReviewSessionKind::TrunkReused)
));
assert_eq!(request_log.requests().len(), 3);
crate::tools::review_native_tool_call(&invocation).await?;
let requests = request_log.requests();
assert_eq!(requests.len(), 3);
let reviewer = requests[0].body_json()["client_metadata"]["thread_id"].clone();
for request in &requests {
assert_eq!(
request.body_json()["client_metadata"]["thread_id"],
reviewer
);
let calls = request
.inputs_of_type("function_call")
.into_iter()
.map(|mut item| {
item.as_object_mut()
.unwrap()
.remove("internal_chat_message_metadata_passthrough");
item
})
.collect::<Vec<_>>();
assert_eq!(calls, vec![expected.clone()]);
assert_eq!(request.inputs_of_type("function_call_output").len(), 1);
}
Ok(())
}
codex-rs/core/src/tools/mod.rs
+2
View File
@@ -11,6 +11,8 @@ pub(crate) mod hosted_spec;
pub(crate) mod lifecycle;
pub(crate) mod metadata_metrics;
mod multi_agent_tool;
mod native_tool_review;
pub(crate) use native_tool_review::review as review_native_tool_call;
pub(crate) mod network_approval;
pub(crate) mod orchestrator;
pub(crate) mod parallel;
codex-rs/core/src/tools/native_tool_review.rs
+113
View File
@@ -0,0 +1,113 @@
//! Reviews opted-in function calls synchronously before dispatch, preserving their native wire data.
use crate::function_tool::FunctionCallError;
use crate::guardian::GuardianApprovalRequest;
use crate::guardian::GuardianReviewContext;
use crate::guardian::GuardianReviewOptions;
use crate::guardian::new_guardian_review_id;
use crate::guardian::spawn_approval_decision;
use crate::tools::context::ToolCallSource;
use crate::tools::context::ToolInvocation;
use crate::tools::context::ToolPayload;
use crate::tools::sandboxing::ApprovalRequestReasons;
use codex_analytics::GuardianApprovalRequestSource;
use codex_protocol::models::ResponseItem;
use codex_protocol::protocol::ReviewDecision;
pub(crate) async fn review(invocation: &ToolInvocation) -> Result<(), FunctionCallError> {
let ToolPayload::Function { arguments } = &invocation.payload else {
return Err(FunctionCallError::RespondToModel(
"Native review requires a function call".to_owned(),
));
};
let mut item = invocation
.session
.clone_history()
.await
.raw_items()
.rev()
.find(|item| {
matches!(
item,
ResponseItem::FunctionCall { call_id, .. } if call_id == &invocation.call_id
)
})
.cloned()
.unwrap_or_else(|| ResponseItem::FunctionCall {
id: None,
name: invocation.tool_name.name.clone(),
namespace: invocation.tool_name.namespace.clone(),
arguments: arguments.clone(),
call_id: invocation.call_id.clone(),
encrypted_content: None,
status: None,
encrypted_function_args: matches!(
invocation.source,
ToolCallSource::DirectPlaintextMessage | ToolCallSource::CodeMode { .. }
)
.then(Vec::new),
internal_chat_message_metadata_passthrough: None,
});
if let ResponseItem::FunctionCall {
arguments: reviewed_arguments,
encrypted_content,
..
} = &mut item
&& reviewed_arguments != arguments
{
// A hook rewrite invalidates replay of the original call, but not encrypted argument fields.
reviewed_arguments.clone_from(arguments);
*encrypted_content = None;
}
let context = GuardianReviewContext::from(&invocation.step_context);
// Native tool review has no user-approval fallback. An unsuccessful required review must stop dispatch.
let require_guardian = codex_guardian_reviewer::routes_approval_policy_to_guardian(
context.approval_policy,
context.approvals_reviewer,
);
let decision = spawn_approval_decision(
invocation.session.clone(),
context,
new_guardian_review_id(),
GuardianApprovalRequest::NativeToolCall {
id: invocation.call_id.clone(),
call: Box::new(item),
},
ApprovalRequestReasons::default(),
GuardianReviewOptions {
require_guardian,
require_synchronous_review: true,
plugin_attribution_override: None,
approval_request_source: GuardianApprovalRequestSource::MainTurn,
external_cancel: Some(invocation.cancellation_token.clone()),
},
)
.await
.map_err(|_| {
FunctionCallError::RespondToModel("automatic approval review did not complete".to_owned())
})?;
match decision {
None | Some(ReviewDecision::Approved | ReviewDecision::ApprovedForSession) => Ok(()),
Some(ReviewDecision::Denied { rejection }) => {
Err(FunctionCallError::RespondToModel(rejection))
}
Some(ReviewDecision::TimedOut) => Err(FunctionCallError::RespondToModel(
codex_prompts::ResolvedModelMessages::from_model(
&invocation.step_context.settings.model_info,
)
.auto_review()
.timeout_instructions
.to_owned(),
)),
Some(ReviewDecision::Abort) => Err(FunctionCallError::RespondToModel(
"automatic approval review was cancelled".to_owned(),
)),
Some(
ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::ApprovedMcpPolicyAmendment
| ReviewDecision::NetworkPolicyAmendment { .. },
) => Err(FunctionCallError::RespondToModel(
"automatic approval review returned an unsupported decision".to_owned(),
)),
}
}
codex-rs/core/src/tools/registry.rs
+11
View File
@@ -62,6 +62,11 @@ pub use codex_tools::ToolExposure;
/// Implementers provide the shared `ToolExecutor` behavior plus optional
/// core-owned metadata for hooks, telemetry, tool search, and argument diffs.
pub(crate) trait CoreToolRuntime: ToolExecutor<ToolInvocation> {
/// Opts this invocation into synchronous native-call review before the handler runs.
fn requires_guardian_review(&self, _invocation: &ToolInvocation) -> bool {
false
}
/// The handler observes its cancellation token and must finish before dispatch is aborted.
fn finishes_on_cancellation(&self) -> bool {
false
@@ -869,6 +874,12 @@ impl ToolRegistry {
}
}
if tool.requires_guardian_review(&invocation)
&& let Err(err) = super::review_native_tool_call(&invocation).await
{
return Err(fail_before_execution(&invocation, err).await);
}
if tool.mcp_server_name().is_none() {
notify_tool_start(&invocation, /*mcp_tool*/ None)
.or_cancel_tool(&cancellation_token)
codex-rs/core/src/tools/registry_tests.rs
+44 -3
View File
@@ -120,7 +120,25 @@ impl LifecycleTestHandler {
}
}
impl CoreToolRuntime for LifecycleTestHandler {}
impl CoreToolRuntime for LifecycleTestHandler {
fn requires_guardian_review(&self, _invocation: &ToolInvocation) -> bool {
self.tool_name.name == "reviewed_tool"
}
}
impl codex_extension_api::ApprovalReviewContributor for LifecycleTestHandler {
fn decide<'a>(
&'a self,
_input: &'a codex_extension_api::ApprovalDecisionInput<'_>,
) -> codex_extension_api::ExtensionFuture<'a, Option<codex_extension_api::ApprovalDecision>>
{
Box::pin(async {
Some(codex_extension_api::ApprovalDecision::Reviewed(
codex_protocol::protocol::ReviewDecision::denied("review denied"),
))
})
}
}
fn test_spec(tool_name: &codex_tools::ToolName) -> codex_tools::ToolSpec {
codex_tools::ToolSpec::Function(codex_tools::ResponsesApiTool {
@@ -742,7 +760,6 @@ async fn dispatch_uses_canonical_tool_names_for_lifecycle_contributors() -> anyh
builder.tool_lifecycle_contributor(Arc::new(ToolLifecycleRecorder {
records: Arc::clone(&records),
}));
session.services.extensions = Arc::new(builder.build());
let ok_tool = codex_tools::ToolName::plain("ok_tool");
let failing_tool = codex_tools::ToolName::namespaced("extensions", "failing_tool");
@@ -754,7 +771,14 @@ async fn dispatch_uses_canonical_tool_names_for_lifecycle_contributors() -> anyh
tool_name: failing_tool.clone(),
result: LifecycleTestResult::Err,
}) as Arc<dyn CoreToolRuntime>;
let registry = ToolRegistry::from_tools([ok_handler, failing_handler]);
let reviewed_tool = codex_tools::ToolName::namespaced("extensions", "reviewed_tool");
let reviewed_handler = Arc::new(LifecycleTestHandler {
tool_name: reviewed_tool.clone(),
result: LifecycleTestResult::Err,
});
builder.approval_review_contributor(reviewed_handler.clone());
session.services.extensions = Arc::new(builder.build());
let registry = ToolRegistry::from_tools([ok_handler, failing_handler, reviewed_handler]);
let session = Arc::new(session);
let turn = Arc::new(turn);
@@ -785,6 +809,16 @@ async fn dispatch_uses_canonical_tool_names_for_lifecycle_contributors() -> anyh
Err(err) => err,
};
assert_eq!(err.to_string(), "handler failed");
let denied = registry
.dispatch_any_with_state(
test_invocation(session, turn, "reviewed-call", reviewed_tool.clone()),
/*call_state*/ None,
)
.await;
assert_eq!(
denied.err().expect("review must deny").to_string(),
"review denied"
);
let expected = vec![
RecordedToolLifecycle::Start {
@@ -809,6 +843,13 @@ async fn dispatch_uses_canonical_tool_names_for_lifecycle_contributors() -> anyh
handler_executed: true,
},
},
RecordedToolLifecycle::Finish {
call_id: "reviewed-call".to_string(),
tool_name: reviewed_tool,
outcome: codex_extension_api::ToolCallOutcome::Failed {
handler_executed: false,
},
},
];
let actual = records
.lock()