feat: add git hook install/uninstall commands#48
Conversation
Add 'argus hook install' to install pre-commit hook Add 'argus hook uninstall' to remove pre-commit hook The hook runs Argus review on staged .rs files before commit.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces a new Hook CLI subcommand with Install and Uninstall actions to manage Git pre-commit hooks. The Hook command generates a pre-commit script that runs Argus review on staged Rust files, sets executable permissions, and handles error cases for existing or missing hooks. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant FileSystem
participant Git
User->>CLI: hook install [path]
CLI->>FileSystem: check .git/hooks/pre-commit exists
alt Hook exists
CLI->>User: error (hook already exists)
else Hook does not exist
CLI->>FileSystem: write pre-commit script
CLI->>FileSystem: set executable permissions
CLI->>Git: register pre-commit hook
CLI->>User: success (hook installed)
end
User->>CLI: hook uninstall [path]
CLI->>FileSystem: check .git/hooks/pre-commit exists
alt Hook exists
CLI->>FileSystem: remove pre-commit hook
CLI->>User: success (hook removed)
else Hook missing
CLI->>User: error (hook not found)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main.rs (1)
1786-1797: Missing.git/hooksdirectory check; uninstall may remove a non-Argus hook.
- Install doesn't verify that
.git/hooksexists. If--pathpoints to a non-repo or a bare repo with a different hooks layout,std::fs::writewill fail with a raw OS error rather than a helpful message.- Uninstall blindly removes any
pre-commithook without checking whether Argus installed it — this could silently destroy a user's custom hook or one managed by another tool (e.g., husky, pre-commit framework).Consider:
- Checking
path.join(".git/hooks").is_dir()before install and bailing with a clear message otherwise.- Embedding a sentinel comment (e.g.,
# ARGUS_MANAGED_HOOK) in the generated script and verifying it on uninstall.♻️ Sketch for safer install/uninstall
+ let hooks_dir = path.join(".git/hooks"); + if !hooks_dir.is_dir() { + miette::bail!( + "No .git/hooks directory found at {}. Is this a git repository?", + path.display() + ); + } let hook_path = path.join(".git/hooks/pre-commit");For uninstall, check for the sentinel before removing:
HookAction::Uninstall => { let hook_path = path.join(".git/hooks/pre-commit"); if !hook_path.exists() { miette::bail!("pre-commit hook not found at {}", hook_path.display()); } + let content = std::fs::read_to_string(&hook_path).into_diagnostic()?; + if !content.contains("# Argus pre-commit hook") { + miette::bail!( + "pre-commit hook at {} was not installed by Argus. Remove it manually if intended.", + hook_path.display() + ); + } std::fs::remove_file(&hook_path).into_diagnostic()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1786 - 1797, Check that the hooks directory exists before writing: before using hook_path = path.join(".git/hooks/pre-commit") and calling std::fs::write(&hook_path, hook_content), verify path.join(".git/hooks").is_dir() and bail with a clear miette::bail! message if it doesn't, so installs on non-repos or bare repos fail with a helpful error; embed a unique sentinel string (e.g., "# ARGUS_MANAGED_HOOK") into the generated hook_content when creating the hook; and modify the uninstall logic to read the existing pre-commit file (using the same hook_path) and only remove it if the file contains that sentinel, otherwise return an error/warning and do not delete the hook.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.rs`:
- Around line 1791-1794: The long chained call to
std::fs::set_permissions(&hook_path,
std::fs::Permissions::from_mode(0o755)).into_diagnostic()? is exceeding
rustfmt's line length; refactor the expression in the #[cfg(unix)] block by
splitting it into smaller statements (e.g. create a local perms via
std::fs::Permissions::from_mode(0o755) and then call
std::fs::set_permissions(&hook_path, perms) and call into_diagnostic() on that
result) so that hook_path, set_permissions, PermissionsExt and
Permissions::from_mode are all still used but lines are shorter and rustfmt will
pass.
- Around line 1754-1784: The pre-commit hook in hook_content incorrectly quotes
staged_files so git diff gets a single arg, leaves an unused against variable,
and silently truncates files with head -20; fix by (1) removing the unused
against assignment (dead code), (2) collect staged Rust paths into a proper
shell array (e.g., build staged_files_array from the output of git diff --cached
--name-only --diff-filter=ACM | grep '\.rs$') and call git diff --cached --
"${staged_files_array[@]}" > "$tmpfile" so each file is a separate arg (do not
pass the newline-separated $staged_files as one quoted string), and (3) either
remove the head -20 limit or detect/count >20 files and print a warning to the
user instead of silently dropping files; update references to staged_files and
tmpfile and ensure argus review is still invoked with --file "$tmpfile" and
correct exit handling.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 1786-1797: Check that the hooks directory exists before writing:
before using hook_path = path.join(".git/hooks/pre-commit") and calling
std::fs::write(&hook_path, hook_content), verify
path.join(".git/hooks").is_dir() and bail with a clear miette::bail! message if
it doesn't, so installs on non-repos or bare repos fail with a helpful error;
embed a unique sentinel string (e.g., "# ARGUS_MANAGED_HOOK") into the generated
hook_content when creating the hook; and modify the uninstall logic to read the
existing pre-commit file (using the same hook_path) and only remove it if the
file contains that sentinel, otherwise return an error/warning and do not delete
the hook.
src/main.rs
Outdated
| let hook_content = r#"#!/bin/sh | ||
| # Argus pre-commit hook | ||
| # Runs Argus review on staged changes before commit | ||
| if git rev-parse --verify HEAD >/dev/null 2>&1; then | ||
| against=HEAD | ||
| else | ||
| against=4b825dc642cb6eb9a060e54bf8d69288fbee4904 # empty tree | ||
| fi | ||
|
|
||
| # Get list of staged .rs files | ||
| staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' | head -20) | ||
|
|
||
| if [ -z "$staged_files" ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Create temp file for diff | ||
| tmpfile=$(mktemp) | ||
| git diff --cached -- "$staged_files" > "$tmpfile" | ||
|
|
||
| # Run argus review | ||
| if argus review --file "$tmpfile" --fail-on warning --repo .; then | ||
| rm -f "$tmpfile" | ||
| exit 0 | ||
| else | ||
| rm -f "$tmpfile" | ||
| echo "Argus review failed. Commit aborted." | ||
| echo "Use --no-verify to skip this hook if needed." | ||
| exit 1 | ||
| fi | ||
| "#; |
There was a problem hiding this comment.
Shell script bug: $staged_files quoting breaks git diff with multiple files.
On line 1772, "$staged_files" is double-quoted, so the entire newline-separated list is passed as a single argument to git diff --cached --. Git will look for a file whose name literally contains newlines, producing an empty diff. The variable must be unquoted (or the approach reworked) so word-splitting expands it to multiple file arguments.
Additionally:
- The
againstvariable (lines 1757–1760) is assigned but never referenced anywhere in the script — dead code left over from a template. head -20silently drops files beyond 20 with no warning to the user.
🐛 Proposed fix for the shell script
let hook_content = r#"#!/bin/sh
# Argus pre-commit hook
# Runs Argus review on staged changes before commit
-if git rev-parse --verify HEAD >/dev/null 2>&1; then
- against=HEAD
-else
- against=4b825dc642cb6eb9a060e54bf8d69288fbee4904 # empty tree
-fi
# Get list of staged .rs files
-staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' | head -20)
+staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$')
if [ -z "$staged_files" ]; then
exit 0
fi
# Create temp file for diff
tmpfile=$(mktemp)
-git diff --cached -- "$staged_files" > "$tmpfile"
+git diff --cached -- $staged_files > "$tmpfile"
# Run argus review
if argus review --file "$tmpfile" --fail-on warning --repo .; then📝 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.
| let hook_content = r#"#!/bin/sh | |
| # Argus pre-commit hook | |
| # Runs Argus review on staged changes before commit | |
| if git rev-parse --verify HEAD >/dev/null 2>&1; then | |
| against=HEAD | |
| else | |
| against=4b825dc642cb6eb9a060e54bf8d69288fbee4904 # empty tree | |
| fi | |
| # Get list of staged .rs files | |
| staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' | head -20) | |
| if [ -z "$staged_files" ]; then | |
| exit 0 | |
| fi | |
| # Create temp file for diff | |
| tmpfile=$(mktemp) | |
| git diff --cached -- "$staged_files" > "$tmpfile" | |
| # Run argus review | |
| if argus review --file "$tmpfile" --fail-on warning --repo .; then | |
| rm -f "$tmpfile" | |
| exit 0 | |
| else | |
| rm -f "$tmpfile" | |
| echo "Argus review failed. Commit aborted." | |
| echo "Use --no-verify to skip this hook if needed." | |
| exit 1 | |
| fi | |
| "#; | |
| let hook_content = r#"#!/bin/sh | |
| # Argus pre-commit hook | |
| # Runs Argus review on staged changes before commit | |
| # Get list of staged .rs files | |
| staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$') | |
| if [ -z "$staged_files" ]; then | |
| exit 0 | |
| fi | |
| # Create temp file for diff | |
| tmpfile=$(mktemp) | |
| git diff --cached -- $staged_files > "$tmpfile" | |
| # Run argus review | |
| if argus review --file "$tmpfile" --fail-on warning --repo .; then | |
| rm -f "$tmpfile" | |
| exit 0 | |
| else | |
| rm -f "$tmpfile" | |
| echo "Argus review failed. Commit aborted." | |
| echo "Use --no-verify to skip this hook if needed." | |
| exit 1 | |
| fi | |
| "#; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.rs` around lines 1754 - 1784, The pre-commit hook in hook_content
incorrectly quotes staged_files so git diff gets a single arg, leaves an unused
against variable, and silently truncates files with head -20; fix by (1)
removing the unused against assignment (dead code), (2) collect staged Rust
paths into a proper shell array (e.g., build staged_files_array from the output
of git diff --cached --name-only --diff-filter=ACM | grep '\.rs$') and call git
diff --cached -- "${staged_files_array[@]}" > "$tmpfile" so each file is a
separate arg (do not pass the newline-separated $staged_files as one quoted
string), and (3) either remove the head -20 limit or detect/count >20 files and
print a warning to the user instead of silently dropping files; update
references to staged_files and tmpfile and ensure argus review is still invoked
with --file "$tmpfile" and correct exit handling.
There was a problem hiding this comment.
Pull request overview
This PR adds git pre-commit hook management functionality to Argus, allowing users to automatically run code reviews on staged changes before commits. The implementation adds a new hook subcommand with install and uninstall actions that manage a shell script in .git/hooks/pre-commit.
Changes:
- Added
Hookcommand withinstall/uninstallactions to manage pre-commit hooks - Created shell script that runs Argus review on staged .rs files before commit
- Hook fails commits on warnings and provides escape hatch via
--no-verify
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
|
|
||
| # Get list of staged .rs files | ||
| staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' | head -20) |
There was a problem hiding this comment.
The hook script limits processing to only the first 20 staged .rs files using 'head -20'. This arbitrary limit means that if more than 20 Rust files are staged, the remaining files won't be reviewed by the pre-commit hook. This could allow unreviewed changes to be committed. Consider either removing this limit or making it configurable, and document the rationale if the limit is intentional for performance reasons.
| staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' | head -20) | |
| staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$') |
src/main.rs
Outdated
| let hook_path = path.join(".git/hooks/pre-commit"); | ||
| if !hook_path.exists() { | ||
| miette::bail!("pre-commit hook not found at {}", hook_path.display()); | ||
| } |
There was a problem hiding this comment.
The uninstall command removes any pre-commit hook without verifying that it was created by Argus. This could accidentally delete a pre-commit hook created by another tool or manually by the user. Consider adding verification by checking if the hook file contains a specific marker (like the "# Argus pre-commit hook" comment) before deletion, or warn the user and ask for confirmation.
| } | |
| } | |
| // Only uninstall hooks that appear to have been installed by Argus. | |
| let hook_contents = std::fs::read_to_string(&hook_path).into_diagnostic()?; | |
| if !hook_contents.contains("# Argus pre-commit hook") { | |
| miette::bail!( | |
| "pre-commit hook at {} does not appear to be an Argus hook; refusing to remove it. \ | |
| If you are sure, remove it manually.", | |
| hook_path.display() | |
| ); | |
| } |
| # Get list of staged .rs files | ||
| staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' | head -20) | ||
|
|
||
| if [ -z "$staged_files" ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Create temp file for diff | ||
| tmpfile=$(mktemp) | ||
| git diff --cached -- "$staged_files" > "$tmpfile" | ||
|
|
There was a problem hiding this comment.
The hook script passes multiple arguments to a single git diff command using a shell variable that could contain spaces or special characters in filenames. When $staged_files contains multiple files, the command 'git diff --cached -- "$staged_files"' will fail if any filename contains spaces. The script should either iterate over files individually or use a different approach to handle filenames with special characters safely.
| # Get list of staged .rs files | |
| staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' | head -20) | |
| if [ -z "$staged_files" ]; then | |
| exit 0 | |
| fi | |
| # Create temp file for diff | |
| tmpfile=$(mktemp) | |
| git diff --cached -- "$staged_files" > "$tmpfile" | |
| # Get list of staged .rs files and write their diffs to a temp file | |
| tmpfile=$(mktemp) | |
| git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' | head -20 | while IFS= read -r file; do | |
| [ -z "$file" ] && continue | |
| git diff --cached -- "$file" >> "$tmpfile" | |
| done | |
| # If no diffs were written (no staged .rs files), exit successfully | |
| if [ ! -s "$tmpfile" ]; then | |
| rm -f "$tmpfile" | |
| exit 0 | |
| fi |
| tmpfile=$(mktemp) | ||
| git diff --cached -- "$staged_files" > "$tmpfile" | ||
|
|
||
| # Run argus review | ||
| if argus review --file "$tmpfile" --fail-on warning --repo .; then | ||
| rm -f "$tmpfile" | ||
| exit 0 | ||
| else | ||
| rm -f "$tmpfile" | ||
| echo "Argus review failed. Commit aborted." | ||
| echo "Use --no-verify to skip this hook if needed." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
If the argus review command is interrupted (e.g., by Ctrl+C) or fails unexpectedly, the temporary file created by mktemp won't be cleaned up because the cleanup only happens in the if/else branches. The trap command should be used to ensure cleanup happens even on script interruption or unexpected exits. Add 'trap "rm -f "$tmpfile"" EXIT' after creating the temp file.
| if git rev-parse --verify HEAD >/dev/null 2>&1; then | ||
| against=HEAD | ||
| else | ||
| against=4b825dc642cb6eb9a060e54bf8d69288fbee4904 # empty tree | ||
| fi |
There was a problem hiding this comment.
The shell variable 'against' is set but never used in the hook script. The variable appears to be a standard pattern from git hook templates for handling initial commits, but since it's not used anywhere in this script, it should be removed to avoid confusion.
| if git rev-parse --verify HEAD >/dev/null 2>&1; then | |
| against=HEAD | |
| else | |
| against=4b825dc642cb6eb9a060e54bf8d69288fbee4904 # empty tree | |
| fi |
src/main.rs
Outdated
| let hook_path = path.join(".git/hooks/pre-commit"); | ||
| if hook_path.exists() { | ||
| miette::bail!("pre-commit hook already exists at {}. Remove it first or run 'argus hook uninstall' first.", hook_path.display()); | ||
| } | ||
| std::fs::write(&hook_path, hook_content).into_diagnostic()?; | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| std::fs::set_permissions(&hook_path, std::fs::Permissions::from_mode(0o755)).into_diagnostic()?; | ||
| } | ||
| println!("Installed pre-commit hook at {}", hook_path.display()); |
There was a problem hiding this comment.
The pre-commit hook script uses shell syntax (#!/bin/sh) that won't work on Windows. While the code conditionally sets Unix permissions with #[cfg(unix)], the hook script itself will fail to execute on Windows where Git hooks typically need to be .bat or .ps1 files, or the user needs Git Bash. Consider either documenting that this feature requires Unix/Git Bash, or implementing platform-specific hook scripts.
| let hook_path = path.join(".git/hooks/pre-commit"); | |
| if hook_path.exists() { | |
| miette::bail!("pre-commit hook already exists at {}. Remove it first or run 'argus hook uninstall' first.", hook_path.display()); | |
| } | |
| std::fs::write(&hook_path, hook_content).into_diagnostic()?; | |
| #[cfg(unix)] | |
| { | |
| use std::os::unix::fs::PermissionsExt; | |
| std::fs::set_permissions(&hook_path, std::fs::Permissions::from_mode(0o755)).into_diagnostic()?; | |
| } | |
| println!("Installed pre-commit hook at {}", hook_path.display()); | |
| #[cfg(unix)] | |
| { | |
| let hook_path = path.join(".git/hooks/pre-commit"); | |
| if hook_path.exists() { | |
| miette::bail!( | |
| "pre-commit hook already exists at {}. Remove it first or run 'argus hook uninstall' first.", | |
| hook_path.display() | |
| ); | |
| } | |
| std::fs::write(&hook_path, hook_content).into_diagnostic()?; | |
| use std::os::unix::fs::PermissionsExt; | |
| std::fs::set_permissions( | |
| &hook_path, | |
| std::fs::Permissions::from_mode(0o755), | |
| ) | |
| .into_diagnostic()?; | |
| println!("Installed pre-commit hook at {}", hook_path.display()); | |
| } | |
| #[cfg(not(unix))] | |
| { | |
| miette::bail!( | |
| "The generated pre-commit hook uses a POSIX shell script and currently only works on Unix \ | |
| or Windows environments with Git Bash (or another POSIX-compatible shell). \ | |
| On this platform, install an appropriate pre-commit hook manually or run this command from a Unix-like shell." | |
| ); | |
| } |
src/main.rs
Outdated
| Some(Command::Hook { action, path }) => { | ||
| match action { | ||
| HookAction::Install => { | ||
| let hook_content = r#"#!/bin/sh | ||
| # Argus pre-commit hook | ||
| # Runs Argus review on staged changes before commit | ||
| if git rev-parse --verify HEAD >/dev/null 2>&1; then | ||
| against=HEAD | ||
| else | ||
| against=4b825dc642cb6eb9a060e54bf8d69288fbee4904 # empty tree | ||
| fi | ||
|
|
||
| # Get list of staged .rs files | ||
| staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' | head -20) | ||
|
|
||
| if [ -z "$staged_files" ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Create temp file for diff | ||
| tmpfile=$(mktemp) | ||
| git diff --cached -- "$staged_files" > "$tmpfile" | ||
|
|
||
| # Run argus review | ||
| if argus review --file "$tmpfile" --fail-on warning --repo .; then | ||
| rm -f "$tmpfile" | ||
| exit 0 | ||
| else | ||
| rm -f "$tmpfile" | ||
| echo "Argus review failed. Commit aborted." | ||
| echo "Use --no-verify to skip this hook if needed." | ||
| exit 1 | ||
| fi | ||
| "#; | ||
|
|
||
| let hook_path = path.join(".git/hooks/pre-commit"); | ||
| if hook_path.exists() { | ||
| miette::bail!("pre-commit hook already exists at {}. Remove it first or run 'argus hook uninstall' first.", hook_path.display()); | ||
| } | ||
| std::fs::write(&hook_path, hook_content).into_diagnostic()?; | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| std::fs::set_permissions(&hook_path, std::fs::Permissions::from_mode(0o755)).into_diagnostic()?; | ||
| } | ||
| println!("Installed pre-commit hook at {}", hook_path.display()); | ||
| } | ||
| HookAction::Uninstall => { | ||
| let hook_path = path.join(".git/hooks/pre-commit"); | ||
| if !hook_path.exists() { | ||
| miette::bail!("pre-commit hook not found at {}", hook_path.display()); | ||
| } | ||
| std::fs::remove_file(&hook_path).into_diagnostic()?; | ||
| println!("Removed pre-commit hook from {}", hook_path.display()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new hook install/uninstall functionality lacks test coverage. Following the pattern established in tests/init.rs, there should be tests that verify: 1) hook installation creates the file with correct permissions, 2) hook installation fails when a hook already exists, 3) hook uninstall removes the file, and 4) hook uninstall fails when no hook exists. Consider adding a tests/hook.rs file with these test cases.
src/main.rs
Outdated
|
|
||
| let hook_path = path.join(".git/hooks/pre-commit"); | ||
| if hook_path.exists() { | ||
| miette::bail!("pre-commit hook already exists at {}. Remove it first or run 'argus hook uninstall' first.", hook_path.display()); |
There was a problem hiding this comment.
The error message contains redundant text: "Remove it first or run 'argus hook uninstall' first." This says the same thing twice. Consider simplifying to: "pre-commit hook already exists at {}. Run 'argus hook uninstall' first to remove it."
| miette::bail!("pre-commit hook already exists at {}. Remove it first or run 'argus hook uninstall' first.", hook_path.display()); | |
| miette::bail!( | |
| "pre-commit hook already exists at {}. Run 'argus hook uninstall' first to remove it.", | |
| hook_path.display() | |
| ); |
src/main.rs
Outdated
| Some(Command::Hook { action, path }) => { | ||
| match action { | ||
| HookAction::Install => { | ||
| let hook_content = r#"#!/bin/sh | ||
| # Argus pre-commit hook | ||
| # Runs Argus review on staged changes before commit | ||
| if git rev-parse --verify HEAD >/dev/null 2>&1; then | ||
| against=HEAD | ||
| else | ||
| against=4b825dc642cb6eb9a060e54bf8d69288fbee4904 # empty tree | ||
| fi | ||
|
|
||
| # Get list of staged .rs files | ||
| staged_files=$(git diff --cached --name-only --diff-filter=ACM | grep '\.rs$' | head -20) | ||
|
|
||
| if [ -z "$staged_files" ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Create temp file for diff | ||
| tmpfile=$(mktemp) | ||
| git diff --cached -- "$staged_files" > "$tmpfile" | ||
|
|
||
| # Run argus review | ||
| if argus review --file "$tmpfile" --fail-on warning --repo .; then | ||
| rm -f "$tmpfile" | ||
| exit 0 | ||
| else | ||
| rm -f "$tmpfile" | ||
| echo "Argus review failed. Commit aborted." | ||
| echo "Use --no-verify to skip this hook if needed." | ||
| exit 1 | ||
| fi | ||
| "#; | ||
|
|
||
| let hook_path = path.join(".git/hooks/pre-commit"); | ||
| if hook_path.exists() { | ||
| miette::bail!("pre-commit hook already exists at {}. Remove it first or run 'argus hook uninstall' first.", hook_path.display()); | ||
| } | ||
| std::fs::write(&hook_path, hook_content).into_diagnostic()?; | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| std::fs::set_permissions(&hook_path, std::fs::Permissions::from_mode(0o755)).into_diagnostic()?; | ||
| } | ||
| println!("Installed pre-commit hook at {}", hook_path.display()); | ||
| } | ||
| HookAction::Uninstall => { | ||
| let hook_path = path.join(".git/hooks/pre-commit"); | ||
| if !hook_path.exists() { | ||
| miette::bail!("pre-commit hook not found at {}", hook_path.display()); | ||
| } | ||
| std::fs::remove_file(&hook_path).into_diagnostic()?; | ||
| println!("Removed pre-commit hook from {}", hook_path.display()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing validation to check if the path is actually a git repository before attempting to install or uninstall hooks. The code should verify that .git directory exists or that git2::Repository::discover() succeeds, similar to the validation done in the History command (lines 961-967). Without this check, the command will only fail with a generic filesystem error when trying to write to a non-existent .git/hooks directory, which provides a poor user experience.
src/main.rs
Outdated
| let hook_path = path.join(".git/hooks/pre-commit"); | ||
| if hook_path.exists() { | ||
| miette::bail!("pre-commit hook already exists at {}. Remove it first or run 'argus hook uninstall' first.", hook_path.display()); | ||
| } | ||
| std::fs::write(&hook_path, hook_content).into_diagnostic()?; | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| std::fs::set_permissions(&hook_path, std::fs::Permissions::from_mode(0o755)).into_diagnostic()?; | ||
| } | ||
| println!("Installed pre-commit hook at {}", hook_path.display()); |
There was a problem hiding this comment.
The code assumes that the .git/hooks directory already exists, but in some cases (like a fresh git init), this directory might not exist. The install command should create the .git/hooks directory if it doesn't exist using std::fs::create_dir_all before attempting to write the hook file. This would prevent filesystem errors when installing hooks in newly initialized repositories.
Summary
Add 'argus hook' subcommand to install/uninstall git pre-commit hooks.
Commands
argus hook install- Install pre-commit hookargus hook uninstall- Remove pre-commit hookUsage
The hook runs Argus review on staged .rs files before commit and fails on warnings.
Closes
#28
Summary by CodeRabbit