mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
guardian: attach network approvals to parent tool items
Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
co-authored by
Codex
parent
cf0223887f
commit
9fc1605b9b
@@ -879,7 +879,7 @@ All items emit shared lifecycle events:
|
||||
- `item/autoApprovalReview/started` — [UNSTABLE] temporary guardian notification carrying `{threadId, turnId, targetItemId, review, action?}` when guardian approval review begins. This shape is expected to change soon.
|
||||
- `item/autoApprovalReview/completed` — [UNSTABLE] temporary guardian notification carrying `{threadId, turnId, targetItemId, review, action?}` when guardian approval review resolves. This shape is expected to change soon.
|
||||
|
||||
`review` is [UNSTABLE] and currently has `{status, riskScore?, riskLevel?, rationale?}`, where `status` is one of `inProgress`, `approved`, `denied`, or `aborted`. `action` is the guardian action summary payload from core when available and is intended to support temporary standalone pending-review UI. These notifications are separate from the target item's own `item/completed` lifecycle and are intentionally temporary while the guardian app protocol is still being designed.
|
||||
`review` is [UNSTABLE] and currently has `{status, riskScore?, riskLevel?, rationale?}`, where `status` is one of `inProgress`, `approved`, `denied`, or `aborted`. `action` is the guardian action summary payload from core when available and is intended to support temporary standalone pending-review UI. `targetItemId` points at the reviewed item when app-server can attribute the review to a parent tool item, including attributed `network_access` reviews; otherwise it falls back to the guardian review id. These notifications are separate from the target item's own `item/completed` lifecycle and are intentionally temporary while the guardian app protocol is still being designed.
|
||||
|
||||
There are additional item-specific events:
|
||||
|
||||
|
||||
@@ -224,13 +224,25 @@ fn guardian_auto_approval_review_notification(
|
||||
risk_level: assessment.risk_level.map(Into::into),
|
||||
rationale: assessment.rationale.clone(),
|
||||
};
|
||||
let target_item_id = assessment
|
||||
.action
|
||||
.as_ref()
|
||||
.filter(|action| {
|
||||
matches!(
|
||||
action.get("tool").and_then(serde_json::Value::as_str),
|
||||
Some("network_access")
|
||||
)
|
||||
})
|
||||
.and_then(|action| action.get("parent_tool_item_id"))
|
||||
.and_then(serde_json::Value::as_str)
|
||||
.map_or_else(|| assessment.id.clone(), ToString::to_string);
|
||||
match assessment.status {
|
||||
codex_protocol::protocol::GuardianAssessmentStatus::InProgress => {
|
||||
ServerNotification::ItemGuardianApprovalReviewStarted(
|
||||
ItemGuardianApprovalReviewStartedNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id,
|
||||
target_item_id: assessment.id.clone(),
|
||||
target_item_id,
|
||||
review,
|
||||
action: assessment.action.clone(),
|
||||
},
|
||||
@@ -243,7 +255,7 @@ fn guardian_auto_approval_review_notification(
|
||||
ItemGuardianApprovalReviewCompletedNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id,
|
||||
target_item_id: assessment.id.clone(),
|
||||
target_item_id,
|
||||
review,
|
||||
action: assessment.action.clone(),
|
||||
},
|
||||
@@ -2989,13 +3001,14 @@ mod tests {
|
||||
let conversation_id = ThreadId::new();
|
||||
let action = json!({
|
||||
"tool": "network_access",
|
||||
"parent_tool_item_id": "command-3",
|
||||
"target": "api.openai.com:443",
|
||||
});
|
||||
let notification = guardian_auto_approval_review_notification(
|
||||
&conversation_id,
|
||||
"turn-from-event",
|
||||
&GuardianAssessmentEvent {
|
||||
id: "item-3".to_string(),
|
||||
id: "guardian-3".to_string(),
|
||||
turn_id: "turn-from-assessment".to_string(),
|
||||
status: codex_protocol::protocol::GuardianAssessmentStatus::Aborted,
|
||||
risk_score: None,
|
||||
@@ -3009,7 +3022,43 @@ mod tests {
|
||||
ServerNotification::ItemGuardianApprovalReviewCompleted(payload) => {
|
||||
assert_eq!(payload.thread_id, conversation_id.to_string());
|
||||
assert_eq!(payload.turn_id, "turn-from-assessment");
|
||||
assert_eq!(payload.target_item_id, "item-3");
|
||||
assert_eq!(payload.target_item_id, "command-3");
|
||||
assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Aborted);
|
||||
assert_eq!(payload.review.risk_score, None);
|
||||
assert_eq!(payload.review.risk_level, None);
|
||||
assert_eq!(payload.review.rationale, None);
|
||||
assert_eq!(payload.action, Some(action));
|
||||
}
|
||||
other => panic!("unexpected notification: {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_assessment_aborted_keeps_guardian_id_when_network_parent_is_missing() {
|
||||
let conversation_id = ThreadId::new();
|
||||
let action = json!({
|
||||
"tool": "network_access",
|
||||
"target": "api.openai.com:443",
|
||||
});
|
||||
let notification = guardian_auto_approval_review_notification(
|
||||
&conversation_id,
|
||||
"turn-from-event",
|
||||
&GuardianAssessmentEvent {
|
||||
id: "guardian-4".to_string(),
|
||||
turn_id: "turn-from-assessment".to_string(),
|
||||
status: codex_protocol::protocol::GuardianAssessmentStatus::Aborted,
|
||||
risk_score: None,
|
||||
risk_level: None,
|
||||
rationale: None,
|
||||
action: Some(action.clone()),
|
||||
},
|
||||
);
|
||||
|
||||
match notification {
|
||||
ServerNotification::ItemGuardianApprovalReviewCompleted(payload) => {
|
||||
assert_eq!(payload.thread_id, conversation_id.to_string());
|
||||
assert_eq!(payload.turn_id, "turn-from-assessment");
|
||||
assert_eq!(payload.target_item_id, "guardian-4");
|
||||
assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Aborted);
|
||||
assert_eq!(payload.review.risk_score, None);
|
||||
assert_eq!(payload.review.risk_level, None);
|
||||
|
||||
@@ -47,6 +47,7 @@ pub(crate) enum GuardianApprovalRequest {
|
||||
NetworkAccess {
|
||||
id: String,
|
||||
turn_id: String,
|
||||
parent_tool_item_id: Option<String>,
|
||||
target: String,
|
||||
host: String,
|
||||
protocol: NetworkApprovalProtocol,
|
||||
@@ -248,17 +249,36 @@ pub(crate) fn guardian_approval_request_to_json(
|
||||
GuardianApprovalRequest::NetworkAccess {
|
||||
id: _,
|
||||
turn_id: _,
|
||||
parent_tool_item_id,
|
||||
target,
|
||||
host,
|
||||
protocol,
|
||||
port,
|
||||
} => Ok(serde_json::json!({
|
||||
"tool": "network_access",
|
||||
"target": target,
|
||||
"host": host,
|
||||
"protocol": protocol,
|
||||
"port": port,
|
||||
})),
|
||||
} => {
|
||||
let mut action = serde_json::Map::from_iter([
|
||||
(
|
||||
"tool".to_string(),
|
||||
Value::String("network_access".to_string()),
|
||||
),
|
||||
("target".to_string(), Value::String(target.clone())),
|
||||
("host".to_string(), Value::String(host.clone())),
|
||||
(
|
||||
"protocol".to_string(),
|
||||
serde_json::to_value(protocol).expect("protocol serializes"),
|
||||
),
|
||||
(
|
||||
"port".to_string(),
|
||||
serde_json::to_value(port).expect("port serializes"),
|
||||
),
|
||||
]);
|
||||
if let Some(parent_tool_item_id) = parent_tool_item_id {
|
||||
action.insert(
|
||||
"parent_tool_item_id".to_string(),
|
||||
Value::String(parent_tool_item_id.clone()),
|
||||
);
|
||||
}
|
||||
Ok(Value::Object(action))
|
||||
}
|
||||
GuardianApprovalRequest::McpToolCall {
|
||||
id: _,
|
||||
server,
|
||||
@@ -320,17 +340,36 @@ pub(crate) fn guardian_assessment_action_value(action: &GuardianApprovalRequest)
|
||||
GuardianApprovalRequest::NetworkAccess {
|
||||
id: _,
|
||||
turn_id: _,
|
||||
parent_tool_item_id,
|
||||
target,
|
||||
host,
|
||||
protocol,
|
||||
port,
|
||||
} => serde_json::json!({
|
||||
"tool": "network_access",
|
||||
"target": target,
|
||||
"host": host,
|
||||
"protocol": protocol,
|
||||
"port": port,
|
||||
}),
|
||||
} => {
|
||||
let mut action = serde_json::Map::from_iter([
|
||||
(
|
||||
"tool".to_string(),
|
||||
Value::String("network_access".to_string()),
|
||||
),
|
||||
("target".to_string(), Value::String(target.clone())),
|
||||
("host".to_string(), Value::String(host.clone())),
|
||||
(
|
||||
"protocol".to_string(),
|
||||
serde_json::to_value(protocol).expect("protocol serializes"),
|
||||
),
|
||||
(
|
||||
"port".to_string(),
|
||||
serde_json::to_value(port).expect("port serializes"),
|
||||
),
|
||||
]);
|
||||
if let Some(parent_tool_item_id) = parent_tool_item_id {
|
||||
action.insert(
|
||||
"parent_tool_item_id".to_string(),
|
||||
Value::String(parent_tool_item_id.clone()),
|
||||
);
|
||||
}
|
||||
Value::Object(action)
|
||||
}
|
||||
GuardianApprovalRequest::McpToolCall {
|
||||
server, tool_name, ..
|
||||
} => serde_json::json!({
|
||||
|
||||
@@ -347,6 +347,7 @@ fn guardian_request_turn_id_prefers_network_access_owner_turn() {
|
||||
let network_access = GuardianApprovalRequest::NetworkAccess {
|
||||
id: "network-1".to_string(),
|
||||
turn_id: "owner-turn".to_string(),
|
||||
parent_tool_item_id: Some("command-1".to_string()),
|
||||
target: "https://example.com:443".to_string(),
|
||||
host: "example.com".to_string(),
|
||||
protocol: NetworkApprovalProtocol::Https,
|
||||
@@ -371,6 +372,31 @@ fn guardian_request_turn_id_prefers_network_access_owner_turn() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_assessment_action_value_includes_network_access_parent_tool_item_id() {
|
||||
let network_access = GuardianApprovalRequest::NetworkAccess {
|
||||
id: "network-1".to_string(),
|
||||
turn_id: "owner-turn".to_string(),
|
||||
parent_tool_item_id: Some("command-1".to_string()),
|
||||
target: "https://example.com:443".to_string(),
|
||||
host: "example.com".to_string(),
|
||||
protocol: NetworkApprovalProtocol::Https,
|
||||
port: 443,
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
guardian_assessment_action_value(&network_access),
|
||||
serde_json::json!({
|
||||
"tool": "network_access",
|
||||
"target": "https://example.com:443",
|
||||
"host": "example.com",
|
||||
"protocol": "https",
|
||||
"port": 443,
|
||||
"parent_tool_item_id": "command-1",
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
|
||||
let (session, turn, rx) = crate::codex::make_session_and_context_with_rx().await;
|
||||
|
||||
@@ -162,6 +162,7 @@ impl PendingHostApproval {
|
||||
struct ActiveNetworkApprovalCall {
|
||||
registration_id: String,
|
||||
turn_id: String,
|
||||
parent_tool_item_id: String,
|
||||
}
|
||||
|
||||
pub(crate) struct NetworkApprovalService {
|
||||
@@ -194,7 +195,12 @@ impl NetworkApprovalService {
|
||||
other_approved_hosts.extend(approved_hosts.iter().cloned());
|
||||
}
|
||||
|
||||
async fn register_call(&self, registration_id: String, turn_id: String) {
|
||||
async fn register_call(
|
||||
&self,
|
||||
registration_id: String,
|
||||
turn_id: String,
|
||||
parent_tool_item_id: String,
|
||||
) {
|
||||
let mut active_calls = self.active_calls.lock().await;
|
||||
let key = registration_id.clone();
|
||||
active_calls.insert(
|
||||
@@ -202,6 +208,7 @@ impl NetworkApprovalService {
|
||||
Arc::new(ActiveNetworkApprovalCall {
|
||||
registration_id,
|
||||
turn_id,
|
||||
parent_tool_item_id,
|
||||
}),
|
||||
);
|
||||
}
|
||||
@@ -361,6 +368,9 @@ impl NetworkApprovalService {
|
||||
turn_id: owner_call
|
||||
.as_ref()
|
||||
.map_or_else(|| turn_context.sub_id.clone(), |call| call.turn_id.clone()),
|
||||
parent_tool_item_id: owner_call
|
||||
.as_ref()
|
||||
.map(|call| call.parent_tool_item_id.clone()),
|
||||
target,
|
||||
host: request.host,
|
||||
protocol,
|
||||
@@ -548,6 +558,7 @@ pub(crate) fn build_network_policy_decider(
|
||||
pub(crate) async fn begin_network_approval(
|
||||
session: &Session,
|
||||
turn_id: &str,
|
||||
parent_tool_item_id: &str,
|
||||
has_managed_network_requirements: bool,
|
||||
spec: Option<NetworkApprovalSpec>,
|
||||
) -> Option<ActiveNetworkApproval> {
|
||||
@@ -560,7 +571,11 @@ pub(crate) async fn begin_network_approval(
|
||||
session
|
||||
.services
|
||||
.network_approval
|
||||
.register_call(registration_id.clone(), turn_id.to_string())
|
||||
.register_call(
|
||||
registration_id.clone(),
|
||||
turn_id.to_string(),
|
||||
parent_tool_item_id.to_string(),
|
||||
)
|
||||
.await;
|
||||
|
||||
Some(ActiveNetworkApproval {
|
||||
|
||||
@@ -197,7 +197,11 @@ fn denied_blocked_request(host: &str) -> BlockedRequest {
|
||||
async fn record_blocked_request_sets_policy_outcome_for_owner_call() {
|
||||
let service = NetworkApprovalService::default();
|
||||
service
|
||||
.register_call("registration-1".to_string(), "turn-1".to_string())
|
||||
.register_call(
|
||||
"registration-1".to_string(),
|
||||
"turn-1".to_string(),
|
||||
"command-1".to_string(),
|
||||
)
|
||||
.await;
|
||||
|
||||
service
|
||||
@@ -216,7 +220,11 @@ async fn record_blocked_request_sets_policy_outcome_for_owner_call() {
|
||||
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())
|
||||
.register_call(
|
||||
"registration-1".to_string(),
|
||||
"turn-1".to_string(),
|
||||
"command-1".to_string(),
|
||||
)
|
||||
.await;
|
||||
|
||||
service
|
||||
@@ -236,10 +244,18 @@ async fn blocked_request_policy_does_not_override_user_denial_outcome() {
|
||||
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())
|
||||
.register_call(
|
||||
"registration-1".to_string(),
|
||||
"turn-1".to_string(),
|
||||
"command-1".to_string(),
|
||||
)
|
||||
.await;
|
||||
service
|
||||
.register_call("registration-2".to_string(), "turn-1".to_string())
|
||||
.register_call(
|
||||
"registration-2".to_string(),
|
||||
"turn-1".to_string(),
|
||||
"command-2".to_string(),
|
||||
)
|
||||
.await;
|
||||
|
||||
service
|
||||
|
||||
@@ -60,6 +60,7 @@ impl ToolOrchestrator {
|
||||
let network_approval = begin_network_approval(
|
||||
&tool_ctx.session,
|
||||
&tool_ctx.turn.sub_id,
|
||||
&tool_ctx.call_id,
|
||||
has_managed_network_requirements,
|
||||
tool.network_approval_spec(req, tool_ctx),
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user