chore(devkit): migrate git hooks to husky#41
Merged
Conversation
📝 WalkthroughWalkthroughThe PR migrates git hook infrastructure from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/format-staged-json.sh (1)
23-23: Filenames with spaces or special characters may break.The newline-delimited approach works for typical filenames, but paths with embedded newlines or special characters could cause issues with
xargs. Consider using null-delimited input for robustness.♻️ Optional fix for safer filename handling
-printf '%s\n' "$filtered_files" | xargs bun run format:staged -- +printf '%s\0' $filtered_files | xargs -0 bun run format:staged --However, this would require refactoring the loop to build a null-separated list. Given that JSON filenames in this repo likely don't contain special characters, the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/format-staged-json.sh` at line 23, The current call that pipes printf '%s\n' "$filtered_files" into xargs bun run format:staged -- can break on filenames with spaces/special chars; change the producer and consumer to use NUL-delimited paths: emit NUL-separated entries for the filtered_files variable (e.g., via printf '%s\0' for each entry) and invoke xargs with the -0/--null flag when calling bun run format:staged -- so the command that references filtered_files and xargs handles arbitrary filenames safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/pre-push.sh`:
- Around line 12-17: The pre-push hook currently auto-creates a commit when
AGENTS.md or .claude-plugin/marketplace.json are changed (see the git status/git
commit logic in the pre-push.sh snippet), which can produce unexpected local
commits; change the hook to stop auto-committing and instead either (A) fail the
push when those files are out of sync—return non-zero with a clear message
instructing the developer to run the manual sync command (e.g., run the docs
sync task such as `bun run docs` and commit), or (B) remove the git add/commit
steps and move the sync-and-commit behavior into a separate script/command that
developers run manually (e.g., a npm/bun script like `check:plugin-list` or
`sync:plugin-list`); update the pre-push.sh to only check and fail (or print
instructions) referencing the same file checks currently used (AGENTS.md and
.claude-plugin/marketplace.json).
---
Nitpick comments:
In `@scripts/format-staged-json.sh`:
- Line 23: The current call that pipes printf '%s\n' "$filtered_files" into
xargs bun run format:staged -- can break on filenames with spaces/special chars;
change the producer and consumer to use NUL-delimited paths: emit NUL-separated
entries for the filtered_files variable (e.g., via printf '%s\0' for each entry)
and invoke xargs with the -0/--null flag when calling bun run format:staged --
so the command that references filtered_files and xargs handles arbitrary
filenames safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10431618-96c1-4ad7-967b-ecbed2f6b2e8
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.claude-plugin/marketplace.json.github/renovate.json5.gitignore.husky/pre-commit.husky/pre-push.prettierignoreAGENTS.mdlefthook.ymllink-commands.shmise.tomlpackage.jsonplugins/devkit/plugin.jsonplugins/devkit/skills/standards-audit/scripts/audit.test.tsplugins/devkit/skills/standards-audit/scripts/checks/config-files/index.test.tsplugins/devkit/skills/standards-audit/scripts/checks/config-files/index.tsplugins/devkit/skills/standards-audit/scripts/core/format.test.tsrenovate.jsonscripts/format-staged-json.shscripts/pre-push.shscripts/security-check.sh
💤 Files with no reviewable changes (4)
- mise.toml
- lefthook.yml
- link-commands.sh
- renovate.json
- Replace lefthook.yml with .husky/ pre-commit and pre-push hooks - Add block-husky-bypass hook to mutils - Move renovate.json to .github/renovate.json5 - Format marketplace.json with oxfmt (remove .prettierignore) - Update lint-staged config in package.json Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Prevents oxfmt from checking Claude Code temp files during pre-push. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
plugins/discord-notify/hooks/lib/db.test.ts (2)
86-89: This test now validates mock behavior, not production behavior.The assertion
(db as unknown as { path: string }).pathaccesses theFakeDatabase.pathproperty, which is an implementation detail of your mock. The realbetter-sqlite3Database exposes the file path via thenameproperty, notpath.Since the entire test file uses mocks, this test verifies that
openDbpasses the path to the Database constructor but doesn't validate actual SQLite file creation. Consider whether this level of confidence is acceptable, or if you need an integration test with the real library elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/discord-notify/hooks/lib/db.test.ts` around lines 86 - 89, The test is asserting the mock's implementation detail by checking FakeDatabase.path instead of the real better-sqlite3 API; update the test for openDb to assert the Database was constructed with the expected filename (e.g., by spying on the mock constructor or its call args) or, if you want to validate runtime behavior, replace this unit test with an integration test that opens a real better-sqlite3 Database and asserts the real property (Database.name) points to dbPath; target symbols: the test's db variable, the openDb function, and the FakeDatabase/mock constructor (or better-sqlite3 Database.name) to locate where to change the assertion.
100-107: Idempotency test no longer validates real SQLite behavior.The mock's
SELECT name FROM sqlite_masteralways returns[{ name: "sessions" }]regardless of how many timesensureTableis called. This test now only verifies that callingensureTablemultiple times doesn't throw, not that SQLite actually handles theCREATE TABLE IF NOT EXISTScorrectly.If true idempotency coverage is needed, consider maintaining a separate integration test with the real
better-sqlite3library.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/discord-notify/hooks/lib/db.test.ts` around lines 100 - 107, The test "is idempotent" currently uses a mock `db` whose `SELECT name FROM sqlite_master` always returns `[{ name: "sessions" }]`, so update the test to exercise real SQLite behavior: create an in-memory `better-sqlite3` Database instance (instead of the mock `db`), call `ensureTable(db)` twice, then run `db.prepare("SELECT name FROM sqlite_master WHERE type='table' AND name='sessions'").all()` and assert the result has length 1; alternatively, if you want to keep the unit test mock-based, change the test name and assertion to only verify that `ensureTable(db)` does not throw on repeated calls rather than validating sqlite idempotency. Ensure you reference the `ensureTable` helper and the `sessions` table in the test change..husky/pre-commit (1)
8-13: Avoid storing full staged diff in a shell variable.For larger commits, buffering the entire diff in
staged_diffis less reliable and duplicates memory usage. Write directly to the temp file and check file size.♻️ Proposed refactor
-staged_diff=$(git diff --cached) - -if [ -z "$staged_diff" ]; then - printf '%s\n' "No staged changes. Skipping security check." - exit 0 -fi - temp_file=$(mktemp) prompt_file=$(mktemp) + +git diff --cached > "$temp_file" +if [ ! -s "$temp_file" ]; then + printf '%s\n' "No staged changes. Skipping security check." + exit 0 +fi @@ -printf '%s' "$staged_diff" > "$temp_file"Also applies to: 15-16, 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.husky/pre-commit around lines 8 - 13, The script currently stores the entire staged diff in the variable staged_diff which can be large; instead, write the output of git diff --cached directly to a temp file (e.g., using mktemp) and then test the file size to decide whether to skip the security check; update the sections referencing staged_diff (the staged_diff assignment and the subsequent if [ -z "$staged_diff" ] check, plus the similar usages at lines indicated) to use the temp file path and a size check (e.g., [ ! -s "$tmpfile" ] or stat) and ensure the temp file is removed on exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.husky/pre-commit:
- Around line 76-79: The current if-statement uses grep -qw "OK" on $result
which matches any token containing "OK" (e.g., "NOT OK"); update the check
around the variable $result (the if conditional that currently calls grep -qw
"OK") to require an exact success token instead—either use a full-line match
(grep -xq "OK") or a strict string equality test (e.g., [ "$result" = "OK" ]) so
only a lone "OK" passes the gate, and leave the surrounding printf/exit behavior
unchanged.
In `@plugins/mutils/hooks/entry/block-husky-bypass.ts`:
- Around line 7-23: HUSKY_BYPASS_PATTERN only matches unquoted HUSKY=0 so quoted
variants like HUSKY='0' or HUSKY="0" slip through; update the
HUSKY_BYPASS_PATTERN constant in block-husky-bypass.ts to also match quoted "0"
and '0' (allowing optional spaces around '=') and ensure the pattern enforces
command boundaries (so it matches at end of token or before separators like
whitespace, semicolon or end-of-line) so the check on
context.input.tool_input.command correctly detects all Husky-disable forms.
---
Nitpick comments:
In @.husky/pre-commit:
- Around line 8-13: The script currently stores the entire staged diff in the
variable staged_diff which can be large; instead, write the output of git diff
--cached directly to a temp file (e.g., using mktemp) and then test the file
size to decide whether to skip the security check; update the sections
referencing staged_diff (the staged_diff assignment and the subsequent if [ -z
"$staged_diff" ] check, plus the similar usages at lines indicated) to use the
temp file path and a size check (e.g., [ ! -s "$tmpfile" ] or stat) and ensure
the temp file is removed on exit.
In `@plugins/discord-notify/hooks/lib/db.test.ts`:
- Around line 86-89: The test is asserting the mock's implementation detail by
checking FakeDatabase.path instead of the real better-sqlite3 API; update the
test for openDb to assert the Database was constructed with the expected
filename (e.g., by spying on the mock constructor or its call args) or, if you
want to validate runtime behavior, replace this unit test with an integration
test that opens a real better-sqlite3 Database and asserts the real property
(Database.name) points to dbPath; target symbols: the test's db variable, the
openDb function, and the FakeDatabase/mock constructor (or better-sqlite3
Database.name) to locate where to change the assertion.
- Around line 100-107: The test "is idempotent" currently uses a mock `db` whose
`SELECT name FROM sqlite_master` always returns `[{ name: "sessions" }]`, so
update the test to exercise real SQLite behavior: create an in-memory
`better-sqlite3` Database instance (instead of the mock `db`), call
`ensureTable(db)` twice, then run `db.prepare("SELECT name FROM sqlite_master
WHERE type='table' AND name='sessions'").all()` and assert the result has length
1; alternatively, if you want to keep the unit test mock-based, change the test
name and assertion to only verify that `ensureTable(db)` does not throw on
repeated calls rather than validating sqlite idempotency. Ensure you reference
the `ensureTable` helper and the `sessions` table in the test change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53ab1118-dea4-404e-ad22-1ba22428aa09
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.claude-plugin/marketplace.json.github/renovate.json5.gitignore.husky/pre-commit.husky/pre-push.prettierignoreAGENTS.mdlefthook.ymllink-commands.shmise.tomlpackage.jsonplugins/devkit/plugin.jsonplugins/devkit/skills/standards-audit/scripts/audit.test.tsplugins/devkit/skills/standards-audit/scripts/checks/config-files/index.test.tsplugins/devkit/skills/standards-audit/scripts/checks/config-files/index.tsplugins/devkit/skills/standards-audit/scripts/core/format.test.tsplugins/discord-notify/hooks/lib/db.test.tsplugins/discord-notify/plugin.jsonplugins/github-workflow/hooks/check-push-pr-conflicts.test.tsplugins/github-workflow/hooks/lib/pr-conflicts.tsplugins/github-workflow/plugin.jsonplugins/mutils/AGENTS.mdplugins/mutils/hooks/entry/block-husky-bypass.tsplugins/mutils/hooks/hooks.jsonplugins/mutils/plugin.jsonrenovate.json
💤 Files with no reviewable changes (4)
- mise.toml
- renovate.json
- lefthook.yml
- link-commands.sh
✅ Files skipped from review due to trivial changes (14)
- .gitignore
- plugins/mutils/plugin.json
- .prettierignore
- plugins/github-workflow/plugin.json
- plugins/discord-notify/plugin.json
- AGENTS.md
- plugins/devkit/plugin.json
- plugins/github-workflow/hooks/check-push-pr-conflicts.test.ts
- .github/renovate.json5
- plugins/mutils/AGENTS.md
- plugins/devkit/skills/standards-audit/scripts/core/format.test.ts
- package.json
- plugins/github-workflow/hooks/lib/pr-conflicts.ts
- .claude-plugin/marketplace.json
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/devkit/skills/standards-audit/scripts/audit.test.ts
- plugins/devkit/skills/standards-audit/scripts/checks/config-files/index.ts
- plugins/devkit/skills/standards-audit/scripts/checks/config-files/index.test.ts
- .husky/pre-push
Comment on lines
+76
to
+79
| if printf '%s\n' "$result" | grep -qw "OK"; then | ||
| printf '%s\n' "Security check passed." | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
OK detection is too permissive and can bypass the gate.
Current matching accepts any output containing the word OK (e.g., NOT OK). This can incorrectly allow unsafe commits.
🔧 Proposed fix (strict success match)
-if printf '%s\n' "$result" | grep -qw "OK"; then
+normalized_result=$(printf '%s' "$result" | tr -d '\r' | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')
+if [ "$normalized_result" = "OK" ]; then
printf '%s\n' "Security check passed."
exit 0
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if printf '%s\n' "$result" | grep -qw "OK"; then | |
| printf '%s\n' "Security check passed." | |
| exit 0 | |
| fi | |
| normalized_result=$(printf '%s' "$result" | tr -d '\r' | sed 's/^[[:space:]]*//;s/[[:space:]]*$//') | |
| if [ "$normalized_result" = "OK" ]; then | |
| printf '%s\n' "Security check passed." | |
| exit 0 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.husky/pre-commit around lines 76 - 79, The current if-statement uses grep
-qw "OK" on $result which matches any token containing "OK" (e.g., "NOT OK");
update the check around the variable $result (the if conditional that currently
calls grep -qw "OK") to require an exact success token instead—either use a
full-line match (grep -xq "OK") or a strict string equality test (e.g., [
"$result" = "OK" ]) so only a lone "OK" passes the gate, and leave the
surrounding printf/exit behavior unchanged.
Comment on lines
+7
to
+23
| const HUSKY_BYPASS_PATTERN = /\bHUSKY\s*=\s*0\b/; | ||
|
|
||
| const hook = defineHook({ | ||
| trigger: { | ||
| PreToolUse: { | ||
| Bash: true, | ||
| }, | ||
| }, | ||
| run: wrapRun(logger, (context) => { | ||
| const input = context.input; | ||
| if (!input) { | ||
| return context.success({}); | ||
| } | ||
|
|
||
| const command = input.tool_input.command; | ||
| if (!command || !HUSKY_BYPASS_PATTERN.test(command)) { | ||
| return context.success({}); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import re
pat = re.compile(r'\bHUSKY\s*=\s*0\b')
cases = [
"HUSKY=0 git commit",
"HUSKY = 0 git push",
"HUSKY='0' git commit",
'HUSKY="0" git push',
]
for c in cases:
print(f"{pat.search(c) is not None}\t{c}")
PYRepository: masseater/claude-code-plugin
Length of output: 171
🏁 Script executed:
cat -n plugins/mutils/hooks/entry/block-husky-bypass.ts | head -30Repository: masseater/claude-code-plugin
Length of output: 1112
🏁 Script executed:
# Test if quoted HUSKY values actually work in shell to disable Husky
# Simulate what a shell would do with quoted environment variables
python - <<'PY'
# Verify that quoted HUSKY=0 is indeed a valid shell syntax
test_commands = [
('HUSKY=0 git commit', 'unquoted'),
('HUSKY = 0 git commit', 'unquoted with spaces'),
("HUSKY='0' git commit", 'single quoted'),
('HUSKY="0" git commit', 'double quoted'),
]
# In shell, all of these set HUSKY to 0
# The quotes matter only for the shell parser, not the actual value assignment
for cmd, desc in test_commands:
print(f"{desc:30} → HUSKY is set to '0' in the environment")
print("\nAll variants are functionally equivalent in shell execution.")
PYRepository: masseater/claude-code-plugin
Length of output: 421
🏁 Script executed:
# Test the proposed regex fix to ensure it handles all cases and doesn't have false positives
python - <<'PY'
import re
# Current regex
current_pat = re.compile(r'\bHUSKY\s*=\s*0\b')
# Proposed regex from the review
proposed_pat = re.compile(r'\bHUSKY\s*=\s*(?:0|"0"|\'0\')(?=\s|$|[;&|])')
test_cases = [
# Valid bypass attempts (should match)
("HUSKY=0 git commit", True),
("HUSKY = 0 git push", True),
("HUSKY='0' git commit", True),
('HUSKY="0" git push', True),
("HUSKY=0;git commit", True),
("HUSKY=0|git commit", True),
("HUSKY=0&git commit", True),
# Edge cases that should NOT match (legitimate uses)
("HUSKY=1 git commit", False),
("HUSKY_VAR=0 git commit", False),
("MY_HUSKY=0 git commit", False),
("echo HUSKY=0", False), # literal string in echo
("HUSKY=00 git commit", False), # Different value
]
print("Current Pattern Results:")
print("-" * 60)
for cmd, should_match in test_cases:
matches = bool(current_pat.search(cmd))
status = "✓" if matches == should_match else "✗"
print(f"{status} {matches:5} {cmd}")
print("\nProposed Pattern Results:")
print("-" * 60)
for cmd, should_match in test_cases:
matches = bool(proposed_pat.search(cmd))
status = "✓" if matches == should_match else "✗"
print(f"{status} {matches:5} {cmd}")
PYRepository: masseater/claude-code-plugin
Length of output: 901
🏁 Script executed:
# Check if there are any other file write operations or concurrent safety concerns in this hook file
tail -n +30 plugins/mutils/hooks/entry/block-husky-bypass.tsRepository: masseater/claude-code-plugin
Length of output: 407
Quoted HUSKY=0 values bypass this check.
At Line 7, the regex /\bHUSKY\s*=\s*0\b/ catches HUSKY=0 but misses HUSKY='0' and HUSKY="0", which are valid shell syntax and still disable Husky. This is a security bypass.
Update the regex to match quoted variants with proper lookahead for command boundaries:
-const HUSKY_BYPASS_PATTERN = /\bHUSKY\s*=\s*0\b/;
+const HUSKY_BYPASS_PATTERN = /\bHUSKY\s*=\s*(?:0|["']0["'])(?:\s|$|[;&|])/;This handles HUSKY=0, HUSKY='0', HUSKY="0", and variants with spaces around =.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const HUSKY_BYPASS_PATTERN = /\bHUSKY\s*=\s*0\b/; | |
| const hook = defineHook({ | |
| trigger: { | |
| PreToolUse: { | |
| Bash: true, | |
| }, | |
| }, | |
| run: wrapRun(logger, (context) => { | |
| const input = context.input; | |
| if (!input) { | |
| return context.success({}); | |
| } | |
| const command = input.tool_input.command; | |
| if (!command || !HUSKY_BYPASS_PATTERN.test(command)) { | |
| return context.success({}); | |
| const HUSKY_BYPASS_PATTERN = /\bHUSKY\s*=\s*(?:0|["']0["'])(?:\s|$|[;&|])/; | |
| const hook = defineHook({ | |
| trigger: { | |
| PreToolUse: { | |
| Bash: true, | |
| }, | |
| }, | |
| run: wrapRun(logger, (context) => { | |
| const input = context.input; | |
| if (!input) { | |
| return context.success({}); | |
| } | |
| const command = input.tool_input.command; | |
| if (!command || !HUSKY_BYPASS_PATTERN.test(command)) { | |
| return context.success({}); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/mutils/hooks/entry/block-husky-bypass.ts` around lines 7 - 23,
HUSKY_BYPASS_PATTERN only matches unquoted HUSKY=0 so quoted variants like
HUSKY='0' or HUSKY="0" slip through; update the HUSKY_BYPASS_PATTERN constant in
block-husky-bypass.ts to also match quoted "0" and '0' (allowing optional spaces
around '=') and ensure the pattern enforces command boundaries (so it matches at
end of token or before separators like whitespace, semicolon or end-of-line) so
the check on context.input.tool_input.command correctly detects all
Husky-disable forms.
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
1 participant
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.
Summary
Validation
Summary by CodeRabbit
New Features
Chores