diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 4f5410c78..825dbfd12 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -473,6 +473,13 @@ } ] }, + "AutoReviewDecisionSource": { + "description": "[UNSTABLE] Source that produced a terminal guardian approval review decision.", + "enum": [ + "agent" + ], + "type": "string" + }, "ByteRange": { "properties": { "end": { @@ -1673,17 +1680,28 @@ "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. This shape is expected to change soon.", "properties": { "action": { "$ref": "#/definitions/GuardianApprovalReviewAction" }, + "decisionSource": { + "$ref": "#/definitions/AutoReviewDecisionSource" + }, "review": { "$ref": "#/definitions/GuardianApprovalReview" }, - "targetItemId": { + "reviewId": { + "description": "Stable identifier for this review.", "type": "string" }, + "targetItemId": { + "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", + "type": [ + "string", + "null" + ] + }, "threadId": { "type": "string" }, @@ -1693,15 +1711,16 @@ }, "required": [ "action", + "decisionSource", "review", - "targetItemId", + "reviewId", "threadId", "turnId" ], "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. This shape is expected to change soon.", "properties": { "action": { "$ref": "#/definitions/GuardianApprovalReviewAction" @@ -1709,9 +1728,17 @@ "review": { "$ref": "#/definitions/GuardianApprovalReview" }, - "targetItemId": { + "reviewId": { + "description": "Stable identifier for this review.", "type": "string" }, + "targetItemId": { + "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", + "type": [ + "string", + "null" + ] + }, "threadId": { "type": "string" }, @@ -1722,7 +1749,7 @@ "required": [ "action", "review", - "targetItemId", + "reviewId", "threadId", "turnId" ], 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 c432c742e..4526b8968 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 @@ -5667,6 +5667,13 @@ } ] }, + "AutoReviewDecisionSource": { + "description": "[UNSTABLE] Source that produced a terminal guardian approval review decision.", + "enum": [ + "agent" + ], + "type": "string" + }, "ByteRange": { "properties": { "end": { @@ -8719,17 +8726,28 @@ }, "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. This shape is expected to change soon.", "properties": { "action": { "$ref": "#/definitions/v2/GuardianApprovalReviewAction" }, + "decisionSource": { + "$ref": "#/definitions/v2/AutoReviewDecisionSource" + }, "review": { "$ref": "#/definitions/v2/GuardianApprovalReview" }, - "targetItemId": { + "reviewId": { + "description": "Stable identifier for this review.", "type": "string" }, + "targetItemId": { + "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", + "type": [ + "string", + "null" + ] + }, "threadId": { "type": "string" }, @@ -8739,8 +8757,9 @@ }, "required": [ "action", + "decisionSource", "review", - "targetItemId", + "reviewId", "threadId", "turnId" ], @@ -8749,7 +8768,7 @@ }, "ItemGuardianApprovalReviewStartedNotification": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", + "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.", "properties": { "action": { "$ref": "#/definitions/v2/GuardianApprovalReviewAction" @@ -8757,9 +8776,17 @@ "review": { "$ref": "#/definitions/v2/GuardianApprovalReview" }, - "targetItemId": { + "reviewId": { + "description": "Stable identifier for this review.", "type": "string" }, + "targetItemId": { + "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", + "type": [ + "string", + "null" + ] + }, "threadId": { "type": "string" }, @@ -8770,7 +8797,7 @@ "required": [ "action", "review", - "targetItemId", + "reviewId", "threadId", "turnId" ], 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 0322866eb..34a628dc3 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 @@ -802,6 +802,13 @@ } ] }, + "AutoReviewDecisionSource": { + "description": "[UNSTABLE] Source that produced a terminal guardian approval review decision.", + "enum": [ + "agent" + ], + "type": "string" + }, "ByteRange": { "properties": { "end": { @@ -5502,17 +5509,28 @@ }, "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. This shape is expected to change soon.", "properties": { "action": { "$ref": "#/definitions/GuardianApprovalReviewAction" }, + "decisionSource": { + "$ref": "#/definitions/AutoReviewDecisionSource" + }, "review": { "$ref": "#/definitions/GuardianApprovalReview" }, - "targetItemId": { + "reviewId": { + "description": "Stable identifier for this review.", "type": "string" }, + "targetItemId": { + "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", + "type": [ + "string", + "null" + ] + }, "threadId": { "type": "string" }, @@ -5522,8 +5540,9 @@ }, "required": [ "action", + "decisionSource", "review", - "targetItemId", + "reviewId", "threadId", "turnId" ], @@ -5532,7 +5551,7 @@ }, "ItemGuardianApprovalReviewStartedNotification": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", + "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.", "properties": { "action": { "$ref": "#/definitions/GuardianApprovalReviewAction" @@ -5540,9 +5559,17 @@ "review": { "$ref": "#/definitions/GuardianApprovalReview" }, - "targetItemId": { + "reviewId": { + "description": "Stable identifier for this review.", "type": "string" }, + "targetItemId": { + "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", + "type": [ + "string", + "null" + ] + }, "threadId": { "type": "string" }, @@ -5553,7 +5580,7 @@ "required": [ "action", "review", - "targetItemId", + "reviewId", "threadId", "turnId" ], 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 1f706e545..f8b9199bb 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json @@ -1,6 +1,13 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "definitions": { + "AutoReviewDecisionSource": { + "description": "[UNSTABLE] Source that produced a terminal guardian approval review decision.", + "enum": [ + "agent" + ], + "type": "string" + }, "GuardianApprovalReview": { "description": "[UNSTABLE] Temporary guardian approval review payload used by `item/autoApprovalReview/*` notifications. This shape is expected to change soon.", "properties": { @@ -256,17 +263,28 @@ "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. This shape is expected to change soon.", "properties": { "action": { "$ref": "#/definitions/GuardianApprovalReviewAction" }, + "decisionSource": { + "$ref": "#/definitions/AutoReviewDecisionSource" + }, "review": { "$ref": "#/definitions/GuardianApprovalReview" }, - "targetItemId": { + "reviewId": { + "description": "Stable identifier for this review.", "type": "string" }, + "targetItemId": { + "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", + "type": [ + "string", + "null" + ] + }, "threadId": { "type": "string" }, @@ -276,8 +294,9 @@ }, "required": [ "action", + "decisionSource", "review", - "targetItemId", + "reviewId", "threadId", "turnId" ], 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 cd9adf32f..9f6c35b2a 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json @@ -256,7 +256,7 @@ "type": "string" } }, - "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.\n\nTODO(ccunningham): Attach guardian review state to the reviewed tool item's lifecycle instead of sending separate standalone review notifications so the app-server API can persist and replay review state via `thread/read`.", + "description": "[UNSTABLE] Temporary notification payload for guardian automatic approval review. This shape is expected to change soon.", "properties": { "action": { "$ref": "#/definitions/GuardianApprovalReviewAction" @@ -264,9 +264,17 @@ "review": { "$ref": "#/definitions/GuardianApprovalReview" }, - "targetItemId": { + "reviewId": { + "description": "Stable identifier for this review.", "type": "string" }, + "targetItemId": { + "description": "Identifier for the reviewed item or tool call when one exists.\n\nIn most cases, one review maps to one target item. The exceptions are - execve reviews, where a single command may contain multiple execve calls to review (only possible when using the shell_zsh_fork feature) - network policy reviews, where there is no target item\n\nA network call is triggered by a CommandExecution item, so having a target_item_id set to the CommandExecution item would be misleading because the review is about the network call, not the command execution. Therefore, target_item_id is set to None for network policy reviews.", + "type": [ + "string", + "null" + ] + }, "threadId": { "type": "string" }, @@ -277,7 +285,7 @@ "required": [ "action", "review", - "targetItemId", + "reviewId", "threadId", "turnId" ], diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDecisionSource.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDecisionSource.ts new file mode 100644 index 000000000..aa776c8ca --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AutoReviewDecisionSource.ts @@ -0,0 +1,8 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * [UNSTABLE] Source that produced a terminal guardian approval review decision. + */ +export type AutoReviewDecisionSource = "agent"; 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 a7ae038b3..a345e3d65 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewCompletedNotification.ts @@ -1,15 +1,30 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { AutoReviewDecisionSource } from "./AutoReviewDecisionSource"; import type { GuardianApprovalReview } from "./GuardianApprovalReview"; import type { GuardianApprovalReviewAction } from "./GuardianApprovalReviewAction"; /** * [UNSTABLE] Temporary notification payload for guardian automatic approval * review. This shape is expected to change soon. - * - * 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: GuardianApprovalReviewAction, }; +export type ItemGuardianApprovalReviewCompletedNotification = { threadId: string, turnId: string, +/** + * Stable identifier for this review. + */ +reviewId: string, +/** + * Identifier for the reviewed item or tool call when one exists. + * + * In most cases, one review maps to one target item. The exceptions are + * - execve reviews, where a single command may contain multiple execve + * calls to review (only possible when using the shell_zsh_fork feature) + * - network policy reviews, where there is no target item + * + * A network call is triggered by a CommandExecution item, so having a + * target_item_id set to the CommandExecution item would be misleading + * because the review is about the network call, not the command execution. + * Therefore, target_item_id is set to None for network policy reviews. + */ +targetItemId: string | null, decisionSource: AutoReviewDecisionSource, review: GuardianApprovalReview, action: GuardianApprovalReviewAction, }; 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 706b8d55a..f917aa3ee 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ItemGuardianApprovalReviewStartedNotification.ts @@ -7,9 +7,23 @@ import type { GuardianApprovalReviewAction } from "./GuardianApprovalReviewActio /** * [UNSTABLE] Temporary notification payload for guardian automatic approval * review. This shape is expected to change soon. - * - * 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: GuardianApprovalReviewAction, }; +export type ItemGuardianApprovalReviewStartedNotification = { threadId: string, turnId: string, +/** + * Stable identifier for this review. + */ +reviewId: string, +/** + * Identifier for the reviewed item or tool call when one exists. + * + * In most cases, one review maps to one target item. The exceptions are + * - execve reviews, where a single command may contain multiple execve + * calls to review (only possible when using the shell_zsh_fork feature) + * - network policy reviews, where there is no target item + * + * A network call is triggered by a CommandExecution item, so having a + * target_item_id set to the CommandExecution item would be misleading + * because the review is about the network call, not the command execution. + * Therefore, target_item_id is set to None for network policy reviews. + */ +targetItemId: string | null, review: GuardianApprovalReview, action: GuardianApprovalReviewAction, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts index f541aed1b..243502752 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -26,6 +26,7 @@ export type { AppsDefaultConfig } from "./AppsDefaultConfig"; export type { AppsListParams } from "./AppsListParams"; export type { AppsListResponse } from "./AppsListResponse"; export type { AskForApproval } from "./AskForApproval"; +export type { AutoReviewDecisionSource } from "./AutoReviewDecisionSource"; export type { ByteRange } from "./ByteRange"; export type { CancelLoginAccountParams } from "./CancelLoginAccountParams"; export type { CancelLoginAccountResponse } from "./CancelLoginAccountResponse"; diff --git a/codex-rs/app-server-protocol/src/protocol/item_builders.rs b/codex-rs/app-server-protocol/src/protocol/item_builders.rs index 9853f69f4..375e5605d 100644 --- a/codex-rs/app-server-protocol/src/protocol/item_builders.rs +++ b/codex-rs/app-server-protocol/src/protocol/item_builders.rs @@ -11,6 +11,7 @@ //! - The projection is presentation-specific. Core protocol events stay generic, while the //! app-server protocol decides how to surface those events as `ThreadItem`s for clients. use crate::protocol::common::ServerNotification; +use crate::protocol::v2::AutoReviewDecisionSource; use crate::protocol::v2::CommandAction; use crate::protocol::v2::CommandExecutionSource; use crate::protocol::v2::CommandExecutionStatus; @@ -142,12 +143,13 @@ pub fn build_item_from_guardian_event( ) -> Option { match &assessment.action { GuardianAssessmentAction::Command { command, cwd, .. } => { + let id = assessment.target_item_id.as_ref()?; let command = command.clone(); let command_actions = vec![CommandAction::Unknown { command: command.clone(), }]; Some(ThreadItem::CommandExecution { - id: assessment.id.clone(), + id: id.clone(), command, cwd: cwd.clone(), process_id: None, @@ -162,6 +164,7 @@ pub fn build_item_from_guardian_event( GuardianAssessmentAction::Execve { program, argv, cwd, .. } => { + let id = assessment.target_item_id.as_ref()?; let argv = if argv.is_empty() { vec![program.clone()] } else { @@ -179,7 +182,7 @@ pub fn build_item_from_guardian_event( parsed_cmd.into_iter().map(CommandAction::from).collect() }; Some(ThreadItem::CommandExecution { - id: assessment.id.clone(), + id: id.clone(), command, cwd: cwd.clone(), process_id: None, @@ -202,9 +205,6 @@ pub fn guardian_auto_approval_review_notification( event_turn_id: &str, assessment: &GuardianAssessmentEvent, ) -> ServerNotification { - // TODO(ccunningham): Attach guardian review state to the reviewed tool - // item's lifecycle instead of sending standalone review notifications so - // the app-server API can persist and replay review state via `thread/read`. let turn_id = if assessment.turn_id.is_empty() { event_turn_id.to_string() } else { @@ -236,7 +236,8 @@ pub fn guardian_auto_approval_review_notification( ItemGuardianApprovalReviewStartedNotification { thread_id: conversation_id.to_string(), turn_id, - target_item_id: assessment.id.clone(), + review_id: assessment.id.clone(), + target_item_id: assessment.target_item_id.clone(), review, action, }, @@ -249,7 +250,12 @@ pub fn guardian_auto_approval_review_notification( ItemGuardianApprovalReviewCompletedNotification { thread_id: conversation_id.to_string(), turn_id, - target_item_id: assessment.id.clone(), + review_id: assessment.id.clone(), + target_item_id: assessment.target_item_id.clone(), + decision_source: assessment + .decision_source + .map(AutoReviewDecisionSource::from) + .unwrap_or(AutoReviewDecisionSource::Agent), review, action, }, diff --git a/codex-rs/app-server-protocol/src/protocol/thread_history.rs b/codex-rs/app-server-protocol/src/protocol/thread_history.rs index b00565796..a99fe6d4f 100644 --- a/codex-rs/app-server-protocol/src/protocol/thread_history.rs +++ b/codex-rs/app-server-protocol/src/protocol/thread_history.rs @@ -2088,12 +2088,14 @@ mod tests { local_images: Vec::new(), }), EventMsg::GuardianAssessment(GuardianAssessmentEvent { - id: "guardian-exec".into(), + id: "review-guardian-exec".into(), + target_item_id: Some("guardian-exec".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, rationale: None, + decision_source: None, action: serde_json::from_value(serde_json::json!({ "type": "command", "source": "shell", @@ -2103,12 +2105,16 @@ mod tests { .expect("guardian action"), }), EventMsg::GuardianAssessment(GuardianAssessmentEvent { - id: "guardian-exec".into(), + id: "review-guardian-exec".into(), + target_item_id: Some("guardian-exec".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::Denied, risk_level: Some(codex_protocol::protocol::GuardianRiskLevel::High), user_authorization: Some(codex_protocol::protocol::GuardianUserAuthorization::Low), rationale: Some("Would delete user data.".into()), + decision_source: Some( + codex_protocol::protocol::GuardianAssessmentDecisionSource::Agent, + ), action: serde_json::from_value(serde_json::json!({ "type": "command", "source": "shell", @@ -2161,12 +2167,14 @@ mod tests { local_images: Vec::new(), }), EventMsg::GuardianAssessment(GuardianAssessmentEvent { - id: "guardian-execve".into(), + id: "review-guardian-execve".into(), + target_item_id: Some("guardian-execve".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, rationale: None, + decision_source: None, action: serde_json::from_value(serde_json::json!({ "type": "execve", "source": "shell", diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index a93fdab15..95308091b 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -9,6 +9,7 @@ use codex_protocol::account::PlanType; use codex_protocol::approvals::ElicitationRequest as CoreElicitationRequest; use codex_protocol::approvals::ExecPolicyAmendment as CoreExecPolicyAmendment; use codex_protocol::approvals::GuardianAssessmentAction as CoreGuardianAssessmentAction; +use codex_protocol::approvals::GuardianAssessmentDecisionSource as CoreGuardianAssessmentDecisionSource; use codex_protocol::approvals::GuardianCommandSource as CoreGuardianCommandSource; use codex_protocol::approvals::NetworkApprovalContext as CoreNetworkApprovalContext; use codex_protocol::approvals::NetworkApprovalProtocol as CoreNetworkApprovalProtocol; @@ -4565,6 +4566,22 @@ pub enum GuardianApprovalReviewStatus { Aborted, } +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +/// [UNSTABLE] Source that produced a terminal guardian approval review decision. +pub enum AutoReviewDecisionSource { + Agent, +} + +impl From for AutoReviewDecisionSource { + fn from(value: CoreGuardianAssessmentDecisionSource) -> Self { + match value { + CoreGuardianAssessmentDecisionSource::Agent => Self::Agent, + } + } +} + #[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "lowercase")] #[ts(export_to = "v2/")] @@ -5321,14 +5338,23 @@ pub struct ItemStartedNotification { #[ts(export_to = "v2/")] /// [UNSTABLE] Temporary notification payload for guardian automatic approval /// review. This shape is expected to change soon. -/// -/// 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`. pub struct ItemGuardianApprovalReviewStartedNotification { pub thread_id: String, pub turn_id: String, - pub target_item_id: String, + /// Stable identifier for this review. + pub review_id: String, + /// Identifier for the reviewed item or tool call when one exists. + /// + /// In most cases, one review maps to one target item. The exceptions are + /// - execve reviews, where a single command may contain multiple execve + /// calls to review (only possible when using the shell_zsh_fork feature) + /// - network policy reviews, where there is no target item + /// + /// A network call is triggered by a CommandExecution item, so having a + /// target_item_id set to the CommandExecution item would be misleading + /// because the review is about the network call, not the command execution. + /// Therefore, target_item_id is set to None for network policy reviews. + pub target_item_id: Option, pub review: GuardianApprovalReview, pub action: GuardianApprovalReviewAction, } @@ -5338,14 +5364,24 @@ pub struct ItemGuardianApprovalReviewStartedNotification { #[ts(export_to = "v2/")] /// [UNSTABLE] Temporary notification payload for guardian automatic approval /// review. This shape is expected to change soon. -/// -/// 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`. pub struct ItemGuardianApprovalReviewCompletedNotification { pub thread_id: String, pub turn_id: String, - pub target_item_id: String, + /// Stable identifier for this review. + pub review_id: String, + /// Identifier for the reviewed item or tool call when one exists. + /// + /// In most cases, one review maps to one target item. The exceptions are + /// - execve reviews, where a single command may contain multiple execve + /// calls to review (only possible when using the shell_zsh_fork feature) + /// - network policy reviews, where there is no target item + /// + /// A network call is triggered by a CommandExecution item, so having a + /// target_item_id set to the CommandExecution item would be misleading + /// because the review is about the network call, not the command execution. + /// Therefore, target_item_id is set to None for network policy reviews. + pub target_item_id: Option, + pub decision_source: AutoReviewDecisionSource, pub review: GuardianApprovalReview, pub action: GuardianApprovalReviewAction, } diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index ce71e8974..b99d7b229 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -291,15 +291,19 @@ pub(crate) async fn apply_bespoke_event_handling( CommandExecutionStatus::InProgress, ) { Some(ThreadItem::CommandExecution { + id, command, cwd, command_actions, .. - }) => Some(CommandExecutionCompletionItem { - command, - cwd, - command_actions, - }), + }) => Some(( + id, + CommandExecutionCompletionItem { + command, + cwd, + command_actions, + }, + )), Some(_) | None => None, }; let assessment_turn_id = if assessment.turn_id.is_empty() { @@ -309,12 +313,13 @@ pub(crate) async fn apply_bespoke_event_handling( }; if assessment.status == codex_protocol::protocol::GuardianAssessmentStatus::InProgress - && let Some(completion_item) = pending_command_execution.as_ref() + && let Some((target_item_id, completion_item)) = + pending_command_execution.as_ref() { start_command_execution_item( &conversation_id, assessment_turn_id.clone(), - assessment.id.clone(), + target_item_id.clone(), completion_item.command.clone(), completion_item.cwd.clone(), completion_item.command_actions.clone(), @@ -334,12 +339,12 @@ pub(crate) async fn apply_bespoke_event_handling( assessment.status, codex_protocol::protocol::GuardianAssessmentStatus::Denied | codex_protocol::protocol::GuardianAssessmentStatus::Aborted - ) && let Some(completion_item) = pending_command_execution + ) && let Some((target_item_id, completion_item)) = pending_command_execution { complete_command_execution_item( &conversation_id, assessment_turn_id, - assessment.id.clone(), + target_item_id, completion_item.command, completion_item.cwd, /*process_id*/ None, @@ -2919,6 +2924,7 @@ mod tests { use anyhow::Result; use anyhow::anyhow; use anyhow::bail; + use codex_app_server_protocol::AutoReviewDecisionSource; use codex_app_server_protocol::GuardianApprovalReviewStatus; use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::TurnPlanStepStatus; @@ -3020,12 +3026,18 @@ mod tests { GuardianAssessmentStatus::Aborted => (None, None, None), }; GuardianAssessmentEvent { - id: id.to_string(), + id: format!("review-{id}"), + target_item_id: Some(id.to_string()), turn_id: turn_id.to_string(), status, risk_level, user_authorization, rationale, + decision_source: if matches!(status, GuardianAssessmentStatus::InProgress) { + None + } else { + Some(codex_protocol::protocol::GuardianAssessmentDecisionSource::Agent) + }, action: serde_json::from_value(json!({ "type": "command", "source": "shell", @@ -3080,12 +3092,14 @@ mod tests { &conversation_id, "turn-from-event", &GuardianAssessmentEvent { - id: "item-1".to_string(), + id: "review-1".to_string(), + target_item_id: Some("item-1".to_string()), turn_id: String::new(), status: codex_protocol::protocol::GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, rationale: None, + decision_source: None, action: action.clone(), }, ); @@ -3094,7 +3108,8 @@ mod tests { ServerNotification::ItemGuardianApprovalReviewStarted(payload) => { 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, "review-1"); + assert_eq!(payload.target_item_id.as_deref(), Some("item-1")); assert_eq!( payload.review.status, GuardianApprovalReviewStatus::InProgress @@ -3120,12 +3135,16 @@ mod tests { &conversation_id, "turn-from-event", &GuardianAssessmentEvent { - id: "item-2".to_string(), + id: "review-2".to_string(), + target_item_id: Some("item-2".to_string()), turn_id: "turn-from-assessment".to_string(), status: codex_protocol::protocol::GuardianAssessmentStatus::Denied, risk_level: Some(codex_protocol::protocol::GuardianRiskLevel::High), user_authorization: Some(codex_protocol::protocol::GuardianUserAuthorization::Low), rationale: Some("too risky".to_string()), + decision_source: Some( + codex_protocol::protocol::GuardianAssessmentDecisionSource::Agent, + ), action: action.clone(), }, ); @@ -3134,7 +3153,9 @@ 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-2"); + assert_eq!(payload.review_id, "review-2"); + assert_eq!(payload.target_item_id.as_deref(), Some("item-2")); + assert_eq!(payload.decision_source, AutoReviewDecisionSource::Agent); assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Denied); assert_eq!( payload.review.risk_level, @@ -3164,12 +3185,16 @@ mod tests { &conversation_id, "turn-from-event", &GuardianAssessmentEvent { - id: "item-3".to_string(), + id: "review-3".to_string(), + target_item_id: None, turn_id: "turn-from-assessment".to_string(), status: codex_protocol::protocol::GuardianAssessmentStatus::Aborted, risk_level: None, user_authorization: None, rationale: None, + decision_source: Some( + codex_protocol::protocol::GuardianAssessmentDecisionSource::Agent, + ), action: action.clone(), }, ); @@ -3178,7 +3203,9 @@ 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.review_id, "review-3"); + assert_eq!(payload.target_item_id, None); + assert_eq!(payload.decision_source, AutoReviewDecisionSource::Agent); assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Aborted); assert_eq!(payload.review.risk_level, None); assert_eq!(payload.review.user_authorization, None); @@ -3395,7 +3422,11 @@ mod tests { OutgoingMessage::AppServerNotification( ServerNotification::ItemGuardianApprovalReviewStarted(payload), ) => { - assert_eq!(payload.target_item_id, "cmd-guardian-approved"); + assert_eq!(payload.review_id, "review-cmd-guardian-approved"); + assert_eq!( + payload.target_item_id.as_deref(), + Some("cmd-guardian-approved") + ); assert_eq!( payload.review.status, GuardianApprovalReviewStatus::InProgress @@ -3416,7 +3447,12 @@ mod tests { OutgoingMessage::AppServerNotification( ServerNotification::ItemGuardianApprovalReviewCompleted(payload), ) => { - assert_eq!(payload.target_item_id, "cmd-guardian-approved"); + assert_eq!(payload.review_id, "review-cmd-guardian-approved"); + assert_eq!( + payload.target_item_id.as_deref(), + Some("cmd-guardian-approved") + ); + assert_eq!(payload.decision_source, AutoReviewDecisionSource::Agent); assert_eq!( payload.review.status, GuardianApprovalReviewStatus::Approved @@ -3453,7 +3489,11 @@ mod tests { OutgoingMessage::AppServerNotification( ServerNotification::ItemGuardianApprovalReviewStarted(payload), ) => { - assert_eq!(payload.target_item_id, "cmd-guardian-denied"); + assert_eq!(payload.review_id, "review-cmd-guardian-denied"); + assert_eq!( + payload.target_item_id.as_deref(), + Some("cmd-guardian-denied") + ); assert_eq!( payload.review.status, GuardianApprovalReviewStatus::InProgress @@ -3474,7 +3514,12 @@ mod tests { OutgoingMessage::AppServerNotification( ServerNotification::ItemGuardianApprovalReviewCompleted(payload), ) => { - assert_eq!(payload.target_item_id, "cmd-guardian-denied"); + assert_eq!(payload.review_id, "review-cmd-guardian-denied"); + assert_eq!( + payload.target_item_id.as_deref(), + Some("cmd-guardian-denied") + ); + assert_eq!(payload.decision_source, AutoReviewDecisionSource::Agent); assert_eq!(payload.review.status, GuardianApprovalReviewStatus::Denied); } other => bail!("unexpected message: {other:?}"), @@ -3491,6 +3536,30 @@ mod tests { other => bail!("unexpected message: {other:?}"), } + let mut missing_target = guardian_command_assessment( + "cmd-guardian-missing-target", + "turn-guardian-missing-target", + GuardianAssessmentStatus::InProgress, + ); + missing_target.target_item_id = None; + guardian_context + .apply_guardian_assessment_event(missing_target) + .await; + let eighth = recv_broadcast_message(&mut rx).await?; + match eighth { + OutgoingMessage::AppServerNotification( + ServerNotification::ItemGuardianApprovalReviewStarted(payload), + ) => { + assert_eq!(payload.review_id, "review-cmd-guardian-missing-target"); + assert_eq!(payload.target_item_id, None); + assert_eq!( + payload.review.status, + GuardianApprovalReviewStatus::InProgress + ); + } + other => bail!("unexpected message: {other:?}"), + } + assert!(rx.try_recv().is_err(), "no extra messages expected"); conversation.shutdown_and_wait().await?; Ok(()) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 70a2883c2..f7932b92a 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -2026,7 +2026,7 @@ impl Session { session_telemetry, models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - guardian_rejection_rationales: Mutex::new(HashMap::new()), + guardian_rejections: Mutex::new(HashMap::new()), skills_manager, plugins_manager: Arc::clone(&plugins_manager), mcp_manager: Arc::clone(&mcp_manager), diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 144d9a971..4deb7d3cf 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -39,6 +39,7 @@ use crate::codex::TurnContext; use crate::codex::emit_subagent_session_started; use crate::config::Config; use crate::guardian::GuardianApprovalRequest; +use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request_with_cancel; use crate::guardian::routes_approval_to_guardian; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT; @@ -459,6 +460,7 @@ async fn handle_exec_approval( let review_rx = spawn_guardian_review( Arc::clone(parent_session), Arc::clone(parent_ctx), + new_guardian_review_id(), GuardianApprovalRequest::Shell { id: call_id.clone(), command, @@ -566,6 +568,7 @@ async fn handle_patch_approval( let review_rx = spawn_guardian_review( Arc::clone(parent_session), Arc::clone(parent_ctx), + new_guardian_review_id(), GuardianApprovalRequest::ApplyPatch { id: approval_id.clone(), cwd: parent_ctx.cwd.to_path_buf(), @@ -686,6 +689,7 @@ async fn maybe_auto_review_mcp_request_user_input( let review_rx = spawn_guardian_review( Arc::clone(parent_session), Arc::clone(parent_ctx), + new_guardian_review_id(), build_guardian_mcp_tool_review_request(&event.call_id, &invocation, metadata.as_ref()), /*retry_reason*/ None, review_cancel.clone(), @@ -729,6 +733,7 @@ async fn maybe_auto_review_mcp_request_user_input( fn spawn_guardian_review( session: Arc, turn: Arc, + review_id: String, request: GuardianApprovalRequest, retry_reason: Option, cancel_token: CancellationToken, @@ -745,6 +750,7 @@ fn spawn_guardian_review( let decision = runtime.block_on(review_approval_request_with_cancel( &session, &turn, + review_id, request, retry_reason, cancel_token, diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 323b41865..62ee88481 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -10,7 +10,6 @@ use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecApprovalRequestEvent; use codex_protocol::protocol::GuardianAssessmentAction; -use codex_protocol::protocol::GuardianAssessmentEvent; use codex_protocol::protocol::GuardianAssessmentStatus; use codex_protocol::protocol::GuardianCommandSource; use codex_protocol::protocol::McpInvocation; @@ -311,22 +310,26 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f }) .await .expect("timed out waiting for guardian assessment"); + let expected_action = GuardianAssessmentAction::Command { + source: GuardianCommandSource::Shell, + command: "rm -rf tmp".to_string(), + cwd: "/tmp".into(), + }; + assert!(!assessment_event.id.is_empty()); assert_eq!( - assessment_event, - GuardianAssessmentEvent { - id: "command-item-1".to_string(), - turn_id: parent_ctx.sub_id.clone(), - status: GuardianAssessmentStatus::InProgress, - risk_level: None, - user_authorization: None, - rationale: None, - action: GuardianAssessmentAction::Command { - source: GuardianCommandSource::Shell, - command: "rm -rf tmp".to_string(), - cwd: "/tmp".into(), - }, - } + assessment_event.target_item_id.as_deref(), + Some("command-item-1") ); + assert_eq!(assessment_event.turn_id, parent_ctx.sub_id); + assert_eq!( + assessment_event.status, + GuardianAssessmentStatus::InProgress + ); + assert_eq!(assessment_event.risk_level, None); + assert_eq!(assessment_event.user_authorization, None); + assert_eq!(assessment_event.rationale, None); + assert_eq!(assessment_event.decision_source, None); + assert_eq!(assessment_event.action, expected_action); cancel_token.cancel(); diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 66c116ee5..9d9858eba 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -2879,7 +2879,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { session_telemetry: session_telemetry.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - guardian_rejection_rationales: Mutex::new(std::collections::HashMap::new()), + guardian_rejections: Mutex::new(std::collections::HashMap::new()), skills_manager, plugins_manager, mcp_manager, @@ -3724,7 +3724,7 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx( session_telemetry: session_telemetry.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - guardian_rejection_rationales: Mutex::new(std::collections::HashMap::new()), + guardian_rejections: Mutex::new(std::collections::HashMap::new()), skills_manager, plugins_manager, mcp_manager, diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index b89d020ca..c9cc9d9fa 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -359,15 +359,15 @@ pub(crate) fn guardian_assessment_action( } } -pub(crate) fn guardian_request_id(request: &GuardianApprovalRequest) -> &str { +pub(crate) fn guardian_request_target_item_id(request: &GuardianApprovalRequest) -> Option<&str> { match request { GuardianApprovalRequest::Shell { id, .. } | GuardianApprovalRequest::ExecCommand { id, .. } | GuardianApprovalRequest::ApplyPatch { id, .. } - | GuardianApprovalRequest::NetworkAccess { id, .. } - | GuardianApprovalRequest::McpToolCall { id, .. } => id, + | GuardianApprovalRequest::McpToolCall { id, .. } => Some(id), + GuardianApprovalRequest::NetworkAccess { .. } => None, #[cfg(unix)] - GuardianApprovalRequest::Execve { id, .. } => id, + GuardianApprovalRequest::Execve { id, .. } => Some(id), } } diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 3d3bf788c..5bd5af133 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -18,6 +18,7 @@ mod review_session; use std::time::Duration; +use codex_protocol::protocol::GuardianAssessmentDecisionSource; use serde::Deserialize; use serde::Serialize; @@ -26,6 +27,7 @@ pub(crate) use approval_request::GuardianMcpAnnotations; pub(crate) use approval_request::guardian_approval_request_to_json; pub(crate) use review::guardian_rejection_message; pub(crate) use review::is_guardian_reviewer_source; +pub(crate) use review::new_guardian_review_id; pub(crate) use review::review_approval_request; pub(crate) use review::review_approval_request_with_cancel; pub(crate) use review::routes_approval_to_guardian; @@ -59,6 +61,12 @@ pub(crate) struct GuardianAssessment { pub(crate) rationale: String, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct GuardianRejection { + pub(crate) rationale: String, + pub(crate) source: GuardianAssessmentDecisionSource, +} + #[cfg(test)] use approval_request::format_guardian_action_pretty; #[cfg(test)] diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 320ef8efe..a80a1a83c 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::GuardianAssessmentDecisionSource; use codex_protocol::protocol::GuardianAssessmentEvent; use codex_protocol::protocol::GuardianAssessmentStatus; use codex_protocol::protocol::GuardianRiskLevel; @@ -19,8 +20,9 @@ use super::GUARDIAN_REVIEWER_NAME; use super::GuardianApprovalRequest; use super::GuardianAssessment; use super::GuardianAssessmentOutcome; +use super::GuardianRejection; use super::approval_request::guardian_assessment_action; -use super::approval_request::guardian_request_id; +use super::approval_request::guardian_request_target_item_id; use super::approval_request::guardian_request_turn_id; use super::prompt::guardian_output_schema; use super::prompt::parse_guardian_assessment; @@ -36,20 +38,29 @@ const GUARDIAN_REJECTION_INSTRUCTIONS: &str = concat!( "Otherwise, stop and request user input.", ); -pub(crate) async fn guardian_rejection_message(session: &Session, assessment_id: &str) -> String { - let rationale = session +pub(crate) fn new_guardian_review_id() -> String { + uuid::Uuid::new_v4().to_string() +} + +pub(crate) async fn guardian_rejection_message(session: &Session, review_id: &str) -> String { + let rejection = session .services - .guardian_rejection_rationales + .guardian_rejections .lock() .await - .remove(assessment_id) - .filter(|rationale| !rationale.trim().is_empty()) - .unwrap_or_else(|| "Guardian denied the action without a specific rationale.".to_string()); - format!( - "This action was rejected due to unacceptable risk.\nReason: {}\n{}", - rationale.trim(), - GUARDIAN_REJECTION_INSTRUCTIONS - ) + .remove(review_id) + .filter(|rejection| !rejection.rationale.trim().is_empty()) + .unwrap_or_else(|| GuardianRejection { + rationale: "Guardian denied the action without a specific rationale.".to_string(), + source: GuardianAssessmentDecisionSource::Agent, + }); + match rejection.source { + GuardianAssessmentDecisionSource::Agent => format!( + "This action was rejected due to unacceptable risk.\nReason: {}\n{}", + rejection.rationale.trim(), + GUARDIAN_REJECTION_INSTRUCTIONS + ), + } } #[derive(Debug)] @@ -91,23 +102,26 @@ pub(crate) fn is_guardian_reviewer_source( async fn run_guardian_review( session: Arc, turn: Arc, + review_id: String, request: GuardianApprovalRequest, retry_reason: Option, external_cancel: Option, ) -> ReviewDecision { - let assessment_id = guardian_request_id(&request).to_string(); + let target_item_id = guardian_request_target_item_id(&request).map(str::to_string); let assessment_turn_id = guardian_request_turn_id(&request, &turn.sub_id).to_string(); let action_summary = guardian_assessment_action(&request); session .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { - id: assessment_id.clone(), + id: review_id.clone(), + target_item_id: target_item_id.clone(), turn_id: assessment_turn_id.clone(), status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, rationale: None, + decision_source: None, action: action_summary.clone(), }), ) @@ -121,12 +135,14 @@ async fn run_guardian_review( .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { - id: assessment_id, + id: review_id, + target_item_id, turn_id: assessment_turn_id, status: GuardianAssessmentStatus::Aborted, risk_level: None, user_authorization: None, rationale: None, + decision_source: Some(GuardianAssessmentDecisionSource::Agent), action: action_summary, }), ) @@ -167,12 +183,14 @@ async fn run_guardian_review( .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { - id: assessment_id, + id: review_id, + target_item_id, turn_id: assessment_turn_id, status: GuardianAssessmentStatus::Aborted, risk_level: None, user_authorization: None, rationale: None, + decision_source: Some(GuardianAssessmentDecisionSource::Agent), action: action_summary, }), ) @@ -209,23 +227,29 @@ async fn run_guardian_review( GuardianAssessmentStatus::Denied }; { - let mut rationales = session.services.guardian_rejection_rationales.lock().await; + let mut rationales = session.services.guardian_rejections.lock().await; if approved { - rationales.remove(&assessment_id); + rationales.remove(&review_id); } else { - rationales.insert(assessment_id.clone(), assessment.rationale.clone()); + let rejection = GuardianRejection { + rationale: assessment.rationale.clone(), + source: GuardianAssessmentDecisionSource::Agent, + }; + rationales.insert(review_id.clone(), rejection); } } session .send_event( turn.as_ref(), EventMsg::GuardianAssessment(GuardianAssessmentEvent { - id: assessment_id, + id: review_id, + target_item_id, turn_id: assessment_turn_id, status, risk_level: Some(assessment.risk_level), user_authorization: Some(assessment.user_authorization), rationale: Some(assessment.rationale.clone()), + decision_source: Some(GuardianAssessmentDecisionSource::Agent), action: terminal_action, }), ) @@ -242,12 +266,14 @@ async fn run_guardian_review( pub(crate) async fn review_approval_request( session: &Arc, turn: &Arc, + review_id: String, request: GuardianApprovalRequest, retry_reason: Option, ) -> ReviewDecision { run_guardian_review( Arc::clone(session), Arc::clone(turn), + review_id, request, retry_reason, /*external_cancel*/ None, @@ -258,6 +284,7 @@ pub(crate) async fn review_approval_request( pub(crate) async fn review_approval_request_with_cancel( session: &Arc, turn: &Arc, + review_id: String, request: GuardianApprovalRequest, retry_reason: Option, cancel_token: CancellationToken, @@ -265,6 +292,7 @@ pub(crate) async fn review_approval_request_with_cancel( run_guardian_review( Arc::clone(session), Arc::clone(turn), + review_id, request, retry_reason, Some(cancel_token), diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 84c8bad1f..67c30c2bf 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -683,6 +683,7 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() { let decision = review_approval_request_with_cancel( &session, &turn, + "review-cancelled-guardian".to_string(), GuardianApprovalRequest::ApplyPatch { id: "patch-1".to_string(), cwd: PathBuf::from("/tmp"), @@ -1248,6 +1249,7 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() -> let decision = review_approval_request( &session, &turn, + "review-shell-guardian-error".to_string(), GuardianApprovalRequest::Shell { id: "shell-guardian-error".to_string(), command: vec!["git".to_string(), "push".to_string()], @@ -1295,8 +1297,13 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() -> }), "denial rationale should not fall back to the generic missing payload error" ); + { + let rationales = session.services.guardian_rejections.lock().await; + assert!(rationales.contains_key("review-shell-guardian-error")); + assert!(!rationales.contains_key("shell-guardian-error")); + } let rejection_message = - guardian_rejection_message(session.as_ref(), "shell-guardian-error").await; + guardian_rejection_message(session.as_ref(), "review-shell-guardian-error").await; assert!( rejection_message.contains("Reason: Automatic approval review failed:") && rejection_message.contains(error_message), @@ -1375,7 +1382,14 @@ async fn guardian_parallel_reviews_fork_from_last_committed_trunk_history() -> a justification: Some("Inspect repo state before proceeding.".to_string()), }; assert_eq!( - review_approval_request(&session, &turn, initial_request, /*retry_reason*/ None).await, + review_approval_request( + &session, + &turn, + "review-shell-guardian-1".to_string(), + initial_request, + /*retry_reason*/ None + ) + .await, ReviewDecision::Approved ); session @@ -1427,6 +1441,7 @@ async fn guardian_parallel_reviews_fork_from_last_committed_trunk_history() -> a review_approval_request( &session_for_second, &turn_for_second, + "review-shell-guardian-2".to_string(), second_request, Some("trunk follow-up".to_string()), ) @@ -1475,6 +1490,7 @@ async fn guardian_parallel_reviews_fork_from_last_committed_trunk_history() -> a let third_decision = review_approval_request( &session, &turn, + "review-shell-guardian-3".to_string(), third_request, Some("parallel follow-up".to_string()), ) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 46654e35f..e038f6c1c 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -24,6 +24,7 @@ use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianMcpAnnotations; use crate::guardian::guardian_approval_request_to_json; use crate::guardian::guardian_rejection_message; +use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian; use crate::mcp_openai_file::rewrite_mcp_tool_arguments_for_openai_files; @@ -772,14 +773,16 @@ async fn maybe_request_mcp_tool_approval( .enabled(Feature::ToolCallMcpElicitation); if routes_approval_to_guardian(turn_context) { + let review_id = new_guardian_review_id(); let decision = review_approval_request( sess, turn_context, + review_id.clone(), build_guardian_mcp_tool_review_request(call_id, invocation, metadata), monitor_reason.clone(), ) .await; - let decision = mcp_tool_approval_decision_from_guardian(sess, call_id, decision).await; + let decision = mcp_tool_approval_decision_from_guardian(sess, &review_id, decision).await; apply_mcp_tool_approval_decision( sess, turn_context, @@ -969,7 +972,7 @@ pub(crate) fn build_guardian_mcp_tool_review_request( async fn mcp_tool_approval_decision_from_guardian( sess: &Session, - call_id: &str, + review_id: &str, decision: ReviewDecision, ) -> McpToolApprovalDecision { match decision { @@ -978,7 +981,7 @@ async fn mcp_tool_approval_decision_from_guardian( | ReviewDecision::NetworkPolicyAmendment { .. } => McpToolApprovalDecision::Accept, ReviewDecision::ApprovedForSession => McpToolApprovalDecision::AcceptForSession, ReviewDecision::Denied => McpToolApprovalDecision::Decline { - message: Some(guardian_rejection_message(sess, call_id).await), + message: Some(guardian_rejection_message(sess, review_id).await), }, ReviewDecision::Abort => McpToolApprovalDecision::Decline { message: None }, } diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 92913ffe2..af99b43c5 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -816,21 +816,22 @@ async fn guardian_review_decision_maps_to_mcp_tool_decision() { assert_eq!( mcp_tool_approval_decision_from_guardian( session.as_ref(), - "approval-id", + "review-id", ReviewDecision::Approved ) .await, McpToolApprovalDecision::Accept ); - session - .services - .guardian_rejection_rationales - .lock() - .await - .insert("approval-id".to_string(), "too risky".to_string()); + session.services.guardian_rejections.lock().await.insert( + "review-id".to_string(), + crate::guardian::GuardianRejection { + rationale: "too risky".to_string(), + source: codex_protocol::protocol::GuardianAssessmentDecisionSource::Agent, + }, + ); let denial = mcp_tool_approval_decision_from_guardian( session.as_ref(), - "approval-id", + "review-id", ReviewDecision::Denied, ) .await; @@ -845,7 +846,7 @@ async fn guardian_review_decision_maps_to_mcp_tool_decision() { assert_eq!( mcp_tool_approval_decision_from_guardian( session.as_ref(), - "approval-id", + "review-id", ReviewDecision::Abort ) .await, diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index b64a3b6d6..2e89585f9 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -7,6 +7,7 @@ use crate::agent::AgentControl; use crate::client::ModelClient; use crate::config::StartedNetworkProxy; use crate::exec_policy::ExecPolicyManager; +use crate::guardian::GuardianRejection; use crate::mcp::McpManager; use crate::plugins::PluginsManager; use crate::skills_watcher::SkillsWatcher; @@ -47,7 +48,7 @@ pub(crate) struct SessionServices { pub(crate) models_manager: Arc, pub(crate) session_telemetry: SessionTelemetry, pub(crate) tool_approvals: Mutex, - pub(crate) guardian_rejection_rationales: Mutex>, + pub(crate) guardian_rejections: Mutex>, pub(crate) skills_manager: Arc, pub(crate) plugins_manager: Arc, pub(crate) mcp_manager: Arc, diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index ffa536c39..58467357c 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -1,6 +1,7 @@ use crate::codex::Session; use crate::guardian::GuardianApprovalRequest; use crate::guardian::guardian_rejection_message; +use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian; use crate::network_policy_decision::denied_network_policy_message; @@ -369,12 +370,13 @@ impl NetworkApprovalService { }; let owner_call = self.resolve_single_active_call().await; let guardian_approval_id = Self::approval_id_for_key(&key); - 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. + let use_guardian = routes_approval_to_guardian(&turn_context); + let guardian_review_id = use_guardian.then(new_guardian_review_id); + let approval_decision = if let Some(review_id) = guardian_review_id.clone() { review_approval_request( &session, &turn_context, + review_id, GuardianApprovalRequest::NetworkAccess { id: guardian_approval_id.clone(), turn_id: owner_call @@ -487,11 +489,9 @@ impl NetworkApprovalService { } }, ReviewDecision::Denied | ReviewDecision::Abort => { - if routes_approval_to_guardian(&turn_context) { + if let Some(review_id) = guardian_review_id.as_deref() { if let Some(owner_call) = owner_call.as_ref() { - let message = - guardian_rejection_message(session.as_ref(), &guardian_approval_id) - .await; + let message = guardian_rejection_message(session.as_ref(), review_id).await; self.record_call_outcome( &owner_call.registration_id, NetworkApprovalOutcome::DeniedByPolicy(message), diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index c05009ff1..fed1ce8b2 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -7,6 +7,7 @@ retry with an escalated sandbox strategy on denial (no re‑approval thanks to caching). */ use crate::guardian::guardian_rejection_message; +use crate::guardian::new_guardian_review_id; use crate::guardian::routes_approval_to_guardian; use crate::network_policy_decision::network_approval_context_from_payload; use crate::tools::network_approval::DeferredNetworkApproval; @@ -115,6 +116,7 @@ impl ToolOrchestrator { let otel_user = ToolDecisionSource::User; let otel_automated_reviewer = ToolDecisionSource::AutomatedReviewer; let otel_cfg = ToolDecisionSource::Config; + let use_guardian = routes_approval_to_guardian(turn_ctx); // 1) Approval let mut already_approved = false; @@ -130,15 +132,17 @@ impl ToolOrchestrator { return Err(ToolError::Rejected(reason)); } ExecApprovalRequirement::NeedsApproval { reason, .. } => { + let guardian_review_id = use_guardian.then(new_guardian_review_id); let approval_ctx = ApprovalCtx { session: &tool_ctx.session, turn: &tool_ctx.turn, call_id: &tool_ctx.call_id, + guardian_review_id: guardian_review_id.clone(), retry_reason: reason, network_approval_context: None, }; let decision = tool.start_approval_async(req, approval_ctx).await; - let otel_source = if routes_approval_to_guardian(turn_ctx) { + let otel_source = if use_guardian { otel_automated_reviewer.clone() } else { otel_user.clone() @@ -148,9 +152,8 @@ impl ToolOrchestrator { match decision { ReviewDecision::Denied | ReviewDecision::Abort => { - let reason = if routes_approval_to_guardian(turn_ctx) { - guardian_rejection_message(tool_ctx.session.as_ref(), &tool_ctx.call_id) - .await + let reason = if let Some(review_id) = guardian_review_id.as_deref() { + guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await } else { "rejected by user".to_string() }; @@ -284,16 +287,18 @@ impl ToolOrchestrator { .should_bypass_approval(approval_policy, already_approved) && network_approval_context.is_none(); if !bypass_retry_approval { + let guardian_review_id = use_guardian.then(new_guardian_review_id); let approval_ctx = ApprovalCtx { session: &tool_ctx.session, turn: &tool_ctx.turn, call_id: &tool_ctx.call_id, + guardian_review_id: guardian_review_id.clone(), retry_reason: Some(retry_reason), network_approval_context: network_approval_context.clone(), }; let decision = tool.start_approval_async(req, approval_ctx).await; - let otel_source = if routes_approval_to_guardian(turn_ctx) { + let otel_source = if use_guardian { otel_automated_reviewer } else { otel_user @@ -302,12 +307,9 @@ impl ToolOrchestrator { match decision { ReviewDecision::Denied | ReviewDecision::Abort => { - let reason = if routes_approval_to_guardian(turn_ctx) { - guardian_rejection_message( - tool_ctx.session.as_ref(), - &tool_ctx.call_id, - ) - .await + let reason = if let Some(review_id) = guardian_review_id.as_deref() { + guardian_rejection_message(tool_ctx.session.as_ref(), review_id) + .await } else { "rejected by user".to_string() }; diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index b2b7a73f8..51f0cc83d 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -8,7 +8,6 @@ use crate::exec::ExecCapturePolicy; use crate::guardian::GuardianApprovalRequest; use crate::guardian::review_approval_request; -use crate::guardian::routes_approval_to_guardian; use crate::sandboxing::ExecOptions; use crate::sandboxing::execute_env; use crate::tools::sandboxing::Approvable; @@ -147,13 +146,15 @@ impl Approvable for ApplyPatchRuntime { let retry_reason = ctx.retry_reason.clone(); let approval_keys = self.approval_keys(req); let changes = req.changes.clone(); + let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { if req.permissions_preapproved && retry_reason.is_none() { return ReviewDecision::Approved; } - if routes_approval_to_guardian(turn) { + if let Some(review_id) = guardian_review_id { let action = ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id); - return review_approval_request(session, turn, action, retry_reason).await; + return review_approval_request(session, turn, review_id, action, retry_reason) + .await; } if let Some(reason) = retry_reason { let rx_approve = session diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 4ff04b002..6f1c13e00 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -12,7 +12,6 @@ use crate::command_canonicalization::canonicalize_command_for_approval; use crate::exec::ExecCapturePolicy; use crate::guardian::GuardianApprovalRequest; use crate::guardian::review_approval_request; -use crate::guardian::routes_approval_to_guardian; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::sandboxing::execute_env; @@ -152,11 +151,13 @@ impl Approvable for ShellRuntime { let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); + let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { - if routes_approval_to_guardian(turn) { + if let Some(review_id) = guardian_review_id { return review_approval_request( session, turn, + review_id, GuardianApprovalRequest::Shell { id: call_id, command, 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 47d1b6253..41195876f 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -4,6 +4,7 @@ use crate::exec::ExecExpiration; use crate::exec::is_likely_sandbox_denied; use crate::guardian::GuardianApprovalRequest; use crate::guardian::guardian_rejection_message; +use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian; use crate::sandboxing::ExecOptions; @@ -315,6 +316,11 @@ enum DecisionSource { UnmatchedCommandFallback, } +struct PromptDecision { + decision: ReviewDecision, + guardian_review_id: Option, +} + fn execve_prompt_is_rejected_by_policy( approval_policy: AskForApproval, decision_source: &DecisionSource, @@ -376,7 +382,7 @@ impl CoreShellActionProvider { workdir: &AbsolutePathBuf, stopwatch: &Stopwatch, additional_permissions: Option, - ) -> anyhow::Result { + ) -> anyhow::Result { let command = join_program_and_argv(program, argv); let workdir = workdir.to_path_buf(); let session = self.session.clone(); @@ -384,12 +390,14 @@ impl CoreShellActionProvider { let call_id = self.call_id.clone(); let approval_id = Some(Uuid::new_v4().to_string()); let source = self.tool_name; + let guardian_review_id = routes_approval_to_guardian(&turn).then(new_guardian_review_id); Ok(stopwatch .pause_for(async move { - if routes_approval_to_guardian(&turn) { - return review_approval_request( + if let Some(review_id) = guardian_review_id.clone() { + let decision = review_approval_request( &session, &turn, + review_id, GuardianApprovalRequest::Execve { id: call_id.clone(), source, @@ -401,8 +409,12 @@ impl CoreShellActionProvider { /*retry_reason*/ None, ) .await; + return PromptDecision { + decision, + guardian_review_id, + }; } - session + let decision = session .request_command_approval( &turn, call_id, @@ -415,7 +427,11 @@ impl CoreShellActionProvider { additional_permissions, Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), ) - .await + .await; + PromptDecision { + decision, + guardian_review_id: None, + } }) .await) } @@ -442,10 +458,10 @@ impl CoreShellActionProvider { { EscalationDecision::deny(Some("Execution forbidden by policy".to_string())) } else { - match self + let prompt_decision = self .prompt(program, argv, workdir, &self.stopwatch, prompt_permissions) - .await? - { + .await?; + match prompt_decision.decision { ReviewDecision::Approved | ReviewDecision::ApprovedForSession | ReviewDecision::ApprovedExecpolicyAmendment { .. } => { @@ -470,9 +486,10 @@ impl CoreShellActionProvider { } }, ReviewDecision::Denied => { - let message = if routes_approval_to_guardian(&self.turn) { - guardian_rejection_message(self.session.as_ref(), &self.call_id) - .await + let message = if let Some(review_id) = + prompt_decision.guardian_review_id.as_deref() + { + guardian_rejection_message(self.session.as_ref(), review_id).await } else { "User denied execution".to_string() }; diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index fd12607c1..94d370fbc 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -9,7 +9,6 @@ use crate::exec::ExecCapturePolicy; use crate::exec::ExecExpiration; use crate::guardian::GuardianApprovalRequest; use crate::guardian::review_approval_request; -use crate::guardian::routes_approval_to_guardian; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; @@ -128,11 +127,13 @@ impl Approvable for UnifiedExecRuntime<'_> { let cwd = req.cwd.to_path_buf(); let retry_reason = ctx.retry_reason.clone(); let reason = retry_reason.clone().or_else(|| req.justification.clone()); + let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { - if routes_approval_to_guardian(turn) { + if let Some(review_id) = guardian_review_id { return review_approval_request( session, turn, + review_id, GuardianApprovalRequest::ExecCommand { id: call_id, command, diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index b1ce3e7d9..f5ceca5c2 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -120,6 +120,13 @@ pub(crate) struct ApprovalCtx<'a> { pub session: &'a Arc, pub turn: &'a Arc, pub call_id: &'a str, + /// Guardian review lifecycle ID for this approval, when guardian is reviewing it. + /// + /// This is separate from `call_id`: `call_id` identifies the tool item under + /// review, while this ID identifies the review itself. Keeping both lets + /// denial handling, overrides, and app-server notifications refer to the + /// review without overloading the tool call ID as a review ID. + pub guardian_review_id: Option, pub retry_reason: Option, pub network_approval_context: Option, } diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 557f3856f..cde6277a5 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -108,6 +108,12 @@ pub enum GuardianAssessmentStatus { Aborted, } +#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "snake_case")] +pub enum GuardianAssessmentDecisionSource { + Agent, +} + #[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "snake_case")] pub enum GuardianCommandSource { @@ -159,6 +165,10 @@ pub struct NetworkPolicyAmendment { pub struct GuardianAssessmentEvent { /// Stable identifier for this guardian review lifecycle. pub id: String, + /// Thread item being reviewed, when the review maps to a concrete item. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub target_item_id: Option, /// Turn ID that this assessment belongs to. /// Uses `#[serde(default)]` for backwards compatibility. #[serde(default)] @@ -176,6 +186,10 @@ pub struct GuardianAssessmentEvent { #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional)] pub rationale: Option, + /// Source that produced the terminal assessment decision. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub decision_source: Option, /// Canonical action payload that was reviewed. pub action: GuardianAssessmentAction, } diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 54eb8e8bd..337b2bd7f 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -65,6 +65,7 @@ pub use crate::approvals::ElicitationAction; pub use crate::approvals::ExecApprovalRequestEvent; pub use crate::approvals::ExecPolicyAmendment; pub use crate::approvals::GuardianAssessmentAction; +pub use crate::approvals::GuardianAssessmentDecisionSource; pub use crate::approvals::GuardianAssessmentEvent; pub use crate::approvals::GuardianAssessmentStatus; pub use crate::approvals::GuardianCommandSource; diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 47733c725..052ce6898 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -174,6 +174,7 @@ use codex_protocol::protocol::ExecCommandSource; #[cfg(test)] use codex_protocol::protocol::ExitedReviewModeEvent; use codex_protocol::protocol::GuardianAssessmentAction; +use codex_protocol::protocol::GuardianAssessmentDecisionSource; use codex_protocol::protocol::GuardianAssessmentEvent; use codex_protocol::protocol::GuardianAssessmentStatus; use codex_protocol::protocol::ImageGenerationBeginEvent; @@ -6682,17 +6683,19 @@ impl ChatWidget { } ServerNotification::ItemGuardianApprovalReviewStarted(notification) => { self.on_guardian_review_notification( - notification.target_item_id, + notification.review_id, notification.turn_id, notification.review, + /*decision_source*/ None, notification.action, ); } ServerNotification::ItemGuardianApprovalReviewCompleted(notification) => { self.on_guardian_review_notification( - notification.target_item_id, + notification.review_id, notification.turn_id, notification.review, + Some(notification.decision_source), notification.action, ); } @@ -6975,10 +6978,12 @@ impl ChatWidget { id: String, turn_id: String, review: codex_app_server_protocol::GuardianApprovalReview, + decision_source: Option, action: GuardianApprovalReviewAction, ) { self.on_guardian_assessment(GuardianAssessmentEvent { id, + target_item_id: None, turn_id, status: match review.status { codex_app_server_protocol::GuardianApprovalReviewStatus::InProgress => { @@ -7025,6 +7030,11 @@ impl ChatWidget { } }), rationale: review.rationale, + decision_source: decision_source.map(|source| match source { + codex_app_server_protocol::AutoReviewDecisionSource::Agent => { + GuardianAssessmentDecisionSource::Agent + } + }), action: action.into(), }); } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 7fbcb90b9..245c26b0c 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -24,6 +24,7 @@ pub(super) use codex_app_server_protocol::AdditionalFileSystemPermissions as App pub(super) use codex_app_server_protocol::AdditionalNetworkPermissions as AppServerAdditionalNetworkPermissions; pub(super) use codex_app_server_protocol::AdditionalPermissionProfile as AppServerAdditionalPermissionProfile; pub(super) use codex_app_server_protocol::AppSummary; +pub(super) use codex_app_server_protocol::AutoReviewDecisionSource as AppServerGuardianApprovalReviewDecisionSource; pub(super) use codex_app_server_protocol::CollabAgentState as AppServerCollabAgentState; pub(super) use codex_app_server_protocol::CollabAgentStatus as AppServerCollabAgentStatus; pub(super) use codex_app_server_protocol::CollabAgentTool as AppServerCollabAgentTool; @@ -150,6 +151,7 @@ pub(super) use codex_protocol::protocol::ExecPolicyAmendment; pub(super) use codex_protocol::protocol::ExitedReviewModeEvent; pub(super) use codex_protocol::protocol::FileChange; pub(super) use codex_protocol::protocol::GuardianAssessmentAction; +pub(super) use codex_protocol::protocol::GuardianAssessmentDecisionSource; pub(super) use codex_protocol::protocol::GuardianAssessmentEvent; pub(super) use codex_protocol::protocol::GuardianAssessmentStatus; pub(super) use codex_protocol::protocol::GuardianCommandSource; diff --git a/codex-rs/tui/src/chatwidget/tests/guardian.rs b/codex-rs/tui/src/chatwidget/tests/guardian.rs index b87f0dbc8..b93a0ae3b 100644 --- a/codex-rs/tui/src/chatwidget/tests/guardian.rs +++ b/codex-rs/tui/src/chatwidget/tests/guardian.rs @@ -16,11 +16,13 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { id: "guardian-in-progress".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".into(), + target_item_id: Some("guardian-target-1".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, rationale: None, + decision_source: None, action: action.clone(), }), }); @@ -34,11 +36,13 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() { id: "guardian-assessment".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "guardian-1".into(), + target_item_id: Some("guardian-target-1".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::Denied, risk_level: Some(GuardianRiskLevel::High), user_authorization: Some(GuardianUserAuthorization::Low), rationale: Some("Would exfiltrate local source code.".into()), + decision_source: Some(GuardianAssessmentDecisionSource::Agent), action, }), }); @@ -77,11 +81,13 @@ async fn guardian_approved_exec_renders_approved_request() { id: "guardian-assessment".into(), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: "thread:child-thread:guardian-1".into(), + target_item_id: Some("guardian-approved-target".into()), turn_id: "turn-1".into(), status: GuardianAssessmentStatus::Approved, risk_level: Some(GuardianRiskLevel::Low), user_authorization: Some(GuardianUserAuthorization::High), rationale: Some("Narrowly scoped to the requested file.".into()), + decision_source: Some(GuardianAssessmentDecisionSource::Agent), action: GuardianAssessmentAction::Command { source: GuardianCommandSource::Shell, command: "rm -f /tmp/guardian-approved.sqlite".to_string(), @@ -130,7 +136,8 @@ async fn app_server_guardian_review_started_sets_review_status() { ItemGuardianApprovalReviewStartedNotification { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), - target_item_id: "guardian-1".to_string(), + review_id: "guardian-1".to_string(), + target_item_id: Some("guardian-target-1".to_string()), review: GuardianApprovalReview { status: GuardianApprovalReviewStatus::InProgress, risk_level: None, @@ -170,7 +177,8 @@ async fn app_server_guardian_review_denied_renders_denied_request_snapshot() { ItemGuardianApprovalReviewStartedNotification { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), - target_item_id: "guardian-1".to_string(), + review_id: "guardian-1".to_string(), + target_item_id: Some("guardian-target-1".to_string()), review: GuardianApprovalReview { status: GuardianApprovalReviewStatus::InProgress, risk_level: None, @@ -188,7 +196,9 @@ async fn app_server_guardian_review_denied_renders_denied_request_snapshot() { ItemGuardianApprovalReviewCompletedNotification { thread_id: "thread-1".to_string(), turn_id: "turn-1".to_string(), - target_item_id: "guardian-1".to_string(), + review_id: "guardian-1".to_string(), + target_item_id: Some("guardian-target-1".to_string()), + decision_source: AppServerGuardianApprovalReviewDecisionSource::Agent, review: GuardianApprovalReview { status: GuardianApprovalReviewStatus::Denied, risk_level: Some(AppServerGuardianRiskLevel::High), @@ -239,11 +249,13 @@ async fn guardian_parallel_reviews_render_aggregate_status_snapshot() { id: format!("event-{id}"), msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent { id: id.to_string(), + target_item_id: Some(format!("{id}-target")), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, rationale: None, + decision_source: None, action: GuardianAssessmentAction::Command { source: GuardianCommandSource::Shell, command: command.to_string(), @@ -269,11 +281,13 @@ 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(), + target_item_id: Some("guardian-1-target".to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, rationale: None, + decision_source: None, action: GuardianAssessmentAction::Command { source: GuardianCommandSource::Shell, command: "rm -rf '/tmp/guardian target 1'".to_string(), @@ -285,11 +299,13 @@ 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(), + target_item_id: Some("guardian-2-target".to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::InProgress, risk_level: None, user_authorization: None, rationale: None, + decision_source: None, action: GuardianAssessmentAction::Command { source: GuardianCommandSource::Shell, command: "rm -rf '/tmp/guardian target 2'".to_string(), @@ -301,11 +317,13 @@ 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(), + target_item_id: Some("guardian-1-target".to_string()), turn_id: "turn-1".to_string(), status: GuardianAssessmentStatus::Denied, risk_level: Some(GuardianRiskLevel::High), user_authorization: Some(GuardianUserAuthorization::Low), rationale: Some("Would delete important data.".to_string()), + decision_source: Some(GuardianAssessmentDecisionSource::Agent), action: GuardianAssessmentAction::Command { source: GuardianCommandSource::Shell, command: "rm -rf '/tmp/guardian target 1'".to_string(),