chore: catch lint failures before push (misspell + scoped golangci-lint) - #2994
chore: catch lint failures before push (misspell + scoped golangci-lint)#2994momosh-ssv wants to merge 12 commits into
Conversation
Greptile SummaryThe PR adds an opt-in pre-push misspell check, a Make target to install repository hooks, and a pinned Go tool declaration for the standalone linter.
Confidence Score: 4/5The PR appears safe to merge, with non-blocking hook reliability and diagnostic issues worth addressing. The hook can silently omit linting in some remote layouts, mislabel tool failures as spelling findings, and mishandle unusual filenames, but these concerns affect the optional local check rather than production behavior. Files Needing Attention: scripts/git-hooks/pre-push
|
| Filename | Overview |
|---|---|
| scripts/git-hooks/pre-push | Adds the committed-blob misspell hook, with non-blocking robustness issues around base discovery, tool-error classification, and pathname handling. |
| Makefile | Adds a straightforward opt-in target that configures the repository-relative hooks path. |
| tool.mod | Promotes the already-pinned golangci misspell command to a Go tool without changing dependency versions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Push[git push] --> Ref{Existing remote ref?}
Ref -->|Yes| Remote[Use remote SHA as base]
Ref -->|No| Merge[Find merge-base with origin/stage or origin/main]
Merge -->|Neither available| Skip[Silently skip ref]
Remote --> Diff[Collect changed Go and Markdown paths]
Merge --> Diff
Diff --> Blob[Read each blob at local SHA]
Blob --> Tool[Run misspell through go tool]
Tool -->|Any output| Block[Label as misspell issue and block]
Tool -->|No output| Allow[Allow push]
Reviews (1): Last reviewed commit: "chore: add versioned pre-push hook runni..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| base=$(git merge-base "$local_sha" origin/stage 2>/dev/null) || | ||
| base=$(git merge-base "$local_sha" origin/main 2>/dev/null) || continue |
There was a problem hiding this comment.
If a contributor's checkout lacks local origin/stage and origin/main refs, both merge-base commands fail and the silent continue permits the new branch push without running misspell, defeating the hook's purpose of surfacing failures before CI.
| out=$(git show "$local_sha:$f" 2>/dev/null | | ||
| GOWORK=off "$GO" tool -modfile="$root/tool.mod" misspell -locale US -error 2>&1) | ||
| if [ -n "$out" ]; then | ||
| echo "pre-push: misspell found issues in $f:" >&2 | ||
| echo "$out" | sed "s|^stdin| $f|" >&2 | ||
| status=1 | ||
| fi |
There was a problem hiding this comment.
When go tool cannot resolve or execute misspell, its stderr is captured in out and labeled as a spelling issue, blocking the push with instructions to amend code instead of reporting the actual local tool failure.
|
|
||
| # Check the blobs at local_sha, not the working tree, so uncommitted edits | ||
| # don't mask (or fake) findings in what's actually being pushed. | ||
| for f in $files; do |
There was a problem hiding this comment.
For a pushed Go or Markdown path containing whitespace or shell glob characters, for f in $files applies field splitting and pathname expansion, so the hook reads nonexistent or unintended blobs and can omit spelling findings from the actual changed file.
There was a problem hiding this comment.
The set -f + newline-IFS fix covers spaces and glob metacharacters (verified at the current head with a zz probe dir/bad file.go probe commit — findings and the path rewrite come out intact), but one filename class still slips through: non-ASCII paths. With the default core.quotePath=true, git diff --name-only emits them C-quoted with the surrounding double quotes ("\346\226\207.go"), so git show "$local_sha:$f" can't resolve the path, the 2>/dev/null swallows the error, and the file is silently treated as clean — the golangci stage misses it too, since the quoted name's dirname isn't a real dir. All filenames in the repo are ASCII today, so this is a latent gap rather than a live one; git -c core.quotePath=off diff ... on L72 would close it.
There was a problem hiding this comment.
The observation is correct. Line 72 is:
files=$(git diff --name-only --diff-filter=ACMR "$base" "$local_sha" -- '*.go' '*.md')With the default core.quotePath=true, git C-quotes non-ASCII paths (e.g., "\346\226\207.go"), so the subsequent git show "$local_sha:$f" and dirname "$f" calls receive the literal C-quoted string — which doesn't resolve. The 2>/dev/null on the git show call swallows the error silently.
The fix is straightforward — pass -c core.quotePath=off to that git diff invocation:
| for f in $files; do | |
| files=$(git -c core.quotePath=off diff --name-only --diff-filter=ACMR "$base" "$local_sha" -- '*.go' '*.md') |
This tells git to emit raw UTF-8 paths with no quoting, which is safe because the existing set -f + newline-IFS already handles spaces and glob metacharacters. The newline is the only character that would break field splitting at that point, and git path names cannot contain a literal newline, so this covers all real-world filenames in the repo.
Worth noting: the same core.quotePath guard applies to the second git diff --name-only used to collect golangci lint dirs — both calls would need it for full consistency, though as you say, all current repo paths are ASCII, so this is a latent gap rather than an active bug.
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
momosh-ssv
changed the title
iurii-ssv
left a comment
iurii-ssv
left a comment
There was a problem hiding this comment.
Good optimization 👍
Some hardening suggestions below.
sed "s|^stdin| $f|" interpolated the path into both the delimiter and the replacement, so a path holding |, & or \ produced a malformed s-command or spliced the matched text back in. Swap the substitution for a POSIX read/case/printf loop, which treats the path as literal text.
CI never lints markdown — make lint's golangci misspell only scans .go, and no workflow checks .md — so a British spelling in a doc blocked the push while telling the user the CI lint check fails on it. Track .md findings apart from the CI-gated ones and print the message that actually applies.
The hook passes only -locale US; .golangci.yaml also sets misspell.ignore-rules (a placeholder today), which isn't forwarded. Say so in the header, with a note to mirror any real entry via misspell's -i, rather than parsing the YAML from a POSIX sh hook.
The misspell stage probes 'go tool' before trusting its output, but the golangci stage didn't: on an offline clone or a transient module-download failure the run exits 1 exactly like real findings do, so it blocked a clean push claiming CI would fail. Probe golangci-lint once up front, and treat only exit 1 as findings — 3 (config/run failure), 4 (timeout) and friends now warn instead of gating the push.
The pre-push hook runs misspell standalone with -locale US only, so a real misspell.ignore-rules entry would be honored by CI and flagged by the hook. Leave the reminder where the list is edited.
git diff compares two trees, so taking the remote tip as the base pulled base-branch churn into the diff whenever the branch moved sideways: after a rebase + force-push the old tip predates the new fork point, and after merging stage in it predates the merge — in both cases the diff spanned everything that moved in stage meanwhile. That made the golangci stage lint packages the push never touched, and let a doc typo in stage (which CI doesn't lint) block a push on a spelling nobody here wrote. Take the remote tip only while it's both an ancestor of what we're pushing and a descendant of the fork point, so a plain incremental push keeps its tight base; otherwise fall back to the merge-base with stage (or main), which the new-branch path already used. An existing branch with no merge-base at all now falls back to the remote tip rather than skipping the lint.
There was a problem hiding this comment.
Doing another thorough pass, looks like the script could use some more improvements ?
Re-reviewed at the current head by exercising the hook directly — synthetic "pushed" commits built with git plumbing, fed to the hook over stdin exactly as git invokes it.
Verified working:
- misspell stage: correct findings on the changed blobs (so uncommitted edits can't mask them), paths with spaces survive, the
stdin→ path rewrite is exact,.mdvs.gomessaging fires correctly, exit 1 blocks. - base selection: an incremental push diffs from the remote tip; rebase + force-push and merge-from-stage cases fall back to the merge-base; deletions and clean pushes pass silently.
- golangci stage: a planted S1002 caught in ~5s warm;
ssvsigner/keyscorrectly remapped to a run fromssvsigner/with the root config (matchingmake ssvsigner-golangci-lint);scripts/differskipped; non-HEAD ref pushes skip the stage as documented; non-1 exits warn and skip instead of blocking. tool.mod: misspell v0.7.0 was already pinned in the require graph, so the one-line tool directive resolves withtool.sumuntouched; ~0.15s per file warm.
Prior threads — both of my open ones are addressed at head, so I've resolved them:
- base selection on rebase + force-push: fix verified as above, including the no-merge-base fallback to the remote tip.
- misspell
ignore-rulesdivergence: the softened hook comment plus the reminder next toignore-rulesin.golangci.yamlis the right trade while the list holds only the placeholder — no need to wire up-ifor now.
New inline notes (all minor, none blocking): CI-attribution of findings in modules CI doesn't lint, a batched golangci run dropped wholesale by one unanalyzable dir, non-ASCII filenames escaping both stages via core.quotePath (follow-up on the earlier filename thread), a missing /opt/homebrew/bin/go fallback, and a core.hooksPath caveat + docs pointer.
Two cosmetic footnotes, no action needed: a .go misspelling on a HEAD push gets reported twice (blob stage + golangci's misspell linter, with a one-column offset between the two tools' conventions), and the // indirect marker on the misspell require in tool.mod is now stale — the next go mod tidy -modfile=tool.mod will reshuffle it, which is what keeps this diff one line today.
| status=1 | ||
| case $f in | ||
| *.md) docs_findings=1 ;; | ||
| *) ci_findings=1 ;; |
There was a problem hiding this comment.
.go findings in modules CI doesn't lint are still labeled as CI failures.
A misspelling in a changed scripts/differ/*.go lands in this *) arm, so the push fails with the L195 "the CI lint check (make lint) fails on them" message — but CI never lints that module: the root ./... run doesn't cross into nested modules, and there's no differ lint target (the golangci stage below skips scripts/differ for exactly that reason). Verified at the current head with a probe commit: the differ typo was flagged with the CI-failure message, while its planted staticcheck violation was (correctly) not reported.
Blocking is still reasonable — it's a real typo — but it's the same class as .md: the hook's own check, not a CI front-run. Consider classifying non-CI-linted modules (today just scripts/differ) like .md, or wording the message to cover them.
| if GOWORK=off "$GO" tool -modfile="$root/tool.mod" golangci-lint version >/dev/null 2>&1; then | ||
| if [ -n "$lint_dirs" ]; then | ||
| # shellcheck disable=SC2086 # lint_dirs is word-split on purpose | ||
| run_golangci "$root" $lint_dirs |
There was a problem hiding this comment.
One unanalyzable dir drops golangci findings for the whole batch.
All collected dirs go into this single run, and golangci-lint fails the entire invocation if any one of them yields no analyzable Go files. Verified at the current head: golangci-lint run ./utils/format ./cli/bootnode/testdata exits 5 (no go files to analyze), so run_golangci takes the *) warn-and-skip arm — and a real S1002 planted in ./utils/format is lost along with it.
Reachable once a changed .go sits under a testdata/ dir (none in the repo today) or in a dir whose files are all excluded by build tags. Fail-open, so the worst case is silent under-linting rather than a false block — fine to punt. If you want to harden it: one run_golangci call per dir (~1.5s each warm), or pre-filter the dirs through go list.
| # may not be on PATH in GUI-spawned shells, so fall back to the default install. | ||
| if command -v go >/dev/null 2>&1; then | ||
| GO=go | ||
| elif [ -x /usr/local/go/bin/go ]; then |
There was a problem hiding this comment.
The GUI-shell fallback misses Homebrew on Apple Silicon.
On arm64 macOS, Homebrew installs Go at /opt/homebrew/bin/go, not /usr/local/go/bin/go, so for GUI-spawned git clients — the exact audience this fallback targets — the hook silently no-ops on a stock Apple Silicon setup. Fail-open, so no harm done, but one more elif [ -x /opt/homebrew/bin/go ] would cover it.
|
|
||
| .PHONY: install-hooks | ||
| install-hooks: | ||
| git config core.hooksPath scripts/git-hooks |
There was a problem hiding this comment.
core.hooksPath replaces .git/hooks entirely.
After this, any hooks a dev already has in .git/hooks (a personal pre-commit, husky leftovers, ...) silently stop firing — git doesn't merge the two locations. Opt-in, so acceptable; a heads-up in the echo below would make the trade visible. Relatedly, nothing in the repo advertises the target yet — a line in docs/DEV_GUIDE.md would help discovery.
|
|
||
| # Check the blobs at local_sha, not the working tree, so uncommitted edits | ||
| # don't mask (or fake) findings in what's actually being pushed. | ||
| for f in $files; do |
There was a problem hiding this comment.
The set -f + newline-IFS fix covers spaces and glob metacharacters (verified at the current head with a zz probe dir/bad file.go probe commit — findings and the path rewrite come out intact), but one filename class still slips through: non-ASCII paths. With the default core.quotePath=true, git diff --name-only emits them C-quoted with the surrounding double quotes ("\346\226\207.go"), so git show "$local_sha:$f" can't resolve the path, the 2>/dev/null swallows the error, and the file is silently treated as clean — the golangci stage misses it too, since the quoted name's dirname isn't a real dir. All filenames in the repo are ASCII today, so this is a latent gap rather than a live one; git -c core.quotePath=off diff ... on L72 would close it.
2 participants
Why
Most PRs lately have hit a red
lintcheck that only surfaces in CI, since nothing in the repo installs a local check. Two flavors so far:misspellfindings (.golangci.yaml,locale: US) — usually British spellings in comments (cancelled,behaviour, ...).QF1008(embedded-field selector) in a test helper.What
scripts/git-hooks/pre-pushruns two stages against what a push actually introduces or modifies (diffed against the remote ref, or the merge-base withstage/mainfor new branches):.go/.mdblobs at the pushed commit with the standalonemisspellbinary, and blocks the push with the samefile:line:coloutput CI would produce. It scans the blobs, not the working tree, so uncommitted edits can't mask findings. Plain text scanning — near-instant.golangci-lint(same configmake lintuses in CI, staticcheck included) on just the packages containing changed.gofiles. Type-checking linters need full packages on disk, so this stage lints the checkout and only runs when the pushed ref is the checked-out HEAD; nested modules (ssvsigner/,scripts/differ/) are skipped since CI's root-module run doesn't cover them either. ~1.5s warm on a one-package change, vs CI's ~7-minute full-repo run.Also:
make install-hooks— opt-in, pointscore.hooksPathatscripts/git-hooks(relative, so it resolves per-worktree).tool.mod— promotesgithub.com/golangci/misspell/cmd/misspellto atooldirective, pinned at the v0.7.0 already in the graph via golangci-lint (one-line diff, no version changes;tool.sumuntouched). Using golangci's fork keeps the hook's word list identical to CI's. golangci-lint itself was already atooldirective.git push --no-verifybypasses the whole hook for a one-off.Verified
S1002bool-comparison + unused func fails both stages with CI-identical output (~6s cold-cache, ~1.5s warm); a clean comment-only probe on the same package passes in ~1.5s.make install-hookssets and resolves the hooks path correctly across worktrees.