feat(hooks): comprehensive hook system with lifecycle events and steering#4
Conversation
Define the core type hierarchy for the hook system: HookEvent variants (PreToolUse, PostToolUse, AfterAgent, SessionStart, Stop, Notification), HookPayload for serialized context, and HookOutcome (Proceed/Block/Modify) for steering decisions. Add HookEntry configuration with command, timeout, and simple pattern matcher supporting exact match, wildcard, and prefix patterns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement the hook execution engine: serialize HookPayload to JSON on stdin, read the command's stdout for a decision (proceed/block/modify), and enforce timeouts. Run commands in the payload's cwd directory so relative hook paths resolve correctly. Treat missing content in modify decisions as block to prevent silent empty-string input substitution. Cap stdout/stderr at 1MB and block on truncated output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Upgrade the Hooks registry to support multi-event synchronous dispatch with configurable hooks from HooksConfigToml. Refactor the legacy user_prompt_submit notification into a standard hook event, and add AfterAgent, SessionStart, and Stop event dispatch points. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wire hook dispatch into the tool execution pipeline: fire PreToolUse before each tool call with structured hook_input(), apply Block/Modify outcomes, and fire PostToolUse after execution completes. Add ToolCallBlocked error variant for surfacing hook blocks into the conversation transcript. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Generate the JSON schema for hook configuration and integrate HooksConfigToml into the main Config struct so hooks are loaded from project and user configuration files. 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: 4658779d8f
ℹ️ 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.
Include all LocalShell params in hook payload
The new hook_input() for ToolPayload::LocalShell only serializes command, workdir, and timeout_ms, but ShellToolCallParams also carries sandbox_permissions, prefix_rule, and justification (see codex-rs/protocol/src/models.rs around ShellToolCallParams). Since ToolRouter uses hook_input() to populate the pre_tool_use hook payload, hooks cannot see or enforce policies around these security‑relevant fields; e.g., a hook that blocks elevated sandbox_permissions will never observe them and will allow the call to proceed. This effectively weakens the new hook system’s ability to enforce safety constraints for LocalShell calls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dismissed — same issue flagged 5 times across PRs #2/#3/#4, already arbitrated.
See #3 discussion for full local arbitration. Summary:
router.rs:119-125hardcodessandbox_permissions: UseDefault,prefix_rule: None,justification: None— these are constants, not model-controlledLocalShellExecActionatprotocol/src/models.rs:664only hascommand/timeout_ms/working_directory/env/user— the model cannot submit these fields- Local Codex (GPT) independently confirmed false positive with protocol-level evidence
There was a problem hiding this comment.
Local arbitration result: DISMISSED (False Positive)
Bot's concern
hook_input() for LocalShell only serializes command, workdir, timeout_ms but omits sandbox_permissions, prefix_rule, justification from ShellToolCallParams, allegedly weakening hook-based security policy enforcement.
Arbiter's independent assessment (GPT-5.3-Codex, local non-interactive)
Verdict: False positive. The arbiter independently verified:
hook_input()atcontext.rs:67-74serializescommand/workdir/timeout_ms— confirmedShellToolCallParamsatprotocol/src/models.rs:755-761has the 3 additional fields — confirmed- Critical finding: In
router.rs:123-125, these fields are hardcoded constants (UseDefault,None,None), not dynamic per-call model inputs - Protocol-level
LocalShellExecAction(models.rs:665-667) does not carry these fields — they are internal to the router - Only one construction site for
ToolPayload::LocalShellexists (inrouter.rs), with no dynamic source - Pre-hook
ModifyforLocalShellis explicitly blocked atrouter.rs:198-202
Conclusion
The 3 fields are compile-time constants set by the router, not model-controlled inputs. A hook cannot "miss" them because they never vary per call. The scenario described (hook failing to block elevated sandbox_permissions) is impossible under current architecture.
Note: This is the same issue flagged 6 times across PR #3 and PR #4. All instances arbitrated with the same conclusion.
Arbitration method: Claude Opus 4.6 (code author) + GPT-5.3-Codex (independent arbiter, local ai-mcp:codex session 019c3b8c-2ab1-7fe1-b99a-52cb1ca8bea9). Model heterogeneity ensures cognitive friction.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4658779d8f
ℹ️ 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.
Include all LocalShell params in hook_input
The pre_tool_use hook payload is built from ToolPayload::hook_input(), but the LocalShell branch only serializes command, workdir, and timeout_ms. This drops other security-relevant fields present in ShellToolCallParams (e.g., sandbox_permissions, justification, prefix_rule). A policy hook that needs to block elevated permissions or require a justification will never see those fields for LocalShell calls, so those requests can bypass the hook’s checks even though the actual execution uses them. Consider serializing the full ShellToolCallParams (or at least including the missing fields) so hooks can enforce policy consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dismissed — 6th occurrence of same false positive (see #3 for full arbitration).
This is the same issue about sandbox_permissions/prefix_rule/justification in hook_input(), flagged repeatedly across PRs #2, #3, and #4. Already confirmed as false positive:
- These fields are hardcoded constants in
router.rs:119-125, not model-controlled LocalShellExecActionat protocol level doesn't include these fields- Local Codex (GPT) independently confirmed false positive
There was a problem hiding this comment.
Dismissed — same issue as comment 2778599157. See full arbitration result above.
TL;DR: sandbox_permissions/prefix_rule/justification are hardcoded constants at router.rs:123-125, not model-controlled inputs. Independently confirmed by GPT-5.3-Codex local arbitration.
Summary
Background
Clean resubmission of #3. The original PR went through multiple rounds of iterative review with @codex. Two real issues were identified and fixed:
payload.cwdto resolve relative paths correctlySee #3 for the full review discussion.
Test plan
cargo clippy -p codex-core -- -D warningscargo test -p codex-core -- hooks(52/52 pass)🤖 Generated with Claude Code