Close the seams a fleet audit found, and publish the four Go ids - #18
Conversation
Issue #2 audited this binary against 65 consuming repositories. Most of what it found was already closed by 0a2da20 and e437d86 -- it was written against a pre-1.1.1 checkout, and thirty-three of its items are answered in the released tree. What follows is the remainder, verified live. Guards: `no-private-repo-names-staged` named the file but never the line. It now reads the patch hunk by hunk and carries the line number a finding is on, which is the half of `path:line` a reader can act on. Auditing the other `git diff` call sites for the same defect found two more, and a filter that dropped an added line whose text began `++` -- all three now pass `--no-ext-diff --no-textconv`, so a difftastic or delta setup in a user's global config cannot silently turn the guard off. The owner-derived regexes are built once per judgement rather than once per candidate line, which is what makes reading every line affordable. Pins: A tree in which ZERO hook configurations were found was a pass. It is exit 2 now: a guard that read no pins established nothing, and saying so is the whole contract. Where configurations WERE read but none names a remote pin, that is a note rather than a refusal -- `repo: local`, `repo: meta` and a lefthook config with no `remotes:` block pin nothing, and the reader should hear which of the two happened. Unchecked pins now travel with the finding instead of being printed and dropped. Shim: A `[[shim]]` no checker rule claims used to collect a subject, check it with nothing, and run the command anyway. It refuses at exit 2 now. An option this shim's table cannot classify leaves the subcommand in doubt only where the two readings disagree about which word it is -- `git log -1 --oneline` is `log` either way, and reporting that as a could-not-look puts a refusal on the terminal for every ordinary command, which trains the reader to ignore the one where the doubt is real. `no-published-host-identity` now stands in front of bare `npm` rather than `npm publish`: the shim matches `pack:*` too, because a tarball's metadata is built by `pack` and merely uploaded by `publish`, and a matched invocation no checker claims is the exit 2 above. The four Go ids (#12): `uphold-gofmt`, `uphold-go-vet`, `uphold-go-build` and `uphold-go-test` are published for both runners. Across one superproject's 39 submodules these were hand-copied 24, 24, 24 and 22 times, byte-identical but for one variant: two repositories declare a `gofmt` entry that cannot exit nonzero, because `gofmt -l` prints what it would reformat and exits 0 regardless. Twenty-two copies enforce and two report a pass over unformatted code. The published id is written once, correctly. A pinned id can drift only in its rev, and that dimension already has a guard. `consumer_check.sh` gains a ninth question that drives all four, because publishing an id the parity harness never runs recreates the hole the publishing was meant to close. Distribution: Dependabot has no ecosystem that reads a lefthook config, so the one version a lefthook consumer pins is watched by nothing. That exclusion is stated in dependabot.yml and README.md rather than papered over.
|
Warning Review limit reached
Next review available in: 110 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
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❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (95.75%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #18 +/- ##
==========================================
+ Coverage 87.98% 88.48% +0.50%
==========================================
Files 25 25
Lines 7583 7835 +252
==========================================
+ Hits 6672 6933 +261
+ Misses 911 902 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Closes #12. Addresses the live remainder of #2.
What #2 actually found
The audit was written against a pre-1.1.1 checkout. Thirty-three of its items are already answered in the released tree by
0a2da20ande437d86— every one re-verified here with file:line evidence rather than "fixed" a second time.src/selection.rs's deadlock,.flatten(), index selection and out-of-repo include;src/main.rs's argv panic and root discovery; all threesrc/audit.rsitems;src/text.rs;src/config.rs; and the whole of section G — the Python reconciler's policy reader is gone andscripts/check_hook_pins.pywas deleted in0a2da20.What follows is the remainder, reproduced live before it was changed.
Guards
no-private-repo-names-stagednamed the file but never the line. It reads the patch hunk by hunk now and carries the line number. Auditing the othergit diffcall sites for the same defect found two more — and a filter that dropped an added line whose text begins++. All three now pass--no-ext-diff --no-textconv, so a difftastic or delta setup in a user's global config cannot silently turn the guard off. Owner-derived regexes are built once per judgement rather than once per candidate line, which is what makes reading every line affordable.Pins
A tree in which zero hook configurations were found was a pass; it is exit 2 now. Where configurations were read but none names a remote pin, that is a note rather than a refusal —
repo: local,repo: metaand a lefthook config with noremotes:block pin nothing, and the reader should hear which of the two happened. Unchecked pins travel with the finding instead of being printed and dropped.Shim
A
[[shim]]no checker rule claims used to collect a subject, check it with nothing, and run the command anyway. Exit 2 now.An option the shim's table cannot classify leaves the subcommand in doubt only where the two readings disagree about which word it is.
git log -1 --onelineislogeither way, and reporting that as a could-not-look puts a refusal on the terminal for every ordinary command — which trains the reader to ignore the one where the doubt is real. Caught during consumer verification on VitaGate.no-published-host-identitynow stands in front of barenpmrather thannpm publish: the shim matchespack:*too, because a tarball's metadata is built bypackand merely uploaded bypublish.The four Go ids (#12)
uphold-gofmt,uphold-go-vet,uphold-go-build,uphold-go-test— published for both runners, gated on(\.go|go\.mod|go\.sum)$,language: systemso they add no toolchain.Across one superproject's 39 submodules these were hand-copied 24, 24, 24 and 22 times, byte-identical but for one variant: two repositories declare a
gofmtentry that cannot exit nonzero, becausegofmt -lprints what it would reformat and exits 0 regardless. Twenty-two copies enforce; two report a pass over unformatted code. The published id is written once, correctly —test -z "$(gofmt -l .)". A pinned id can drift only in its rev, and that dimension already has a guard.consumer_check.shgains a ninth question that drives all four. Publishing an id the parity harness never runs would recreate the hole the publishing was meant to close.Distribution
Dependabot has no ecosystem that reads a lefthook config, so the one version a lefthook consumer pins is watched by nothing. Stated in
dependabot.ymlandREADME.mdrather than papered over.Verified on consumers
Read-only, against five sibling repositories on one machine — FinanceGate (39 submodules, 36G), ghostkey, GroundGate, RouterPlane, VitaGate.
scanbyte-identical old vs new, exit 0 everywhere. FinanceGate pre-push over 744 commits: 0.30s, no hang. A submodule with.gitand no policy refuses rather than climbing into the superproject. The lefthook-only install path was driven on a fixture, since no consumer here has a top-levellefthook.yml.357 Rust tests, 79 Python tests, clippy under the crate's own lint profile.
Not in scope
Section I (rule halves that did not survive the port) and section J (the base-sets proposal) are design work, not defects. #11 and #13 untouched.