From 170b58d39e2aa8e0f0feb5094c4943f9d517bd78 Mon Sep 17 00:00:00 2001 From: Jonathan Liebig Date: Sun, 12 Jul 2026 12:48:24 +0200 Subject: [PATCH] feat(hooks): route ask decisions through Guardian --- codex-rs/analytics/src/events.rs | 4 + codex-rs/analytics/src/reducer.rs | 15 +- .../schema/json/ServerNotification.json | 26 +++ .../codex_app_server_protocol.schemas.json | 26 +++ .../codex_app_server_protocol.v2.schemas.json | 26 +++ ...anApprovalReviewCompletedNotification.json | 26 +++ ...dianApprovalReviewStartedNotification.json | 26 +++ .../v2/GuardianApprovalReviewAction.ts | 3 +- .../src/protocol/item_builders.rs | 1 + .../src/protocol/v2/item.rs | 25 +++ codex-rs/app-server/README.md | 2 +- .../core/src/guardian/approval_request.rs | 83 ++++++++- codex-rs/core/src/guardian/metrics.rs | 1 + codex-rs/core/src/guardian/mod.rs | 4 + codex-rs/core/src/guardian/tests.rs | 45 ++++- codex-rs/core/src/hook_runtime.rs | 5 + .../core/src/tools/codex_plus_plus/mod.rs | 1 + .../codex_plus_plus/pre_tool_use_review.rs | 60 ++++++ codex-rs/core/src/tools/handlers/mcp.rs | 11 ++ codex-rs/core/src/tools/mod.rs | 1 + codex-rs/core/src/tools/registry.rs | 30 +++ codex-rs/core/tests/suite/hooks.rs | 118 ++++++++++++ codex-rs/hooks/src/engine/output_parser.rs | 42 ++++- codex-rs/hooks/src/events/pre_tool_use.rs | 175 ++++++++++++++++-- codex-rs/protocol/src/approvals.rs | 5 + codex-rs/tui/src/auto_review_denials.rs | 3 + ...t_permissions_renders_request_summary.snap | 4 +- codex-rs/tui/src/chatwidget/tests/guardian.rs | 17 ++ codex-rs/tui/src/chatwidget/tool_requests.rs | 14 ++ 29 files changed, 771 insertions(+), 28 deletions(-) create mode 100644 codex-rs/core/src/tools/codex_plus_plus/mod.rs create mode 100644 codex-rs/core/src/tools/codex_plus_plus/pre_tool_use_review.rs diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index ad8adf14cbf6..26a4e5540869 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -257,6 +257,9 @@ pub enum GuardianReviewedAction { connector_name: Option, tool_title: Option, }, + PreToolUse { + tool_name: String, + }, RequestPermissions {}, } @@ -551,6 +554,7 @@ pub(crate) enum ReviewSubjectKind { CommandExecution, FileChange, McpToolCall, + ToolCall, Permissions, NetworkAccess, } diff --git a/codex-rs/analytics/src/reducer.rs b/codex-rs/analytics/src/reducer.rs index 79649a553b8e..829f7b8a6d94 100644 --- a/codex-rs/analytics/src/reducer.rs +++ b/codex-rs/analytics/src/reducer.rs @@ -1758,7 +1758,9 @@ fn item_review_summary_key(pending_review: &PendingReviewState) -> Option None, + ReviewSubjectKind::ToolCall + | ReviewSubjectKind::Permissions + | ReviewSubjectKind::NetworkAccess => None, } } @@ -2311,6 +2313,11 @@ fn guardian_review_subject_metadata( tool_name.clone(), ReviewTrigger::Initial, ), + GuardianApprovalReviewAction::PreToolUse { tool_name, .. } => ( + ReviewSubjectKind::ToolCall, + tool_name.clone(), + ReviewTrigger::Initial, + ), } } @@ -2324,7 +2331,8 @@ fn guardian_review_requested_additional_permissions(action: &GuardianApprovalRev } GuardianApprovalReviewAction::Command { .. } | GuardianApprovalReviewAction::Execve { .. } - | GuardianApprovalReviewAction::McpToolCall { .. } => false, + | GuardianApprovalReviewAction::McpToolCall { .. } + | GuardianApprovalReviewAction::PreToolUse { .. } => false, } } @@ -2337,7 +2345,8 @@ fn guardian_review_requested_network_access(action: &GuardianApprovalReviewActio GuardianApprovalReviewAction::ApplyPatch { .. } | GuardianApprovalReviewAction::Command { .. } | GuardianApprovalReviewAction::Execve { .. } - | GuardianApprovalReviewAction::McpToolCall { .. } => false, + | GuardianApprovalReviewAction::McpToolCall { .. } + | GuardianApprovalReviewAction::PreToolUse { .. } => false, } } diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index e025a61a3344..220b5bbd36ff 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -1900,6 +1900,32 @@ "title": "McpToolCallGuardianApprovalReviewAction", "type": "object" }, + { + "properties": { + "reason": { + "type": "string" + }, + "toolInput": true, + "toolName": { + "type": "string" + }, + "type": { + "enum": [ + "preToolUse" + ], + "title": "PreToolUseGuardianApprovalReviewActionType", + "type": "string" + } + }, + "required": [ + "reason", + "toolInput", + "toolName", + "type" + ], + "title": "PreToolUseGuardianApprovalReviewAction", + "type": "object" + }, { "properties": { "permissions": { 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 b7526b080da1..c0332b1368a6 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 @@ -10675,6 +10675,32 @@ "title": "McpToolCallGuardianApprovalReviewAction", "type": "object" }, + { + "properties": { + "reason": { + "type": "string" + }, + "toolInput": true, + "toolName": { + "type": "string" + }, + "type": { + "enum": [ + "preToolUse" + ], + "title": "PreToolUseGuardianApprovalReviewActionType", + "type": "string" + } + }, + "required": [ + "reason", + "toolInput", + "toolName", + "type" + ], + "title": "PreToolUseGuardianApprovalReviewAction", + "type": "object" + }, { "properties": { "permissions": { 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 e1afc7883a51..363500e4280f 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 @@ -7026,6 +7026,32 @@ "title": "McpToolCallGuardianApprovalReviewAction", "type": "object" }, + { + "properties": { + "reason": { + "type": "string" + }, + "toolInput": true, + "toolName": { + "type": "string" + }, + "type": { + "enum": [ + "preToolUse" + ], + "title": "PreToolUseGuardianApprovalReviewActionType", + "type": "string" + } + }, + "required": [ + "reason", + "toolInput", + "toolName", + "type" + ], + "title": "PreToolUseGuardianApprovalReviewAction", + "type": "object" + }, { "properties": { "permissions": { 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 48a3193cc735..a69b1636ff67 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json @@ -467,6 +467,32 @@ "title": "McpToolCallGuardianApprovalReviewAction", "type": "object" }, + { + "properties": { + "reason": { + "type": "string" + }, + "toolInput": true, + "toolName": { + "type": "string" + }, + "type": { + "enum": [ + "preToolUse" + ], + "title": "PreToolUseGuardianApprovalReviewActionType", + "type": "string" + } + }, + "required": [ + "reason", + "toolInput", + "toolName", + "type" + ], + "title": "PreToolUseGuardianApprovalReviewAction", + "type": "object" + }, { "properties": { "permissions": { 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 9f534e517d2c..8234b63d1f92 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json @@ -460,6 +460,32 @@ "title": "McpToolCallGuardianApprovalReviewAction", "type": "object" }, + { + "properties": { + "reason": { + "type": "string" + }, + "toolInput": true, + "toolName": { + "type": "string" + }, + "type": { + "enum": [ + "preToolUse" + ], + "title": "PreToolUseGuardianApprovalReviewActionType", + "type": "string" + } + }, + "required": [ + "reason", + "toolInput", + "toolName", + "type" + ], + "title": "PreToolUseGuardianApprovalReviewAction", + "type": "object" + }, { "properties": { "permissions": { diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/GuardianApprovalReviewAction.ts b/codex-rs/app-server-protocol/schema/typescript/v2/GuardianApprovalReviewAction.ts index 4f00e37d20e8..9b2522f07174 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/GuardianApprovalReviewAction.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/GuardianApprovalReviewAction.ts @@ -2,8 +2,9 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { AbsolutePathBuf } from "../AbsolutePathBuf"; +import type { JsonValue } from "../serde_json/JsonValue"; import type { GuardianCommandSource } from "./GuardianCommandSource"; import type { NetworkApprovalProtocol } from "./NetworkApprovalProtocol"; import type { RequestPermissionProfile } from "./RequestPermissionProfile"; -export type GuardianApprovalReviewAction = { "type": "command", source: GuardianCommandSource, command: string, cwd: AbsolutePathBuf, } | { "type": "execve", source: GuardianCommandSource, program: string, argv: Array, cwd: AbsolutePathBuf, } | { "type": "applyPatch", cwd: AbsolutePathBuf, files: Array, } | { "type": "networkAccess", target: string, host: string, protocol: NetworkApprovalProtocol, port: number, } | { "type": "mcpToolCall", server: string, toolName: string, connectorId: string | null, connectorName: string | null, toolTitle: string | null, } | { "type": "requestPermissions", reason: string | null, permissions: RequestPermissionProfile, }; +export type GuardianApprovalReviewAction = { "type": "command", source: GuardianCommandSource, command: string, cwd: AbsolutePathBuf, } | { "type": "execve", source: GuardianCommandSource, program: string, argv: Array, cwd: AbsolutePathBuf, } | { "type": "applyPatch", cwd: AbsolutePathBuf, files: Array, } | { "type": "networkAccess", target: string, host: string, protocol: NetworkApprovalProtocol, port: number, } | { "type": "mcpToolCall", server: string, toolName: string, connectorId: string | null, connectorName: string | null, toolTitle: string | null, } | { "type": "preToolUse", toolName: string, toolInput: JsonValue, reason: string, } | { "type": "requestPermissions", reason: string | null, permissions: RequestPermissionProfile, }; 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 751bf4f67b71..391d4a7627e1 100644 --- a/codex-rs/app-server-protocol/src/protocol/item_builders.rs +++ b/codex-rs/app-server-protocol/src/protocol/item_builders.rs @@ -248,6 +248,7 @@ pub fn build_item_from_guardian_event( GuardianAssessmentAction::ApplyPatch { .. } | GuardianAssessmentAction::NetworkAccess { .. } | GuardianAssessmentAction::McpToolCall { .. } + | GuardianAssessmentAction::PreToolUse { .. } | GuardianAssessmentAction::RequestPermissions { .. } => None, } } diff --git a/codex-rs/app-server-protocol/src/protocol/v2/item.rs b/codex-rs/app-server-protocol/src/protocol/v2/item.rs index 7a5e4dc1f45c..db22d3d733d3 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/item.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/item.rs @@ -653,6 +653,13 @@ pub enum GuardianApprovalReviewAction { }, #[serde(rename_all = "camelCase")] #[ts(rename_all = "camelCase")] + PreToolUse { + tool_name: String, + tool_input: JsonValue, + reason: String, + }, + #[serde(rename_all = "camelCase")] + #[ts(rename_all = "camelCase")] RequestPermissions { reason: Option, permissions: RequestPermissionProfile, @@ -709,6 +716,15 @@ impl From for GuardianApprovalReviewAction { connector_name, tool_title, }, + CoreGuardianAssessmentAction::PreToolUse { + tool_name, + tool_input, + reason, + } => Self::PreToolUse { + tool_name, + tool_input, + reason, + }, CoreGuardianAssessmentAction::RequestPermissions { reason, permissions, @@ -772,6 +788,15 @@ impl TryFrom for CoreGuardianAssessmentAction { connector_name, tool_title, }, + GuardianApprovalReviewAction::PreToolUse { + tool_name, + tool_input, + reason, + } => Self::PreToolUse { + tool_name, + tool_input, + reason, + }, GuardianApprovalReviewAction::RequestPermissions { reason, permissions, diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 6ee6baf2dc6c..401d78e0f367 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -1390,7 +1390,7 @@ All items emit shared lifecycle events: - `item/autoApprovalReview/started` — [UNSTABLE] temporary auto-review notification carrying `{threadId, turnId, targetItemId, review, action}` when approval auto-review begins. This shape is expected to change soon. - `item/autoApprovalReview/completed` — [UNSTABLE] temporary auto-review notification carrying `{threadId, turnId, targetItemId, review, action}` when approval auto-review resolves. This shape is expected to change soon. -`review` is [UNSTABLE] and currently has `{status, riskLevel?, userAuthorization?, rationale?}`, where `status` is one of `inProgress`, `approved`, `denied`, or `aborted`. `riskLevel` is one of `"low"`, `"medium"`, `"high"`, or `"critical"` when present. `userAuthorization` is one of `"unknown"`, `"low"`, `"medium"`, or `"high"` when present. `action` is a tagged union with `type: "command" | "execve" | "applyPatch" | "networkAccess" | "mcpToolCall"`. Command-like actions include a `source` discriminator (`"shell"` or `"unifiedExec"`). These notifications are separate from the target item's own `item/completed` lifecycle and are intentionally temporary while the auto-review app protocol is still being designed. +`review` is [UNSTABLE] and currently has `{status, riskLevel?, userAuthorization?, rationale?}`, where `status` is one of `inProgress`, `approved`, `denied`, or `aborted`. `riskLevel` is one of `"low"`, `"medium"`, `"high"`, or `"critical"` when present. `userAuthorization` is one of `"unknown"`, `"low"`, `"medium"`, or `"high"` when present. `action` is a tagged union with `type: "command" | "execve" | "applyPatch" | "networkAccess" | "mcpToolCall" | "preToolUse"`. Command-like actions include a `source` discriminator (`"shell"` or `"unifiedExec"`). A `preToolUse` action carries `{toolName, toolInput, reason}` and targets the tool call gated by the hook-requested review. These notifications are separate from the target item's own `item/completed` lifecycle and are intentionally temporary while the auto-review app protocol is still being designed. There are additional item-specific events: diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index bd9a0f7c2099..3c35e98bf03e 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -11,6 +11,10 @@ use serde::Serialize; use serde_json::Value; use super::GUARDIAN_MAX_ACTION_STRING_TOKENS; +use super::GUARDIAN_MAX_ACTION_SUMMARY_TOKENS; +use super::GUARDIAN_MAX_ACTION_TOKENS; +use super::GUARDIAN_MAX_ASSESSMENT_INPUT_TOKENS; +use super::GUARDIAN_MAX_ASSESSMENT_REASON_TOKENS; use super::prompt::guardian_truncate_text; #[derive(Debug, Clone, PartialEq)] @@ -69,6 +73,13 @@ pub(crate) enum GuardianApprovalRequest { tool_description: Option, annotations: Option, }, + PreToolUse { + id: String, + tool_name: String, + tool_input: Value, + reason: String, + cwd: AbsolutePathBuf, + }, RequestPermissions { id: String, turn_id: String, @@ -151,6 +162,15 @@ struct McpToolCallApprovalAction<'a> { annotations: Option<&'a GuardianMcpAnnotations>, } +#[derive(Serialize)] +struct PreToolUseApprovalAction<'a> { + tool: &'static str, + tool_name: &'a str, + tool_input: &'a Value, + reason: &'a str, + cwd: &'a Path, +} + #[derive(Serialize)] #[serde(rename_all = "camelCase")] struct NetworkAccessApprovalAction<'a> { @@ -253,6 +273,16 @@ fn truncate_guardian_action_value(value: Value) -> (Value, bool) { } } +fn bounded_guardian_assessment_input(value: &Value) -> Value { + let (summary, truncated) = + guardian_truncate_text(&value.to_string(), GUARDIAN_MAX_ASSESSMENT_INPUT_TOKENS); + if truncated { + Value::String(summary) + } else { + value.clone() + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct FormattedGuardianAction { pub(crate) text: String, @@ -363,6 +393,19 @@ pub(crate) fn guardian_approval_request_to_json( tool_description: tool_description.as_ref(), annotations: annotations.as_ref(), }), + GuardianApprovalRequest::PreToolUse { + id: _, + tool_name, + tool_input, + reason, + cwd, + } => serialize_guardian_action(PreToolUseApprovalAction { + tool: "pre_tool_use", + tool_name, + tool_input, + reason, + cwd, + }), GuardianApprovalRequest::RequestPermissions { id: _, turn_id, @@ -434,6 +477,16 @@ pub(crate) fn guardian_assessment_action( connector_name: connector_name.clone(), tool_title: tool_title.clone(), }, + GuardianApprovalRequest::PreToolUse { + tool_name, + tool_input, + reason, + .. + } => GuardianAssessmentAction::PreToolUse { + tool_name: guardian_truncate_text(tool_name, GUARDIAN_MAX_ASSESSMENT_REASON_TOKENS).0, + tool_input: bounded_guardian_assessment_input(tool_input), + reason: guardian_truncate_text(reason, GUARDIAN_MAX_ASSESSMENT_REASON_TOKENS).0, + }, GuardianApprovalRequest::RequestPermissions { reason, permissions, @@ -499,6 +552,11 @@ pub(crate) fn guardian_reviewed_action( connector_name: connector_name.clone(), tool_title: tool_title.clone(), }, + GuardianApprovalRequest::PreToolUse { tool_name, .. } => { + GuardianReviewedAction::PreToolUse { + tool_name: tool_name.clone(), + } + } GuardianApprovalRequest::RequestPermissions { .. } => { GuardianReviewedAction::RequestPermissions {} } @@ -512,6 +570,9 @@ pub(crate) fn guardian_request_target_item_id(request: &GuardianApprovalRequest) | GuardianApprovalRequest::ApplyPatch { id, .. } | GuardianApprovalRequest::McpToolCall { id, .. } | GuardianApprovalRequest::RequestPermissions { id, .. } => Some(id), + // PreToolUse targets the model tool call even though its ThreadItem is + // emitted only after Guardian allows the handler to run. + GuardianApprovalRequest::PreToolUse { id, .. } => Some(id), GuardianApprovalRequest::NetworkAccess { .. } => None, #[cfg(unix)] GuardianApprovalRequest::Execve { id, .. } => Some(id), @@ -528,7 +589,8 @@ pub(crate) fn guardian_request_turn_id<'a>( GuardianApprovalRequest::Shell { .. } | GuardianApprovalRequest::ExecCommand { .. } | GuardianApprovalRequest::ApplyPatch { .. } - | GuardianApprovalRequest::McpToolCall { .. } => default_turn_id, + | GuardianApprovalRequest::McpToolCall { .. } + | GuardianApprovalRequest::PreToolUse { .. } => default_turn_id, #[cfg(unix)] GuardianApprovalRequest::Execve { .. } => default_turn_id, } @@ -538,9 +600,22 @@ pub(crate) fn format_guardian_action_pretty( action: &GuardianApprovalRequest, ) -> serde_json::Result { let value = guardian_approval_request_to_json(action)?; - let (value, truncated) = truncate_guardian_action_value(value); + let (value, fields_truncated) = truncate_guardian_action_value(value); + let text = serde_json::to_string_pretty(&value)?; + let (_, action_truncated) = guardian_truncate_text(&text, GUARDIAN_MAX_ACTION_TOKENS); + let text = if action_truncated { + let summary = guardian_truncate_text(&text, GUARDIAN_MAX_ACTION_SUMMARY_TOKENS).0; + serde_json::to_string_pretty(&serde_json::json!({ + "tool": value.get("tool"), + "tool_name": value.get("tool_name").map(bounded_guardian_assessment_input), + "reason": value.get("reason").map(bounded_guardian_assessment_input), + "summary": summary, + }))? + } else { + text + }; Ok(FormattedGuardianAction { - text: serde_json::to_string_pretty(&value)?, - truncated, + text, + truncated: fields_truncated || action_truncated, }) } diff --git a/codex-rs/core/src/guardian/metrics.rs b/codex-rs/core/src/guardian/metrics.rs index 9b9d35a5260f..27173f8be352 100644 --- a/codex-rs/core/src/guardian/metrics.rs +++ b/codex-rs/core/src/guardian/metrics.rs @@ -178,6 +178,7 @@ fn reviewed_action_tag(action: &GuardianReviewedAction) -> &'static str { GuardianReviewedAction::ApplyPatch {} => "apply_patch", GuardianReviewedAction::NetworkAccess { .. } => "network_access", GuardianReviewedAction::McpToolCall { .. } => "mcp_tool_call", + GuardianReviewedAction::PreToolUse { .. } => "pre_tool_use", GuardianReviewedAction::RequestPermissions {} => "request_permissions", } } diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index b06a2daad303..303f51f4e32e 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -55,7 +55,11 @@ const GUARDIAN_MAX_MESSAGE_TRANSCRIPT_TOKENS: usize = 10_000; const GUARDIAN_MAX_TOOL_TRANSCRIPT_TOKENS: usize = 10_000; const GUARDIAN_MAX_MESSAGE_ENTRY_TOKENS: usize = 2_000; const GUARDIAN_MAX_TOOL_ENTRY_TOKENS: usize = 1_000; +const GUARDIAN_MAX_ACTION_TOKENS: usize = 1_000; +const GUARDIAN_MAX_ACTION_SUMMARY_TOKENS: usize = 100; const GUARDIAN_MAX_ACTION_STRING_TOKENS: usize = 16_000; +const GUARDIAN_MAX_ASSESSMENT_INPUT_TOKENS: usize = 100; +const GUARDIAN_MAX_ASSESSMENT_REASON_TOKENS: usize = 50; const GUARDIAN_RECENT_ENTRY_LIMIT: usize = 40; const TRUNCATION_TAG: &str = "truncated"; diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index e23475d30e74..efc5ea0039dd 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -29,6 +29,7 @@ use codex_model_provider_info::OPENAI_PROVIDER_ID; use codex_models_manager::manager::StaticModelsManager; use codex_network_proxy::NetworkProxyConfig; use codex_protocol::ThreadId; +use codex_protocol::approvals::GuardianAssessmentAction; use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::ContentItem; @@ -52,6 +53,7 @@ use codex_protocol::protocol::GuardianUserAuthorization; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::TurnCompleteEvent; +use codex_utils_output_truncation::approx_token_count; use core_test_support::PathBufExt; use core_test_support::TempDirExt; use core_test_support::context_snapshot; @@ -912,6 +914,36 @@ fn format_guardian_action_pretty_reports_no_truncation_for_small_payload() -> se Ok(()) } +#[test] +fn format_guardian_action_pretty_caps_aggregate_nested_input() -> serde_json::Result<()> { + let action = GuardianApprovalRequest::PreToolUse { + id: "call-1".to_string(), + tool_name: "nested_tool".to_string(), + tool_input: serde_json::json!({ "values": vec![r#"\"quoted\\path\n"#.repeat(200); 100] }), + reason: "Review this tool call".to_string(), + cwd: test_path_buf("/tmp").abs(), + }; + + let rendered = format_guardian_action_pretty(&action)?; + let assessment = guardian_assessment_action(&action); + assert!( + approx_token_count(&serde_json::to_string(&assessment)?) <= GUARDIAN_MAX_TOOL_ENTRY_TOKENS + ); + let GuardianAssessmentAction::PreToolUse { tool_input, .. } = assessment else { + panic!("expected PreToolUse assessment action"); + }; + + assert!(rendered.text.contains("(&rendered.text).is_ok()); + assert!(approx_token_count(&rendered.text) <= GUARDIAN_MAX_ACTION_TOKENS); + assert!(tool_input.as_str().is_some_and(|input| { + input.contains(" }, + Review { reason: String }, Blocked(String), } @@ -189,6 +190,7 @@ pub(crate) async fn run_pre_tool_use_hooks( hook_events, should_block, block_reason, + review_reason, additional_contexts, updated_input, } = hooks.run_pre_tool_use(request).await; @@ -196,6 +198,9 @@ pub(crate) async fn run_pre_tool_use_hooks( record_additional_contexts(sess, turn_context, additional_contexts).await; if !should_block { + if let Some(reason) = review_reason { + return PreToolUseHookResult::Review { reason }; + } return PreToolUseHookResult::Continue { updated_input }; } diff --git a/codex-rs/core/src/tools/codex_plus_plus/mod.rs b/codex-rs/core/src/tools/codex_plus_plus/mod.rs new file mode 100644 index 000000000000..fc8b8faaf497 --- /dev/null +++ b/codex-rs/core/src/tools/codex_plus_plus/mod.rs @@ -0,0 +1 @@ +pub(super) mod pre_tool_use_review; diff --git a/codex-rs/core/src/tools/codex_plus_plus/pre_tool_use_review.rs b/codex-rs/core/src/tools/codex_plus_plus/pre_tool_use_review.rs new file mode 100644 index 000000000000..660c0673980b --- /dev/null +++ b/codex-rs/core/src/tools/codex_plus_plus/pre_tool_use_review.rs @@ -0,0 +1,60 @@ +use crate::guardian::GuardianApprovalRequest; +use crate::guardian::guardian_rejection_message; +use crate::guardian::guardian_timeout_message; +use crate::guardian::new_guardian_review_id; +use crate::guardian::review_approval_request; +use crate::tools::context::ToolInvocation; +use crate::tools::registry::PreToolUsePayload; +use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::protocol::ReviewDecision; +use futures::future::BoxFuture; + +pub(crate) fn review<'a>( + invocation: &'a ToolInvocation, + payload: &'a PreToolUsePayload, + reason: String, + approvals_reviewer: ApprovalsReviewer, +) -> BoxFuture<'a, Result<(), String>> { + Box::pin(async move { + let strict = invocation + .session + .strict_auto_review_enabled_for_turn() + .await; + if approvals_reviewer != ApprovalsReviewer::AutoReview && !strict { + return Err( + "PreToolUse hook requested Guardian review, but auto review is disabled." + .to_string(), + ); + } + + let review_id = new_guardian_review_id(); + let decision = review_approval_request( + &invocation.session, + &invocation.turn, + review_id.clone(), + GuardianApprovalRequest::PreToolUse { + id: invocation.call_id.clone(), + tool_name: payload.tool_name.name().to_string(), + tool_input: payload.tool_input.clone(), + reason, + #[allow(deprecated)] + cwd: invocation.turn.cwd.clone(), + }, + /*retry_reason*/ None, + ) + .await; + match decision { + ReviewDecision::Approved + | ReviewDecision::ApprovedForSession + | ReviewDecision::ApprovedExecpolicyAmendment { .. } + | ReviewDecision::NetworkPolicyAmendment { .. } => Ok(()), + ReviewDecision::Denied => { + Err(guardian_rejection_message(&invocation.session, &review_id).await) + } + ReviewDecision::TimedOut => Err(guardian_timeout_message()), + ReviewDecision::Abort => Err( + "Automatic approval review was cancelled. The tool call was blocked.".to_string(), + ), + } + }) +} diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index c9533fd223ea..2709c1d37ad2 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -164,6 +164,17 @@ impl McpHandler { } impl CoreToolRuntime for McpHandler { + fn approvals_reviewer( + &self, + invocation: &ToolInvocation, + ) -> codex_protocol::config_types::ApprovalsReviewer { + crate::connectors::mcp_approvals_reviewer( + invocation.turn.config.as_ref(), + &self.tool_info.server_name, + self.tool_info.connector_id.as_deref(), + ) + } + fn telemetry_tags<'a>( &'a self, _invocation: &'a ToolInvocation, diff --git a/codex-rs/core/src/tools/mod.rs b/codex-rs/core/src/tools/mod.rs index 9bca3d3a56d6..b11b3adf0734 100644 --- a/codex-rs/core/src/tools/mod.rs +++ b/codex-rs/core/src/tools/mod.rs @@ -1,4 +1,5 @@ pub(crate) mod code_mode; +mod codex_plus_plus; pub(crate) mod context; pub(crate) mod events; pub(crate) mod handlers; diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 2220d4244a9c..18af3fc402f2 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -13,6 +13,7 @@ use crate::memory_usage::emit_metric_for_tool_read; use crate::sandbox_tags::permission_profile_policy_tag; use crate::sandbox_tags::permission_profile_sandbox_tag; use crate::session::turn_context::TurnContext; +use crate::tools::codex_plus_plus::pre_tool_use_review; use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; @@ -25,6 +26,7 @@ use crate::tools::lifecycle::notify_tool_start; use crate::tools::tool_dispatch_trace::ToolDispatchTrace; use crate::util::error_or_panic; use codex_extension_api::ToolCallOutcome; +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::FunctionCallOutputPayload; use codex_protocol::models::ResponseInputItem; use codex_protocol::protocol::EventMsg; @@ -46,6 +48,10 @@ pub use codex_tools::ToolExposure; /// Implementers provide the shared `ToolExecutor` behavior plus optional /// core-owned metadata for hooks, telemetry, tool search, and argument diffs. pub(crate) trait CoreToolRuntime: ToolExecutor { + fn approvals_reviewer(&self, invocation: &ToolInvocation) -> ApprovalsReviewer { + invocation.turn.config.approvals_reviewer + } + fn matches_kind(&self, payload: &ToolPayload) -> bool { matches!( payload, @@ -278,6 +284,10 @@ impl ToolExecutor for ExposureOverride { } impl CoreToolRuntime for ExposureOverride { + fn approvals_reviewer(&self, invocation: &ToolInvocation) -> ApprovalsReviewer { + self.handler.approvals_reviewer(invocation) + } + fn matches_kind(&self, payload: &ToolPayload) -> bool { self.handler.matches_kind(payload) } @@ -513,6 +523,26 @@ impl ToolRegistry { .await; return Err(err); } + PreToolUseHookResult::Review { reason } => { + if let Err(message) = pre_tool_use_review::review( + &invocation, + &pre_tool_use_payload, + reason, + tool.approvals_reviewer(&invocation), + ) + .await + { + let err = FunctionCallError::RespondToModel(message); + dispatch_trace.record_failed(&err); + notify_tool_finish_if_unclaimed( + &invocation, + terminal_outcome_reached.as_deref(), + ToolCallOutcome::Blocked, + ) + .await; + return Err(err); + } + } PreToolUseHookResult::Continue { updated_input: Some(updated_input), } => match tool.with_updated_hook_input(invocation.clone(), updated_input) { diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index a5c82d5767ec..907978e2104c 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -10,6 +10,7 @@ use codex_model_provider_info::ModelProviderInfo; use codex_model_provider_info::built_in_model_providers; use codex_plugin::PluginHookSource; use codex_plugin::PluginId; +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::items::parse_hook_prompt_fragment; use codex_protocol::models::ContentItem; use codex_protocol::models::PermissionProfile; @@ -20,6 +21,7 @@ use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::RolloutLine; +use codex_protocol::protocol::SandboxPolicy; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; use core_test_support::hooks::trust_discovered_hooks; @@ -42,6 +44,7 @@ use core_test_support::skip_if_host_windows; use core_test_support::skip_if_no_network; use core_test_support::streaming_sse::StreamingSseChunk; use core_test_support::streaming_sse::start_streaming_sse_server; +use core_test_support::test_codex::TestCodex; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use pretty_assertions::assert_eq; @@ -59,6 +62,53 @@ const BLOCKED_PROMPT_CONTEXT: &str = "Remember the blocked lighthouse note."; const PERMISSION_REQUEST_HOOK_MATCHER: &str = "^Bash$"; const PERMISSION_REQUEST_ALLOW_REASON: &str = "should not be used for allow"; +async fn submit_yolo_auto_review_turn(test: &TestCodex, prompt: &str) -> Result<()> { + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: prompt.to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { + approval_policy: Some(AskForApproval::Never), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + sandbox_policy: Some(SandboxPolicy::DangerFullAccess), + ..Default::default() + }, + }) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + Ok(()) +} + +fn guardian_review_sse(response_id: &str, outcome: &str, rationale: &str) -> String { + let (risk_level, user_authorization) = if outcome == "allow" { + ("low", "high") + } else { + ("high", "low") + }; + sse(vec![ + ev_response_created(response_id), + ev_assistant_message( + &format!("msg-{response_id}"), + &serde_json::json!({ + "risk_level": risk_level, + "user_authorization": user_authorization, + "outcome": outcome, + "rationale": rationale, + }) + .to_string(), + ), + ev_completed(response_id), + ]) +} + fn restrictive_workspace_write_profile() -> PermissionProfile { PermissionProfile::workspace_write_with( &[], @@ -367,6 +417,14 @@ elif mode == "json_deny_with_context": "additionalContext": reason }} }})) +elif mode == "ask": + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PreToolUse", + "permissionDecision": "ask", + "permissionDecisionReason": reason + }} + }})) elif mode == "exit_2": sys.stderr.write(reason + "\n") raise SystemExit(2) @@ -3603,6 +3661,66 @@ async fn pre_tool_use_blocks_local_function_tool_before_execution() -> Result<() Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn pre_tool_use_ask_reviews_generic_tool_under_yolo() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-ask-generic"; + let args = serde_json::json!({ "sleep_before_ms": 0 }); + let reason = "Review this generic tool call"; + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-parent-tool"), + ev_function_call(call_id, "test_sync_tool", &serde_json::to_string(&args)?), + ev_completed("resp-parent-tool"), + ]), + guardian_review_sse( + "resp-guardian-allow", + "allow", + "The requested test tool call is safe.", + ), + sse(vec![ + ev_response_created("resp-parent-done"), + ev_assistant_message("msg-parent-done", "done"), + ev_completed("resp-parent-done"), + ]), + ], + ) + .await; + + let mut builder = test_codex() + .with_model("test-gpt-5.1-codex") + .with_pre_build_hook(move |home| { + write_pre_tool_use_hook(home, Some("^test_sync_tool$"), "ask", reason) + .expect("failed to write pre tool use hook test fixture"); + }) + .with_config(trust_discovered_hooks); + let test = builder.build(&server).await?; + + submit_yolo_auto_review_turn(&test, "review the generic function tool").await?; + + let requests = responses.requests(); + let guardian_request = requests + .iter() + .find(|request| request.body_contains_text(reason)) + .expect("expected hook-enforced Guardian review request"); + assert!(guardian_request.body_contains_text("test_sync_tool")); + assert!(guardian_request.body_contains_text("sleep_before_ms")); + assert_eq!( + requests + .iter() + .filter_map(|request| request.function_call_output_text(call_id)) + .count(), + 1, + "approved original invocation should execute exactly once" + ); + + Ok(()) +} + #[tokio::test] async fn pre_tool_use_rewrites_local_function_tool_before_execution() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index b4f214146a8f..46496545930c 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -16,9 +16,11 @@ pub(crate) struct SessionStartOutput { pub(crate) struct PreToolUseOutput { pub universal: UniversalOutput, pub block_reason: Option, + pub review_reason: Option, pub additional_context: Option, pub updated_input: Option, pub invalid_reason: Option, + pub fail_closed: bool, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -141,6 +143,12 @@ pub(crate) fn parse_pre_tool_use(stdout: &str) -> Option { unsupported_pre_tool_use_legacy_decision(decision.as_ref(), reason.as_deref()) } }); + let fail_closed = hook_specific_output.is_some_and(|output| { + matches!( + output.permission_decision, + Some(PreToolUsePermissionDecisionWire::Ask) + ) && invalid_reason.is_some() + }); let block_reason = if invalid_reason.is_none() { if use_hook_specific_decision { hook_specific_output.and_then(|output| match output.permission_decision { @@ -159,6 +167,17 @@ pub(crate) fn parse_pre_tool_use(stdout: &str) -> Option { } else { None }; + let review_reason = if invalid_reason.is_none() { + hook_specific_output.and_then(|output| match output.permission_decision { + Some(PreToolUsePermissionDecisionWire::Ask) => output + .permission_decision_reason + .as_deref() + .and_then(trimmed_reason), + _ => None, + }) + } else { + None + }; let updated_input = if invalid_reason.is_none() { hook_specific_output.and_then(|output| { matches!( @@ -175,9 +194,11 @@ pub(crate) fn parse_pre_tool_use(stdout: &str) -> Option { Some(PreToolUseOutput { universal, block_reason, + review_reason, additional_context, updated_input, invalid_reason, + fail_closed, }) } @@ -435,6 +456,13 @@ fn unsupported_pre_tool_use_hook_specific_output( output: &crate::schema::PreToolUseHookSpecificOutputWire, ) -> Option { if output.updated_input.is_some() + && matches!( + output.permission_decision, + Some(PreToolUsePermissionDecisionWire::Ask) + ) + { + Some("PreToolUse hook returned permissionDecision:ask with updatedInput".to_string()) + } else if output.updated_input.is_some() && !matches!( output.permission_decision, Some(PreToolUsePermissionDecisionWire::Allow) @@ -448,9 +476,12 @@ fn unsupported_pre_tool_use_hook_specific_output( "PreToolUse hook returned unsupported permissionDecision:allow".to_string() }) } - Some(PreToolUsePermissionDecisionWire::Ask) => { - Some("PreToolUse hook returned unsupported permissionDecision:ask".to_string()) - } + Some(PreToolUsePermissionDecisionWire::Ask) => output + .permission_decision_reason + .as_deref() + .and_then(trimmed_reason) + .is_none() + .then(invalid_pre_tool_use_ask_reason_message), Some(PreToolUsePermissionDecisionWire::Deny) => { if output .permission_decision_reason @@ -504,6 +535,11 @@ fn invalid_pre_tool_use_reason_message() -> String { .to_string() } +fn invalid_pre_tool_use_ask_reason_message() -> String { + "PreToolUse hook returned permissionDecision:ask without a non-empty permissionDecisionReason" + .to_string() +} + fn trimmed_reason(reason: &str) -> Option { let trimmed = reason.trim(); if trimmed.is_empty() { diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index b3579aba824e..eb2d729f724f 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -39,6 +39,7 @@ pub struct PreToolUseOutcome { pub hook_events: Vec, pub should_block: bool, pub block_reason: Option, + pub review_reason: Option, pub additional_contexts: Vec, pub updated_input: Option, } @@ -47,10 +48,18 @@ pub struct PreToolUseOutcome { struct PreToolUseHandlerData { should_block: bool, block_reason: Option, + review_reason: Option, additional_contexts_for_model: Vec, updated_input: Option, } +#[derive(Debug, PartialEq, Eq)] +struct PreToolUseDecision { + should_block: bool, + block_reason: Option, + review_reason: Option, +} + pub(crate) fn preview( handlers: &[ConfiguredHandler], request: &PreToolUseRequest, @@ -84,6 +93,7 @@ pub(crate) async fn run( hook_events: Vec::new(), should_block: false, block_reason: None, + review_reason: None, additional_contexts: Vec::new(), updated_input: None, }; @@ -112,16 +122,17 @@ pub(crate) async fn run( ) .await; - let should_block = results.iter().any(|result| result.data.should_block); - let block_reason = results - .iter() - .find_map(|result| result.data.block_reason.clone()); + let PreToolUseDecision { + should_block, + block_reason, + review_reason, + } = aggregate_decision(&results); let additional_contexts = common::flatten_additional_contexts( results .iter() .map(|result| result.data.additional_contexts_for_model.as_slice()), ); - let updated_input = if should_block { + let updated_input = if should_block || review_reason.is_some() { None } else { latest_updated_input(&results) @@ -136,11 +147,33 @@ pub(crate) async fn run( .collect(), should_block, block_reason, + review_reason, additional_contexts, updated_input, } } +fn aggregate_decision( + results: &[dispatcher::ParsedHandler], +) -> PreToolUseDecision { + let should_block = results.iter().any(|result| result.data.should_block); + let review_reason = (!should_block) + .then(|| results.iter().find_map(|r| r.data.review_reason.clone())) + .flatten(); + if review_reason.is_some() && latest_updated_input(results).is_some() { + return PreToolUseDecision { + should_block: true, + block_reason: Some("PreToolUse hooks returned ask and updatedInput".to_string()), + review_reason: None, + }; + } + PreToolUseDecision { + should_block, + block_reason: results.iter().find_map(|r| r.data.block_reason.clone()), + review_reason, + } +} + /// Chooses the rewrite from the hook that actually finished last. /// /// Hook results stay in configured order for stable reporting, but the @@ -194,6 +227,7 @@ fn parse_completed( let mut status = HookRunStatus::Completed; let mut should_block = false; let mut block_reason = None; + let mut review_reason = None; let mut additional_contexts_for_model = Vec::new(); let mut updated_input = None; @@ -218,6 +252,10 @@ fn parse_completed( } if let Some(invalid_reason) = parsed.invalid_reason { status = HookRunStatus::Failed; + if parsed.fail_closed { + should_block = true; + block_reason = Some(invalid_reason.clone()); + } entries.push(HookOutputEntry { kind: HookOutputEntryKind::Error, text: invalid_reason, @@ -239,12 +277,27 @@ fn parse_completed( text: reason, }); } + if let Some(reason) = parsed.review_reason { + review_reason = Some(reason.clone()); + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: reason, + }); + } if !should_block { updated_input = parsed.updated_input; } } } else if output_parser::looks_like_json(&run_result.stdout) { status = HookRunStatus::Failed; + let asks = regex::Regex::new( + r#""(?:p|\\u0070)(?:e|\\u0065)(?:r|\\u0072)(?:m|\\u006[dD])(?:i|\\u0069)(?:s|\\u0073)(?:s|\\u0073)(?:i|\\u0069)(?:o|\\u006[fF])(?:n|\\u006[eE])(?:D|\\u0044)(?:e|\\u0065)(?:c|\\u0063)(?:i|\\u0069)(?:s|\\u0073)(?:i|\\u0069)(?:o|\\u006[fF])(?:n|\\u006[eE])"\s*:\s*"(?:a|\\u0061)(?:s|\\u0073)(?:k|\\u006[bB])"#, + ) + .is_ok_and(|regex| regex.is_match(&run_result.stdout)); + if asks { + should_block = true; + block_reason = Some("invalid PreToolUse hook JSON".into()); + } entries.push(HookOutputEntry { kind: HookOutputEntryKind::Error, text: "hook returned invalid pre-tool-use JSON output".to_string(), @@ -295,6 +348,7 @@ fn parse_completed( data: PreToolUseHandlerData { should_block, block_reason, + review_reason, additional_contexts_for_model, updated_input, }, @@ -307,6 +361,7 @@ fn serialization_failure_outcome(hook_events: Vec) -> PreToo hook_events, should_block: false, block_reason: None, + review_reason: None, additional_contexts: Vec::new(), updated_input: None, } @@ -323,7 +378,9 @@ mod tests { use codex_utils_absolute_path::test_support::test_path_buf; use pretty_assertions::assert_eq; + use super::PreToolUseDecision; use super::PreToolUseHandlerData; + use super::aggregate_decision; use super::command_input_json; use super::latest_updated_input; use super::parse_completed; @@ -361,6 +418,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + review_reason: None, additional_contexts_for_model: Vec::new(), updated_input: None, } @@ -392,6 +450,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + review_reason: None, additional_contexts_for_model: Vec::new(), updated_input: Some(serde_json::json!({ "command": "echo rewritten" })), } @@ -429,6 +488,37 @@ mod tests { ); } + #[test] + fn ask_with_separate_rewrite_fails_closed() { + let ask = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"review this"}}"#, + "", + ), + Some("turn-1".to_string()), + ); + let rewrite = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow","updatedInput":{"command":"echo rewritten"}}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + aggregate_decision(&[ask, rewrite]), + PreToolUseDecision { + should_block: true, + block_reason: Some("PreToolUse hooks returned ask and updatedInput".to_string()), + review_reason: None, + } + ); + } + #[test] fn permission_decision_allow_without_updated_input_fails_open() { let parsed = parse_completed( @@ -446,6 +536,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + review_reason: None, additional_contexts_for_model: Vec::new(), updated_input: None, } @@ -477,6 +568,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + review_reason: None, additional_contexts_for_model: Vec::new(), updated_input: None, } @@ -508,6 +600,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + review_reason: None, additional_contexts_for_model: vec!["remember this".to_string()], updated_input: None, } @@ -529,7 +622,7 @@ mod tests { } #[test] - fn unsupported_permission_decision_fails_open() { + fn permission_decision_ask_requests_review() { let parsed = parse_completed( &handler(), run_result( @@ -545,20 +638,70 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + review_reason: Some("please confirm".to_string()), additional_contexts_for_model: Vec::new(), updated_input: None, } ); - assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); + assert_eq!(parsed.completed.run.status, HookRunStatus::Completed); assert_eq!( parsed.completed.run.entries, vec![HookOutputEntry { - kind: HookOutputEntryKind::Error, - text: "PreToolUse hook returned unsupported permissionDecision:ask".to_string(), + kind: HookOutputEntryKind::Feedback, + text: "please confirm".to_string(), }] ); } + #[test] + fn permission_decision_ask_with_updated_input_fails_closed() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"please confirm","updatedInput":{"command":"echo rewritten"}}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: true, + block_reason: Some( + "PreToolUse hook returned permissionDecision:ask with updatedInput".to_string() + ), + review_reason: None, + additional_contexts_for_model: Vec::new(), + updated_input: None, + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); + } + + #[test] + fn permission_decision_ask_without_reason_fails_closed() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask"}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert!(parsed.data.should_block); + assert_eq!( + parsed.data.block_reason.as_deref(), + Some( + "PreToolUse hook returned permissionDecision:ask without a non-empty permissionDecisionReason" + ) + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); + } + #[test] fn deprecated_approve_decision_fails_open() { let parsed = parse_completed( @@ -572,6 +715,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + review_reason: None, additional_contexts_for_model: Vec::new(), updated_input: None, } @@ -603,6 +747,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + review_reason: None, additional_contexts_for_model: vec!["nope".to_string()], updated_input: None, } @@ -636,6 +781,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + review_reason: None, additional_contexts_for_model: Vec::new(), updated_input: None, } @@ -645,18 +791,20 @@ mod tests { } #[test] - fn invalid_json_like_stdout_fails_instead_of_becoming_noop() { + fn malformed_ask_output_fails_closed() { + let stdout = r#"{"hookSpecificOutput":{"permission\u0044ecision":"\u0061sk"#; let parsed = parse_completed( &handler(), - run_result(Some(0), "{\"decision\":\n", ""), + run_result(Some(0), stdout, ""), Some("turn-1".to_string()), ); assert_eq!( parsed.data, PreToolUseHandlerData { - should_block: false, - block_reason: None, + should_block: true, + block_reason: Some("invalid PreToolUse hook JSON".into()), + review_reason: None, additional_contexts_for_model: Vec::new(), updated_input: None, } @@ -684,6 +832,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("blocked by policy".to_string()), + review_reason: None, additional_contexts_for_model: Vec::new(), updated_input: None, } diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 5cf4f27f183c..877d144ffd20 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -163,6 +163,11 @@ pub enum GuardianAssessmentAction { connector_name: Option, tool_title: Option, }, + PreToolUse { + tool_name: String, + tool_input: JsonValue, + reason: String, + }, RequestPermissions { reason: Option, permissions: RequestPermissionProfile, diff --git a/codex-rs/tui/src/auto_review_denials.rs b/codex-rs/tui/src/auto_review_denials.rs index 149a60f04939..18308c4e0d20 100644 --- a/codex-rs/tui/src/auto_review_denials.rs +++ b/codex-rs/tui/src/auto_review_denials.rs @@ -67,6 +67,9 @@ pub(crate) fn action_summary(action: &GuardianAssessmentAction) -> String { let label = connector_name.as_deref().unwrap_or(server.as_str()); format!("MCP {tool_name} on {label}") } + GuardianAssessmentAction::PreToolUse { + tool_name, reason, .. + } => format!("{tool_name}: {reason}"), GuardianAssessmentAction::RequestPermissions { reason, .. } => reason .as_deref() .map(|reason| format!("permission request: {reason}")) diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__guardian_approved_request_permissions_renders_request_summary.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__guardian_approved_request_permissions_renders_request_summary.snap index 4a0515362543..0ae91d993c01 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__guardian_approved_request_permissions_renders_request_summary.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__guardian_approved_request_permissions_renders_request_summary.snap @@ -3,11 +3,11 @@ source: tui/src/chatwidget/tests/guardian.rs expression: normalize_snapshot_paths(term.backend().vt100().screen().contents()) --- - - ✔ Request approved for permission request: Need write access for generated report assets. +✔ Request approved for custom_tool: Review report access + • Working (0s • esc to interrupt) diff --git a/codex-rs/tui/src/chatwidget/tests/guardian.rs b/codex-rs/tui/src/chatwidget/tests/guardian.rs index 27a226db020f..056f7423fc8b 100644 --- a/codex-rs/tui/src/chatwidget/tests/guardian.rs +++ b/codex-rs/tui/src/chatwidget/tests/guardian.rs @@ -224,6 +224,23 @@ async fn guardian_approved_request_permissions_renders_request_summary() { decision_source: Some(GuardianAssessmentDecisionSource::Agent), action, }); + chat.on_guardian_assessment(GuardianAssessmentEvent { + id: "guardian-pre-tool-use".into(), + target_item_id: Some("tool-call-1".into()), + turn_id: "turn-1".into(), + started_at_ms: 2, + completed_at_ms: Some(3), + status: GuardianAssessmentStatus::Approved, + risk_level: Some(GuardianRiskLevel::Low), + user_authorization: Some(GuardianUserAuthorization::High), + rationale: Some("Hook-requested review approved.".into()), + decision_source: Some(GuardianAssessmentDecisionSource::Agent), + action: GuardianAssessmentAction::PreToolUse { + tool_name: "custom_tool".to_string(), + tool_input: serde_json::json!({ "path": "report.txt" }), + reason: "Review report access".to_string(), + }, + }); let width: u16 = 110; let ui_height: u16 = chat.desired_height(width); diff --git a/codex-rs/tui/src/chatwidget/tool_requests.rs b/codex-rs/tui/src/chatwidget/tool_requests.rs index f3ff2391f64c..1349d27a0f95 100644 --- a/codex-rs/tui/src/chatwidget/tool_requests.rs +++ b/codex-rs/tui/src/chatwidget/tool_requests.rs @@ -73,6 +73,9 @@ impl ChatWidget { let label = connector_name.as_deref().unwrap_or(server.as_str()); Some(format!("MCP {tool_name} on {label}")) } + GuardianAssessmentAction::PreToolUse { + tool_name, reason, .. + } => Some(format!("{tool_name}: {reason}")), GuardianAssessmentAction::RequestPermissions { reason, .. } => { Some(permission_request_summary("permission request", reason)) } @@ -90,6 +93,7 @@ impl ChatWidget { GuardianAssessmentAction::ApplyPatch { .. } | GuardianAssessmentAction::NetworkAccess { .. } | GuardianAssessmentAction::McpToolCall { .. } + | GuardianAssessmentAction::PreToolUse { .. } | GuardianAssessmentAction::RequestPermissions { .. } => None, }; @@ -189,6 +193,11 @@ impl ChatWidget { } => history_cell::new_guardian_timed_out_action_request(format!( "codex could call MCP tool {server}.{tool_name}" )), + GuardianAssessmentAction::PreToolUse { tool_name, .. } => { + history_cell::new_guardian_timed_out_action_request(format!( + "codex could call {tool_name}" + )) + } GuardianAssessmentAction::NetworkAccess { target, .. } => { history_cell::new_guardian_timed_out_action_request(format!( "codex could access {target}" @@ -233,6 +242,11 @@ impl ChatWidget { } => history_cell::new_guardian_denied_action_request(format!( "codex to call MCP tool {server}.{tool_name}" )), + GuardianAssessmentAction::PreToolUse { tool_name, .. } => { + history_cell::new_guardian_denied_action_request(format!( + "codex to call {tool_name}" + )) + } GuardianAssessmentAction::NetworkAccess { target, .. } => { history_cell::new_guardian_denied_action_request(format!( "codex to access {target}"