fix(hooks): scope pre-commit build check to target repo, fix stale symlinks#2175
Conversation
The PreToolUse hook matched any Bash command containing "git commit" and always ran this repo's bun build/lint/typecheck from its own process cwd, even when the command targeted a different repo (e.g. a sibling worktree reached via a leading `cd`). Resolve the real target directory from the command text first, and skip silently for any repo that isn't this one.
The format hook's auto-restage (`git add {staged_files}`) exits non-zero
for any staged file that lives under a gitignored directory (e.g.
.claude/settings.json, tracked despite .claude/ being ignored for worktree
noise), silently aborting the whole commit even though the file was
already correctly staged.
…stall packages/producer's tracked node_modules symlinks still pointed at puppeteer@24.43.1, which no longer exists after a fresh install resolves package.json's ^25.2.1 range to 25.3.0 — breaking the producer TypeScript build with a missing puppeteer-core module error.
packages/producer/node_modules was accidentally swept into a prior commit despite the repo-wide node_modules/ gitignore rule, and its ~30 tracked symlinks silently drift from whatever bun install actually resolves — the exact cause of the stale puppeteer symlinks fixed earlier in this branch. CI always runs bun install --frozen-lockfile before building, so nothing depends on these being pre-committed.
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: 🟢 LGTM
Three tidy hygiene fixes, each of which resolves a real, verified footgun. Elementary in scale, but each dispatches a genuine bug — the sort of housekeeping that keeps a repository honest.
Fix 1 — .claude/settings.json hook target-dir scoping
Traced the new script end-to-end. The cd regex ^\s*cd\s+('([^']+)'|(\S+))\s*(?:&&|;|\n|$) handles the common shapes cleanly (cd /abs, cd 'single quoted', chained &&/;), and — crucially — every off-nominal input fails open to process.cwd():
- Double-quoted paths (
cd "path with spaces"): the\S+alt captures only"path, then the terminator\s*(?:&&|;|\n|$)fails against the followingwith— the regex misses, fallback engages. Non-space-containing quoted paths (cd "foo") capture the literal quotes,path.resolveyields/cwd/"foo",existsSyncfalse → fallback. A very minor cosmetic gap (a"…"alternate mirror of the'…'branch would be tidier), but no correctness hazard: worst case the check runs from the hook process cwd — the exact status quo pre-PR. Not worth blocking on. cd ~otheruser/foo: the~prefix tripsdir.slice(1), yieldingotheruser/foo, resolved beneath cwd,existsSyncfalse → fallback. Uncommon shape, same benign outcome.cd -(OLDPWD) and nocdat all:\S+matches-but resolves to a non-existent path → fallback; baregit commitskips the regex entirely → fallback. Both correct.- Chained
cd repo1 && cd repo2 && git commit: only the leadingcdis captured. Uncommon in practice; if it bites, the check runs againstrepo1and either short-circuits at thepkg.nameguard or produces a slightly-off report. Also fail-open. - Command-substitution injection (
cd $(evil) && git commit): the captured string is passed only topath.resolve+fs.existsSync+execSync(..., { cwd }).execSyncwithcwd:does not shell-expand the directory — the literal$(evil)bytes reachgit rev-parse,existsSyncfails, fallback. No injection surface.
The pkg.name === 'hyperframes-monorepo' guard I verified against the head SHA's root package.json — matches. Sibling repos with different names now skip silently, as advertised.
Fix 2 — lefthook.yml git add {staged_files} → git add -f {staged_files}
Confirmed the diff touches only the format step (line 12). Swept the rest of the file: the only other git add is on line 19 (skills-manifest:), which stages skills-manifest.json — a repo-root file not matched by any .gitignore entry (skills-lock.json at line 111 is a distinct file). No latent twin bug.
The -f is safe by construction: {staged_files} is the set lefthook already sees as staged, i.e. tracked or newly-added-and-indexed. git add -f on such a set only bypasses the ignore check that was falsely aborting the commit when .claude/settings.json — an ignored-but-tracked file — was staged. No new files smuggled in.
Fix 3 — untrack 33 producer node_modules symlinks
Verified:
- Repo-root
.gitignoreline 2:node_modules/— the untrack matches intent. bun install --frozen-lockfileappears 14× across.github/workflows/ci.yml(lines 85, 104, 138, 194, 212, 231, 307, 310, 322, 344, 362, 483, 560, plus the--ignore-scriptsvariants). CI regeneratespackages/producer/node_modules/before every build.- The two workspace-internal symlinks (
@hyperframes/core,@hyperframes/engine) are standard bun workspace resolutions fromworkspace:^deps inpackages/producer/package.json— bun rewrites these on install; no pre-commit dependency. - No
preinstall/postinstallinpackages/producer/package.jsonexpects a pre-populatednode_modules/. - The stale-drift claim in the PR body checks out on the puppeteer symlink: producer declares
puppeteer: ^25.2.1, and a committed@24.43.1symlink would resolve wrong on a fresh install. Removing the source of truth from git and lettingbun installbe authoritative is the correct move.
Summary
All three fixes are properly scoped, verifiable, and leave the system in a strictly better state. The regex could grow a double-quoted alternate for symmetry, but every failure mode I could construct falls back to the pre-PR behaviour rather than misfiring — the fix is defensively coded. Ship it.
— R1 by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 1fff376a.
Small tooling fix, three coherent changes. Reproduced each: .claude/settings.json is ls-files-tracked despite .claude/ being in .gitignore, so git add .claude/settings.json on a modified copy exits 1 with the "ignored by .gitignore" warning (git add -f is the right unblock, and safe because {staged_files} is by definition already-tracked); the ~30 packages/producer/node_modules symlinks are correctly untracked (CI's bun install --frozen-lockfile restores them from the lockfile every run, no consumer of the pre-committed paths); and the hook now scopes to pkg.name === "hyperframes-monorepo" after resolving the target dir from a leading cd <path>.
Two non-blocking observations worth surfacing:
-
The old regex never matched anything.
/git\\s+commit\\b/in the pre-PR JSON, once JSON-decoded and evaluated as a JS regex literal, becomesgit+ literal backslash + one-or-mores+commit+ literal backslash +b— verified vianode -e 'console.log(/git\\s+commit\\b/.test("git commit -m x"))'→false. So the pre-PR hook was silently no-op'ing on every commit; theon('end')callback fired, failed the match, and exited 0. This PR is really doing two things: fixing the escape (/git\s+commit\b/now actually matches) and scoping it. Thepkg.namegate is what makes the newly-active hook safe — worth stating that in the summary so future-readers of the change history don't think this was purely a scope-narrowing fix. -
git rev-parseexecSync leaks its stderr. Thetry { execSync('git rev-parse --show-toplevel', { cwd: targetDir, encoding: 'utf8' }) } catch { process.exit(0); }block inherits stderr by default, so every commit whose target dir isn't a git checkout now spewsfatal: not a git repository (or any of the parent directories): .gitto the user's terminal before the hook cleanly exits. Addstdio: 'pipe'to theexecSyncoptions (matching what thebun run build/lint/typechecksteps already do) to silence it. Not a functional bug — just noise the pre-PR dead-hook didn't produce.
Everything else lines up cleanly: tilde expansion, single-quoted paths, and non-existent cd targets all fall through to process.cwd(), and CI is green including the required CLI smoke/Test/Build matrix. LGTM from my side — leaving as a comment.
Summary
git commit, then always ran this repo'sbun build/lint/typecheckfrom its own process cwd. A command that targeted a different repo (e.g.cd ../other-repo && git commit ...) still triggered this repo's build, misreporting unrelated repos' commits as failing. The hook now resolves the real target directory from the command's leadingcd, and skips silently for anything that isn't this repo.lefthook.yml'sformatstep re-stages formatted files viagit add {staged_files}, which exits non-zero for any already-tracked file living under a gitignored directory (.claude/settings.jsonis tracked despite.claude/being ignored for worktree noise) — silently aborting the whole commit even though the file was staged correctly. Switched togit add -f, which is safe here since every path in{staged_files}is already staged/tracked by definition.packages/producer/node_moduleshad ~30 symlinks accidentally tracked in git despite the repo-widenode_modules/gitignore rule. These silently drift from whateverbun installactually resolves, which is exactly what broke the producer TypeScript build here (stalepuppeteer@24.43.1symlinks after a fresh install resolved^25.2.1to25.3.0). CI always runsbun install --frozen-lockfilebefore building, so nothing depends on these being pre-committed — untracked them entirely rather than just repointing the ones that happened to be stale.Validation
sh -cwith realisticPreToolUsepayloads: a cross-repocd-prefixed commit now skips silently, a same-repo commit still runs the real checks, and a non-git command still no-ops.bun run buildpasses cleanly end-to-end on this branch, both before and after untrackingpackages/producer/node_modules(previously failed on@hyperframes/producerdue to the stale symlinks).lefthook.ymlbug directly (git addon the ignored-but-tracked path exits 1;git add -fexits 0) and confirmed a fulllefthook run pre-commitnow passes.