mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat: add guardian network approval trigger context (#18197)
## Summary
Give guardian network-access reviews the command context that triggered
a managed-network approval. The prompt JSON now includes the originating
tool call id, tool name, command argv, cwd, sandbox permissions,
additional permissions, justification, and tty state when a single
active tool call can be attributed.
The implementation keeps the trigger shape canonical by serializing
`GuardianNetworkAccessTrigger` directly and lets each runtime build that
trigger from its `ToolCtx`. Non-guardian approval prompts avoid cloning
the full trigger payload.
## UX changes
Guardian network-access reviews now include a `trigger` object that
explains what command caused the network approval. Instead of seeing
only the requested host, the guardian reviewer can also see the
originating tool call, argv, working directory, sandbox mode,
justification, and tty state.
Example payload the guardian reviewer can see:
```json
{
"tool": "network_access",
"target": "https://api.github.com:443",
"host": "api.github.com",
"protocol": "https",
"port": 443,
"trigger": {
"callId": "call_abc123",
"toolName": "shell",
"command": ["gh", "api", "/repos/openai/codex/pulls/18197"],
"cwd": "/workspace/codex",
"sandboxPermissions": "require_escalated",
"justification": "Fetch PR metadata from GitHub.",
"tty": false
}
}
```
The network review itself remains scoped to the network decision:
`target_item_id` stays `null`. `trigger.callId` is attribution context
only, so clients can still distinguish network reviews from
item-targeted command reviews.
## Verification
- Added coverage for serializing network trigger context in guardian
approval JSON.
- Added regression coverage that network guardian reviews do not reuse
`trigger.callId` as `target_item_id`.
---------
Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
9360f267f3
commit
2d73bac45f
@@ -54,6 +54,7 @@ pub(crate) enum GuardianApprovalRequest {
|
||||
host: String,
|
||||
protocol: NetworkApprovalProtocol,
|
||||
port: u16,
|
||||
trigger: Option<GuardianNetworkAccessTrigger>,
|
||||
},
|
||||
McpToolCall {
|
||||
id: String,
|
||||
@@ -75,6 +76,22 @@ pub(crate) enum GuardianApprovalRequest {
|
||||
},
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Serialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub(crate) struct GuardianNetworkAccessTrigger {
|
||||
pub(crate) call_id: String,
|
||||
pub(crate) tool_name: String,
|
||||
pub(crate) command: Vec<String>,
|
||||
pub(crate) cwd: AbsolutePathBuf,
|
||||
pub(crate) sandbox_permissions: crate::sandboxing::SandboxPermissions,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub(crate) additional_permissions: Option<PermissionProfile>,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub(crate) justification: Option<String>,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub(crate) tty: Option<bool>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
|
||||
pub(crate) struct GuardianMcpAnnotations {
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
@@ -131,6 +148,18 @@ struct McpToolCallApprovalAction<'a> {
|
||||
annotations: Option<&'a GuardianMcpAnnotations>,
|
||||
}
|
||||
|
||||
#[derive(Serialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
struct NetworkAccessApprovalAction<'a> {
|
||||
tool: &'static str,
|
||||
target: &'a str,
|
||||
host: &'a str,
|
||||
protocol: NetworkApprovalProtocol,
|
||||
port: u16,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
trigger: Option<&'a GuardianNetworkAccessTrigger>,
|
||||
}
|
||||
|
||||
#[derive(Serialize)]
|
||||
struct RequestPermissionsApprovalAction<'a> {
|
||||
tool: &'static str,
|
||||
@@ -297,13 +326,15 @@ pub(crate) fn guardian_approval_request_to_json(
|
||||
host,
|
||||
protocol,
|
||||
port,
|
||||
} => Ok(serde_json::json!({
|
||||
"tool": "network_access",
|
||||
"target": target,
|
||||
"host": host,
|
||||
"protocol": protocol,
|
||||
"port": port,
|
||||
})),
|
||||
trigger,
|
||||
} => serialize_guardian_action(NetworkAccessApprovalAction {
|
||||
tool: "network_access",
|
||||
target,
|
||||
host,
|
||||
protocol: *protocol,
|
||||
port: *port,
|
||||
trigger: trigger.as_ref(),
|
||||
}),
|
||||
GuardianApprovalRequest::McpToolCall {
|
||||
id: _,
|
||||
server,
|
||||
@@ -371,12 +402,13 @@ pub(crate) fn guardian_assessment_action(
|
||||
}
|
||||
}
|
||||
GuardianApprovalRequest::NetworkAccess {
|
||||
id: _,
|
||||
turn_id: _,
|
||||
id: _id,
|
||||
turn_id: _turn_id,
|
||||
target,
|
||||
host,
|
||||
protocol,
|
||||
port,
|
||||
trigger: _trigger,
|
||||
} => GuardianAssessmentAction::NetworkAccess {
|
||||
target: target.clone(),
|
||||
host: host.clone(),
|
||||
|
||||
@@ -25,6 +25,7 @@ use serde::Serialize;
|
||||
|
||||
pub(crate) use approval_request::GuardianApprovalRequest;
|
||||
pub(crate) use approval_request::GuardianMcpAnnotations;
|
||||
pub(crate) use approval_request::GuardianNetworkAccessTrigger;
|
||||
pub(crate) use approval_request::guardian_approval_request_to_json;
|
||||
pub(crate) use review::guardian_rejection_message;
|
||||
pub(crate) use review::guardian_timeout_message;
|
||||
|
||||
@@ -12,6 +12,7 @@ use crate::config_loader::NetworkDomainPermissionToml;
|
||||
use crate::config_loader::NetworkDomainPermissionsToml;
|
||||
use crate::config_loader::RequirementSource;
|
||||
use crate::config_loader::Sourced;
|
||||
use crate::guardian::approval_request::guardian_request_target_item_id;
|
||||
use crate::session::session::Session;
|
||||
use crate::session::turn_context::TurnContext;
|
||||
use crate::test_support;
|
||||
@@ -727,6 +728,50 @@ fn guardian_approval_request_to_json_renders_mcp_tool_call_shape() -> serde_json
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_approval_request_to_json_renders_network_access_trigger() -> serde_json::Result<()> {
|
||||
let cwd = test_path_buf("/repo").abs();
|
||||
let action = GuardianApprovalRequest::NetworkAccess {
|
||||
id: "network-1".to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
target: "https://example.com:443".to_string(),
|
||||
host: "example.com".to_string(),
|
||||
protocol: NetworkApprovalProtocol::Https,
|
||||
port: 443,
|
||||
trigger: Some(GuardianNetworkAccessTrigger {
|
||||
call_id: "call-1".to_string(),
|
||||
tool_name: "shell".to_string(),
|
||||
command: vec!["curl".to_string(), "https://example.com".to_string()],
|
||||
cwd: cwd.clone(),
|
||||
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: Some("Fetch the release metadata.".to_string()),
|
||||
tty: None,
|
||||
}),
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
guardian_approval_request_to_json(&action)?,
|
||||
serde_json::json!({
|
||||
"tool": "network_access",
|
||||
"target": "https://example.com:443",
|
||||
"host": "example.com",
|
||||
"protocol": "https",
|
||||
"port": 443,
|
||||
"trigger": {
|
||||
"callId": "call-1",
|
||||
"toolName": "shell",
|
||||
"command": ["curl", "https://example.com"],
|
||||
"cwd": cwd.to_string_lossy().to_string(),
|
||||
"sandboxPermissions": "use_default",
|
||||
"justification": "Fetch the release metadata.",
|
||||
},
|
||||
})
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_assessment_action_redacts_apply_patch_patch_text() {
|
||||
let cwd = test_path_buf("/tmp").abs();
|
||||
@@ -758,6 +803,7 @@ fn guardian_request_turn_id_prefers_network_access_owner_turn() {
|
||||
host: "example.com".to_string(),
|
||||
protocol: NetworkApprovalProtocol::Https,
|
||||
port: 443,
|
||||
trigger: None,
|
||||
};
|
||||
let apply_patch = GuardianApprovalRequest::ApplyPatch {
|
||||
id: "patch-1".to_string(),
|
||||
@@ -777,6 +823,30 @@ fn guardian_request_turn_id_prefers_network_access_owner_turn() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_request_target_item_id_omits_network_access_trigger_call_id() {
|
||||
let network_access = GuardianApprovalRequest::NetworkAccess {
|
||||
id: "network-1".to_string(),
|
||||
turn_id: "owner-turn".to_string(),
|
||||
target: "https://example.com:443".to_string(),
|
||||
host: "example.com".to_string(),
|
||||
protocol: NetworkApprovalProtocol::Https,
|
||||
port: 443,
|
||||
trigger: Some(GuardianNetworkAccessTrigger {
|
||||
call_id: "call-1".to_string(),
|
||||
tool_name: "shell".to_string(),
|
||||
command: vec!["curl".to_string(), "https://example.com".to_string()],
|
||||
cwd: test_path_buf("/repo").abs(),
|
||||
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: None,
|
||||
tty: None,
|
||||
}),
|
||||
};
|
||||
|
||||
assert_eq!(guardian_request_target_item_id(&network_access), None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
|
||||
let (session, turn, rx) = crate::session::tests::make_session_and_context_with_rx().await;
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::GuardianNetworkAccessTrigger;
|
||||
use crate::guardian::guardian_rejection_message;
|
||||
use crate::guardian::guardian_timeout_message;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
@@ -47,6 +48,7 @@ pub(crate) enum NetworkApprovalMode {
|
||||
pub(crate) struct NetworkApprovalSpec {
|
||||
pub network: Option<NetworkProxy>,
|
||||
pub mode: NetworkApprovalMode,
|
||||
pub trigger: GuardianNetworkAccessTrigger,
|
||||
pub command: String,
|
||||
}
|
||||
|
||||
@@ -177,6 +179,7 @@ impl PendingHostApproval {
|
||||
struct ActiveNetworkApprovalCall {
|
||||
registration_id: String,
|
||||
turn_id: String,
|
||||
trigger: GuardianNetworkAccessTrigger,
|
||||
command: String,
|
||||
}
|
||||
|
||||
@@ -210,7 +213,13 @@ impl NetworkApprovalService {
|
||||
other_approved_hosts.extend(approved_hosts.iter().cloned());
|
||||
}
|
||||
|
||||
async fn register_call(&self, registration_id: String, turn_id: String, command: String) {
|
||||
async fn register_call(
|
||||
&self,
|
||||
registration_id: String,
|
||||
turn_id: String,
|
||||
trigger: GuardianNetworkAccessTrigger,
|
||||
command: String,
|
||||
) {
|
||||
let mut active_calls = self.active_calls.lock().await;
|
||||
let key = registration_id.clone();
|
||||
active_calls.insert(
|
||||
@@ -218,6 +227,7 @@ impl NetworkApprovalService {
|
||||
Arc::new(ActiveNetworkApprovalCall {
|
||||
registration_id,
|
||||
turn_id,
|
||||
trigger,
|
||||
command,
|
||||
}),
|
||||
);
|
||||
@@ -369,11 +379,11 @@ impl NetworkApprovalService {
|
||||
return NetworkDecision::deny(REASON_NOT_ALLOWED);
|
||||
}
|
||||
|
||||
let owner_call = self.resolve_single_active_call().await;
|
||||
let network_approval_context = NetworkApprovalContext {
|
||||
host: request.host.clone(),
|
||||
protocol,
|
||||
};
|
||||
let owner_call = self.resolve_single_active_call().await;
|
||||
let guardian_approval_id = Self::approval_id_for_key(&key);
|
||||
let prompt_command = vec!["network-access".to_string(), target.clone()];
|
||||
let command = owner_call
|
||||
@@ -431,6 +441,7 @@ impl NetworkApprovalService {
|
||||
host: request.host,
|
||||
protocol,
|
||||
port: key.port,
|
||||
trigger: owner_call.as_ref().map(|call| call.trigger.clone()),
|
||||
},
|
||||
Some(policy_denial_message.clone()),
|
||||
)
|
||||
@@ -623,8 +634,13 @@ pub(crate) async fn begin_network_approval(
|
||||
managed_network_active: bool,
|
||||
spec: Option<NetworkApprovalSpec>,
|
||||
) -> Option<ActiveNetworkApproval> {
|
||||
let spec = spec?;
|
||||
if !managed_network_active || spec.network.is_none() {
|
||||
let NetworkApprovalSpec {
|
||||
network,
|
||||
mode,
|
||||
trigger,
|
||||
command,
|
||||
} = spec?;
|
||||
if !managed_network_active || network.is_none() {
|
||||
return None;
|
||||
}
|
||||
|
||||
@@ -632,12 +648,17 @@ pub(crate) async fn begin_network_approval(
|
||||
session
|
||||
.services
|
||||
.network_approval
|
||||
.register_call(registration_id.clone(), turn_id.to_string(), spec.command)
|
||||
.register_call(
|
||||
registration_id.clone(),
|
||||
turn_id.to_string(),
|
||||
trigger,
|
||||
command,
|
||||
)
|
||||
.await;
|
||||
|
||||
Some(ActiveNetworkApproval {
|
||||
registration_id: Some(registration_id),
|
||||
mode: spec.mode,
|
||||
mode,
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -1,7 +1,10 @@
|
||||
use super::*;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use codex_network_proxy::BlockedRequestArgs;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::test_path_buf;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[tokio::test]
|
||||
@@ -207,17 +210,66 @@ fn denied_blocked_request(host: &str) -> BlockedRequest {
|
||||
})
|
||||
}
|
||||
|
||||
async fn register_call_with_default_shell_trigger(
|
||||
service: &NetworkApprovalService,
|
||||
registration_id: &str,
|
||||
) {
|
||||
service
|
||||
.register_call(
|
||||
registration_id.to_string(),
|
||||
"turn-1".to_string(),
|
||||
GuardianNetworkAccessTrigger {
|
||||
call_id: "call-1".to_string(),
|
||||
tool_name: "shell".to_string(),
|
||||
command: vec!["curl".to_string(), "https://example.com".to_string()],
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: None,
|
||||
tty: None,
|
||||
},
|
||||
"curl https://example.com".to_string(),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn record_blocked_request_sets_policy_outcome_for_owner_call() {
|
||||
async fn active_call_preserves_triggering_command_context() {
|
||||
let service = NetworkApprovalService::default();
|
||||
let expected = GuardianNetworkAccessTrigger {
|
||||
call_id: "call-1".to_string(),
|
||||
tool_name: "shell".to_string(),
|
||||
command: vec!["curl".to_string(), "https://example.com".to_string()],
|
||||
cwd: test_path_buf("/repo").abs(),
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: Some("fetch release metadata".to_string()),
|
||||
tty: None,
|
||||
};
|
||||
|
||||
service
|
||||
.register_call(
|
||||
"registration-1".to_string(),
|
||||
"turn-1".to_string(),
|
||||
"curl http://example.com".to_string(),
|
||||
expected.clone(),
|
||||
"curl https://example.com".to_string(),
|
||||
)
|
||||
.await;
|
||||
|
||||
let call = service
|
||||
.resolve_single_active_call()
|
||||
.await
|
||||
.expect("single active call should resolve");
|
||||
|
||||
assert_eq!(&call.trigger, &expected);
|
||||
assert_eq!(call.command, "curl https://example.com");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn record_blocked_request_sets_policy_outcome_for_owner_call() {
|
||||
let service = NetworkApprovalService::default();
|
||||
register_call_with_default_shell_trigger(&service, "registration-1").await;
|
||||
|
||||
service
|
||||
.record_blocked_request(denied_blocked_request("example.com"))
|
||||
.await;
|
||||
@@ -233,13 +285,7 @@ async fn record_blocked_request_sets_policy_outcome_for_owner_call() {
|
||||
#[tokio::test]
|
||||
async fn blocked_request_policy_does_not_override_user_denial_outcome() {
|
||||
let service = NetworkApprovalService::default();
|
||||
service
|
||||
.register_call(
|
||||
"registration-1".to_string(),
|
||||
"turn-1".to_string(),
|
||||
"curl http://example.com".to_string(),
|
||||
)
|
||||
.await;
|
||||
register_call_with_default_shell_trigger(&service, "registration-1").await;
|
||||
|
||||
service
|
||||
.record_call_outcome("registration-1", NetworkApprovalOutcome::DeniedByUser)
|
||||
@@ -257,20 +303,8 @@ async fn blocked_request_policy_does_not_override_user_denial_outcome() {
|
||||
#[tokio::test]
|
||||
async fn record_blocked_request_ignores_ambiguous_unattributed_blocked_requests() {
|
||||
let service = NetworkApprovalService::default();
|
||||
service
|
||||
.register_call(
|
||||
"registration-1".to_string(),
|
||||
"turn-1".to_string(),
|
||||
"curl http://example.com".to_string(),
|
||||
)
|
||||
.await;
|
||||
service
|
||||
.register_call(
|
||||
"registration-2".to_string(),
|
||||
"turn-1".to_string(),
|
||||
"gh api /foo".to_string(),
|
||||
)
|
||||
.await;
|
||||
register_call_with_default_shell_trigger(&service, "registration-1").await;
|
||||
register_call_with_default_shell_trigger(&service, "registration-2").await;
|
||||
|
||||
service
|
||||
.record_blocked_request(denied_blocked_request("example.com"))
|
||||
|
||||
@@ -11,6 +11,7 @@ pub(crate) mod zsh_fork_backend;
|
||||
use crate::command_canonicalization::canonicalize_command_for_approval;
|
||||
use crate::exec::ExecCapturePolicy;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::GuardianNetworkAccessTrigger;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::sandboxing::ExecOptions;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
@@ -217,12 +218,22 @@ impl ToolRuntime<ShellRequest, ExecToolCallOutput> for ShellRuntime {
|
||||
fn network_approval_spec(
|
||||
&self,
|
||||
req: &ShellRequest,
|
||||
_ctx: &ToolCtx,
|
||||
ctx: &ToolCtx,
|
||||
) -> Option<NetworkApprovalSpec> {
|
||||
req.network.as_ref()?;
|
||||
Some(NetworkApprovalSpec {
|
||||
network: req.network.clone(),
|
||||
mode: NetworkApprovalMode::Immediate,
|
||||
trigger: GuardianNetworkAccessTrigger {
|
||||
call_id: ctx.call_id.clone(),
|
||||
tool_name: ctx.tool_name.clone(),
|
||||
command: req.command.clone(),
|
||||
cwd: req.cwd.clone(),
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
additional_permissions: req.additional_permissions.clone(),
|
||||
justification: req.justification.clone(),
|
||||
tty: None,
|
||||
},
|
||||
command: req.hook_command.clone(),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -8,6 +8,7 @@ use crate::command_canonicalization::canonicalize_command_for_approval;
|
||||
use crate::exec::ExecCapturePolicy;
|
||||
use crate::exec::ExecExpiration;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::GuardianNetworkAccessTrigger;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::sandboxing::ExecOptions;
|
||||
use crate::sandboxing::ExecServerEnvConfig;
|
||||
@@ -202,12 +203,22 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
fn network_approval_spec(
|
||||
&self,
|
||||
req: &UnifiedExecRequest,
|
||||
_ctx: &ToolCtx,
|
||||
ctx: &ToolCtx,
|
||||
) -> Option<NetworkApprovalSpec> {
|
||||
req.network.as_ref()?;
|
||||
Some(NetworkApprovalSpec {
|
||||
network: req.network.clone(),
|
||||
mode: NetworkApprovalMode::Deferred,
|
||||
trigger: GuardianNetworkAccessTrigger {
|
||||
call_id: ctx.call_id.clone(),
|
||||
tool_name: ctx.tool_name.clone(),
|
||||
command: req.command.clone(),
|
||||
cwd: req.cwd.clone(),
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
additional_permissions: req.additional_permissions.clone(),
|
||||
justification: req.justification.clone(),
|
||||
tty: Some(req.tty),
|
||||
},
|
||||
command: req.hook_command.clone(),
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user