feat(git-permission-guard): add centralized git/gh permission hook plugin #12
Conversation
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
…ugin Introduces a PreToolUse hook that replaces scattered permission rules with intelligent command parsing and categorized allow/ask/deny logic: - DENY: Commands that bypass safety (--no-verify, hook manipulation) - ASK: Destructive commands (reset, force push, merge, gh pr merge) - ALLOW: Safe read/write operations (status, log, diff, commit, push) Handles git -C <path> by extracting the actual subcommand for rule matching. Provides detailed warning messages explaining risks and safer alternatives. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @JacobPEvans, 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 introduces a robust 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 new git-permission-guard plugin, which is a great initiative for centralizing and enforcing rules around git and gh command usage. The implementation is well-structured with configuration-driven rules and clear, helpful messages for blocked commands.
My review has identified a critical issue in the command matching logic for "ask" commands, which could lead to both false positives and negatives, and a high-severity issue in a "deny" rule that could be bypassed. I've provided suggestions to make the command parsing more robust and to strengthen the security rule. Addressing these points will significantly improve the reliability and effectiveness of this new plugin.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to ca32c8c in 2 minutes and 21 seconds. Click for details.
- Reviewed
755lines of code in6files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. git-permission-guard/scripts/git-permission-guard.py:65
- Draft comment:
Consider refactoring the repeated regex option-stripping loop (lines 65–95) in extract_git_command for improved maintainability. A loop over a collection of option patterns could simplify future modifications. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 35% vs. threshold = 50% This is a code quality refactoring suggestion. The rules state "Comments that suggest code quality refactors are good! But only if they are actionable and clear." The comment is actionable - it suggests using a loop over a collection of patterns. It's also clear what the benefit would be (improved maintainability, easier to add new options). However, I need to consider: 1) Is this an obvious or unimportant suggestion? The current code works fine and is reasonably clear with only 4 options. 2) The code is brand new (this is a new file), so there's no evidence this needs to be more maintainable yet. 3) The suggestion is somewhat generic - it doesn't provide specific implementation guidance. 4) With only 4 patterns, the benefit is marginal. While the refactoring could make the code slightly more maintainable, the current implementation is clear and handles only 4 options. The benefit is marginal, and without a specific implementation proposal, the author might not find this actionable enough. This could be considered an "obvious" or "unimportant" suggestion given the limited scope. The comment does provide a clear direction (loop over a collection), and if more git options need to be added in the future, this refactoring would be valuable. However, for a new file with only 4 options, this is a minor optimization that the author may have intentionally chosen not to do for clarity. The rules emphasize removing comments unless there's STRONG EVIDENCE they're correct and necessary. This is a borderline case - it's a valid refactoring suggestion that is actionable and clear, but the benefit is marginal for only 4 options in a new file. Given the instruction to err on the side of deletion unless there's strong evidence the comment is necessary, and that this could be considered "unimportant" given the limited scope, I'll give it a moderate-low score.
2. git-permission-guard/scripts/git-permission-guard.py:132
- Draft comment:
The substring check in check_ask_patterns (lines 132–139) may yield false positives. Consider using stricter matching (e.g. regex with word boundaries) to ensure command intent is accurately detected. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid concern about substring matching. For example, ifcmdis "merge", the current logic would match "merge" in "emergency" (via the substring check). However, looking more carefully: the first check issubcommand.startswith(cmd)which would match "merge main" correctly. The second check isf" {cmd}" in f" {subcommand}"which prepends a space, so it's looking for " merge" in " emergency" - this wouldn't match. But it could still have issues: if cmd is "force" and subcommand is "enforce something", then " force" would be found in " enforce something". The comment is pointing out a legitimate potential issue and suggesting a concrete improvement (regex with word boundaries). This is actionable and about code quality/correctness. The matching logic might be intentionally loose to catch variations. Without seeing the actual config files that define whatcmdvalues are used, I can't be certain this is a real problem in practice. The comment is somewhat speculative ("may yield false positives") rather than demonstrating a concrete bug. Also, the code comment on line 137 shows the author was thinking about this matching behavior explicitly. While I can't see the config files, the comment identifies a legitimate code quality issue with the matching logic. Even if it works for current config values, the function should be robust against future config changes. The suggestion to use word boundaries is concrete and actionable. However, the comment uses "may" which makes it speculative, and per the rules, speculative comments should be deleted unless there's strong evidence of an actual issue. This comment is speculative ("may yield false positives") without demonstrating a concrete bug. While the concern about substring matching is theoretically valid, without seeing the actual config files or a specific example of a false positive, this falls into the category of speculative comments that should be removed per the rules.
3. git-permission-guard/scripts/git-permission-guard.py:205
- Draft comment:
Consider consolidating the context-specific warning logic in format_ask_message (lines 205–240) into a mapping or helper function for clarity and easier extension. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a code quality refactoring suggestion. The rules state that "Comments that suggest code quality refactors are good! But only if they are actionable and clear." This comment is actionable (consolidate into a mapping/helper) and clear. However, I need to consider: 1) Is this an important refactor? The current code works and is reasonably readable. 2) The if-elif chain is only about 35 lines and handles 5 specific cases plus a default - this is not excessively long or complex. 3) A mapping would arguably make it easier to extend, but the current approach is straightforward. 4) This is a new file, so there's no existing pattern being violated. The suggestion is somewhat subjective - the current code isn't problematic, just could potentially be structured differently. While the suggestion is actionable and clear, the current implementation is not particularly problematic - it's readable and maintainable as-is. The if-elif chain is a common pattern for this type of conditional logic. Whether a mapping would be "better" is subjective, and the improvement would be marginal. This might fall into the category of minor/unimportant suggestions that don't clearly require a code change. The comment does meet the criteria of being actionable and clear for a code quality refactor. However, given that the current code is functional, readable, and not overly complex, and that this is a new file where the author chose this pattern deliberately, the suggestion may not be important enough to warrant keeping. It's more of a stylistic preference than a clear improvement. This is a borderline case - it's a valid refactoring suggestion that is actionable and clear, but the current code is not problematic and the improvement would be marginal. Given the rule to err on the side of deleting comments unless there's STRONG EVIDENCE they're correct and important, and that this is more of a stylistic preference, I should delete this comment.
Workflow ID: wflow_puz5jUwccRy2V2eg
You can customize 
There was a problem hiding this comment.
Pull request overview
This PR introduces a new git-permission-guard plugin that centralizes git and GitHub CLI permission management through a PreToolUse hook. The plugin intelligently parses commands to apply allow/ask/deny logic with detailed explanatory messages.
Changes:
- Adds Python-based PreToolUse hook that intercepts Bash tool calls for git/gh commands
- Implements three-tier permission system (DENY/ASK/ALLOW) with configurable rules via JSON files
- Provides context-aware warning messages explaining risks and suggesting safer alternatives
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| git-permission-guard/scripts/git-permission-guard.py | Core hook logic with command parsing, permission checking, and message formatting |
| git-permission-guard/hooks/hooks.json | Hook configuration that registers the Python script as a PreToolUse hook for Bash commands |
| git-permission-guard/config/deny.json | Rules for commands that bypass safety mechanisms (always blocked) |
| git-permission-guard/config/ask.json | Rules for potentially dangerous operations requiring explicit confirmation |
| git-permission-guard/README.md | Comprehensive documentation covering usage, configuration, and examples |
| git-permission-guard/.claude-plugin/plugin.json | Plugin metadata and package information |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix hooks.json format: use PreToolUse event key, string matcher
- Use proper JSON output with permissionDecision deny/ask (not exit 2)
- Inline all rules in script (remove config/ directory)
- Add early exit for non-git/gh commands (performance)
- Reduce script from 311 to 139 lines
- Use ${CLAUDE_PLUGIN_ROOT} per repo convention
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 0f7e340 in 2 minutes and 59 seconds. Click for details.
- Reviewed
900lines of code in5files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. git-permission-guard/README.md:23
- Draft comment:
The blocked commands table showsgit commit --no-verify / -nbut the '/' notation may be unclear. Consider clarifying that both '--no-verify' and '-n' are checked via a regexp (see DENY_PATTERNS in the code). - Reason this comment was not posted:
Comment was on unchanged code.
2. git-permission-guard/README.md:55
- Draft comment:
The 'Special Handling' section mentionsgit -C <path>andgit -c <key=value>, which is consistent with the code. Consider adding a note that other git options (e.g. --git-dir) aren’t handled, to clarify the limitations. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% This comment is suggesting adding documentation about limitations. However, the diff shows this is a major simplification of the README - going from verbose to concise. The comment wants to add more detail about what's NOT handled, which seems contrary to the simplification goal. Also, this is a "consider adding" suggestion, which is speculative and not a clear code issue. The rules state not to make comments that are "obvious or unimportant" and to avoid speculative comments. Without seeing the actual code implementation, I can't verify if --git-dir is truly not handled or if this is even a significant limitation worth documenting. The comment might be valid if documenting limitations is important for users who might expect --git-dir to work. The author may have intentionally omitted this detail as part of the simplification, but it could be a useful addition for completeness. While documenting limitations can be useful, this is clearly a documentation simplification effort. The comment uses "consider adding" language which is speculative and not actionable. Without evidence that --git-dir is causing actual problems or confusion, this is just a "nice to have" suggestion that goes against the clear intent of making the README more concise. The rules explicitly say not to make comments that are "obvious or unimportant." This comment should be deleted. It's a speculative suggestion to add more documentation detail when the clear intent of the change is to simplify and condense the README. It's not pointing out a bug or required change, just a "consider adding" suggestion.
3. git-permission-guard/hooks/hooks.json:5
- Draft comment:
The hook configuration uses a simple matcher 'Bash' and the command uses${CLAUDE_PLUGIN_ROOT}. This looks correct; ensure that the environment variable is reliably set in your deployment. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold15%This comment is asking the PR author to ensure that an environment variable is set, which falls under the category of asking the author to double-check or ensure something. This violates the rules provided.
4. git-permission-guard/scripts/git-permission-guard.py:97
- Draft comment:
In the subcommand extraction loop (lines 97–112), only '-C' and '-c' options are stripped. If other options like '--git-dir' are to be supported, consider adding those or update the README to clarify the limitation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is interesting - the comment is pointing out a limitation, but it's actually a regression from the old code. The old code explicitly handled '--git-dir' and '--work-tree' (I can see this in the diff), but the new simplified code removed this functionality. The comment is technically correct that only '-C' and '-c' are handled now. However, the comment frames this as "if other options are to be supported" rather than "this is a regression that removes existing functionality." The comment is about code that was changed (the subcommand extraction logic was rewritten), so it IS about the changes. The question is whether this is actionable - it's suggesting either adding back the support or documenting the limitation. The comment might be valid if the removal of '--git-dir' and '--work-tree' support was intentional simplification. Perhaps the author decided these edge cases weren't important enough. Without knowing the intent, I can't be 100% sure this is a problem. Also, the comment is somewhat speculative ("if other options are to be supported") rather than definitively stating this is a bug. While the tone is speculative, the comment does identify a real regression - functionality that existed before (handling '--git-dir' and '--work-tree') was removed. This is a concrete, actionable issue: either restore the functionality or document why it was removed. The comment is about changed code (the extraction logic was completely rewritten). This comment identifies a regression where the new code removed support for '--git-dir' and '--work-tree' options that the old code handled. However, the comment is phrased speculatively ("if other options are to be supported, consider...") rather than definitively stating this is a problem. Given the rule to only keep comments with STRONG EVIDENCE of correctness, and the speculative framing, I should delete this.
5. git-permission-guard/scripts/git-permission-guard.py:123
- Draft comment:
The ASK pattern matching logic uses a simplestartswith(and substring check) which could lead to false positives if a command is a prefix of another word. Consider tokenizing the command or using regex with word boundaries for improved accuracy. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment raises a theoretical concern about false positives. Looking at the actual patterns: "merge" could match "mergetool", "reset" could match "resetfoo" (though unlikely), "gc" could match "gc-something". However, the substring check{cmd}in{subcommand}adds a space prefix which helps. For example, if subcommand is "mergetool", it won't match "merge" via startswith, and " merge" won't be in " mergetool". But wait - "merge" WOULD match "mergetool" via startswith. So "git mergetool" would trigger the "merge" warning. That's a real false positive. Similarly "gc" would match "gc-auto" or any gc-prefixed command. This seems like a legitimate issue, not just theoretical. However, I need to consider: are there actually git commands that start with these prefixes but aren't the dangerous command? "mergetool" is a real git command that's safe. "gc" is dangerous but are there safe "gc-*" commands? Looking at git documentation, I don't think there are common false positives for most of these. The patterns like "commit --amend", "push --force", "worktree remove" are multi-word so less prone to this. The main risk is single-word patterns like "merge", "reset", "gc", "prune", "rebase", "clean". While "mergetool" is a real false positive case, the comment is suggesting a code quality improvement without demonstrating that this is actually causing problems in practice. The rules state I should only keep comments that have STRONG EVIDENCE of being correct and actionable. This is more of a "could lead to" speculative suggestion rather than "this definitely causes X problem". The code works for its intended purpose even if not perfectly precise. This comment is speculative about potential false positives without demonstrating actual harm. While technically correct that "mergetool" would match "merge", this falls into the category of code quality suggestions that aren't clearly actionable or critical. The rules emphasize removing comments unless there's STRONG EVIDENCE of an issue.
6. git-permission-guard/scripts/git-permission-guard.py:118
- Draft comment:
Both DENY and ASK decisions result in an exit with code 0 (using sys.exit(0)). Confirm that returning exit code 0 for denied commands is acceptable per your hook integration requirements, as it deviates from a typical non-zero error code for errors. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_cWhpbEnpnMYeJkjA
You can customize
- Fix rm hooks regex to catch individual hook files - Fix command matching to avoid false positives (word boundaries) - Sort ASK patterns from most to least specific - Add support for push --force-with-lease - Move DENY checks before early exit to catch non-git commands Fixes: - rm .git/hooks/pre-commit now blocked (not just directory) - git show some-reset-branch no longer false positive - Simpler token-based matching instead of substring search Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
JacobPEvans
commented
Jan 22, 2026
Owner
Author
JacobPEvans
left a comment
JacobPEvans
left a comment
There was a problem hiding this comment.
All review feedback has been addressed in commit 799828e:
- rm hooks regex - Now catches individual hook files (e.g., .git/hooks/pre-commit)
- Command matching - Token-based matching with word boundaries (no more false positives like 'emergency')
- Pattern ordering - Most specific first (commit --amend before commit, push --force-with-lease before push --force)
- Typo fixed - force-w-lease → force-with-lease in README
- DENY before early exit - Non-git commands (like rm) now checked before early exit
Tests confirm:
- rm .git/hooks/pre-commit → DENY
- git show some-reset-branch → ALLOW (no false positive)
- git push --force-with-lease → ASK
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 799828e in 1 minute and 33 seconds. Click for details.
- Reviewed
109lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. git-permission-guard/README.md:37
- Draft comment:
Updated the blocked commands list to includepush --force-with-lease. This change aligns the documentation with the new behavior in the code. - Reason this comment was not posted:
Confidence changes required:0%<= threshold15%None
2. git-permission-guard/scripts/git-permission-guard.py:22
- Draft comment:
The regex for the 'rm' command in DENY_PATTERNS has been broadened (from using an optional '-rf?' flag to '.*'). Confirm that this more permissive match intentionally covers additional cases without matching unintended patterns. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. git-permission-guard/scripts/git-permission-guard.py:28
- Draft comment:
ASK_GIT has been reordered and now includes extra entries (e.g. 'push --force-with-lease'). This is a good move for specificity. Verify that the generic token 'rm' won’t lead to unintended matches in legitimate contexts. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold15%The comment is asking the PR author to verify something, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code. Therefore, it should be removed.
4. git-permission-guard/scripts/git-permission-guard.py:45
- Draft comment:
The ASK_GH patterns have been re-ordered (with 'release delete' moved up). This change improves consistency with the intended specificity and documentation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold15%None
5. git-permission-guard/scripts/git-permission-guard.py:88
- Draft comment:
Moving the DENY patterns check before verifying if the command starts with 'git' or 'gh' ensures even dangerous non-git commands (like 'rm .git/hooks') are caught. This early interception is a positive safety improvement. - Reason this comment was not posted:
Confidence changes required:0%<= threshold15%None
6. git-permission-guard/scripts/git-permission-guard.py:125
- Draft comment:
The new token-based matching for ASK patterns (splitting both command and subcommand) effectively reduces false positives (e.g., avoiding matching 'merge' in 'emergency'). Ensure that this approach reliably handles commands with quoted arguments. - Reason this comment was not posted:
Confidence changes required:0%<= threshold15%None
Workflow ID: wflow_RlWncbhGipgEtbHs
You can customize
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
git-permission-guardplugin with PreToolUse hook for centralized git/gh permission managementgit -C <path>by extracting the actual subcommand for rule matchingPermission Categories
--no-verify, hook manipulationreset,force push,merge,gh pr mergestatus,log,diff,commit,pushTest plan
git statuspasses silently (ALLOW)git reflogpasses silently (ALLOW)git commit -nblocked with DENY messagegit reset --hardblocked with ASK messagegit -C /path statuspasses silently (ALLOW)git -C /path merge mainblocked with ASK messagegh pr listpasses silently (ALLOW)gh pr mergeblocked with ASK message + explicit user request note🤖 Generated with Claude Code
Important
Adds
git-permission-guardplugin for centralized git/gh permission management with categorized allow/ask/deny logic and detailed warnings.git-permission-guardplugin withPreToolUsehook for centralized git/gh permission management.git -C <path>by extracting subcommand for rule matching.--no-verify, hook manipulation.reset,force push,merge,gh pr merge.status,log,diff,commit,push.git-permission-guard.py: Implements deny and ask logic using regex patterns for command matching.hooks.json: ConfiguresPreToolUsehook to use the Python script.plugin.json: Metadata for the plugin including name, version, and description.This description was created by
for 799828e. You can customize this summary. It will automatically update as commits are pushed.