chore(make): auto-install pre-commit hooks on lint

[ralphbean]

Summary

• Add ensure-hooks target that runs pre-commit install if .git/hooks/pre-commit is missing
• Make lint and lint-all depend on ensure-hooks

Fresh clones that skip make bootstrap now get hooks installed automatically on first make lint instead of silently skipping pre-commit checks and failing in CI.

Test plan

• Verified hooks install automatically on make lint in a clone without hooks
• Verified no-op when hooks already installed

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Auto-install pre-commit hooks on first make lint

✨ Enhancement ⚙️ Configuration changes 🕐 10-20 Minutes

AI Description

• Add ensure-hooks Make target to install pre-commit hooks when missing.
• Make lint/lint-all depend on ensure-hooks to avoid fresh-clone CI surprises.

Diagram

graph TD
  Dev(["Developer"]) --> LintCmd["make lint / lint-all"] --> Ensure["ensure-hooks"] --> Install["pre-commit install (if missing)"] --> Run["pre-commit run (--all-files)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Fail fast with a bootstrap message

• ➕ No automatic repo mutation from lint targets
• ➕ Clearer separation: bootstrap/setup vs lint execution
• ➖ Still leaves developers blocked until they run the extra step
• ➖ Doesn't proactively prevent “I forgot bootstrap” until lint is run

2. Adopt `core.hooksPath` (or a repo-managed hooks directory)

• ➕ Avoids writing into .git/hooks directly
• ➕ Can centralize hook management across tooling
• ➖ Requires git config changes per-clone (or enforced tooling)
• ➖ More complex rollout and more room for environment-specific issues

Recommendation: The current approach is a pragmatic low-friction fix: it keeps make lint behavior aligned with CI by ensuring hooks exist in fresh clones. If this ever causes issues in non-git contexts (missing .git/) or for contributors who dislike implicit installation, consider switching to a fail-fast message, but for most dev flows this is the best tradeoff.

Files changed (1) +9 / -3

Other (1) +9 / -3

MakefileAdd 'ensure-hooks' and make lint targets depend on it +9/-3

Add 'ensure-hooks' and make lint targets depend on it

• Introduces an 'ensure-hooks' target that installs pre-commit hooks when '.git/hooks/pre-commit' is missing. Updates 'lint' and 'lint-all' targets to depend on 'ensure-hooks' so pre-commit checks aren’t silently skipped in fresh clones.

Makefile

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:02 PM UTC · Completed 10:10 PM UTC
Commit: a3b8418 · View workflow run →

[github-actions]

Site preview

Preview: https://f7bd90f8-site.fullsend-ai.workers.dev

Commit: 0b88574d8cdd4c670115e6eb984f0683031b4d22

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

1. Wrong hook path check ✓ Resolved 🐞 Bug ≡ Correctness

Description

ensure-hooks checks only for .git/hooks/pre-commit, which is not the authoritative hook location
when using git worktrees or a configured core.hooksPath; this will mis-detect an already-installed
hook and re-run pre-commit install on every make lint/make lint-all. That defeats the intended
no-op behavior and repeatedly rewrites hooks, adding overhead and side effects.

Code

Makefile[R74-78]

+ensure-hooks:
+	@if [ ! -f .git/hooks/pre-commit ]; then \
+		echo "==> Installing pre-commit hooks..."; \
+		pre-commit install; \
+	fi

Relevance

⭐⭐⭐ High

Team has accepted git edge-case robustness fixes (merge-base fallbacks) in scripts; likely to fix
hooksPath/worktree probe too.

PR-#286
PR-#1959

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new target hardcodes .git/hooks/pre-commit, while the repo explicitly supports/mentions
worktrees and core.hooksPath, meaning .git/hooks is not a reliable probe for whether hooks are
installed.

Makefile[74-84]
.gitignore[1-9]
mise.toml[1-4]
docs/ADRs/0032-safe-push-wrapper-for-sandboxed-agents.md[36-45]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ensure-hooks` uses a hardcoded `.git/hooks/pre-commit` existence check. In git worktrees (where `.git` is commonly a file) and when `core.hooksPath` is configured, this path is not where hooks live, so the check can be wrong and trigger `pre-commit install` repeatedly.

## Issue Context
This repo explicitly references worktrees and also acknowledges `core.hooksPath` as a git mechanism, so the hook probe should respect git’s actual hooks path.

## Fix Focus Areas
- Makefile[74-84]

## Suggested fix
Update `ensure-hooks` to compute the hook path using git:
- Determine hooks dir from `git config --get core.hooksPath` when set.
- Otherwise use `git rev-parse --git-path hooks` (worktree-safe) to locate the hooks dir.
- Then test for `$$hooks_dir/pre-commit` (quoted) instead of `.git/hooks/pre-commit`.
- Optionally guard with `git rev-parse --is-inside-work-tree >/dev/null 2>&1` to produce a clear error if not in a git checkout.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unneeded CI hook install ✓ Resolved 🐞 Bug ➹ Performance

Description

Because lint-all now depends on ensure-hooks, CI will run pre-commit install and write into
the checkout’s hooks directory even though CI only needs pre-commit run --all-files. This adds
redundant work and mutates .git during CI runs without improving CI correctness.

Code

Makefile[R80-84]

+lint: ensure-hooks
	pre-commit run

-lint-all:
+lint-all: ensure-hooks
	pre-commit run --all-files

Relevance

⭐ Low

Similar CI perf/redundant-work concerns were rejected (keep extra Go test run); likely won’t change
CI just to avoid install.

PR-#2069

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The workflow executes make lint-all, and lint-all now depends on ensure-hooks, which runs
pre-commit install (a write into hooks) before running the checks.

.github/workflows/lint.yml[27-46]
Makefile[74-84]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`make lint-all` now runs `ensure-hooks`, which invokes `pre-commit install` and writes hook files during CI. CI does not need hooks installed to run `pre-commit run --all-files`, so this is redundant work and an unnecessary mutation.

## Issue Context
GitHub Actions runs `make lint-all` in CI.

## Fix Focus Areas
- Makefile[74-84]
- .github/workflows/lint.yml[27-46]

## Suggested fix
Gate hook installation to local developer runs:
- Skip `pre-commit install` when `CI=true` or `GITHUB_ACTIONS=true`, or
- Provide an override (e.g., `SKIP_HOOK_INSTALL=1`) and set it in CI, while keeping the local auto-install behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Review

Verdict: Approve

A clean, well-scoped chore change. The ensure-hooks target adds robust conditional logic that correctly handles CI environments, custom core.hooksPath configurations, and git worktrees. The second commit (0b88574) addressed the hardcoded .git/hooks/ path from the initial implementation, making the target worktree-safe via git rev-parse --git-path hooks. Two low-severity findings are noted below for optional follow-up — neither blocks merge.

Findings

Low

• [error-handling] Makefile:79 — If pre-commit is not on PATH, the ensure-hooks target prints ==> Installing pre-commit hooks... then fails with command not found at pre-commit install. Previously, make lint would fail at pre-commit run with a direct command not found error, which was slightly clearer. The net effect is a less informative error message, not new breakage.
  Remediation: Guard with command -v pre-commit >/dev/null 2>&1 before calling pre-commit install, or emit a helpful message like pre-commit not found; run make bootstrap.

• [documentation-missing] Makefile:15 — The new ensure-hooks target is listed in .PHONY but has no corresponding entry in the help target (lines 13–33). Every other .PHONY target currently has a help entry. While ensure-hooks is an internal dependency not meant to be invoked directly, adding a brief entry would maintain the established pattern.
  Remediation: Add a help entry, e.g.: @echo " ensure-hooks - Install pre-commit hooks if not present (runs automatically via lint)"

Previous run

Review

Findings

Low

• [documentation-missing] Makefile:74 — The new ensure-hooks target lacks documentation in the help target. All other .PHONY targets in this Makefile have corresponding help entries (lines 13–33), but ensure-hooks is missing. While it is an internal dependency target not meant to be invoked directly, the established pattern is to document all .PHONY targets.
  Remediation: Add a help entry, e.g.: @echo " ensure-hooks - Install pre-commit hooks if not present"

• [scope-creep] Makefile:74 — The change makes make lint auto-install pre-commit hooks on first run, converting it from a pure check into an idempotent install+check operation. AGENTS.md's guidance to "always stage your changes before running make lint" could benefit from a note about the new auto-install behavior, though the staging advice itself remains valid regardless (it is about source files, not hook state).
  Remediation: Consider updating AGENTS.md to mention that make lint now also installs hooks if missing.

---

Labels: PR modifies build system (Makefile) with a chore-type change.

[ifireball]

This scares me a bit, I don't particularly like the idea of make test installing stuff into my machine, but I'm willing to try anything once....

[ralphbean]

Good catches. I pushed 0b88574 which:

• Uses git rev-parse --git-path hooks to find the actual hooks dir (works in worktrees where .git is a file)
• Skips when core.hooksPath is set — pre-commit install refuses to run in that case anyway
• Skips when CI is set — no reason to install hooks there

Does that cover it?

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 10:44 PM UTC · Completed 10:52 PM UTC
Commit: 0b88574 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:49 PM UTC · Completed 2:59 PM UTC
Commit: 0b88574 · View workflow run →

[fullsend-ai-retro]

Retro: PR #3187 — chore(make): auto-install pre-commit hooks on lint

Timeline

1. Jul 6 22:00 — Ralph Bean opens PR with initial commit (a3b8418), adding ensure-hooks Makefile target. PR body notes it was generated with Claude Code.
2. Jul 6 22:05 — Qodo bot catches a correctness bug: the hardcoded .git/hooks/pre-commit path doesn't work in git worktrees (where .git is a file, not a directory).
3. Jul 6 22:10 — fullsend review agent approves (run 28826240959) with two low-severity findings: missing help entry for ensure-hooks, and scope-creep (AGENTS.md not updated). The agent did not catch the worktree bug that Qodo flagged 5 minutes earlier.
4. Jul 6 22:31 — Human reviewer waynesun09 confirms the worktree bug with detailed local testing, noting .git is a file in worktrees and pre-commit install runs on every make lint as a result.
5. Jul 7 12:14 — Human reviewer ifireball approves with mild concern about make lint installing things.
6. Jul 7 22:43 — Ralph pushes fix commit (0b88574) using git rev-parse --git-path hooks and adding core.hooksPath/$CI guards.
7. Jul 7 22:44 — Second review run (28903840623) starts, produces valid approve review, but the workflow step shows ❌ Failure status.
8. Jul 8 12:06 — Human reviewer rh-hemartin approves.
9. Jul 8 14:47 — PR merged.

Key Findings

Review quality gap: The review agent approved code with a real correctness bug (hardcoded .git/hooks/ path). Qodo caught it first, and a human confirmed it. This caused an extra iteration (fix commit + re-review). This is additional evidence for existing issue #1375 (Improve review agent coverage of shell script correctness bugs).

Second review failure: The re-review run shows ❌ Failure despite producing a valid approve verdict. This is additional evidence for existing issue #3548 (Review workflow shows Failure status despite valid output).

Unaddressed finding: The review agent's scope-creep finding suggesting AGENTS.md be updated to mention make lint now auto-installs hooks was valid but was not addressed before merge. This is minor.

Proposals filed

• Evidence for #1375: review agent missed git worktree path bug caught by Qodo and human (in fullsend-ai/fullsend)
• Evidence for #3548: review workflow failure despite valid approve output on PR #3187 (in fullsend-ai/fullsend)