Skip to content

feat(hooks): add user-configurable hook system#2757

Open
ssddOnTop wants to merge 30 commits intomainfrom
feat/user-configurable-hooks
Open

feat(hooks): add user-configurable hook system#2757
ssddOnTop wants to merge 30 commits intomainfrom
feat/user-configurable-hooks

Conversation

@ssddOnTop
Copy link
Copy Markdown
Collaborator

No description provided.

#[test]
fn test_no_matcher_group_fires_unconditionally() {
let json = r#"{
"PostToolUse": [
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use fixtures

@github-actions github-actions bot added the type: feature Brand new functionality, features, pages, workflows, endpoints, etc. label Apr 1, 2026
@ssddOnTop ssddOnTop marked this pull request as ready for review April 2, 2026 04:19
@franklintra
Copy link
Copy Markdown

great looking forward for this !

@franklintra
Copy link
Copy Markdown

Went through the full diff. Really nice work, the layering between config/executor/handler/service is clean and the exit code convention (0/1/2) is simple enough that hook authors won't have to think twice. The Hook::zip() composition to layer user hooks on top of internal ones is a great call too.

A few things I noticed:

The .forge path bug - Graphite already flagged this but worth repeating since it's a silent failure. home.join("forge") needs to be home.join(".forge") in the config loader, otherwise user-level hooks just never get picked up and nobody would know why.

hook_timeout_ms is wired into the config but never actually read. You added it to ForgeConfig, .forge.toml, and the info display, but UserHookHandler just hardcodes DEFAULT_HOOK_TIMEOUT = 600s and only looks at per-hook timeout fields. Guessing this was meant to be the global fallback that individual hooks can override? Either way it's dead right now.

The updatedInput path in PreToolUse is a no-op. The handler parses it, matches on AllowWithUpdate, logs a message, and then does nothing. If this is intentionally punted to a follow-up that's fine, but hook authors will see updatedInput in the output schema and assume it works. Might be cleaner to either drop it from HookOutput for now or add a doc comment saying it's reserved.

SessionStart hooks can't block, but the code doesn't make that obvious. The handler runs the hooks and checks for additional_context but never calls process_results(), so exit code 2 is silently ignored. A one-line comment saying "SessionStart hooks provide context but cannot block" would save someone 10 minutes of debugging later.

Regex gets recompiled on every event. find_matching_hooks calls Regex::new(pattern) each time. For tools that fire a lot of PreToolUse/PostToolUse events in a session this adds up. The patterns come from static config so caching them at construction time would be straightforward. Not a blocker for this PR but worth a fast follow-up.

Small thing: the #[serde(untagged)] on HookEventInput could get surprising if you ever need to deserialize the input back (e.g. in tests), since PostToolUse is a superset of PreToolUse fields and serde will try variants in order. Doesn't matter today since it's only serialized outward, just something to keep in mind.

Cleanup: the .gitignore additions (/hooksref* and #/cc) look like personal dev artifacts that shouldn't ship.

Overall this is solid. The main things before merge are the .forge path fix and deciding what to do with the unused timeout config. Everything else can be follow-ups. Looking forward to using this 👍

Comment on lines +360 to +363
let feedback_msg = format!(
"<hook_feedback>\n<event>UserPromptSubmit</event>\n<status>blocked</status>\n<reason>{reason}</reason>\n</hook_feedback>"
);
context
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use element

Comment on lines +3230 to +3236
ChatResponse::HookError { tool_name, reason } => {
writer.finish()?;
self.spinner.stop(None)?;
self.writeln_title(TitleFormat::error(format!(
"PreToolUse:{tool_name} hook error: {reason}"
)))?;
self.spinner.start(None)?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message format is incorrect. It displays PreToolUse:{tool_name} which will produce output like PreToolUse:Bash without a space. Should format as:

self.writeln_title(TitleFormat::error(format!(
    "PreToolUse: {} - Hook error: {}", tool_name, reason
)))?;
Suggested change
ChatResponse::HookError { tool_name, reason } => {
writer.finish()?;
self.spinner.stop(None)?;
self.writeln_title(TitleFormat::error(format!(
"PreToolUse:{tool_name} hook error: {reason}"
)))?;
self.spinner.start(None)?;
ChatResponse::HookError { tool_name, reason } => {
writer.finish()?;
self.spinner.stop(None)?;
self.writeln_title(TitleFormat::error(format!(
"PreToolUse: {} - Hook error: {}", tool_name, reason
)))?;
self.spinner.start(None)?;

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@ssddOnTop ssddOnTop changed the title feat(hooks): add user-configurable hook system with config, executor feat(hooks): add user-configurable hook system Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: feature Brand new functionality, features, pages, workflows, endpoints, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants