app-server: keep unique guardian review ids

Do not collapse legacy network approval review notifications onto the parent tool item id.

Co-authored-by: Codex <noreply@openai.com>

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:
 

codex-rs/app-server/src/bespoke_event_handling.rs

@@ -224,13 +224,14 @@ fn guardian_auto_approval_review_notification(
         risk_level: assessment.risk_level.map(Into::into),
         rationale: assessment.rationale.clone(),
     };
+    let target_item_id = assessment.id.clone();
     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 +244,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(),
                 },
@@ -2985,17 +2986,18 @@ mod tests {
     }
 
     #[test]
-    fn guardian_assessment_aborted_emits_completed_review_payload() {
+    fn guardian_assessment_aborted_keeps_unique_review_id_for_network_access() {
         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 +3011,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, "guardian-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);

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<String>,
         target: String,
         host: String,
         protocol: NetworkApprovalProtocol,
@@ -248,17 +249,33 @@ 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(),
+                    network_approval_protocol_value(*protocol),
+                ),
+                ("port".to_string(), Value::from(*port)),
+            ]);
+            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 +337,33 @@ 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(),
+                    network_approval_protocol_value(*protocol),
+                ),
+                ("port".to_string(), Value::from(*port)),
+            ]);
+            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!({
@@ -341,6 +374,18 @@ pub(crate) fn guardian_assessment_action_value(action: &GuardianApprovalRequest)
     }
 }
 
+fn network_approval_protocol_value(protocol: NetworkApprovalProtocol) -> Value {
+    Value::String(
+        match protocol {
+            NetworkApprovalProtocol::Http => "http",
+            NetworkApprovalProtocol::Https => "https",
+            NetworkApprovalProtocol::Socks5Tcp => "socks5_tcp",
+            NetworkApprovalProtocol::Socks5Udp => "socks5_udp",
+        }
+        .to_string(),
+    )
+}
+
 pub(crate) fn guardian_request_id(request: &GuardianApprovalRequest) -> &str {
     match request {
         GuardianApprovalRequest::Shell { id, .. }

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;

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<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 {

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

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),
         )