Make a check that could not look say so, everywhere it can happen - #3
Merged
Conversation
A guard's subject is what a commit or a push makes public, and several parts of that were never opened. A file NAME is published exactly as a file's contents are, so a private repository's name in a directory name, or a zero-width character in a file name, went out under a guard that reported the tree clean; the tree-wide guards now scan the path as well as the blob under it, and a tab or a newline -- legal inside a file, never inside a path -- is a finding there. At a push the guards also read the commit MESSAGES the push publishes, which is the one surface no earlier seam can reach for a commit written under --no-verify. The staged half had three ways to see nothing at all. A repository, global or system diff.external or textconv driver emptied `git diff --cached` and the guard passed on an empty diff, so the staged scan now runs with --no-ext-diff --no-textconv --no-color and core.quotepath=false. A committed `* -diff` attribute hid a plain-ASCII file the same way, so paths git will not diff as text are found with --numstat, read through their staged oid, and re-diffed with --text; a NUL in the first 8000 bytes is the one honest skip left. And a rename introduces a path while adding no line, so newly introduced paths are read from --diff-filter=ACR rather than inferred from the diff body. Each source is now one PATH rather than one blob labelled "staged changes", which also means [rule.files] finally bounds the staged guard and every finding names the file it arrived in. A submodule ended both tree-wide guards before they started: a gitlink is enumerated by `git ls-tree` and `git cat-file blob` cannot read one, so any tree with a submodule exited 2. Blob now carries git's mode and answers has_content(), so a gitlink is enumerated by path and never read, and scope::read refuses one with a sentence rather than letting cat-file fail. The two halves of a pre-push range also went through two separate resolutions that were free to drift; range_of() is the single answer both now use. A blob that will not decode is exit 2 rather than a silent skip, and the same for `scan --text`, where from_utf8_lossy searched U+FFFD where the bytes were and printed "policy checks passed" over text nobody had read. The identity fallback is chosen by testing for the identity rule rather than for its check KIND, because declaring an unrelated forbidden_literals rule is not a decision to stop checking what host the author is standing on.
A content rule is a claim about what this repository carries, and what it carries is what git tracks. Selection walked the tree instead, honouring every ignore file it found -- so a tracked file matched by a .gitignore line, a .git/info/exclude entry, or the operator's own global ignore file, which is not in the repository at all, was searched by no rule and reported clean. Git ignore rules do not apply to a file git already tracks; a walker's do. The globs now apply to `git ls-files`, and where there is no index to read the walk consults no ignore file at all, which selects a superset -- over- reporting is the direction a checker may fail in, and hiding a file is not. A path a rule could not open was dropped from the list on the way in, so the rule searched what was left, found nothing there, and printed "policy checks passed" over a tree it had not finished reading. Those paths are collected now, named on stderr with the cures, and turn exit 0 into exit 2 after every rule has reported. Collected rather than fatal on purpose: a tree with one unstaged deletion still has an answer for every other rule, and a reader who only ever sees "restore this file" never learns what was waiting behind it. A finding still outranks them, as it does in audit and in the pin guard. `files.include` naming a path outside the repository is refused at load. Such a root selects files that have no repository-relative name, so every hit was dropped on the way out and the rule reported a pass over a search that had found things. Both spellings are caught, absolute and climbing out with `..`. `git check-attr --stdin` was written to and then read from, in that order, which deadlocks on any repository whose answer exceeds a pipe buffer: measured at 3000 tracked paths, 150 KiB in and 200 KiB out against a 64 KiB pipe, the check hangs with no output and no exit code. The two pipes now move at once. The same deadlock in the shim's checker consultation is gone the same way, and a checker that exits 0 without draining the subject is exit 2 rather than a pass over the part it read. The shim ends in a real exec now, so pid, process group, terminal control and death by signal survive instead of being flattened to exit 1; the stdin a `-F -` invocation ate is handed back to the command as a descriptor, because a guard that silently eats the body it approved publishes an empty one. The subcommand is found by walking argv for the first two words that are neither an option nor an option's value, so `gh --repo owner/name issue create` is examined rather than execed unseen. The editor case is closed rather than warned about: the shim installs itself in the command's declared editor variable, runs the real editor, and checks what the editor leaves in the file when it closes. argv keeps its bytes from `main` to the exec. `std::env::args()` panics at exit 101 on an argument that is not UTF-8, out of a binary installed in front of `git` exactly where a latin-1 file name gets typed; the words the shim compares are a lossy copy, the exec gets the originals, and an invocation whose text is actually checked refuses the untranslatable argument with a sentence instead of checking U+FFFD. Three smaller holes on the same theme. The policy walk stops at the repository boundary, so a repository with no policy no longer borrows an enclosing superproject's and reports on another tree under this repository's name. `--policy PATH` asserts the <root>/policy/<name>.toml layout instead of taking the file's grandparent unchecked, which had rooted a scan at the repository's parent. dedent counts the common indent in chars rather than bytes, which panicked on a page whose indentation was not all ASCII. And `uphold rules --effective [--json]` prints what inheritance resolved to, so nothing outside the loader has to re-derive which rules a repository runs.
The command exists to say whether a private-to-public flip would republish anything, and it could not say no. A note about comment edit history -- true of every run, on every repository, and nothing this run could change -- was pushed into the unreadable list, so the list was unconditionally non-empty and exit 0 was unreachable. It is a standing caveat now, stated in the body of every report, and the unreadable list is reserved for surfaces this run actually failed to open. The verdict is one const fn over two counts, so the clean answer can be asserted from a unit test rather than only from a live forge. What it reads was also short in three places. It scanned HEAD's tree, but a name committed and deleted before HEAD is served by the forge forever and survives a rewrite of the default branch, so it now reads every blob reachable from HEAD, from origin's branches and from the retained pull-request refs, deduplicated by sha through one cat-file --batch-check. On the forge side it requested bodies and not titles, which is the field the `gh` shim guards with -t, and it never asked for review bodies or review-thread comments at all; all four are read now, and a failure becomes a named unreadable note carrying gh's own stderr as the reason. And `--limit 200` silently truncated a listing at 200, so the cap is 5000 and a listing that comes back at exactly the cap is reported as truncated rather than quietly cut short.
Two checkers read the same `rev:` lines, reached the same remote, and were free to return different verdicts about the same pin. The no-stale-hook-pins guard asked whether a pin had fallen behind its upstream and counted a pin whose remote it could not reach as passed; scripts/check_hook_pins.py asked whether the ref a pin names still exists and called that same pin unresolvable. Which answer a repository got depended on which seam ran, and the guard's answer was the wrong one: a pin nobody could check exited 0 out of a guard whose whole subject is whether a pin is still what it claims to be. One `git ls-remote` answers both questions now, so the script is gone along with the chance of two contradictory verdicts. A pin that could not be checked is exit 2, and the refusal names UPHOLD_ALLOW as the deliberate bypass -- a runner with no network fails this guard where it used to pass it, loudly and with a cure. A violation still outranks it: a pin genuinely behind is exit 1, with the unchecked pins printed beside the refusal rather than replacing it. Three more holes closed while the guard is the only reader left. A tree with no .pre-commit-config.yaml is an answer rather than an ENOENT, because the documented lefthook-only install path pins nothing there. lefthook `remotes:` entries are pins like any other, and one with no `ref:` is refused in the same words as a `repo:` with no `rev:`. The work tree is walked, so a config below the root is checked too and every finding names the file holding the pin; and a config with no top-level `repos:` key is could-not-look rather than zero pins, which is the same distinction one level up. The explicit-unknown claim moves to the rule that carries it, the generated review tier is rebuilt without the retired id, and the manual sweep the deleted hook held is kept: guards-manual in .pre-commit-config.yaml and the uphold-manual group in lefthook.yml are what turn "whoever pushes next finds out" back into something a schedule finds first.
The reconciler answered "does this rule run here" from the `- id:` lines in a runner's config, which is a list of what a repository PINNED rather than a list of what runs. Pinning the reconciler itself -- `uphold-check`, whose entry is this script -- was therefore evidence that content rules ran, and a guard claim was supplied by any pinned id at all rather than by the id that installs the stage the guard fires at. Evidence now comes from the `entry:` lines: an id counts when it actually runs `uphold scan` or `uphold guard --stage X`, the stage-to-id map is read out of those entries rather than written down beside them, and a rule is supplied only where a seam it declares is installed. A pre-push guard claim in a repository pinning only uphold-scan is refused, which is what it always should have been. `[inherit]` has three fields and this script read one. `inherit.paths` names a repository's own extra policy files, which config::load merges exactly as it merges the bundled sets, so every rule arriving that way was invisible and a claim on one was refused as supplied by nothing while the engine was running it -- a false negative in the direction that costs most, because the answer a person acts on is to delete a claim that was true. All three fields are read now, merged in the engine's own order. It is still a second reader of what config::load already resolves. The engine can be asked directly since `uphold rules --effective --json`, and the reason this script does not call it is written on the function: it is the hook other repositories install, and two of the three runners keep the binary inside their own environment directory rather than on PATH, so shelling out would turn a working reconcile into exit 2 for them. What keeps the duplication honest instead is a test that fails when the two readers disagree about this repository's own policy. Four smaller refusals, all of the same family. A lefthook key was read as a command by its indentation, so the `configs:` key README.md tells every lefthook consumer to write under `remotes:` was reported as a rule named `configs`; only valueless keys whose parent key is `commands:` count now. A file that is not UTF-8 is exit 2 with a sentence rather than a traceback at exit 1, for both the text and the TOML readers. The review settings are validated field by field instead of coerced, so a max_lines of "many" names the field it came from; and an `emit` name that is absolute, contains `..`, or resolves outside the root is refused before anything is rendered. Finally the coverage numerator no longer counts a claim the same report has just listed as supplied by nothing here.
Three published hook ids were pinned by the consumer harness and reached by no question it asked, so nothing in CI had ever run them: uphold-check, whose firing condition no earlier question satisfied; uphold-guard-merge, because no question made a merge; and uphold-guard-manual, because no question ran the manual stage. A pinned id nobody invokes is a claim about a seam rather than evidence of one. The harness asks eight questions now instead of five, and the three new ones edit the declaration in a commit, make a real --no-ff merge carrying a zero-width space, and run the manual stage per runner. Question 4's planted character is removed once its assertions pass, because it would otherwise refuse the merge in question 7 and the manual sweep in question 8 for the plant rather than for the case under test. The runner-parity job installed a Rust toolchain for all three runners, which is exactly what the pre-commit and prek legs exist to prove is unnecessary: `language: rust` is part of their manifest contract, and a leg that starts with a compiler already on PATH cannot tell a working bootstrap from a broken one. The toolchain and the cargo cache are gated on the lefthook leg, which is the one that genuinely needs a binary on PATH. `no-pinned-tool-install` missed the one cargo line that has nowhere else to put its pin: `--version` cannot be combined with `--git`, so `cargo install --git <url> --tag vX.Y.Z` is the pinned spelling and the rule did not match it. A lefthook `remotes: ref: vX.Y.Z` is the twin of a pre-commit `rev:`, except that dependabot has an ecosystem that moves a `rev:` and nothing moves a `ref:`, so it is the shape most likely to go stale unnoticed. Both are matched now. The promotion corpus is restored alongside, with the parity test that reads it: every line a promoted rule was promoted for still matches the rule, and no corpus list is empty -- a rule whose corpus emptied would satisfy a "everything matches" test vacuously. That is what keeps a future edit from narrowing a promoted pattern back to the shape one repository happened to have.
The documentation described several things this binary no longer does, and one it could not do at all. docs/REFERENCE.md said comment edit history was reported as unreadable and that exit 0 arrived when every surface was read and clean -- the first of which made the second unreachable, so the reference documented an outcome the tool could not produce. The standing caveat and the reachable clean answer are both written down now. The rest follows the behaviour it belongs to. Selection is what git tracks rather than a directory walk, and a path a rule could not open is exit 2 named on stderr. A `command.before` and a `[[shim]]` are two halves of one seam and the load refuses either half alone, so both worked examples now carry the shim block a reader would otherwise copy without it. The guard table says that a path is committed text, that a push publishes its commit messages, and that a pin the guard could not check is exit 2 with a named bypass. The shim section says what `editor_env` does, how the subcommand is found among the options, that a checker must drain its subject, and that the editor is a checkpoint rather than a blind spot. `uphold rules --effective` is documented where the inheritance fields it resolves are described.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (85.78%) 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 #3 +/- ##
==========================================
+ Coverage 82.87% 86.01% +3.14%
==========================================
Files 22 22
Lines 4933 6527 +1594
==========================================
+ Hits 4088 5614 +1526
- Misses 845 913 +68 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Warning Review limit reached
Next review available in: 46 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 (2)
📝 WalkthroughWalkthroughThis PR consolidates hook-pin checks into guards, expands scan and audit coverage, hardens policy and shim validation, adds ChangesGuard and policy execution changes
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Shim
participant Editor
participant Checker
participant Command
User->>Shim: invoke shimmed command
alt body comes from editor
Shim->>Editor: run configured editor wrapper
Editor-->>Shim: return edited content
end
Shim->>Checker: send subjects and stdin
Checker-->>Shim: allow or refuse
alt allowed
Shim->>Command: exec real command with original argv and replayed stdin
end
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pins.rs (1)
405-410: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftValidate SHA pins before accepting them
remote_refscollects tag names, not object IDs. A SHA-shapedrevthat is not also a tag name reachescontinuewithout proving that the remote contains it. Resolve SHA pins against the remote before skipping stale checks, and return exit2when object existence cannot be established.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pins.rs` around lines 405 - 410, Update the SHA handling in the pin validation flow around looks_like_a_sha so SHA-shaped revisions are resolved against the remote before bypassing stale checks. Only continue without further tag checks when the object is confirmed present; if remote resolution cannot establish the object’s existence, return exit code 2.src/shim.rs (1)
231-327: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
collect_flagsdoes not stop at--, butverb_noundoes.
verb_nountreats--as the end of options.collect_flagskeeps walking and still matches flag names after it. Forfaux pr create -- -F -, the two walkers disagree:verb_nounreads-Fand-as positionals, whilecollect_flagsreads-F -, consumes this process's stdin, and marksbody_given. The command then receives the replayed bytes as stdin even though it was told to treat-Fas a positional argument.Align the two walkers on the
--terminator so one argv grammar decides both questions.♻️ Proposed change to stop flag collection at `--`
let mut index = 0; while let Some(argument) = argv.get(index) { + // `--` ends the options, exactly as `verb_noun` reads it. Two + // walkers that disagree about where the options stop can decide + // different things about the same command line. + if argument == "--" { + break; + } // `--flag=value` and `--flag value` are the same flag. SplittingBased on learnings: "If two units of code differ only in a constant, they are one unit with an unextracted parameter." — the two argv walkers share one grammar and should not diverge on it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shim.rs` around lines 231 - 327, Update collect_flags to stop scanning options when it encounters the "--" terminator, matching verb_noun’s argv grammar. Once "--" is reached, do not inspect or consume subsequent positional arguments as flags or values, including preventing stdin handling and body_given updates from those arguments.Source: Learnings
🧹 Nitpick comments (13)
tests/audit_publication_cli.rs (2)
156-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the section header before splitting on it.
Line 169 calls
unwrap()onsplit_once("could NOT be read:"). The section is present only while the unreadable list is non-empty. This test does not assert that condition, so a future change that makes the run fully readable turns this test into an opaque panic onNoneinstead of a clear failure.♻️ Proposed change
- let (_, measured) = report.split_once("could NOT be read:").unwrap(); + let (_, measured) = report + .split_once("could NOT be read:") + .expect("the fixture has no forge, so the measured unreadable list must be present");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/audit_publication_cli.rs` around lines 156 - 175, Update the test the_standing_caveat_is_stated_without_being_counted_as_unread() to explicitly assert that report contains the "could NOT be read:" section header before calling split_once(...). Preserve the existing measured-content assertion while replacing the opaque unwrap path with a clear failure if the header is absent.
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate the fixture from the developer's global Git configuration.
git initinherits the user's global config. If a machine setscommit.gpgsign = true, everygit commitin these tests fails andgit()panics.--no-verifycovers hooks only. A globalinit.templateDircan also install hooks into the fixture.Set the fixture config explicitly.
♻️ Proposed change
git(&root, &["init", "-q", "-b", "main"]); git(&root, &["config", "user.name", "Test"]); git(&root, &["config", "user.email", "test@example.test"]); + git(&root, &["config", "commit.gpgsign", "false"]); + git(&root, &["config", "tag.gpgsign", "false"]); + git(&root, &["config", "core.hooksPath", "/dev/null"]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/audit_publication_cli.rs` around lines 57 - 59, Update the fixture setup around git initialization in the test to explicitly disable inherited signing and hooks-related configuration, and prevent global init templates from installing hooks. Configure these settings locally for the fixture before commits run, while preserving the existing user.name and user.email setup.tests/fixtures/test_promotion_corpus.py (1)
120-120: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftEvaluate corpus patterns with the production regex engine.
Python
reaccepts lookarounds and backreferences that Rust regex rejects. A future policy pattern could pass this test whileuphold scanfails to compile it. Use the production matcher or add a Rust integration test for the corpus.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fixtures/test_promotion_corpus.py` at line 120, Update the corpus matching logic around the test’s re.compile call to use the same production regex matcher and compilation behavior as uphold scan, rather than Python’s re engine. Ensure every corpus pattern is validated against the Rust-compatible engine so unsupported lookarounds or backreferences fail consistently with production.Source: Coding guidelines
tests/shim_cli.rs (2)
192-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClear the shim's editor variables in the
shimhelper.
shim()removes onlyUPHOLD_ALLOW.runinsrc/shim.rschecksUPHOLD_SHIM_EDITORbefore anything else, so a machine that has that variable set routes this case intoedit_and_checkand the assertion measures the harness rather than the code.tests/shim_handoff_cli.rsalready removesUPHOLD_SHIM_EDITOR,UPHOLD_SHIM_EDITOR_REAL, andUPHOLD_SHIM_EDITOR_ARGVfor this reason.♻️ Proposed change to the shared helper
.current_dir(root) .env("PATH", path) .env_remove("UPHOLD_ALLOW") + .env_remove("UPHOLD_SHIM_EDITOR") + .env_remove("UPHOLD_SHIM_EDITOR_REAL") + .env_remove("UPHOLD_SHIM_EDITOR_ARGV") .output()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shim_cli.rs` around lines 192 - 230, Update the shared shim() test helper to remove UPHOLD_SHIM_EDITOR, UPHOLD_SHIM_EDITOR_REAL, and UPHOLD_SHIM_EDITOR_ARGV in addition to UPHOLD_ALLOW before invoking the shim. Match the environment cleanup already used by tests/shim_handoff_cli.rs so tests such as a_body_composed_in_an_editor_makes_the_editor_the_checkpoint exercise the shim rather than an externally configured editor.
327-387: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the raw bytes reached the command.
The doc comment states the bytes pass through untouched. The assertion only checks
faux ran: repo clone, which holds even if the shim replacedcaf\xe9.txtwith a lossy copy. Compare the raw stdout bytes so the case measures what it describes.♻️ Proposed stronger assertion
assert_eq!(code(&output), 0, "{}", stderr(&output)); assert!( - stdout(&output).contains("faux ran: repo clone"), + output + .stdout + .windows(b"faux ran: repo clone caf\xe9.txt".len()) + .any(|window| window == b"faux ran: repo clone caf\xe9.txt"), "{}", stdout(&output) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shim_cli.rs` around lines 327 - 387, Strengthen the first invocation in an_argument_that_is_not_text_reaches_the_command_it_was_typed_for by asserting raw stdout bytes include the exact non-UTF-8 argument caf\xe9.txt, rather than only checking the textual “faux ran: repo clone” prefix. Keep the existing successful-exit and command-routing assertions, and compare stdout as bytes so lossy replacement cannot satisfy the test.src/main.rs (1)
314-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing one exit-ranking predicate with
audit.This block encodes the same rule as
verdictinsrc/audit.rs(Lines 505-513): violations outrank unreadable surfaces, unreadable outranks clean. The comment states the pin guard carries it as well. Three copies of one exit-code contract can drift, and a drift here is a run that reports a different code for the same condition.Extract the predicate once and call it from each site.
♻️ Sketch
- if !failures.is_empty() { - return Ok(Exit::Violations); - } - if !unreadable.is_empty() { - return Ok(Exit::Broken); - } - println!("policy checks passed"); - Ok(Exit::Clean) + let exit = crate::error::verdict(failures.len(), unreadable.len()); + if exit == Exit::Clean { + println!("policy checks passed"); + } + Ok(exit)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 314 - 345, Extract the shared exit-ranking logic that prioritizes violations over unreadable paths and unreadable paths over clean results, then reuse it in this flow and the `verdict` logic in `src/audit.rs` (and the pin guard if it has the same predicate). Preserve the existing `Exit::Violations`, `Exit::Broken`, and `Exit::Clean` outcomes while ensuring all sites use the single shared predicate.Source: Coding guidelines
src/selection.rs (1)
164-173: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Selection::buildrunsgit ls-filesonce per rule.
index_pathsspawnsgit ls-files -zon every call, andbuildis called once for every rule that selects files.Scan::selectcalls it for each rule of each check kind, so a policy with twenty file rules starts twenty full index listings and decodes the whole listing twenty times.The index does not change during one run.
Scanalready resolvesnot_textonce innew; resolve the tracked list the same way and hand it in.♻️ Sketch
- pub(crate) fn build(root: &Path, rule: &Rule, not_text: &[String]) -> Result<Self> { + pub(crate) fn build( + root: &Path, + rule: &Rule, + not_text: &[String], + tracked: Option<&[String]>, + ) -> Result<Self> { let overrides = overrides_for(root, rule, not_text)?; let roots = search_roots(root, rule)?; - // An index if there is one, and a walk only where there is not. - let (files, unreadable) = index_paths(root).map_or_else( - || by_walking(root, &roots, &overrides), - |tracked| from_index(root, &roots, &overrides, &tracked), - ); + // An index if there is one, and a walk only where there is not. + let (files, unreadable) = tracked.map_or_else( + || by_walking(root, &roots, &overrides), + |tracked| from_index(root, &roots, &overrides, tracked), + ); Ok(Self { files, unreadable }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/selection.rs` around lines 164 - 173, Update the scan initialization flow to resolve the tracked index paths once, alongside the existing not_text resolution in Scan::new, then pass that result through Scan::select into Selection::build. Change Selection::build and its callers to accept the resolved tracked list and reuse it when choosing from_index, avoiding repeated index_paths/git ls-files calls for each rule while preserving the walking fallback when no index is available.src/guard/unicode.rs (2)
287-296: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueA path can be reported more than once at pre-push.
scope::blobskeys entries by(path, sha), so a file changed twice in a pushed range yields severalBlobvalues with the samepath.scan_namethen produces the sameFILE NAMEfinding once per sha. Deduplicate the names before scanning so the report names each path once.♻️ Proposed change
let mut findings: Vec<String> = Vec::new(); let mut looked = 0usize; + let mut named: BTreeSet<&str> = BTreeSet::new(); for blob in &blobs { @@ - findings.extend(scan_name(&blob.path, &allowances)); + if named.insert(blob.path.as_str()) { + findings.extend(scan_name(&blob.path, &allowances)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/guard/unicode.rs` around lines 287 - 296, Deduplicate blob paths before invoking scan_name in the scope::blobs processing flow, so repeated (path, sha) entries produce only one filename finding per path. Preserve content scanning for each blob and continue handling gitlinks and contentless blobs as before.
250-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo copies of the binary test disagree on how much they read.
This branch uses
bytes.contains(&0), which scans the whole buffer.src/guard/names.rs(inin_staged) usesbytes.iter().take(8000).any(|byte| *byte == 0), which is git's own rule. The two guards therefore classify the same blob differently when the first NUL sits past byte 8000: this file calls it binary and skips it, while the other file calls it text. Extract one predicate and call it from both places.♻️ Proposed shared predicate
- Err(_) if bytes.contains(&0) => Decoded::Binary, + // git's rule: a NUL in the first 8000 bytes. + Err(_) if crate::guard::scope::looks_binary(bytes) => Decoded::Binary,As per coding guidelines: "If two units of code differ only in a constant, they are one unit with an unextracted parameter."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/guard/unicode.rs` around lines 250 - 258, Extract the shared binary-detection predicate used by the UTF-8 decoding logic around Decoded and the staged-name logic in in_staged, parameterized by the existing 8000-byte limit if needed. Replace bytes.contains(&0) and the local take(8000) check with this predicate so both guards apply git’s first-8000-bytes rule consistently.Source: Coding guidelines
uphold_check.py (1)
483-491: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winA rule body that is not a table resolves to zero stages instead of an error.
If
bodyis not a dict,gitbecomes{}and the rule is recorded with an empty stage set. An empty stage set means "the file scan runs it", so a malformed rule is reported as supplied by the scan. Refuse the shape instead, as[rule]itself is refused two lines above.♻️ Proposed change
for rule_id, body in rules.items(): - git = body.get("git", {}) if isinstance(body, dict) else {} + if not isinstance(body, dict): + raise CouldNotLook(f"policy: [rule.{rule_id}] must be a table") + git = body.get("git", {})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uphold_check.py` around lines 483 - 491, Update the rule-processing loop around `rules.items()` to reject any `body` that is not a dictionary by raising `CouldNotLook`, matching the existing `[rule]` shape validation. Only derive `git`, validate `git.hooks`, and populate `stages[rule_id]` after the rule body passes this check.tests/test_review.py (1)
216-249: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThe symlink branch of
emit_targethas no test.
emit_targethas three refusals: absolute path,..in parts, and a resolved target outsideroot. The third one (a symlinked parent directory) is the only one that is not covered here. A test creatinglink -> <outside dir>and emittinglink/REVIEW.mdwould pin it.Based on learnings: "Authority must be verified on every access to a protected object or effect, on every path that can reach it, and not only on the path the designer had in mind."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_review.py` around lines 216 - 249, Add a test alongside the existing emit path refusal tests that creates an outside temporary directory and a symlink inside the repository pointing to it, then emits a file through that symlink. Assert the review command returns 2, does not create the outside target, and does not report “wrote,” covering the resolved-target containment refusal in emit_target while preserving the existing absolute and parent-traversal cases.Source: Learnings
src/guard/scope.rs (1)
537-541: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe failure message drops git's own stderr.
read_objectreports only the sha and the path.git cat-filewrites the reason to stderr, and this run is exactly the could-not-look case the PR makes fatal. Include the captured stderr so the reader gets the evidence with the refusal.🔍 Proposed change
if !output.status.success() { return Err(Fatal::new(format!( - "git cat-file blob {sha} ({path}) failed" + "git cat-file blob {sha} ({path}) failed: {}", + String::from_utf8_lossy(&output.stderr).trim() ))); }Based on learnings: "A system should emit structured, correlated evidence sufficient to explain relevant state transitions, decisions, dependencies, latency, and failures without reproducing every incident."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/guard/scope.rs` around lines 537 - 541, Update the read_object failure handling around git cat-file to include the captured command stderr in the Fatal message alongside the sha and path. Preserve the existing fatal return behavior while exposing git’s refusal reason to the caller.Source: Learnings
src/guard/names.rs (1)
690-746: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffOne
git diffprocess per changed path.
added_linesspawns a separategit diff --cachedfor every staged path in scope, and the binary-marked branch spawnsrev-parseandcat-fileon top. A commit touching several hundred files therefore runs at the pre-commit stage with hundreds of process launches. The per-path source label is worth keeping, so consider onegit diff --cached -U0 -zpass that is split bydiff --githeader instead, or cache the diff for the whole index and index it by path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/guard/names.rs` around lines 690 - 746, Refactor the staged-file scanning flow around added_lines so one git diff --cached pass serves all in-scope paths instead of spawning a process per path. Cache or parse the whole-index diff, including per-path added-line data, while preserving each source path label and the existing handling for text, binary-marked, deletions, and out-of-scope files; avoid additional per-path rev-parse/cat-file launches where the cached diff can provide the needed content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/REFERENCE.md`:
- Around line 537-539: Update the documented current_exe() handling so an
unresolved executable is treated as a fatal installation failure: emit the
warning, return exit status 2, and do not execute the configured target command.
Remove the warn-and-exec behavior while preserving the existing handling for
other unresolvable targets.
In `@README.md`:
- Around line 119-123: Update the README explanation of tracked-file inspection
to state that an unreadable selected tracked file produces an explicit
“could-not-look” result, includes the affected path, and follows the documented
exit-2 contract; remove the claim that such files are reported clean.
- Line 97: Complete the README command description for “uphold rules
--effective” so it states what every rule inheritance is resolved to, finishing
the sentence with the command’s resulting effective rules.
In `@src/audit.rs`:
- Around line 557-565: The origin fetch and prune currently occur inside history
after reachable_blobs runs; extract that fetch/prune step into its own operation
and invoke it before reachable_blobs in for_publication. In src/audit.rs lines
557-565, place the new fetch step before the reachable-object walk while
preserving the existing --remotes=origin traversal. In src/audit.rs lines
432-497, update history so it no longer owns the fetch/prune, and preserve the
documentation claim that its ref set is provided by the earlier fetch; no other
direct change is required there.
- Around line 371-391: Fix the pipe deadlock in blob_shas by moving the loop
that writes SHA values to the child stdin into a separate thread while the
parent concurrently waits for and reads git cat-file stdout via
wait_with_output. Preserve the existing Fatal error handling for stdin
acquisition and write failures, and ensure the writer thread is joined or its
failure is propagated before returning.
In `@src/config.rs`:
- Around line 1197-1255: Update policy loading and validation around
validate_shims so inherited shims from both inherit.sets and inherit.paths are
either merged into the effective shim collection used by validation and Policy,
or inherited shim declarations are explicitly rejected; do not leave them
silently ignored. Also reject command.before entries whose
split_whitespace().next() is None, aligning Rule::validate with
CommandWhere::matches. Add regression tests covering inherited shims and
whitespace-only command.before entries.
In `@src/pins.rs`:
- Around line 132-160: Update hook_configs to return Result<Vec<PathBuf>> and
iterate walker.build() without flattening, converting traversal errors into
Fatal while preserving discovered paths. Extend filter_entry to prune
Gitlink/submodule directories, including entries identified by a .git file, so
nested repositories are not traversed; update read_pins callers to propagate the
result. Add tests covering unreadable nested directories and initialized
submodules.
In `@src/selection.rs`:
- Around line 47-95: Update not_text_paths to preserve and return a distinct
failure result for git check-attr spawn, pipe, read, and non-success exit errors
instead of converting them to Vec::new(). Adjust its caller to distinguish
command failure from a successful empty declaration result and report the
failure using the existing Selection unreadable pattern.
In `@src/shim.rs`:
- Around line 842-863: Update replayed and the non-Unix hand_off flow so
temporary replay files are not left on disk: either restrict unlink-before-use
behavior to Unix and explicitly remove the path after the child status returns
on non-Unix, or gate the temp-file replay to Unix. Preserve the existing replay
behavior and ensure cleanup occurs even after the spawned command completes.
- Line 471: Update the visibility handling around the self.visibility(root,
&target) match to parse the GitLab response as JSON and read the top-level
visibility field, rather than using json_string_field. Preserve the existing
public-target decision behavior while correctly handling nested fields and
escaped quotes.
In `@tests/shim_handoff_cli.rs`:
- Around line 105-111: The test harness requires portable handling of
platform-specific utilities: in tests/shim_handoff_cli.rs lines 105-111, update
the guarded command setup to use timeout only when available and otherwise
invoke the uphold binary directly; in tests/shim_handoff_cli.rs lines 182-203,
pipe wc -c output through tr -d ' ' so the stub emits an unpadded count matching
body.len().
In `@uphold_check.py`:
- Around line 549-571: Validate every entry in inherit.sets and inherit.paths
instead of silently filtering non-string values; when an invalid entry is found,
raise CouldNotLook with CONTENT_POLICY context and evidence identifying the
offending declaration/value, preserving the existing exit-2 handling used for
missing inherited files.
- Around line 410-417: Update the remote-handling branch in the relevant check
function so the all-stage credit is granted only when the same remote entry
identifies the intended repository and the selected configs path is
hooks/lefthook.yml. Do not rely solely on lefthook_remote().search(text);
validate both conditions together before setting scan, merging guards, and
appending the remote message.
---
Outside diff comments:
In `@src/pins.rs`:
- Around line 405-410: Update the SHA handling in the pin validation flow around
looks_like_a_sha so SHA-shaped revisions are resolved against the remote before
bypassing stale checks. Only continue without further tag checks when the object
is confirmed present; if remote resolution cannot establish the object’s
existence, return exit code 2.
In `@src/shim.rs`:
- Around line 231-327: Update collect_flags to stop scanning options when it
encounters the "--" terminator, matching verb_noun’s argv grammar. Once "--" is
reached, do not inspect or consume subsequent positional arguments as flags or
values, including preventing stdin handling and body_given updates from those
arguments.
---
Nitpick comments:
In `@src/guard/names.rs`:
- Around line 690-746: Refactor the staged-file scanning flow around added_lines
so one git diff --cached pass serves all in-scope paths instead of spawning a
process per path. Cache or parse the whole-index diff, including per-path
added-line data, while preserving each source path label and the existing
handling for text, binary-marked, deletions, and out-of-scope files; avoid
additional per-path rev-parse/cat-file launches where the cached diff can
provide the needed content.
In `@src/guard/scope.rs`:
- Around line 537-541: Update the read_object failure handling around git
cat-file to include the captured command stderr in the Fatal message alongside
the sha and path. Preserve the existing fatal return behavior while exposing
git’s refusal reason to the caller.
In `@src/guard/unicode.rs`:
- Around line 287-296: Deduplicate blob paths before invoking scan_name in the
scope::blobs processing flow, so repeated (path, sha) entries produce only one
filename finding per path. Preserve content scanning for each blob and continue
handling gitlinks and contentless blobs as before.
- Around line 250-258: Extract the shared binary-detection predicate used by the
UTF-8 decoding logic around Decoded and the staged-name logic in in_staged,
parameterized by the existing 8000-byte limit if needed. Replace
bytes.contains(&0) and the local take(8000) check with this predicate so both
guards apply git’s first-8000-bytes rule consistently.
In `@src/main.rs`:
- Around line 314-345: Extract the shared exit-ranking logic that prioritizes
violations over unreadable paths and unreadable paths over clean results, then
reuse it in this flow and the `verdict` logic in `src/audit.rs` (and the pin
guard if it has the same predicate). Preserve the existing `Exit::Violations`,
`Exit::Broken`, and `Exit::Clean` outcomes while ensuring all sites use the
single shared predicate.
In `@src/selection.rs`:
- Around line 164-173: Update the scan initialization flow to resolve the
tracked index paths once, alongside the existing not_text resolution in
Scan::new, then pass that result through Scan::select into Selection::build.
Change Selection::build and its callers to accept the resolved tracked list and
reuse it when choosing from_index, avoiding repeated index_paths/git ls-files
calls for each rule while preserving the walking fallback when no index is
available.
In `@tests/audit_publication_cli.rs`:
- Around line 156-175: Update the test
the_standing_caveat_is_stated_without_being_counted_as_unread() to explicitly
assert that report contains the "could NOT be read:" section header before
calling split_once(...). Preserve the existing measured-content assertion while
replacing the opaque unwrap path with a clear failure if the header is absent.
- Around line 57-59: Update the fixture setup around git initialization in the
test to explicitly disable inherited signing and hooks-related configuration,
and prevent global init templates from installing hooks. Configure these
settings locally for the fixture before commits run, while preserving the
existing user.name and user.email setup.
In `@tests/fixtures/test_promotion_corpus.py`:
- Line 120: Update the corpus matching logic around the test’s re.compile call
to use the same production regex matcher and compilation behavior as uphold
scan, rather than Python’s re engine. Ensure every corpus pattern is validated
against the Rust-compatible engine so unsupported lookarounds or backreferences
fail consistently with production.
In `@tests/shim_cli.rs`:
- Around line 192-230: Update the shared shim() test helper to remove
UPHOLD_SHIM_EDITOR, UPHOLD_SHIM_EDITOR_REAL, and UPHOLD_SHIM_EDITOR_ARGV in
addition to UPHOLD_ALLOW before invoking the shim. Match the environment cleanup
already used by tests/shim_handoff_cli.rs so tests such as
a_body_composed_in_an_editor_makes_the_editor_the_checkpoint exercise the shim
rather than an externally configured editor.
- Around line 327-387: Strengthen the first invocation in
an_argument_that_is_not_text_reaches_the_command_it_was_typed_for by asserting
raw stdout bytes include the exact non-UTF-8 argument caf\xe9.txt, rather than
only checking the textual “faux ran: repo clone” prefix. Keep the existing
successful-exit and command-routing assertions, and compare stdout as bytes so
lossy replacement cannot satisfy the test.
In `@tests/test_review.py`:
- Around line 216-249: Add a test alongside the existing emit path refusal tests
that creates an outside temporary directory and a symlink inside the repository
pointing to it, then emits a file through that symlink. Assert the review
command returns 2, does not create the outside target, and does not report
“wrote,” covering the resolved-target containment refusal in emit_target while
preserving the existing absolute and parent-traversal cases.
In `@uphold_check.py`:
- Around line 483-491: Update the rule-processing loop around `rules.items()` to
reject any `body` that is not a dictionary by raising `CouldNotLook`, matching
the existing `[rule]` shape validation. Only derive `git`, validate `git.hooks`,
and populate `stages[rule_id]` after the rule body passes this check.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 318e6618-5df3-4f1f-b2f3-2d21b60f0cb3
📒 Files selected for processing (38)
.github/workflows/test.yml.pre-commit-config.yamlAGENTS.mdREADME.mdREVIEW.mddocs/DESIGN.mddocs/REFERENCE.mdhooks/lefthook.ymllefthook.ymlpolicy/base/unmanaged-pins.tomlpolicy/upheld.tomlscripts/check_hook_pins.pyscripts/consumer_check.shsrc/audit.rssrc/config.rssrc/guard/names.rssrc/guard/scope.rssrc/guard/unicode.rssrc/main.rssrc/pins.rssrc/report.rssrc/scan.rssrc/selection.rssrc/shim.rssrc/text.rstests/audit_publication_cli.rstests/fixtures/__init__.pytests/fixtures/promotion-corpus.jsontests/fixtures/test_promotion_corpus.pytests/guard_recovered_halves.rstests/hook_pins_cli.rstests/scan_cli.rstests/shim_cli.rstests/shim_handoff_cli.rstests/test_hook_pins.pytests/test_review.pytests/test_uphold_check.pyuphold_check.py
💤 Files with no reviewable changes (4)
- REVIEW.md
- AGENTS.md
- scripts/check_hook_pins.py
- tests/test_hook_pins.py
Comment on lines
+537
to
+539
| Where `current_exe()` cannot be resolved there is nothing to install as the | ||
| editor. That case warns loudly and execs anyway, the way an unresolvable target | ||
| does: a guard that becomes the reason work stops is a guard that gets removed. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail when editor re-entry cannot be installed.
If current_exe() cannot resolve, the shim cannot inspect editor-authored text. The documented warning-and-exec path can publish that text without running the configured checkers. Return exit 2 before executing the target command.
As per coding guidelines: “When continuing cannot satisfy the contract safely, detect the condition at the earliest reliable boundary and return an explicit failure with evidence.” Based on learnings: “Authority must be verified on every access to a protected object or effect, on every path that can reach it.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/REFERENCE.md` around lines 537 - 539, Update the documented
current_exe() handling so an unresolved executable is treated as a fatal
installation failure: emit the warning, return exit status 2, and do not execute
the configured target command. Remove the warn-and-exec behavior while
preserving the existing handling for other unresolvable targets.
Sources: Coding guidelines, Learnings
Comment on lines
+119
to
+123
| meaning what it meant. "Its own files" is **what git tracks**, not a directory | ||
| walk: a tracked file some ignore pattern also matches is still pushed and still | ||
| cloned, and a rule that cannot see a file reports it clean. `--text -` runs it | ||
| over prose that never becomes a file. `uphold rules --effective` prints what | ||
| inheritance actually resolved to, so nothing has to re-derive it. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe unreadable tracked files as could-not-look.
Line 121 says that a rule that cannot inspect a file “reports it clean.” This contradicts the exit-2 contract in Lines 89-90. If the scan cannot inspect a selected tracked file, document an explicit could-not-look result and the affected path.
As per coding guidelines, “When continuing cannot satisfy the contract safely, detect the condition at the earliest reliable boundary and return an explicit failure with evidence.”
🧰 Tools
🪛 LanguageTool
[grammar] ~121-~121: Use a hyphen to join words.
Context: ...hat cannot see a file reports it clean. --text - runs it over prose that never becomes ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 119 - 123, Update the README explanation of
tracked-file inspection to state that an unreadable selected tracked file
produces an explicit “could-not-look” result, includes the affected path, and
follows the documented exit-2 contract; remove the claim that such files are
reported clean.
Source: Coding guidelines
`blob_shas` wrote every reachable object's sha into `git cat-file --batch-check` and only then waited, with nothing reading stdout. `--batch-check` answers as it reads, at roughly fifty bytes an answer, so it fills a 64 KiB pipe somewhere near the thirteen-hundredth object and stops reading stdin -- and every repository this audit is for is far past that count. Writing first was not a rare hang; it was the ordinary case, and `audit --for-publication` could not finish on a real tree. The same defect with the same shape was fixed in `selection` earlier in this branch, where it carries a comment explaining exactly this failure. It was reintroduced here two files away. Both pipes now move at once. `git fetch --prune origin` also moves out of `history` and runs before anything reads a ref. It sat inside the function that runs third, so the object walk read a ref set that had not been fetched and had not been pruned: an object the forge holds but this clone never saw was missed, and a branch deleted upstream was still walked as something served.
Two defects in one walk, both of them the shape this branch exists to remove. `walker.build().flatten()` dropped every `Err` the walk yielded, so a directory that could not be entered hid whatever hook configuration was inside it and the guard reported the pins it did manage to read as the whole answer. A walk that did not finish is a could-not-look, and it now exits 2 naming the directories. The walk also entered submodules. `filter_entry` excluded the NAME `.git`, which is a directory in an ordinary checkout and a FILE in an initialized submodule -- so excluding the file left the directory around it perfectly traversable. Every pin in every submodule was read, asked about over the network, and reported against a repository that does not own it. A `.git` file is what git itself calls the boundary.
`not_text_paths` returned an empty list when the spawn failed, when the pipes were missing, when the drain failed, and when the child exited non-zero -- the same answer it gives for a repository that declares no `-text` attribute at all. The caller could not tell them apart. The consequence is an invented finding rather than a missed one, since a declared binary file stops being excluded and an `encoding` or `allowed_scripts` rule then reports on bytes nobody wrote as text. That is the safe direction to fail in, and it is still a claim about a question this tool never got an answer to. `index_bytes`, twenty lines below, carries a comment saying `None` and an empty list must not fold together; this was the same fold in the same module. The reason travels to `Scan`, which already collects unreadable surfaces and already exits 2 on them, so the reader is told which of the two answers they are holding.
Where `current_exe()` could not be resolved there was nothing to install as the command's editor, and `install_editor` printed a warning ending "This is not a pass" and returned -- after which the caller execed the command anyway. So the one path the editor re-entry exists to close stayed open, and a run that could not check the text still published it, at exit 0, while saying it was not a pass. There is no safe way to continue here. The body does not exist yet, so it cannot be checked now, and after the hand-off there is no process left to check it later. It exits 2. Two more in the same file. Visibility is now read out of a parsed document rather than found by scanning for a quoted name. `json_string_field` answered with the first textual occurrence of the key anywhere -- nested, or inside a string value, or in a description quoting the word -- and a `public-target` decision was made on it. YAML 1.2 is a superset of JSON, so the parser already in the dependency tree reads a forge response without adding one. And the stdin replay splits by platform. Unlink-on-open is a Unix property: Windows refuses to remove a file while a handle is open, and that branch spawns rather than execs, so the temp file survived the run holding the exact body the command published, in a directory every account on the machine can read. The branch that does not exec has a process to write a pipe with, and now uses one. The handoff tests no longer assume GNU coreutils: `timeout` is absent on macOS, and BSD `wc -c` pads its count, either of which fails the suite for the environment rather than for the shim.
`load` merges only `.rules` from an inherited file, so an inherited `[[shim]]` was dropped -- and dropped in the worst available way, because the `exec` rule that arrived with it survived and `validate_shims` then reported that rule's `command.before` as naming a shim nobody declared. The author was told to declare the shim they had in fact declared, in the file they were pointing at. Merging is the other answer and it is not obviously right: a shim stands in front of a real command, and inheriting one puts a program in front of `git` on the strength of a path in an `[inherit]` line. Until that is a decision somebody makes on purpose, it is refused and says so. A `command.before` entry with no command in it is refused too. `[" "]` parses, and the set of checked commands is built from `split_whitespace().next()`, which answers `None` for it -- so the entry fell out of the check and took the rule's whole reason for existing with it. `CommandWhere::matches` can never match it either, so there is no reading of a blank entry that does anything at all.
A non-string entry in `inherit.sets` or `inherit.paths` was filtered out in silence, which makes this reconcile resolve FEWER rules than the engine reading the same file. A claim on one of the rules that went missing then failed as "no seam here supplies it" -- exit 1, which in this tool means the claim is false. It is not false; nobody looked. It now exits 2, the same answer a missing inherited file already gets a few lines below. `lefthook_remote` was one pattern alternating between this repository's slug and the string `hooks/lefthook.yml`, so either half alone matched. A fork, a mirror, or an unrelated project following the same conventional filename was credited with running every guard published here, because the branch it feeds grants all of them. Both halves must now appear in the SAME `remotes:` entry, which means reading the block by indentation rather than scanning it for a needle.
Two lines contradicting the contract stated thirty lines above them. README said a rule that cannot see a file "reports it clean", which was true of the walk it described and is now the defect this branch removed; it also left a command description ending mid-clause at "resolved to". REFERENCE still described the editor path as warning and exec'ing where `current_exe()` cannot be resolved. It refuses now, and the reason is worth stating: the argument that a guard which stops work gets removed belongs to a guard that looked and found nothing, not to one that never looked.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/audit.rs`:
- Around line 118-131: Change refresh_origin to return an optional
refresh-failure note that covers both git launch errors and non-zero fetch
statuses, instead of discarding the result. In the caller before reachable_blobs
and history execute, add any returned note to unreadable while preserving the
partial scan; ensure the final exit is Exit::Broken when this unreadable
condition exists, unless a violation already takes precedence.
In `@uphold_check.py`:
- Around line 619-620: Update the inherit-validation logic alongside names and
relatives to validate inherit.disabled_rules with _string_list, using the
appropriate field label. Ensure malformed non-string values fail explicitly with
the existing exit-2/error path before reconciliation, and remove any later
silent filtering that bypasses this validation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 19a1f2e1-39ba-43b0-99a9-e23a5205c523
📒 Files selected for processing (11)
README.mddocs/REFERENCE.mdsrc/audit.rssrc/config.rssrc/pins.rssrc/scan.rssrc/selection.rssrc/shim.rstests/shim_handoff_cli.rstests/test_uphold_check.pyuphold_check.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/selection.rs
- tests/shim_handoff_cli.rs
- src/pins.rs
- src/shim.rs
`refresh_origin` returned nothing and discarded both a launch error and a non-zero exit, while the comment directly above it said "What must not happen is a silent success, and the caller reports the staleness instead." The caller did no such thing: every reading after it went on against whatever refs the clone happened to hold, and the audit could still answer clean about a forge it never reached. It comes back as a note in `unreadable` now, which is what turns the run into an exit 2. The partial audit is still worth having offline, and it is no longer worth mistaking for a complete one. `inherit.disabled_rules` gets the same validation its two neighbours got in the previous commit. It was left filtering non-strings in silence, and it is the list where dropping an entry is worst: the engine refuses a `disabled_rules` id that names nothing inherited, so an entry that vanishes here is a load failure over there, and this tool would be reconciling a policy the binary will not accept.
Requiring `owner/name` in a `remotes:` entry rejected every git url that does not spell one, and lefthook takes any git url: a clone by filesystem path, by ssh, or from a mirror is still a remote naming this repository, and it carries no owner to check against. `scripts/consumer_check.sh` points its consumer at the checkout under test, which is a path -- so the one CI job that drives a real lefthook consumer refused a clean commit, reporting `prevent-ai-author` as a rule no seam here supplies. It is supplied; the remote naming the seam was not recognised. The bare repository name is accepted where the slug is absent. It is the weaker half and it is not the one doing the work: what the previous commit fixed, and what still holds, is that the repository and the config must appear in the SAME entry rather than either one alone.
The test was "does this url name us", and most git urls cannot answer it. lefthook takes any git url, so a consumer may clone this repository from a filesystem path or a mirror whose name says nothing -- and `scripts/consumer_check.sh` does exactly that on purpose, re-cloning to a neutral `$WORK/hooks` so that no home path reaches a file the consumer's own content policy then reads. The url it writes carries neither the owner nor the repository name. Demanding a slug there demanded evidence the format does not carry, and answering "no seam here supplies it" is exit 1: the claim is false. The claim was true. The one CI job that drives a real lefthook consumer refused a clean commit for it. A remote is rejected now only when it is identifiably somebody else's -- it spells a forge `owner/name` and the pair is not ours. A url with no host is a path, and a path is unidentifiable rather than foreign. What the previous commit fixed still holds: the remote and the config must appear in the SAME entry, so a fork pinning its own config is still not credited with running every guard here. The new test also stops writing a realistic runner workspace path. This repository's own `no-running-os-identity-metadata` rule reads the running home path and searches the tracked files for it, so `/home/runner/...` passed on a developer's machine and refused the scan on every CI runner -- which is the rule working, on a fact the test invented.
The comment added with the previous fix quoted the very path it was warning about. `no-running-os-identity-metadata` reads the running home directory and searches the tracked files for what it read, and it does not care whether the string it finds is test data or prose about test data -- so the explanation refused the scan on every CI runner exactly as the data had, and passed everywhere else exactly as the data had. Verified this time against the path CI actually runs under rather than against the one this machine has, which is the check the first fix skipped.
`KNOWN_PUBLIC_IDENTITY` has held `runner` since it was written, and the username needle has consulted it since then. The home path needle consulted nothing, so one account name was a leak spelled as a path and not as a name -- and since both needles are read from the environment the scan runs in, the practical effect was a tree that passed on every developer's machine and refused on every CI runner, reported as identity metadata about a string that identifies nobody. The one place the gate is authoritative was the one place it was wrong. The account is now taken off the home path and asked the same question the username is asked. The list it is asked against grows to the shared accounts other providers and images use, which are the same fact under other names. This does not weaken the neighbouring rule and is not meant to. A home path that will not exist on the next machine is `no-hardcoded-home-paths`, whose subject is reproducibility rather than identity -- it still refuses every literal home path, including a runner's, including in the test added here, which assembles its fixtures for exactly that reason.
The promotion corpus planted the ambient HOME and asserted the rule refused it, which made the assertion depend on whose machine ran the suite. A hosted runner's home belongs to a shared build account, and `KNOWN_PUBLIC_IDENTITY` deliberately does not read that as anybody's identity -- so the rule correctly did not fire, and the corpus read the correct answer as the rule having stopped working. It sets HOME for the scan now rather than inheriting it, and plants a personal home. That is the question the set was promoted to answer, and it is now the same question on every machine: the test passes under a runner's home and under a developer's, where before it could only pass under one of them at a time.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audit.rs (1)
601-611: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject literal owners from every matching rule.
Lines 601-611 aggregate owners from every matching rule. The disclosure guard at Line 650 still checks only
rule, which is the first matching rule. If a later rule contains literalprivate_owners, the audit scans for those names but does not refuse to publish the policy that exposes them.Iterate over
ruleswhen creating literal-owner refusals. Add an audit test where only a non-first matching rule contains literal owners.Proposed fix
- if !rule.private_owners().is_empty() { - refusals.push(Refusal { - id: rule.id.clone(), + for candidate in &rules { + if candidate.private_owners().is_empty() { + continue; + } + refusals.push(Refusal { + id: candidate.id.clone(), report: format!( "the rule declares {} private owner(s) literally, in a file this flip \ would publish. A public repository cannot hold the list of what must not \ be published. Move them out with `private_owners_from = \"...\"`, a \ command whose stdout is one owner per line, and keep the rule committed \ without the names.", - rule.private_owners().len() + candidate.private_owners().len() ), }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audit.rs` around lines 601 - 611, Update the disclosure guard near the existing literal-owner refusal logic to inspect every matching rule in rules, not only the first rule variable. Create refusals for literal private_owners found in any rule while preserving the aggregated owners behavior in the surrounding audit flow, and add a test covering literal owners present only in a non-first matching rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/audit.rs`:
- Around line 601-611: Update the disclosure guard near the existing
literal-owner refusal logic to inspect every matching rule in rules, not only
the first rule variable. Create refusals for literal private_owners found in any
rule while preserving the aggregated owners behavior in the surrounding audit
flow, and add a test covering literal owners present only in a non-first
matching rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31baa006-4b61-43cc-a515-43b30647e5b2
📒 Files selected for processing (5)
src/audit.rssrc/sources.rstests/scan_cli.rstests/test_uphold_check.pyuphold_check.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_uphold_check.py
- uphold_check.py
The owner LIST was taken off all three `no-private-repo-names` variants forty lines earlier, for the reason written there: the variants carry different fields, and which one a policy file lists first is not a decision anybody makes. The disclosure refusal went on reading `rules.first()`. So a name written literally into the second or third variant was handed to the scan as something to look for, in a file the audit then declined to object to -- the audit hunting for a name it had just been given, in the place it was given it. The one surface the flip publishes first is the policy file itself. The test appends the literal-carrying variant deliberately after the one that has none, and fails against the previous code.
This was referenced Aug 12, 2026
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.
Addresses the defect findings in #2, sections A through I. Every
one of them is the same shape: a check that could not look reporting a pass. The
work is grouped below by seam, one commit each.
What is deliberately NOT here
Section J of #2 -- the proposal to bundle the guards as base
sets -- is not in this pull request, and not because it was overlooked. It is a
design change affecting 65 consumer repositories, with a five-step ship order in
which the order is the design: the hand-copy detector first, the derived-owner
note second, the four guard sets third, the shadow warning and the set provenance
in the same release as the sets. The issue states plainly that shipping step 3
while steps 2 and 4 slip leaves the fleet worse off than doing nothing, because
two forked reimplementations become permanently hidden behind a set they silently
shadow. That is a sequenced design programme, not a defect fix, and folding it
into a pull request whose subject is exit-code discipline would put the two under
one review and one revert.
The behaviour changes below that consumers will notice are listed at the end.
Selection: what a content rule looks at
uphold scanwalked the tree and honoured every ignore file it found. Git ignorerules do not apply to a file git already tracks, so a tracked file matched by a
.gitignoreline, a.git/info/excludeentry, or the operator's own globalignore file -- which is not in the repository at all -- was searched by no content
rule and reported clean. The globs now apply to
git ls-files. Where there is noindex to read, the walk consults no ignore file at all, which selects a superset:
over-reporting is the direction a checker may fail in.
Walk errors were dropped with
.flatten(), so a subtree the process could notenter left a half-read tree looking like a small repository. A path a rule could
not open is now collected, named on stderr with its cure, and turns exit 0 into
exit 2 after every rule has reported -- collected rather than fatal, so a tree
with one unstaged deletion still gets an answer for every other rule. A finding
outranks it, as in
auditand the pin guard.A
files.includenaming a path outside the repository is refused at load. Such aroot selects files with no repository-relative name, so every hit was dropped on
the way out and the rule reported a pass over a search that had found things.
Hangs and panics
git check-attr --stdinwas written to and then read from, in that order, whichdeadlocks whenever the answer exceeds a pipe buffer: 3000 tracked paths is 150 KiB
in and 200 KiB out against a 64 KiB pipe, and the check hangs with no output and
no exit code. The same shape in the shim's checker consultation is fixed the same
way. Both are covered by tests that fail on a deadline rather than hanging the
suite with no failure named.
std::env::args()panics at exit 101 on an argument that is not UTF-8, out of abinary installed in front of
gitexactly where a latin-1 file name gets typed.argv is read and carried as bytes now; the shim compares against a lossy copy and
hands the exec the originals, and an invocation whose text is actually checked
refuses the untranslatable argument with a sentence rather than checking U+FFFD.
dedentcounted its common indent in bytes and sliced on it, which panicked on apage whose indentation was not all ASCII.
Root and scope resolution
The policy walk climbed past a repository's own root into an enclosing
superproject, loaded that policy and adopted the superproject's directory as the
scan root -- so the report named files outside the repository the command was run
in. It stops at the first
.git, directory or file, and the boundary is testedafter the policy lookup so a repository root carrying its own policy is still
found.
--policy PATHasserts the<root>/policy/<name>.tomllayout instead oftaking the file's grandparent unchecked.
Guards: what a commit and a push actually publish
A file NAME is published exactly as its contents are. The tree-wide guards read
the path as well as the blob, so a private repository's name in a directory name
is a finding; a tab or newline, legal in a file and never in a path, is a finding
there. At a push the guards also read the commit messages the push publishes,
which is the one surface no earlier seam reaches for a commit written under
--no-verify.The staged half had three ways to see nothing: a
diff.externalor textconvdriver emptied
git diff --cached, a committed* -diffattribute hid aplain-ASCII file, and a rename introduced a path while adding no line. All three
are closed, and each source is now one path rather than one blob labelled "staged
changes" -- which is also what makes
[rule.files]finally bound the stagedguard.
A submodule ended both tree-wide guards before they started:
git cat-file blobcannot read a gitlink, so any tree with one exited 2. Gitlinks are enumerated by
path and never read.
A blob that will not decode is exit 2 rather than a silent skip, and so is
scan --textover bytes that are not UTF-8, wherefrom_utf8_lossyhad printed"policy checks passed" over text nobody read.
Shim: transparency of the passthrough
The shim now ends in a real
exec, so pid, process group, terminal control anddeath by signal survive instead of being flattened to exit 1. The stdin a
-F -invocation consumed is handed back to the command as a descriptor, because a
guard that silently eats the body it approved publishes an empty one. The
subcommand is found by walking argv for the first two words that are neither an
option nor an option's value, so
gh --repo owner/name issue createis examinedrather than exec'd unseen. A checker that exits 0 without draining the subject is
exit 2 rather than a pass over the part it read.
The editor case is closed rather than warned about: the shim installs itself in
the command's declared editor variable, runs the real editor, and checks what the
editor leaves in the file when it closes -- exiting 1 there is what makes
ghandglababandon the publication.forge_visibilityasked GitHub about a GitLab project. Visibility is nowdispatched on the shim's command, and on the origin remote's host otherwise.
Audit surface
audit --for-publicationcould never exit 0: an always-true note about commentedit history was pushed into the unreadable list, so the clean branch was dead
code while
docs/REFERENCE.mddocumented it. It is a standing caveat now, statedin every report and not counted as a surface this run failed to read.
It scanned
HEAD's tree, but a name committed and deleted beforeHEADis servedby the forge forever and survives a rewrite of the default branch; it now reads
every reachable blob. On the forge side it read bodies but not titles -- the field
the
ghshim guards with-t-- and never asked for review bodies orreview-thread comments at all.
--limit 200silently truncated; the cap is 5000and a listing that returns at exactly the cap is reported as truncated.
Pins: one question, one answer
Two checkers read the same
rev:lines and were free to disagree, and did: theguard counted a pin whose remote it could not reach as passed while
scripts/check_hook_pins.pycalled the same pin unresolvable. Onegit ls-remoteanswers both questions now and the script is gone. lefthook
remotes:entries arepins, configs below the root are read, an absent
.pre-commit-config.yamlis ananswer rather than an ENOENT, and a config with no
repos:key is could-not-lookrather than zero pins.
Reconciler
Evidence that a rule runs came from the
- id:lines a repository pinned ratherthan from the entries that actually run something, so pinning the reconciler
itself proved that content rules ran. Evidence is read from
entry:lines now,the stage-to-id map is derived rather than written down, and a rule is supplied
only where a seam it declares is installed.
[inherit]has three fields and thereader saw one, so every rule arriving through
inherit.pathswas invisible and atrue claim on one was refused.
uphold rules --effective [--json]is new, and exists so nothing has tore-implement
config::loadto find out what a repository runs. The reconcilerdoes not call it yet -- it is the hook other repositories install, and two of the
three runners keep the binary inside their own environment directory rather than
on PATH -- so a test fails when the two readers disagree about this repository's
own policy.
Configuration: fields read by nothing
command.*on a non-execrule, anexecrule with an emptycommand.before, a[[shim]]no checker names, and acommand.beforeentry no[[shim]]declaresare all refused at load. Each of them is silence at run time, so load is the only
place they can be said.
CI and the consumer harness
Three published hook ids were pinned by the consumer harness and reached by no
question it asked, so nothing had ever run them. The harness asks eight questions
now; the three new ones edit the declaration in a commit, make a real
--no-ffmerge carrying a zero-width space, and run the manual stage per runner. The
runner-parity job installed a Rust toolchain for all three runners, which is
exactly what the pre-commit and prek legs exist to prove is unnecessary; the
toolchain is gated on the lefthook leg.
Rules that did not survive the port
no-pinned-tool-installmissedcargo install --git <url> --tag vX.Y.Z, which isthe one cargo line with nowhere else to put its pin, and a lefthook
remotes: ref: vX.Y.Z, which is the twin of a pre-commitrev:except thatnothing moves it automatically. The promotion corpus is restored with a parity
test, so a future edit cannot narrow a promoted pattern back to the shape one
repository happened to have.
Behaviour changes a consumer will notice
no-stale-hook-pinsexits 2 on a remote it cannot reach, so a runner with nonetwork fails this guard where it used to pass it.
UPHOLD_ALLOW=no-stale-hook-pinsis the deliberate bypass and the refusal says so.
gets exit 2 from every entry point instead of silently borrowing.
uphold scanreads only what git tracks, so an untracked file is no longersearched.
after a partial read is now exit 2.
command.beforerule needs a[[shim]]beside it, and the reverse.Verification
cargo build,cargo test(247 tests across nine suites),cargo clippy --all-targets --all-featuresunder the crate's own pedantic/nursery/cargoprofile,
cargo fmt --check, andpython3 -m pytest tests/(108 tests, 224subtests) are all green.
uphold scananduphold guard --stage pre-commitpassagainst this repository, which self-hosts its own policy, and every commit ran the
full pre-commit hook set.
Summary by CodeRabbit
New Features
rules --effectiveto inspect active rules, inheritance, paths, disabled rules, and Git hook coverage.Bug Fixes
Documentation