Four seams that reported a pass over something they never looked at - #4
Conversation
`keep_blobs` wrote every candidate sha into `git cat-file --batch-check` and only drained stdout afterwards. `--batch-check` answers each object as it reads it, so it fills its 64 KiB stdout pipe somewhere past the fifteen-hundredth and stops reading stdin; the parent is then blocked writing to a full stdin pipe while the child is blocked writing to a stdout pipe nobody is draining. Neither moves again -- no output, no exit code, `git push` simply stops. Measured boundary: 1,503 objects completes, 2,003 hangs, and the guards that run at pre-push are the tree-wide ones every repository past that size reaches. `audit::blob_shas` and `selection::not_text_paths` each grew a writer thread to avoid exactly this, and audit.rs carries a comment naming the thirteen-hundredth object. Two of the three call sites learned it and the third kept the version that hangs, which is the argument for one copy rather than a third fix: the pumping, the closed stdin and the exit-status refusal now live in `git::blob_shas`, and both callers ask it. Covered by `git::tests::several_thousand_objects_are_asked_about_without_deadlocking`, which asks about 4000 objects on a thread with a deadline -- a test that proves a deadlock is gone has to fail when it is not, and one that hangs names nothing.
`message_text` never consulted the stage, so `prevent-ai-author` and `prevent-unusual-unicode` read `.git/COMMIT_EDITMSG` at every stage they run at. Under `git commit` that is the right file, which is what made the mistake survivable and therefore permanent. At `pre-push` it is whatever the last commit happened to write -- usually clean, and never the commits being pushed. So a marker recorded by `git commit-tree`, a rebase, a cherry-pick, `git am`, `--no-verify`, or a fast-forward carrying somebody else's commit in from a hookless clone reached the remote with the guard reporting `1 guard(s) passed`, exit 0. It failed in the other direction too: a stale `COMMIT_EDITMSG` left by a refused attempt refused a push that published nothing wrong. `no-private-repo-names-in-files` already reads the pushed range for this exact reason, and `scope::pushed_messages` is the function it uses. These two guards were the ones still asking the wrong file; they ask that one now, and a finding names the commit rather than a path the guard never opened. Three tests, each failing before this: a marker in a pushed commit behind a later clean one, an invisible character in the same shape, and the inverse where a stale `COMMIT_EDITMSG` must not refuse a clean push.
`overrides_for` handed each `git check-attr` answer to the glob builder as
`!{path}`. The globs above it are author-written and their metacharacters are
meant; these are literal filenames, and three different characters turned one
into something else.
`data{1,2}.bin` became an alternation, so `data1.bin` and `data2.bin` -- neither
declared anything -- were removed from every content rule in the policy and
named in no report. `page[1].html` became a character class that does not match
its own literal name, so a file declared not-text was searched AND listed as
skipped in the same output, which is the invented finding this module's own doc
comment says must not happen. `capture[1.bin` is an unclosed class: a parse
error that took the whole run to exit 2 with no rule having reported anything.
`globset::escape` before the `!`. Two tests, one per direction: that a declared
path with metacharacters excludes only itself, and that an unclosed class in a
filename is not a malformed glob.
`[rule.files]` on a guard built-in is the supported way to narrow one -- `guard::scope::in_file_scope` reads it, and `config` exempts built-ins from the refusal that would otherwise reject the keys. The scan aborted on it anyway, returning exit 2 for the whole repository with a diagnosis that was not true of the rule it named: the keys are not read by nothing, they are read at the other seam. Scoping one guard switched off every content rule in the policy. `git.hooks` answers which seam owns the rule. A guard built-in that names a hook runs there and this scan is not its seam to fail from; one that names no hook is run by nothing at either seam, so its `files.*` really is read by nothing and the refusal stands -- passing over that would report a check that did not happen as one that did. Both sides are tested: a scoped guard beside a pattern rule now reports the pattern rule's finding, and a guard built-in with `files.*` and no hook is still exit 2.
📝 WalkthroughWalkthroughThe change centralizes Git blob lookup, expands message guards to pre-push commit ranges, updates scan handling for hook rules, and escapes not-text paths before creating exclusions. Documentation and regression tests cover the updated behavior. ChangesGuard and scanning behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PrePushHook
participant message_subjects
participant PushedCommits
participant MessageGuards
PrePushHook->>message_subjects: collect messages for pushed refs
message_subjects->>PushedCommits: read commit messages
PushedCommits-->>message_subjects: return labeled messages
message_subjects-->>MessageGuards: provide selected messages
MessageGuards-->>PrePushHook: return first refusal or success
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (93.44%) 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 #4 +/- ##
==========================================
+ Coverage 86.01% 87.19% +1.17%
==========================================
Files 22 22
Lines 6527 6605 +78
==========================================
+ Hits 5614 5759 +145
+ Misses 913 846 -67 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/selection.rs (1)
676-714: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the closed character-class case.
This test declares only
data{1,2}.bin. The comments describepage[1].html, but the test does not create or assert that path. Addpage[1].htmland a near-match such aspage1.html. Verify that only the declared path is excluded.🤖 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 676 - 714, Extend the test a_not_text_path_holding_glob_metacharacters_excludes_only_itself to create page[1].html as a declared not-text path and page1.html as an undeclared near-match, then assert the declared literal path is excluded while page1.html remains selected. Keep the existing data{1,2}.bin and other undeclared-path assertions intact.
🤖 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/selection.rs`:
- Around line 243-255: Replace globset::escape in the not_text path handling
with gitignore-compatible escaping that preserves trailing whitespace and
escapes backslashes before passing the pattern to OverrideBuilder::add. Update
the relevant selection tests to cover literal paths ending in spaces and
backslashes, ensuring they neither trim nor produce DanglingEscape errors.
---
Nitpick comments:
In `@src/selection.rs`:
- Around line 676-714: Extend the test
a_not_text_path_holding_glob_metacharacters_excludes_only_itself to create
page[1].html as a declared not-text path and page1.html as an undeclared
near-match, then assert the declared literal path is excluded while page1.html
remains selected. Keep the existing data{1,2}.bin and other undeclared-path
assertions intact.
🪄 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: b9a9faca-ad17-4cf6-b92f-7119111a7ed3
📒 Files selected for processing (9)
docs/REFERENCE.mdsrc/audit.rssrc/git.rssrc/guard/message.rssrc/guard/scope.rssrc/scan.rssrc/selection.rstests/guard_recovered_halves.rstests/scan_cli.rs
| // Escaped, because these are PATHS and not patterns. The globs above are | ||
| // author-written and their metacharacters are meant; these come back from | ||
| // `git check-attr` and are literal names, so a tracked file called | ||
| // `page[1].html` or `data{1,2}.bin` was read as a character class or an | ||
| // alternation. Each way it went wrong is worse than the last: the class did | ||
| // not match its own name, so a file declared not-text was searched AND | ||
| // listed as skipped in the same report; the alternation matched two files | ||
| // nobody declared, removing them from every rule silently; and an unclosed | ||
| // class was a parse error that took the whole run to exit 2. | ||
| for path in not_text { | ||
| let literal = globset::escape(path); | ||
| builder | ||
| .add(&format!("!{path}")) | ||
| .add(&format!("!{literal}")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 'name = "(ignore|globset)"|ignore\s*=|globset\s*=' \
Cargo.toml Cargo.lock || true
rg -n -C 6 'not_text_paths|globset::escape|OverrideBuilder' --glob '*.rs' .Repository: HackingGate/uphold
Length of output: 7170
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- selection.rs ---'
sed -n '220,260p' src/selection.rs
printf '%s\n' '--- local cargo sources ---'
find "${CARGO_HOME:-$HOME/.cargo}/registry/src" -maxdepth 3 -type f \
\( -path '*/ignore-0.4.33/src/overrides.rs' -o -path '*/globset-0.4.20/src/lib.rs' \) \
-print 2>/dev/null || true
printf '%s\n' '--- pinned dependency source references ---'
curl -fsSL https://raw.githubusercontent.com/BurntSushi/ripgrep/ignore-0.4.33/ignore/src/overrides.rs \
| sed -n '1,260p' || true
curl -fsSL https://raw.githubusercontent.com/BurntSushi/ripgrep/globset-0.4.20/src/lib.rs \
| rg -n -C 8 'pub fn escape|fn escape' || trueRepository: HackingGate/uphold
Length of output: 2521
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL https://crates.io/api/v1/crates/ignore/0.4.33/download \
-o "$tmp/ignore.tgz"
curl -fsSL https://crates.io/api/v1/crates/globset/0.4.20/download \
-o "$tmp/globset.tgz"
tar -xzf "$tmp/ignore.tgz" -C "$tmp"
tar -xzf "$tmp/globset.tgz" -C "$tmp"
printf '%s\n' '--- ignore overrides parser ---'
rg -n -C 8 'pub fn add|trim|trailing|backslash|escape' \
"$tmp/ignore-0.4.33/src/overrides.rs" \
"$tmp/ignore-0.4.33/src/gitignore.rs"
printf '%s\n' '--- globset escape implementation ---'
rg -n -C 12 'pub fn escape|fn escape' \
"$tmp/globset-0.4.20/src"
printf '%s\n' '--- relevant tests ---'
rg -n -C 6 'trailing|backslash|escape|space|OverrideBuilder' \
"$tmp/ignore-0.4.33/src" "$tmp/globset-0.4.20/src" | head -n 240Repository: HackingGate/uphold
Length of output: 207
🌐 Web query:
ignore 0.4.33 overrides.rs OverrideBuilder add trailing whitespace backslash source
💡 Result:
There is no functionality in the ignore crate's OverrideBuilder (as found in overrides.rs) to "add back" trailing whitespace or interpret trailing backslashes specifically for override globs [1][2][3]. The OverrideBuilder provides a mechanism to specify override globs for file matching, with semantics identical to a line in a.gitignore file [4][5][2]. In this context: 1. Override Globs: The add method in OverrideBuilder simply adds a glob string to an underlying GitignoreBuilder [1][2][3]. These globs follow standard gitignore rules, where whitespace and backslashes are generally treated as literal parts of the pattern or path, unless they have special shell-like or glob-specific meaning (e.g., escaping a character) [5][2]. 2. Trailing Backslashes: In standard glob patterns (and gitignore files), a trailing backslash can be used to escape a character. If you intended to imply a "line continuation" or similar behavior often found in other contexts (like CommonMark or shell scripts), it is not a documented feature of the ignore crate's glob parsing [5][6]. 3. Trailing Whitespace: Because these globs are matched against file paths, trailing whitespace in a glob pattern is treated literally as part of the path matching criteria [5][2]. If your environment (such as an IDE or editor) strips trailing whitespace from your source code, you may need to use escape sequences like \x20 in your Rust string literal to ensure the whitespace is preserved in the compiled binary [7]. If you are encountering issues with trailing whitespace being stripped, it is likely occurring at the editor or source-control level rather than within the OverrideBuilder logic itself [7]. You can bypass this by using character escapes in your Rust code [7].
Citations:
- 1: https://docs.rs/ignore/latest/src/ignore/overrides.rs.html
- 2: https://github.com/BurntSushi/ripgrep/blob/57c190d5/crates/ignore/src/overrides.rs
- 3: https://github.com/BurntSushi/ripgrep/blob/4519153e/crates/ignore/src/overrides.rs
- 4: https://difftastic.wilfred.me.uk/rustdoc/ignore/overrides/struct.OverrideBuilder.html
- 5: https://docs.rs/ignore/latest/ignore/overrides/struct.OverrideBuilder.html
- 6: Trailing whitespace trimmed from doc comments rust-lang/rust-analyzer#15877
- 7: https://stackoverflow.com/questions/72992041/string-literal-with-trailing-whitespace-in-rust
🌐 Web query:
globset 0.4.20 escape source implementation
💡 Result:
In the globset crate (version 0.4.20), the escape function is designed to sanitize glob patterns by wrapping meta-characters in bracket expressions [1][2]. The implementation works by iterating through the input string and identifying characters that have special meaning in glob syntax [1]. When one of these characters is encountered, it is surrounded by square brackets to force the glob parser to treat it as a literal character [1][3]. The characters targeted by this function are:? * [ ] { } The source code implementation is as follows [1]: pub fn escape(s: &str) -> String { let mut escaped = String::with_capacity(s.len); for c in s.chars { match c { '?' | '*' | '[' | ']' | '{' | '}' => { escaped.push('['); escaped.push(c); escaped.push(']'); } c => { escaped.push(c); } } } escaped } Note that the exclamation mark (!) is not escaped by this function because it is only considered a meta-character when used inside brackets for character classes [1]. This escaping mechanism was originally adopted from the glob crate to provide a consistent way to handle literals [4]. In addition to this function, the GlobBuilder struct provides a backslash_escape method, which can be toggled to allow backslashes to escape meta-characters, depending on the platform's conventions [5][6].
Citations:
- 1: https://github.com/BurntSushi/ripgrep/blob/4519153e/crates/globset/src/lib.rs
- 2: https://docs.rs/globset/latest/globset/fn.escape.html
- 3: https://docs.rs/globset/0.4.18/globset/
- 4: globset: add escaping mechanism BurntSushi/ripgrep#2061
- 5: https://github.com/BurntSushi/ripgrep/blob/4519153e/crates/globset/src/glob.rs
- 6: https://docs.rs/globset/latest/x86_64-pc-windows-msvc/globset/struct.GlobBuilder.html?search=
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
for url in \
'https://docs.rs/crate/ignore/0.4.33/source/src/overrides.rs' \
'https://docs.rs/crate/ignore/0.4.33/source/src/gitignore.rs' \
'https://docs.rs/crate/globset/0.4.20/source/src/lib.rs' \
'https://raw.githubusercontent.com/BurntSushi/ripgrep/57c190d5/crates/ignore/src/overrides.rs' \
'https://raw.githubusercontent.com/BurntSushi/ripgrep/57c190d5/crates/ignore/src/gitignore.rs' \
'https://raw.githubusercontent.com/BurntSushi/ripgrep/57c190d5/crates/globset/src/lib.rs'
do
name="$(printf '%s' "$url" | tr '/:' '__')"
curl -fsSL "$url" -o "$tmp/$name" || true
done
printf '%s\n' '--- fetched files ---'
find "$tmp" -type f -printf '%f %s bytes\n'
printf '%s\n' '--- parser implementation references ---'
rg -n -C 12 'pub fn add|add_line|trim|trailing|backslash|escape' "$tmp" \
| head -n 400Repository: HackingGate/uphold
Length of output: 50374
Encode gitignore metacharacters for literal paths.
ignore 0.4.33 trims trailing whitespace in OverrideBuilder::add. It also enables backslash escaping. globset::escape does not escape backslashes. Therefore, a path such as payload.bin is reduced to payload.bin, while a path ending in \ causes a DanglingEscape error. Encode paths with gitignore-compatible escaping and add regression tests for trailing spaces and backslashes.
🤖 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 243 - 255, Replace globset::escape in the
not_text path handling with gitignore-compatible escaping that preserves
trailing whitespace and escapes backslashes before passing the pattern to
OverrideBuilder::add. Update the relevant selection tests to cover literal paths
ending in spaces and backslashes, ensuring they neither trim nor produce
DanglingEscape errors.
2 participants
Four defects, each reproduced against a built binary before the fix and each
covered by a test that fails without it. Three of the four are the same failure
this codebase writes essays about elsewhere: a check that could not look, or did
not look, reporting as one that did.
They arrived together from a review of the engine, so they are one branch, but
the commits are independent and can be split if that reads better.
guard::scope::keep_blobsdeadlocked on any push past ~1,500 objectsIt wrote every candidate sha into
git cat-file --batch-checkand only drainedstdout afterwards.
--batch-checkanswers each object as it reads it, so itfills its 64 KiB stdout pipe and stops reading stdin; the parent is blocked on a
full stdin pipe, the child on a stdout pipe nobody is draining.
git pushstops, with no output and no exit code.
Measured boundary: 1,503 objects completes, 2,003 hangs. Every tree-wide guard
at pre-push reaches it in a repository of any age.
audit::blob_shasandselection::not_text_pathseach grew a writer thread toavoid this, and
audit.rseven names the thirteen-hundredth object in acomment. Two call sites learned it and the third kept the version that hangs,
which is why this is one shared
git::blob_shasrather than a third copy ofthe fix.
prevent-ai-authorandprevent-unusual-unicodejudged the wrong messagemessage_textnever consulted the stage, so both read.git/COMMIT_EDITMSGeverywhere. Under
git committhat is the right file, which is what made themistake survivable and therefore permanent. At pre-push it is whatever the last
commit wrote.
A marker recorded by
git commit-tree, a rebase, a cherry-pick,git am,--no-verify, or a fast-forward out of a hookless clone therefore reached theremote with
1 guard(s) passed, exit 0. It failed the other way too: a staleCOMMIT_EDITMSGfrom a refused attempt refused a push that published nothingwrong.
no-private-repo-names-in-filesalready reads the pushed range for exactly thisreason. These two now use the same
scope::pushed_messages, and a finding namesthe commit instead of a path the guard never opened.
A not-text path was read as a glob, not a name
overrides_forhanded eachgit check-attranswer to the glob builder as!{path}. Those are literal filenames, and three characters turned one intosomething else:
data{1,2}.binbecame an alternation, sodata1.binanddata2.bin— neitherdeclared — were dropped from every content rule and named in no report.
page[1].htmlbecame a character class that does not match its own name, so afile declared not-text was searched and listed as skipped in the same
output. That is the invented finding
selection's own doc comment says mustnot happen.
capture[1.binis an unclosed class: a parse error that took the whole run toexit 2 with nothing reported.
No such filename exists in the fleet today, which is the only reason this is
third rather than first.
Scoping a guard killed the content scan for the whole repository
[rule.files]on a guard built-in is the supported way to narrow one —guard::scope::in_file_scopereads it, andconfigdeliberately exemptsbuilt-ins from the refusal that would reject the keys.
scanaborted on itanyway, exit 2 for the whole tree, with a diagnosis that was not true of the
rule it named: the keys are not read by nothing, they are read at the other
seam.
git.hooksdecides which seam owns the rule. A guard that names a hook runsthere; one that names no hook is run by nothing at either seam, so the refusal
still stands for that case — passing over it would report a check that did not
happen as one that did.
Verification
cargo test— 262 tests, all suites green. Nine new, and each was confirmed tofail against the code before its fix.
the reason
selection's existing one gives: a test that proves a deadlock isgone has to fail when it is not, and one that hangs names nothing.
cargo clippy --all-targetsclean under the crate's own profile;cargo fmt --checkclean.python3 -m pytest tests/— 110 passed, 227 subtests.uphold scanoverthis tree is clean.
Docs: the
prevent-ai-authorandprevent-unusual-unicoderows indocs/REFERENCE.mdsaid "a commit message" and now say which ones.Summary by CodeRabbit
New Features
Bug Fixes
Documentation