feat(hooks): comprehensive hook system with lifecycle events and command executor by RyderFreeman4Logos · Pull Request #3 · RyderFreeman4Logos/codex

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

Summary

Add hook event types covering all agent lifecycle events (PreToolUse, PostToolUse, AfterAgent, UserPromptSubmit, Stop, Notification)
Implement command executor with stdin/stdout JSON protocol and configurable timeouts
Wire hook dispatch into tool execution pipeline with Block/Modify/Proceed outcomes
Support simple pattern matching for tool-name filtering (exact, prefix*, *)
Extend configuration schema and wire HooksConfigToml into Config

Background

Clean resubmission of #2. The original PR went through 3 rounds of iterative review with @codex. Fix commits (doc clarifications for matcher semantics and Modify carry-forward behavior) have been consolidated into logical groups here.

See #2 for the full review discussion.

Test plan

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

🤖 Generated with Claude Code

Introduce the foundational types for the hook system: - HookEvent enum covering all lifecycle events (PreToolUse, PostToolUse, AfterAgent, UserPromptSubmit, Stop, Notification) - HookOutcome enum with Proceed, Block, and Modify variants - HookPayload with session metadata and event details - HookEntryToml / HooksConfigToml for TOML configuration parsing - Simple pattern matcher supporting exact, prefix*, and * wildcards (suffix/infix patterns intentionally not supported) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Implement the hook execution engine that spawns external commands: - JSON payload piped to stdin, JSON outcome read from stdout - Configurable per-hook timeout with graceful process cleanup - Outcome parsing: exit 0 = Proceed, exit 2 = Block, stdout JSON for Modify results - Comprehensive error handling (spawn failure, timeout, bad output) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Upgrade the hook registry from notification-only to full multi-event dispatch: - Register hooks by event type with synchronous sequential execution - Last-Modify-wins semantics (subsequent hooks see original payload) - Block short-circuits immediately - Refactor legacy user_notification to use standard hook dispatch - Add HooksConfigToml -> Hooks conversion via hook_from_entry Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Wire hook dispatch into the tool execution pipeline: - Fire PreToolUse before tool execution with structured JSON input - Respect Block outcome to skip tool execution - Fire PostToolUse after execution with tool output - Add hook_input() method for structured serialization of tool args (preserves shell arg boundaries and workdir overrides) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Extend the configuration system to support hooks: - Add hooks section to config.schema.json with all event types - Wire HooksConfigToml into the main Config struct - Register configured hooks in the Hooks registry at startup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dbb104c9ca

ℹ️ 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-08T04:08:11Z

+            ToolPayload::LocalShell { params } => {
+                let mut obj = serde_json::json!({
+                    "command": params.command,
+                });
+                if let Some(workdir) = &params.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());


Include all shell args in hook payload

The new hook_input JSON for LocalShell only includes command, workdir, and timeout_ms, so any hook policy that needs to inspect sandbox_permissions, prefix_rule, or justification (all part of ShellToolCallParams) cannot see them and will silently allow elevated/sandbox-bypassing requests. This defeats the stated goal of using hooks for security policies; a pre_tool_use hook intended to block require_escalated cannot do so because that field is omitted from the payload.

Useful? React with 👍 / 👎.

Disagree. These three fields are hardcoded constants in router.rs:119-125, not per-call parameters set by the model:

sandbox_permissions: always SandboxPermissions::UseDefault

prefix_rule: always None

justification: always None

The model controls only command, workdir, and timeout_ms — all of which are already included in the hook payload. Including hardcoded constants would add noise without enabling any additional policy decisions.

If/when the model gains the ability to request per-call escalation, the hook payload should be extended accordingly. But today these fields carry zero information.

This was previously debated in #2 (comment) and in #1. @codex

Summary

Clarified that LocalShell hook payloads only include per-call parameters and omit constant defaults like sandbox permissions in the hook input documentation. codex-rs/core/src/tools/context.rsL57-L78

Testing

✅ just fmt

⚠️ cargo test -p codex-core (fails in this environment: loopback requests are forbidden for model refresh tests, and exec/shell_snapshot tests fail due to shell/tooling constraints)

View task →

RyderFreeman4Logos · 2026-02-08T04:10:12Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dbb104c9ca

ℹ️ 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-08T04:14:15Z

+            ToolPayload::LocalShell { params } => {
+                let mut obj = serde_json::json!({
+                    "command": params.command,
+                });
+                if let Some(workdir) = &params.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());
+                }


Include all LocalShell args in hook_input

Pre-tool hooks only receive the JSON built here, which currently includes command, workdir, and timeout_ms but omits sandbox_permissions, prefix_rule, and justification from ShellToolCallParams. That means a policy hook cannot detect or block calls that request escalated sandbox permissions or rely on those other fields, so it can silently allow actions it intended to deny. Consider serializing the full params or explicitly adding the missing fields so hooks can enforce policies based on the complete tool input.

Useful? React with 👍 / 👎.

Dismissed after local arbitration.

Bot's concern

hook_input() omits sandbox_permissions, prefix_rule, and justification from the JSON payload, potentially allowing hooks to silently miss escalated sandbox requests.

Independent local review (GPT Codex, read-only sandbox)

The local reviewer traced the full call chain and concluded false positive:

Protocol layer: LocalShellExecAction (protocol/src/models.rs:664) has only command/timeout_ms/working_directory/env/user. The model cannot submit these three fields via the LocalShellCall path.

Router layer: router.rs:119-125 hardcodes sandbox_permissions: Some(UseDefault), prefix_rule: None, justification: None — they are not dynamic per-call inputs.

Conclusion: Including hardcoded constants in the hook payload would add noise without enabling any additional policy decisions. If/when the protocol gains per-call escalation support, the hook payload should be extended accordingly.

Verdict: FALSE POSITIVE

No code changes needed. The hook payload correctly includes all model-controlled fields.

RyderFreeman4Logos · 2026-02-08T04:42:22Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dbb104c9ca

ℹ️ 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-08T04:48:08Z

+                let Some(mut command) = super::registry::command_from_argv(&argv) else {
+                    tracing::warn!("hook command argv is empty, skipping");
+                    return HookOutcome::Proceed;
+                };
+
+                command
+                    .stdin(Stdio::piped())
+                    .stdout(Stdio::piped())
+                    .stderr(Stdio::piped());
+
+                let mut child = match command.spawn() {


Run hook commands in session cwd to avoid silent skips

Hook commands are spawned without setting current_dir, so they inherit the process working directory instead of the session’s payload.cwd. If a user configures a relative hook command (e.g. ./hooks/pre_tool.sh) or changes the session cwd, the spawn will fail and the hook will “fail open” (Proceed), bypassing intended pre‑tool policy checks. Consider setting command.current_dir(&payload.cwd) (or resolving relative paths against it) so hooks execute in the same directory the agent operates on.

Useful? React with 👍 / 👎.

Local arbitration result: CONFIRMED.

Bot's concern

Hook commands are spawned without setting current_dir, so they inherit the process working directory. Relative hook paths (e.g. ./hooks/pre_tool.sh) will fail to resolve and the hook will fail-open, bypassing policy.

Independent local review

Confirmed as genuine issue. Key evidence:

Config tests use relative paths (./hook.sh, ./pre-tool.sh at config.rs:130,247) — relative paths are a supported use case

command_from_argv (registry.rs:110,115) does Command::new(program) without path resolution or cwd binding

payload.cwd is available in the executor closure but unused for current_dir

Claude Code reference implementation runs hooks in the project directory

Conclusion

Real P2. Fix: set command.current_dir(&payload.cwd) so hooks resolve relative paths against the session working directory. This matches the semantic intent (hooks operate in the same context as the agent's tools).

Set command.current_dir(&payload.cwd) so hook commands resolve relative paths against the session working directory instead of inheriting the process cwd. This prevents relative hook paths (e.g. ./hooks/pre_tool.sh) from silently failing and falling through to Proceed (fail-open). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

RyderFreeman4Logos · 2026-02-08T05:01:50Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 86ae27feca

ℹ️ 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-08T05:08:06Z

+impl From<HookCommandResult> for HookOutcome {
+    fn from(result: HookCommandResult) -> Self {
+        match result.decision {
+            HookDecision::Proceed => HookOutcome::Proceed,
+            HookDecision::Block => HookOutcome::Block {
+                message: result.message,
+            },
+            HookDecision::Modify => HookOutcome::Modify {
+                content: result.content.unwrap_or_default(),
+            },


Require modify decisions to include content

If a hook command outputs {"decision":"modify"} without a content field (e.g., due to a script bug or version mismatch), the conversion silently substitutes an empty string. For pre_tool_use hooks this replaces the tool input with "", which can trigger unintended behavior (empty/invalid JSON for function tools or empty command semantics) instead of failing closed. This makes a malformed hook response mutate tool calls in a way the hook author didn’t intend. Consider treating missing content as a Block (or at least Proceed) rather than wiping inputs.

Useful? React with 👍 / 👎.

Local arbitration result: CONFIRMED.

Bot's concern

HookOutcome::Modify silently uses empty string when content field is missing from hook output, which can cause unintended tool input mutation (especially for MCP tools where empty params = valid no-arg call).

Independent local review

Confirmed as genuine issue. Key evidence:

MCP tool handler (mcp_tool_call.rs:38-40) treats empty string as valid "no parameters", causing unintended no-arg execution

unwrap_or_default() conflates "protocol error (missing field)" with "valid modification (empty content)"

Claude Code reference: missing updatedInput means "don't modify input", not "replace with empty"

Fix: distinguish None (missing → Block) from Some("") (explicit empty → allow)

Conclusion

Real P2. Fix: treat missing content in Modify decision as Block (malformed hook response) while preserving Some("") for intentional empty content.

Treat a hook returning {"decision":"modify"} without a "content" field as Block instead of silently substituting an empty string. This prevents malformed hook responses from wiping tool inputs, which is especially dangerous for MCP tools where empty params are treated as valid no-arg calls. Explicit empty content (Some("")) is still allowed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

RyderFreeman4Logos · 2026-02-08T05:14:11Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c2da3dda8e

ℹ️ 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-08T05:20:18Z

+                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,


Emit MCP tool-call output when pre_hook blocks MCP calls

This pre_tool_use block path always routes through failure_response with payload_outputs_custom, which only distinguishes Custom vs everything else. For MCP tool calls, a blocked hook will therefore return a function_call_output instead of the required mcp_tool_call_output, so the Responses API won’t associate the output with the MCP call and the turn can stall or error when hooks block MCP tools. Consider passing the payload variant into failure_response (or adding an MCP-specific branch here) so MCP blocks return ResponseInputItem::McpToolCallOutput { result: Err(...) }.

Useful? React with 👍 / 👎.

Dismissed after local arbitration.

Bot's concern

MCP tool calls blocked by pre_hook return FunctionCallOutput instead of McpToolCallOutput.

Independent local review

This is an existing pattern across the entire codebase, not a new bug introduced by hooks:

stream_events_utils.rs:130 (RespondToModel) and :169 (ToolCallBlocked) both use the same Custom-vs-non-Custom distinction with no MCP branch

McpToolCallOutput is an internal intermediate form that gets converted to FunctionCallOutput at models.rs:632 and stream_events_utils.rs:290

Tests explicitly verify MCP responses are sent as function_call_output to the model (truncation.rs:453 comment: "converted to function_call_output", rmcp_client.rs:318 asserts type: function_call_output)

Our hook code follows the same established pattern. If this needed changing, it would be a full-pipeline behavior change across the codebase, not a hooks-specific fix.

RyderFreeman4Logos · 2026-02-08T05:27:21Z

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

RyderFreeman4Logos and others added 5 commits February 7, 2026 20:04

RyderFreeman4Logos mentioned this pull request Feb 8, 2026

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

Closed

3 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 steering #4

Merged

3 tasks

RyderFreeman4Logos closed this Feb 8, 2026

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 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

Bot's concern

Independent local review (GPT Codex, read-only sandbox)

Verdict: FALSE POSITIVE

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

Bot's concern

Independent local review

Conclusion

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

Bot's concern

Independent local review

Conclusion

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

Bot's concern

Independent local review

Uh oh!