feat(ci): wire LOC-gate into pre-commit hook (staged files only) - #237
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesGit hook enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Agentflare
participant GitRepository
participant Hooks
participant LocGate
User->>Agentflare: run git install-hooks
Agentflare->>GitRepository: copy pre-commit and pre-push
Agentflare->>GitRepository: set core.hooksPath
GitRepository->>Hooks: run hook on commit
Hooks->>LocGate: validate staged Rust paths
LocGate-->>Hooks: return validation status
Hooks-->>GitRepository: allow or reject operation
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
scripts/loc-gate.sh now accepts an optional file list; with no args it does the existing full-repo git ls-files scan (CI), with args it checks only those (pre-commit). .githooks/pre-commit calls it against staged .rs files so violations get caught before commit, not just in CI.
201cec8 to
a0075b9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.githooks/pre-commit:
- Around line 76-79: Update the staged Rust filename collection in the
pre-commit hook to request NUL-delimited output from git and consume it with
NUL-aware reading, preserving complete filenames—including embedded
newlines—when populating staged_rs for the LOC gate.
In @.githooks/pre-push:
- Around line 43-45: Update the branch-matching case in the pre-push hook’s read
loop to inspect remote_ref rather than local_ref, so protection applies to
pushes targeting the remote default branch while allowing pushes sourced from it
to other branches.
In `@scripts/loc-gate.sh`:
- Around line 47-54: Update the file-reading logic in the loc-gate scan loop to
count lines from each file’s staged Git index blob rather than the working-tree
path. Preserve the existing hidden-folder filtering and regular-file checks, and
ensure the pre-commit invocation continues validating the exact staged content
passed by the hook.
In `@src/cli/git.rs`:
- Around line 73-94: Update the repository-root resolution in the install-hooks
flow to obtain repo_root from git rev-parse --show-toplevel, rather than
current_dir. Preserve the existing not-a-repository error handling and use the
resolved root for .githooks creation and subsequent operations.
- Around line 34-38: Update the install-hooks flow using InstallHooksArgs and
its opts value to honor the confirmation contract: before overwriting existing
.githooks/pre-commit or .githooks/pre-push files, prompt for confirmation unless
opts.yes is true. Preserve non-interactive behavior when --yes is supplied, and
abort without overwriting if confirmation is declined.
- Around line 72-98: Update install_hooks to return a Result and propagate
failures from ensure_shared_templates, directory creation, hook copying,
permission updates, and git config operations instead of returning success or
ignoring errors. Check the git config result before printing the configured
core.hooksPath, and emit the success message only after every installation step
completes; update the related call sites and ranges around install_hooks
accordingly.
- Around line 52-62: Update ensure_shared_templates to overwrite the existing
pre-commit and pre-push files on every installation using the current PRE_COMMIT
and PRE_PUSH embedded contents; remove the existence checks while preserving
directory creation and error propagation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 79d4d6f9-d188-4318-a542-49b738c7f202
📒 Files selected for processing (5)
.githooks/pre-commit.githooks/pre-pushscripts/loc-gate.shsrc/cli/git.rssrc/cli/mod.rs
| staged_rs=() | ||
| while IFS= read -r f; do | ||
| [ -n "$f" ] && staged_rs+=("$f") | ||
| done < <(git diff --cached --name-only --diff-filter=ACMR -- '*.rs' 2>/dev/null || true) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use NUL-delimited staged filenames.
--name-only plus line-based read splits valid filenames containing newlines, causing the resulting fragments to be silently skipped by the LOC gate.
Proposed fix
staged_rs=()
- while IFS= read -r f; do
+ while IFS= read -r -d '' f; do
[ -n "$f" ] && staged_rs+=("$f")
- done < <(git diff --cached --name-only --diff-filter=ACMR -- '*.rs' 2>/dev/null || true)
+ done < <(git diff --cached --name-only -z --diff-filter=ACMR -- '*.rs' 2>/dev/null || true)📝 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.
| staged_rs=() | |
| while IFS= read -r f; do | |
| [ -n "$f" ] && staged_rs+=("$f") | |
| done < <(git diff --cached --name-only --diff-filter=ACMR -- '*.rs' 2>/dev/null || true) | |
| staged_rs=() | |
| while IFS= read -r -d '' f; do | |
| [ -n "$f" ] && staged_rs+=("$f") | |
| done < <(git diff --cached --name-only -z --diff-filter=ACMR -- '*.rs' 2>/dev/null || true) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.githooks/pre-commit around lines 76 - 79, Update the staged Rust filename
collection in the pre-commit hook to request NUL-delimited output from git and
consume it with NUL-aware reading, preserving complete filenames—including
embedded newlines—when populating staged_rs for the LOC gate.
| while read -r local_ref local_sha remote_ref remote_sha; do | ||
| case "$local_ref" in | ||
| refs/heads/"$default") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard the destination ref, not the source ref.
The protected branch is remote_ref. Checking local_ref allows git push origin feature:main to update the remote default branch while rejecting harmless pushes such as main:backup.
Proposed fix
while read -r local_ref local_sha remote_ref remote_sha; do
- case "$local_ref" in
+ case "$remote_ref" in
refs/heads/"$default")📝 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.
| while read -r local_ref local_sha remote_ref remote_sha; do | |
| case "$local_ref" in | |
| refs/heads/"$default") | |
| while read -r local_ref local_sha remote_ref remote_sha; do | |
| case "$remote_ref" in | |
| refs/heads/"$default") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.githooks/pre-push around lines 43 - 45, Update the branch-matching case in
the pre-push hook’s read loop to inspect remote_ref rather than local_ref, so
protection applies to pushes targeting the remote default branch while allowing
pushes sourced from it to other branches.
| while IFS= read -r -d '' file; do | ||
| # Skip anything under a hidden folder (.worktrees, .claude, .github, …) — | ||
| # never project source, regardless of tracking state. | ||
| case "$file" in | ||
| .*/*|*/.*) continue ;; | ||
| esac | ||
| [[ -f "$file" ]] || continue | ||
| lines=$(wc -l <"$file" | tr -d ' ') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the staged blob, not the working-tree file.
The pre-commit hook passes staged paths, but wc reads their current working-tree contents. A user can stage an oversized file, replace it with a shorter working copy, and commit the oversized index version successfully. Read each partial-scan file from the Git index, or add an explicit cached-input mode used by .githooks/pre-commit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/loc-gate.sh` around lines 47 - 54, Update the file-reading logic in
the loc-gate scan loop to count lines from each file’s staged Git index blob
rather than the working-tree path. Preserve the existing hidden-folder filtering
and regular-file checks, and ensure the pre-commit invocation continues
validating the exact staged content passed by the hook.
| #[derive(Args)] | ||
| pub struct InstallHooksArgs { | ||
| /// Skip the confirmation prompt (for non-interactive/scripted use). | ||
| #[arg(long)] | ||
| pub yes: bool, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Honor the confirmation contract before overwriting hooks.
--yes is documented as skipping confirmation, but opts is discarded and existing .githooks/pre-commit and .githooks/pre-push files are overwritten without consent. Prompt unless opts.yes, especially when either destination already exists.
Also applies to: 100-104, 131-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/git.rs` around lines 34 - 38, Update the install-hooks flow using
InstallHooksArgs and its opts value to honor the confirmation contract: before
overwriting existing .githooks/pre-commit or .githooks/pre-push files, prompt
for confirmation unless opts.yes is true. Preserve non-interactive behavior when
--yes is supplied, and abort without overwriting if confirmation is declined.
| fn ensure_shared_templates() -> std::io::Result<()> { | ||
| let dir = shared_hooks_dir(); | ||
| fs::create_dir_all(&dir)?; | ||
| let pc = dir.join("pre-commit"); | ||
| if !pc.exists() { | ||
| fs::write(&pc, PRE_COMMIT)?; | ||
| } | ||
| let pp = dir.join("pre-push"); | ||
| if !pp.exists() { | ||
| fs::write(&pp, PRE_PUSH)?; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Refresh shared templates on every installation.
Files are written only when absent, so an older Agentflare installation permanently retains stale embedded hooks. Subsequent runs copy those stale scripts instead of the current PRE_COMMIT and PRE_PUSH versions.
Proposed fix
let pc = dir.join("pre-commit");
- if !pc.exists() {
- fs::write(&pc, PRE_COMMIT)?;
- }
+ fs::write(&pc, PRE_COMMIT)?;
let pp = dir.join("pre-push");
- if !pp.exists() {
- fs::write(&pp, PRE_PUSH)?;
- }
+ fs::write(&pp, PRE_PUSH)?;📝 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.
| fn ensure_shared_templates() -> std::io::Result<()> { | |
| let dir = shared_hooks_dir(); | |
| fs::create_dir_all(&dir)?; | |
| let pc = dir.join("pre-commit"); | |
| if !pc.exists() { | |
| fs::write(&pc, PRE_COMMIT)?; | |
| } | |
| let pp = dir.join("pre-push"); | |
| if !pp.exists() { | |
| fs::write(&pp, PRE_PUSH)?; | |
| } | |
| fn ensure_shared_templates() -> std::io::Result<()> { | |
| let dir = shared_hooks_dir(); | |
| fs::create_dir_all(&dir)?; | |
| let pc = dir.join("pre-commit"); | |
| fs::write(&pc, PRE_COMMIT)?; | |
| let pp = dir.join("pre-push"); | |
| fs::write(&pp, PRE_PUSH)?; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/git.rs` around lines 52 - 62, Update ensure_shared_templates to
overwrite the existing pre-commit and pre-push files on every installation using
the current PRE_COMMIT and PRE_PUSH embedded contents; remove the existence
checks while preserving directory creation and error propagation.
| fn install_hooks(opts: InstallHooksArgs) { | ||
| let repo_root = match std::env::current_dir() { | ||
| Ok(d) => d, | ||
| Err(e) => { | ||
| eprintln!("agentflare git install-hooks: cannot resolve cwd: {e}"); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| // Sanity: must be inside a git repo. | ||
| if !repo_root.join(".git").exists() | ||
| && run_git(&repo_root, &["rev-parse", "--git-dir"]).is_none() | ||
| { | ||
| eprintln!("agentflare git install-hooks: not a git repository (run inside a repo root)"); | ||
| return; | ||
| } | ||
|
|
||
| if let Err(e) = ensure_shared_templates() { | ||
| eprintln!("agentflare git install-hooks: cannot write shared templates: {e}"); | ||
| return; | ||
| } | ||
|
|
||
| let local_dir = repo_root.join(".githooks"); | ||
| if let Err(e) = fs::create_dir_all(&local_dir) { | ||
| eprintln!("agentflare git install-hooks: cannot create {local_dir:?}: {e}"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate installation failures before reporting success.
Template, directory, copy, permission, and git config failures either return a successful CLI status or are ignored entirely. In particular, the command prints core.hooksPath as configured even when git config fails. Return a Result, check every operation, and print success only after all steps complete.
Also applies to: 111-129, 153-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/git.rs` around lines 72 - 98, Update install_hooks to return a Result
and propagate failures from ensure_shared_templates, directory creation, hook
copying, permission updates, and git config operations instead of returning
success or ignoring errors. Check the git config result before printing the
configured core.hooksPath, and emit the success message only after every
installation step completes; update the related call sites and ranges around
install_hooks accordingly.
| let repo_root = match std::env::current_dir() { | ||
| Ok(d) => d, | ||
| Err(e) => { | ||
| eprintln!("agentflare git install-hooks: cannot resolve cwd: {e}"); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| // Sanity: must be inside a git repo. | ||
| if !repo_root.join(".git").exists() | ||
| && run_git(&repo_root, &["rev-parse", "--git-dir"]).is_none() | ||
| { | ||
| eprintln!("agentflare git install-hooks: not a git repository (run inside a repo root)"); | ||
| return; | ||
| } | ||
|
|
||
| if let Err(e) = ensure_shared_templates() { | ||
| eprintln!("agentflare git install-hooks: cannot write shared templates: {e}"); | ||
| return; | ||
| } | ||
|
|
||
| let local_dir = repo_root.join(".githooks"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve the actual repository root.
When invoked from a repository subdirectory, rev-parse --git-dir passes but .githooks is created beneath that subdirectory. The relative core.hooksPath is resolved for the repository, so the installed hooks are not found. Use git rev-parse --show-toplevel as repo_root.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/git.rs` around lines 73 - 94, Update the repository-root resolution
in the install-hooks flow to obtain repo_root from git rev-parse
--show-toplevel, rather than current_dir. Preserve the existing not-a-repository
error handling and use the resolved root for .githooks creation and subsequent
operations.
1 participant
Summary
chore/branch-guard-git-hooksbranch's git-hooks commit (.githooks/pre-commit/pre-push,src/cli/git.rsinstall command) onto current master — its second commit (worktree squash-merge skip) was dropped, already landed independently on master via a different implementation (run_git_in_okinsrc/worktree.rs)scripts/loc-gate.shnow accepts an optional file list: no args = full-repo scan (unchanged, CI-ready), with args = fast partial scan of just those files, skipping the whole-repo allowlist-ratchet check.githooks/pre-commitnow runs the LOC gate against staged*.rsfiles only, so violations are caught before commit rather than only in CI (which isn't wired up yet — see feat: asset MCP tool — attach/get/list/delete with storage, dedup, and tests #168, blocked on splittingsrc/mcp_server.rs)Depends on #236 (the
git ls-files/hidden-folder traversal fix) — this branch includes that commit too since it was built on top of it locally. Once #236 merges, this branch needs a quick rebase to drop the now-duplicate commit; I'll handle that before this one merges.Test plan
cargo build/cargo fmt --checkclean;cargo clippy -p agentflare --bin agentflareclean (full--all-targetsclippy currently fails on an unrelated, pre-existing Windows-only issue inagent_launch.rs— filed as handoff: assign items + attach versioned assets instead of raw artifacts #169, not part of this diff)bash -nsyntax-checked both shell scripts.rsfile commits pass, an oversized.rsfile is blocked, a commit touching no.rsfiles is unaffectedcore.hooksPathalready.githookshere) — the hook fired and passed on this PR's own commitSummary by CodeRabbit
agentflare git install-hooksto install and enable pre-commit and pre-push branch safeguards in the current repository.*.rschanges.