Support PreToolUse updatedInput rewrites#20527
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61724ae083
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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 review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/hooks/src/events/pre_tool_use.rs
Lines 103 to 106 in 04ae865
All matched PreToolUse hooks are launched with the single input_json serialized from the original request, and only after they finish is one updated_input selected. This lets one hook rewrite a command after other PreToolUse policy hooks have inspected only the old command, so the executed input can bypass blockers that would have denied the rewritten payload.
codex/codex-rs/core/src/tools/registry.rs
Lines 297 to 298 in 04ae865
log_payload is captured before PreToolUse can replace invocation.payload, but the later telemetry call logs this stale payload while the rewritten invocation is executed. Audit/telemetry records can therefore show a harmless original command even when hooks caused a different command or patch to run.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
I think this is working as intended under the trust model for hooks
Re-running the same trusted |
| let call_id_owned = invocation.call_id.clone(); | ||
| let otel = invocation.turn.session_telemetry.clone(); | ||
| let payload_for_response = invocation.payload.clone(); | ||
| let log_payload = payload_for_response.log_payload(); |
There was a problem hiding this comment.
shifted this down so we log the rewritten tool payload
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ 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". |
eternal-openai
left a comment
There was a problem hiding this comment.
hmm, one substantial concern and two minor ones:
- so an MCP tool like mcp__foo__exec_command has name = "exec_command" -> since we only switch on the name, we could mistake this MCP tool as being the real builtin one
- hooks spec says that when multiple PreToolUse hooks rewrite input, the last one to finish wins, with nondeterministic ordering because the hooks run in parallel. This implementation instead makes the last configured hook always win, which is a spec mismatch
- since the PR description says we only support updatedInput paired with "allow", bare "allow" should still be rejected until the actual allow semantics are implemented
…-input # 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/mcp.rs # codex-rs/core/src/tools/registry.rs
…-input # 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/mcp.rs # codex-rs/core/src/tools/registry.rs
Why
PreToolUsealready exposesupdatedInputin its hook output schema, but Codex currently rejects it instead of applying the rewrite. That leaves hook authors unable to make the documented pre-execution adjustment to a tool call before it runs.What
updatedInputfromPreToolUsehooks when paired withpermissionDecision: "allow".shell,container.exec,local_shell,shell_command,exec_command) use{ "command": ... }.apply_patchexposes its patch body through the same command-shaped hook contract.updatedInputback into its concrete invocation shape.Verification
Direct Bash-like rewrite coverage:
pre_tool_use_rewrites_shell_before_executionpre_tool_use_rewrites_container_exec_before_executionpre_tool_use_rewrites_local_shell_before_executionpre_tool_use_rewrites_shell_command_before_executionpre_tool_use_rewrites_exec_command_before_executionThese cases assert that each supported Bash-like surface runs only the rewritten command while the hook still observes the original
{ "command": ... }input.pre_tool_use_rewrites_apply_patch_before_executionpre_tool_use_rewrites_code_mode_nested_exec_command_before_executionpre_tool_use_rewrites_mcp_tool_before_execution