Repository navigation
feat: add lefthook pre-commit hooks with gitleaks secret scanning - #2019
Merged
Merged
Conversation
panish16
requested review from
davemck513,
mengdong19,
ozamani9gh and
stevenc987
as code owners
September 23, 2026 17:15
Part of the Git Hooks RFC rollout (bcgov/entity#34619). No root package.json existed, so this adds a minimal hooks-only one, matching the approach used on bcgov/ppr#2364. Scoped to secret scanning only (bare 'npx gitleaks-secret-scanner', no custom install script, matching bcregistry/connect/ppr) - this repo has 16 separately-versioned services each with their own CI-enforced lint, so this intentionally doesn't attempt to replicate per-service lint wiring the way ppr's pilot did; that's a separate, larger effort if wanted later. prepare is guarded with '|| exit 0' from the start, learned from bcgov/bcregistry#281 breaking CD after merging this same base setup unguarded.
lefthook's own npm postinstall already skips itself under CI=true by default (only installs if LEFTHOOK=1/true is explicitly set - see https://lefthook.dev/usage/envs/LEFTHOOK.html). Our prepare script called 'lefthook install' directly though, which bypasses that and always runs - that's what broke bcgov/bcregistry#281's CD build (no git in the image). '|| exit 0' (the fix used on bcregistry/ppr) stops the failure from crashing the install, but still attempts the install in CI every time. This skips it outright when CI is set, since hooks have no purpose in a non-interactive build anyway.
CI=true isn't a safe assumption across CI systems - confirmed on bcgov/bcregistry#282 that Google Cloud Build doesn't set it at all, so that version of this guard would have silently kept failing there despite looking correct. Checking for git's actual presence instead doesn't depend on any CI provider's conventions.
pnpm/npm run lifecycle scripts through cmd.exe on Windows by default, where `command -v` and `/dev/null` do not exist. The previous guard errored and fell through to `|| true`, exiting 0 without installing the hook, so Windows developers silently got no secret scanning. `git --version && lefthook install || exit 0` has the same semantics (skip entirely when git is absent, never fail the install) and works in both cmd.exe and POSIX sh.
panish16
force-pushed
the
feat/git-hooks-gitleaks
branch
from
September 29, 2026 00:04
4758d16 to
e134f65
Compare
This was referenced Sep 29, 2026
pnpm v11 no longer reads the package.json `pnpm` field, so onlyBuiltDependencies was ignored and a root `pnpm install` failed with ERR_PNPM_IGNORED_BUILDS for anyone on default build-script approval. Disable lefthook's own postinstall via allowBuilds instead (prepare already installs the hook), and commit the lockfile.
|
stevenc987
approved these changes
Sep 29, 2026
stevenc987
approved these changes
Oct 1, 2026
pwei1018
pushed a commit
to pwei1018/bcregistry
that referenced
this pull request
Oct 7, 2026
lefthook's own npm postinstall already skips itself under CI=true by default (only installs if LEFTHOOK=1/true is explicitly set). Our prepare script called 'lefthook install' directly, which bypasses that and always runs regardless of environment. '|| exit 0' stopped the crash but still attempted the install in CI every time. This skips it outright, since hooks have no purpose in a non-interactive build. Same fix already applied on bcgov/namex#2019.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the Git Hooks RFC rollout (bcgov/entity#34619). Now unblocked - the hold on namex changes (pending the solr migration) was lifted 2026-09-14.
No root
package.jsonexisted, so this adds a minimal hooks-only one, matching the approach used on bcgov/ppr#2364 (also a repo with no root package.json).Scoped to secret scanning only - bare
npx gitleaks-secret-scanner, no custom install script, matching the pattern now live on bcregistry/connect/ppr. This repo has 16 separately-versioned services (api,jobs,solr-admin-app,solr-feeder,solr-synonyms-api, etc.), each with its own CI-enforced lint already. Intentionally not attempting to replicate per-service lint wiring the way ppr's pilot did for its 3 services - that's a much larger, separate effort if wanted later.preparechecks forgitdirectly rather than swallowing a failure or relying onCI=true:lefthook has no purpose in a non-interactive build, so it shouldn't attempt to run there at all - not just fail-safe. An earlier version of this checked
CI=true, but that's not a safe assumption across CI systems: confirmed on bcgov/bcregistry#282 that Google Cloud Build doesn't setCI=true, so that check would have silently stayed broken there despite looking correct. Checking forgit's actual presence avoids depending on any CI provider's conventions.Note: unlike bcregistry, this repo has no CI job that installs at the repo root, so I can't prove this specific change through a real pipeline run the way #282 was verified. The logic itself is the same, just flagging that this one doesn't have the same live-fire confirmation.
Verified locally with real
git commit: hook installs when git is present, and a fake secret gets caught and blocks the commit whether it's at repo root or nested under a subfolder likeapi/.Update: Windows fix
The original
command -v git >/dev/null 2>&1 && lefthook install || trueguard silently did nothing on Windows. pnpm/npm run lifecycle scripts throughcmd.exethere by default,command -vand/dev/nulldon't exist, and the failure fell through to|| true, so the install exited 0 but no hook was installed. Windows devs would have had no secret scanning and no error telling them so.Replaced with:
Same semantics (skip entirely when git is absent, never fail the install), works in both
cmd.exeand POSIXsh. Verified all four combinations (cmd.exe / sh, git present / absent), plus a realgit commitwith a fake token on Windows is blocked.Update: build approval moved to pnpm-workspace.yaml
The
pnpm.onlyBuiltDependenciesfield inpackage.jsonis ignored by pnpm v11, so a rootpnpm installfailed withERR_PNPM_IGNORED_BUILDSfor anyone on default build-script approval (the hook still installed, but the install exited 1). Moved it topnpm-workspace.yamlasallowBuilds: lefthook: falseand added the lockfile. Re-tested with default approval: install exits 0, hook installed, fake token blocked.