mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
## Summary Lifecycle hooks currently treat `PreToolUse`, `PostToolUse`, and `PermissionRequest` as Bash-only flows - hook schema constrains `tool_name` to `Bash` - hook input assumes a command-shaped `tool_input` - core hook dispatch path passes only shell command strings That means hooks cannot target MCP tools even though MCP tool names are model-visible and stable This change generalizes those hook paths so they can match and receive payloads for MCP tools while preserving the existing Bash behavior. ## Reviewer Notes I think these are the key files - `codex-rs/core/src/tools/handlers/mcp.rs` - `codex-rs/core/src/mcp_tool_call.rs` Otherwise the changes across apply_patch, shell, and unified_exec are mainly to rewire everything to be `tool_input` based instead of just `command` so that it'll make sense for MCP tools. ## Changes - Allow `PreToolUse`, `PostToolUse`, and `PermissionRequest` hook inputs to carry arbitrary `tool_name` and `tool_input` values instead of hard-coding `Bash` and command-only payloads. - Add MCP hook payload support through `McpHandler`, using the model-visible tool name from `ToolInvocation` and the raw MCP arguments as `tool_input`. - Include MCP tool responses in `PostToolUse` by serializing `McpToolOutput` into the hook response payload. - Run `PermissionRequest` hooks for MCP approval requests after remembered approval checks and before falling back to user-facing MCP elicitation. - Preserve exact matching for literal hook matchers like `Bash` and `mcp__memory__create_entities`, while keeping regex matcher support for patterns like `mcp__memory__.*` and `mcp__.*__write.*`. --------- Co-authored-by: Andrei Eternal <eternal@openai.com> Co-authored-by: Codex <noreply@openai.com>
141 lines
4.2 KiB
Rust
141 lines
4.2 KiB
Rust
use super::*;
|
|
use crate::sandboxing::SandboxPermissions;
|
|
use crate::tools::hook_names::HookToolName;
|
|
use codex_protocol::protocol::GranularApprovalConfig;
|
|
use codex_protocol::protocol::NetworkAccess;
|
|
use pretty_assertions::assert_eq;
|
|
use serde_json::json;
|
|
|
|
#[test]
|
|
fn bash_permission_request_payload_omits_missing_description() {
|
|
assert_eq!(
|
|
PermissionRequestPayload::bash("echo hi".to_string(), /*description*/ None),
|
|
PermissionRequestPayload {
|
|
tool_name: HookToolName::bash(),
|
|
tool_input: json!({ "command": "echo hi" }),
|
|
}
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn bash_permission_request_payload_includes_description_when_present() {
|
|
assert_eq!(
|
|
PermissionRequestPayload::bash(
|
|
"echo hi".to_string(),
|
|
Some("network-access example.com".to_string()),
|
|
),
|
|
PermissionRequestPayload {
|
|
tool_name: HookToolName::bash(),
|
|
tool_input: json!({
|
|
"command": "echo hi",
|
|
"description": "network-access example.com",
|
|
}),
|
|
}
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn external_sandbox_skips_exec_approval_on_request() {
|
|
let sandbox_policy = SandboxPolicy::ExternalSandbox {
|
|
network_access: NetworkAccess::Restricted,
|
|
};
|
|
assert_eq!(
|
|
default_exec_approval_requirement(
|
|
AskForApproval::OnRequest,
|
|
&FileSystemSandboxPolicy::from(&sandbox_policy),
|
|
),
|
|
ExecApprovalRequirement::Skip {
|
|
bypass_sandbox: false,
|
|
proposed_execpolicy_amendment: None,
|
|
}
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn restricted_sandbox_requires_exec_approval_on_request() {
|
|
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
|
assert_eq!(
|
|
default_exec_approval_requirement(
|
|
AskForApproval::OnRequest,
|
|
&FileSystemSandboxPolicy::from(&sandbox_policy)
|
|
),
|
|
ExecApprovalRequirement::NeedsApproval {
|
|
reason: None,
|
|
proposed_execpolicy_amendment: None,
|
|
}
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn default_exec_approval_requirement_rejects_sandbox_prompt_when_granular_disables_it() {
|
|
let policy = AskForApproval::Granular(GranularApprovalConfig {
|
|
sandbox_approval: false,
|
|
rules: true,
|
|
skill_approval: true,
|
|
request_permissions: true,
|
|
mcp_elicitations: true,
|
|
});
|
|
|
|
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
|
let requirement =
|
|
default_exec_approval_requirement(policy, &FileSystemSandboxPolicy::from(&sandbox_policy));
|
|
|
|
assert_eq!(
|
|
requirement,
|
|
ExecApprovalRequirement::Forbidden {
|
|
reason: "approval policy disallowed sandbox approval prompt".to_string(),
|
|
}
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn default_exec_approval_requirement_keeps_prompt_when_granular_allows_sandbox_approval() {
|
|
let policy = AskForApproval::Granular(GranularApprovalConfig {
|
|
sandbox_approval: true,
|
|
rules: false,
|
|
skill_approval: true,
|
|
request_permissions: true,
|
|
mcp_elicitations: false,
|
|
});
|
|
|
|
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
|
let requirement =
|
|
default_exec_approval_requirement(policy, &FileSystemSandboxPolicy::from(&sandbox_policy));
|
|
|
|
assert_eq!(
|
|
requirement,
|
|
ExecApprovalRequirement::NeedsApproval {
|
|
reason: None,
|
|
proposed_execpolicy_amendment: None,
|
|
}
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn additional_permissions_allow_bypass_sandbox_first_attempt_when_execpolicy_skips() {
|
|
assert_eq!(
|
|
sandbox_override_for_first_attempt(
|
|
SandboxPermissions::WithAdditionalPermissions,
|
|
&ExecApprovalRequirement::Skip {
|
|
bypass_sandbox: true,
|
|
proposed_execpolicy_amendment: None,
|
|
},
|
|
),
|
|
SandboxOverride::BypassSandboxFirstAttempt
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn guardian_bypasses_sandbox_for_explicit_escalation_on_first_attempt() {
|
|
assert_eq!(
|
|
sandbox_override_for_first_attempt(
|
|
SandboxPermissions::RequireEscalated,
|
|
&ExecApprovalRequirement::Skip {
|
|
bypass_sandbox: false,
|
|
proposed_execpolicy_amendment: None,
|
|
},
|
|
),
|
|
SandboxOverride::BypassSandboxFirstAttempt
|
|
);
|
|
}
|