Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # codex-rs/core/src/tools/handlers/unified_exec.rs # codex-rs/core/src/tools/registry.rs # codex-rs/core/src/unified_exec/mod.rs
| result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; | ||
| Some(PostToolUsePayload { | ||
| tool_name: invocation.tool_name.display(), | ||
| tool_input: mcp_hook_tool_input(raw_arguments), |
There was a problem hiding this comment.
The hook payload here is built from raw_arguments, but the actual dispatch path can rewrite MCP arguments before the server call, for example when openai/fileParams uploads a local file and replaces the path with the provided-file payload. The hooks spec defines PostToolUse.tool_input as the arguments sent to the tool, so this will make post hooks audit the wrong request on those paths.
This comment was marked as outdated.
This comment was marked as outdated.
74b0d89 to
eb4bc8a
Compare
Keep ToolRegistry focused on dispatching and let the hook runtime return the final model-facing block message for Bash and MCP tool calls. Co-authored-by: Codex <noreply@openai.com>
eb4bc8a to
8414af5
Compare
Align apply_patch and MCP permission payloads with the rebased HookToolName plus generic tool_input model. Co-authored-by: Codex <noreply@openai.com>
8414af5 to
9d32374
Compare
| } else { | ||
| format!( | ||
| "Tool call blocked by PreToolUse hook: {reason}. Tool: {}", | ||
| tool_name.name() |
There was a problem hiding this comment.
should we also show the tool call with the input?
8f7e88a to
9d32374
Compare
…tool_use' into abhinav/hooks-mcp-tools-support # Conflicts: # codex-rs/core/src/tools/handlers/apply_patch.rs # codex-rs/core/src/tools/handlers/apply_patch_tests.rs # codex-rs/core/src/tools/handlers/shell.rs # codex-rs/core/src/tools/handlers/shell_tests.rs # codex-rs/core/src/tools/handlers/unified_exec.rs # codex-rs/core/src/tools/handlers/unified_exec_tests.rs # codex-rs/core/src/tools/registry.rs # codex-rs/hooks/src/events/common.rs
| pub(crate) tool_input: JsonValue, | ||
| } | ||
|
|
||
| async fn handle_approved_mcp_tool_call( |
There was a problem hiding this comment.
pulled this into a helper because we had two “allowed to execute” paths doing the same work
- explicit approval (
Accept*) - no-approval-needed (
None)
also when rewrite_mcp_tool_arguments_for_openai_files rewrites file params, the same rewritten args are sent to the MCP server and exposed to PostToolUse; otherwise we fall back to the original model args which was the issue @eternal-openai pointed out
…ls-support # Conflicts: # codex-rs/core/src/tools/handlers/apply_patch.rs # codex-rs/core/src/tools/handlers/apply_patch_tests.rs # codex-rs/core/src/tools/handlers/shell.rs # codex-rs/core/src/tools/handlers/shell_tests.rs # codex-rs/core/src/tools/handlers/unified_exec.rs # codex-rs/core/src/tools/handlers/unified_exec_tests.rs # codex-rs/core/src/tools/registry.rs # codex-rs/hooks/src/events/common.rs
PR 18385 MCP Hook EvidenceGenerated from a live Setup
Summary Matrix
Scenario EvidenceSCENARIO_PRE_ALLOW
Mini chat transcript: SCENARIO_MATCHER_CWD
Mini chat transcript: SCENARIO_DASH_TOOL
Mini chat transcript: SCENARIO_PRE_DENY
Mini chat transcript: SCENARIO_PERMISSION_ALLOW
Mini chat transcript: SCENARIO_PERMISSION_DENY
Mini chat transcript: SCENARIO_POST_CONTEXT
Mini chat transcript: Raw Evidence Files
Notes
|
Summary
Lifecycle hooks currently treat
PreToolUse,PostToolUse, andPermissionRequestas Bash-only flowstool_nametoBashtool_inputThat 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.rscodex-rs/core/src/mcp_tool_call.rsOtherwise the changes across apply_patch, shell, and unified_exec are mainly to rewire everything to be
tool_inputbased instead of justcommandso that it'll make sense for MCP tools.Changes
PreToolUse,PostToolUse, andPermissionRequesthook inputs to carry arbitrarytool_nameandtool_inputvalues instead of hard-codingBashand command-only payloads.McpHandler, using the model-visible tool name fromToolInvocationand the raw MCP arguments astool_input.PostToolUseby serializingMcpToolOutputinto the hook response payload.PermissionRequesthooks for MCP approval requests after remembered approval checks and before falling back to user-facing MCP elicitation.Bashandmcp__memory__create_entities, while keeping regex matcher support for patterns likemcp__memory__.*andmcp__.*__write.*.