feat(hooks): comprehensive hook system for tool execution by RyderFreeman4Logos · Pull Request #2 · RyderFreeman4Logos/codex

RyderFreeman4Logos · 2026-02-08T03:24:44Z

Summary

Add PreToolUse and PostToolUse hook events with Block/Modify/Proceed semantics
Implement command executor with stdin/stdout JSON protocol and configurable timeouts
Wire hooks into ToolRouter dispatch with structured LocalShell param serialization
Add JSON schema and TOML-based hook configuration with glob matchers
Comprehensive test coverage for executor, registry, matcher, and integration

Background

Clean resubmission of #1. The original PR went through multiple rounds of iterative review with @codex. Fix commits have been consolidated into 5 logical groups here.

See #1 for the full review discussion.

Test plan

cargo clippy -p codex-core -- -D warnings
cargo test -p codex-core
@codex review

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

Introduce the core abstractions for the hook system: - HookEvent enum (PreToolUse, PostToolUse) with typed payloads - HookOutcome with Proceed/Block/Modify semantics for pre-hooks - HookPayload envelope with session_id, cwd, and timestamp - HooksConfigToml with TOML-based hook configuration and glob matchers - ToolCallBlocked error variant for hook-blocked tool calls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Implement HookCommandExecutor that spawns hook commands as child processes, communicates via stdin/stdout JSON, and supports: - Configurable timeout with graceful kill + force-kill fallback - Structured JSON input (HookPayload) and output parsing - Stdout truncation handling (blocks execution on incomplete output) - Comprehensive test coverage including timeout and error scenarios Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Extend HookRegistry to dispatch HookPayload through matching hooks, aggregate outcomes (Block > Modify > Proceed), and integrate with user notifications for hook-level feedback. Includes test coverage for matcher logic, outcome aggregation, and notification integration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Wire hooks into ToolRouter::dispatch_tool_call: - PreToolUse hook runs before tool execution with Block/Modify support - PostToolUse hook fires asynchronously after execution (fire-and-forget) - Add hook_input() for structured LocalShell param serialization - Emit correct output types (CustomToolCallOutput for custom tools) - Block Modify attempts on LocalShell payloads for safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Add hook configuration to config.schema.json with pre_tool_use and post_tool_use arrays, each supporting command, timeout_ms, and tool_name_match glob patterns. Wire HooksConfigToml loading into the Config struct for end-to-end configuration support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

RyderFreeman4Logos · 2026-02-08T03:24:57Z

@codex review

The dispatch() doc comment claimed modified content is "carried forward" between hooks, but the implementation passes the original payload to each hook. Clarify that the last Modify wins and subsequent hooks see the original payload. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eb2a2aa6d8

ℹ️ 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".

chatgpt-codex-connector · 2026-02-08T03:27:20Z

+    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;
+                }


Apply Modify payload before running subsequent hooks

The dispatcher keeps passing the original hook_payload to every hook even after a hook returns Modify. That means later hooks in the same event never see the modified tool_input, which defeats chaining (e.g., a first hook sanitizes input and a second hook validates it, but the second still sees the unsanitized content). The comment above says the modified content is “carried forward,” but the code only updates result, not the payload passed to later hooks, so the behavior will be inconsistent with expectations whenever multiple hooks are configured.

Useful? React with 👍 / 👎.

Already fixed. The misleading doc comment was corrected in commit 219678376 (pushed after the reviewed commit). The comment now accurately states that subsequent hooks see the original payload and the last Modify wins.

chatgpt-codex-connector · 2026-02-08T03:27:20Z

+/// Check if a tool name matches a glob pattern.
+/// Supports: "*" matches anything, "shell*" matches "shell", "shell_exec", etc.,
+/// exact string match for patterns without wildcards.
+fn matches_pattern(pattern: &str, tool_name: &str) -> bool {
+    if pattern == "*" {
+        return true;
+    }
+    if let Some(prefix) = pattern.strip_suffix('*') {
+        return tool_name.starts_with(prefix);
+    }
+    pattern == tool_name


Documented glob matcher does not match common patterns

The matcher is described as a glob pattern, but the implementation only supports * as a full wildcard or a trailing prefix wildcard. Patterns like read_* or *shell will never match even though they are valid glob shapes users are likely to try. This will cause hooks to silently not fire for those matchers. Either broaden the matcher to real glob semantics or tighten the documentation/schema to reflect prefix-only behavior.

Useful? React with 👍 / 👎.

Partially disagree. read_* actually works fine — strip_suffix('*') yields "read_" and tool_name.starts_with("read_") matches correctly. You can verify in the tests.

However, *shell (suffix match) indeed won't work, and the doc comment saying "glob pattern" overpromises. The implementation is intentionally prefix-only for simplicity — full glob semantics would require a glob crate dependency for marginal benefit.

Proposal: rename "glob pattern" to "match pattern" in the doc comment and add a note about supported syntax. @codex do you agree this is sufficient, or do you think full glob support is needed?

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2196783764

ℹ️ 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".

chatgpt-codex-connector · 2026-02-08T03:49:12Z

+fn matches_pattern(pattern: &str, tool_name: &str) -> bool {
+    if pattern == "*" {
+        return true;
+    }
+    if let Some(prefix) = pattern.strip_suffix('*') {
+        return tool_name.starts_with(prefix);


Restrict matcher semantics or support full glob

The matcher logic only strips a trailing * and then does starts_with, so patterns with wildcards anywhere else (e.g. "*shell" or "read_*_file") never match and the hook silently won’t run. Because the config/schema and comment describe this as a “glob pattern,” users can plausibly configure suffix/infix globs and assume hooks are enforced when they aren’t, which is especially risky for security/guardrail hooks. Either tighten the docs to explicitly say “prefix-only (or *)” or implement real glob matching.

Useful? React with 👍 / 👎.

Fixed in 851af50. Updated doc comments and schema to accurately describe the matcher as supporting three forms: "*" (any tool), "prefix*" (prefix match), or exact tool name. Explicitly noted that suffix/infix wildcards are not supported.

The matcher documentation said "glob pattern" but the implementation only supports prefix wildcards (e.g. "shell*"), exact matches, and the universal wildcard "*". Suffix and infix patterns are not supported. Updated doc comments and schema description to accurately describe the supported matching semantics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

RyderFreeman4Logos · 2026-02-08T03:55:27Z

@codex review

chatgpt-codex-connector · 2026-02-08T04:02:42Z

Codex Review: Didn't find any major issues. Chef's kiss.

ℹ️ 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".

RyderFreeman4Logos · 2026-02-08T04:05:24Z

Superseded by #3. Clean resubmission with fix commits consolidated into logical groups. Preserved for review discussion reference.

RyderFreeman4Logos and others added 5 commits February 7, 2026 19:23

RyderFreeman4Logos mentioned this pull request Feb 8, 2026

feat(hooks): implement comprehensive hook system with blocking semantics #1

Closed

4 tasks

chatgpt-codex-connector Bot reviewed Feb 8, 2026

View reviewed changes

RyderFreeman4Logos mentioned this pull request Feb 8, 2026

feat(hooks): comprehensive hook system with lifecycle events and command executor #3

Closed

3 tasks

RyderFreeman4Logos closed this Feb 8, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hooks): comprehensive hook system for tool execution#2

feat(hooks): comprehensive hook system for tool execution#2
RyderFreeman4Logos wants to merge 7 commits into
mainfrom
feat/hooks-system-clean

RyderFreeman4Logos commented Feb 8, 2026

Uh oh!

RyderFreeman4Logos commented Feb 8, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Feb 8, 2026

Uh oh!

RyderFreeman4Logos Feb 8, 2026

Uh oh!

chatgpt-codex-connector Bot Feb 8, 2026

Uh oh!

RyderFreeman4Logos Feb 8, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Feb 8, 2026

Uh oh!

RyderFreeman4Logos Feb 8, 2026

Uh oh!

RyderFreeman4Logos commented Feb 8, 2026

Uh oh!

chatgpt-codex-connector Bot commented Feb 8, 2026

Uh oh!

RyderFreeman4Logos commented Feb 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

RyderFreeman4Logos commented Feb 8, 2026

Summary

Background

Test plan

Uh oh!

RyderFreeman4Logos commented Feb 8, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

RyderFreeman4Logos Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

RyderFreeman4Logos Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

RyderFreeman4Logos Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

RyderFreeman4Logos commented Feb 8, 2026

Uh oh!

chatgpt-codex-connector Bot commented Feb 8, 2026

Uh oh!

RyderFreeman4Logos commented Feb 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant