A module-wide check is triggered by the module, not by the file that changed - #129
Merged
Conversation
…changed `uphold-go-vet`, `uphold-go-build` and `uphold-go-test` declared `pass_filenames: false` and a `files:` regex on Go paths, and the two did not agree. The command reads the whole module; the trigger asked whether a Go path was staged. A commit staging only a testdata fixture, a //go:embed asset or a golden file changes what the module is compiled and tested against and skipped all three, measured in a consumer as prek run --files testdata/x.json -> (no files to check)Skipped CI running --all-files found it afterwards, which makes the local gate a green the later red contradicts, at the seam that promised to catch it first. The hand-copied declarations these ids replaced across the fleet carried no filter, so the filter was a regression handed to every repository that adopted the id. The regex comes off those three and nothing takes its place. With no `files:` a runner filters the staged set by nothing, so the list is non-empty on any commit and empty only where nothing was staged: the whole module on any change, and nothing on an empty file set, which both runners agree on. `always_run: true` would be one step further and run `go test ./...` where a runner was handed no files at all. `uphold-gofmt` keeps the regex, because formatting is the one question here whose answer a file that is not Go cannot change. `stages:` reaches every stage a runner has a file set at, which is what the fleet declarations these ids replaced declared. The `[pre-commit]` they carried was a lefthook reason -- `lefthook run <group>` applies no `glob` -- and that reason was never about pre-commit or prek. The merge stage is where the strongest version of the same defect lives: git runs pre-merge-commit for a merge and pre-commit for a commit, so a merge bringing in a module change ran none of these four. hooks/lefthook.yml conditions its three on `go.mod` existing instead of on a glob, because the two files are distributed differently: a manifest id is NAMED by the consumer, so an unconditional id fires only where it was asked for, while a lefthook remote config is merged wholesale and an unconditional job would arrive in every consuming repository on a `ref:` bump, Go or not. The condition moves from the commit to the repository, which is the level it was always about. Second defect, same four ids. `uphold-go-build` ran `go build -o <dir> ./...`, and `-o` is right for one shape of module and a refusal for the other: it keeps a single main package from writing its executable into the tree the hook has just pronounced clean, and it exits 1 with "go: no main packages to build" on every library-only module, which is most of a Go fleet. A repository with no `package main` in it could not commit. The packages are asked about first now, and a module with no main is compiled by the bare `go build ./...`, which builds every package and writes nothing. scripts/consumer_check.sh gains the two questions that would have caught them. Question 10 plants a failing test with hooks off and then commits ONLY a testdata file, which is the commit the regex skipped; question 11 makes the module a library, which is the shape `-o` refuses. Both fail against the manifest as it stood and pass against this one, under prek locally and under all three runners in CI. The four ids are now pinned by that consumer at question 9, when it becomes a repository with Go in it, rather than from the start: a module-wide check pinned by a repository with no module fails on every commit, loudly and correctly. Closes #127 Closes #128 Claude-Session: https://claude.ai/code/session_01HEudouCNhFHK6UPEWcWqXd
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request updates Go hook triggers across supported stages, changes Lefthook gating to detect ChangesGo hook execution and validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The changes correct hook triggering and library-only Go build behavior, with targeted consumer checks and passing validation reported; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant HookRunner
participant Lefthook
participant GoToolchain
HookRunner->>Lefthook: run Go validation jobs
Lefthook->>Lefthook: check for go.mod
Lefthook->>GoToolchain: inspect package names
GoToolchain-->>Lefthook: return package names
Lefthook->>GoToolchain: build with temporary output or bare build
GoToolchain-->>Lefthook: return validation status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy both linked issues. They remove staged-file filters from the three module-wide Go hooks while preserving the gofmt filter, restore all required stages, add the go.mod condition for lefthook, select the correct build mode for main and library-only modules, and add regression coverage [ Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
=======================================
Coverage 94.12% 94.12%
=======================================
Files 37 37
Lines 13493 13493
=======================================
Hits 12700 12700
Misses 793 793 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
HackingGate
added a commit
that referenced
this pull request
Sep 2, 2026
This branch sat on integration-2026-09-02, whose five branches have now landed on main as squashes (#116, #117, #123, #124, #126) alongside #129, so git saw the same lines added twice in src/scan.rs, src/shim.rs, src/text.rs, src/config.rs and tests/structural_documentation.rs. Resolved to the tree a rebase of this branch's own commit onto main produces: main's copy of all six, plus this branch's rule-as-a-type refactor on top. Claude-Session: https://claude.ai/code/session_01HEudouCNhFHK6UPEWcWqXd
HackingGate
added a commit
that referenced
this pull request
Sep 2, 2026
… unknown that is no longer a pass (#132) Point the documented pins at 1.14.0 and bump the crate version. Minor, not patch. This release carries names a policy written against 1.13.0 cannot use: the bundled set mismatched-author; api:* as a matchable verb on the gh and glab shim tables, so a forge call that carries a body is read the way gh pr create is; unresolved on a [[shim]] table, whose default refuses before exec where a scope could not be evaluated; and refuse_unknown, foreign_hosts and redact_matches on the private-name family. It also closes four seams that reported a pass they had not established. The MCP hook seam consulted no prose rule, so every prose_regexp was dark exactly where an agent publishes. Five command lines walked past the shim, four of them ending in exit 0 with nothing printed. A blob no charset decodes was read as clean rather than refused. A forge host nobody could ask about produced no finding, no report and no exit code. Landed in this release: #116, #117, #123, #124, #126, #129, #131. A policy written against 1.13.0 loads unchanged, and every new name has to be named or inherited to run. Two things do change for a consumer who only bumps a rev or a ref: the seam fixes above start refusing text and command lines that used to pass unexamined, and the four published Go ids are triggered by the module rather than by the file that changed, at all four stages rather than at pre-commit alone. Claude-Session: https://claude.ai/code/session_01HEudouCNhFHK6UPEWcWqXd
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
2 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.
Two defects in the four published Go ids, found by a fleet migrating off its
own hand-copied guards manifest onto
uphold-gofmt/uphold-go-vet/uphold-go-build/uphold-go-test.The trigger asked about the file, the command reads the module (#127)
uphold-go-vet,uphold-go-buildanduphold-go-testcarriedpass_filenames: falseandfiles: '(\.go|go\.mod|go\.sum)$'. A commit stagingonly a testdata fixture, a
//go:embedasset or a golden file changes what themodule is compiled and tested against and skipped all three:
pre-commitanswers identically. CI running--all-filesfinds it afterwards,so the local gate is a green the later red contradicts, at the seam that
promised to catch it first -- and the fleet declarations these ids replaced
carried no filter, so the published id was a regression for the repositories
adopting it.
The regex comes off those three and nothing replaces it. Measured on both
runners: with no
files:the staged set is filtered by nothing, so the hookruns on any commit and is skipped only where the file set is empty. That is
exactly the wanted spelling -- the whole module on any change, nothing on an
empty file set.
always_run: truewas the other candidate and goes one steptoo far: it runs where a runner was handed no files at all.
uphold-gofmtkeeps the regex. Formatting is the one question here whoseanswer a file that is not Go cannot change.
stages:was narrowed for a lefthook reason that is not a prek reasonThe four declared
stages: [pre-commit]where the guards manifest they replaceddeclared
[pre-commit, pre-merge-commit, pre-push, manual], and the reasonwritten in the file is the lefthook group limitation:
lefthook run <group>applies no
glob, so a Go job reachable that way fires in a repository with noGo in it. That reason was never about pre-commit or prek, which have a file set
at every one of the four stages. It is fixed here: all four now declare all
four stages. The merge stage matters most -- git runs
pre-merge-commitfor amerge and
pre-commitfor a commit, so a merge bringing in a module change rannone of these four.
hooks/lefthook.ymlkeeps its Go jobs at pre-commit and conditions three ofthem on
test -f go.modrather than on a glob. This is the one place the twopublished files stop matching token for token, and the asymmetry is between the
distribution shapes rather than between the checks: a
.pre-commit-config.yamlconsumer NAMES the id, so an unconditional id fires only where it was asked for,
while a lefthook remote config is merged into the consumer's own wholesale, so
an unconditional job arrives in every consuming repository on a
ref:bump, Goor not. That is the same argument this file already makes for keeping
uphold-supply-chainout of it. The condition moves from the staged set to therepository, which is the level it was always about. Verified against lefthook
2.1.9 directly: the go.mod condition and the
{{.Name}}template both survivelefthook's own command templating.
go build -o <dir> ./...refuses every library-only module (#128)-okeeps a single main package from writing its executable into the tree thehook has just pronounced clean, and it also exits 1 with
go: no main packages to buildon a module with nopackage mainin it, which is most of a Go fleet.A repository like that could not commit at all, and the message it was refused
with said nothing about whether the module compiles.
The shape is asked about first now:
go build ./...with no-ocompiles every package and writes nothing, whichis the right command for the library case and the wrong one for the single-main
case.
go listis silenced because a module too broken to enumerate takes thebare branch, which prints the compiler's own error rather than a listing failure
standing in for one.
Tests
scripts/consumer_check.shis where the four ids are driven, and it gains thetwo questions that would have caught these:
file -- the commit the regex skipped.
-orefuses.Both were run against the manifest as it stood and both fail there
(
uphold-go-test did not refuse, andgo: no main packages to build), and bothpass against this branch. The script also now pins the four ids at question 9,
when the consumer becomes a repository with Go in it, rather than from the
start: three of them run on every commit by design now, and a module-wide check
pinned by a repository with no module fails on every commit, loudly and
correctly. lefthook needs no equivalent, which is what its
go.modcondition isfor.
Run locally:
cargo build,cargo test(725 passing),cargo clippy --all-targets,cargo fmt --check,prek run --all-files(21 hooks, allpassed), and
scripts/consumer_check.sh prek(all eleven questions).Closes #127
Closes #128
Summary by CodeRabbit
Enhancements
Documentation
Tests
https://claude.ai/code/session_01HEudouCNhFHK6UPEWcWqXd