diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 3248a44424..6f0b4e1a95 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -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: diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 34640a50cf..ff4b69da27 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -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); diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 533c9ee11e..c11051d058 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -47,6 +47,7 @@ pub(crate) enum GuardianApprovalRequest { NetworkAccess { id: String, turn_id: String, + parent_tool_item_id: Option, 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!({ diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index e1595ea167..f110789913 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -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; diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 9e92a6b00f..b4e96eeb08 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -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, ) -> Option { @@ -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 { diff --git a/codex-rs/core/src/tools/network_approval_tests.rs b/codex-rs/core/src/tools/network_approval_tests.rs index ad01a45bbd..03e65285a3 100644 --- a/codex-rs/core/src/tools/network_approval_tests.rs +++ b/codex-rs/core/src/tools/network_approval_tests.rs @@ -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 diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 4b53ac156f..1820a254a9 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -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), )