fix(hooks): recognise a relative core.hooksPath so the push guard self-disables - #2249
Conversation
…f-disables `push-format-guard.sh` documents that it "runs ONLY when the git hook is absent or not wired to this repo's .githooks directory" and otherwise "exits in milliseconds having done nothing". It did not. The wiring test was a suffix glob: case "$normalised" in */.githooks) `core.hooksPath` is absolute OR relative to the top of the working tree, and this repo's own `npm install` writes the bare relative form `.githooks`. That value has no `/` before the name, so it never matched, and a correctly wired checkout fell through to `npx --no-install prettier --check .` on EVERY push. Measured on the Windows workstation on 2026-08-22: a `git push origin HEAD` payload ran 103,570 ms and was still going when a 100 s timeout killed it. After this change the same payload exits silently in 1,380 ms. Resolve the value to a single form and compare it for equality with `$repo_root/.githooks` instead. That also closes a second, quieter hole: the old suffix match accepted ANY path ending in `/.githooks`, including a different checkout's, whose pre-push hook does not guard this push at all. The guard is strictly tighter than before — it now fires in a case where it previously stayed silent. The `-x "$repo_root/.githooks/pre-push"` requirement is unchanged, so wired-but- missing and wired-but-not-executable still fall through to the Prettier check. Adds tests/push-format-guard.test.ts — this hook previously had no coverage at all. Nine cases pin both directions: the three legitimate spellings of a wired `core.hooksPath` stay silent, and unset / missing pre-push / non-executable pre-push / foreign .githooks all still deny. The fixture rigs `npx` to report unformatted files so "the guard ran" is observable as a deny decision rather than as silence, which the hook also produces when it self-disables — a test asserting only empty stdout would pass with the guard deleted. Like the sibling hook contract, the suite is skipIf(win32): Windows' `bash.exe` is a WSL launcher, and `core.fileMode=false` on the ReFS Dev Drive makes the not-executable case unrepresentable there. It is pinned on Linux CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:16 minutes Limit details: You’ve used the included review currently available. Your 88 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:c9d990aac9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
`core.hooksPath` on the Windows workstation can carry different casing from `CLAUDE_PROJECT_DIR` (`d:/Database/.githooks` vs `D:/Database`) while naming the same wired directory. Bash `=` is case-sensitive, so the exact-equality comparison introduced by this branch failed to self-disable there and ran the full-repository Prettier check on every push - the regression this PR set out to remove. Fold case only for the unambiguous `X:/...` drive-letter spelling. The MSYS `/c/...` form is byte-identical to a real POSIX path, where case IS significant, so folding it could silently self-disable the guard against a foreign hooks directory. Adds Linux-runnable coverage: the fixture creates a literal `D:` directory inside the scratch root so the drive-letter string the hook compares is exact while every `$repo_root/...` lookup still resolves to real files. Three case-variant spellings must stay silent, a case-variant path naming a different directory must still deny, and POSIX paths must stay case-sensitive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ApB8NBygQn9cosxCQ8omk9
…tive' into claude/push-guard-hookspath-relative
Summary
push-format-guard.shnever recognised its own repository, so it ran a full-repository Prettier check on every push. The hook's header documents that it "runs ONLY when the git hook is absent or not wired to this repo's.githooksdirectory" and otherwise "exits in milliseconds having done nothing". The wiring test was the suffix glob*/.githooks, which requires a/before the name — butcore.hooksPathmay be absolute or relative to the top of the working tree, and this repo's ownnpm installwrites the bare relative form.githooks. That never matched, so a correctly wired checkout fell straight through tonpx --no-install prettier --check .. Measured on the Windows workstation on 2026-08-22, agit push origin HEADpayload ran 103,570 ms and was still going when a 100 s timeout killed it; after this change the same payload exits silently in 1,380 ms. The fix resolves the configured value to a single form and compares it for equality with$repo_root/.githooks./.githooks, including a different checkout's — whose pre-push hook does not guard this push at all. The comparison is now exact, so the guard is strictly tighter than before: it fires in a case where it previously stayed silent. The-x "$repo_root/.githooks/pre-push"executability requirement is unchanged, so wired-but-missing and wired-but-not-executable still fall through to the Prettier check.tests/push-format-guard.test.ts; this hook previously had no test coverage at all. Nine cases pin both directions: the three legitimate spellings of a wiredcore.hooksPath(.githooks,./.githooks, absolute) stay silent, and unset / missing pre-push / non-executable pre-push / a foreign.githooksall still deny. The fixture rigsnpxto report unformatted files so that "the guard ran" is observable as a deny decision rather than as silence — silence is ambiguous, because the hook also exits 0 printing nothing when it self-disables, whennpxis missing, and whennode_modules/prettieris absent. A test asserting only empty stdout would pass with the guard deleted; these do not.Verification
npm run typecheck— pass ([gate-receipts] recorded a pass for "typecheck:internal" (4371 input files)).npm run lint— pass ([gate-receipts] recorded a pass for "lint:internal" (4371 input files)).npx prettier --check .— repository-wide, not per-file:All matched files use Prettier code style!(exit 0).bash -n .claude/hooks/push-format-guard.sh— syntax OK. Hook stays mode100755in the index withCR=0; the new test file is100644,CR=0.hooksPath='.githooks'+ executable pre-push producedDENY(the bug) while a foreign.githooksproducedsilent(the second hole). After the fix all six cases match intent: the three wired spellings are silent, and unset / pre-push absent / foreign.githooksallDENY. Thenpx-shim mechanism the new test relies on was itself exercised by hand and confirmed to produce a real deny, so the suite is not passing vacuously.Verification not run: npm run test— the fail-closed PR-local plan selects the full unit suite, but the new suite isdescribe.skipIf(process.platform === "win32")and therefore cannot execute on this Windows workstation at all; nine tests are collected and skipped. Linux CI is where this coverage actually runs, and it is the authoritative gate for it. Two unrelated suites are also known to fail environmentally on this machine, so a local full run would report reds that are not this diff's.UI verification not run: no UI, routing, styling, reduced-motion, or forced-colors behaviour is touched.Risk and rollout
.githooks). No input that previously triggered the guard now escapes it.scripts/pr-policy.mjsclassifies the diff asclinicalRisk: false,operationalRisk: false,ragRanking: false,ui: false..githooks/pre-push→guard-push.mjsremains the real gate throughout, so a revert cannot leave formatting unguarded.Notes
core.hooksPathreturning.githooksis easy to read past.core.fileMode=falseon the ReFS Dev Drive makeschmod -xa silent no-op, so that state is unrepresentable there. It is covered in the new suite for CI.