Skip to content

ci: run fallow audit in lefthook pre-commit#948

Merged
jrusso1020 merged 1 commit into
mainfrom
ci/fallow-lefthook
May 19, 2026
Merged

ci: run fallow audit in lefthook pre-commit#948
jrusso1020 merged 1 commit into
mainfrom
ci/fallow-lefthook

Conversation

@jrusso1020
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 commented May 19, 2026

What

Adds fallow audit to the lefthook pre-commit hook so the same check that gates PRs in CI also runs locally before the commit is created.

Why

Without this, the CI fallow audit (added in #942) catches new findings only on push — a ~5-minute round-trip per attempted fix. Pre-commit gives the feedback before the push (~1s locally with bunx).

How

Adds fallow@^2.75.0 as a root devDep so the binary resolves from node_modules instead of being re-downloaded by npx on every commit. New lefthook block in the existing parallel: true group:

fallow:
  glob: "packages/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}"
  run: bunx fallow audit --base origin/main --fail-on-issues
  • --base origin/main matches the CI gate exactly (originally --base HEAD, switched per Vance's review — diffing against the last commit lets the hook approve a branch that CI rejects because CI audits the cumulative diff)
  • --fail-on-issues is required for non-zero exit (audit normally exits 0 even with findings)
  • Default --gate new-only matches CI behavior — inherited findings don't block commits
  • Glob scopes to package source files; doc/config edits skip the hook
  • bunx (not npx -y) resolves the pinned devDep, no per-commit fetch

Test plan

  • Hook fires on packages/** edits — measured ~0.8s with bunx, parallel with existing lint/format/typecheck so wall-clock unchanged (typecheck ~11s is the long pole)
  • Hook correctly skips for non-package edits (README, top-level configs)
  • Hook passes when only inherited findings exist
  • Hook fails when a new finding is introduced (same gate as CI)

Known sharp edges (documented for follow-up, not fixed here)

  • Working-tree vs staged-index: fallow audit --base origin/main audits the working tree, not just the staged subset. So unstaged WIP in packages/** is part of the diff. If that surprises you, stash before committing. Fallow doesn't expose a --staged flag today; if it becomes a pattern of false positives, can wire git stash + git stash pop around the hook.

Notes

Pinned fallow@^2.75.0 matches the CI workflow pin. Bumps land via a deliberate PR that updates both package.json and the workflow together — the lockfile is the source of truth for both.

Copy link
Copy Markdown
Collaborator Author

jrusso1020 commented May 19, 2026

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid +26/-0 that closes the CI-feedback-loop gap from #942 — local fallow audit so devs see findings in ~5s instead of 5min.

Calibrated strengths

  • lefthook.yml:15-17 comment block explains the --gate new-only semantics inline — future readers (and AI agents) won't have to dig into the CI PR history to know what this gates. Good.
  • Adding fallow@^2.75.0 as a root devDep (package.json:43) instead of relying on npx -y fallow@2.75.0 like CI does is the right call: removes the per-commit npx resolution latency and lets the version float through the lockfile rather than via inline-string drift. Pairs cleanly with bunx fallow in the hook.
  • Glob packages/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs} (line 18) correctly matches fallow's own corpus in .fallowrc.jsonc — non-package edits skip the hook entirely, so doc / config / workflow commits stay fast.

Findings

important — pre-commit/CI parity divergence (--base HEAD vs --base origin/main). The hook diffs against the last commit; CI diffs against the branch's merge base with main. This is not the same gate. Worked example:

  • dev commits A (introduces a fallow finding), then B (partial fix), then C (full fix).
  • pre-commit on C (--base HEAD = B) passes — only C's delta is clean.
  • CI on the branch (--base origin/main) audits A+B+C as a single diff and still sees the finding from A surviving into the working tree.

This is the classic dev-passes / CI-fails surprise the team is trying to avoid by adding the hook in the first place. Pre-commit can never perfectly replicate a PR-scope audit (you'd need --base origin/main locally, which is a different UX), but the PR body should call this out explicitly so devs know that a clean local hook is necessary-but-not-sufficient — and that if CI flags a finding the local hook didn't, the right move is npx -y fallow@2.75.0 audit --base origin/main --fail-on-issues to reproduce.

important — --base HEAD sees the working tree, not the staged index. fallow audit --base HEAD compares HEAD to the worktree, which includes both staged AND unstaged changes. Two failure modes:

  1. dev has unrelated unstaged WIP in packages/**, runs git commit on a clean staged file → hook fails on the unstaged WIP, blocking an otherwise-clean commit.
  2. dev has unstaged "fix" for a finding alongside a staged "regression" → hook passes because the unstaged fix masks the staged regression; the next push reintroduces the finding.

Neither is hypothetical with a parallel-edit dev workflow. The lefthook-correct shape is {staged_files} interpolation or pre-staging via stage_fixed, but fallow's --base model doesn't trivially accept a file list — it wants a tree-ish. Worth a follow-up: either (a) document the "stash your unstaged work before committing" expectation in CONTRIBUTING / PR body, or (b) check whether fallow has an --only-staged or accepts an explicit file glob that we can wire to {staged_files}. Don't block on this, but call it out so the team can pattern-match it when the first false-positive / false-negative report comes in.

nit — PR body / diff drift on the invocation. PR body's "How" section says npx -y fallow@2.75.0 audit --base HEAD --fail-on-issues; the actual lefthook.yml diff uses bunx fallow audit --base HEAD --fail-on-issues. The diff is better (uses the lockfile-pinned version via the new devDep). Update the body so future readers / spelunking AIs aren't misled.

nit — fallow devDep adds 8 optional platform binaries (bun.lock:520-535). Real disk + install-time cost on every dev box and every CI runner (most of which don't need the hook). Acceptable tradeoff for the local-feedback win, but if the team ever feels the install bloat, an alternative is leaving it on npx -y fallow@2.75.0 and accepting the per-commit cold-start (npm cache makes the second commit fast). Not a blocker.

nit — bypass discoverability. When the hook fails, devs will reach for git commit --no-verify. That's fine for a static-analysis gate (CI is the real wall) but the hook output should ideally remind them what they can do — fallow's audit output presumably prints findings, but a pre-commit: tags: field or a one-line "see #942 for context on this check" wrapper would shorten the time-to-context for newcomers. Skip if it'd just be noise.

Verdict: APPROVE
Reasoning: The parity-divergence and worktree-vs-index points are real but neither is a correctness bug — they're shape considerations that a dev will hit, recognize, and work around within a week of adoption. The PR delivers the stated goal (5s local vs 5min CI feedback) cleanly, the CI is green at 79b72c6f, and the design (root devDep + parallel hook + glob-scoped + new-only gate) is the right one. Worth merging and iterating on the parity question as a follow-up if it bites.

Review by Vai

Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — clean, minimal integration.

Verified:

  • bunx fallow audit --base HEAD --fail-on-issues is correct for pre-commit context
  • parallel: true means fallow (~5s) runs alongside typecheck (~11s) — zero added wall-clock time
  • Glob packages/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs} covers all JS/TS extensions including ESM/CJS variants
  • Native Rust binary via platform-specific optionalDependencies — fast by nature
  • ^2.75.0 semver range + lockfile pin is fine

Nit: PR description says npx -y fallow@2.75.0 but diff correctly uses bunx fallow — minor body drift.

Worth noting (non-blocking): --base HEAD compares against last commit, not staged index. If fallow supports a --staged flag, that would be more precise for pre-commit. Current behavior is acceptable since it catches the common case.

@jrusso1020 jrusso1020 force-pushed the ci/fallow-lefthook branch from 79b72c6 to 9b57050 Compare May 19, 2026 02:19
@jrusso1020
Copy link
Copy Markdown
Collaborator Author

Thanks @vanceingalls @miguel-heygen — addressed:

Fixed

  • 🔧 Important Initial repo setup #1 (CI/hook gate parity): switched lefthook from --base HEAD to --base origin/main. Hook now audits the same diff CI does. Resolved the "commit C passes but branch fails" worked-example scenario.
  • 📝 Nit (body drift): PR description updated to say bunx fallow (matches the actual diff).

Documented (not fixed — no clean code fix)

  • 📝 Important initial code #2 (working-tree vs staged-index): added a "Known sharp edges" section to the PR body. Fallow doesn't expose a --staged flag, and wiring git stash / pop around the hook risks losing work on the rare crash. Acceptable trade-off; revisit if false positives become a pattern.

Skipped (acceptable trade-offs)

  • Optional-binary lockfile bloat — confirmed real but the local-feedback win outweighs it
  • --no-verify bypass discoverability — fallow's audit output already prints findings; extra wrapper would just be noise

Force-pushed @ 9b57050c.

Mirrors the same `fallow audit --base ... --fail-on-issues` check that
runs in CI, but locally against HEAD so issues surface at commit time
instead of after the push round-trip.

Scoped to `packages/**` source files via the glob — non-code edits
(README, docs, top-level configs) skip the hook entirely.

Measured locally: ~5s in parallel with the existing lint/format/typecheck
checks. Doesn't extend wall-clock time because typecheck (~11s) is the
long pole, and lefthook runs commands in parallel.

The default `--gate new-only` means inherited findings don't block the
commit — same gate behavior as CI, so local pre-commit and PR audit
agree.
@jrusso1020 jrusso1020 force-pushed the ci/fallow-lefthook branch from 9b57050 to a86c3c1 Compare May 19, 2026 02:32
@jrusso1020 jrusso1020 merged commit b6b7bcb into main May 19, 2026
39 checks passed
@jrusso1020 jrusso1020 deleted the ci/fallow-lefthook branch May 19, 2026 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants