feat(hooks): implement comprehensive hook system with blocking semantics#1
feat(hooks): implement comprehensive hook system with blocking semantics#1RyderFreeman4Logos wants to merge 14 commits into
Conversation
[MOTIVATION]
The existing HookOutcome enum (Continue/Stop) only supports simple
pass/halt control flow. To enable hooks that can deny operations
or modify content (e.g. blocking dangerous tool calls, injecting
context), richer outcome semantics are needed.
[IMPLEMENTATION DETAILS]
- Rename Continue -> Proceed, Stop -> Block { message }
- Add Modify { content } variant for content transformation
- dispatch() now returns HookOutcome with aggregate semantics:
Block short-circuits immediately, Modify carries forward
- Remove Copy derive (String fields prevent it)
- Update all tests to use new variant names
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
[MOTIVATION] Enable hooks to be implemented as external commands that communicate via a structured JSON protocol. This is the foundation for blocking hooks (pre-tool-use, prompt-submit) where the hook script can inspect the payload and return a decision (proceed/block/modify). [IMPLEMENTATION DETAILS] - Add executor.rs with command_hook(argv, timeout) -> Hook - Payload sent as JSON via stdin, result read from stdout - Exit code 0 + empty stdout = proceed (fire-and-forget compatible) - Exit code 0 + JSON stdout = interpret decision field - Non-zero exit = block with stderr message - Timeout = block with timeout message - Spawn failure = proceed (fail-open for resilience) - HookCommandResult/HookDecision types for stdout deserialization - 5 unit tests for serialization and outcome conversion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
[MOTIVATION]
Define TOML-compatible configuration types for the hooks system,
enabling users to declare hooks per event type with command, timeout,
and optional tool-name matcher patterns in their config.toml.
[IMPLEMENTATION DETAILS]
- Add config.rs with HookEntryToml (command, timeout, matcher)
- Add HooksConfigToml grouping entries by event type
(after_agent, pre_tool_use, post_tool_use, notification, stop,
user_prompt_submit)
- hook_from_entry() converts config entries to Hook via executor
- matches_tool() supports exact match, prefix glob ("shell*"), and
wildcard ("*") patterns for tool-use hook filtering
- 9 unit tests for TOML deserialization and matcher logic
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
[MOTIVATION] Enable external scripts to intercept and observe tool executions, matching Claude Code's pre_tool_use and post_tool_use hook events. PreToolUse hooks can block tool calls before execution. [IMPLEMENTATION DETAILS] - Add PreToolUse dispatch before tool execution in ToolRouter - Add PostToolUse dispatch after tool execution with output text - Add ToolCallBlocked error variant for blocked tool calls - Handle ToolCallBlocked in stream_events_utils match arms - Add 6 HookEvent variants (PreToolUse, PostToolUse, Stop, UserPromptSubmit, Notification) with event structs - Expand Hooks registry with per-event-type hook vectors - Fix clippy issues in executor (expect → let-else, format args) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
[MOTIVATION] Ensure all hook system components have thorough test coverage including new event types, dispatch semantics, and legacy notification fallback. [IMPLEMENTATION DETAILS] - types.rs: 5 serialization tests for all event variants (PreToolUse, PostToolUse, Stop, UserPromptSubmit, Notification) - registry.rs: 5 dispatch tests for PreToolUse/PostToolUse routing, cross-event isolation, Modify carry-forward semantics - config.rs: 3 tests for TOML deserialization and hook_from_entry integration with command executor - user_notification.rs: 3 tests for legacy notification fallback behavior with new event types - Total: 41 tests passing across hooks module Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
[MOTIVATION] Code review identified security gap: stderr read had no size limit, allowing malicious hooks to exhaust memory. Also, the core command_hook() function had zero integration test coverage despite being the primary execution path. [IMPLEMENTATION DETAILS] - Add MAX_STDERR_BYTES (1MB) limit with truncation on overflow - Remove stale #[allow(dead_code)] on Block/Modify variants (now used in live code via router.rs and registry.rs dispatch) - Remove meaningless placeholder test (test_empty_stdout_returns_proceed) - Add 11 async integration tests for command_hook() covering: empty stdout, proceed/block/modify JSON responses, non-zero exit codes, timeout, invalid JSON (fail-open), spawn failure, empty argv Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d9751bf80
ℹ️ 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".
| if let HookOutcome::Block { message } = pre_outcome { | ||
| let block_msg = message.unwrap_or_else(|| "Blocked by pre_tool_use hook".to_string()); | ||
| return Ok(Self::failure_response( | ||
| failure_call_id, | ||
| payload_outputs_custom, |
There was a problem hiding this comment.
Only the Block case is checked here, so a HookOutcome::Modify returned by a pre_tool_use hook is silently ignored and the tool invocation proceeds with the original payload. This makes MODIFY semantics ineffective for pre-tool hooks (e.g., hooks that try to rewrite tool input or redact secrets). Consider applying the modified content to the payload or otherwise honoring Modify before dispatching the tool.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 4c95048. The pre-tool hook dispatch now uses an exhaustive match on all three HookOutcome variants. Modify { content } is handled by applying the modified content to the tool arguments (Function, Mcp, and Custom payloads). LocalShell logs a warning since modifying shell command structure from a hook is not safe.
| // Read stderr (best effort, size-limited to prevent memory exhaustion) | ||
| let mut stderr_bytes = Vec::new(); | ||
| let mut stderr_buf = [0u8; 4096]; | ||
| loop { | ||
| match stderr.read(&mut stderr_buf).await { | ||
| Ok(0) => break, |
There was a problem hiding this comment.
The timeout only wraps the stdout read. After stdout completes, stderr is read in a blocking loop and child.wait() is awaited without any timeout, so a hook that closes stdout but keeps running (or leaves stderr open) can hang indefinitely and bypass the configured timeout. This can stall tool execution until the process exits. Consider applying the timeout to the entire hook execution or adding a timeout around stderr read and the wait.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 4c95048. The timeout now wraps the entire IO sequence (stdin write + concurrent stdout/stderr drain via tokio::join!) in a single tokio::time::timeout, and child.wait() has its own 5-second grace period timeout. Pipes are also drained concurrently to prevent deadlocks, and continue draining after hitting size caps instead of breaking.
…ation Connect the hook configuration types to the main Config struct so hooks can be configured via config.toml [hooks] section. This closes the gap that previously prevented hooks from being loaded at startup. - Add `hooks` field to both ConfigToml and Config structs - Make HookEntryToml/HooksConfigToml pub with JsonSchema/PartialEq derives - Populate Hooks registry from HooksConfigToml in Hooks::new() - Move test-only matches_tool() into #[cfg(test)] module - Fix clippy collapsible_if in matcher closure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Wrap entire hook IO (stdin write, concurrent stdout/stderr drain, child.wait) in a single timeout to prevent hangs at any phase - Drain stdout/stderr concurrently via tokio::join! to avoid pipe deadlock when a hook writes verbose output to both streams - Continue draining pipes after hitting size caps instead of breaking, preventing child process from blocking on full pipes - Keep overflow chunk bytes when capping stdout to preserve valid JSON that only slightly exceeds the limit - On stdin write failure, continue reading stdout/stderr and exit status instead of returning Proceed, so block/modify decisions from hooks that close stdin early are not silently lost - Handle pre-tool Modify outcome by applying modified content to tool arguments (Function, Mcp, Custom payloads) - Extract text from McpToolCallOutput in post-tool hook payloads using existing FunctionCallOutputPayload conversion - Spawn post-tool hook dispatch as background task to avoid adding latency to the tool response path - Remove stale #[allow(dead_code)] annotations - Regenerate config.schema.json to include hooks field Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c95048624
ℹ️ 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".
| } | ||
| ToolPayload::Custom { input } => { | ||
| *input = content; | ||
| } | ||
| ToolPayload::LocalShell { .. } => { | ||
| tracing::warn!( | ||
| "pre_tool_use hook returned Modify for LocalShell which is not supported; proceeding without modification" | ||
| ); |
There was a problem hiding this comment.
The pre_tool_use hook’s Modify outcome is silently ignored for ToolPayload::LocalShell, so any hook that tries to sanitize or rewrite local_shell arguments will still execute the original command. This creates a policy bypass when users expect hooks to enforce changes (e.g., stripping dangerous flags). Consider parsing the modified content into ShellToolCallParams or treating Modify as Block for local_shell so the hook’s intent is enforced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 9e205d5. LocalShell Modify now returns ToolCallBlocked instead of silently proceeding. If a pre_tool_use hook tries to modify a local_shell call, the execution is blocked with an explicit error message, ensuring the hook's policy intent is enforced.
Treating unsupported Modify as a no-op for LocalShell creates a policy bypass: hooks that intend to sanitize shell arguments would silently fail while the original command executes unchanged. Now return ToolCallBlocked so the hook's intent is enforced. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e205d5232
ℹ️ 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".
| // A tool call was blocked by a pre_tool_use hook; surface the block message back into the transcript. | ||
| Err(FunctionCallError::ToolCallBlocked(message)) => { | ||
| let response = ResponseInputItem::FunctionCallOutput { | ||
| call_id: String::new(), | ||
| output: FunctionCallOutputPayload { |
There was a problem hiding this comment.
When a pre_tool_use hook blocks a tool call, this path emits a FunctionCallOutput with call_id set to the empty string. During history normalization, outputs without a matching call ID are removed (context_manager/normalize.rs::remove_orphan_outputs), which means the block message gets dropped and replaced by a synthetic "aborted" output; it also triggers error_or_panic for the orphan output. This only happens for blocked tool calls, but it prevents the user from seeing the hook’s block reason in the transcript. Consider reusing the call_id from the original tool call item so the output is retained.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit fd576cf. The ToolCallBlocked handler now extracts the actual call_id from the original ResponseItem (FunctionCall, LocalShellCall, or CustomToolCall) instead of using an empty string. This ensures the blocked output is properly matched during history normalization and the block reason is visible in the transcript.
When a pre_tool_use hook blocks a tool call, the blocked output used an empty call_id which causes orphan output removal during history normalization. Extract the actual call_id from the original ResponseItem so the block message is retained in the transcript. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd576cf1df
ℹ️ 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".
| let response = ResponseInputItem::FunctionCallOutput { | ||
| call_id: blocked_call_id, | ||
| output: FunctionCallOutputPayload { | ||
| body: FunctionCallOutputBody::Text(message), | ||
| ..Default::default() | ||
| }, |
There was a problem hiding this comment.
When a pre_tool_use hook blocks a ResponseItem::CustomToolCall, this branch always builds a ResponseInputItem::FunctionCallOutput. That later becomes ResponseItem::FunctionCallOutput via response_input_to_response_item, so the model never receives the expected CustomToolCallOutput for the blocked custom tool. In that scenario the tool call stays unresolved (or is treated as a mismatched response type), which can stall or confuse the conversation. Consider mirroring failure_response behavior and emit CustomToolCallOutput when the original item was a custom tool call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dfa9ad0. Both ToolCallBlocked and RespondToModel arms now detect CustomToolCall items and emit CustomToolCallOutput instead of FunctionCallOutput, mirroring the failure_response pattern used in router.rs.
When a pre_tool_use hook blocks a CustomToolCall, emit CustomToolCallOutput instead of FunctionCallOutput so the model receives the expected response type. Also fix RespondToModel arm with the same issue. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfa9ad0416
ℹ️ 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".
| match serde_json::from_slice::<HookCommandResult>(&stdout_bytes) { | ||
| Ok(result) => result.into(), | ||
| Err(err) => { | ||
| tracing::warn!("failed to parse hook command result: {err}"); | ||
| HookOutcome::Proceed |
There was a problem hiding this comment.
When a hook writes more than MAX_STDOUT_BYTES, stdout is truncated earlier in read_stdout, but the truncated buffer is still parsed here. That means a hook that returns a valid decision JSON plus large output (e.g., a modify with >1MB content or verbose logging before the JSON) will hit a parse error and fall back to HookOutcome::Proceed, silently bypassing the hook’s intended block/modify behavior. Consider tracking the “capped” state and returning a Block (or at least a clear error) when stdout was truncated so oversized hook output can’t accidentally disable enforcement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6c4a706. The capped flag is now propagated from read_stdout and checked before JSON parsing. When stdout was truncated, the executor returns Block with a clear message instead of attempting to parse corrupted JSON and falling through to Proceed.
…roceed When hook stdout exceeds MAX_STDOUT_BYTES the output is truncated, which corrupts any JSON decision the hook emitted. Previously, the corrupted bytes were still fed to serde which would fail and fall back to Proceed, silently bypassing the hook's intended block/modify. Now the capped flag is propagated and triggers an explicit Block so oversized hook output cannot disable enforcement. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c4a70635e
ℹ️ 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".
| // Extract tool input before moving payload into invocation. | ||
| let tool_input = payload.log_payload().into_owned(); |
There was a problem hiding this comment.
The pre_tool_use hook payload is built from payload.log_payload(), which is a logging helper and is lossy for ToolPayload::LocalShell (it joins params.command with spaces and drops fields like workdir, timeout_ms, and sandbox_permissions from ShellToolCallParams in tools/context.rs). That means hooks intended to enforce policy on local shell calls can't reliably inspect the real arguments or execution context (e.g., commands with space-containing args or unsafe workdirs), so a policy that should block can be bypassed. Consider serializing the full shell params (or the original LocalShellCall JSON) into tool_input instead of using the log preview helper.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex Disagree for current scope. The concern about lossy log_payload() for LocalShell overlooks existing context and overstates the practical risk:
-
Working directory is already available.
HookPayloadincludescwd: PathBuf(seetypes.rs:38), which is the session's working directory and the default execution directory for shell commands.ShellToolCallParams.workdiris an optional override that is almost alwaysNone(seerouter.rs:121— only set fromexec.working_directory). So hooks already have the relevant directory context. -
Space-joining preserves practical fidelity. For policy hooks checking command patterns (e.g., blocking
rm -rforcurl | bash),command.join(" ")produces the same string a human would type. The theoretical ambiguity between["bash", "-c", "echo hello world"]and["bash", "-c", "echo", "hello", "world"]doesn't affect pattern-matching hooks, because both representations carry the same semantic information for policy decisions. -
sandbox_permissionsis alwaysUseDefault. Line 123 inrouter.rshardcodesSome(SandboxPermissions::UseDefault)— exposing this to hooks provides no actionable information. -
Protocol crate boundary.
ShellToolCallParamsderivesDeserializebut notSerialize(seeprotocol/src/models.rs:745). AddingSerializeto the protocol crate to serve a hook implementation detail is a cross-crate API change that should be driven by a concrete use case, not a theoretical gap.
Disposition: Acknowledged as a valid future enhancement. When a hook author demonstrates a concrete case where space-joined output is insufficient, we'll add structured serialization. For the MVP hook system, the current representation is adequate.
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
There was a problem hiding this comment.
@codex The environment is now set up. Please address the counter-arguments above:
HookPayload.cwdalready provides the working directory context —ShellToolCallParams.workdiris almost alwaysNone- Space-joining preserves the same semantic information for policy pattern matching
sandbox_permissionsis hardcoded toUseDefaultand provides no actionable info- Adding
Serializeto the protocol crate is a cross-crate change that should be driven by a concrete use case
Do you still consider this a real issue given these points, or would you agree this is acceptable for the MVP hook system?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c4a70635e
ℹ️ 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".
| // Extract tool input before moving payload into invocation. | ||
| let tool_input = payload.log_payload().into_owned(); |
There was a problem hiding this comment.
The pre_tool_use hook payload is built from payload.log_payload(), which for ToolPayload::LocalShell only joins command with spaces. That drops important fields like workdir, timeout_ms, and sandbox_permissions, and also loses argument boundaries (e.g., ['echo','a b'] vs ['echo','a','b']). As a result, hook policies cannot reliably block or allow based on the actual execution context — e.g., a tool call with workdir: "/etc" will still present the hook with the session cwd and a flattened command, so a policy that expects to block commands outside the cwd can be bypassed. Consider serializing the full ShellToolCallParams (or at least include workdir and arguments as structured data) for hook input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Your workdir: "/etc" example is valid — the old code used log_payload() which joins the command array with spaces (lossy) and drops the workdir override entirely, so hooks would only see the session cwd, not the model-specified workdir.
Fixed by adding a dedicated hook_input() method to ToolPayload that serializes LocalShell params as structured JSON:
{"command": ["ls", "-la"], "workdir": "/etc", "timeout_ms": 30000}This preserves argument boundaries and exposes the workdir override so security hooks can enforce accurate policies.
See upcoming commit for the implementation.
Replace log_payload() with a new hook_input() method that serializes LocalShell params as structured JSON (command array + workdir + timeout), preserving argument boundaries and exposing workdir overrides so security hooks can enforce accurate policies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bdceef3c3
ℹ️ 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".
| ToolPayload::LocalShell { params } => { | ||
| let mut obj = serde_json::json!({ | ||
| "command": params.command, | ||
| }); | ||
| if let Some(workdir) = ¶ms.workdir { | ||
| obj["workdir"] = serde_json::Value::String(workdir.clone()); | ||
| } | ||
| if let Some(timeout_ms) = params.timeout_ms { | ||
| obj["timeout_ms"] = serde_json::Value::Number(timeout_ms.into()); | ||
| } |
There was a problem hiding this comment.
Pre-tool hooks for LocalShell only see command, workdir, and timeout_ms because hook_input() rebuilds a partial JSON object from ShellToolCallParams. That drops fields like sandbox_permissions, prefix_rule, and justification (defined on ShellToolCallParams), so a policy hook cannot distinguish an escalated/sandbox-permissions request from a normal one. In practice, a pre_tool_use hook that is supposed to block or log escalated calls will silently miss them, allowing commands to run with higher permissions than the hook intended to permit. Consider serializing the full ShellToolCallParams instead of reconstructing a partial object.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disagree. These three fields are hardcoded constants in the sole construction site (router.rs:119-125):
sandbox_permissions: Some(SandboxPermissions::UseDefault), // always
prefix_rule: None, // always
justification: None, // alwaysThey carry zero information — a hook cannot distinguish anything by observing them because they never vary. Including them would add noise without policy value.
Additionally, ShellToolCallParams does not derive Serialize (only Deserialize), and SandboxPermissions is an internal enum — serializing the full struct would require protocol crate changes for zero practical benefit.
The hook_input() method intentionally exposes only the model-controllable fields (command, workdir, timeout_ms) since those are the ones hooks need for security decisions. If these hardcoded fields ever become dynamic, we should include them at that point — but right now, serializing constants is premature.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bdceef3c3
ℹ️ 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".
| ToolPayload::LocalShell { params } => { | ||
| let mut obj = serde_json::json!({ | ||
| "command": params.command, | ||
| }); | ||
| if let Some(workdir) = ¶ms.workdir { |
There was a problem hiding this comment.
For local_shell calls, hook_input() only serializes command, workdir, and timeout_ms, but it drops sandbox_permissions, prefix_rule, and justification from ShellToolCallParams. Any pre_tool_use hook policy that needs those fields (for example, blocking escalated sandbox_permissions or enforcing an allowed prefix_rule) cannot make a correct decision and will silently permit calls that should be blocked. Consider serializing the full params struct (or at least these missing fields) so hooks can enforce security policy based on the complete request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Duplicate of previous comment. See the reply there — these three fields (sandbox_permissions, prefix_rule, justification) are hardcoded constants in the sole construction site (router.rs:119-125), carry zero information, and serializing them would require protocol crate changes for no practical benefit.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bdceef3c3
ℹ️ 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".
| pub(crate) async fn dispatch(&self, hook_payload: HookPayload) -> HookOutcome { | ||
| let mut result = HookOutcome::Proceed; | ||
| for hook in self.hooks_for_event(&hook_payload.hook_event) { | ||
| let outcome = hook.execute(&hook_payload).await; | ||
| if matches!(outcome, HookOutcome::Stop) { | ||
| break; | ||
| match &outcome { | ||
| HookOutcome::Block { .. } => return outcome, | ||
| HookOutcome::Modify { .. } => { | ||
| result = outcome; |
There was a problem hiding this comment.
The hook dispatch loop always passes the original hook_payload into every hook, even after a prior hook returns Modify. That means later hooks still see the pre‑modified tool input, so any validation/logging hooks run against stale data and may allow policy bypasses (e.g., a redaction hook runs first but a later auditing hook logs the unredacted input). If Modify is intended to be “carried forward,” the payload should be updated before invoking subsequent hooks (or dispatch should stop on Modify).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — partially. The doc comment on line 89-90 says "the modified content is carried forward" but the implementation doesn't actually do that: each hook sees the original hook_payload, and only the last Modify result is returned to the caller.
Two separate issues here:
-
Doc accuracy (real bug): The comment claims carry-forward semantics that don't exist. Will fix.
-
Multi-hook chaining (valid design concern, deferred): For the current scope, hooks are expected to be independent (0-1 per event type in typical configs). True multi-hook pipelines where order and carry-forward matter would require mutating
HookPayload.hook_event.tool_inputbetween invocations — a meaningful design change better addressed when there's a concrete multi-hook use case.
Will fix the misleading doc comment in the clean PR.
|
Summary
Testing
|
Fixes openai#9050 When a draft is stashed with Ctrl+C, we now persist the full draft state (text elements, local image paths, and pending paste payloads) in local history. Up/Down recall rehydrates placeholder elements and attachments so styling remains correct and large pastes still expand on submit. Persistent (cross‑session) history remains text‑only. Backtrack prefills now reuse the selected user message’s text elements and local image paths, so image placeholders/attachments rehydrate when rolling back. External editor replacements keep only attachments whose placeholders remain and then normalize image placeholders to `[Image #1]..[Image #N]` to keep the attachment mapping consistent. Docs: - docs/tui-chat-composer.md Testing: - just fix -p codex-tui - cargo test -p codex-tui Co-authored-by: Eric Traut <etraut@openai.com>
1 participant
Summary
Implements a full hook system for codex-rs matching Claude Code's hook functionality, building on the existing PR openai#9691 hooks service framework.
shell*,*, exact match)Commits
refactor(hooks)- Redesign HookOutcome with Block/Modify semanticsfeat(hooks)- Add command executor with stdin/stdout JSON protocolfeat(hooks)- Add hook configuration types and matcher mechanismfeat(hooks)- Wire up PreToolUse/PostToolUse hooks in tool dispatchtest(hooks)- Add comprehensive tests for hook systemfix(hooks)- Harden executor (stderr limit, stale dead_code cleanup, 11 integration tests)style(hooks)- Apply cargo fmt formatting fixesDeferred (separate PRs)
Test plan
cargo test -p codex-core -- hooks(51 tests pass)cargo clippy -p codex-core --all-features --tests -- -D warnings(zero warnings)cargo fmt(fully formatted)🤖 Generated with Claude Code