Skip to content

chore(pre-commit): set default_install_hook_types so install arms both stages - #1221

Merged
IAmJSD merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-234
Oct 25, 2026
Merged

IAmJSD merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-234

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator

Root cause

.pre-commit-config.yaml sets default_stages: [pre-commit, pre-push] but not default_install_hook_types. pre-commit's install only writes the hook types listed there, and the default is [pre-commit]. So a plain pre-commit install creates only .git/hooks/pre-commit. The pre-push stage (go-test, frontend-build, frontend-test, plus every default-stage hook re-run on push) never runs locally unless the developer remembers pre-commit install -t pre-commit -t pre-push.

Fix

  • Add default_install_hook_types: [pre-commit, pre-push] next to default_stages, as the issue asks.
  • Update the two in-file comments that described the old behavior: the # Setup: header, and the pre-push block comment that told developers to run pre-commit install --hook-type pre-push.

I checked the other places that might give hook-install instructions (README.md, docs/DEVELOPMENT.md, CLAUDE.md, Makefile). None of them tell developers to run pre-commit install -t ..., and there is no install-hooks target, so nothing else needed changing. make pre-commit is an unrelated make target (fmt/vet/complexity/unit tests).

Closes #234

Verification

This is a config-only change, so the evidence comes from running real pre-commit in a throwaway clone, not from a Go test. I used pre-commit 4.0.1 (the version .github/workflows/pre-commit.yml pins) in a throwaway venv, with an isolated HOME/PRE_COMMIT_HOME, and core.hooksPath unset. Nothing was installed into the real worktree.

Fail before (clone of origin/main @ 5d4ab6b):

$ pre-commit install
pre-commit installed at .git/hooks/pre-commit
$ ls .git/hooks | grep -v '\.sample$'
pre-commit

Pass after (same clone, hooks removed, this branch's .pre-commit-config.yaml copied in):

$ pre-commit install
pre-commit installed at .git/hooks/pre-commit
pre-commit installed at .git/hooks/pre-push
$ ls .git/hooks | grep -v '\.sample$'
pre-commit
pre-push
$ grep ARGS= .git/hooks/pre-push
ARGS=(hook-impl --config=.pre-commit-config.yaml --hook-type=pre-push)

Acceptance criteria and other hooks that apply to this file:

$ pre-commit validate-config .pre-commit-config.yaml      # OK
$ pre-commit run check-yaml --files .pre-commit-config.yaml          # Passed
$ pre-commit run trailing-whitespace --files .pre-commit-config.yaml # Passed
$ pre-commit run end-of-file-fixer --files .pre-commit-config.yaml   # Passed
$ pre-commit run check-merge-conflict --files .pre-commit-config.yaml # Passed

Secret scanning: git secrets --scan .pre-commit-config.yaml came back clean in a fresh git init repo with an isolated HOME, using the patterns from scripts/setup-git-secrets.sh and .gitallowed. bash scripts/test-git-secrets-allowlist.sh gave 109 PASS, 0 FAIL.

No Go code changed, so build/vet/golangci-lint/gocyclo/internal/mocks guards don't apply. scripts/test-actionlint-hook.sh extracts only the actionlint hook from this file, so the new top-level key doesn't affect it.

Notes / out of scope

  • CI is unaffected. .github/workflows/pre-commit.yml runs pre-commit run --all-files, which ignores install hook types.
  • Behavior change for developers: after a fresh pre-commit install (or a re-run on an existing clone), git push now runs the pre-push stage. That includes go test -short -race ./... and the frontend build/jest when frontend/src/ changed. This is what the issue intends. Existing clones keep their current hooks until someone re-runs pre-commit install.
  • default_install_hook_types needs pre-commit >= 2.18.0. Older versions warn about an unknown key and fall back to installing only the pre-commit hook. I didn't add minimum_pre_commit_version, to keep the change to what the issue asked for.

Maintainer: please mirror the issue's triage labels onto this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/xs.

🤖 Generated with Claude Code

…h stages

default_stages already lists pre-commit and pre-push, but without
default_install_hook_types a plain `pre-commit install` only writes
.git/hooks/pre-commit, so the pre-push stage (go test, frontend build
and jest) never runs locally unless the developer remembers
`pre-commit install -t pre-commit -t pre-push`.

Declare both hook types next to default_stages and update the two
in-file comments that told developers to pass --hook-type pre-push.

Closes LeanerCloud#234

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 1 billable file and costs up to $0.25.

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 2f1dbca and 6d4955b.

Files selected for processing: 1
  • .pre-commit-config.yaml
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@IAmJSD IAmJSD added impact/internal Team-internal only type/chore Maintenance / non-user-visible triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/xs Trivial / one-liner labels Oct 20, 2026
@IAmJSD IAmJSD closed this Oct 20, 2026
@IAmJSD IAmJSD reopened this Oct 20, 2026
@cristim

cristim commented Oct 25, 2026

Copy link
Copy Markdown
Member

Independent review 2 (claude-fleet, cold): APPROVE 6d4955b

Findings (none blocking):

  • [non-blocking] .pre-commit-config.yaml:278 - Developers who already ran a plain pre-commit install keep only .git/hooks/pre-commit, so they get the pre-push stage only after re-running pre-commit install. The PR body could say so. Nothing in the repo has to change.
  • [non-blocking, intended] .pre-commit-config.yaml:275-278 - default_stages: [pre-commit, pre-push] was already set.
    • With the pre-push hook now installed, every git push runs the full default-stage suite on the pushed range, plus go test -short -race ./... and the frontend build and jest.
    • That is the purpose of the change, and it makes local pushes noticeably slower.
  • [non-blocking, pre-existing] docs/DEVELOPMENT.md:308-309 - The "Pre-commit hook" line points to make pre-commit. That Makefile target (Makefile:226) runs fmt, vet, complexity and unit tests, which is not the git hook. The PR does not make this worse.

Attacked:

  • Hook types: the only non-default stage is pre-push, used by go-test, frontend-build and frontend-test (head :259, :267, :275). No hook declares commit-msg or another stage, so [pre-commit, pre-push] is exactly the needed set.
  • Docs and Makefile:
    • Searching for pre-commit install, hook-type and install -t finds only the two comments in .pre-commit-config.yaml, and the PR rewrites both.
    • Nothing is left redundant or contradictory, and the Makefile:274 hint is still accurate.
  • Version floor:
    • default_install_hook_types arrived in pre-commit 2.18.0.
    • The config already uses the pre-commit/pre-push stage names, which need 3.2.0 or later, so no new floor is added.
    • CI pins 4.0.1 (.github/workflows/pre-commit.yml:219), which I tested.
  • Probes:
    • With the key removed, as on base, install writes only the pre-commit hook. That confirms the gap.
    • Set to [pre-push], only the pre-push hook is written, so the key alone decides.
    • A typo (pre-pushh) makes validate-config report INVALID, so a typo fails loudly.
    • With commit-msg added, all three hooks are written, so the value really is read.
  • scripts/setup-git-secrets.sh writes commit-msg, pre-commit and prepare-commit-msg and never touches pre-push. Its existing overlap with pre-commit on .git/hooks/pre-commit is unchanged.

Verified locally, in throwaway git init copies with HOME and PRE_COMMIT_HOME isolated under a scratch dir and global git config disabled. No real hooks or caches were touched, and no hooks were run.

  • pre-commit 4.7.0:
    • Head: validate-config OK, and pre-commit install writes .git/hooks/pre-commit and .git/hooks/pre-push.
    • Base: validate-config OK, and install writes only .git/hooks/pre-commit.
  • pre-commit 4.0.1 (the CI pin), head: validate-config OK, and install writes both hooks.
  • Merge:
    • git merge-tree --write-tree origin/main 6d4955b8 against origin/main 503eb2e is clean.
    • The only change on main in this file is an unrelated gosec comment block.
  • No Go, frontend, Terraform or migration changes.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

@cristim

cristim commented Oct 25, 2026

Copy link
Copy Markdown
Member

VERDICT: APPROVE 6d4955b

No findings.

Issue #234 check: the PR adds default_install_hook_types: [pre-commit, pre-push] next to default_stages in .pre-commit-config.yaml and updates the two comments that described the old manual --hook-type pre-push step. Both acceptance criteria hold. The branch merges cleanly onto origin/main 8f186a8 (GitHub: CLEAN).

Attacked:

  • Ran real pre-commit (4.7.0, isolated HOME and PRE_COMMIT_HOME, fresh git init repos, no core.hooksPath) with the head config: pre-commit install wrote both .git/hooks/pre-commit and .git/hooks/pre-push; pre-commit validate-config passes.
  • Mutation probe: the same run with origin/main's config (that line missing) wrote only .git/hooks/pre-commit. The one line is what arms the pre-push stage.
  • The three stages: [pre-push] hooks (go-test, frontend-build, frontend-test) exist, so the comment's claim about what now runs on push is accurate.
  • Searched the tree for other install instructions (pre-commit install, hook-type, install -t): none outside this file, so no docs drift.
  • CI impact: pre-commit.yml runs pre-commit run --all-files, which ignores install hook types. CI on 6d4955b: 24 pass, 1 skipping.

@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 6d4955b8334981e333a027e69d8de71c11aa361d merged onto main (5aa3df5b), together with #1155, #1219, #1221, #1223, #1224, #1225, #1226, #1227, #1256, #1262, #1266, #1270, #1272, #1275, #1283, #1286, #1293, #1297, #1299, #1300, #1301, #1304, #1307, #1311, #1313, #1325 in that order (combined tree f3a3331f, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 25, 2026 20:22
@IAmJSD
IAmJSD merged commit 090eac0 into LeanerCloud:main Oct 25, 2026
45 of 47 checks passed
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 6d4955b8, green CI, and the combined-tree verification above. Squash-merged at exactly 6d4955b8334981e333a027e69d8de71c11aa361d.

This was referenced Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(pre-commit): set default_install_hook_types so install arms both stages [platform part]

2 participants