feat(install): warn about hooks that won't run with the installed shims - #2205
feat(install): warn about hooks that won't run with the installed shims#2205BitWeaverDev wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ada96bec03
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc3a27032d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eec0f0e94c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eec0f0e94c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2205 +/- ##
==========================================
+ Coverage 93.16% 93.38% +0.21%
==========================================
Files 127 129 +2
Lines 28182 27681 -501
==========================================
- Hits 26257 25849 -408
+ Misses 1925 1832 -93 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
📦 Cargo Bloat ComparisonBinary size change: +0.00% (29.1 MiB → 29.1 MiB) Expand for cargo-bloat outputHead Branch ResultsBase Branch Results |
⚡️ Hyperfine BenchmarksSummary: 1 regressions, 0 improvements above the 10% threshold. Environment
CLI CommandsBenchmarking basic commands in the main repo:
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base --version |
2.6 ± 0.1 | 2.5 | 2.9 | 1.09 ± 0.04 |
prek-head --version |
2.4 ± 0.1 | 2.3 | 2.6 | 1.00 |
prek list
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base list |
10.7 ± 0.3 | 10.1 | 11.3 | 1.00 |
prek-head list |
11.1 ± 1.2 | 10.3 | 18.0 | 1.04 ± 0.11 |
prek validate-config .pre-commit-config.yaml
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base validate-config .pre-commit-config.yaml |
3.8 ± 0.1 | 3.7 | 4.1 | 1.10 ± 0.04 |
prek-head validate-config .pre-commit-config.yaml |
3.5 ± 0.1 | 3.3 | 3.6 | 1.00 |
prek sample-config
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base sample-config |
2.9 ± 0.1 | 2.7 | 3.2 | 1.07 ± 0.04 |
prek-head sample-config |
2.7 ± 0.1 | 2.6 | 2.8 | 1.00 |
Cold vs Warm Runs
Comparing first run (cold) vs subsequent runs (warm cache):
prek run --all-files (cold - no cache)
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run --all-files |
77.9 ± 2.3 | 74.1 | 82.3 | 1.00 |
prek-head run --all-files |
78.9 ± 2.9 | 74.4 | 83.5 | 1.01 ± 0.05 |
prek run --all-files (warm - with cache)
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run --all-files |
78.8 ± 2.7 | 75.2 | 85.2 | 1.00 |
prek-head run --all-files |
80.0 ± 2.4 | 75.8 | 85.5 | 1.02 ± 0.05 |
Full Hook Suite
Running the builtin hook suite on the benchmark workspace:
prek run --all-files (full builtin hook suite)
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run --all-files |
79.7 ± 3.5 | 75.2 | 92.6 | 1.00 |
prek-head run --all-files |
81.1 ± 3.8 | 75.7 | 95.6 | 1.02 ± 0.07 |
Individual Hook Performance
Benchmarking each hook individually on the test repo:
prek run trailing-whitespace --all-files
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run trailing-whitespace --all-files |
22.2 ± 0.4 | 21.6 | 23.1 | 1.00 ± 0.03 |
prek-head run trailing-whitespace --all-files |
22.2 ± 0.4 | 21.5 | 23.3 | 1.00 |
prek run end-of-file-fixer --all-files
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run end-of-file-fixer --all-files |
28.2 ± 2.2 | 25.7 | 34.7 | 1.00 |
prek-head run end-of-file-fixer --all-files |
28.6 ± 2.2 | 25.4 | 34.1 | 1.01 ± 0.11 |
prek run check-json --all-files
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run check-json --all-files |
9.0 ± 0.3 | 8.4 | 9.9 | 1.04 ± 0.05 |
prek-head run check-json --all-files |
8.7 ± 0.3 | 8.2 | 9.4 | 1.00 |
prek run check-yaml --all-files
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run check-yaml --all-files |
8.8 ± 0.1 | 8.6 | 9.0 | 1.00 ± 0.02 |
prek-head run check-yaml --all-files |
8.8 ± 0.2 | 8.5 | 9.2 | 1.00 |
prek run check-toml --all-files
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run check-toml --all-files |
9.2 ± 0.3 | 8.5 | 9.9 | 1.05 ± 0.05 |
prek-head run check-toml --all-files |
8.7 ± 0.3 | 8.2 | 9.4 | 1.00 |
prek run check-xml --all-files
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run check-xml --all-files |
8.6 ± 0.3 | 8.1 | 9.6 | 1.01 ± 0.05 |
prek-head run check-xml --all-files |
8.5 ± 0.3 | 7.8 | 9.1 | 1.00 |
prek run detect-private-key --all-files
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run detect-private-key --all-files |
14.5 ± 1.0 | 13.0 | 16.8 | 1.00 |
prek-head run detect-private-key --all-files |
15.0 ± 1.3 | 12.9 | 17.7 | 1.03 ± 0.11 |
prek run fix-byte-order-marker --all-files
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run fix-byte-order-marker --all-files |
19.7 ± 0.8 | 18.3 | 21.1 | 1.04 ± 0.07 |
prek-head run fix-byte-order-marker --all-files |
19.0 ± 0.9 | 17.5 | 21.2 | 1.00 |
Installation Performance
Benchmarking hook installation (fast path hooks skip Python setup):
prek install-hooks (cold - no cache)
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base install-hooks |
5.3 ± 0.1 | 5.2 | 5.3 | 1.06 ± 0.02 |
prek-head install-hooks |
5.0 ± 0.1 | 4.9 | 5.1 | 1.00 |
prek install-hooks (warm - with cache)
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base install-hooks |
5.3 ± 0.1 | 5.3 | 5.4 | 1.06 ± 0.02 |
prek-head install-hooks |
5.0 ± 0.1 | 4.9 | 5.1 | 1.00 |
File Filtering/Scoping Performance
Testing different file selection modes:
prek run (staged files only)
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run |
41.3 ± 0.7 | 40.0 | 42.8 | 1.00 |
prek-head run |
42.4 ± 1.9 | 40.1 | 48.8 | 1.03 ± 0.05 |
prek run --files '*.json' (specific file type)
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run --files '*.json' |
9.2 ± 0.2 | 8.9 | 9.6 | 1.00 |
prek-head run --files '*.json' |
13.2 ± 15.3 | 8.5 | 75.3 | 1.43 ± 1.67 |
prek run --files '*.json' (specific file type): 43.2100% slower
Workspace Discovery & Initialization
Benchmarking hook discovery and initialization overhead:
prek run --dry-run --all-files (measures init overhead)
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run --dry-run --all-files |
7.7 ± 0.1 | 7.5 | 7.9 | 1.04 ± 0.02 |
prek-head run --dry-run --all-files |
7.4 ± 0.1 | 7.2 | 7.6 | 1.00 |
Meta Hooks Performance
Benchmarking meta hooks separately:
prek run check-hooks-apply --all-files
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run check-hooks-apply --all-files |
12.0 ± 0.7 | 11.5 | 13.4 | 1.00 |
prek-head run check-hooks-apply --all-files |
12.8 ± 2.4 | 11.5 | 18.9 | 1.07 ± 0.21 |
prek run check-useless-excludes --all-files
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run check-useless-excludes --all-files |
12.4 ± 0.3 | 11.9 | 12.9 | 1.04 ± 0.04 |
prek-head run check-useless-excludes --all-files |
11.9 ± 0.4 | 11.3 | 12.5 | 1.00 |
prek run identity --all-files
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
prek-base run identity --all-files |
11.0 ± 0.1 | 10.8 | 11.3 | 1.00 |
prek-head run identity --all-files |
11.4 ± 0.3 | 10.9 | 12.0 | 1.03 ± 0.03 |
e19b651 to
66e19bc
Compare
|
Thanks! This is a common source of confusion and has come up a few times before, so I'm open to improving the situation. That said, I don't think this should be a warning. A warning implies something is wrong and should be fixed, but it is perfectly valid to not install a Git hook and only run a hook manually by its id when that is intended. I think this should be framed more as a tip/reminder instead. |
|
That's a fair point, and yeah, framing it as a warning does imply something needs fixing when really it's often on purpose. I went ahead and reworked it: dropped the |
`prek install` only sets up shims for the requested hook types (via `--hook-type`/`default_install_hook_types`, defaulting to `pre-commit`). Hooks confined to other stages, e.g. `stages: [pre-push]`, would then silently never run, which is easy to miss. Warn at install time and list the offending hooks plus how to install the missing hook type(s). Stages are read straight from config (hook `stages`, falling back to `default_stages`), so install stays offline and fast; manual-only hooks are skipped since they have no shim.
A remote hook can declare its stages in the repo manifest, which the install command never fetches. Falling back to default_stages for a remote hook that omits stages in the project config could therefore warn about a hook that actually runs. Only trust explicit config-level stages for remote hooks and skip the rest; local/meta/builtin hooks are fully described by the config, so they keep using default_stages.
A plain `prek install` at the workspace root installs a shim that runs every nested project at hook time, so the warning has to look at all of them. Scanning only the project discovered at the cwd missed a subproject whose hooks are confined to an uninstalled stage. Discover the workspace the same way `run` does and check each project's config. This stays offline since discovery only reads config files.
A remote hook with an explicit `stages: []` overrides its manifest and resolves through `default_stages`, the same way the hook builder does, so only hooks that omit `stages` entirely are skipped as unknown. Hooks ruled out by the selectors persisted into the shim (`--include`, `--skip`) are no longer reported, since the installed scripts will never run them. Env-var skips are not written into the shim and keep the warning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Skipping a stage on purpose (e.g. running a hook manually by id, or only from CI) is a valid setup, not a problem to fix, so this drops the `warning:` prefix and prints to stdout instead of stderr as an informational note.
66e19bc to
8a8de51
Compare
|
One remaining false positive: the current implementation only treats hook types from this What makes it a little more complicated: an existing shim should not automatically mean “this stage is covered for every hook”. A previously installed shim may have persisted selectors, for example: prek install --hook-type pre-push --skip slow-hook
prek install --hook-type pre-push some-project:some-hookIn those cases, |
A plain `prek install` only knows about the hook types passed to this invocation, so a stage covered by a shim from an earlier `prek install --hook-type` call was wrongly reported as unmatched. Existing shims are now inspected too, but only actually count as coverage for a hook if the shim's own persisted --include/--skip selectors would select it, since those may differ from the current invocation's.
|
good catch, yeah that was a real gap. pushed a fix now that also looks at shims already sitting in the hooks dir, not just the types being installed this run. for a type thats not being reinstalled, it checks if theres an existing prek shim there and if so parses the include/skip args that got persisted into it back out, so it can tell whether that specific hook would actually be selected by it, not just "a shim exists so its fine". matches the two-command example you gave pretty closely, added a test for exactly that case (persisted include only matches one hook, other hook still gets flagged). also made sure a non-prek script sitting at that path (foreign hook) doesnt count as coverage, since we cant trust it to run our stuff. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b93f4eb811
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
An existing shim installed with --cd (from a subproject) or --config (an explicit config file) only ever discovers that scoped project when it runs, never the whole workspace. Crediting it as coverage for any hook in the workspace could silently hide a real gap. A --cd=path shim now only counts as coverage for hooks under that path. A --config=path shim never counts, since the persisted path can't be reliably mapped back to a project.
|
Thanks for continuing to refine this. The original issue is real, but this has become quite a lot of install-time inference for what is ultimately a tip/reminder. We now need to parse existing shim arguments, reconstruct selectors, reason about So I’d rather pause this for now than add this much machinery around |
Closes #2056
What
prek installonly installs shims for the hook types you ask for —--hook-type, ordefault_install_hook_types, falling back topre-commit. If the config has hooks pinned to a different stage (the classic case being a slowerstages: [pre-push]hook), those hooks just silently never run after a plainprek install, and it's easy not to notice.This adds a warning at install time that names the hooks whose stages aren't covered and points at the fix:
Notes on the approach
stages→default_stages→ all), soinstallstays offline and fast. The trade-off is that a remote hook narrowed only by its manifest (not by the project config) won't be flagged — that's the conservative choice: no false positives, no new network I/O, and it covers the case from the issue.manualis skipped, since manual-only hooks have no Git shim and aren't meant to run automatically.Stages::resolvesoinstallandHookSpec::apply_project_defaultsshare one implementation instead of duplicating the fallback rules.Tests
Added integration coverage in
tests/install.rs: an unmatched hook warns (while an all-stages sibling does not), installing the matching--hook-typesilences it, manual-only never warns, anddefault_stagesinheritance is taken into account.