mirror of
https://github.com/openai/codex.git
synced 2026-10-27 18:48:37 +00:00
Include approval context in PermissionRequest hooks
Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
@@ -26,6 +26,7 @@ use serde_json::Value;
|
||||
use crate::codex::Session;
|
||||
use crate::codex::TurnContext;
|
||||
use crate::event_mapping::parse_turn_item;
|
||||
use crate::tools::sandboxing::PermissionRequestPayload;
|
||||
|
||||
pub(crate) struct HookRuntimeOutcome {
|
||||
pub should_stop: bool,
|
||||
@@ -152,8 +153,7 @@ pub(crate) async fn run_permission_request_hooks(
|
||||
sess: &Arc<Session>,
|
||||
turn_context: &Arc<TurnContext>,
|
||||
run_id_suffix: String,
|
||||
tool_name: String,
|
||||
command: String,
|
||||
payload: PermissionRequestPayload,
|
||||
) -> Option<PermissionRequestDecision> {
|
||||
let request = PermissionRequestRequest {
|
||||
session_id: sess.conversation_id,
|
||||
@@ -162,9 +162,12 @@ pub(crate) async fn run_permission_request_hooks(
|
||||
transcript_path: sess.hook_transcript_path().await,
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
permission_mode: hook_permission_mode(turn_context),
|
||||
tool_name,
|
||||
tool_name: payload.tool_name,
|
||||
run_id_suffix,
|
||||
command,
|
||||
command: payload.command,
|
||||
sandbox_permissions: payload.sandbox_permissions,
|
||||
additional_permissions: payload.additional_permissions,
|
||||
justification: payload.justification,
|
||||
};
|
||||
let preview_runs = sess.hooks().preview_permission_request(&request);
|
||||
emit_hook_started_events(sess, turn_context, preview_runs).await;
|
||||
|
||||
@@ -46,6 +46,18 @@ pub(crate) struct OrchestratorRunResult<Out> {
|
||||
pub deferred_network_approval: Option<DeferredNetworkApproval>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
enum ApprovalAttempt {
|
||||
Initial,
|
||||
Retry,
|
||||
}
|
||||
|
||||
struct ApprovalTelemetry<'a> {
|
||||
otel: &'a SessionTelemetry,
|
||||
tool_name: &'a str,
|
||||
call_id: &'a str,
|
||||
}
|
||||
|
||||
impl ToolOrchestrator {
|
||||
pub fn new() -> Self {
|
||||
Self {
|
||||
@@ -150,11 +162,14 @@ impl ToolOrchestrator {
|
||||
let decision = Self::request_approval(
|
||||
tool,
|
||||
req,
|
||||
ApprovalAttempt::Initial,
|
||||
approval_ctx,
|
||||
turn_ctx,
|
||||
&otel,
|
||||
otel_tn,
|
||||
otel_ci,
|
||||
ApprovalTelemetry {
|
||||
otel: &otel,
|
||||
tool_name: otel_tn,
|
||||
call_id: otel_ci,
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -311,11 +326,14 @@ impl ToolOrchestrator {
|
||||
let decision = Self::request_approval(
|
||||
tool,
|
||||
req,
|
||||
ApprovalAttempt::Retry,
|
||||
approval_ctx,
|
||||
turn_ctx,
|
||||
&otel,
|
||||
otel_tn,
|
||||
otel_ci,
|
||||
ApprovalTelemetry {
|
||||
otel: &otel,
|
||||
tool_name: otel_tn,
|
||||
call_id: otel_ci,
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -388,38 +406,40 @@ impl ToolOrchestrator {
|
||||
async fn request_approval<Rq, Out, T>(
|
||||
tool: &mut T,
|
||||
req: &Rq,
|
||||
approval_attempt: ApprovalAttempt,
|
||||
approval_ctx: ApprovalCtx<'_>,
|
||||
turn_ctx: &crate::codex::TurnContext,
|
||||
otel: &SessionTelemetry,
|
||||
otel_tn: &str,
|
||||
otel_ci: &str,
|
||||
telemetry: ApprovalTelemetry<'_>,
|
||||
) -> Result<ReviewDecision, ToolError>
|
||||
where
|
||||
T: ToolRuntime<Rq, Out>,
|
||||
{
|
||||
if let Some(permission_request) = tool.permission_request_payload(req) {
|
||||
let run_id_suffix = match approval_attempt {
|
||||
ApprovalAttempt::Initial => format!("{}:initial", approval_ctx.call_id),
|
||||
ApprovalAttempt::Retry => format!("{}:retry", approval_ctx.call_id),
|
||||
};
|
||||
match run_permission_request_hooks(
|
||||
approval_ctx.session,
|
||||
approval_ctx.turn,
|
||||
approval_ctx.call_id.to_string(),
|
||||
permission_request.tool_name,
|
||||
permission_request.command,
|
||||
run_id_suffix,
|
||||
permission_request,
|
||||
)
|
||||
.await
|
||||
{
|
||||
Some(PermissionRequestDecision::Allow) => {
|
||||
otel.tool_decision(
|
||||
otel_tn,
|
||||
otel_ci,
|
||||
telemetry.otel.tool_decision(
|
||||
telemetry.tool_name,
|
||||
telemetry.call_id,
|
||||
&ReviewDecision::Approved,
|
||||
ToolDecisionSource::Config,
|
||||
);
|
||||
return Ok(ReviewDecision::Approved);
|
||||
}
|
||||
Some(PermissionRequestDecision::Deny { message }) => {
|
||||
otel.tool_decision(
|
||||
otel_tn,
|
||||
otel_ci,
|
||||
telemetry.otel.tool_decision(
|
||||
telemetry.tool_name,
|
||||
telemetry.call_id,
|
||||
&ReviewDecision::Denied,
|
||||
ToolDecisionSource::Config,
|
||||
);
|
||||
@@ -435,7 +455,12 @@ impl ToolOrchestrator {
|
||||
} else {
|
||||
ToolDecisionSource::User
|
||||
};
|
||||
otel.tool_decision(otel_tn, otel_ci, &decision, otel_source);
|
||||
telemetry.otel.tool_decision(
|
||||
telemetry.tool_name,
|
||||
telemetry.call_id,
|
||||
&decision,
|
||||
otel_source,
|
||||
);
|
||||
Ok(decision)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -200,7 +200,12 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
}
|
||||
|
||||
fn permission_request_payload(&self, req: &ShellRequest) -> Option<PermissionRequestPayload> {
|
||||
Some(PermissionRequestPayload::bash(req.hook_command.clone()))
|
||||
Some(PermissionRequestPayload::bash(
|
||||
req.hook_command.clone(),
|
||||
req.sandbox_permissions,
|
||||
req.additional_permissions.clone(),
|
||||
req.justification.clone(),
|
||||
))
|
||||
}
|
||||
|
||||
fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride {
|
||||
|
||||
@@ -183,7 +183,12 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
&self,
|
||||
req: &UnifiedExecRequest,
|
||||
) -> Option<PermissionRequestPayload> {
|
||||
Some(PermissionRequestPayload::bash(req.hook_command.clone()))
|
||||
Some(PermissionRequestPayload::bash(
|
||||
req.hook_command.clone(),
|
||||
req.sandbox_permissions,
|
||||
req.additional_permissions.clone(),
|
||||
req.justification.clone(),
|
||||
))
|
||||
}
|
||||
|
||||
fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride {
|
||||
|
||||
@@ -14,6 +14,7 @@ use codex_network_proxy::NetworkProxy;
|
||||
use codex_protocol::approvals::ExecPolicyAmendment;
|
||||
use codex_protocol::approvals::NetworkApprovalContext;
|
||||
use codex_protocol::error::CodexErr;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::permissions::FileSystemSandboxKind;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
@@ -135,13 +136,24 @@ pub(crate) struct ApprovalCtx<'a> {
|
||||
pub(crate) struct PermissionRequestPayload {
|
||||
pub tool_name: String,
|
||||
pub command: String,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
pub additional_permissions: Option<PermissionProfile>,
|
||||
pub justification: Option<String>,
|
||||
}
|
||||
|
||||
impl PermissionRequestPayload {
|
||||
pub(crate) fn bash(command: String) -> Self {
|
||||
pub(crate) fn bash(
|
||||
command: String,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
additional_permissions: Option<PermissionProfile>,
|
||||
justification: Option<String>,
|
||||
) -> Self {
|
||||
Self {
|
||||
tool_name: "Bash".to_string(),
|
||||
command,
|
||||
sandbox_permissions,
|
||||
additional_permissions,
|
||||
justification,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1095,7 +1095,7 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
|
||||
let server = start_mock_server().await;
|
||||
let call_id = "permissionrequest-shell-command";
|
||||
let marker = std::env::temp_dir().join("permissionrequest-shell-command-marker");
|
||||
let command = format!("printf allowed > {}", marker.display());
|
||||
let command = format!("rm -f {}", marker.display());
|
||||
let args = serde_json::json!({ "command": command });
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
@@ -1137,9 +1137,7 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
if marker.exists() {
|
||||
fs::remove_file(&marker).context("remove leftover permission request marker")?;
|
||||
}
|
||||
fs::write(&marker, "seed").context("create permission request marker")?;
|
||||
|
||||
test.submit_turn_with_policies(
|
||||
"run the shell command after hook approval",
|
||||
@@ -1150,18 +1148,10 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
|
||||
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), 2);
|
||||
let output_item = requests[1].function_call_output(call_id);
|
||||
let output = output_item
|
||||
.get("output")
|
||||
.and_then(Value::as_str)
|
||||
.expect("shell command output string");
|
||||
requests[1].function_call_output(call_id);
|
||||
assert!(
|
||||
output.contains("allowed"),
|
||||
"shell command output should reach the model after hook approval",
|
||||
);
|
||||
assert!(
|
||||
marker.exists(),
|
||||
"approved command should create marker file"
|
||||
!marker.exists(),
|
||||
"approved command should remove marker file"
|
||||
);
|
||||
|
||||
let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?;
|
||||
@@ -1169,6 +1159,14 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
|
||||
assert_eq!(hook_inputs[0]["hook_event_name"], "PermissionRequest");
|
||||
assert_eq!(hook_inputs[0]["tool_name"], "Bash");
|
||||
assert_eq!(hook_inputs[0]["tool_input"]["command"], command);
|
||||
assert_eq!(
|
||||
hook_inputs[0]["approval_context"],
|
||||
serde_json::json!({
|
||||
"sandbox_permissions": "use_default",
|
||||
"additional_permissions": null,
|
||||
"justification": null,
|
||||
})
|
||||
);
|
||||
assert!(
|
||||
hook_inputs[0].get("tool_use_id").is_none(),
|
||||
"PermissionRequest input should not include a tool_use_id",
|
||||
|
||||
@@ -2,12 +2,70 @@
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"additionalProperties": false,
|
||||
"definitions": {
|
||||
"AbsolutePathBuf": {
|
||||
"description": "A path that is guaranteed to be absolute and normalized (though it is not guaranteed to be canonicalized or exist on the filesystem).\n\nIMPORTANT: When deserializing an `AbsolutePathBuf`, a base path must be set using [AbsolutePathBufGuard::new]. If no base path is set, the deserialization will fail unless the path being deserialized is already absolute.",
|
||||
"type": "string"
|
||||
},
|
||||
"FileSystemPermissions": {
|
||||
"properties": {
|
||||
"read": {
|
||||
"items": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
},
|
||||
"type": "array"
|
||||
},
|
||||
"write": {
|
||||
"items": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
},
|
||||
"type": "array"
|
||||
}
|
||||
},
|
||||
"type": "object"
|
||||
},
|
||||
"NetworkPermissions": {
|
||||
"properties": {
|
||||
"enabled": {
|
||||
"type": "boolean"
|
||||
}
|
||||
},
|
||||
"type": "object"
|
||||
},
|
||||
"NullableString": {
|
||||
"type": [
|
||||
"string",
|
||||
"null"
|
||||
]
|
||||
},
|
||||
"PermissionProfile": {
|
||||
"properties": {
|
||||
"file_system": {
|
||||
"$ref": "#/definitions/FileSystemPermissions"
|
||||
},
|
||||
"network": {
|
||||
"$ref": "#/definitions/NetworkPermissions"
|
||||
}
|
||||
},
|
||||
"type": "object"
|
||||
},
|
||||
"PermissionRequestApprovalContext": {
|
||||
"additionalProperties": false,
|
||||
"properties": {
|
||||
"additional_permissions": {
|
||||
"$ref": "#/definitions/PermissionProfile"
|
||||
},
|
||||
"justification": {
|
||||
"type": "string"
|
||||
},
|
||||
"sandbox_permissions": {
|
||||
"$ref": "#/definitions/SandboxPermissions"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"sandbox_permissions"
|
||||
],
|
||||
"type": "object"
|
||||
},
|
||||
"PermissionRequestToolInput": {
|
||||
"additionalProperties": false,
|
||||
"properties": {
|
||||
@@ -19,9 +77,38 @@
|
||||
"command"
|
||||
],
|
||||
"type": "object"
|
||||
},
|
||||
"SandboxPermissions": {
|
||||
"description": "Controls the per-command sandbox override requested by a shell-like tool call.",
|
||||
"oneOf": [
|
||||
{
|
||||
"description": "Run with the turn's configured sandbox policy unchanged.",
|
||||
"enum": [
|
||||
"use_default"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
{
|
||||
"description": "Request to run outside the sandbox.",
|
||||
"enum": [
|
||||
"require_escalated"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
{
|
||||
"description": "Request to stay in the sandbox while widening permissions for this command only.",
|
||||
"enum": [
|
||||
"with_additional_permissions"
|
||||
],
|
||||
"type": "string"
|
||||
}
|
||||
]
|
||||
}
|
||||
},
|
||||
"properties": {
|
||||
"approval_context": {
|
||||
"$ref": "#/definitions/PermissionRequestApprovalContext"
|
||||
},
|
||||
"cwd": {
|
||||
"type": "string"
|
||||
},
|
||||
@@ -61,6 +148,7 @@
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"approval_context",
|
||||
"cwd",
|
||||
"hook_event_name",
|
||||
"model",
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
use std::path::PathBuf;
|
||||
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::models::SandboxPermissions;
|
||||
use codex_protocol::protocol::HookCompletedEvent;
|
||||
use codex_protocol::protocol::HookEventName;
|
||||
use codex_protocol::protocol::HookOutputEntry;
|
||||
@@ -14,6 +16,7 @@ use crate::engine::ConfiguredHandler;
|
||||
use crate::engine::command_runner::CommandRunResult;
|
||||
use crate::engine::dispatcher;
|
||||
use crate::engine::output_parser;
|
||||
use crate::schema::PermissionRequestApprovalContext;
|
||||
use crate::schema::PermissionRequestCommandInput;
|
||||
use crate::schema::PermissionRequestToolInput;
|
||||
|
||||
@@ -28,6 +31,9 @@ pub struct PermissionRequestRequest {
|
||||
pub tool_name: String,
|
||||
pub run_id_suffix: String,
|
||||
pub command: String,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
pub additional_permissions: Option<PermissionProfile>,
|
||||
pub justification: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
@@ -83,19 +89,7 @@ pub(crate) async fn run(
|
||||
};
|
||||
}
|
||||
|
||||
let input_json = match serde_json::to_string(&PermissionRequestCommandInput {
|
||||
session_id: request.session_id.to_string(),
|
||||
turn_id: request.turn_id.clone(),
|
||||
transcript_path: crate::schema::NullableString::from_path(request.transcript_path.clone()),
|
||||
cwd: request.cwd.display().to_string(),
|
||||
hook_event_name: "PermissionRequest".to_string(),
|
||||
model: request.model.clone(),
|
||||
permission_mode: request.permission_mode.clone(),
|
||||
tool_name: "Bash".to_string(),
|
||||
tool_input: PermissionRequestToolInput {
|
||||
command: request.command.clone(),
|
||||
},
|
||||
}) {
|
||||
let input_json = match serde_json::to_string(&build_command_input(&request)) {
|
||||
Ok(input_json) => input_json,
|
||||
Err(error) => {
|
||||
let hook_events = common::serialization_failure_hook_events_for_tool_use(
|
||||
@@ -136,6 +130,27 @@ pub(crate) async fn run(
|
||||
}
|
||||
}
|
||||
|
||||
fn build_command_input(request: &PermissionRequestRequest) -> PermissionRequestCommandInput {
|
||||
PermissionRequestCommandInput {
|
||||
session_id: request.session_id.to_string(),
|
||||
turn_id: request.turn_id.clone(),
|
||||
transcript_path: crate::schema::NullableString::from_path(request.transcript_path.clone()),
|
||||
cwd: request.cwd.display().to_string(),
|
||||
hook_event_name: "PermissionRequest".to_string(),
|
||||
model: request.model.clone(),
|
||||
permission_mode: request.permission_mode.clone(),
|
||||
tool_name: "Bash".to_string(),
|
||||
tool_input: PermissionRequestToolInput {
|
||||
command: request.command.clone(),
|
||||
},
|
||||
approval_context: PermissionRequestApprovalContext {
|
||||
sandbox_permissions: request.sandbox_permissions,
|
||||
additional_permissions: request.additional_permissions.clone(),
|
||||
justification: request.justification.clone(),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
fn parse_completed(
|
||||
handler: &ConfiguredHandler,
|
||||
run_result: CommandRunResult,
|
||||
|
||||
@@ -12,6 +12,9 @@ use serde_json::Value;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::models::SandboxPermissions;
|
||||
|
||||
const GENERATED_DIR: &str = "generated";
|
||||
const POST_TOOL_USE_INPUT_FIXTURE: &str = "post-tool-use.command.input.schema.json";
|
||||
const POST_TOOL_USE_OUTPUT_FIXTURE: &str = "post-tool-use.command.output.schema.json";
|
||||
@@ -235,6 +238,14 @@ pub(crate) struct PermissionRequestToolInput {
|
||||
pub command: String,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, JsonSchema)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
pub(crate) struct PermissionRequestApprovalContext {
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
pub additional_permissions: Option<PermissionProfile>,
|
||||
pub justification: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, JsonSchema)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
#[schemars(rename = "permission-request.command.input")]
|
||||
@@ -252,6 +263,7 @@ pub(crate) struct PermissionRequestCommandInput {
|
||||
#[schemars(schema_with = "permission_request_tool_name_schema")]
|
||||
pub tool_name: String,
|
||||
pub tool_input: PermissionRequestToolInput,
|
||||
pub approval_context: PermissionRequestApprovalContext,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, JsonSchema)]
|
||||
|
||||
Reference in New Issue
Block a user