refactor(git): replace bash+jq hook scripts with Python - #16
Merged
Conversation
Replace jq-dependent bash scripts with Python 3 stdlib equivalents to improve portability (jq is not installed by default on macOS). - Create git-bash-router.py replicating all 7 routing categories - Extract inline jq fork detection into fork-detector.py - Delete git-bash-router.sh - Update hooks.json command references - Update CLAUDE.md and hooks/README.md documentation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis pull request migrates the Git hooks router implementation from a Bash/jq-based script to Python, introduces a new fork-detection script, and updates related documentation and configuration. The router enforces skill-based access controls and safety checks for Git commands with an override mechanism. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
![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 (2)
git/scripts/fork-detector.py (2)
14-17: Add timeout to subprocess call for consistency.The
subprocess.runcall here lacks atimeoutparameter, unlike the similar call ingit-bash-router.py(Line 19) which usestimeout=5. If thegit remote get-urlcommand hangs (e.g., due to network issues with some remote configurations), this could cause the hook to block indefinitely.Proposed fix
result = subprocess.run( ["git", "remote", "get-url", "upstream"], - capture_output=True, text=True + capture_output=True, text=True, timeout=5 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@git/scripts/fork-detector.py` around lines 14 - 17, The subprocess.run call that executes ["git","remote","get-url","upstream"] (assigned to result) must include a timeout to avoid hanging; update the subprocess.run invocation in fork-detector.py to pass timeout=5 (matching git-bash-router.py) and keep capture_output=True, text=True so the call fails fast on network hangs and raises a TimeoutExpired if exceeded.
28-36: Handle emptypr_ownerto avoid false warnings.If
tool_input.owneris missing or empty,pr_ownerwill be an empty string. Comparing an empty string against a validupstream_ownerwill always trigger the warning, even though the issue isn't a fork mismatch but rather missing input data.Proposed fix
upstream_owner = match.group("owner") pr_owner = data.get("tool_input", {}).get("owner", "") + if not pr_owner: + return None # No owner specified, can't validate + if pr_owner != upstream_owner:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@git/scripts/fork-detector.py` around lines 28 - 36, The current comparison between pr_owner and upstream_owner triggers false fork warnings when tool_input.owner is missing or empty; update the logic around pr_owner and the conditional that returns the warning (the block referencing pr_owner and upstream_owner) to first detect and handle a missing/empty pr_owner (e.g., if not pr_owner) by returning a clear non-fork result or a different hookSpecificOutput indicating missing input, and only perform the pr_owner != upstream_owner fork check when pr_owner is present; ensure you update the branch that currently returns the "WARNING: Fork detected..." message to only run when pr_owner is non-empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@git/scripts/git-bash-router.py`:
- Around line 14-21: The code may leave mainline empty when
CLAUDE_MAINLINE_BRANCH is unset and the detect script fails; update the mainline
detection logic (the block that calls detect-mainline.sh and sets variable
mainline) to validate that mainline is non-empty and, if empty, set a sensible
default like "main" (or skip constructing regexes) before any use in
pushing_mainline() and the rebase check; ensure all regex constructions and
matches in functions pushing_mainline() and the rebase check use this validated
non-empty mainline to avoid unintended matches.
---
Nitpick comments:
In `@git/scripts/fork-detector.py`:
- Around line 14-17: The subprocess.run call that executes
["git","remote","get-url","upstream"] (assigned to result) must include a
timeout to avoid hanging; update the subprocess.run invocation in
fork-detector.py to pass timeout=5 (matching git-bash-router.py) and keep
capture_output=True, text=True so the call fails fast on network hangs and
raises a TimeoutExpired if exceeded.
- Around line 28-36: The current comparison between pr_owner and upstream_owner
triggers false fork warnings when tool_input.owner is missing or empty; update
the logic around pr_owner and the conditional that returns the warning (the
block referencing pr_owner and upstream_owner) to first detect and handle a
missing/empty pr_owner (e.g., if not pr_owner) by returning a clear non-fork
result or a different hookSpecificOutput indicating missing input, and only
perform the pr_owner != upstream_owner fork check when pr_owner is present;
ensure you update the branch that currently returns the "WARNING: Fork
detected..." message to only run when pr_owner is non-empty.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
git/CLAUDE.mdgit/hooks/README.mdgit/hooks/hooks.jsongit/scripts/fork-detector.pygit/scripts/git-bash-router.pygit/scripts/git-bash-router.sh
💤 Files with no reviewable changes (1)
- git/scripts/git-bash-router.sh
Comment on lines
+14
to
+21
| if not mainline: | ||
| plugin_root = os.environ.get("CLAUDE_PLUGIN_ROOT", "") | ||
| detect_script = os.path.join(plugin_root, "scripts", "detect-mainline.sh") | ||
| result = subprocess.run( | ||
| ["bash", detect_script], | ||
| capture_output=True, text=True, timeout=5 | ||
| ) | ||
| mainline = result.stdout.strip() |
There was a problem hiding this comment.
Empty mainline can cause regex patterns to match unintended content.
If CLAUDE_MAINLINE_BRANCH is unset and the detect script fails (e.g., script not found, non-git directory), mainline remains an empty string. The patterns in pushing_mainline() (Line 41-43) and the rebase check (Line 92) would then match any whitespace followed by end-of-string or more whitespace, potentially causing false positives.
Consider validating that mainline is non-empty before using it in patterns, or falling back to a sensible default like "main".
Proposed fix
if not mainline:
plugin_root = os.environ.get("CLAUDE_PLUGIN_ROOT", "")
detect_script = os.path.join(plugin_root, "scripts", "detect-mainline.sh")
result = subprocess.run(
["bash", detect_script],
capture_output=True, text=True, timeout=5
)
mainline = result.stdout.strip()
+ if not mainline:
+ mainline = "main" # Fallback default🧰 Tools
🪛 Ruff (0.15.2)
[error] 17-17: subprocess call: check for execution of untrusted input
(S603)
[error] 18-18: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@git/scripts/git-bash-router.py` around lines 14 - 21, The code may leave
mainline empty when CLAUDE_MAINLINE_BRANCH is unset and the detect script fails;
update the mainline detection logic (the block that calls detect-mainline.sh and
sets variable mainline) to validate that mainline is non-empty and, if empty,
set a sensible default like "main" (or skip constructing regexes) before any use
in pushing_mainline() and the rebase check; ensure all regex constructions and
matches in functions pushing_mainline() and the rebase check use this validated
non-empty mainline to avoid unintended matches.
8 tasks
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
jq-dependent bash scripts with Python 3 stdlib equivalents for better portability (jqis not installed by default on macOS)jqone-liner inhooks.jsoninto standalone scriptChanges
git/scripts/git-bash-router.py(new) — Python 3 replacement forgit-bash-router.sh, replicating all routing logic usingjson,re, andsubprocessmodulesgit/scripts/fork-detector.py(new) — Extracted inline jq fork detection fromhooks.jsoninto standalone Python scriptgit/scripts/git-bash-router.sh(deleted) — Old bash+jq routergit/hooks/hooks.json— Updated command references to.pyscriptsgit/CLAUDE.md— Updated architecture, performance (~30-60ms), and technical notesgit/hooks/README.md— Updated architecture, test commands, and technical referenceTesting
All 14 test cases pass:
git commit,git checkout -b,gh pr create→ denyGIT_WORKFLOWS_OVERRIDE=1prefix → null (passthrough)git reset --hard,git clean -f,git rebase main→ additionalContextgit status→ nullSummary by CodeRabbit
Refactor
Documentation
Chores