uphold 1.1.1: one answer about a file that is not text - #10
Merged
Conversation
`uphold scan` reads `.gitattributes`, skips a path declared `-text` and says it skipped it -- the use its own doc comment names, a captured page kept byte-for-byte in the encoding its venue served. The guard over the same files did not read the declaration: it opened the bytes, failed to decode them, and refused. So a repository that took one of the three cures the reference names got a clean tree from one seam and exit 2 from the other, on every commit that touched the file. The NUL test keeps its own job, and the comment beside it is still right about what it is for: it is the guess about bytes NOBODY declared. A declaration is not a guess, so where there is one it answers first, and where the declaration itself could not be read the refusal stands with that reason attached. The fixture builder here now writes its policy after `git init` rather than before, because several fixtures carry a policy meant to be refused at load -- and on a machine where this binary is installed in front of `git`, the shim reads that policy before it knows whether anything stands in front of the command, so the setup failed in the shape of the subject.
📝 WalkthroughWalkthroughThe Unicode guard now reads ChangesUnicode guard and release update
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant UnicodeGuard as Unicode guard
participant GitAttributes as .gitattributes
participant StagedFile as staged file
UnicodeGuard->>GitAttributes: read non-text declaration
GitAttributes-->>UnicodeGuard: return normalized path
UnicodeGuard->>StagedFile: inspect filename and bytes
UnicodeGuard->>UnicodeGuard: skip declared non-text path
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
+ Coverage 88.38% 88.41% +0.03%
==========================================
Files 24 24
Lines 7162 7181 +19
==========================================
+ Hits 6330 6349 +19
Misses 832 832 ☔ 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
🤖 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/guard/unicode.rs`:
- Around line 291-293: Update src/guard/unicode.rs at lines 291-293 so
not_text_paths resolves .gitattributes from the same staged or committed tree
used by scope::blobs, rather than the working tree; preserve path normalization
and declared_not_text construction. Add regression coverage in
tests/guard_cli.rs at lines 265-279 that reproduces an unstaged .gitattributes
change suppressing scanning of a staged blob and verifies the blob is still
scanned.
🪄 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: 36fc8428-984d-45b2-af82-43930a9dfa86
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlREADME.mdhooks/lefthook.ymlsrc/guard/unicode.rstests/guard_cli.rs
Comment on lines
+291
to
+293
| let (not_text, unmeasured) = not_text_paths(request.root); | ||
| let declared_not_text: BTreeSet<&str> = | ||
| not_text.iter().map(|path| normalize_rel(path)).collect(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the declaration-selection module before inspecting the implementation.
ast-grep outline src/selection.rs --items all
# Inspect whether declaration lookup uses Git's index, for example via
# `git check-attr --cached`, rather than the working tree.
rg -n -C 10 'fn not_text_paths\b|not_text_paths\(|check-attr|--cached|gitattributes' \
src/selection.rs src/guard/unicode.rs
# Inspect the current staged-declaration test before adding the unstaged case.
sed -n '238,280p' tests/guard_cli.rsRepository: HackingGate/uphold
Length of output: 21277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- selection index and attribute implementation ---'
sed -n '50,180p' src/selection.rs
printf '%s\n' '--- staged blob collection and guard request construction ---'
rg -n -C 12 'fn blobs\b|blobs\(|struct Request|stage|index_bytes|from_index' src/guard src/selection.rs
printf '%s\n' '--- relevant guard tests and helpers ---'
rg -n -C 8 'a_path_declared_not_text|fn guard\b|--stage|pre-commit|captured.html' tests/guard_cli.rs tests
printf '%s\n' '--- local Git attribute semantics, without changing the repository ---'
git check-attr text -- .gitattributes 2>&1 || true
git check-attr --cached text -- .gitattributes 2>&1 || true
printf '%s\n' '--- source-level behavioral verifier ---'
python3 - <<'PY'
from pathlib import Path
selection = Path("src/selection.rs").read_text()
unicode = Path("src/guard/unicode.rs").read_text()
start = selection.index("pub(crate) fn not_text_paths")
end = selection.index("\npub(crate) struct Selection", start)
impl_text = selection[start:end]
assert 'args(["check-attr", "--stdin", "-z", "text"])' in impl_text
assert "--cached" not in impl_text
assert "current_dir(root)" in impl_text
blobs_call = unicode.index("let blobs =")
blobs_end = unicode.index(";", blobs_call)
request_call = unicode.index("not_text_paths(request.root)")
print("not_text_paths uses working-tree `git check-attr` without `--cached`.")
print("The guard resolves attributes from `request.root` independently of its staged blob collection.")
print("Conclusion:", request_call > blobs_end)
PYRepository: HackingGate/uphold
Length of output: 50375
Resolve .gitattributes against the scanned tree.
not_text_paths uses working-tree attributes, while scope::blobs scans staged or pushed objects. An unstaged .gitattributes change can suppress scanning of a staged blob. Use the same staged or committed tree for both decisions, and add regression coverage.
📍 Affects 2 files
src/guard/unicode.rs#L291-L293(this comment)tests/guard_cli.rs#L265-L279
🤖 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 291 - 293, Update src/guard/unicode.rs at
lines 291-293 so not_text_paths resolves .gitattributes from the same staged or
committed tree used by scope::blobs, rather than the working tree; preserve path
normalization and declared_not_text construction. Add regression coverage in
tests/guard_cli.rs at lines 265-279 that reproduces an unstaged .gitattributes
change suppressing scanning of a staged blob and verifies the blob is still
scanned.
Source: Coding guidelines
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.
uphold scanreads.gitattributes, skips a path declared-text, and says it skipped it. The guard over the same files did not read the declaration -- it opened the bytes, failed to decode them, and refused -- so a repository that took one of the three cures the reference names got a clean tree from one seam and exit 2 from the other, on every commit touching the file. Found on a real repository with a Shift-JIS capture fixture already declaredbinaryin.gitattributes.The NUL test keeps its job: it is the guess about bytes nobody declared. Where the declaration itself could not be read, the refusal stands with that reason attached.
Also reorders the guard fixture builder to write its policy after
git init: several fixtures carry a policy meant to be refused at load, and with this binary installed in front ofgitthe shim reads that policy before it knows whether anything stands in front of the command.Summary by CodeRabbit
New Features
.gitattributesentries that mark files as non-text, skipping those files during content checks.Bug Fixes
Documentation