diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 5b06ab539c9..021a26bed03 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -1349,13 +1349,30 @@ "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": { + "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" + ] + }, "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": { @@ -1374,13 +1391,30 @@ "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": { + "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" + ] + }, "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 c24c8ac2493..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 @@ -8196,13 +8196,30 @@ }, "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": { + "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" + ] + }, "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": { @@ -8223,13 +8240,30 @@ }, "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": { + "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" + ] + }, "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 c479da94e4f..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 @@ -4944,13 +4944,30 @@ }, "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": { + "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" + ] + }, "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": { @@ -4971,13 +4988,30 @@ }, "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": { + "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" + ] + }, "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 df96e86d164..b76741b2139 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json @@ -57,13 +57,30 @@ "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": { + "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" + ] + }, "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 339396a50b4..043cd1efdce 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json @@ -57,13 +57,30 @@ "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": { + "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" + ] + }, "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 ac4ae1b78a1..dc2261818cd 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,24 @@ 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, 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, +/** + * 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 b229626817e..76b8f34f69e 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,24 @@ 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, 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, +/** + * 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 57017833a6a..8e15590bf1a 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,16 @@ 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, + /// 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, pub action: Option, } @@ -4847,7 +4856,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 @@ -4855,7 +4864,16 @@ 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, + /// 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, pub action: Option, } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 3248a444240..f64e2177e71 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, 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. 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 34640a50cf4..c2d5e1e2b17 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -224,13 +224,18 @@ 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 review_id = assessment.id.clone(); + let target_item_id = review_id.clone(); match assessment.status { codex_protocol::protocol::GuardianAssessmentStatus::InProgress => { ServerNotification::ItemGuardianApprovalReviewStarted( ItemGuardianApprovalReviewStartedNotification { thread_id: conversation_id.to_string(), turn_id, - target_item_id: assessment.id.clone(), + target_item_id, + review_id: Some(review_id), + parent_tool_item_id, review, action: assessment.action.clone(), }, @@ -243,7 +248,9 @@ 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_id: Some(review_id), + parent_tool_item_id, review, action: assessment.action.clone(), }, @@ -2918,6 +2925,7 @@ mod tests { "turn-from-event", &GuardianAssessmentEvent { id: "item-1".to_string(), + parent_tool_item_id: Some("item-1".to_string()), turn_id: String::new(), status: codex_protocol::protocol::GuardianAssessmentStatus::InProgress, risk_score: None, @@ -2932,6 +2940,8 @@ 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.as_deref(), Some("item-1")); assert_eq!( payload.review.status, GuardianApprovalReviewStatus::InProgress @@ -2957,6 +2967,7 @@ mod tests { "turn-from-event", &GuardianAssessmentEvent { id: "item-2".to_string(), + 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), @@ -2971,6 +2982,8 @@ 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.as_deref(), Some("item-2")); assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Denied); assert_eq!(payload.review.risk_score, Some(91)); assert_eq!( @@ -2985,7 +2998,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", @@ -2995,7 +3008,8 @@ mod tests { &conversation_id, "turn-from-event", &GuardianAssessmentEvent { - id: "item-3".to_string(), + 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, @@ -3009,7 +3023,48 @@ mod tests { ServerNotification::ItemGuardianApprovalReviewCompleted(payload) => { assert_eq!(payload.thread_id, conversation_id.to_string()); assert_eq!(payload.turn_id, "turn-from-assessment"); - assert_eq!(payload.target_item_id, "item-3"); + assert_eq!(payload.target_item_id, "guardian-3"); + assert_eq!(payload.review_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); + assert_eq!(payload.review.risk_level, None); + assert_eq!(payload.review.rationale, None); + assert_eq!(payload.action, Some(action)); + } + other => panic!("unexpected notification: {other:?}"), + } + } + + #[test] + fn guardian_assessment_aborted_keeps_guardian_id_when_network_parent_is_missing() { + let conversation_id = ThreadId::new(); + let action = json!({ + "tool": "network_access", + "target": "api.openai.com:443", + }); + let notification = guardian_auto_approval_review_notification( + &conversation_id, + "turn-from-event", + &GuardianAssessmentEvent { + id: "guardian-4".to_string(), + parent_tool_item_id: None, + turn_id: "turn-from-assessment".to_string(), + status: codex_protocol::protocol::GuardianAssessmentStatus::Aborted, + risk_score: None, + risk_level: None, + rationale: None, + action: Some(action.clone()), + }, + ); + + match notification { + ServerNotification::ItemGuardianApprovalReviewCompleted(payload) => { + assert_eq!(payload.thread_id, conversation_id.to_string()); + assert_eq!(payload.turn_id, "turn-from-assessment"); + assert_eq!(payload.target_item_id, "guardian-4"); + assert_eq!(payload.review_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); assert_eq!(payload.review.risk_level, None); diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 1b02e4bb6cf..e6c4811bcee 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), + 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 e1a9cb3def1..99200b7e473 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, + parent_tool_item_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, + 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_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 8201424d8e5..73e44cdedd9 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: 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/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index c7a715cd93a..9f8103e73d8 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, + parent_tool_item_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, + 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 af0fccc9ac2..17a68b033ef 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, + 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 9be0518fa07..6d408e4a0b7 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -81,6 +81,9 @@ pub struct ExecParams { pub capture_policy: ExecCapturePolicy, pub env: HashMap, pub network: 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, @@ -263,6 +266,7 @@ pub fn build_exec_request( expiration, capture_policy, network, + parent_tool_item_id, sandbox_permissions, windows_sandbox_level, windows_sandbox_private_desktop, @@ -270,7 +274,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_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( @@ -301,6 +305,7 @@ pub fn build_exec_request( sandbox: sandbox_type, enforce_managed_network, network: network.as_ref(), + parent_tool_item_id: parent_tool_item_id.as_deref(), sandbox_policy_cwd: sandbox_cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions: None, @@ -324,6 +329,7 @@ pub(crate) async fn execute_exec_request( cwd, env, network, + parent_tool_item_id, expiration, capture_policy, sandbox, @@ -345,6 +351,7 @@ pub(crate) async fn execute_exec_request( capture_policy, env, network: network.clone(), + parent_tool_item_id, sandbox_permissions, windows_sandbox_level, windows_sandbox_private_desktop, @@ -448,6 +455,7 @@ async fn exec_windows_sandbox( cwd, mut env, network, + parent_tool_item_id, expiration, capture_policy, windows_sandbox_level, @@ -455,7 +463,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_parent_tool_item(&mut env, parent_tool_item_id.as_deref()); } // TODO(iceweasel-oai): run_windows_sandbox_capture should support all @@ -838,6 +846,7 @@ async fn exec( cwd, mut env, network, + parent_tool_item_id, arg0, expiration, capture_policy, @@ -845,7 +854,7 @@ async fn exec( .. } = params; if let Some(network) = network.as_ref() { - network.apply_to_env(&mut env); + 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 fc312ec88e3..703c357761b 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, + parent_tool_item_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, + parent_tool_item_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, + parent_tool_item_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, + parent_tool_item_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, + 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/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 533c9ee11e9..817ac31ca18 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,33 @@ pub(crate) fn guardian_approval_request_to_json( GuardianApprovalRequest::NetworkAccess { id: _, turn_id: _, + parent_tool_item_id, target, host, protocol, port, - } => Ok(serde_json::json!({ - "tool": "network_access", - "target": target, - "host": host, - "protocol": protocol, - "port": port, - })), + } => { + let mut action = serde_json::Map::from_iter([ + ( + "tool".to_string(), + Value::String("network_access".to_string()), + ), + ("target".to_string(), Value::String(target.clone())), + ("host".to_string(), Value::String(host.clone())), + ( + "protocol".to_string(), + network_approval_protocol_value(*protocol), + ), + ("port".to_string(), Value::from(*port)), + ]); + if let Some(parent_tool_item_id) = parent_tool_item_id { + action.insert( + "parent_tool_item_id".to_string(), + Value::String(parent_tool_item_id.clone()), + ); + } + Ok(Value::Object(action)) + } GuardianApprovalRequest::McpToolCall { id: _, server, @@ -324,11 +341,12 @@ pub(crate) fn guardian_assessment_action_value(action: &GuardianApprovalRequest) host, protocol, port, + .. } => serde_json::json!({ "tool": "network_access", "target": target, "host": host, - "protocol": protocol, + "protocol": network_approval_protocol_value(*protocol), "port": port, }), GuardianApprovalRequest::McpToolCall { @@ -341,6 +359,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, .. } @@ -368,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 { id, .. } + | GuardianApprovalRequest::ExecCommand { id, .. } + | GuardianApprovalRequest::ApplyPatch { id, .. } + | GuardianApprovalRequest::McpToolCall { id, .. } => Some(id), + #[cfg(unix)] + GuardianApprovalRequest::Execve { id, .. } => Some(id), + } +} + 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 e1595ea1676..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; @@ -347,6 +348,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, @@ -369,6 +371,38 @@ 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] +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(), + 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, + }) + ); } #[tokio::test] @@ -415,6 +449,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/core/src/network_policy_decision_tests.rs b/codex-rs/core/src/network_policy_decision_tests.rs index ebb17f724fe..f0a1a0fc8d3 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(), + parent_tool_item_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(), + 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 277ff2b2414..4975ef431ec 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -68,6 +68,9 @@ pub struct ExecRequest { pub cwd: PathBuf, pub env: HashMap, pub network: 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, @@ -94,6 +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>, + /// 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>, @@ -592,6 +599,7 @@ impl SandboxManager { sandbox, enforce_managed_network, network, + parent_tool_item_id, sandbox_policy_cwd, #[cfg(target_os = "macos")] macos_seatbelt_profile_extensions, @@ -709,6 +717,7 @@ impl SandboxManager { cwd: spec.cwd, env, network: network.cloned(), + 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 9a7a34e49d0..75c0722f17d 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, + parent_tool_item_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, + parent_tool_item_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, + 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 6b42be3cef2..b69fb0fa0f7 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(), + 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/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 446ca100b76..e6dfd8e0525 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(), + 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 @@ -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(), + 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 diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index 4f7c3d74363..1cd53425d3e 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, + 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 9e92a6b00fc..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, } @@ -65,9 +64,11 @@ impl ActiveNetworkApproval { } 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, } @@ -160,8 +161,8 @@ impl PendingHostApproval { } struct ActiveNetworkApprovalCall { - registration_id: String, turn_id: String, + parent_tool_item_id: String, } pub(crate) struct NetworkApprovalService { @@ -194,23 +195,35 @@ 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, 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_active_call( + &self, + parent_tool_item_id: Option<&str>, + ) -> Option> { + if let Some(parent_tool_item_id) = parent_tool_item_id { + let active_calls = self.active_calls.lock().await; + return active_calls.get(parent_tool_item_id).cloned(); + } + + self.resolve_single_active_call().await } async fn resolve_single_active_call(&self) -> Option> { @@ -236,28 +249,36 @@ 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_active_call( + &self, + parent_tool_item_id: Option<&str>, + outcome: NetworkApprovalOutcome, + ) { + 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) { @@ -265,8 +286,11 @@ impl NetworkApprovalService { return; }; - self.record_outcome_for_single_active_call(NetworkApprovalOutcome::DeniedByPolicy(message)) - .await; + self.record_outcome_for_active_call( + blocked.parent_tool_item_id.as_deref(), + NetworkApprovalOutcome::DeniedByPolicy(message), + ) + .await; } async fn active_turn_context(session: &Session) -> Option> { @@ -328,9 +352,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_active_call( + request.parent_tool_item_id.as_deref(), + NetworkApprovalOutcome::DeniedByPolicy(policy_denial_message), + ) .await; return NetworkDecision::deny(REASON_NOT_ALLOWED); }; @@ -338,9 +363,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_active_call( + request.parent_tool_item_id.as_deref(), + NetworkApprovalOutcome::DeniedByPolicy(policy_denial_message), + ) .await; return NetworkDecision::deny(REASON_NOT_ALLOWED); } @@ -349,18 +375,23 @@ impl NetworkApprovalService { host: request.host.clone(), protocol, }; - let owner_call = self.resolve_single_active_call().await; + 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: active_call + .as_ref() + .map(|call| call.parent_tool_item_id.clone()), target, host: request.host, protocol, @@ -457,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; @@ -470,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; @@ -548,6 +579,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 { @@ -556,15 +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()) + .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, }) } @@ -573,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 { @@ -608,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 ad01a45bbd3..fe2dd40b80c 100644 --- a/codex-rs/core/src/tools/network_approval_tests.rs +++ b/codex-rs/core/src/tools/network_approval_tests.rs @@ -180,9 +180,17 @@ fn only_never_policy_disables_network_approval_flow() { } fn denied_blocked_request(host: &str) -> BlockedRequest { + denied_blocked_request_with_parent_tool_item(host, None) +} + +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(), + parent_tool_item_id: parent_tool_item_id.map(ToString::to_string), client: None, method: None, mode: None, @@ -194,10 +202,10 @@ fn denied_blocked_request(host: &str) -> BlockedRequest { } #[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()) + .register_call("turn-1".to_string(), "command-1".to_string()) .await; service @@ -205,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() )) @@ -216,18 +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()) + .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) ); } @@ -236,16 +244,61 @@ 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("turn-1".to_string(), "command-1".to_string()) .await; service - .register_call("registration-2".to_string(), "turn-1".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_active_call_uses_parent_tool_item_id_when_multiple_calls_are_active() { + let service = NetworkApprovalService::default(); + service + .register_call("turn-1".to_string(), "command-1".to_string()) + .await; + service + .register_call("turn-2".to_string(), "command-2".to_string()) + .await; + + let active_call = service + .resolve_active_call(Some("command-2")) + .await + .expect("active call should resolve"); + + 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_parent_tool_item_id_when_multiple_calls_are_active() { + let service = NetworkApprovalService::default(); + service + .register_call("turn-1".to_string(), "command-1".to_string()) + .await; + service + .register_call("turn-2".to_string(), "command-2".to_string()) + .await; + + service + .record_blocked_request(denied_blocked_request_with_parent_tool_item( + "example.com", + Some("command-2"), + )) + .await; + + assert_eq!(service.take_call_outcome("command-1").await, None); + assert_eq!( + 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 4b53ac156fd..bae22f4f927 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -60,18 +60,12 @@ 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), ) .await; - - let attempt_tool_ctx = ToolCtx { - session: tool_ctx.session.clone(), - turn: tool_ctx.turn.clone(), - call_id: tool_ctx.call_id.clone(), - 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 f1e9912bc59..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) + .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 6ff8349b532..34b65c1eb4a 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -253,7 +253,7 @@ impl ToolRuntime for ShellRuntime { req.justification.clone(), )?; let env = attempt - .env_for(spec, req.network.as_ref()) + .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 948018dae6a..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,13 +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()) + .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, + parent_tool_item_id, expiration: _sandbox_expiration, capture_policy: _capture_policy, sandbox, @@ -153,6 +154,7 @@ pub(super) async fn try_run_zsh_fork( sandbox, env: sandbox_env, network: sandbox_network, + parent_tool_item_id, windows_sandbox_level, sandbox_permissions, justification, @@ -259,6 +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(), + 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(), @@ -856,6 +859,7 @@ struct CoreShellCommandExecutor { sandbox: SandboxType, env: HashMap, network: Option, + parent_tool_item_id: Option, windows_sandbox_level: WindowsSandboxLevel, sandbox_permissions: SandboxPermissions, justification: Option, @@ -904,6 +908,7 @@ impl ShellCommandExecutor for CoreShellCommandExecutor { cwd: self.cwd.clone(), env: exec_env, network: self.network.clone(), + parent_tool_item_id: self.parent_tool_item_id.clone(), expiration: ExecExpiration::Cancellation(cancel_rx), capture_policy: ExecCapturePolicy::ShellTool, sandbox: self.sandbox, @@ -1060,6 +1065,7 @@ impl CoreShellCommandExecutor { sandbox, enforce_managed_network: self.network.is_some(), network: self.network.as_ref(), + 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, @@ -1069,7 +1075,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_parent_tool_item( + &mut exec_request.env, + exec_request.parent_tool_item_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..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,6 +655,7 @@ async fn prepare_escalated_exec_turn_default_preserves_macos_seatbelt_extensions cwd: cwd.to_path_buf(), env: HashMap::new(), network: 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(), @@ -707,6 +708,7 @@ async fn prepare_escalated_exec_permissions_preserve_macos_seatbelt_extensions() cwd: cwd.to_path_buf(), env: HashMap::new(), network: None, + parent_tool_item_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, + 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 49ef0e63855..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(&mut env); + 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,7 +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()) + .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, @@ -269,7 +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()) + .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 bf386a12d0b..e894b4b202c 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -341,6 +341,7 @@ impl<'a> SandboxAttempt<'a> { &self, spec: CommandSpec, network: Option<&NetworkProxy>, + parent_tool_item_id: Option<&str>, ) -> Result { self.manager .transform(crate::sandboxing::SandboxTransformRequest { @@ -351,6 +352,7 @@ impl<'a> SandboxAttempt<'a> { sandbox: self.sandbox, enforce_managed_network: self.enforce_managed_network, network, + 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 52d668c0004..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, diff --git a/codex-rs/core/tests/suite/exec.rs b/codex-rs/core/tests/suite/exec.rs index 069e824ee57..be67274e9fb 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 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![ diff --git a/codex-rs/linux-sandbox/tests/suite/landlock.rs b/codex-rs/linux-sandbox/tests/suite/landlock.rs index 8e273783b80..30bfdfc4b29 100644 --- a/codex-rs/linux-sandbox/tests/suite/landlock.rs +++ b/codex-rs/linux-sandbox/tests/suite/landlock.rs @@ -120,6 +120,7 @@ async fn run_cmd_result_with_policies( capture_policy: ExecCapturePolicy::ShellTool, env: create_env_from_core_vars(), network: None, + parent_tool_item_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, @@ -380,6 +381,7 @@ async fn assert_network_blocked(cmd: &[&str]) { capture_policy: ExecCapturePolicy::ShellTool, env: create_env_from_core_vars(), network: 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/network-proxy/src/http_proxy.rs b/codex-rs/network-proxy/src/http_proxy.rs index a3da7e2ddd5..9b7c3c13617 100644 --- a/codex-rs/network-proxy/src/http_proxy.rs +++ b/codex-rs/network-proxy/src/http_proxy.rs @@ -50,6 +50,7 @@ use rama_http::StatusCode; use rama_http::header; use rama_http::headers::HeaderMapExt; use rama_http::headers::Host; +use rama_http::headers::ProxyAuthorization; use rama_http::layer::remove_header::RemoveResponseHeaderLayer; use rama_http::matcher::MethodMatcher; use rama_http_backend::client::proxy::layer::HttpProxyConnector; @@ -65,6 +66,7 @@ use rama_net::proxy::ProxyRequest; use rama_net::proxy::ProxyTarget; use rama_net::proxy::StreamForwardService; use rama_net::stream::SocketInfo; +use rama_net::user::Basic; use rama_tcp::client::Request as TcpRequest; use rama_tcp::client::service::TcpConnector; use rama_tcp::server::TcpListener; @@ -149,6 +151,12 @@ async fn run_http_proxy_with_listener( Ok(()) } +fn proxy_authorization_parent_tool_item_id(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 parent_tool_item_id = proxy_authorization_parent_tool_item_id(&req); let enabled = app_state .enabled() .await @@ -182,12 +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), - 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); } @@ -196,6 +208,7 @@ async fn http_connect_accept( protocol: NetworkProtocol::HttpsConnect, host: host.clone(), port: authority.port, + parent_tool_item_id: parent_tool_item_id.clone(), client_addr: client.clone(), method: Some("CONNECT".to_string()), command: None, @@ -220,6 +233,7 @@ async fn http_connect_accept( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: reason.clone(), + parent_tool_item_id, client: client.clone(), method: Some("CONNECT".to_string()), mode: None, @@ -283,6 +297,7 @@ async fn http_connect_accept( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_MITM_REQUIRED.to_string(), + parent_tool_item_id: None, client: client.clone(), method: Some("CONNECT".to_string()), mode: Some(NetworkMode::Limited), @@ -432,6 +447,7 @@ async fn http_plain_proxy( } }; let client = client_addr(&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 @@ -468,12 +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), - 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); } @@ -613,12 +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), - 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); } @@ -627,6 +649,7 @@ async fn http_plain_proxy( protocol: NetworkProtocol::Http, host: host.clone(), port, + parent_tool_item_id: parent_tool_item_id.clone(), client_addr: client.clone(), method: Some(req.method().as_str().to_string()), command: None, @@ -651,6 +674,7 @@ async fn http_plain_proxy( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: reason.clone(), + parent_tool_item_id: parent_tool_item_id.clone(), client: client.clone(), method: Some(req.method().as_str().to_string()), mode: None, @@ -696,6 +720,7 @@ async fn http_plain_proxy( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: host.clone(), reason: REASON_METHOD_NOT_ALLOWED.to_string(), + parent_tool_item_id, client: client.clone(), method: Some(req.method().as_str().to_string()), mode: Some(NetworkMode::Limited), @@ -884,15 +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( @@ -913,6 +952,7 @@ async fn proxy_disabled_response( .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: blocked_host, reason: REASON_PROXY_DISABLED.to_string(), + parent_tool_item_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..f443d80f2d2 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(), + parent_tool_item_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(), + 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 e759e75c0d6..1f505ffa3ef 100644 --- a/codex-rs/network-proxy/src/network_policy.rs +++ b/codex-rs/network-proxy/src/network_policy.rs @@ -79,6 +79,10 @@ pub struct NetworkPolicyRequest { pub protocol: NetworkProtocol, pub host: String, pub port: u16, + /// 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, @@ -89,6 +93,9 @@ pub struct NetworkPolicyRequestArgs { pub protocol: NetworkProtocol, pub host: String, pub port: u16, + /// 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, @@ -101,6 +108,7 @@ impl NetworkPolicyRequest { protocol, host, port, + parent_tool_item_id, client_addr, method, command, @@ -110,6 +118,7 @@ impl NetworkPolicyRequest { protocol, host, port, + parent_tool_item_id, client_addr, method, command, @@ -625,6 +634,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "example.com".to_string(), port: 80, + parent_tool_item_id: None, client_addr: None, method: None, command: None, @@ -685,6 +695,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "blocked.com".to_string(), port: 80, + parent_tool_item_id: None, client_addr: Some("127.0.0.1:1234".to_string()), method: Some("GET".to_string()), command: None, @@ -726,6 +737,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "example.com".to_string(), port: 80, + parent_tool_item_id: None, client_addr: None, method: Some("GET".to_string()), command: None, @@ -776,6 +788,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "example.com".to_string(), port: 80, + parent_tool_item_id: None, client_addr: None, method: Some("GET".to_string()), command: None, @@ -859,6 +872,7 @@ mod tests { protocol: NetworkProtocol::Http, host: "127.0.0.1".to_string(), port: 80, + 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 new file mode 100644 index 00000000000..6fbae7d1cd6 --- /dev/null +++ b/codex-rs/network-proxy/src/owner_identity.rs @@ -0,0 +1,41 @@ +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 ProxyParentToolItemAuthorizer; + +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 ProxyParentToolItemAuthorizer { + 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)), + } + } +} + +/// 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(), + 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..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,15 +306,34 @@ fn set_env_keys(env: &mut HashMap, keys: &[&str], value: &str) { } } +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( env: &mut HashMap, http_addr: SocketAddr, socks_addr: SocketAddr, socks_enabled: bool, allow_local_binding: bool, + parent_tool_item_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, 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 { @@ -414,6 +434,16 @@ impl NetworkProxy { } pub fn apply_to_env(&self, env: &mut HashMap) { + self.apply_to_env_for_parent_tool_item(env, /*parent_tool_item_id*/ None); + } + + /// 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, + 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. apply_proxy_env_overrides( @@ -422,6 +452,7 @@ impl NetworkProxy { self.socks_addr, self.socks_enabled, self.allow_local_binding, + parent_tool_item_id, ); } @@ -696,6 +727,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, + /*parent_tool_item_id*/ None, ); assert_eq!( @@ -746,6 +778,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), false, true, + /*parent_tool_item_id*/ None, ); assert_eq!( @@ -764,6 +797,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, + /*parent_tool_item_id*/ None, ); assert_eq!( @@ -809,6 +843,7 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, + /*parent_tool_item_id*/ None, ); assert_eq!( @@ -816,4 +851,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..d3e58993664 100644 --- a/codex-rs/network-proxy/src/runtime.rs +++ b/codex-rs/network-proxy/src/runtime.rs @@ -84,6 +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 parent_tool_item_id: Option, pub client: Option, pub method: Option, pub mode: Option, @@ -100,6 +105,9 @@ pub struct BlockedRequest { pub struct BlockedRequestArgs { pub host: String, pub reason: String, + /// 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, @@ -114,6 +122,7 @@ impl BlockedRequest { let BlockedRequestArgs { host, reason, + parent_tool_item_id, client, method, mode, @@ -125,6 +134,7 @@ impl BlockedRequest { Self { host, reason, + parent_tool_item_id, client, method, mode, @@ -994,6 +1004,7 @@ mod tests { .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: "google.com".to_string(), reason: "not_allowed".to_string(), + parent_tool_item_id: None, client: None, method: Some("GET".to_string()), mode: None, @@ -1034,6 +1045,7 @@ mod tests { .record_blocked(BlockedRequest::new(BlockedRequestArgs { host: format!("example{idx}.com"), reason: "not_allowed".to_string(), + parent_tool_item_id: None, client: None, method: Some("GET".to_string()), mode: None, @@ -1056,6 +1068,7 @@ mod tests { let entry = BlockedRequest { host: "google.com".to_string(), reason: "not_allowed".to_string(), + 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 30ec4d2903e..d31cbe77278 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::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; @@ -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(ProxyParentToolItemAuthorizer); 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 parent_tool_item_id = extract_parent_tool_item_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(), + parent_tool_item_id: parent_tool_item_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(), + parent_tool_item_id: parent_tool_item_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, + parent_tool_item_id: parent_tool_item_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(), + parent_tool_item_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 parent_tool_item_id = extract_parent_tool_item_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(), + parent_tool_item_id: parent_tool_item_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(), + parent_tool_item_id: parent_tool_item_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, + parent_tool_item_id: parent_tool_item_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(), + parent_tool_item_id, client: client.clone(), method: None, mode: None, 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)] 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.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..e1c08acfa08 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: Some("guardian-1".into()), 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: Some("guardian-1".into()), 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: Some("thread:child-thread:guardian-1".into()), 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: Some("guardian-1".to_string()), 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: Some("guardian-1".to_string()), 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: Some("guardian-1".to_string()), 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: Some(id.to_string()), 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: Some("guardian-1".to_string()), 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: Some("guardian-2".to_string()), 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: Some("guardian-1".to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::Denied, risk_score: Some(92),