fix: generate valid Claude Code hook schema (closes #138)#141
Open
sebastianbreguel wants to merge 1 commit intotirth8205:mainfrom
Open
fix: generate valid Claude Code hook schema (closes #138)#141sebastianbreguel wants to merge 1 commit intotirth8205:mainfrom
sebastianbreguel wants to merge 1 commit intotirth8205:mainfrom
Conversation
Author
|
@tirth8205 hey! could you review this one too? it fixes the hook schema generation (issue #138) |
This was referenced Apr 8, 2026
tirth8205
reviewed
Apr 8, 2026
Owner
tirth8205
left a comment
There was a problem hiding this comment.
This is a critical correctness fix — the hook schema rewrite looks solid. 4 regression tests, clean approach.
Please rebase on latest main (PR #152 just merged and touches the same skills.py file). Once rebased and CI passes, this is good to merge.
This was referenced Apr 8, 2026
The hooks config emitted by `generate_hooks_config()` did not match the
Claude Code schema, so `code-review-graph install` was writing an
invalid ~/.claude/settings.json. Three problems:
1. `PreCommit` is not a valid hook event — removed.
2. Every event entry needs an inner `hooks: [{type: "command", ...}]`
array; the old output put `command`/`timeout` directly on the entry,
which Claude Code rejects with "hooks: Expected array, but received
undefined".
3. Timeouts were in milliseconds (5000, 3000, 10000), but the schema
uses seconds — the PostToolUse hook effectively had an 83-minute
timeout. Converted to 5 and 3 seconds.
Verified against the official schema at
https://docs.claude.com/en/docs/claude-code/hooks.
Tests updated to assert the new nested structure and guard against
regressions (PreCommit not emitted, all handlers wrapped in inner
hooks array with type=command).
573b855 to
27c0cc3
Compare
Author
|
@tirth8205 Rebased on latest main, conflicts resolved, CI is green. Ready to merge when you are! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #138
Problem
generate_hooks_config()emits a hooks config that Claude Code rejects with schema errors, socode-review-graph installcorrupts~/.claude/settings.json. Three bugs in the same function:PreCommitis not a valid Claude Code hook event →Invalid key in record.hooksarray wrapper. Each entry must havehooks: [{type: "command", command, timeout}], but the old output putcommand/timeoutdirectly on the entry →hooks: Expected array, but received undefined(the exact error in the issue forSessionStart).5000,3000,10000— thePostToolUsehook effectively had an 83-minute timeout.Verified against the official schema: https://docs.claude.com/en/docs/claude-code/hooks
Fix
Rewrite
generate_hooks_config()to match the documented schema:PreCommitentry (no equivalent Claude Code event; pre-commit behavior belongs in a git hook).PostToolUseandSessionStarthandlers inhooks: [{type: "command", ...}].5000→5,3000→3).Testing
test_has_post_tool_use/test_has_session_startupdated for the nested structuretest_does_not_emit_precommit— regression guard for this issuetest_all_commands_wrapped_in_inner_hooks_array— structural invariantruff checkclean