feat(completion-integrity): add git pre-commit hook for bypass mode #53
Conversation
Add install-git-hook.sh that installs a native git pre-commit hook, which works even when Claude runs with --dangerously-skip-permissions. Changes: - Add scripts/install-git-hook.sh (standalone git hook installer) - Update README.md with bypass mode compatibility matrix - Remove unused SCRIPT_DIR and INTEGRITY_SCRIPT variables - Clarify that git hook is recommended, Claude hooks are secondary 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughUpdates documentation for pre-commit and phase-end checks, replacing entries with Git pre-commit hook and Claude plugin hooks. Adds entry for ancplua-docs-librarian plugin. Introduces new shell script to install and enforce git pre-commit hook with integrity rule validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
Summary of ChangesHello @ANcpLua, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a native git pre-commit hook for the completion-integrity plugin, which is a valuable addition as it functions even when Claude's permission system is bypassed. The implementation is solid, with updates to the README and CHANGELOG to reflect the new functionality. I've identified a significant logical issue in the new hook script where it incorrectly excludes test files from analysis, thereby disabling some of its own checks. Additionally, I've provided a few minor suggestions to enhance code clarity and the robustness of the installer script. Overall, great work on improving the integrity checks.
| STAGED_DIFF=$(git diff --cached --unified=0 -- \ | ||
| ':(exclude)*.md' \ | ||
| ':(exclude)**/hooks/scripts/*.sh' \ | ||
| ':(exclude)**/scripts/*.sh' \ | ||
| ':(exclude)**/*.test.*' \ | ||
| ':(exclude)**/*.spec.*' \ | ||
| ':(exclude)**/test-fixtures/**' \ | ||
| 2>/dev/null || echo "") |
There was a problem hiding this comment.
The git diff command is configured to exclude test files (e.g., **/*.test.*, **/*.spec.*) from the diff analysis. However, RULE 2 of this script is specifically designed to detect commented-out tests, which are typically found only in these files. By excluding them, the check for commented-out tests is rendered ineffective. To ensure the hook functions as intended, these exclusion patterns for test files should be removed.
| STAGED_DIFF=$(git diff --cached --unified=0 -- \ | |
| ':(exclude)*.md' \ | |
| ':(exclude)**/hooks/scripts/*.sh' \ | |
| ':(exclude)**/scripts/*.sh' \ | |
| ':(exclude)**/*.test.*' \ | |
| ':(exclude)**/*.spec.*' \ | |
| ':(exclude)**/test-fixtures/**' \ | |
| 2>/dev/null || echo "") | |
| STAGED_DIFF=$(git diff --cached --unified=0 -- \ | |
| ':(exclude)*.md' \ | |
| ':(exclude)**/hooks/scripts/*.sh' \ | |
| ':(exclude)**/scripts/*.sh' \ | |
| ':(exclude)**/test-fixtures/**' \ | |
| 2>/dev/null || echo "") |
| echo "Backing up to ${PRE_COMMIT_HOOK}.backup" | ||
| cp "${PRE_COMMIT_HOOK}" "${PRE_COMMIT_HOOK}.backup" |
There was a problem hiding this comment.
The current backup mechanism for an existing pre-commit hook overwrites pre-commit.backup on each run. This means if you run the installer multiple times, you will lose older backups. To prevent data loss, consider using a timestamp in the backup filename to make it unique.
| echo "Backing up to ${PRE_COMMIT_HOOK}.backup" | |
| cp "${PRE_COMMIT_HOOK}" "${PRE_COMMIT_HOOK}.backup" | |
| BACKUP_FILE="${PRE_COMMIT_HOOK}.backup.$(date +%s)" | |
| echo "Backing up to ${BACKUP_FILE}" | |
| cp "${PRE_COMMIT_HOOK}" "${BACKUP_FILE}" |
| NC='\033[0m' | ||
| VIOLATIONS=() | ||
| WARNINGS=() |
There was a problem hiding this comment.
The WARNINGS array is initialized but never used within the script. It seems to be a remnant from another script. Removing it will make the code cleaner and easier to understand.
| | Feature | Normal Mode | `--dangerously-skip-permissions` | | ||
| |---------|-------------|----------------------------------| | ||
| | Git pre-commit hook | ✅ Works | ✅ Works | | ||
| | PreToolUse hook | ✅ Works | ❌ Bypassed | | ||
| | Stop hook | ✅ Works | ✅ Works (informational) | |
There was a problem hiding this comment.
The new compatibility table's columns are not aligned in the raw markdown source. To improve readability and adhere to the repository's style guide, please add padding to the table cells.
| | Feature | Normal Mode | `--dangerously-skip-permissions` | | |
| |---------|-------------|----------------------------------| | |
| | Git pre-commit hook | ✅ Works | ✅ Works | | |
| | PreToolUse hook | ✅ Works | ❌ Bypassed | | |
| | Stop hook | ✅ Works | ✅ Works (informational) | | |
| | Feature | Normal Mode | `--dangerously-skip-permissions` | | |
| |---------------------|-------------|----------------------------------| | |
| | Git pre-commit hook | ✅ Works | ✅ Works | | |
| | PreToolUse hook | ✅ Works | ❌ Bypassed | | |
| | Stop hook | ✅ Works | ✅ Works (informational) | |
References
- Repository style guide line 79 requires that markdown tables must be padded. (link)
There was a problem hiding this comment.
Pull request overview
This PR adds a native git pre-commit hook installer for the completion-integrity plugin to ensure integrity checks work in bypass mode (--dangerously-skip-permissions). The existing Claude plugin hooks are bypassed in this mode, so a native git hook provides enforcement regardless of Claude's permission settings.
Key Changes:
- Adds
install-git-hook.shscript that installs a native git pre-commit hook with integrity checks - Updates README with compatibility matrix explaining which features work in bypass mode vs normal mode
- Updates CHANGELOG to document the new git hook mechanism
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
plugins/completion-integrity/scripts/install-git-hook.sh |
New installer script that creates a git pre-commit hook embedding integrity checks for warning suppressions, commented tests, deleted assertions, and deleted test files |
plugins/completion-integrity/README.md |
Documents the two enforcement mechanisms (git hook vs Claude hooks) and adds compatibility matrix showing git hook works in all modes |
CHANGELOG.md |
Updates completion-integrity plugin entry to describe the new git hook option and bypass mode compatibility |
| echo "${CS_SUPPRESS}" | head -3 | while read -r line; do echo " + ${line}"; done | ||
| fi | ||
| JS_SUPPRESS=$(echo "${ADDED_LINES}" | grep -iE 'eslint-disable|@ts-ignore|@ts-nocheck' || true) |
There was a problem hiding this comment.
The JS/TS suppression pattern is incomplete compared to integrity-check.sh. Missing patterns: '@ts-expect-error' and 'tslint:disable'. The complete pattern should be: 'eslint-disable|@ts-ignore|@ts-nocheck|@ts-expect-error|tslint:disable' (see integrity-check.sh line 62)
| JS_SUPPRESS=$(echo "${ADDED_LINES}" | grep -iE 'eslint-disable|@ts-ignore|@ts-nocheck' || true) | |
| JS_SUPPRESS=$(echo "${ADDED_LINES}" | grep -iE 'eslint-disable|@ts-ignore|@ts-nocheck|@ts-expect-error|tslint:disable' || true) |
| echo "${JS_SUPPRESS}" | head -3 | while read -r line; do echo " + ${line}"; done | ||
| fi | ||
| PY_SUPPRESS=$(echo "${ADDED_LINES}" | grep -iE '#\s*noqa|#\s*type:\s*ignore' || true) |
There was a problem hiding this comment.
The Python suppression pattern is missing 'pylint:disable'. The complete pattern should be: '#\snoqa|#\stype:\signore|#\spylint:\s*disable' (see integrity-check.sh line 72)
| PY_SUPPRESS=$(echo "${ADDED_LINES}" | grep -iE '#\s*noqa|#\s*type:\s*ignore' || true) | |
| PY_SUPPRESS=$(echo "${ADDED_LINES}" | grep -iE '#\s*noqa|#\s*type:\s*ignore|#\s*pylint:\s*disable' || true) |
| fi | ||
| # RULE 2: Commented tests | ||
| COMMENTED_TESTS=$(echo "${ADDED_LINES}" | grep -iE '//\s*\[(Test|Fact|Theory)\]|//\s*(it|test|describe)\s*\(' || true) |
There was a problem hiding this comment.
The commented test pattern is missing 'TestMethod' which is used in some C# test frameworks. The complete pattern should be: '//\s*[(Test|Fact|Theory|TestMethod)]' (see integrity-check.sh line 86)
| COMMENTED_TESTS=$(echo "${ADDED_LINES}" | grep -iE '//\s*\[(Test|Fact|Theory)\]|//\s*(it|test|describe)\s*\(' || true) | |
| COMMENTED_TESTS=$(echo "${ADDED_LINES}" | grep -iE '//\s*\[(Test|Fact|Theory|TestMethod)\]|//\s*(it|test|describe)\s*\(' || true) |
| fi | ||
| # RULE 3: Deleted assertions (>2) | ||
| DELETED_ASSERTS=$(echo "${DELETED_LINES}" | grep -iE 'Assert\.|Should\.|Expect\(' || true) |
There was a problem hiding this comment.
The assertion detection only uses C#-style patterns (Assert.|Should.|Expect(). This misses JavaScript/TypeScript assertions which use different patterns like 'expect(|assert.|should.' (lowercase). The integrity-check.sh script has separate detection for C# (line 139) and JS (line 151) assertions. Consider either adding JS-specific assertion detection or updating this pattern to catch both.
| DELETED_ASSERTS=$(echo "${DELETED_LINES}" | grep -iE 'Assert\.|Should\.|Expect\(' || true) | |
| DELETED_ASSERTS=$(echo "${DELETED_LINES}" | grep -iE 'Assert\.|Should\.|Expect\(|assert\.|should\.|expect\(' || true) |
| # RULE 4: Test files deleted | ||
| DELETED_FILES=$(git diff --cached --name-only --diff-filter=D 2>/dev/null || true) | ||
| DELETED_TESTS=$(echo "${DELETED_FILES}" | grep -iE '\.test\.|\.spec\.|_test\.|Tests\.cs' || true) |
There was a problem hiding this comment.
The test file deletion pattern is incomplete compared to integrity-check.sh. Missing pattern: 'Test.cs' (single test files). The complete pattern should be: '.test.|.spec.|_test.|Tests.cs|Test.cs' (see integrity-check.sh line 167)
| DELETED_TESTS=$(echo "${DELETED_FILES}" | grep -iE '\.test\.|\.spec\.|_test\.|Tests\.cs' || true) | |
| DELETED_TESTS=$(echo "${DELETED_FILES}" | grep -iE '\.test\.|\.spec\.|_test\.|Tests\.cs|Test\.cs' || true) |
|
|
||
| HOOKS_DIR="${GIT_ROOT}/.git/hooks" | ||
| PRE_COMMIT_HOOK="${HOOKS_DIR}/pre-commit" | ||
|
|
There was a problem hiding this comment.
The script assumes the .git/hooks directory exists, but in some edge cases (e.g., a freshly initialized git repo), this directory might not exist yet. Consider adding a check to create it if needed: 'mkdir -p "${HOOKS_DIR}"' before line 26.
| mkdir -p "${HOOKS_DIR}" |
| DELETED_LINES=$(echo "${STAGED_DIFF}" | grep -E '^-[^-]' | sed 's/^-//' || true) | ||
| # RULE 1: Warning suppressions | ||
| CS_SUPPRESS=$(echo "${ADDED_LINES}" | grep -iE '#pragma\s+warning\s+disable|SuppressMessage' || true) |
There was a problem hiding this comment.
The C# suppression pattern is incomplete compared to the existing integrity-check.sh script. The pattern should include 'DisableWarning' to match line 52 of integrity-check.sh which uses: '#pragma\s+warning\s+disable|SuppressMessage|DisableWarning'
|
Superseded by PR #54 which includes install-git-hook.sh and removes Claude hooks |
Summary
install-git-hook.shthat installs a native git pre-commit hook--dangerously-skip-permissionsmode (Claude hooks don't)Changes
Bypass Mode Compatibility
--dangerously-skip-permissionsTest plan
bash scripts/install-git-hook.shin a git repo#pragma warning disableand try to commit🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.