fix(hooks): run the pre-push check and warn when hooks are missing (#3493) - #3495
Conversation
…3493) - lib/hooks/install.js copied the branch-name hook into .git/hooks, which Git ignores because core.hooksPath is .husky/_; the committed .hook-version also made it skip installing. Remove both and add .husky/pre-push, so the check runs through Husky. - scripts/check-git-hooks.mjs (npm run hooks:check, and pretest) warns when core.hooksPath is missing, e.g. after npm ci --ignore-scripts. - The pre-push hook claimed forced pushes bypass it; Git never passes push flags to the hook. Point to --no-verify instead. - Docs: the pre-push hook validates branch names; CI runs the tests.
📋 Changelog Quality Validation
Status✅ Validation PASSED - No new failures introduced by this PR. No action required. |
PR Template RoutingBranch Type: This PR was automatically routed based on the branch naming strategy. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: lightspeedwp/.github/.coderabbit.yml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pre-push hook now runs branch-name validation. New scripts detect missing Husky hooks and warn when hooks are inactive. Package scripts, tests, the changelog, and the hook guide reflect these changes. ChangesHusky hook checks
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: 🟡 Moderate · up to Some developers can miss the warning when local hooks are inactive, and the guide incorrectly describes what runs before a push. Resolve the hook-detection gaps before merging; CI remains the authoritative test gate. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new pre-push hook restores a local branch-name check, but the new status check can say hooks are present when Git is using a different hook directory. This creates false assurance about local checks; no server-side bypass has been established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (5 skipped: 5 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
|
@coderabbitai full review |
|
@Mergifyio update |
☑️ Nothing to do, the required conditions are not metDetails
|
|
@Mergifyio refresh |
✅ Pull request refreshed |
|
@Mergifyio update |
☑️ Nothing to do, the required conditions are not metDetails
|
github-actions
Bot
added
type:bug
area:documentation
area:scripts
lang:js
area:testing
labels
|
/agentic_review |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo found that the restored pre-push hook read the branch with `git rev-parse --abbrev-ref HEAD`, which is not the branch being pushed. Git feeds a pre-push hook one ref-update record per ref on stdin, so an explicit `git push other-branch` escaped validation whenever the current branch was valid, and an invalid current branch could block an unrelated push or a ref deletion. The hook now parses the records and validates every refs/heads entry in them, skipping deletions and non-branch refs such as tags, and reporting how many of how many were rejected. With no records on stdin it falls back to HEAD, so the documented `node lib/hooks/pre-push` invocation still works outside a push. Also corrects DEVELOPMENT.md, which claimed the pre-push hook runs the full test suite. It validates branch names; the full suite runs in CI, which docs/HUSKY_PRECOMMITS.md already stated correctly. Adds lib/hooks/__tests__/pre-push.test.js, because the hook had no behavioural coverage: the existing check-git-hooks test only covers hook installation. Seven cases pin the stdin contract, and reverting the hook to the HEAD-only logic fails four of them.
The manual-invocation example still said the hook "validates the current branch name". Since 5b20dfb it validates every branch in the ref-update records git supplies, and only falls back to the checked-out branch when there are none.
Review disposition — Qodo's three findingsRecording here rather than in the threads, because Qodo auto-resolved them on the push and my reply only landed on 1. "Valid release branches cannot be pushed" — not reproduced, believe incorrectThe finding says There is no second parser. import { validateBranchName, formatErrorMessage } from '../validate-branch-name.js';so the hook and the canonical validator are the same code. Tested directly against
Dotted release branches are accepted nowhere in the repository today, by the hook or by CI — which is why If Qodo was comparing against a semver pattern it saw in #3558 or in the spec rather than in the validator, that 2. "Developers are told pushes run tests" — fixed
3. "The wrong branch is checked before push" — fixed,
|
| stdin | Result |
|---|---|
refs/heads/Invalid_Branch |
rejected, exit 1 — previously bypassed |
refs/heads/fix/abc-def |
exit 0 |
(delete) … refs/heads/… |
skipped, exit 0 |
refs/tags/v1.0.0 |
skipped, exit 0 |
| empty | falls back to HEAD |
Test coverage added
lib/hooks/__tests__/pre-push.test.js — seven cases, because the hook had no behavioural coverage: the existing
check-git-hooks test only covers hook installation. I confirmed the coverage is real by reverting the hook to the
HEAD-only logic, which fails four of the seven.
Full suite: 287 suites, 5666 passed, 0 failed (up from 285 — the new file).
Also brought current with develop
Merged origin/develop; the branch is now 0 behind and MERGEABLE. The conflict GitHub previously reported was in
CHANGELOG.md, which now union-merges via the .gitattributes change in #3581, so it resolved without intervention.
Both changelog entries survived.
Qodo's local review caught a second completeness gap in the same area CodeRabbit flagged. FR-008's exception, FR-009's bypass list and the shipped-gate constraint all named only Dependabot and docs-bot, but changelog-unified.yml skips the require-gate job outright when github.actor is imgbot[bot] -- a job-level if: on line 30, not an in-script author check. So an image bot's pull requests are exempt from the changelog requirement too. All three now say so, and distinguish the job-level skip from the in-script author checks, since they happen at different points. Its other finding, on .github/reports/changelog-metrics/20260926.json, is a generated artefact left in the worktree by a test run: the file is untracked and the pull request diff contains no report files, so it is not part of this change. Also merges origin/develop for #3495, which touches none of these files.
…3583) * fix: correct a false claim about the shipped changelog gate Spec 016 FR-009 asserted that the shipped gate exempts a pull request changing only CHANGELOG.md, because its docs-only test accepts any file ending in .md and CHANGELOG.md ends in .md. That claim is wrong, and it reached the specification through my own review of #3500, where I read one line of the gate's require step rather than its control flow. The gate tests changed.includes("CHANGELOG.md") and returns run_validation=true before the docs-only exemption is ever evaluated, so a changelog-only pull request is validated. Verified three ways: the guard appears at character 4246 and the docs-only test at 5368 in the workflow, so the guard is reached first; the full require-gate suite passes 41 of 41; and it already contains "runs validation when the root changelog changed", which asserts exactly this and contradicts the claim, alongside "exempts a nested CHANGELOG.md under docs/ as docs-only", which is the case the .md suffix really does cover. No gate change is needed, so none is made. FR-009 now states the gate's actual order of tests, notes that the ordering is what keeps FR-008 and FR-009 consistent rather than conflicting, distinguishes a root CHANGELOG.md from one nested under docs/, and cites the tests that pin the behaviour. FR-008's cross-reference to a non-existent conflict is removed, the constraint line states the full shipped order, and the same correction is applied to FEEDBACK_RESPONSE.md, which repeated the claim on develop. * docs: qualify the root changelog check with the gate's author skips CodeRabbit is right that FR-008 and the corrected FR-009 were incomplete. The gate skips Dependabot and docs-bot authors before it inspects any file, so those pull requests never reach the changed.includes("CHANGELOG.md") guard and are not validated even when they modify the root CHANGELOG.md. Verified against the workflow: the dependabot skip is at character 430, the docs-bot skip at 931, and the CHANGELOG.md guard at 2304, so the author skips unambiguously come first. FR-008 now carries the exception and FR-009 states that the ordering means the guard applies only to pull requests that reach the file inspection. * docs: list the imgbot gate bypass in spec 016 Qodo's local review caught a second completeness gap in the same area CodeRabbit flagged. FR-008's exception, FR-009's bypass list and the shipped-gate constraint all named only Dependabot and docs-bot, but changelog-unified.yml skips the require-gate job outright when github.actor is imgbot[bot] -- a job-level if: on line 30, not an in-script author check. So an image bot's pull requests are exempt from the changelog requirement too. All three now say so, and distinguish the job-level skip from the in-script author checks, since they happen at different points. Its other finding, on .github/reports/changelog-metrics/20260926.json, is a generated artefact left in the worktree by a test run: the file is untracked and the pull request diff contains no report files, so it is not part of this change. Also merges origin/develop for #3495, which touches none of these files.
1 participant
Bugfix Pull Request
Linked issues
Closes #3493
Context
npm ci --ignore-scripts.Reproduction
npm ci --ignore-scriptsin a fresh worktree. 2)git config core.hooksPathreturns.husky/_. 3)ls .husky/_reports "No such file or directory". 4) Commit or push: no hook runs.Root Cause
lib/hooks/install.jscopied the branch-name hook into.git/hooks. Git ignores that directory becausecore.hooksPathis.husky/_.lib/hooks/.hook-versionis committed, so on a fresh clone the installer decided the hook was already installed and skipped it..husky/_is created only bynpm run prepare, so--ignore-scriptsinstalls have no hooks at all. The main clone was in this state too..husky/pre-pushwas removed in docs: Reports & Projects Restructuring Phase 4 #1989, but the docs still describe it.Fix Summary
.husky/pre-push, which runslib/hooks/pre-push. Removesinstall.js,.hook-versionand thepostinstallscript.scripts/check-git-hooks.mjs, run bynpm run hooks:checkandpretest. It warns when hooks are missing and prints the fix,npm run prepare.--strictexits 1. It is skipped in CI.--no-verify.npm testpre-push hook would block every push until test: 7 failing suites / 5 failing tests on develop (2026-09-23 full-suite inventory) #3472 is fixed.Verification
check-git-hooksJest suite 5/5 pass; branch-name integration script 11/11 pass.npm run preparethe check warns (--strictexit 1); after it, the check passes. The commits in this PR went through the live pre-commit hook, and the pushes through the live pre-push hook.git hook run pre-pushfails with exit 1 onbad_Name_x; the check skips in CI; unsetcore.hooksPath; a single missing hook.Risk & Rollback
pretestonly warns.Changelog
Fixed
Summary by CodeRabbit
git push --no-verify.