From 9fc1605b9b8acab091df960b22c700d35f69c470 Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Sun, 22 Mar 2026 00:27:50 -0700 Subject: [PATCH 01/12] guardian: attach network approvals to parent tool items Co-authored-by: Codex --- codex-rs/app-server/README.md | 2 +- .../app-server/src/bespoke_event_handling.rs | 57 ++++++++++++++-- .../core/src/guardian/approval_request.rs | 67 +++++++++++++++---- codex-rs/core/src/guardian/tests.rs | 26 +++++++ codex-rs/core/src/tools/network_approval.rs | 19 +++++- .../core/src/tools/network_approval_tests.rs | 24 +++++-- codex-rs/core/src/tools/orchestrator.rs | 1 + 7 files changed, 171 insertions(+), 25 deletions(-) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 3248a444240..6f0b4e1a951 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 34640a50cf4..ff4b69da270 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(), }, @@ -2986,6 +2998,43 @@ mod tests { #[test] fn guardian_assessment_aborted_emits_completed_review_payload() { + 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: "guardian-3".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, "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", @@ -2995,7 +3044,7 @@ mod tests { &conversation_id, "turn-from-event", &GuardianAssessmentEvent { - id: "item-3".to_string(), + id: "guardian-4".to_string(), turn_id: "turn-from-assessment".to_string(), status: codex_protocol::protocol::GuardianAssessmentStatus::Aborted, risk_score: None, @@ -3009,7 +3058,7 @@ 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-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 533c9ee11e9..c11051d0580 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 e1595ea1676..f110789913c 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 9e92a6b00fc..b4e96eeb085 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 ad01a45bbd3..03e65285a3a 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 4b53ac156fd..1820a254a95 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), ) From 9e6f3de553b1bdae9c0da3371e518732f216d38a Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Sun, 22 Mar 2026 01:01:49 -0700 Subject: [PATCH 02/12] codex: fix CI failure on PR #15439 Co-authored-by: Codex --- .../core/src/guardian/approval_request.rs | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index c11051d0580..652d2579e5b 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -264,12 +264,17 @@ pub(crate) fn guardian_approval_request_to_json( ("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"), + Value::String( + match protocol { + NetworkApprovalProtocol::Http => "Http", + NetworkApprovalProtocol::Https => "Https", + NetworkApprovalProtocol::Socks5Tcp => "Socks5Tcp", + NetworkApprovalProtocol::Socks5Udp => "Socks5Udp", + } + .to_string(), + ), ), + ("port".to_string(), Value::from(*port)), ]); if let Some(parent_tool_item_id) = parent_tool_item_id { action.insert( @@ -355,12 +360,17 @@ pub(crate) fn guardian_assessment_action_value(action: &GuardianApprovalRequest) ("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"), + Value::String( + match protocol { + NetworkApprovalProtocol::Http => "Http", + NetworkApprovalProtocol::Https => "Https", + NetworkApprovalProtocol::Socks5Tcp => "Socks5Tcp", + NetworkApprovalProtocol::Socks5Udp => "Socks5Udp", + } + .to_string(), + ), ), + ("port".to_string(), Value::from(*port)), ]); if let Some(parent_tool_item_id) = parent_tool_item_id { action.insert( From 2c43c6d71d6bc350587cd3147b44b83e9ccbfc7e Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Sun, 22 Mar 2026 01:12:48 -0700 Subject: [PATCH 03/12] codex: fix Bazel test failure on PR #15439 Co-authored-by: Codex --- .../core/src/guardian/approval_request.rs | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 652d2579e5b..1ca808daa69 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -264,15 +264,7 @@ pub(crate) fn guardian_approval_request_to_json( ("host".to_string(), Value::String(host.clone())), ( "protocol".to_string(), - Value::String( - match protocol { - NetworkApprovalProtocol::Http => "Http", - NetworkApprovalProtocol::Https => "Https", - NetworkApprovalProtocol::Socks5Tcp => "Socks5Tcp", - NetworkApprovalProtocol::Socks5Udp => "Socks5Udp", - } - .to_string(), - ), + network_approval_protocol_value(protocol), ), ("port".to_string(), Value::from(*port)), ]); @@ -360,15 +352,7 @@ pub(crate) fn guardian_assessment_action_value(action: &GuardianApprovalRequest) ("host".to_string(), Value::String(host.clone())), ( "protocol".to_string(), - Value::String( - match protocol { - NetworkApprovalProtocol::Http => "Http", - NetworkApprovalProtocol::Https => "Https", - NetworkApprovalProtocol::Socks5Tcp => "Socks5Tcp", - NetworkApprovalProtocol::Socks5Udp => "Socks5Udp", - } - .to_string(), - ), + network_approval_protocol_value(protocol), ), ("port".to_string(), Value::from(*port)), ]); @@ -390,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, .. } From 7b565d13a2ea5bfb29f3dee88b813c718d277f8b Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Sun, 22 Mar 2026 01:18:02 -0700 Subject: [PATCH 04/12] codex: fix clippy follow-up on PR #15439 Co-authored-by: Codex --- codex-rs/core/src/guardian/approval_request.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 1ca808daa69..515f93c197d 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -264,7 +264,7 @@ pub(crate) fn guardian_approval_request_to_json( ("host".to_string(), Value::String(host.clone())), ( "protocol".to_string(), - network_approval_protocol_value(protocol), + network_approval_protocol_value(*protocol), ), ("port".to_string(), Value::from(*port)), ]); @@ -352,7 +352,7 @@ pub(crate) fn guardian_assessment_action_value(action: &GuardianApprovalRequest) ("host".to_string(), Value::String(host.clone())), ( "protocol".to_string(), - network_approval_protocol_value(protocol), + network_approval_protocol_value(*protocol), ), ("port".to_string(), Value::from(*port)), ]); @@ -374,7 +374,7 @@ pub(crate) fn guardian_assessment_action_value(action: &GuardianApprovalRequest) } } -fn network_approval_protocol_value(protocol: &NetworkApprovalProtocol) -> Value { +fn network_approval_protocol_value(protocol: NetworkApprovalProtocol) -> Value { Value::String( match protocol { NetworkApprovalProtocol::Http => "http", From d66e3dfa1906c2d171a6ba4092af5247e5a94fc4 Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Sun, 22 Mar 2026 11:32:59 -0700 Subject: [PATCH 05/12] 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 --- .../app-server/src/bespoke_event_handling.rs | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index ff4b69da270..275731ac25d 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -224,18 +224,7 @@ 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); + let target_item_id = assessment.id.clone(); match assessment.status { codex_protocol::protocol::GuardianAssessmentStatus::InProgress => { ServerNotification::ItemGuardianApprovalReviewStarted( @@ -2997,7 +2986,7 @@ 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", @@ -3022,7 +3011,7 @@ 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, "command-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); From 33077d50e19c031071c67a8dd2b2c6f57e571765 Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Sun, 22 Mar 2026 14:59:50 -0700 Subject: [PATCH 06/12] app-server: type network approval parent item ids Thread a typed parent tool item id through guardian assessment events and legacy app-server guardian review notifications for network approvals. Co-authored-by: Codex --- .../schema/json/ServerNotification.json | 14 +++++ .../codex_app_server_protocol.schemas.json | 14 +++++ .../codex_app_server_protocol.v2.schemas.json | 14 +++++ ...anApprovalReviewCompletedNotification.json | 7 +++ ...dianApprovalReviewStartedNotification.json | 7 +++ ...dianApprovalReviewCompletedNotification.ts | 2 +- ...ardianApprovalReviewStartedNotification.ts | 2 +- .../app-server-protocol/src/protocol/v2.rs | 4 ++ codex-rs/app-server/README.md | 6 +-- .../app-server/src/bespoke_event_handling.rs | 12 ++++- codex-rs/core/src/codex_delegate_tests.rs | 1 + .../core/src/guardian/approval_request.rs | 48 +++++++++-------- codex-rs/core/src/guardian/review.rs | 7 +++ codex-rs/core/src/guardian/tests.rs | 53 ++++++++++++++++++- codex-rs/protocol/src/approvals.rs | 5 ++ 15 files changed, 165 insertions(+), 31 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 5b06ab539c9..6161a4aa1de 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -1352,6 +1352,13 @@ "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, + "parentToolItemId": { + "default": null, + "type": [ + "string", + "null" + ] + }, "review": { "$ref": "#/definitions/GuardianApprovalReview" }, @@ -1377,6 +1384,13 @@ "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, + "parentToolItemId": { + "default": null, + "type": [ + "string", + "null" + ] + }, "review": { "$ref": "#/definitions/GuardianApprovalReview" }, diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index c24c8ac2493..4d7b6a2c7fd 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -8199,6 +8199,13 @@ "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, + "parentToolItemId": { + "default": null, + "type": [ + "string", + "null" + ] + }, "review": { "$ref": "#/definitions/v2/GuardianApprovalReview" }, @@ -8226,6 +8233,13 @@ "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, + "parentToolItemId": { + "default": null, + "type": [ + "string", + "null" + ] + }, "review": { "$ref": "#/definitions/v2/GuardianApprovalReview" }, diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index c479da94e4f..14a9939056c 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -4947,6 +4947,13 @@ "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, + "parentToolItemId": { + "default": null, + "type": [ + "string", + "null" + ] + }, "review": { "$ref": "#/definitions/GuardianApprovalReview" }, @@ -4974,6 +4981,13 @@ "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, + "parentToolItemId": { + "default": null, + "type": [ + "string", + "null" + ] + }, "review": { "$ref": "#/definitions/GuardianApprovalReview" }, diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json index df96e86d164..d0a95b8b3e9 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json @@ -60,6 +60,13 @@ "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, + "parentToolItemId": { + "default": null, + "type": [ + "string", + "null" + ] + }, "review": { "$ref": "#/definitions/GuardianApprovalReview" }, diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json index 339396a50b4..80159d3c184 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json @@ -60,6 +60,13 @@ "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, + "parentToolItemId": { + "default": null, + "type": [ + "string", + "null" + ] + }, "review": { "$ref": "#/definitions/GuardianApprovalReview" }, diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts index ac4ae1b78a1..06b953f9f56 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts @@ -12,4 +12,4 @@ import type { GuardianApprovalReview } from "./GuardianApprovalReview"; * lifecycle instead of sending separate standalone review notifications so the * app-server API can persist and replay review state via `thread/read`. */ -export type ItemGuardianApprovalReviewCompletedNotification = { threadId: string, turnId: string, targetItemId: string, review: GuardianApprovalReview, action: JsonValue | null, }; +export type ItemGuardianApprovalReviewCompletedNotification = { threadId: string, turnId: string, targetItemId: string, parentToolItemId: string | null, review: GuardianApprovalReview, action: JsonValue | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts index b229626817e..29e38b99228 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts @@ -12,4 +12,4 @@ import type { GuardianApprovalReview } from "./GuardianApprovalReview"; * lifecycle instead of sending separate standalone review notifications so the * app-server API can persist and replay review state via `thread/read`. */ -export type ItemGuardianApprovalReviewStartedNotification = { threadId: string, turnId: string, targetItemId: string, review: GuardianApprovalReview, action: JsonValue | null, }; +export type ItemGuardianApprovalReviewStartedNotification = { threadId: string, turnId: string, targetItemId: string, parentToolItemId: string | null, review: GuardianApprovalReview, action: JsonValue | null, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 57017833a6a..89c8a67af15 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -4839,6 +4839,8 @@ pub struct ItemGuardianApprovalReviewStartedNotification { pub thread_id: String, pub turn_id: String, pub target_item_id: String, + #[serde(default)] + pub parent_tool_item_id: Option, pub review: GuardianApprovalReview, pub action: Option, } @@ -4856,6 +4858,8 @@ pub struct ItemGuardianApprovalReviewCompletedNotification { pub thread_id: String, pub turn_id: String, pub target_item_id: String, + #[serde(default)] + pub parent_tool_item_id: Option, pub review: GuardianApprovalReview, pub action: Option, } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 6f0b4e1a951..aedb4a41a1f 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -876,10 +876,10 @@ All items emit shared lifecycle events: - `item/started` — emits the full `item` when a new unit of work begins so the UI can render it immediately; the `item.id` in this payload matches the `itemId` used by deltas. - `item/completed` — sends the final `item` once that work itself finishes (for example, after a tool call or message completes); treat this as the authoritative execution/result state. -- `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. +- `item/autoApprovalReview/started` — [UNSTABLE] temporary guardian notification carrying `{threadId, turnId, targetItemId, parentToolItemId, 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, parentToolItemId, 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. `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. +`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` is the stable guardian review id for matching started/completed notifications. `parentToolItemId` points at the reviewed parent tool item when app-server can attribute the review to one, including attributed `network_access` reviews. 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 275731ac25d..f6bc03768db 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -224,6 +224,7 @@ fn guardian_auto_approval_review_notification( risk_level: assessment.risk_level.map(Into::into), rationale: assessment.rationale.clone(), }; + let parent_tool_item_id = assessment.parent_tool_item_id.clone(); let target_item_id = assessment.id.clone(); match assessment.status { codex_protocol::protocol::GuardianAssessmentStatus::InProgress => { @@ -232,6 +233,7 @@ fn guardian_auto_approval_review_notification( thread_id: conversation_id.to_string(), turn_id, target_item_id, + parent_tool_item_id, review, action: assessment.action.clone(), }, @@ -245,6 +247,7 @@ fn guardian_auto_approval_review_notification( thread_id: conversation_id.to_string(), turn_id, target_item_id, + parent_tool_item_id, review, action: assessment.action.clone(), }, @@ -2919,6 +2922,7 @@ mod tests { "turn-from-event", &GuardianAssessmentEvent { id: "item-1".to_string(), + parent_tool_item_id: None, turn_id: String::new(), status: codex_protocol::protocol::GuardianAssessmentStatus::InProgress, risk_score: None, @@ -2933,6 +2937,7 @@ mod tests { assert_eq!(payload.thread_id, conversation_id.to_string()); assert_eq!(payload.turn_id, "turn-from-event"); assert_eq!(payload.target_item_id, "item-1"); + assert_eq!(payload.parent_tool_item_id, None); assert_eq!( payload.review.status, GuardianApprovalReviewStatus::InProgress @@ -2958,6 +2963,7 @@ mod tests { "turn-from-event", &GuardianAssessmentEvent { id: "item-2".to_string(), + parent_tool_item_id: None, turn_id: "turn-from-assessment".to_string(), status: codex_protocol::protocol::GuardianAssessmentStatus::Denied, risk_score: Some(91), @@ -2972,6 +2978,7 @@ mod tests { assert_eq!(payload.thread_id, conversation_id.to_string()); assert_eq!(payload.turn_id, "turn-from-assessment"); assert_eq!(payload.target_item_id, "item-2"); + assert_eq!(payload.parent_tool_item_id, None); assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Denied); assert_eq!(payload.review.risk_score, Some(91)); assert_eq!( @@ -2990,7 +2997,6 @@ 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( @@ -2998,6 +3004,7 @@ mod tests { "turn-from-event", &GuardianAssessmentEvent { id: "guardian-3".to_string(), + parent_tool_item_id: Some("command-3".to_string()), turn_id: "turn-from-assessment".to_string(), status: codex_protocol::protocol::GuardianAssessmentStatus::Aborted, risk_score: None, @@ -3012,6 +3019,7 @@ mod tests { assert_eq!(payload.thread_id, conversation_id.to_string()); assert_eq!(payload.turn_id, "turn-from-assessment"); assert_eq!(payload.target_item_id, "guardian-3"); + assert_eq!(payload.parent_tool_item_id.as_deref(), Some("command-3")); assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Aborted); assert_eq!(payload.review.risk_score, None); assert_eq!(payload.review.risk_level, None); @@ -3034,6 +3042,7 @@ mod tests { "turn-from-event", &GuardianAssessmentEvent { id: "guardian-4".to_string(), + parent_tool_item_id: None, turn_id: "turn-from-assessment".to_string(), status: codex_protocol::protocol::GuardianAssessmentStatus::Aborted, risk_score: None, @@ -3048,6 +3057,7 @@ mod tests { 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.parent_tool_item_id, None); 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/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 8201424d8e5..abf52ade432 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -313,6 +313,7 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f assessment_event, GuardianAssessmentEvent { id: "command-item-1".to_string(), + parent_tool_item_id: None, turn_id: parent_ctx.sub_id.clone(), status: GuardianAssessmentStatus::InProgress, risk_score: None, diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 515f93c197d..579ea5547c9 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -337,33 +337,18 @@ pub(crate) fn guardian_assessment_action_value(action: &GuardianApprovalRequest) GuardianApprovalRequest::NetworkAccess { id: _, turn_id: _, - parent_tool_item_id, target, host, protocol, 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) - } + .. + } => serde_json::json!({ + "tool": "network_access", + "target": target, + "host": host, + "protocol": network_approval_protocol_value(*protocol), + "port": port, + }), GuardianApprovalRequest::McpToolCall { server, tool_name, .. } => serde_json::json!({ @@ -413,6 +398,23 @@ pub(crate) fn guardian_request_turn_id<'a>( } } +pub(crate) fn guardian_request_parent_tool_item_id( + request: &GuardianApprovalRequest, +) -> Option<&str> { + match request { + GuardianApprovalRequest::NetworkAccess { + parent_tool_item_id, + .. + } => parent_tool_item_id.as_deref(), + GuardianApprovalRequest::Shell { .. } + | GuardianApprovalRequest::ExecCommand { .. } + | GuardianApprovalRequest::ApplyPatch { .. } + | GuardianApprovalRequest::McpToolCall { .. } => None, + #[cfg(unix)] + GuardianApprovalRequest::Execve { .. } => None, + } +} + pub(crate) fn format_guardian_action_pretty( action: &GuardianApprovalRequest, ) -> serde_json::Result { diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 3a491f6efbe..1b89de5ad49 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -20,6 +20,7 @@ use super::GuardianApprovalRequest; use super::GuardianAssessment; use super::approval_request::guardian_assessment_action_value; use super::approval_request::guardian_request_id; +use super::approval_request::guardian_request_parent_tool_item_id; use super::approval_request::guardian_request_turn_id; use super::prompt::build_guardian_prompt_items; use super::prompt::guardian_output_schema; @@ -81,12 +82,15 @@ async fn run_guardian_review( ) -> ReviewDecision { let assessment_id = guardian_request_id(&request).to_string(); let assessment_turn_id = guardian_request_turn_id(&request, &turn.sub_id).to_string(); + let assessment_parent_tool_item_id = + guardian_request_parent_tool_item_id(&request).map(ToString::to_string); let action_summary = guardian_assessment_action_value(&request); session .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: assessment_id.clone(), + parent_tool_item_id: assessment_parent_tool_item_id.clone(), turn_id: assessment_turn_id.clone(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -106,6 +110,7 @@ async fn run_guardian_review( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: assessment_id, + parent_tool_item_id: assessment_parent_tool_item_id.clone(), turn_id: assessment_turn_id, status: GuardianAssessmentStatus::Aborted, risk_score: None, @@ -156,6 +161,7 @@ async fn run_guardian_review( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: assessment_id, + parent_tool_item_id: assessment_parent_tool_item_id.clone(), turn_id: assessment_turn_id, status: GuardianAssessmentStatus::Aborted, risk_score: None, @@ -192,6 +198,7 @@ async fn run_guardian_review( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: assessment_id, + parent_tool_item_id: assessment_parent_tool_item_id, turn_id: assessment_turn_id, status, risk_score: Some(assessment.risk_score), diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index f110789913c..06e52136feb 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -373,7 +373,7 @@ fn guardian_request_turn_id_prefers_network_access_owner_turn() { } #[test] -fn guardian_assessment_action_value_includes_network_access_parent_tool_item_id() { +fn guardian_assessment_action_value_omits_network_access_parent_tool_item_id() { let network_access = GuardianApprovalRequest::NetworkAccess { id: "network-1".to_string(), turn_id: "owner-turn".to_string(), @@ -392,7 +392,6 @@ fn guardian_assessment_action_value_includes_network_access_parent_tool_item_id( "host": "example.com", "protocol": "https", "port": 443, - "parent_tool_item_id": "command-1", }) ); } @@ -441,6 +440,56 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() { assert!(warnings.is_empty()); } +#[tokio::test] +async fn cancelled_network_guardian_review_preserves_parent_tool_item_id() { + let (session, turn, rx) = crate::codex::make_session_and_context_with_rx().await; + let cancel_token = CancellationToken::new(); + cancel_token.cancel(); + + let decision = review_approval_request_with_cancel( + &session, + &turn, + 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, + }, + None, + cancel_token, + ) + .await; + + assert_eq!(decision, ReviewDecision::Abort); + + let mut guardian_events = Vec::new(); + while let Ok(event) = rx.try_recv() { + if let EventMsg::GuardianAssessment(event) = event.msg { + guardian_events.push(event); + } + } + + assert_eq!( + guardian_events + .into_iter() + .map(|event| (event.status, event.parent_tool_item_id)) + .collect::>(), + vec![ + ( + GuardianAssessmentStatus::InProgress, + Some("command-1".to_string()), + ), + ( + GuardianAssessmentStatus::Aborted, + Some("command-1".to_string()), + ), + ] + ); +} + #[tokio::test] async fn routes_approval_to_guardian_requires_auto_only_review_policy() { let (_session, mut turn) = crate::codex::make_session_and_context().await; diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 848ea31c029..0252b1f744b 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -118,6 +118,11 @@ pub struct ExecApprovalRequestSkillMetadata { pub struct GuardianAssessmentEvent { /// Stable identifier for this guardian review lifecycle. pub id: String, + /// Parent tool item identifier when this guardian review is attached to a + /// specific tool call. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub parent_tool_item_id: Option, /// Turn ID that this assessment belongs to. /// Uses `#[serde(default)]` for backwards compatibility. #[serde(default)] From 8e2dc92d6a249d2acf0237e67e5f4e3c91eba003 Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Sun, 22 Mar 2026 15:36:06 -0700 Subject: [PATCH 07/12] app-server: add review ids for guardian notifications Keep `targetItemId` as a deprecated compatibility alias while emitting the clearer `reviewId` field on legacy guardian approval review notifications.\n\nCo-authored-by: Codex --- .../schema/json/ServerNotification.json | 22 ++++++- .../codex_app_server_protocol.schemas.json | 22 ++++++- .../codex_app_server_protocol.v2.schemas.json | 22 ++++++- ...anApprovalReviewCompletedNotification.json | 11 +++- ...dianApprovalReviewStartedNotification.json | 11 +++- ...dianApprovalReviewCompletedNotification.ts | 12 +++- ...ardianApprovalReviewStartedNotification.ts | 12 +++- .../app-server-protocol/src/protocol/v2.rs | 12 +++- codex-rs/app-server/README.md | 6 +- .../app-server/src/bespoke_event_handling.rs | 9 ++- codex-rs/tui_app_server/src/chatwidget.rs | 12 +++- .../tui_app_server/src/chatwidget/tests.rs | 64 +++++++++++++++++++ 12 files changed, 195 insertions(+), 20 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 6161a4aa1de..722da8334cd 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -1349,7 +1349,7 @@ "type": "object" }, "ItemGuardianApprovalReviewCompletedNotification": { - "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", + "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, "parentToolItemId": { @@ -1362,7 +1362,16 @@ "review": { "$ref": "#/definitions/GuardianApprovalReview" }, + "reviewId": { + "default": null, + "description": "Stable guardian review identifier for matching started/completed events.", + "type": [ + "string", + "null" + ] + }, "targetItemId": { + "description": "Deprecated alias for `review_id`, retained for backwards compatibility.", "type": "string" }, "threadId": { @@ -1381,7 +1390,7 @@ "type": "object" }, "ItemGuardianApprovalReviewStartedNotification": { - "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", + "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, "parentToolItemId": { @@ -1394,7 +1403,16 @@ "review": { "$ref": "#/definitions/GuardianApprovalReview" }, + "reviewId": { + "default": null, + "description": "Stable guardian review identifier for matching started/completed events.", + "type": [ + "string", + "null" + ] + }, "targetItemId": { + "description": "Deprecated alias for `review_id`, retained for backwards compatibility.", "type": "string" }, "threadId": { diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 4d7b6a2c7fd..987f134c131 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -8196,7 +8196,7 @@ }, "ItemGuardianApprovalReviewCompletedNotification": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", + "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, "parentToolItemId": { @@ -8209,7 +8209,16 @@ "review": { "$ref": "#/definitions/v2/GuardianApprovalReview" }, + "reviewId": { + "default": null, + "description": "Stable guardian review identifier for matching started/completed events.", + "type": [ + "string", + "null" + ] + }, "targetItemId": { + "description": "Deprecated alias for `review_id`, retained for backwards compatibility.", "type": "string" }, "threadId": { @@ -8230,7 +8239,7 @@ }, "ItemGuardianApprovalReviewStartedNotification": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", + "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, "parentToolItemId": { @@ -8243,7 +8252,16 @@ "review": { "$ref": "#/definitions/v2/GuardianApprovalReview" }, + "reviewId": { + "default": null, + "description": "Stable guardian review identifier for matching started/completed events.", + "type": [ + "string", + "null" + ] + }, "targetItemId": { + "description": "Deprecated alias for `review_id`, retained for backwards compatibility.", "type": "string" }, "threadId": { diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 14a9939056c..0bf0e147f2f 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -4944,7 +4944,7 @@ }, "ItemGuardianApprovalReviewCompletedNotification": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", + "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, "parentToolItemId": { @@ -4957,7 +4957,16 @@ "review": { "$ref": "#/definitions/GuardianApprovalReview" }, + "reviewId": { + "default": null, + "description": "Stable guardian review identifier for matching started/completed events.", + "type": [ + "string", + "null" + ] + }, "targetItemId": { + "description": "Deprecated alias for `review_id`, retained for backwards compatibility.", "type": "string" }, "threadId": { @@ -4978,7 +4987,7 @@ }, "ItemGuardianApprovalReviewStartedNotification": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", + "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, "parentToolItemId": { @@ -4991,7 +5000,16 @@ "review": { "$ref": "#/definitions/GuardianApprovalReview" }, + "reviewId": { + "default": null, + "description": "Stable guardian review identifier for matching started/completed events.", + "type": [ + "string", + "null" + ] + }, "targetItemId": { + "description": "Deprecated alias for `review_id`, retained for backwards compatibility.", "type": "string" }, "threadId": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json index d0a95b8b3e9..a41e28df547 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json @@ -57,7 +57,7 @@ "type": "string" } }, - "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", + "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, "parentToolItemId": { @@ -70,7 +70,16 @@ "review": { "$ref": "#/definitions/GuardianApprovalReview" }, + "reviewId": { + "default": null, + "description": "Stable guardian review identifier for matching started/completed events.", + "type": [ + "string", + "null" + ] + }, "targetItemId": { + "description": "Deprecated alias for `review_id`, retained for backwards compatibility.", "type": "string" }, "threadId": { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json index 80159d3c184..fa4a0bfbe07 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json @@ -57,7 +57,7 @@ "type": "string" } }, - "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", + "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", "properties": { "action": true, "parentToolItemId": { @@ -70,7 +70,16 @@ "review": { "$ref": "#/definitions/GuardianApprovalReview" }, + "reviewId": { + "default": null, + "description": "Stable guardian review identifier for matching started/completed events.", + "type": [ + "string", + "null" + ] + }, "targetItemId": { + "description": "Deprecated alias for `review_id`, retained for backwards compatibility.", "type": "string" }, "threadId": { diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts index 06b953f9f56..53b0f4abbbd 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts @@ -6,10 +6,18 @@ import type { GuardianApprovalReview } from "./GuardianApprovalReview"; /** * [UNSTABLE] Temporary notification payload for guardian automatic approval - * review. This shape is expected to change soon. + * review. * * TODO(ccunningham): Attach guardian review state to the reviewed tool item's * lifecycle instead of sending separate standalone review notifications so the * app-server API can persist and replay review state via `thread/read`. */ -export type ItemGuardianApprovalReviewCompletedNotification = { threadId: string, turnId: string, targetItemId: string, parentToolItemId: string | null, review: GuardianApprovalReview, action: JsonValue | null, }; +export type ItemGuardianApprovalReviewCompletedNotification = { threadId: string, turnId: string, +/** + * Deprecated alias for `review_id`, retained for backwards compatibility. + */ +targetItemId: string, +/** + * Stable guardian review identifier for matching started/completed events. + */ +reviewId: string | null, parentToolItemId: string | null, review: GuardianApprovalReview, action: JsonValue | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts index 29e38b99228..6fda7a1a865 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts @@ -6,10 +6,18 @@ import type { GuardianApprovalReview } from "./GuardianApprovalReview"; /** * [UNSTABLE] Temporary notification payload for guardian automatic approval - * review. This shape is expected to change soon. + * review. * * TODO(ccunningham): Attach guardian review state to the reviewed tool item's * lifecycle instead of sending separate standalone review notifications so the * app-server API can persist and replay review state via `thread/read`. */ -export type ItemGuardianApprovalReviewStartedNotification = { threadId: string, turnId: string, targetItemId: string, parentToolItemId: string | null, review: GuardianApprovalReview, action: JsonValue | null, }; +export type ItemGuardianApprovalReviewStartedNotification = { threadId: string, turnId: string, +/** + * Deprecated alias for `review_id`, retained for backwards compatibility. + */ +targetItemId: string, +/** + * Stable guardian review identifier for matching started/completed events. + */ +reviewId: string | null, parentToolItemId: string | null, review: GuardianApprovalReview, action: JsonValue | null, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 89c8a67af15..2400d1771f3 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -4830,7 +4830,7 @@ pub struct ItemStartedNotification { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] /// [UNSTABLE] Temporary notification payload for guardian automatic approval -/// review. This shape is expected to change soon. +/// review. /// /// TODO(ccunningham): Attach guardian review state to the reviewed tool item's /// lifecycle instead of sending separate standalone review notifications so the @@ -4838,7 +4838,11 @@ pub struct ItemStartedNotification { pub struct ItemGuardianApprovalReviewStartedNotification { pub thread_id: String, pub turn_id: String, + /// Deprecated alias for `review_id`, retained for backwards compatibility. pub target_item_id: String, + /// Stable guardian review identifier for matching started/completed events. + #[serde(default)] + pub review_id: Option, #[serde(default)] pub parent_tool_item_id: Option, pub review: GuardianApprovalReview, @@ -4849,7 +4853,7 @@ pub struct ItemGuardianApprovalReviewStartedNotification { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] /// [UNSTABLE] Temporary notification payload for guardian automatic approval -/// review. This shape is expected to change soon. +/// review. /// /// TODO(ccunningham): Attach guardian review state to the reviewed tool item's /// lifecycle instead of sending separate standalone review notifications so the @@ -4857,7 +4861,11 @@ pub struct ItemGuardianApprovalReviewStartedNotification { pub struct ItemGuardianApprovalReviewCompletedNotification { pub thread_id: String, pub turn_id: String, + /// Deprecated alias for `review_id`, retained for backwards compatibility. pub target_item_id: String, + /// Stable guardian review identifier for matching started/completed events. + #[serde(default)] + pub review_id: Option, #[serde(default)] pub parent_tool_item_id: Option, pub review: GuardianApprovalReview, diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index aedb4a41a1f..253eace3032 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -876,10 +876,10 @@ All items emit shared lifecycle events: - `item/started` — emits the full `item` when a new unit of work begins so the UI can render it immediately; the `item.id` in this payload matches the `itemId` used by deltas. - `item/completed` — sends the final `item` once that work itself finishes (for example, after a tool call or message completes); treat this as the authoritative execution/result state. -- `item/autoApprovalReview/started` — [UNSTABLE] temporary guardian notification carrying `{threadId, turnId, targetItemId, parentToolItemId, 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, parentToolItemId, review, action?}` when guardian approval review resolves. This shape is expected to change soon. +- `item/autoApprovalReview/started` — [UNSTABLE] temporary guardian notification carrying `{threadId, turnId, targetItemId, reviewId, parentToolItemId, review, action?}` when guardian approval review begins. +- `item/autoApprovalReview/completed` — [UNSTABLE] temporary guardian notification carrying `{threadId, turnId, targetItemId, reviewId, parentToolItemId, review, action?}` when guardian approval review resolves. -`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` is the stable guardian review id for matching started/completed notifications. `parentToolItemId` points at the reviewed parent tool item when app-server can attribute the review to one, including attributed `network_access` reviews. 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. `reviewId` is the stable guardian review id for matching started/completed notifications. `targetItemId` is a deprecated alias for `reviewId` retained for backwards compatibility. `parentToolItemId` points at the reviewed parent tool item when app-server can attribute the review to one, including attributed `network_access` reviews. 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 f6bc03768db..158598ea83e 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -225,7 +225,8 @@ fn guardian_auto_approval_review_notification( rationale: assessment.rationale.clone(), }; let parent_tool_item_id = assessment.parent_tool_item_id.clone(); - let target_item_id = assessment.id.clone(); + let review_id = assessment.id.clone(); + let target_item_id = review_id.clone(); match assessment.status { codex_protocol::protocol::GuardianAssessmentStatus::InProgress => { ServerNotification::ItemGuardianApprovalReviewStarted( @@ -233,6 +234,7 @@ fn guardian_auto_approval_review_notification( thread_id: conversation_id.to_string(), turn_id, target_item_id, + review_id: Some(review_id), parent_tool_item_id, review, action: assessment.action.clone(), @@ -247,6 +249,7 @@ fn guardian_auto_approval_review_notification( thread_id: conversation_id.to_string(), turn_id, target_item_id, + review_id: Some(review_id), parent_tool_item_id, review, action: assessment.action.clone(), @@ -2937,6 +2940,7 @@ mod tests { assert_eq!(payload.thread_id, conversation_id.to_string()); assert_eq!(payload.turn_id, "turn-from-event"); assert_eq!(payload.target_item_id, "item-1"); + assert_eq!(payload.review_id.as_deref(), Some("item-1")); assert_eq!(payload.parent_tool_item_id, None); assert_eq!( payload.review.status, @@ -2978,6 +2982,7 @@ mod tests { assert_eq!(payload.thread_id, conversation_id.to_string()); assert_eq!(payload.turn_id, "turn-from-assessment"); assert_eq!(payload.target_item_id, "item-2"); + assert_eq!(payload.review_id.as_deref(), Some("item-2")); assert_eq!(payload.parent_tool_item_id, None); assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Denied); assert_eq!(payload.review.risk_score, Some(91)); @@ -3019,6 +3024,7 @@ mod tests { assert_eq!(payload.thread_id, conversation_id.to_string()); assert_eq!(payload.turn_id, "turn-from-assessment"); assert_eq!(payload.target_item_id, "guardian-3"); + assert_eq!(payload.review_id.as_deref(), Some("guardian-3")); assert_eq!(payload.parent_tool_item_id.as_deref(), Some("command-3")); assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Aborted); assert_eq!(payload.review.risk_score, None); @@ -3057,6 +3063,7 @@ mod tests { 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_id.as_deref(), Some("guardian-4")); assert_eq!(payload.parent_tool_item_id, None); assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Aborted); assert_eq!(payload.review.risk_score, None); diff --git a/codex-rs/tui_app_server/src/chatwidget.rs b/codex-rs/tui_app_server/src/chatwidget.rs index 82cd0d99bcd..a08268d95e8 100644 --- a/codex-rs/tui_app_server/src/chatwidget.rs +++ b/codex-rs/tui_app_server/src/chatwidget.rs @@ -5984,16 +5984,22 @@ impl ChatWidget { ), ServerNotification::ItemGuardianApprovalReviewStarted(notification) => { self.on_guardian_review_notification( - notification.target_item_id, + notification + .review_id + .unwrap_or(notification.target_item_id), notification.turn_id, + notification.parent_tool_item_id, notification.review, notification.action, ); } ServerNotification::ItemGuardianApprovalReviewCompleted(notification) => { self.on_guardian_review_notification( - notification.target_item_id, + notification + .review_id + .unwrap_or(notification.target_item_id), notification.turn_id, + notification.parent_tool_item_id, notification.review, notification.action, ); @@ -6260,11 +6266,13 @@ impl ChatWidget { &mut self, id: String, turn_id: String, + parent_tool_item_id: Option, review: codex_app_server_protocol::GuardianApprovalReview, action: Option, ) { self.on_guardian_assessment(GuardianAssessmentEvent { id, + parent_tool_item_id, turn_id, status: match review.status { codex_app_server_protocol::GuardianApprovalReviewStatus::InProgress => { diff --git a/codex-rs/tui_app_server/src/chatwidget/tests.rs b/codex-rs/tui_app_server/src/chatwidget/tests.rs index 639b57da09e..108fb49e7e3 100644 --- a/codex-rs/tui_app_server/src/chatwidget/tests.rs +++ b/codex-rs/tui_app_server/src/chatwidget/tests.rs @@ -10105,6 +10105,7 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { id: "guardian-in-progress".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".into(), + parent_tool_item_id: None, turn_id: "turn-1".into(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -10123,6 +10124,7 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { id: "guardian-assessment".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".into(), + parent_tool_item_id: None, turn_id: "turn-1".into(), status: GuardianAssessmentStatus::Denied, risk_score: Some(96), @@ -10166,6 +10168,7 @@ async fn guardian_approved_exec_renders_approved_request() { id: "guardian-assessment".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "thread:child-thread:guardian-1".into(), + parent_tool_item_id: None, turn_id: "turn-1".into(), status: GuardianAssessmentStatus::Approved, risk_score: Some(14), @@ -10217,6 +10220,8 @@ async fn app_server_guardian_review_started_sets_review_status() { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), target_item_id: "guardian-1".to_string(), + review_id: Some("guardian-1".to_string()), + parent_tool_item_id: None, review: GuardianApprovalReview { status: GuardianApprovalReviewStatus::InProgress, risk_score: None, @@ -10255,6 +10260,8 @@ async fn app_server_guardian_review_denied_renders_denied_request_snapshot() { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), target_item_id: "guardian-1".to_string(), + review_id: Some("guardian-1".to_string()), + parent_tool_item_id: None, review: GuardianApprovalReview { status: GuardianApprovalReviewStatus::InProgress, risk_score: None, @@ -10273,6 +10280,8 @@ async fn app_server_guardian_review_denied_renders_denied_request_snapshot() { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), target_item_id: "guardian-1".to_string(), + review_id: Some("guardian-1".to_string()), + parent_tool_item_id: None, review: GuardianApprovalReview { status: GuardianApprovalReviewStatus::Denied, risk_score: Some(96), @@ -10310,6 +10319,57 @@ async fn app_server_guardian_review_denied_renders_denied_request_snapshot() { ); } +#[tokio::test] +async fn app_server_guardian_review_prefers_review_id_over_target_item_id() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + let action = serde_json::json!({ + "tool": "shell", + "command": "curl -sS https://example.com", + }); + + chat.handle_server_notification( + ServerNotification::ItemGuardianApprovalReviewStarted( + ItemGuardianApprovalReviewStartedNotification { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + target_item_id: "deprecated-guardian-id".to_string(), + review_id: Some("guardian-1".to_string()), + parent_tool_item_id: Some("command-1".to_string()), + review: GuardianApprovalReview { + status: GuardianApprovalReviewStatus::InProgress, + risk_score: None, + risk_level: None, + rationale: None, + }, + action: Some(action.clone()), + }, + ), + None, + ); + + chat.handle_server_notification( + ServerNotification::ItemGuardianApprovalReviewCompleted( + ItemGuardianApprovalReviewCompletedNotification { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + target_item_id: "different-deprecated-id".to_string(), + review_id: Some("guardian-1".to_string()), + parent_tool_item_id: Some("command-1".to_string()), + review: GuardianApprovalReview { + status: GuardianApprovalReviewStatus::Denied, + risk_score: Some(96), + risk_level: Some(AppServerGuardianRiskLevel::High), + rationale: Some("Would exfiltrate local source code.".to_string()), + }, + action: Some(action), + }, + ), + None, + ); + + assert!(chat.pending_guardian_review_status.is_empty()); +} + // Snapshot test: status widget active (StatusIndicatorView) // Ensures the VT100 rendering of the status indicator is stable when active. #[tokio::test] @@ -10420,6 +10480,7 @@ async fn guardian_parallel_reviews_render_aggregate_status_snapshot() { id: format!("event-{id}"), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: id.to_string(), + parent_tool_item_id: None, turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -10449,6 +10510,7 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "event-guardian-1".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".to_string(), + parent_tool_item_id: None, turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -10464,6 +10526,7 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "event-guardian-2".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-2".to_string(), + parent_tool_item_id: None, turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -10479,6 +10542,7 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "event-guardian-1-denied".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".to_string(), + parent_tool_item_id: None, turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::Denied, risk_score: Some(92), From 7beebc0c5787480fb09e6e3862785da1979a5f15 Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Sun, 22 Mar 2026 18:02:27 -0700 Subject: [PATCH 08/12] core: populate guardian parent ids for direct reviews Populate parent tool item ids for direct guardian-reviewed tool actions while preserving distinct network approval review ids. Co-authored-by: Codex --- .../schema/json/ServerNotification.json | 2 ++ .../codex_app_server_protocol.schemas.json | 2 ++ .../codex_app_server_protocol.v2.schemas.json | 2 ++ ...anApprovalReviewCompletedNotification.json | 1 + ...dianApprovalReviewStartedNotification.json | 1 + ...dianApprovalReviewCompletedNotification.ts | 8 +++++++- ...ardianApprovalReviewStartedNotification.ts | 8 +++++++- .../app-server-protocol/src/protocol/v2.rs | 6 ++++++ codex-rs/app-server/README.md | 2 +- .../app-server/src/bespoke_event_handling.rs | 8 ++++---- codex-rs/core/src/codex_delegate_tests.rs | 2 +- .../core/src/guardian/approval_request.rs | 10 +++++----- codex-rs/core/src/guardian/tests.rs | 9 +++++++++ codex-rs/tui/src/chatwidget/tests.rs | 7 +++++++ .../tui_app_server/src/chatwidget/tests.rs | 20 +++++++++---------- 15 files changed, 65 insertions(+), 23 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 722da8334cd..021a26bed03 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -1354,6 +1354,7 @@ "action": true, "parentToolItemId": { "default": null, + "description": "Reviewed parent tool item identifier. For direct tool reviews this matches `review_id`; attributed `network_access` reviews may differ, and unattributed `network_access` reviews may omit it.", "type": [ "string", "null" @@ -1395,6 +1396,7 @@ "action": true, "parentToolItemId": { "default": null, + "description": "Reviewed parent tool item identifier. For direct tool reviews this matches `review_id`; attributed `network_access` reviews may differ, and unattributed `network_access` reviews may omit it.", "type": [ "string", "null" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 987f134c131..2af9d0c90c6 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -8201,6 +8201,7 @@ "action": true, "parentToolItemId": { "default": null, + "description": "Reviewed parent tool item identifier. For direct tool reviews this matches `review_id`; attributed `network_access` reviews may differ, and unattributed `network_access` reviews may omit it.", "type": [ "string", "null" @@ -8244,6 +8245,7 @@ "action": true, "parentToolItemId": { "default": null, + "description": "Reviewed parent tool item identifier. For direct tool reviews this matches `review_id`; attributed `network_access` reviews may differ, and unattributed `network_access` reviews may omit it.", "type": [ "string", "null" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 0bf0e147f2f..67f1186e6b1 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -4949,6 +4949,7 @@ "action": true, "parentToolItemId": { "default": null, + "description": "Reviewed parent tool item identifier. For direct tool reviews this matches `review_id`; attributed `network_access` reviews may differ, and unattributed `network_access` reviews may omit it.", "type": [ "string", "null" @@ -4992,6 +4993,7 @@ "action": true, "parentToolItemId": { "default": null, + "description": "Reviewed parent tool item identifier. For direct tool reviews this matches `review_id`; attributed `network_access` reviews may differ, and unattributed `network_access` reviews may omit it.", "type": [ "string", "null" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json index a41e28df547..b76741b2139 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json @@ -62,6 +62,7 @@ "action": true, "parentToolItemId": { "default": null, + "description": "Reviewed parent tool item identifier. For direct tool reviews this matches `review_id`; attributed `network_access` reviews may differ, and unattributed `network_access` reviews may omit it.", "type": [ "string", "null" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json index fa4a0bfbe07..043cd1efdce 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json @@ -62,6 +62,7 @@ "action": true, "parentToolItemId": { "default": null, + "description": "Reviewed parent tool item identifier. For direct tool reviews this matches `review_id`; attributed `network_access` reviews may differ, and unattributed `network_access` reviews may omit it.", "type": [ "string", "null" diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts index 53b0f4abbbd..dc2261818cd 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts @@ -20,4 +20,10 @@ targetItemId: string, /** * Stable guardian review identifier for matching started/completed events. */ -reviewId: string | null, parentToolItemId: string | null, review: GuardianApprovalReview, action: JsonValue | null, }; +reviewId: string | null, +/** + * Reviewed parent tool item identifier. For direct tool reviews this + * matches `review_id`; attributed `network_access` reviews may differ, + * and unattributed `network_access` reviews may omit it. + */ +parentToolItemId: string | null, review: GuardianApprovalReview, action: JsonValue | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts index 6fda7a1a865..76b8f34f69e 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts @@ -20,4 +20,10 @@ targetItemId: string, /** * Stable guardian review identifier for matching started/completed events. */ -reviewId: string | null, parentToolItemId: string | null, review: GuardianApprovalReview, action: JsonValue | null, }; +reviewId: string | null, +/** + * Reviewed parent tool item identifier. For direct tool reviews this + * matches `review_id`; attributed `network_access` reviews may differ, + * and unattributed `network_access` reviews may omit it. + */ +parentToolItemId: string | null, review: GuardianApprovalReview, action: JsonValue | null, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 2400d1771f3..8e15590bf1a 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -4843,6 +4843,9 @@ pub struct ItemGuardianApprovalReviewStartedNotification { /// Stable guardian review identifier for matching started/completed events. #[serde(default)] pub review_id: Option, + /// Reviewed parent tool item identifier. For direct tool reviews this + /// matches `review_id`; attributed `network_access` reviews may differ, + /// and unattributed `network_access` reviews may omit it. #[serde(default)] pub parent_tool_item_id: Option, pub review: GuardianApprovalReview, @@ -4866,6 +4869,9 @@ pub struct ItemGuardianApprovalReviewCompletedNotification { /// Stable guardian review identifier for matching started/completed events. #[serde(default)] pub review_id: Option, + /// Reviewed parent tool item identifier. For direct tool reviews this + /// matches `review_id`; attributed `network_access` reviews may differ, + /// and unattributed `network_access` reviews may omit it. #[serde(default)] pub parent_tool_item_id: Option, pub review: GuardianApprovalReview, diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 253eace3032..f64e2177e71 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, reviewId, parentToolItemId, review, action?}` when guardian approval review begins. - `item/autoApprovalReview/completed` — [UNSTABLE] temporary guardian notification carrying `{threadId, turnId, targetItemId, reviewId, parentToolItemId, review, action?}` when guardian approval review resolves. -`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. `reviewId` is the stable guardian review id for matching started/completed notifications. `targetItemId` is a deprecated alias for `reviewId` retained for backwards compatibility. `parentToolItemId` points at the reviewed parent tool item when app-server can attribute the review to one, including attributed `network_access` reviews. 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. `reviewId` is the stable guardian review id for matching started/completed notifications. `targetItemId` is a deprecated alias for `reviewId` retained for backwards compatibility. `parentToolItemId` points at the reviewed parent tool item; for direct tool reviews it matches `reviewId`, while attributed `network_access` reviews may point at a different parent item and can remain absent when attribution is unavailable. 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 158598ea83e..c2d5e1e2b17 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -2925,7 +2925,7 @@ mod tests { "turn-from-event", &GuardianAssessmentEvent { id: "item-1".to_string(), - parent_tool_item_id: None, + parent_tool_item_id: Some("item-1".to_string()), turn_id: String::new(), status: codex_protocol::protocol::GuardianAssessmentStatus::InProgress, risk_score: None, @@ -2941,7 +2941,7 @@ mod tests { assert_eq!(payload.turn_id, "turn-from-event"); assert_eq!(payload.target_item_id, "item-1"); assert_eq!(payload.review_id.as_deref(), Some("item-1")); - assert_eq!(payload.parent_tool_item_id, None); + assert_eq!(payload.parent_tool_item_id.as_deref(), Some("item-1")); assert_eq!( payload.review.status, GuardianApprovalReviewStatus::InProgress @@ -2967,7 +2967,7 @@ mod tests { "turn-from-event", &GuardianAssessmentEvent { id: "item-2".to_string(), - parent_tool_item_id: None, + parent_tool_item_id: Some("item-2".to_string()), turn_id: "turn-from-assessment".to_string(), status: codex_protocol::protocol::GuardianAssessmentStatus::Denied, risk_score: Some(91), @@ -2983,7 +2983,7 @@ mod tests { assert_eq!(payload.turn_id, "turn-from-assessment"); assert_eq!(payload.target_item_id, "item-2"); assert_eq!(payload.review_id.as_deref(), Some("item-2")); - assert_eq!(payload.parent_tool_item_id, None); + assert_eq!(payload.parent_tool_item_id.as_deref(), Some("item-2")); assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Denied); assert_eq!(payload.review.risk_score, Some(91)); assert_eq!( diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index abf52ade432..73e44cdedd9 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -313,7 +313,7 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f assessment_event, GuardianAssessmentEvent { id: "command-item-1".to_string(), - parent_tool_item_id: None, + parent_tool_item_id: Some("command-item-1".to_string()), turn_id: parent_ctx.sub_id.clone(), status: GuardianAssessmentStatus::InProgress, risk_score: None, diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 579ea5547c9..817ac31ca18 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -406,12 +406,12 @@ pub(crate) fn guardian_request_parent_tool_item_id( parent_tool_item_id, .. } => parent_tool_item_id.as_deref(), - GuardianApprovalRequest::Shell { .. } - | GuardianApprovalRequest::ExecCommand { .. } - | GuardianApprovalRequest::ApplyPatch { .. } - | GuardianApprovalRequest::McpToolCall { .. } => None, + GuardianApprovalRequest::Shell { id, .. } + | GuardianApprovalRequest::ExecCommand { id, .. } + | GuardianApprovalRequest::ApplyPatch { id, .. } + | GuardianApprovalRequest::McpToolCall { id, .. } => Some(id), #[cfg(unix)] - GuardianApprovalRequest::Execve { .. } => None, + GuardianApprovalRequest::Execve { id, .. } => Some(id), } } diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 06e52136feb..df1f29c66b6 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -1,3 +1,4 @@ +use super::approval_request::guardian_request_parent_tool_item_id; use super::*; use crate::codex::Session; use crate::codex::TurnContext; @@ -370,6 +371,14 @@ fn guardian_request_turn_id_prefers_network_access_owner_turn() { guardian_request_turn_id(&apply_patch, "fallback-turn"), "fallback-turn" ); + assert_eq!( + guardian_request_parent_tool_item_id(&network_access), + Some("command-1") + ); + assert_eq!( + guardian_request_parent_tool_item_id(&apply_patch), + Some("patch-1") + ); } #[test] diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index a614d1361d2..92c9c97e6de 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -9557,6 +9557,7 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { id: "guardian-in-progress".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".into(), + parent_tool_item_id: Some("guardian-1".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -9575,6 +9576,7 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { id: "guardian-assessment".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".into(), + parent_tool_item_id: Some("guardian-1".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::Denied, risk_score: Some(96), @@ -9618,6 +9620,7 @@ async fn guardian_approved_exec_renders_approved_request() { id: "guardian-assessment".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "thread:child-thread:guardian-1".into(), + parent_tool_item_id: Some("thread:child-thread:guardian-1".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::Approved, risk_score: Some(14), @@ -9765,6 +9768,7 @@ async fn guardian_parallel_reviews_render_aggregate_status_snapshot() { id: format!("event-{id}"), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: id.to_string(), + parent_tool_item_id: Some(id.to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -9794,6 +9798,7 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "event-guardian-1".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".to_string(), + parent_tool_item_id: Some("guardian-1".to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -9809,6 +9814,7 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "event-guardian-2".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-2".to_string(), + parent_tool_item_id: Some("guardian-2".to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -9824,6 +9830,7 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "event-guardian-1-denied".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".to_string(), + parent_tool_item_id: Some("guardian-1".to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::Denied, risk_score: Some(92), diff --git a/codex-rs/tui_app_server/src/chatwidget/tests.rs b/codex-rs/tui_app_server/src/chatwidget/tests.rs index 108fb49e7e3..e1c08acfa08 100644 --- a/codex-rs/tui_app_server/src/chatwidget/tests.rs +++ b/codex-rs/tui_app_server/src/chatwidget/tests.rs @@ -10105,7 +10105,7 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { id: "guardian-in-progress".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".into(), - parent_tool_item_id: None, + parent_tool_item_id: Some("guardian-1".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -10124,7 +10124,7 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { id: "guardian-assessment".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".into(), - parent_tool_item_id: None, + parent_tool_item_id: Some("guardian-1".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::Denied, risk_score: Some(96), @@ -10168,7 +10168,7 @@ async fn guardian_approved_exec_renders_approved_request() { id: "guardian-assessment".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "thread:child-thread:guardian-1".into(), - parent_tool_item_id: None, + parent_tool_item_id: Some("thread:child-thread:guardian-1".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::Approved, risk_score: Some(14), @@ -10221,7 +10221,7 @@ async fn app_server_guardian_review_started_sets_review_status() { turn_id: "turn-1".to_string(), target_item_id: "guardian-1".to_string(), review_id: Some("guardian-1".to_string()), - parent_tool_item_id: None, + parent_tool_item_id: Some("guardian-1".to_string()), review: GuardianApprovalReview { status: GuardianApprovalReviewStatus::InProgress, risk_score: None, @@ -10261,7 +10261,7 @@ async fn app_server_guardian_review_denied_renders_denied_request_snapshot() { turn_id: "turn-1".to_string(), target_item_id: "guardian-1".to_string(), review_id: Some("guardian-1".to_string()), - parent_tool_item_id: None, + parent_tool_item_id: Some("guardian-1".to_string()), review: GuardianApprovalReview { status: GuardianApprovalReviewStatus::InProgress, risk_score: None, @@ -10281,7 +10281,7 @@ async fn app_server_guardian_review_denied_renders_denied_request_snapshot() { turn_id: "turn-1".to_string(), target_item_id: "guardian-1".to_string(), review_id: Some("guardian-1".to_string()), - parent_tool_item_id: None, + parent_tool_item_id: Some("guardian-1".to_string()), review: GuardianApprovalReview { status: GuardianApprovalReviewStatus::Denied, risk_score: Some(96), @@ -10480,7 +10480,7 @@ async fn guardian_parallel_reviews_render_aggregate_status_snapshot() { id: format!("event-{id}"), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: id.to_string(), - parent_tool_item_id: None, + parent_tool_item_id: Some(id.to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -10510,7 +10510,7 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "event-guardian-1".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".to_string(), - parent_tool_item_id: None, + parent_tool_item_id: Some("guardian-1".to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -10526,7 +10526,7 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "event-guardian-2".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-2".to_string(), - parent_tool_item_id: None, + parent_tool_item_id: Some("guardian-2".to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_score: None, @@ -10542,7 +10542,7 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial() id: "event-guardian-1-denied".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".to_string(), - parent_tool_item_id: None, + parent_tool_item_id: Some("guardian-1".to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::Denied, risk_score: Some(92), From 7d50d6c3f0b4af1bf075c28b55bf13290217bdba Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Sun, 22 Mar 2026 22:47:46 -0700 Subject: [PATCH 09/12] core: relax shell snapshot apply_patch timeout Co-authored-by: Codex --- codex-rs/core/tests/suite/shell_snapshot.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/core/tests/suite/shell_snapshot.rs b/codex-rs/core/tests/suite/shell_snapshot.rs index 68228a412eb..1f6632b33c1 100644 --- a/codex-rs/core/tests/suite/shell_snapshot.rs +++ b/codex-rs/core/tests/suite/shell_snapshot.rs @@ -528,7 +528,7 @@ async fn shell_command_snapshot_still_intercepts_apply_patch() -> Result<()> { let script = "apply_patch <<'EOF'\n*** Begin Patch\n*** Add File: snapshot-apply.txt\n+hello from snapshot\n*** End Patch\nEOF\n"; let args = json!({ "command": script, - "timeout_ms": 1_000, + "timeout_ms": 3_000, }); let call_id = "shell-snapshot-apply-patch"; let responses = vec![ From 58b860f1bb4cf112ee8704212c9f68b4b7aa7983 Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Mon, 23 Mar 2026 14:10:54 -0700 Subject: [PATCH 10/12] core: attribute network approvals to owning tool calls Thread a stable network owner id through managed proxy credentials and use it to resolve the exact active tool call for blocked network requests. This makes concurrent network approvals attribute deterministically and preserves parent tool item ids downstream. Co-authored-by: Codex --- .../app-server/src/codex_message_processor.rs | 1 + codex-rs/app-server/src/command_exec.rs | 2 + codex-rs/core/src/codex_tests.rs | 2 + codex-rs/core/src/codex_tests_guardian.rs | 1 + codex-rs/core/src/exec.rs | 13 +++- codex-rs/core/src/exec_tests.rs | 5 ++ .../core/src/network_policy_decision_tests.rs | 2 + codex-rs/core/src/sandboxing/mod.rs | 4 ++ codex-rs/core/src/sandboxing/mod_tests.rs | 3 + codex-rs/core/src/tasks/user_shell.rs | 1 + .../core/src/tools/handlers/apply_patch.rs | 2 + codex-rs/core/src/tools/handlers/shell.rs | 3 + codex-rs/core/src/tools/js_repl/mod.rs | 1 + codex-rs/core/src/tools/network_approval.rs | 49 +++++++++++--- .../core/src/tools/network_approval_tests.rs | 67 +++++++++++++++++++ codex-rs/core/src/tools/orchestrator.rs | 6 ++ .../core/src/tools/runtimes/apply_patch.rs | 2 +- codex-rs/core/src/tools/runtimes/shell.rs | 6 +- .../tools/runtimes/shell/unix_escalation.rs | 17 ++++- .../runtimes/shell/unix_escalation_tests.rs | 3 + .../core/src/tools/runtimes/unified_exec.rs | 14 +++- codex-rs/core/src/tools/sandboxing.rs | 3 + .../core/src/unified_exec/process_manager.rs | 1 + codex-rs/core/tests/suite/exec.rs | 1 + .../linux-sandbox/tests/suite/landlock.rs | 2 + codex-rs/network-proxy/src/http_proxy.rs | 21 ++++++ codex-rs/network-proxy/src/lib.rs | 1 + codex-rs/network-proxy/src/mitm.rs | 2 + codex-rs/network-proxy/src/network_policy.rs | 9 +++ codex-rs/network-proxy/src/owner_identity.rs | 38 +++++++++++ codex-rs/network-proxy/src/proxy.rs | 47 ++++++++++++- codex-rs/network-proxy/src/runtime.rs | 8 +++ codex-rs/network-proxy/src/socks5.rs | 18 ++++- 33 files changed, 331 insertions(+), 24 deletions(-) create mode 100644 codex-rs/network-proxy/src/owner_identity.rs diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 1b02e4bb6cf..dfa6da2cb5f 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -1690,6 +1690,7 @@ impl CodexMessageProcessor { network: started_network_proxy .as_ref() .map(codex_core::config::StartedNetworkProxy::proxy), + network_owner_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level, windows_sandbox_private_desktop: self diff --git a/codex-rs/app-server/src/command_exec.rs b/codex-rs/app-server/src/command_exec.rs index e1a9cb3def1..1e2bcc7ef4a 100644 --- a/codex-rs/app-server/src/command_exec.rs +++ b/codex-rs/app-server/src/command_exec.rs @@ -732,6 +732,7 @@ mod tests { cwd: PathBuf::from("."), env: HashMap::new(), network: None, + network_owner_id: None, expiration: ExecExpiration::DefaultTimeout, capture_policy: codex_core::exec::ExecCapturePolicy::ShellTool, sandbox: SandboxType::WindowsRestrictedToken, @@ -845,6 +846,7 @@ mod tests { cwd: PathBuf::from("."), env: HashMap::new(), network: None, + network_owner_id: None, expiration: ExecExpiration::Cancellation(CancellationToken::new()), capture_policy: codex_core::exec::ExecCapturePolicy::ShellTool, sandbox: SandboxType::None, diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index c7a715cd93a..087b0660ef6 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -4813,6 +4813,7 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() { capture_policy: ExecCapturePolicy::ShellTool, env: HashMap::new(), network: None, + network_owner_id: None, sandbox_permissions, windows_sandbox_level: turn_context.windows_sandbox_level, windows_sandbox_private_desktop: turn_context @@ -4831,6 +4832,7 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() { capture_policy: ExecCapturePolicy::ShellTool, env: HashMap::new(), network: None, + network_owner_id: None, windows_sandbox_level: turn_context.windows_sandbox_level, windows_sandbox_private_desktop: turn_context .config diff --git a/codex-rs/core/src/codex_tests_guardian.rs b/codex-rs/core/src/codex_tests_guardian.rs index af0fccc9ac2..8eea077dcdc 100644 --- a/codex-rs/core/src/codex_tests_guardian.rs +++ b/codex-rs/core/src/codex_tests_guardian.rs @@ -128,6 +128,7 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid capture_policy: ExecCapturePolicy::ShellTool, env: HashMap::new(), network: None, + network_owner_id: None, sandbox_permissions: SandboxPermissions::WithAdditionalPermissions, windows_sandbox_level: turn_context.windows_sandbox_level, windows_sandbox_private_desktop: turn_context diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 9be0518fa07..ce53baf98fd 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -81,6 +81,7 @@ pub struct ExecParams { pub capture_policy: ExecCapturePolicy, pub env: HashMap, pub network: Option, + pub network_owner_id: Option, pub sandbox_permissions: SandboxPermissions, pub windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel, pub windows_sandbox_private_desktop: bool, @@ -263,6 +264,7 @@ pub fn build_exec_request( expiration, capture_policy, network, + network_owner_id, sandbox_permissions, windows_sandbox_level, windows_sandbox_private_desktop, @@ -270,7 +272,7 @@ pub fn build_exec_request( arg0: _, } = params; if let Some(network) = network.as_ref() { - network.apply_to_env(&mut env); + network.apply_to_env_for_owner(&mut env, network_owner_id.as_deref()); } let (program, args) = command.split_first().ok_or_else(|| { CodexErr::Io(io::Error::new( @@ -301,6 +303,7 @@ pub fn build_exec_request( sandbox: sandbox_type, enforce_managed_network, network: network.as_ref(), + network_owner_id: network_owner_id.as_deref(), sandbox_policy_cwd: sandbox_cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, @@ -324,6 +327,7 @@ pub(crate) async fn execute_exec_request( cwd, env, network, + network_owner_id, expiration, capture_policy, sandbox, @@ -345,6 +349,7 @@ pub(crate) async fn execute_exec_request( capture_policy, env, network: network.clone(), + network_owner_id, sandbox_permissions, windows_sandbox_level, windows_sandbox_private_desktop, @@ -448,6 +453,7 @@ async fn exec_windows_sandbox( cwd, mut env, network, + network_owner_id, expiration, capture_policy, windows_sandbox_level, @@ -455,7 +461,7 @@ async fn exec_windows_sandbox( .. } = params; if let Some(network) = network.as_ref() { - network.apply_to_env(&mut env); + network.apply_to_env_for_owner(&mut env, network_owner_id.as_deref()); } // TODO(iceweasel-oai): run_windows_sandbox_capture should support all @@ -838,6 +844,7 @@ async fn exec( cwd, mut env, network, + network_owner_id, arg0, expiration, capture_policy, @@ -845,7 +852,7 @@ async fn exec( .. } = params; if let Some(network) = network.as_ref() { - network.apply_to_env(&mut env); + network.apply_to_env_for_owner(&mut env, network_owner_id.as_deref()); } let (program, args) = command.split_first().ok_or_else(|| { diff --git a/codex-rs/core/src/exec_tests.rs b/codex-rs/core/src/exec_tests.rs index fc312ec88e3..ca0d0c5ef97 100644 --- a/codex-rs/core/src/exec_tests.rs +++ b/codex-rs/core/src/exec_tests.rs @@ -259,6 +259,7 @@ async fn exec_full_buffer_capture_ignores_expiration() -> Result<()> { capture_policy: ExecCapturePolicy::FullBuffer, env, network: None, + network_owner_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, @@ -298,6 +299,7 @@ async fn exec_full_buffer_capture_keeps_io_drain_timeout_when_descendant_holds_p capture_policy: ExecCapturePolicy::FullBuffer, env: std::env::vars().collect(), network: None, + network_owner_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, @@ -348,6 +350,7 @@ async fn process_exec_tool_call_preserves_full_buffer_capture_policy() -> Result capture_policy: ExecCapturePolicy::FullBuffer, env: std::env::vars().collect(), network: None, + network_owner_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, @@ -588,6 +591,7 @@ async fn kill_child_process_group_kills_grandchildren_on_timeout() -> Result<()> capture_policy: ExecCapturePolicy::ShellTool, env, network: None, + network_owner_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, @@ -646,6 +650,7 @@ async fn process_exec_tool_call_respects_cancellation_token() -> Result<()> { capture_policy: ExecCapturePolicy::ShellTool, env, network: None, + network_owner_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, diff --git a/codex-rs/core/src/network_policy_decision_tests.rs b/codex-rs/core/src/network_policy_decision_tests.rs index ebb17f724fe..404dca6cbda 100644 --- a/codex-rs/core/src/network_policy_decision_tests.rs +++ b/codex-rs/core/src/network_policy_decision_tests.rs @@ -156,6 +156,7 @@ fn denied_network_policy_message_requires_deny_decision() { let blocked = BlockedRequest { host: "example.com".to_string(), reason: "not_allowed".to_string(), + network_owner_id: None, client: None, method: Some("GET".to_string()), mode: None, @@ -173,6 +174,7 @@ fn denied_network_policy_message_for_denylist_block_is_explicit() { let blocked = BlockedRequest { host: "example.com".to_string(), reason: "denied".to_string(), + network_owner_id: None, client: None, method: Some("GET".to_string()), mode: None, diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index 277ff2b2414..81246c0bc68 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -68,6 +68,7 @@ pub struct ExecRequest { pub cwd: PathBuf, pub env: HashMap, pub network: Option, + pub network_owner_id: Option, pub expiration: ExecExpiration, pub capture_policy: ExecCapturePolicy, pub sandbox: SandboxType, @@ -94,6 +95,7 @@ pub(crate) struct SandboxTransformRequest<'a> { // TODO(viyatb): Evaluate switching this to Option> // to make shared ownership explicit across runtime/sandbox plumbing. pub network: Option<&'a NetworkProxy>, + pub network_owner_id: Option<&'a str>, pub sandbox_policy_cwd: &'a Path, #[cfg(target_os = "macos")] pub macos_seatbelt_profile_extensions: Option<&'a MacOsSeatbeltProfileExtensions>, @@ -592,6 +594,7 @@ impl SandboxManager { sandbox, enforce_managed_network, network, + network_owner_id, sandbox_policy_cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions, @@ -709,6 +712,7 @@ impl SandboxManager { cwd: spec.cwd, env, network: network.cloned(), + network_owner_id: network_owner_id.map(ToString::to_string), expiration: spec.expiration, capture_policy: spec.capture_policy, sandbox, diff --git a/codex-rs/core/src/sandboxing/mod_tests.rs b/codex-rs/core/src/sandboxing/mod_tests.rs index 9a7a34e49d0..f93108abc90 100644 --- a/codex-rs/core/src/sandboxing/mod_tests.rs +++ b/codex-rs/core/src/sandboxing/mod_tests.rs @@ -171,6 +171,7 @@ fn transform_preserves_unrestricted_file_system_policy_for_restricted_network() sandbox: SandboxType::None, enforce_managed_network: false, network: None, + network_owner_id: None, sandbox_policy_cwd: cwd.as_path(), #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, @@ -541,6 +542,7 @@ fn transform_additional_permissions_enable_network_for_external_sandbox() { sandbox: SandboxType::None, enforce_managed_network: false, network: None, + network_owner_id: None, sandbox_policy_cwd: cwd.as_path(), #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, @@ -615,6 +617,7 @@ fn transform_additional_permissions_preserves_denied_entries() { sandbox: SandboxType::None, enforce_managed_network: false, network: None, + network_owner_id: None, sandbox_policy_cwd: cwd.as_path(), #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index 6b42be3cef2..f049f062ed4 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -163,6 +163,7 @@ pub(crate) async fn execute_user_shell_command( Some(session.conversation_id), ), network: turn_context.network.clone(), + network_owner_id: None, // TODO(zhao-oai): Now that we have ExecExpiration::Cancellation, we // should use that instead of an "arbitrarily large" timeout here. expiration: USER_SHELL_TIMEOUT_MS.into(), diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 1d799da7e74..35ebcea37be 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -215,6 +215,7 @@ impl ToolHandler for ApplyPatchHandler { session: session.clone(), turn: turn.clone(), call_id: call_id.clone(), + network_approval_owner_id: None, tool_name: tool_name.to_string(), }; let out = orchestrator @@ -319,6 +320,7 @@ pub(crate) async fn intercept_apply_patch( session: session.clone(), turn: turn.clone(), call_id: call_id.to_string(), + network_approval_owner_id: None, tool_name: tool_name.to_string(), }; let out = orchestrator diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 446ca100b76..14bf8717a6f 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -74,6 +74,7 @@ impl ShellHandler { capture_policy: ExecCapturePolicy::ShellTool, env: create_env(&turn_context.shell_environment_policy, Some(thread_id)), network: turn_context.network.clone(), + network_owner_id: None, sandbox_permissions: params.sandbox_permissions.unwrap_or_default(), windows_sandbox_level: turn_context.windows_sandbox_level, windows_sandbox_private_desktop: turn_context @@ -129,6 +130,7 @@ impl ShellCommandHandler { capture_policy: ExecCapturePolicy::ShellTool, env: create_env(&turn_context.shell_environment_policy, Some(thread_id)), network: turn_context.network.clone(), + network_owner_id: None, sandbox_permissions: params.sandbox_permissions.unwrap_or_default(), windows_sandbox_level: turn_context.windows_sandbox_level, windows_sandbox_private_desktop: turn_context @@ -474,6 +476,7 @@ impl ShellHandler { session: session.clone(), turn: turn.clone(), call_id: call_id.clone(), + network_approval_owner_id: None, tool_name, }; let out = orchestrator diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index 4f7c3d74363..7c15a87f188 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -1067,6 +1067,7 @@ impl JsReplManager { sandbox: sandbox_type, enforce_managed_network: has_managed_network_requirements, network: None, + network_owner_id: None, sandbox_policy_cwd: &turn.cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index b4e96eeb085..d2a36d70069 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -64,6 +64,10 @@ impl ActiveNetworkApproval { self.mode } + pub(crate) fn owner_id(&self) -> Option<&str> { + self.registration_id.as_deref() + } + pub(crate) fn into_deferred(self) -> Option { match (self.mode, self.registration_id) { (NetworkApprovalMode::Deferred, Some(registration_id)) => { @@ -220,6 +224,18 @@ impl NetworkApprovalService { call_outcomes.remove(registration_id); } + async fn resolve_owner_call( + &self, + network_owner_id: Option<&str>, + ) -> Option> { + if let Some(network_owner_id) = network_owner_id { + let active_calls = self.active_calls.lock().await; + return active_calls.get(network_owner_id).cloned(); + } + + self.resolve_single_active_call().await + } + async fn resolve_single_active_call(&self) -> Option> { let active_calls = self.active_calls.lock().await; if active_calls.len() == 1 { @@ -243,8 +259,12 @@ impl NetworkApprovalService { (created, true) } - async fn record_outcome_for_single_active_call(&self, outcome: NetworkApprovalOutcome) { - let Some(owner_call) = self.resolve_single_active_call().await else { + async fn record_outcome_for_owner_call( + &self, + network_owner_id: Option<&str>, + outcome: NetworkApprovalOutcome, + ) { + let Some(owner_call) = self.resolve_owner_call(network_owner_id).await else { return; }; self.record_call_outcome(&owner_call.registration_id, outcome) @@ -272,8 +292,11 @@ impl NetworkApprovalService { return; }; - self.record_outcome_for_single_active_call(NetworkApprovalOutcome::DeniedByPolicy(message)) - .await; + self.record_outcome_for_owner_call( + blocked.network_owner_id.as_deref(), + NetworkApprovalOutcome::DeniedByPolicy(message), + ) + .await; } async fn active_turn_context(session: &Session) -> Option> { @@ -335,9 +358,10 @@ impl NetworkApprovalService { pending.set_decision(PendingApprovalDecision::Deny).await; let mut pending_approvals = self.pending_host_approvals.lock().await; pending_approvals.remove(&key); - self.record_outcome_for_single_active_call(NetworkApprovalOutcome::DeniedByPolicy( - policy_denial_message, - )) + self.record_outcome_for_owner_call( + request.network_owner_id.as_deref(), + NetworkApprovalOutcome::DeniedByPolicy(policy_denial_message), + ) .await; return NetworkDecision::deny(REASON_NOT_ALLOWED); }; @@ -345,9 +369,10 @@ impl NetworkApprovalService { pending.set_decision(PendingApprovalDecision::Deny).await; let mut pending_approvals = self.pending_host_approvals.lock().await; pending_approvals.remove(&key); - self.record_outcome_for_single_active_call(NetworkApprovalOutcome::DeniedByPolicy( - policy_denial_message, - )) + self.record_outcome_for_owner_call( + request.network_owner_id.as_deref(), + NetworkApprovalOutcome::DeniedByPolicy(policy_denial_message), + ) .await; return NetworkDecision::deny(REASON_NOT_ALLOWED); } @@ -356,7 +381,9 @@ impl NetworkApprovalService { host: request.host.clone(), protocol, }; - let owner_call = self.resolve_single_active_call().await; + let owner_call = self + .resolve_owner_call(request.network_owner_id.as_deref()) + .await; let approval_decision = if routes_approval_to_guardian(&turn_context) { // TODO(ccunningham): Attach guardian network reviews to the reviewed tool item // lifecycle instead of this temporary standalone network approval id. diff --git a/codex-rs/core/src/tools/network_approval_tests.rs b/codex-rs/core/src/tools/network_approval_tests.rs index 03e65285a3a..a3fc4d29097 100644 --- a/codex-rs/core/src/tools/network_approval_tests.rs +++ b/codex-rs/core/src/tools/network_approval_tests.rs @@ -180,9 +180,14 @@ fn only_never_policy_disables_network_approval_flow() { } fn denied_blocked_request(host: &str) -> BlockedRequest { + denied_blocked_request_with_owner(host, None) +} + +fn denied_blocked_request_with_owner(host: &str, network_owner_id: Option<&str>) -> BlockedRequest { BlockedRequest::new(BlockedRequestArgs { host: host.to_string(), reason: "not_allowed".to_string(), + network_owner_id: network_owner_id.map(ToString::to_string), client: None, method: None, mode: None, @@ -265,3 +270,65 @@ async fn record_blocked_request_ignores_ambiguous_unattributed_blocked_requests( assert_eq!(service.take_call_outcome("registration-1").await, None); assert_eq!(service.take_call_outcome("registration-2").await, None); } + +#[tokio::test] +async fn resolve_owner_call_uses_network_owner_id_when_multiple_calls_are_active() { + let service = NetworkApprovalService::default(); + service + .register_call( + "registration-1".to_string(), + "turn-1".to_string(), + "command-1".to_string(), + ) + .await; + service + .register_call( + "registration-2".to_string(), + "turn-2".to_string(), + "command-2".to_string(), + ) + .await; + + let owner_call = service + .resolve_owner_call(Some("registration-2")) + .await + .expect("owner call should resolve"); + + assert_eq!(owner_call.registration_id, "registration-2"); + assert_eq!(owner_call.turn_id, "turn-2"); + assert_eq!(owner_call.parent_tool_item_id, "command-2"); +} + +#[tokio::test] +async fn record_blocked_request_uses_network_owner_id_when_multiple_calls_are_active() { + let service = NetworkApprovalService::default(); + service + .register_call( + "registration-1".to_string(), + "turn-1".to_string(), + "command-1".to_string(), + ) + .await; + service + .register_call( + "registration-2".to_string(), + "turn-2".to_string(), + "command-2".to_string(), + ) + .await; + + service + .record_blocked_request(denied_blocked_request_with_owner( + "example.com", + Some("registration-2"), + )) + .await; + + assert_eq!(service.take_call_outcome("registration-1").await, None); + assert_eq!( + service.take_call_outcome("registration-2").await, + Some(NetworkApprovalOutcome::DeniedByPolicy( + "Network access to \"example.com\" was blocked: domain is not on the allowlist for the current sandbox mode.".to_string() + )) + ); +} diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 1820a254a95..95a5e241db3 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -13,6 +13,7 @@ use crate::guardian::GUARDIAN_REJECTION_MESSAGE; use crate::guardian::routes_approval_to_guardian; use crate::network_policy_decision::network_approval_context_from_payload; use crate::sandboxing::SandboxManager; +use crate::tools::network_approval::ActiveNetworkApproval; use crate::tools::network_approval::DeferredNetworkApproval; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::begin_network_approval; @@ -65,11 +66,16 @@ impl ToolOrchestrator { tool.network_approval_spec(req, tool_ctx), ) .await; + let network_approval_owner_id = network_approval + .as_ref() + .and_then(ActiveNetworkApproval::owner_id) + .map(ToString::to_string); let attempt_tool_ctx = ToolCtx { session: tool_ctx.session.clone(), turn: tool_ctx.turn.clone(), call_id: tool_ctx.call_id.clone(), + network_approval_owner_id, tool_name: tool_ctx.tool_name.clone(), }; let run_result = tool.run(req, attempt, &attempt_tool_ctx).await; diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index f1e9912bc59..151b5d06a33 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -208,7 +208,7 @@ impl ToolRuntime for ApplyPatchRuntime { ) -> Result { let spec = Self::build_command_spec(req, &ctx.turn.config.codex_home)?; let env = attempt - .env_for(spec, /*network*/ None) + .env_for(spec, /*network*/ None, /*network_owner_id*/ None) .map_err(|err| ToolError::Codex(err.into()))?; let out = execute_env(env, Self::stdout_stream(ctx)) .await diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 6ff8349b532..01f6493a607 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -253,7 +253,11 @@ impl ToolRuntime for ShellRuntime { req.justification.clone(), )?; let env = attempt - .env_for(spec, req.network.as_ref()) + .env_for( + spec, + req.network.as_ref(), + ctx.network_approval_owner_id.as_deref(), + ) .map_err(|err| ToolError::Codex(err.into()))?; let out = execute_env(env, Self::stdout_stream(ctx)) .await diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 948018dae6a..4e8da53a0c4 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -117,13 +117,18 @@ pub(super) async fn try_run_zsh_fork( req.justification.clone(), )?; let sandbox_exec_request = attempt - .env_for(spec, req.network.as_ref()) + .env_for( + spec, + req.network.as_ref(), + ctx.network_approval_owner_id.as_deref(), + ) .map_err(|err| ToolError::Codex(err.into()))?; let crate::sandboxing::ExecRequest { command, cwd: sandbox_cwd, env: sandbox_env, network: sandbox_network, + network_owner_id, expiration: _sandbox_expiration, capture_policy: _capture_policy, sandbox, @@ -153,6 +158,7 @@ pub(super) async fn try_run_zsh_fork( sandbox, env: sandbox_env, network: sandbox_network, + network_owner_id, windows_sandbox_level, sandbox_permissions, justification, @@ -259,6 +265,7 @@ pub(crate) async fn prepare_unified_exec_zsh_fork( sandbox: exec_request.sandbox, env: exec_request.env.clone(), network: exec_request.network.clone(), + network_owner_id: exec_request.network_owner_id.clone(), windows_sandbox_level: exec_request.windows_sandbox_level, sandbox_permissions: exec_request.sandbox_permissions, justification: exec_request.justification.clone(), @@ -856,6 +863,7 @@ struct CoreShellCommandExecutor { sandbox: SandboxType, env: HashMap, network: Option, + network_owner_id: Option, windows_sandbox_level: WindowsSandboxLevel, sandbox_permissions: SandboxPermissions, justification: Option, @@ -904,6 +912,7 @@ impl ShellCommandExecutor for CoreShellCommandExecutor { cwd: self.cwd.clone(), env: exec_env, network: self.network.clone(), + network_owner_id: self.network_owner_id.clone(), expiration: ExecExpiration::Cancellation(cancel_rx), capture_policy: ExecCapturePolicy::ShellTool, sandbox: self.sandbox, @@ -1060,6 +1069,7 @@ impl CoreShellCommandExecutor { sandbox, enforce_managed_network: self.network.is_some(), network: self.network.as_ref(), + network_owner_id: self.network_owner_id.as_deref(), sandbox_policy_cwd: &self.sandbox_policy_cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions, @@ -1069,7 +1079,10 @@ impl CoreShellCommandExecutor { windows_sandbox_private_desktop: false, })?; if let Some(network) = exec_request.network.as_ref() { - network.apply_to_env(&mut exec_request.env); + network.apply_to_env_for_owner( + &mut exec_request.env, + exec_request.network_owner_id.as_deref(), + ); } Ok(PreparedExec { diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 37c1b53a136..ec26724af4d 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -655,6 +655,7 @@ async fn prepare_escalated_exec_turn_default_preserves_macos_seatbelt_extensions cwd: cwd.to_path_buf(), env: HashMap::new(), network: None, + network_owner_id: None, sandbox: SandboxType::None, sandbox_policy: SandboxPolicy::new_read_only_policy(), file_system_sandbox_policy: read_only_file_system_sandbox_policy(), @@ -707,6 +708,7 @@ async fn prepare_escalated_exec_permissions_preserve_macos_seatbelt_extensions() cwd: cwd.to_path_buf(), env: HashMap::new(), network: None, + network_owner_id: None, sandbox: SandboxType::None, sandbox_policy: SandboxPolicy::DangerFullAccess, file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), @@ -782,6 +784,7 @@ async fn prepare_escalated_exec_permission_profile_unions_turn_and_requested_mac cwd: cwd.to_path_buf(), env: HashMap::new(), network: None, + network_owner_id: None, sandbox: SandboxType::None, sandbox_policy: sandbox_policy.clone(), file_system_sandbox_policy: read_only_file_system_sandbox_policy(), diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 49ef0e63855..b17f9c7dded 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -207,7 +207,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt let mut env = req.env.clone(); if let Some(network) = req.network.as_ref() { - network.apply_to_env(&mut env); + network.apply_to_env_for_owner(&mut env, ctx.network_approval_owner_id.as_deref()); } if let UnifiedExecShellMode::ZshFork(zsh_fork_config) = &self.shell_mode { let spec = build_command_spec( @@ -221,7 +221,11 @@ impl<'a> ToolRuntime for UnifiedExecRunt ) .map_err(|_| ToolError::Rejected("missing command line for PTY".to_string()))?; let exec_env = attempt - .env_for(spec, req.network.as_ref()) + .env_for( + spec, + req.network.as_ref(), + ctx.network_approval_owner_id.as_deref(), + ) .map_err(|err| ToolError::Codex(err.into()))?; match zsh_fork_backend::maybe_prepare_unified_exec( req, @@ -269,7 +273,11 @@ impl<'a> ToolRuntime for UnifiedExecRunt ) .map_err(|_| ToolError::Rejected("missing command line for PTY".to_string()))?; let exec_env = attempt - .env_for(spec, req.network.as_ref()) + .env_for( + spec, + req.network.as_ref(), + ctx.network_approval_owner_id.as_deref(), + ) .map_err(|err| ToolError::Codex(err.into()))?; self.manager .open_session_with_exec_env(&exec_env, req.tty, Box::new(NoopSpawnLifecycle)) diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index bf386a12d0b..3251f833022 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -300,6 +300,7 @@ pub(crate) struct ToolCtx { pub session: Arc, pub turn: Arc, pub call_id: String, + pub network_approval_owner_id: Option, pub tool_name: String, } @@ -341,6 +342,7 @@ impl<'a> SandboxAttempt<'a> { &self, spec: CommandSpec, network: Option<&NetworkProxy>, + network_owner_id: Option<&str>, ) -> Result { self.manager .transform(crate::sandboxing::SandboxTransformRequest { @@ -351,6 +353,7 @@ impl<'a> SandboxAttempt<'a> { sandbox: self.sandbox, enforce_managed_network: self.enforce_managed_network, network, + network_owner_id, sandbox_policy_cwd: self.sandbox_cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 52d668c0004..0ea2e653e2b 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -627,6 +627,7 @@ impl UnifiedExecProcessManager { session: context.session.clone(), turn: context.turn.clone(), call_id: context.call_id.clone(), + network_approval_owner_id: None, tool_name: "exec_command".to_string(), }; orchestrator diff --git a/codex-rs/core/tests/suite/exec.rs b/codex-rs/core/tests/suite/exec.rs index 069e824ee57..22b19c71b76 100644 --- a/codex-rs/core/tests/suite/exec.rs +++ b/codex-rs/core/tests/suite/exec.rs @@ -41,6 +41,7 @@ async fn run_test_cmd(tmp: TempDir, cmd: Vec<&str>) -> Result(req: &Request) -> Option { + req.headers() + .typed_get::>() + .map(|header| header.0.username().to_string()) +} + async fn http_connect_accept( policy_decider: Option>, mut req: Request, @@ -173,6 +181,7 @@ async fn http_connect_accept( } let client = client_addr(&req); + let network_owner_id = proxy_authorization_network_owner_id(&req); let enabled = app_state .enabled() .await @@ -185,6 +194,7 @@ async fn http_connect_accept( host, authority.port, client_addr(&req), + network_owner_id, Some("CONNECT".to_string()), NetworkProtocol::HttpsConnect, /*audit_endpoint_override*/ None, @@ -196,6 +206,7 @@ async fn http_connect_accept( protocol: NetworkProtocol::HttpsConnect, host: host.clone(), port: authority.port, + network_owner_id: network_owner_id.clone(), client_addr: client.clone(), method: Some("CONNECT".to_string()), command: None, @@ -220,6 +231,7 @@ async fn http_connect_accept( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: reason.clone(), + network_owner_id, client: client.clone(), method: Some("CONNECT".to_string()), mode: None, @@ -283,6 +295,7 @@ async fn http_connect_accept( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_MITM_REQUIRED.to_string(), + network_owner_id: None, client: client.clone(), method: Some("CONNECT".to_string()), mode: Some(NetworkMode::Limited), @@ -432,6 +445,7 @@ async fn http_plain_proxy( } }; let client = client_addr(&req); + let network_owner_id = proxy_authorization_network_owner_id(&req); let method_allowed = match app_state .method_allowed(req.method().as_str()) .await @@ -471,6 +485,7 @@ async fn http_plain_proxy( socket_path, /*port*/ 0, client_addr(&req), + network_owner_id.clone(), Some(req.method().as_str().to_string()), NetworkProtocol::Http, Some(("unix-socket", 0)), @@ -616,6 +631,7 @@ async fn http_plain_proxy( host, port, client_addr(&req), + network_owner_id.clone(), Some(req.method().as_str().to_string()), NetworkProtocol::Http, /*audit_endpoint_override*/ None, @@ -627,6 +643,7 @@ async fn http_plain_proxy( protocol: NetworkProtocol::Http, host: host.clone(), port, + network_owner_id: network_owner_id.clone(), client_addr: client.clone(), method: Some(req.method().as_str().to_string()), command: None, @@ -651,6 +668,7 @@ async fn http_plain_proxy( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: reason.clone(), + network_owner_id: network_owner_id.clone(), client: client.clone(), method: Some(req.method().as_str().to_string()), mode: None, @@ -696,6 +714,7 @@ async fn http_plain_proxy( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_METHOD_NOT_ALLOWED.to_string(), + network_owner_id, client: client.clone(), method: Some(req.method().as_str().to_string()), mode: Some(NetworkMode::Limited), @@ -889,6 +908,7 @@ async fn proxy_disabled_response( host: String, port: u16, client: Option, + network_owner_id: Option, method: Option, protocol: NetworkProtocol, audit_endpoint_override: Option<(&str, u16)>, @@ -913,6 +933,7 @@ async fn proxy_disabled_response( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: blocked_host, reason: REASON_PROXY_DISABLED.to_string(), + network_owner_id, client, method, mode: None, diff --git a/codex-rs/network-proxy/src/lib.rs b/codex-rs/network-proxy/src/lib.rs index 1093a14aa44..f896bf72e3c 100644 --- a/codex-rs/network-proxy/src/lib.rs +++ b/codex-rs/network-proxy/src/lib.rs @@ -5,6 +5,7 @@ mod config; mod http_proxy; mod mitm; mod network_policy; +mod owner_identity; mod policy; mod proxy; mod reasons; diff --git a/codex-rs/network-proxy/src/mitm.rs b/codex-rs/network-proxy/src/mitm.rs index 8f20afaa3d2..5b37e1207c0 100644 --- a/codex-rs/network-proxy/src/mitm.rs +++ b/codex-rs/network-proxy/src/mitm.rs @@ -289,6 +289,7 @@ async fn mitm_blocking_response( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: policy.target_host.clone(), reason: reason.to_string(), + network_owner_id: None, client: client.clone(), method: Some(method.clone()), mode: Some(policy.mode), @@ -311,6 +312,7 @@ async fn mitm_blocking_response( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: policy.target_host.clone(), reason: REASON_METHOD_NOT_ALLOWED.to_string(), + network_owner_id: None, client: client.clone(), method: Some(method.clone()), mode: Some(policy.mode), diff --git a/codex-rs/network-proxy/src/network_policy.rs b/codex-rs/network-proxy/src/network_policy.rs index e759e75c0d6..0aadc10bfda 100644 --- a/codex-rs/network-proxy/src/network_policy.rs +++ b/codex-rs/network-proxy/src/network_policy.rs @@ -79,6 +79,7 @@ pub struct NetworkPolicyRequest { pub protocol: NetworkProtocol, pub host: String, pub port: u16, + pub network_owner_id: Option, pub client_addr: Option, pub method: Option, pub command: Option, @@ -89,6 +90,7 @@ pub struct NetworkPolicyRequestArgs { pub protocol: NetworkProtocol, pub host: String, pub port: u16, + pub network_owner_id: Option, pub client_addr: Option, pub method: Option, pub command: Option, @@ -101,6 +103,7 @@ impl NetworkPolicyRequest { protocol, host, port, + network_owner_id, client_addr, method, command, @@ -110,6 +113,7 @@ impl NetworkPolicyRequest { protocol, host, port, + network_owner_id, client_addr, method, command, @@ -625,6 +629,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "example.com".to_string(), port: 80, + network_owner_id: None, client_addr: None, method: None, command: None, @@ -685,6 +690,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "blocked.com".to_string(), port: 80, + network_owner_id: None, client_addr: Some("127.0.0.1:1234".to_string()), method: Some("GET".to_string()), command: None, @@ -726,6 +732,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "example.com".to_string(), port: 80, + network_owner_id: None, client_addr: None, method: Some("GET".to_string()), command: None, @@ -776,6 +783,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "example.com".to_string(), port: 80, + network_owner_id: None, client_addr: None, method: Some("GET".to_string()), command: None, @@ -859,6 +867,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "127.0.0.1".to_string(), port: 80, + network_owner_id: None, client_addr: None, method: Some("GET".to_string()), command: None, diff --git a/codex-rs/network-proxy/src/owner_identity.rs b/codex-rs/network-proxy/src/owner_identity.rs new file mode 100644 index 00000000000..3c898ec1d2a --- /dev/null +++ b/codex-rs/network-proxy/src/owner_identity.rs @@ -0,0 +1,38 @@ +use rama_core::extensions::Extensions; +use rama_core::extensions::ExtensionsRef; +use rama_http::headers::authorization::AuthoritySync; +use rama_net::user::Basic; +use rama_net::user::UserId; +use rama_net::user::authority::AuthorizeResult; +use rama_net::user::authority::Authorizer; + +#[derive(Clone, Copy, Debug, Default)] +pub(crate) struct ProxyOwnerAuthorizer; + +impl AuthoritySync for ProxyOwnerAuthorizer { + fn authorized(&self, ext: &mut Extensions, credentials: &Basic) -> bool { + ext.insert(UserId::Username(credentials.username().to_string())); + true + } +} + +impl Authorizer for ProxyOwnerAuthorizer { + type Error = std::convert::Infallible; + + async fn authorize(&self, credentials: Basic) -> AuthorizeResult { + let mut extensions = Extensions::new(); + extensions.insert(UserId::Username(credentials.username().to_string())); + AuthorizeResult { + credentials, + result: Ok(Some(extensions)), + } + } +} + +pub(crate) fn network_owner_id(input: &T) -> Option { + match input.extensions().get::() { + Some(UserId::Username(username)) => Some(username.clone()), + Some(UserId::Token(token)) => String::from_utf8(token.clone()).ok(), + Some(UserId::Anonymous) | None => None, + } +} diff --git a/codex-rs/network-proxy/src/proxy.rs b/codex-rs/network-proxy/src/proxy.rs index 8f596f68420..8cf281d27f1 100644 --- a/codex-rs/network-proxy/src/proxy.rs +++ b/codex-rs/network-proxy/src/proxy.rs @@ -305,15 +305,23 @@ fn set_env_keys(env: &mut HashMap, keys: &[&str], value: &str) { } } +fn proxy_url(scheme: &str, addr: SocketAddr, network_owner_id: Option<&str>) -> String { + match network_owner_id { + Some(network_owner_id) => format!("{scheme}://{network_owner_id}@{addr}"), + None => format!("{scheme}://{addr}"), + } +} + fn apply_proxy_env_overrides( env: &mut HashMap, http_addr: SocketAddr, socks_addr: SocketAddr, socks_enabled: bool, allow_local_binding: bool, + network_owner_id: Option<&str>, ) { - let http_proxy_url = format!("http://{http_addr}"); - let socks_proxy_url = format!("socks5h://{socks_addr}"); + let http_proxy_url = proxy_url("http", http_addr, network_owner_id); + let socks_proxy_url = proxy_url("socks5h", socks_addr, network_owner_id); env.insert( ALLOW_LOCAL_BINDING_ENV_KEY.to_string(), if allow_local_binding { @@ -414,6 +422,14 @@ impl NetworkProxy { } pub fn apply_to_env(&self, env: &mut HashMap) { + self.apply_to_env_for_owner(env, /*network_owner_id*/ None); + } + + pub fn apply_to_env_for_owner( + &self, + env: &mut HashMap, + network_owner_id: Option<&str>, + ) { // Enforce proxying for child processes. We intentionally override existing values so // command-level environment cannot bypass the managed proxy endpoint. apply_proxy_env_overrides( @@ -422,6 +438,7 @@ impl NetworkProxy { self.socks_addr, self.socks_enabled, self.allow_local_binding, + network_owner_id, ); } @@ -696,6 +713,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, + /*network_owner_id*/ None, ); assert_eq!( @@ -746,6 +764,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), false, true, + /*network_owner_id*/ None, ); assert_eq!( @@ -764,6 +783,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, + /*network_owner_id*/ None, ); assert_eq!( @@ -809,6 +829,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, + /*network_owner_id*/ None, ); assert_eq!( @@ -816,4 +837,26 @@ mod tests { Some(&"ssh -o ProxyCommand='tsh proxy ssh --cluster=dev %r@%h:%p'".to_string()) ); } + + #[test] + fn apply_proxy_env_overrides_includes_owner_credentials() { + let mut env = HashMap::new(); + apply_proxy_env_overrides( + &mut env, + SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 3128), + SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), + true, + false, + Some("owner-1"), + ); + + assert_eq!( + env.get("HTTP_PROXY"), + Some(&"http://owner-1@127.0.0.1:3128".to_string()) + ); + assert_eq!( + env.get("ALL_PROXY"), + Some(&"socks5h://owner-1@127.0.0.1:8081".to_string()) + ); + } } diff --git a/codex-rs/network-proxy/src/runtime.rs b/codex-rs/network-proxy/src/runtime.rs index b634a8630e3..d51729aed89 100644 --- a/codex-rs/network-proxy/src/runtime.rs +++ b/codex-rs/network-proxy/src/runtime.rs @@ -84,6 +84,8 @@ pub enum HostBlockDecision { pub struct BlockedRequest { pub host: String, pub reason: String, + #[serde(skip_serializing)] + pub network_owner_id: Option, pub client: Option, pub method: Option, pub mode: Option, @@ -100,6 +102,7 @@ pub struct BlockedRequest { pub struct BlockedRequestArgs { pub host: String, pub reason: String, + pub network_owner_id: Option, pub client: Option, pub method: Option, pub mode: Option, @@ -114,6 +117,7 @@ impl BlockedRequest { let BlockedRequestArgs { host, reason, + network_owner_id, client, method, mode, @@ -125,6 +129,7 @@ impl BlockedRequest { Self { host, reason, + network_owner_id, client, method, mode, @@ -994,6 +999,7 @@ mod tests { .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: "google.com".to_string(), reason: "not_allowed".to_string(), + network_owner_id: None, client: None, method: Some("GET".to_string()), mode: None, @@ -1034,6 +1040,7 @@ mod tests { .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: format!("example{idx}.com"), reason: "not_allowed".to_string(), + network_owner_id: None, client: None, method: Some("GET".to_string()), mode: None, @@ -1056,6 +1063,7 @@ mod tests { let entry = BlockedRequest { host: "google.com".to_string(), reason: "not_allowed".to_string(), + network_owner_id: None, client: Some("127.0.0.1".to_string()), method: Some("GET".to_string()), mode: Some(NetworkMode::Full), diff --git a/codex-rs/network-proxy/src/socks5.rs b/codex-rs/network-proxy/src/socks5.rs index 30ec4d2903e..16b4f653aed 100644 --- a/codex-rs/network-proxy/src/socks5.rs +++ b/codex-rs/network-proxy/src/socks5.rs @@ -9,6 +9,8 @@ use crate::network_policy::NetworkPolicyRequestArgs; use crate::network_policy::NetworkProtocol; use crate::network_policy::emit_block_decision_audit_event; use crate::network_policy::evaluate_host_policy; +use crate::owner_identity::ProxyOwnerAuthorizer; +use crate::owner_identity::network_owner_id as extract_network_owner_id; use crate::policy::normalize_host; use crate::reasons::REASON_METHOD_NOT_ALLOWED; use crate::reasons::REASON_PROXY_DISABLED; @@ -105,7 +107,11 @@ async fn run_socks5_with_listener( }); let socks_connector = DefaultConnector::default().with_connector(policy_tcp_connector); - let base = Socks5Acceptor::new().with_connector(socks_connector); + let mut base = Socks5Acceptor::new(); + base.set_auth_optional(true); + let base = base + .with_connector(socks_connector) + .with_authorizer(ProxyOwnerAuthorizer); if enable_socks5_udp { let udp_state = state.clone(); @@ -150,6 +156,7 @@ async fn handle_socks5_tcp( .extensions() .get::() .map(|info| info.peer_addr().to_string()); + let network_owner_id = extract_network_owner_id(&req); match app_state.enabled().await { Ok(true) => {} @@ -175,6 +182,7 @@ async fn handle_socks5_tcp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_PROXY_DISABLED.to_string(), + network_owner_id: network_owner_id.clone(), client: client.clone(), method: None, mode: None, @@ -217,6 +225,7 @@ async fn handle_socks5_tcp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_METHOD_NOT_ALLOWED.to_string(), + network_owner_id: network_owner_id.clone(), client: client.clone(), method: None, mode: Some(NetworkMode::Limited), @@ -243,6 +252,7 @@ async fn handle_socks5_tcp( protocol: NetworkProtocol::Socks5Tcp, host: host.clone(), port, + network_owner_id: network_owner_id.clone(), client_addr: client.clone(), method: None, command: None, @@ -267,6 +277,7 @@ async fn handle_socks5_tcp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: reason.clone(), + network_owner_id, client: client.clone(), method: None, mode: None, @@ -314,6 +325,7 @@ async fn inspect_socks5_udp( let client = extensions .get::() .map(|info| info.peer_addr().to_string()); + let network_owner_id = extract_network_owner_id(&extensions); match state.enabled().await { Ok(true) => {} @@ -339,6 +351,7 @@ async fn inspect_socks5_udp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_PROXY_DISABLED.to_string(), + network_owner_id: network_owner_id.clone(), client: client.clone(), method: None, mode: None, @@ -381,6 +394,7 @@ async fn inspect_socks5_udp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_METHOD_NOT_ALLOWED.to_string(), + network_owner_id: network_owner_id.clone(), client: client.clone(), method: None, mode: Some(NetworkMode::Limited), @@ -403,6 +417,7 @@ async fn inspect_socks5_udp( protocol: NetworkProtocol::Socks5Udp, host: host.clone(), port, + network_owner_id: network_owner_id.clone(), client_addr: client.clone(), method: None, command: None, @@ -427,6 +442,7 @@ async fn inspect_socks5_udp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: reason.clone(), + network_owner_id, client: client.clone(), method: None, mode: None, From da3dc36b63b40de20ce979cf7dacf2ab9a5af80b Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Mon, 23 Mar 2026 17:38:01 -0700 Subject: [PATCH 11/12] core: carry parent tool item id for network approvals Remove the synthetic network approval token and carry parent_tool_item_id directly through the managed proxy attribution path. Co-authored-by: Codex --- .../app-server/src/codex_message_processor.rs | 2 +- codex-rs/app-server/src/command_exec.rs | 4 +- codex-rs/core/src/codex_tests.rs | 4 +- codex-rs/core/src/codex_tests_guardian.rs | 2 +- codex-rs/core/src/exec.rs | 22 ++-- codex-rs/core/src/exec_tests.rs | 10 +- .../core/src/network_policy_decision_tests.rs | 4 +- codex-rs/core/src/sandboxing/mod.rs | 13 +- codex-rs/core/src/sandboxing/mod_tests.rs | 6 +- codex-rs/core/src/tasks/user_shell.rs | 2 +- .../core/src/tools/handlers/apply_patch.rs | 2 - codex-rs/core/src/tools/handlers/shell.rs | 5 +- codex-rs/core/src/tools/js_repl/mod.rs | 2 +- codex-rs/core/src/tools/network_approval.rs | 119 ++++++++---------- .../core/src/tools/network_approval_tests.rs | 92 +++++--------- codex-rs/core/src/tools/orchestrator.rs | 15 +-- .../core/src/tools/runtimes/apply_patch.rs | 4 +- codex-rs/core/src/tools/runtimes/shell.rs | 6 +- .../tools/runtimes/shell/unix_escalation.rs | 22 ++-- .../runtimes/shell/unix_escalation_tests.rs | 6 +- .../core/src/tools/runtimes/unified_exec.rs | 14 +-- codex-rs/core/src/tools/sandboxing.rs | 5 +- .../core/src/unified_exec/process_manager.rs | 3 +- codex-rs/core/tests/suite/exec.rs | 2 +- .../linux-sandbox/tests/suite/landlock.rs | 4 +- codex-rs/network-proxy/src/http_proxy.rs | 28 ++--- codex-rs/network-proxy/src/mitm.rs | 4 +- codex-rs/network-proxy/src/network_policy.rs | 23 ++-- codex-rs/network-proxy/src/owner_identity.rs | 11 +- codex-rs/network-proxy/src/proxy.rs | 44 ++++--- codex-rs/network-proxy/src/runtime.rs | 19 +-- codex-rs/network-proxy/src/socks5.rs | 26 ++-- 32 files changed, 243 insertions(+), 282 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index dfa6da2cb5f..e6c4811bcee 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -1690,7 +1690,7 @@ impl CodexMessageProcessor { network: started_network_proxy .as_ref() .map(codex_core::config::StartedNetworkProxy::proxy), - network_owner_id: None, + parent_tool_item_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level, windows_sandbox_private_desktop: self diff --git a/codex-rs/app-server/src/command_exec.rs b/codex-rs/app-server/src/command_exec.rs index 1e2bcc7ef4a..99200b7e473 100644 --- a/codex-rs/app-server/src/command_exec.rs +++ b/codex-rs/app-server/src/command_exec.rs @@ -732,7 +732,7 @@ mod tests { cwd: PathBuf::from("."), env: HashMap::new(), network: None, - network_owner_id: None, + parent_tool_item_id: None, expiration: ExecExpiration::DefaultTimeout, capture_policy: codex_core::exec::ExecCapturePolicy::ShellTool, sandbox: SandboxType::WindowsRestrictedToken, @@ -846,7 +846,7 @@ mod tests { cwd: PathBuf::from("."), env: HashMap::new(), network: None, - network_owner_id: None, + parent_tool_item_id: None, expiration: ExecExpiration::Cancellation(CancellationToken::new()), capture_policy: codex_core::exec::ExecCapturePolicy::ShellTool, sandbox: SandboxType::None, diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 087b0660ef6..9f8103e73d8 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -4813,7 +4813,7 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() { capture_policy: ExecCapturePolicy::ShellTool, env: HashMap::new(), network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox_permissions, windows_sandbox_level: turn_context.windows_sandbox_level, windows_sandbox_private_desktop: turn_context @@ -4832,7 +4832,7 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() { capture_policy: ExecCapturePolicy::ShellTool, env: HashMap::new(), network: None, - network_owner_id: None, + parent_tool_item_id: None, windows_sandbox_level: turn_context.windows_sandbox_level, windows_sandbox_private_desktop: turn_context .config diff --git a/codex-rs/core/src/codex_tests_guardian.rs b/codex-rs/core/src/codex_tests_guardian.rs index 8eea077dcdc..17a68b033ef 100644 --- a/codex-rs/core/src/codex_tests_guardian.rs +++ b/codex-rs/core/src/codex_tests_guardian.rs @@ -128,7 +128,7 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid capture_policy: ExecCapturePolicy::ShellTool, env: HashMap::new(), network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox_permissions: SandboxPermissions::WithAdditionalPermissions, windows_sandbox_level: turn_context.windows_sandbox_level, windows_sandbox_private_desktop: turn_context diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index ce53baf98fd..6d408e4a0b7 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -81,7 +81,9 @@ pub struct ExecParams { pub capture_policy: ExecCapturePolicy, pub env: HashMap, pub network: Option, - pub network_owner_id: Option, + /// Parent tool item id to propagate through managed proxy settings so + /// blocked requests can be attributed back to the originating tool call. + pub parent_tool_item_id: Option, pub sandbox_permissions: SandboxPermissions, pub windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel, pub windows_sandbox_private_desktop: bool, @@ -264,7 +266,7 @@ pub fn build_exec_request( expiration, capture_policy, network, - network_owner_id, + parent_tool_item_id, sandbox_permissions, windows_sandbox_level, windows_sandbox_private_desktop, @@ -272,7 +274,7 @@ pub fn build_exec_request( arg0: _, } = params; if let Some(network) = network.as_ref() { - network.apply_to_env_for_owner(&mut env, network_owner_id.as_deref()); + network.apply_to_env_for_parent_tool_item(&mut env, parent_tool_item_id.as_deref()); } let (program, args) = command.split_first().ok_or_else(|| { CodexErr::Io(io::Error::new( @@ -303,7 +305,7 @@ pub fn build_exec_request( sandbox: sandbox_type, enforce_managed_network, network: network.as_ref(), - network_owner_id: network_owner_id.as_deref(), + parent_tool_item_id: parent_tool_item_id.as_deref(), sandbox_policy_cwd: sandbox_cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, @@ -327,7 +329,7 @@ pub(crate) async fn execute_exec_request( cwd, env, network, - network_owner_id, + parent_tool_item_id, expiration, capture_policy, sandbox, @@ -349,7 +351,7 @@ pub(crate) async fn execute_exec_request( capture_policy, env, network: network.clone(), - network_owner_id, + parent_tool_item_id, sandbox_permissions, windows_sandbox_level, windows_sandbox_private_desktop, @@ -453,7 +455,7 @@ async fn exec_windows_sandbox( cwd, mut env, network, - network_owner_id, + parent_tool_item_id, expiration, capture_policy, windows_sandbox_level, @@ -461,7 +463,7 @@ async fn exec_windows_sandbox( .. } = params; if let Some(network) = network.as_ref() { - network.apply_to_env_for_owner(&mut env, network_owner_id.as_deref()); + network.apply_to_env_for_parent_tool_item(&mut env, parent_tool_item_id.as_deref()); } // TODO(iceweasel-oai): run_windows_sandbox_capture should support all @@ -844,7 +846,7 @@ async fn exec( cwd, mut env, network, - network_owner_id, + parent_tool_item_id, arg0, expiration, capture_policy, @@ -852,7 +854,7 @@ async fn exec( .. } = params; if let Some(network) = network.as_ref() { - network.apply_to_env_for_owner(&mut env, network_owner_id.as_deref()); + network.apply_to_env_for_parent_tool_item(&mut env, parent_tool_item_id.as_deref()); } let (program, args) = command.split_first().ok_or_else(|| { diff --git a/codex-rs/core/src/exec_tests.rs b/codex-rs/core/src/exec_tests.rs index ca0d0c5ef97..703c357761b 100644 --- a/codex-rs/core/src/exec_tests.rs +++ b/codex-rs/core/src/exec_tests.rs @@ -259,7 +259,7 @@ async fn exec_full_buffer_capture_ignores_expiration() -> Result<()> { capture_policy: ExecCapturePolicy::FullBuffer, env, network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, @@ -299,7 +299,7 @@ async fn exec_full_buffer_capture_keeps_io_drain_timeout_when_descendant_holds_p capture_policy: ExecCapturePolicy::FullBuffer, env: std::env::vars().collect(), network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, @@ -350,7 +350,7 @@ async fn process_exec_tool_call_preserves_full_buffer_capture_policy() -> Result capture_policy: ExecCapturePolicy::FullBuffer, env: std::env::vars().collect(), network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, @@ -591,7 +591,7 @@ async fn kill_child_process_group_kills_grandchildren_on_timeout() -> Result<()> capture_policy: ExecCapturePolicy::ShellTool, env, network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, @@ -650,7 +650,7 @@ async fn process_exec_tool_call_respects_cancellation_token() -> Result<()> { capture_policy: ExecCapturePolicy::ShellTool, env, network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, diff --git a/codex-rs/core/src/network_policy_decision_tests.rs b/codex-rs/core/src/network_policy_decision_tests.rs index 404dca6cbda..f0a1a0fc8d3 100644 --- a/codex-rs/core/src/network_policy_decision_tests.rs +++ b/codex-rs/core/src/network_policy_decision_tests.rs @@ -156,7 +156,7 @@ fn denied_network_policy_message_requires_deny_decision() { let blocked = BlockedRequest { host: "example.com".to_string(), reason: "not_allowed".to_string(), - network_owner_id: None, + parent_tool_item_id: None, client: None, method: Some("GET".to_string()), mode: None, @@ -174,7 +174,7 @@ fn denied_network_policy_message_for_denylist_block_is_explicit() { let blocked = BlockedRequest { host: "example.com".to_string(), reason: "denied".to_string(), - network_owner_id: None, + parent_tool_item_id: None, client: None, method: Some("GET".to_string()), mode: None, diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index 81246c0bc68..4975ef431ec 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -68,7 +68,9 @@ pub struct ExecRequest { pub cwd: PathBuf, pub env: HashMap, pub network: Option, - pub network_owner_id: Option, + /// Parent tool item id propagated into child process proxy settings for + /// blocked-request attribution. + pub parent_tool_item_id: Option, pub expiration: ExecExpiration, pub capture_policy: ExecCapturePolicy, pub sandbox: SandboxType, @@ -95,7 +97,10 @@ pub(crate) struct SandboxTransformRequest<'a> { // TODO(viyatb): Evaluate switching this to Option> // to make shared ownership explicit across runtime/sandbox plumbing. pub network: Option<&'a NetworkProxy>, - pub network_owner_id: Option<&'a str>, + /// Parent tool item id for the current tool call. This is threaded into + /// managed proxy credentials so the proxy can report blocked requests + /// against the exact originating call. + pub parent_tool_item_id: Option<&'a str>, pub sandbox_policy_cwd: &'a Path, #[cfg(target_os = "macos")] pub macos_seatbelt_profile_extensions: Option<&'a MacOsSeatbeltProfileExtensions>, @@ -594,7 +599,7 @@ impl SandboxManager { sandbox, enforce_managed_network, network, - network_owner_id, + parent_tool_item_id, sandbox_policy_cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions, @@ -712,7 +717,7 @@ impl SandboxManager { cwd: spec.cwd, env, network: network.cloned(), - network_owner_id: network_owner_id.map(ToString::to_string), + parent_tool_item_id: parent_tool_item_id.map(ToString::to_string), expiration: spec.expiration, capture_policy: spec.capture_policy, sandbox, diff --git a/codex-rs/core/src/sandboxing/mod_tests.rs b/codex-rs/core/src/sandboxing/mod_tests.rs index f93108abc90..75c0722f17d 100644 --- a/codex-rs/core/src/sandboxing/mod_tests.rs +++ b/codex-rs/core/src/sandboxing/mod_tests.rs @@ -171,7 +171,7 @@ fn transform_preserves_unrestricted_file_system_policy_for_restricted_network() sandbox: SandboxType::None, enforce_managed_network: false, network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox_policy_cwd: cwd.as_path(), #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, @@ -542,7 +542,7 @@ fn transform_additional_permissions_enable_network_for_external_sandbox() { sandbox: SandboxType::None, enforce_managed_network: false, network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox_policy_cwd: cwd.as_path(), #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, @@ -617,7 +617,7 @@ fn transform_additional_permissions_preserves_denied_entries() { sandbox: SandboxType::None, enforce_managed_network: false, network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox_policy_cwd: cwd.as_path(), #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index f049f062ed4..b69fb0fa0f7 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -163,7 +163,7 @@ pub(crate) async fn execute_user_shell_command( Some(session.conversation_id), ), network: turn_context.network.clone(), - network_owner_id: None, + parent_tool_item_id: None, // TODO(zhao-oai): Now that we have ExecExpiration::Cancellation, we // should use that instead of an "arbitrarily large" timeout here. expiration: USER_SHELL_TIMEOUT_MS.into(), diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 35ebcea37be..1d799da7e74 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -215,7 +215,6 @@ impl ToolHandler for ApplyPatchHandler { session: session.clone(), turn: turn.clone(), call_id: call_id.clone(), - network_approval_owner_id: None, tool_name: tool_name.to_string(), }; let out = orchestrator @@ -320,7 +319,6 @@ pub(crate) async fn intercept_apply_patch( session: session.clone(), turn: turn.clone(), call_id: call_id.to_string(), - network_approval_owner_id: None, tool_name: tool_name.to_string(), }; let out = orchestrator diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 14bf8717a6f..e6dfd8e0525 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -74,7 +74,7 @@ impl ShellHandler { capture_policy: ExecCapturePolicy::ShellTool, env: create_env(&turn_context.shell_environment_policy, Some(thread_id)), network: turn_context.network.clone(), - network_owner_id: None, + parent_tool_item_id: None, sandbox_permissions: params.sandbox_permissions.unwrap_or_default(), windows_sandbox_level: turn_context.windows_sandbox_level, windows_sandbox_private_desktop: turn_context @@ -130,7 +130,7 @@ impl ShellCommandHandler { capture_policy: ExecCapturePolicy::ShellTool, env: create_env(&turn_context.shell_environment_policy, Some(thread_id)), network: turn_context.network.clone(), - network_owner_id: None, + parent_tool_item_id: None, sandbox_permissions: params.sandbox_permissions.unwrap_or_default(), windows_sandbox_level: turn_context.windows_sandbox_level, windows_sandbox_private_desktop: turn_context @@ -476,7 +476,6 @@ impl ShellHandler { session: session.clone(), turn: turn.clone(), call_id: call_id.clone(), - network_approval_owner_id: None, tool_name, }; let out = orchestrator diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index 7c15a87f188..1cd53425d3e 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -1067,7 +1067,7 @@ impl JsReplManager { sandbox: sandbox_type, enforce_managed_network: has_managed_network_requirements, network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox_policy_cwd: &turn.cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index d2a36d70069..274c10ec501 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -28,7 +28,6 @@ use tokio::sync::Mutex; use tokio::sync::Notify; use tokio::sync::RwLock; use tracing::warn; -use uuid::Uuid; #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(crate) enum NetworkApprovalMode { @@ -44,18 +43,18 @@ pub(crate) struct NetworkApprovalSpec { #[derive(Clone, Debug)] pub(crate) struct DeferredNetworkApproval { - registration_id: String, + parent_tool_item_id: String, } impl DeferredNetworkApproval { - pub(crate) fn registration_id(&self) -> &str { - &self.registration_id + pub(crate) fn parent_tool_item_id(&self) -> &str { + &self.parent_tool_item_id } } #[derive(Debug)] pub(crate) struct ActiveNetworkApproval { - registration_id: Option, + parent_tool_item_id: Option, mode: NetworkApprovalMode, } @@ -64,14 +63,12 @@ impl ActiveNetworkApproval { self.mode } - pub(crate) fn owner_id(&self) -> Option<&str> { - self.registration_id.as_deref() - } - pub(crate) fn into_deferred(self) -> Option { - match (self.mode, self.registration_id) { - (NetworkApprovalMode::Deferred, Some(registration_id)) => { - Some(DeferredNetworkApproval { registration_id }) + match (self.mode, self.parent_tool_item_id) { + (NetworkApprovalMode::Deferred, Some(parent_tool_item_id)) => { + Some(DeferredNetworkApproval { + parent_tool_item_id, + }) } _ => None, } @@ -164,7 +161,6 @@ impl PendingHostApproval { } struct ActiveNetworkApprovalCall { - registration_id: String, turn_id: String, parent_tool_item_id: String, } @@ -199,38 +195,32 @@ impl NetworkApprovalService { other_approved_hosts.extend(approved_hosts.iter().cloned()); } - async fn register_call( - &self, - registration_id: String, - turn_id: String, - parent_tool_item_id: String, - ) { + async fn register_call(&self, turn_id: String, parent_tool_item_id: String) { let mut active_calls = self.active_calls.lock().await; - let key = registration_id.clone(); + let key = parent_tool_item_id.clone(); active_calls.insert( key, Arc::new(ActiveNetworkApprovalCall { - registration_id, turn_id, parent_tool_item_id, }), ); } - pub(crate) async fn unregister_call(&self, registration_id: &str) { + pub(crate) async fn unregister_call(&self, parent_tool_item_id: &str) { let mut active_calls = self.active_calls.lock().await; - active_calls.shift_remove(registration_id); + active_calls.shift_remove(parent_tool_item_id); let mut call_outcomes = self.call_outcomes.lock().await; - call_outcomes.remove(registration_id); + call_outcomes.remove(parent_tool_item_id); } - async fn resolve_owner_call( + async fn resolve_active_call( &self, - network_owner_id: Option<&str>, + parent_tool_item_id: Option<&str>, ) -> Option> { - if let Some(network_owner_id) = network_owner_id { + if let Some(parent_tool_item_id) = parent_tool_item_id { let active_calls = self.active_calls.lock().await; - return active_calls.get(network_owner_id).cloned(); + return active_calls.get(parent_tool_item_id).cloned(); } self.resolve_single_active_call().await @@ -259,32 +249,36 @@ impl NetworkApprovalService { (created, true) } - async fn record_outcome_for_owner_call( + async fn record_outcome_for_active_call( &self, - network_owner_id: Option<&str>, + parent_tool_item_id: Option<&str>, outcome: NetworkApprovalOutcome, ) { - let Some(owner_call) = self.resolve_owner_call(network_owner_id).await else { + let Some(active_call) = self.resolve_active_call(parent_tool_item_id).await else { return; }; - self.record_call_outcome(&owner_call.registration_id, outcome) + self.record_call_outcome(&active_call.parent_tool_item_id, outcome) .await; } - async fn take_call_outcome(&self, registration_id: &str) -> Option { + async fn take_call_outcome(&self, parent_tool_item_id: &str) -> Option { let mut call_outcomes = self.call_outcomes.lock().await; - call_outcomes.remove(registration_id) + call_outcomes.remove(parent_tool_item_id) } - async fn record_call_outcome(&self, registration_id: &str, outcome: NetworkApprovalOutcome) { + async fn record_call_outcome( + &self, + parent_tool_item_id: &str, + outcome: NetworkApprovalOutcome, + ) { let mut call_outcomes = self.call_outcomes.lock().await; if matches!( - call_outcomes.get(registration_id), + call_outcomes.get(parent_tool_item_id), Some(NetworkApprovalOutcome::DeniedByUser) ) { return; } - call_outcomes.insert(registration_id.to_string(), outcome); + call_outcomes.insert(parent_tool_item_id.to_string(), outcome); } pub(crate) async fn record_blocked_request(&self, blocked: BlockedRequest) { @@ -292,8 +286,8 @@ impl NetworkApprovalService { return; }; - self.record_outcome_for_owner_call( - blocked.network_owner_id.as_deref(), + self.record_outcome_for_active_call( + blocked.parent_tool_item_id.as_deref(), NetworkApprovalOutcome::DeniedByPolicy(message), ) .await; @@ -358,8 +352,8 @@ impl NetworkApprovalService { pending.set_decision(PendingApprovalDecision::Deny).await; let mut pending_approvals = self.pending_host_approvals.lock().await; pending_approvals.remove(&key); - self.record_outcome_for_owner_call( - request.network_owner_id.as_deref(), + self.record_outcome_for_active_call( + request.parent_tool_item_id.as_deref(), NetworkApprovalOutcome::DeniedByPolicy(policy_denial_message), ) .await; @@ -369,8 +363,8 @@ impl NetworkApprovalService { pending.set_decision(PendingApprovalDecision::Deny).await; let mut pending_approvals = self.pending_host_approvals.lock().await; pending_approvals.remove(&key); - self.record_outcome_for_owner_call( - request.network_owner_id.as_deref(), + self.record_outcome_for_active_call( + request.parent_tool_item_id.as_deref(), NetworkApprovalOutcome::DeniedByPolicy(policy_denial_message), ) .await; @@ -381,21 +375,21 @@ impl NetworkApprovalService { host: request.host.clone(), protocol, }; - let owner_call = self - .resolve_owner_call(request.network_owner_id.as_deref()) + let active_call = self + .resolve_active_call(request.parent_tool_item_id.as_deref()) .await; let approval_decision = if routes_approval_to_guardian(&turn_context) { // TODO(ccunningham): Attach guardian network reviews to the reviewed tool item - // lifecycle instead of this temporary standalone network approval id. + // lifecycle instead of this temporary standalone network review id. review_approval_request( &session, &turn_context, GuardianApprovalRequest::NetworkAccess { id: Self::approval_id_for_key(&key), - turn_id: owner_call + turn_id: active_call .as_ref() .map_or_else(|| turn_context.sub_id.clone(), |call| call.turn_id.clone()), - parent_tool_item_id: owner_call + parent_tool_item_id: active_call .as_ref() .map(|call| call.parent_tool_item_id.clone()), target, @@ -494,9 +488,9 @@ impl NetworkApprovalService { .await; } } - if let Some(owner_call) = owner_call.as_ref() { + if let Some(active_call) = active_call.as_ref() { self.record_call_outcome( - &owner_call.registration_id, + &active_call.parent_tool_item_id, NetworkApprovalOutcome::DeniedByUser, ) .await; @@ -507,18 +501,18 @@ impl NetworkApprovalService { }, ReviewDecision::Denied | ReviewDecision::Abort => { if routes_approval_to_guardian(&turn_context) { - if let Some(owner_call) = owner_call.as_ref() { + if let Some(active_call) = active_call.as_ref() { self.record_call_outcome( - &owner_call.registration_id, + &active_call.parent_tool_item_id, NetworkApprovalOutcome::DeniedByPolicy( GUARDIAN_REJECTION_MESSAGE.to_string(), ), ) .await; } - } else if let Some(owner_call) = owner_call.as_ref() { + } else if let Some(active_call) = active_call.as_ref() { self.record_call_outcome( - &owner_call.registration_id, + &active_call.parent_tool_item_id, NetworkApprovalOutcome::DeniedByUser, ) .await; @@ -594,19 +588,14 @@ pub(crate) async fn begin_network_approval( return None; } - let registration_id = Uuid::new_v4().to_string(); session .services .network_approval - .register_call( - registration_id.clone(), - turn_id.to_string(), - parent_tool_item_id.to_string(), - ) + .register_call(turn_id.to_string(), parent_tool_item_id.to_string()) .await; Some(ActiveNetworkApproval { - registration_id: Some(registration_id), + parent_tool_item_id: Some(parent_tool_item_id.to_string()), mode: spec.mode, }) } @@ -615,20 +604,20 @@ pub(crate) async fn finish_immediate_network_approval( session: &Session, active: ActiveNetworkApproval, ) -> Result<(), ToolError> { - let Some(registration_id) = active.registration_id.as_deref() else { + let Some(parent_tool_item_id) = active.parent_tool_item_id.as_deref() else { return Ok(()); }; let approval_outcome = session .services .network_approval - .take_call_outcome(registration_id) + .take_call_outcome(parent_tool_item_id) .await; session .services .network_approval - .unregister_call(registration_id) + .unregister_call(parent_tool_item_id) .await; match approval_outcome { @@ -650,7 +639,7 @@ pub(crate) async fn finish_deferred_network_approval( session .services .network_approval - .unregister_call(deferred.registration_id()) + .unregister_call(deferred.parent_tool_item_id()) .await; } diff --git a/codex-rs/core/src/tools/network_approval_tests.rs b/codex-rs/core/src/tools/network_approval_tests.rs index a3fc4d29097..fe2dd40b80c 100644 --- a/codex-rs/core/src/tools/network_approval_tests.rs +++ b/codex-rs/core/src/tools/network_approval_tests.rs @@ -180,14 +180,17 @@ fn only_never_policy_disables_network_approval_flow() { } fn denied_blocked_request(host: &str) -> BlockedRequest { - denied_blocked_request_with_owner(host, None) + denied_blocked_request_with_parent_tool_item(host, None) } -fn denied_blocked_request_with_owner(host: &str, network_owner_id: Option<&str>) -> BlockedRequest { +fn denied_blocked_request_with_parent_tool_item( + host: &str, + parent_tool_item_id: Option<&str>, +) -> BlockedRequest { BlockedRequest::new(BlockedRequestArgs { host: host.to_string(), reason: "not_allowed".to_string(), - network_owner_id: network_owner_id.map(ToString::to_string), + parent_tool_item_id: parent_tool_item_id.map(ToString::to_string), client: None, method: None, mode: None, @@ -199,14 +202,10 @@ fn denied_blocked_request_with_owner(host: &str, network_owner_id: Option<&str>) } #[tokio::test] -async fn record_blocked_request_sets_policy_outcome_for_owner_call() { +async fn record_blocked_request_sets_policy_outcome_for_active_call() { let service = NetworkApprovalService::default(); service - .register_call( - "registration-1".to_string(), - "turn-1".to_string(), - "command-1".to_string(), - ) + .register_call("turn-1".to_string(), "command-1".to_string()) .await; service @@ -214,7 +213,7 @@ async fn record_blocked_request_sets_policy_outcome_for_owner_call() { .await; assert_eq!( - service.take_call_outcome("registration-1").await, + service.take_call_outcome("command-1").await, Some(NetworkApprovalOutcome::DeniedByPolicy( "Network access to \"example.com\" was blocked: domain is not on the allowlist for the current sandbox mode.".to_string() )) @@ -225,22 +224,18 @@ 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(), - "command-1".to_string(), - ) + .register_call("turn-1".to_string(), "command-1".to_string()) .await; service - .record_call_outcome("registration-1", NetworkApprovalOutcome::DeniedByUser) + .record_call_outcome("command-1", NetworkApprovalOutcome::DeniedByUser) .await; service .record_blocked_request(denied_blocked_request("example.com")) .await; assert_eq!( - service.take_call_outcome("registration-1").await, + service.take_call_outcome("command-1").await, Some(NetworkApprovalOutcome::DeniedByUser) ); } @@ -249,84 +244,59 @@ 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(), - "command-1".to_string(), - ) + .register_call("turn-1".to_string(), "command-1".to_string()) .await; service - .register_call( - "registration-2".to_string(), - "turn-1".to_string(), - "command-2".to_string(), - ) + .register_call("turn-1".to_string(), "command-2".to_string()) .await; service .record_blocked_request(denied_blocked_request("example.com")) .await; - assert_eq!(service.take_call_outcome("registration-1").await, None); - assert_eq!(service.take_call_outcome("registration-2").await, None); + assert_eq!(service.take_call_outcome("command-1").await, None); + assert_eq!(service.take_call_outcome("command-2").await, None); } #[tokio::test] -async fn resolve_owner_call_uses_network_owner_id_when_multiple_calls_are_active() { +async fn resolve_active_call_uses_parent_tool_item_id_when_multiple_calls_are_active() { let service = NetworkApprovalService::default(); service - .register_call( - "registration-1".to_string(), - "turn-1".to_string(), - "command-1".to_string(), - ) + .register_call("turn-1".to_string(), "command-1".to_string()) .await; service - .register_call( - "registration-2".to_string(), - "turn-2".to_string(), - "command-2".to_string(), - ) + .register_call("turn-2".to_string(), "command-2".to_string()) .await; - let owner_call = service - .resolve_owner_call(Some("registration-2")) + let active_call = service + .resolve_active_call(Some("command-2")) .await - .expect("owner call should resolve"); + .expect("active call should resolve"); - assert_eq!(owner_call.registration_id, "registration-2"); - assert_eq!(owner_call.turn_id, "turn-2"); - assert_eq!(owner_call.parent_tool_item_id, "command-2"); + assert_eq!(active_call.turn_id, "turn-2"); + assert_eq!(active_call.parent_tool_item_id, "command-2"); } #[tokio::test] -async fn record_blocked_request_uses_network_owner_id_when_multiple_calls_are_active() { +async fn record_blocked_request_uses_parent_tool_item_id_when_multiple_calls_are_active() { let service = NetworkApprovalService::default(); service - .register_call( - "registration-1".to_string(), - "turn-1".to_string(), - "command-1".to_string(), - ) + .register_call("turn-1".to_string(), "command-1".to_string()) .await; service - .register_call( - "registration-2".to_string(), - "turn-2".to_string(), - "command-2".to_string(), - ) + .register_call("turn-2".to_string(), "command-2".to_string()) .await; service - .record_blocked_request(denied_blocked_request_with_owner( + .record_blocked_request(denied_blocked_request_with_parent_tool_item( "example.com", - Some("registration-2"), + Some("command-2"), )) .await; - assert_eq!(service.take_call_outcome("registration-1").await, None); + assert_eq!(service.take_call_outcome("command-1").await, None); assert_eq!( - service.take_call_outcome("registration-2").await, + service.take_call_outcome("command-2").await, Some(NetworkApprovalOutcome::DeniedByPolicy( "Network access to \"example.com\" was blocked: domain is not on the allowlist for the current sandbox mode.".to_string() )) diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 95a5e241db3..bae22f4f927 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -13,7 +13,6 @@ use crate::guardian::GUARDIAN_REJECTION_MESSAGE; use crate::guardian::routes_approval_to_guardian; use crate::network_policy_decision::network_approval_context_from_payload; use crate::sandboxing::SandboxManager; -use crate::tools::network_approval::ActiveNetworkApproval; use crate::tools::network_approval::DeferredNetworkApproval; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::begin_network_approval; @@ -66,19 +65,7 @@ impl ToolOrchestrator { tool.network_approval_spec(req, tool_ctx), ) .await; - let network_approval_owner_id = network_approval - .as_ref() - .and_then(ActiveNetworkApproval::owner_id) - .map(ToString::to_string); - - let attempt_tool_ctx = ToolCtx { - session: tool_ctx.session.clone(), - turn: tool_ctx.turn.clone(), - call_id: tool_ctx.call_id.clone(), - network_approval_owner_id, - tool_name: tool_ctx.tool_name.clone(), - }; - let run_result = tool.run(req, attempt, &attempt_tool_ctx).await; + let run_result = tool.run(req, attempt, tool_ctx).await; let Some(network_approval) = network_approval else { return (run_result, None); diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 151b5d06a33..0b73d14df59 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -208,7 +208,9 @@ impl ToolRuntime for ApplyPatchRuntime { ) -> Result { let spec = Self::build_command_spec(req, &ctx.turn.config.codex_home)?; let env = attempt - .env_for(spec, /*network*/ None, /*network_owner_id*/ None) + .env_for( + spec, /*network*/ None, /*parent_tool_item_id*/ None, + ) .map_err(|err| ToolError::Codex(err.into()))?; let out = execute_env(env, Self::stdout_stream(ctx)) .await diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 01f6493a607..34b65c1eb4a 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -253,11 +253,7 @@ impl ToolRuntime for ShellRuntime { req.justification.clone(), )?; let env = attempt - .env_for( - spec, - req.network.as_ref(), - ctx.network_approval_owner_id.as_deref(), - ) + .env_for(spec, req.network.as_ref(), Some(ctx.call_id.as_str())) .map_err(|err| ToolError::Codex(err.into()))?; let out = execute_env(env, Self::stdout_stream(ctx)) .await diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 4e8da53a0c4..ce48554b9cb 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -117,18 +117,14 @@ pub(super) async fn try_run_zsh_fork( req.justification.clone(), )?; let sandbox_exec_request = attempt - .env_for( - spec, - req.network.as_ref(), - ctx.network_approval_owner_id.as_deref(), - ) + .env_for(spec, req.network.as_ref(), Some(ctx.call_id.as_str())) .map_err(|err| ToolError::Codex(err.into()))?; let crate::sandboxing::ExecRequest { command, cwd: sandbox_cwd, env: sandbox_env, network: sandbox_network, - network_owner_id, + parent_tool_item_id, expiration: _sandbox_expiration, capture_policy: _capture_policy, sandbox, @@ -158,7 +154,7 @@ pub(super) async fn try_run_zsh_fork( sandbox, env: sandbox_env, network: sandbox_network, - network_owner_id, + parent_tool_item_id, windows_sandbox_level, sandbox_permissions, justification, @@ -265,7 +261,7 @@ pub(crate) async fn prepare_unified_exec_zsh_fork( sandbox: exec_request.sandbox, env: exec_request.env.clone(), network: exec_request.network.clone(), - network_owner_id: exec_request.network_owner_id.clone(), + parent_tool_item_id: exec_request.parent_tool_item_id.clone(), windows_sandbox_level: exec_request.windows_sandbox_level, sandbox_permissions: exec_request.sandbox_permissions, justification: exec_request.justification.clone(), @@ -863,7 +859,7 @@ struct CoreShellCommandExecutor { sandbox: SandboxType, env: HashMap, network: Option, - network_owner_id: Option, + parent_tool_item_id: Option, windows_sandbox_level: WindowsSandboxLevel, sandbox_permissions: SandboxPermissions, justification: Option, @@ -912,7 +908,7 @@ impl ShellCommandExecutor for CoreShellCommandExecutor { cwd: self.cwd.clone(), env: exec_env, network: self.network.clone(), - network_owner_id: self.network_owner_id.clone(), + parent_tool_item_id: self.parent_tool_item_id.clone(), expiration: ExecExpiration::Cancellation(cancel_rx), capture_policy: ExecCapturePolicy::ShellTool, sandbox: self.sandbox, @@ -1069,7 +1065,7 @@ impl CoreShellCommandExecutor { sandbox, enforce_managed_network: self.network.is_some(), network: self.network.as_ref(), - network_owner_id: self.network_owner_id.as_deref(), + parent_tool_item_id: self.parent_tool_item_id.as_deref(), sandbox_policy_cwd: &self.sandbox_policy_cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions, @@ -1079,9 +1075,9 @@ impl CoreShellCommandExecutor { windows_sandbox_private_desktop: false, })?; if let Some(network) = exec_request.network.as_ref() { - network.apply_to_env_for_owner( + network.apply_to_env_for_parent_tool_item( &mut exec_request.env, - exec_request.network_owner_id.as_deref(), + exec_request.parent_tool_item_id.as_deref(), ); } diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index ec26724af4d..68ef9148b36 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -655,7 +655,7 @@ async fn prepare_escalated_exec_turn_default_preserves_macos_seatbelt_extensions cwd: cwd.to_path_buf(), env: HashMap::new(), network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox: SandboxType::None, sandbox_policy: SandboxPolicy::new_read_only_policy(), file_system_sandbox_policy: read_only_file_system_sandbox_policy(), @@ -708,7 +708,7 @@ async fn prepare_escalated_exec_permissions_preserve_macos_seatbelt_extensions() cwd: cwd.to_path_buf(), env: HashMap::new(), network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox: SandboxType::None, sandbox_policy: SandboxPolicy::DangerFullAccess, file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), @@ -784,7 +784,7 @@ async fn prepare_escalated_exec_permission_profile_unions_turn_and_requested_mac cwd: cwd.to_path_buf(), env: HashMap::new(), network: None, - network_owner_id: None, + parent_tool_item_id: None, sandbox: SandboxType::None, sandbox_policy: sandbox_policy.clone(), file_system_sandbox_policy: read_only_file_system_sandbox_policy(), diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index b17f9c7dded..9205008d2a8 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -207,7 +207,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt let mut env = req.env.clone(); if let Some(network) = req.network.as_ref() { - network.apply_to_env_for_owner(&mut env, ctx.network_approval_owner_id.as_deref()); + network.apply_to_env_for_parent_tool_item(&mut env, Some(ctx.call_id.as_str())); } if let UnifiedExecShellMode::ZshFork(zsh_fork_config) = &self.shell_mode { let spec = build_command_spec( @@ -221,11 +221,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt ) .map_err(|_| ToolError::Rejected("missing command line for PTY".to_string()))?; let exec_env = attempt - .env_for( - spec, - req.network.as_ref(), - ctx.network_approval_owner_id.as_deref(), - ) + .env_for(spec, req.network.as_ref(), Some(ctx.call_id.as_str())) .map_err(|err| ToolError::Codex(err.into()))?; match zsh_fork_backend::maybe_prepare_unified_exec( req, @@ -273,11 +269,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt ) .map_err(|_| ToolError::Rejected("missing command line for PTY".to_string()))?; let exec_env = attempt - .env_for( - spec, - req.network.as_ref(), - ctx.network_approval_owner_id.as_deref(), - ) + .env_for(spec, req.network.as_ref(), Some(ctx.call_id.as_str())) .map_err(|err| ToolError::Codex(err.into()))?; self.manager .open_session_with_exec_env(&exec_env, req.tty, Box::new(NoopSpawnLifecycle)) diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 3251f833022..e894b4b202c 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -300,7 +300,6 @@ pub(crate) struct ToolCtx { pub session: Arc, pub turn: Arc, pub call_id: String, - pub network_approval_owner_id: Option, pub tool_name: String, } @@ -342,7 +341,7 @@ impl<'a> SandboxAttempt<'a> { &self, spec: CommandSpec, network: Option<&NetworkProxy>, - network_owner_id: Option<&str>, + parent_tool_item_id: Option<&str>, ) -> Result { self.manager .transform(crate::sandboxing::SandboxTransformRequest { @@ -353,7 +352,7 @@ impl<'a> SandboxAttempt<'a> { sandbox: self.sandbox, enforce_managed_network: self.enforce_managed_network, network, - network_owner_id, + parent_tool_item_id, sandbox_policy_cwd: self.sandbox_cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 0ea2e653e2b..79842aad267 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -198,7 +198,7 @@ impl UnifiedExecProcessManager { if process_started_alive { let network_approval_id = deferred_network_approval .as_ref() - .map(|deferred| deferred.registration_id().to_string()); + .map(|deferred| deferred.parent_tool_item_id().to_string()); self.store_process( Arc::clone(&process), context, @@ -627,7 +627,6 @@ impl UnifiedExecProcessManager { session: context.session.clone(), turn: context.turn.clone(), call_id: context.call_id.clone(), - network_approval_owner_id: None, tool_name: "exec_command".to_string(), }; orchestrator diff --git a/codex-rs/core/tests/suite/exec.rs b/codex-rs/core/tests/suite/exec.rs index 22b19c71b76..be67274e9fb 100644 --- a/codex-rs/core/tests/suite/exec.rs +++ b/codex-rs/core/tests/suite/exec.rs @@ -41,7 +41,7 @@ async fn run_test_cmd(tmp: TempDir, cmd: Vec<&str>) -> Result(req: &Request) -> Option { +fn proxy_authorization_parent_tool_item_id(req: &Request) -> Option { req.headers() .typed_get::>() .map(|header| header.0.username().to_string()) @@ -181,7 +181,7 @@ async fn http_connect_accept( } let client = client_addr(&req); - let network_owner_id = proxy_authorization_network_owner_id(&req); + let parent_tool_item_id = proxy_authorization_parent_tool_item_id(&req); let enabled = app_state .enabled() .await @@ -194,7 +194,7 @@ async fn http_connect_accept( host, authority.port, client_addr(&req), - network_owner_id, + parent_tool_item_id, Some("CONNECT".to_string()), NetworkProtocol::HttpsConnect, /*audit_endpoint_override*/ None, @@ -206,7 +206,7 @@ async fn http_connect_accept( protocol: NetworkProtocol::HttpsConnect, host: host.clone(), port: authority.port, - network_owner_id: network_owner_id.clone(), + parent_tool_item_id: parent_tool_item_id.clone(), client_addr: client.clone(), method: Some("CONNECT".to_string()), command: None, @@ -231,7 +231,7 @@ async fn http_connect_accept( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: reason.clone(), - network_owner_id, + parent_tool_item_id, client: client.clone(), method: Some("CONNECT".to_string()), mode: None, @@ -295,7 +295,7 @@ async fn http_connect_accept( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_MITM_REQUIRED.to_string(), - network_owner_id: None, + parent_tool_item_id: None, client: client.clone(), method: Some("CONNECT".to_string()), mode: Some(NetworkMode::Limited), @@ -445,7 +445,7 @@ async fn http_plain_proxy( } }; let client = client_addr(&req); - let network_owner_id = proxy_authorization_network_owner_id(&req); + let parent_tool_item_id = proxy_authorization_parent_tool_item_id(&req); let method_allowed = match app_state .method_allowed(req.method().as_str()) .await @@ -485,7 +485,7 @@ async fn http_plain_proxy( socket_path, /*port*/ 0, client_addr(&req), - network_owner_id.clone(), + parent_tool_item_id.clone(), Some(req.method().as_str().to_string()), NetworkProtocol::Http, Some(("unix-socket", 0)), @@ -631,7 +631,7 @@ async fn http_plain_proxy( host, port, client_addr(&req), - network_owner_id.clone(), + parent_tool_item_id.clone(), Some(req.method().as_str().to_string()), NetworkProtocol::Http, /*audit_endpoint_override*/ None, @@ -643,7 +643,7 @@ async fn http_plain_proxy( protocol: NetworkProtocol::Http, host: host.clone(), port, - network_owner_id: network_owner_id.clone(), + parent_tool_item_id: parent_tool_item_id.clone(), client_addr: client.clone(), method: Some(req.method().as_str().to_string()), command: None, @@ -668,7 +668,7 @@ async fn http_plain_proxy( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: reason.clone(), - network_owner_id: network_owner_id.clone(), + parent_tool_item_id: parent_tool_item_id.clone(), client: client.clone(), method: Some(req.method().as_str().to_string()), mode: None, @@ -714,7 +714,7 @@ async fn http_plain_proxy( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_METHOD_NOT_ALLOWED.to_string(), - network_owner_id, + parent_tool_item_id, client: client.clone(), method: Some(req.method().as_str().to_string()), mode: Some(NetworkMode::Limited), @@ -908,7 +908,7 @@ async fn proxy_disabled_response( host: String, port: u16, client: Option, - network_owner_id: Option, + parent_tool_item_id: Option, method: Option, protocol: NetworkProtocol, audit_endpoint_override: Option<(&str, u16)>, @@ -933,7 +933,7 @@ async fn proxy_disabled_response( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: blocked_host, reason: REASON_PROXY_DISABLED.to_string(), - network_owner_id, + parent_tool_item_id, client, method, mode: None, diff --git a/codex-rs/network-proxy/src/mitm.rs b/codex-rs/network-proxy/src/mitm.rs index 5b37e1207c0..f443d80f2d2 100644 --- a/codex-rs/network-proxy/src/mitm.rs +++ b/codex-rs/network-proxy/src/mitm.rs @@ -289,7 +289,7 @@ async fn mitm_blocking_response( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: policy.target_host.clone(), reason: reason.to_string(), - network_owner_id: None, + parent_tool_item_id: None, client: client.clone(), method: Some(method.clone()), mode: Some(policy.mode), @@ -312,7 +312,7 @@ async fn mitm_blocking_response( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: policy.target_host.clone(), reason: REASON_METHOD_NOT_ALLOWED.to_string(), - network_owner_id: None, + parent_tool_item_id: None, client: client.clone(), method: Some(method.clone()), mode: Some(policy.mode), diff --git a/codex-rs/network-proxy/src/network_policy.rs b/codex-rs/network-proxy/src/network_policy.rs index 0aadc10bfda..1f505ffa3ef 100644 --- a/codex-rs/network-proxy/src/network_policy.rs +++ b/codex-rs/network-proxy/src/network_policy.rs @@ -79,7 +79,10 @@ pub struct NetworkPolicyRequest { pub protocol: NetworkProtocol, pub host: String, pub port: u16, - pub network_owner_id: Option, + /// Parent tool item id forwarded from the originating tool call. Core uses + /// this to resolve blocked requests back to the exact active call instead + /// of guessing by session state. + pub parent_tool_item_id: Option, pub client_addr: Option, pub method: Option, pub command: Option, @@ -90,7 +93,9 @@ pub struct NetworkPolicyRequestArgs { pub protocol: NetworkProtocol, pub host: String, pub port: u16, - pub network_owner_id: Option, + /// Parent tool item id forwarded from the originating tool call for later + /// attribution. + pub parent_tool_item_id: Option, pub client_addr: Option, pub method: Option, pub command: Option, @@ -103,7 +108,7 @@ impl NetworkPolicyRequest { protocol, host, port, - network_owner_id, + parent_tool_item_id, client_addr, method, command, @@ -113,7 +118,7 @@ impl NetworkPolicyRequest { protocol, host, port, - network_owner_id, + parent_tool_item_id, client_addr, method, command, @@ -629,7 +634,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "example.com".to_string(), port: 80, - network_owner_id: None, + parent_tool_item_id: None, client_addr: None, method: None, command: None, @@ -690,7 +695,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "blocked.com".to_string(), port: 80, - network_owner_id: None, + parent_tool_item_id: None, client_addr: Some("127.0.0.1:1234".to_string()), method: Some("GET".to_string()), command: None, @@ -732,7 +737,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "example.com".to_string(), port: 80, - network_owner_id: None, + parent_tool_item_id: None, client_addr: None, method: Some("GET".to_string()), command: None, @@ -783,7 +788,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "example.com".to_string(), port: 80, - network_owner_id: None, + parent_tool_item_id: None, client_addr: None, method: Some("GET".to_string()), command: None, @@ -867,7 +872,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "127.0.0.1".to_string(), port: 80, - network_owner_id: None, + parent_tool_item_id: None, client_addr: None, method: Some("GET".to_string()), command: None, diff --git a/codex-rs/network-proxy/src/owner_identity.rs b/codex-rs/network-proxy/src/owner_identity.rs index 3c898ec1d2a..6fbae7d1cd6 100644 --- a/codex-rs/network-proxy/src/owner_identity.rs +++ b/codex-rs/network-proxy/src/owner_identity.rs @@ -7,16 +7,16 @@ use rama_net::user::authority::AuthorizeResult; use rama_net::user::authority::Authorizer; #[derive(Clone, Copy, Debug, Default)] -pub(crate) struct ProxyOwnerAuthorizer; +pub(crate) struct ProxyParentToolItemAuthorizer; -impl AuthoritySync for ProxyOwnerAuthorizer { +impl AuthoritySync for ProxyParentToolItemAuthorizer { fn authorized(&self, ext: &mut Extensions, credentials: &Basic) -> bool { ext.insert(UserId::Username(credentials.username().to_string())); true } } -impl Authorizer for ProxyOwnerAuthorizer { +impl Authorizer for ProxyParentToolItemAuthorizer { type Error = std::convert::Infallible; async fn authorize(&self, credentials: Basic) -> AuthorizeResult { @@ -29,7 +29,10 @@ impl Authorizer for ProxyOwnerAuthorizer { } } -pub(crate) fn network_owner_id(input: &T) -> Option { +/// Extract the parent tool item id from proxy auth/user extensions. Managed +/// proxy callbacks carry the parent tool item id as the proxy auth username so +/// core can attribute blocked requests back to the originating tool call. +pub(crate) fn extract_parent_tool_item_id(input: &T) -> Option { match input.extensions().get::() { Some(UserId::Username(username)) => Some(username.clone()), Some(UserId::Token(token)) => String::from_utf8(token.clone()).ok(), diff --git a/codex-rs/network-proxy/src/proxy.rs b/codex-rs/network-proxy/src/proxy.rs index 8cf281d27f1..95b3c97a17f 100644 --- a/codex-rs/network-proxy/src/proxy.rs +++ b/codex-rs/network-proxy/src/proxy.rs @@ -15,6 +15,7 @@ use std::sync::Arc; use std::sync::Mutex; use tokio::task::JoinHandle; use tracing::warn; +use url::Url; #[derive(Debug, Clone, Parser)] #[command(name = "codex-network-proxy", about = "Codex network sandbox proxy")] @@ -305,11 +306,22 @@ fn set_env_keys(env: &mut HashMap, keys: &[&str], value: &str) { } } -fn proxy_url(scheme: &str, addr: SocketAddr, network_owner_id: Option<&str>) -> String { - match network_owner_id { - Some(network_owner_id) => format!("{scheme}://{network_owner_id}@{addr}"), - None => format!("{scheme}://{addr}"), +fn proxy_url(scheme: &str, addr: SocketAddr, parent_tool_item_id: Option<&str>) -> String { + let base = format!("{scheme}://{addr}"); + let mut url = match Url::parse(&base) { + Ok(url) => url, + Err(err) => panic!("failed to build proxy URL for {base}: {err}"), + }; + if let Some(parent_tool_item_id) = parent_tool_item_id + && let Err(()) = url.set_username(parent_tool_item_id) + { + panic!("failed to encode parent tool item id in proxy URL"); + } + let mut proxy_url = url.to_string(); + if proxy_url.ends_with('/') { + proxy_url.pop(); } + proxy_url } fn apply_proxy_env_overrides( @@ -318,10 +330,10 @@ fn apply_proxy_env_overrides( socks_addr: SocketAddr, socks_enabled: bool, allow_local_binding: bool, - network_owner_id: Option<&str>, + parent_tool_item_id: Option<&str>, ) { - let http_proxy_url = proxy_url("http", http_addr, network_owner_id); - let socks_proxy_url = proxy_url("socks5h", socks_addr, network_owner_id); + let http_proxy_url = proxy_url("http", http_addr, parent_tool_item_id); + let socks_proxy_url = proxy_url("socks5h", socks_addr, parent_tool_item_id); env.insert( ALLOW_LOCAL_BINDING_ENV_KEY.to_string(), if allow_local_binding { @@ -422,13 +434,15 @@ impl NetworkProxy { } pub fn apply_to_env(&self, env: &mut HashMap) { - self.apply_to_env_for_owner(env, /*network_owner_id*/ None); + self.apply_to_env_for_parent_tool_item(env, /*parent_tool_item_id*/ None); } - pub fn apply_to_env_for_owner( + /// Apply managed proxy environment variables, optionally tagging them with + /// the originating parent tool item id for blocked-request attribution. + pub fn apply_to_env_for_parent_tool_item( &self, env: &mut HashMap, - network_owner_id: Option<&str>, + parent_tool_item_id: Option<&str>, ) { // Enforce proxying for child processes. We intentionally override existing values so // command-level environment cannot bypass the managed proxy endpoint. @@ -438,7 +452,7 @@ impl NetworkProxy { self.socks_addr, self.socks_enabled, self.allow_local_binding, - network_owner_id, + parent_tool_item_id, ); } @@ -713,7 +727,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, - /*network_owner_id*/ None, + /*parent_tool_item_id*/ None, ); assert_eq!( @@ -764,7 +778,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), false, true, - /*network_owner_id*/ None, + /*parent_tool_item_id*/ None, ); assert_eq!( @@ -783,7 +797,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, - /*network_owner_id*/ None, + /*parent_tool_item_id*/ None, ); assert_eq!( @@ -829,7 +843,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, - /*network_owner_id*/ None, + /*parent_tool_item_id*/ None, ); assert_eq!( diff --git a/codex-rs/network-proxy/src/runtime.rs b/codex-rs/network-proxy/src/runtime.rs index d51729aed89..d3e58993664 100644 --- a/codex-rs/network-proxy/src/runtime.rs +++ b/codex-rs/network-proxy/src/runtime.rs @@ -84,8 +84,11 @@ pub enum HostBlockDecision { pub struct BlockedRequest { pub host: String, pub reason: String, + /// Parent tool item id carried alongside the blocked request for + /// attribution in core. This is internal-only and is intentionally omitted + /// from serialized logs/snapshots. #[serde(skip_serializing)] - pub network_owner_id: Option, + pub parent_tool_item_id: Option, pub client: Option, pub method: Option, pub mode: Option, @@ -102,7 +105,9 @@ pub struct BlockedRequest { pub struct BlockedRequestArgs { pub host: String, pub reason: String, - pub network_owner_id: Option, + /// Parent tool item id carried alongside the blocked request for + /// attribution in core. + pub parent_tool_item_id: Option, pub client: Option, pub method: Option, pub mode: Option, @@ -117,7 +122,7 @@ impl BlockedRequest { let BlockedRequestArgs { host, reason, - network_owner_id, + parent_tool_item_id, client, method, mode, @@ -129,7 +134,7 @@ impl BlockedRequest { Self { host, reason, - network_owner_id, + parent_tool_item_id, client, method, mode, @@ -999,7 +1004,7 @@ mod tests { .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: "google.com".to_string(), reason: "not_allowed".to_string(), - network_owner_id: None, + parent_tool_item_id: None, client: None, method: Some("GET".to_string()), mode: None, @@ -1040,7 +1045,7 @@ mod tests { .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: format!("example{idx}.com"), reason: "not_allowed".to_string(), - network_owner_id: None, + parent_tool_item_id: None, client: None, method: Some("GET".to_string()), mode: None, @@ -1063,7 +1068,7 @@ mod tests { let entry = BlockedRequest { host: "google.com".to_string(), reason: "not_allowed".to_string(), - network_owner_id: None, + parent_tool_item_id: None, client: Some("127.0.0.1".to_string()), method: Some("GET".to_string()), mode: Some(NetworkMode::Full), diff --git a/codex-rs/network-proxy/src/socks5.rs b/codex-rs/network-proxy/src/socks5.rs index 16b4f653aed..d31cbe77278 100644 --- a/codex-rs/network-proxy/src/socks5.rs +++ b/codex-rs/network-proxy/src/socks5.rs @@ -9,8 +9,8 @@ use crate::network_policy::NetworkPolicyRequestArgs; use crate::network_policy::NetworkProtocol; use crate::network_policy::emit_block_decision_audit_event; use crate::network_policy::evaluate_host_policy; -use crate::owner_identity::ProxyOwnerAuthorizer; -use crate::owner_identity::network_owner_id as extract_network_owner_id; +use crate::owner_identity::ProxyParentToolItemAuthorizer; +use crate::owner_identity::extract_parent_tool_item_id; use crate::policy::normalize_host; use crate::reasons::REASON_METHOD_NOT_ALLOWED; use crate::reasons::REASON_PROXY_DISABLED; @@ -111,7 +111,7 @@ async fn run_socks5_with_listener( base.set_auth_optional(true); let base = base .with_connector(socks_connector) - .with_authorizer(ProxyOwnerAuthorizer); + .with_authorizer(ProxyParentToolItemAuthorizer); if enable_socks5_udp { let udp_state = state.clone(); @@ -156,7 +156,7 @@ async fn handle_socks5_tcp( .extensions() .get::() .map(|info| info.peer_addr().to_string()); - let network_owner_id = extract_network_owner_id(&req); + let parent_tool_item_id = extract_parent_tool_item_id(&req); match app_state.enabled().await { Ok(true) => {} @@ -182,7 +182,7 @@ async fn handle_socks5_tcp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_PROXY_DISABLED.to_string(), - network_owner_id: network_owner_id.clone(), + parent_tool_item_id: parent_tool_item_id.clone(), client: client.clone(), method: None, mode: None, @@ -225,7 +225,7 @@ async fn handle_socks5_tcp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_METHOD_NOT_ALLOWED.to_string(), - network_owner_id: network_owner_id.clone(), + parent_tool_item_id: parent_tool_item_id.clone(), client: client.clone(), method: None, mode: Some(NetworkMode::Limited), @@ -252,7 +252,7 @@ async fn handle_socks5_tcp( protocol: NetworkProtocol::Socks5Tcp, host: host.clone(), port, - network_owner_id: network_owner_id.clone(), + parent_tool_item_id: parent_tool_item_id.clone(), client_addr: client.clone(), method: None, command: None, @@ -277,7 +277,7 @@ async fn handle_socks5_tcp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: reason.clone(), - network_owner_id, + parent_tool_item_id, client: client.clone(), method: None, mode: None, @@ -325,7 +325,7 @@ async fn inspect_socks5_udp( let client = extensions .get::() .map(|info| info.peer_addr().to_string()); - let network_owner_id = extract_network_owner_id(&extensions); + let parent_tool_item_id = extract_parent_tool_item_id(&extensions); match state.enabled().await { Ok(true) => {} @@ -351,7 +351,7 @@ async fn inspect_socks5_udp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_PROXY_DISABLED.to_string(), - network_owner_id: network_owner_id.clone(), + parent_tool_item_id: parent_tool_item_id.clone(), client: client.clone(), method: None, mode: None, @@ -394,7 +394,7 @@ async fn inspect_socks5_udp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_METHOD_NOT_ALLOWED.to_string(), - network_owner_id: network_owner_id.clone(), + parent_tool_item_id: parent_tool_item_id.clone(), client: client.clone(), method: None, mode: Some(NetworkMode::Limited), @@ -417,7 +417,7 @@ async fn inspect_socks5_udp( protocol: NetworkProtocol::Socks5Udp, host: host.clone(), port, - network_owner_id: network_owner_id.clone(), + parent_tool_item_id: parent_tool_item_id.clone(), client_addr: client.clone(), method: None, command: None, @@ -442,7 +442,7 @@ async fn inspect_socks5_udp( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: reason.clone(), - network_owner_id, + parent_tool_item_id, client: client.clone(), method: None, mode: None, From 95e86180e5895cfab1a624dce6fed50c0f0cc15f Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Mon, 23 Mar 2026 17:51:36 -0700 Subject: [PATCH 12/12] network-proxy: reduce proxy disabled args Bundle proxy-disabled response parameters into a single typed args struct so the callsites stay readable and clippy stops flagging the helper for too many arguments. Co-authored-by: Codex --- codex-rs/network-proxy/src/http_proxy.rs | 67 +++++++++++++++--------- 1 file changed, 43 insertions(+), 24 deletions(-) diff --git a/codex-rs/network-proxy/src/http_proxy.rs b/codex-rs/network-proxy/src/http_proxy.rs index 31ba3923f71..9b7c3c13617 100644 --- a/codex-rs/network-proxy/src/http_proxy.rs +++ b/codex-rs/network-proxy/src/http_proxy.rs @@ -191,13 +191,15 @@ async fn http_connect_accept( warn!("CONNECT blocked; proxy disabled (client={client}, host={host})"); return Err(proxy_disabled_response( &app_state, - host, - authority.port, - client_addr(&req), - parent_tool_item_id, - Some("CONNECT".to_string()), - NetworkProtocol::HttpsConnect, - /*audit_endpoint_override*/ None, + ProxyDisabledResponseArgs { + host, + port: authority.port, + client: client_addr(&req), + parent_tool_item_id, + method: Some("CONNECT".to_string()), + protocol: NetworkProtocol::HttpsConnect, + audit_endpoint_override: None, + }, ) .await); } @@ -482,13 +484,15 @@ async fn http_plain_proxy( warn!("unix socket blocked; proxy disabled (client={client}, path={socket_path})"); return Ok(proxy_disabled_response( &app_state, - socket_path, - /*port*/ 0, - client_addr(&req), - parent_tool_item_id.clone(), - Some(req.method().as_str().to_string()), - NetworkProtocol::Http, - Some(("unix-socket", 0)), + ProxyDisabledResponseArgs { + host: socket_path, + port: 0, + client: client_addr(&req), + parent_tool_item_id: parent_tool_item_id.clone(), + method: Some(req.method().as_str().to_string()), + protocol: NetworkProtocol::Http, + audit_endpoint_override: Some(("unix-socket", 0)), + }, ) .await); } @@ -628,13 +632,15 @@ async fn http_plain_proxy( warn!("request blocked; proxy disabled (client={client}, host={host}, method={method})"); return Ok(proxy_disabled_response( &app_state, - host, - port, - client_addr(&req), - parent_tool_item_id.clone(), - Some(req.method().as_str().to_string()), - NetworkProtocol::Http, - /*audit_endpoint_override*/ None, + ProxyDisabledResponseArgs { + host, + port, + client: client_addr(&req), + parent_tool_item_id: parent_tool_item_id.clone(), + method: Some(req.method().as_str().to_string()), + protocol: NetworkProtocol::Http, + audit_endpoint_override: None, + }, ) .await); } @@ -903,16 +909,29 @@ fn blocked_text_with_details(reason: &str, details: &PolicyDecisionDetails<'_>) blocked_text_response_with_policy(reason, details) } -async fn proxy_disabled_response( - app_state: &NetworkProxyState, +struct ProxyDisabledResponseArgs { host: String, port: u16, client: Option, parent_tool_item_id: Option, method: Option, protocol: NetworkProtocol, - audit_endpoint_override: Option<(&str, u16)>, + audit_endpoint_override: Option<(&'static str, u16)>, +} + +async fn proxy_disabled_response( + app_state: &NetworkProxyState, + args: ProxyDisabledResponseArgs, ) -> Response { + let ProxyDisabledResponseArgs { + host, + port, + client, + parent_tool_item_id, + method, + protocol, + audit_endpoint_override, + } = args; let (audit_server_address, audit_server_port) = audit_endpoint_override.unwrap_or((host.as_str(), port)); emit_http_block_decision_audit_event(