chore: add tracked fallow-stop.sh Stop hook (prerequisite for #15) - #40
Conversation
Commit the fallow audit Stop hook directly into the repo so any contributor can wire it with no external dependency on the owner's private ~/tools/fallow-hook. The hook runs fallow audit --changed-since over changed TS/JS files and writes a JSON report under .fallow/. - Informational only: exits 0 on every path (no fallow binary, nothing changed, audit error, broken symlink on Windows). CI (#15) is the gate. - Honors FALLOW_DISABLE=1 and FALLOW_BASE_REF; base ref falls back main -> master -> origin/HEAD. One-time install hint when fallow absent. - .fallow/ added to .gitignore (runtime output); the script stays tracked. - .gitattributes pins *.sh to LF so a fresh clone on Windows (autocrlf=true) gets a runnable shebang instead of CRLF corruption. Closes #36 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
s-annam
left a comment
There was a problem hiding this comment.
PR Review: chore: add tracked fallow-stop.sh Stop hook (prerequisite for #15)
Summary
Clean, defensive, well-documented prerequisite. Commits an informational fallow audit Stop hook in-repo, removing the private ~/tools/fallow-hook dependency. Exits 0 on every path. All 8 acceptance criteria in #36 are met. Tooling-only (no src/), so no test suites apply.
Acceptance criteria — verified
| Criterion | Status |
|---|---|
Tracked + executable (100755) |
✅ diff shows new file mode 100755 |
Script not in .gitignore |
✅ only .fallow/ added |
.fallow/ in .gitignore |
✅ |
Exit 0 + one-time hint when fallow missing |
✅ marker in .fallow/ |
| Exit 0 silent when no TS/JS changed | ✅ gate runs before binary check |
FALLOW_DISABLE=1 immediate no-op |
✅ first line after set |
| Fresh-clone wiring documented | ✅ in header block |
| Windows note | ✅ + bonus .gitattributes LF pin |
Verified locally
bash -nclean on the fetched script.- Embedded python summary parser AST-parses and runs correctly on all 3 documented schema shapes: dict summary (
dead:3 | dupes:2 | complexity:5 | circular:1), findings-list (dead:2 | dupes:1 | complexity:0 | circular:1), and garbage input (graceful all-0s). No crash path — missingpython3falls back to the?summary. set -uo pipefailwithoutset -eis correct here: every&& exit 0and unchecked pipeline still reachesexit 0.
Highlights
.gitattributes *.sh text eol=lfis a genuine catch — protects the shebang from CRLF corruption on Windowscore.autocrlf=true, and retroactively hardens the existingscripts/hooks/*.sh.- Install hint is gated behind the "files changed" check, so it never nags on no-op sessions.
- Schema-tolerant parser with a documented provisional-status rationale tied to #15 Step 1.
Minor notes (non-blocking, optional)
- [Nit] Hook directory split: existing Claude hooks live at
scripts/hooks/(registered in.claude/settings.json); this lands at.claude/hooks/. The path is mandated by #36, so it's correct — just flagging that two hook locations now coexist. - [Suggestion] The gate computes its own
merge-base...HEAD+ working-tree diff, but the audit relies on fallow's--changed-since. The two file sets can diverge slightly. Fine as a conservative gate; consider passing an explicit file list once the fallow JSON schema is pinned in #15.
Verdict
Action: APPROVE
Rationale: All acceptance criteria met, functionality verified locally, zero blocking items. The notes are optional and don't gate merge.
Pass the exact changed-file list the gate computed to `fallow audit` instead of relying on fallow re-deriving its own set via --changed-since, which can diverge (different diff base, deletions, non-TS/JS noise). Falls back to --changed-since if this fallow build does not accept a positional file list, so behavior never regresses below the prior path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the thorough review @s-annam! Addressed suggestion #2 (explicit file list) in The audit now runs against the exact set the gate computed ( Left note #1 (the |
s-annam
left a comment
There was a problem hiding this comment.
PR Review: chore: add tracked fallow-stop.sh Stop hook (prerequisite for #15)
Summary
Re-review. The prior APPROVE (s-annam) was dismissed when 023b0e3a ("audit the gate-selected file set (#36 review)") landed — a commit that implements the prior review's only open Suggestion: pass an explicit changed-file list rather than relying on fallow's --changed-since. The fix is correct and defensive (try explicit -- <files>, fall back to --changed-since if the report is empty). Still tooling-only, still exits 0 on every path.
Acceptance criteria (#36) — all 8 still met
| Criterion | Status |
|---|---|
Tracked + executable (100755) |
✅ new file mode 100755 |
Script not in .gitignore |
✅ only .fallow/ added |
.fallow/ in .gitignore |
✅ |
Exit 0 + one-time hint when fallow missing |
✅ |
| Exit 0 silent when no TS/JS changed | ✅ gate before binary check |
FALLOW_DISABLE=1 immediate no-op |
✅ |
| Fresh-clone wiring documented | ✅ header block |
| Windows note | ✅ + bonus .gitattributes LF pin |
Path .claude/hooks/ is mandated by #36 — the two-hook-location split vs existing scripts/hooks/ is per-spec, not drift.
Verified
bash -nclean on the fetched script.- No
src/touched; no.ts/.tsx/.jschanged → no test suites apply. - Explicit-list →
--changed-sincefallback logic reviewed; the[[ -s "$report" ]]guard correctly drives the fallback.
Highlights
- The
023b0e3afix makes the gate authoritative over the audited file set, with a documented provisional rationale tied to #15. .gitattributes *.sh text eol=lfretroactively hardens the existingscripts/hooks/*.shagainst CRLF shebang corruption.
Minor note (non-blocking)
- [Suggestion]
mapfile(line 96) is bash 4+; stock macOS ships bash 3.2. Only reached oncefallowis installed (post-#15), andenv bashresolves to Homebrew bash 5 on the owner's box — so narrow. Given how hard the script bends for portability elsewhere (Windows git-bash, CRLF pin), awhile IFS= read -rloop would close the last gap. Safe to defer to #15. - The garbage-stdout edge (an unsupported
audit -- <files>printing usage to stdout → non-empty report → fallback skipped) is already flagged in the code's own comment as provisional-until-#15. Fine as-is.
Verdict
Action: APPROVE
Rationale: All acceptance criteria met, prior review's suggestion now implemented, zero blocking items. The remaining note is optional and tied to #15.
| # gate authoritative. Until fallow's CLI is pinned (#15) we don't assume it | ||
| # accepts positional files: try the explicit form, and if that yields no | ||
| # usable report, fall back to --changed-since (the prior behavior). | ||
| mapfile -t changed_files <<< "$changed" |
There was a problem hiding this comment.
[Suggestion]: mapfile is bash 4+; stock macOS ships bash 3.2, so a contributor without Homebrew bash whose env bash resolves to /usr/bin/bash would hit mapfile: command not found here. Only reached once fallow is installed (post-#15), so narrow — but the script otherwise bends hard for portability (Windows git-bash, the .gitattributes CRLF pin). A while IFS= read -r line; do changed_files+=("$line"); done <<< "$changed" loop closes the gap. Safe to defer to #15.
chore: add tracked fallow-stop.sh Stop hook (prerequisite for #15)
chore: add tracked fallow-stop.sh Stop hook (prerequisite for #15)
chore: add tracked fallow-stop.sh Stop hook (prerequisite for #15)
Summary
Commits the fallow audit Stop hook directly into the repo at
.claude/hooks/fallow-stop.shso any contributor can wire it with no external dependency on the owner's private~/tools/fallow-hook(which is macOS-only and inaccessible to others). The hook runsfallow audit --changed-since <base>over changed TS/JS files and drops a JSON report under.fallow/.This unblocks #15 Step 2: once merged, wiring becomes a one-line
Stophook entry in.claude/settings.local.jsonpointing at the in-repo script.Design — informational, never gates a session. The hook exits
0on every path: nofallowbinary, nothing changed vs base, audit error, even a failed symlink on Windows. The CI SARIF gate in #15 is the enforcement point, not this hook.FALLOW_DISABLE=1(immediate no-op) andFALLOW_BASE_REF; base ref falls backmain→master→origin/HEAD.fallowis absent.🌾 fallow audit (vs main): dead:N | dupes:N | complexity:N | circular:Nsummary to stderr..fallow/added to.gitignore(runtime output); the script itself stays tracked + executable (100755)..gitattributespins*.shto LF so a fresh clone on Windows (core.autocrlf=true) gets a runnable shebang instead of CRLF corruption — also hardens the existingscripts/hooks/*.sh.Closes #36
Test plan
bash -nsyntax check passesfallowmissing → one-time hint + exit 0 (no repeat on 2nd run)FALLOW_DISABLE=1→ immediate no-op exit 0100755and stored LF in index;.fallow/ignored, script notnpm run typecheckclean (nosrc/changes — tooling only)npm run testgreen