From 8a54678a678e461471cf312f5212cf27607fad7d Mon Sep 17 00:00:00 2001 From: Tong Date: Wed, 12 Aug 2026 19:55:56 +0900 Subject: [PATCH 1/4] Drain the pipe the pre-push guard fills, in the one place it is filled `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. --- src/audit.rs | 83 ++-------------------- src/git.rs | 173 +++++++++++++++++++++++++++++++++++++++++++++ src/guard/scope.rs | 62 +++------------- 3 files changed, 189 insertions(+), 129 deletions(-) diff --git a/src/audit.rs b/src/audit.rs index 4eab4cd..b0fe11d 100644 --- a/src/audit.rs +++ b/src/audit.rs @@ -42,9 +42,8 @@ //! still carries the caveat in its body. use std::collections::BTreeSet; -use std::io::{Read as _, Write as _}; use std::path::Path; -use std::process::{Command, Stdio}; +use std::process::Command; use crate::config::{Check, Policy, Rule}; use crate::error::{Exit, Fatal, Result}; @@ -394,83 +393,11 @@ fn forge_conversations(root: &Path) -> (Vec, Vec) { } /// Which of these objects git says are blobs, asked once rather than once each. +/// +/// `git::blob_shas` holds the pumping and the exit-status refusal, because the +/// pre-push guard asks the identical question and had the version that hangs. fn blob_shas(root: &Path, shas: &[String]) -> Result> { - let mut blobs = BTreeSet::new(); - if shas.is_empty() { - return Ok(blobs); - } - let mut child = Command::new("git") - .args(["cat-file", "--batch-check=%(objectname) %(objecttype)"]) - .current_dir(root) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .spawn() - .map_err(|error| Fatal::new(format!("git cat-file: {error}")))?; - let mut stdin = child - .stdin - .take() - .ok_or_else(|| Fatal::new("git cat-file: no stdin"))?; - let mut stdout = child - .stdout - .take() - .ok_or_else(|| Fatal::new("git cat-file: no stdout"))?; - // Both pipes move at once, for the reason `selection::not_text_paths` gives - // at greater length: `--batch-check` answers each object as it reads it, at - // roughly fifty bytes an answer, so it fills its stdout pipe -- 64 KiB on - // Linux -- somewhere near the thirteen-hundredth object and stops reading - // stdin. A parent that writes the whole list first is then blocked on a full - // stdin pipe while the child is blocked on a stdout pipe nobody is draining. - // Every repository this audit is meant for is far past that count, so - // writing first is not a rare hang, it is the ordinary case. - let mut answered: Vec = Vec::new(); - let written = std::thread::scope(|scope| { - let writer = scope.spawn(move || { - for sha in shas { - writeln!(stdin, "{sha}")?; - } - // Dropped here, and closing stdin is what tells `--batch-check` the - // list is finished. Without it the child waits for more input that - // is never coming and the read below never sees end of file. - drop(stdin); - Ok::<(), std::io::Error>(()) - }); - let drained = stdout.read_to_end(&mut answered); - // The writer's own error outranks the drain's: a child that died early - // shows up here as a broken pipe, and the drain merely stops. - writer.join().map_or_else( - |_| { - Err(std::io::Error::other( - "git cat-file: writer thread panicked", - )) - }, - |result| result.and_then(|()| drained.map(|_| ())), - ) - }); - written.map_err(|error| Fatal::new(format!("git cat-file: {error}")))?; - let status = child - .wait() - .map_err(|error| Fatal::new(format!("git cat-file: {error}")))?; - // Reported rather than swallowed, for the reason `keep_blobs` in - // `guard::scope` gives: no stdout means no known kinds, the filter below - // then keeps nothing, and a repository whose objects could not be identified - // would be audited as a repository with nothing in it. - if !status.success() { - return Err(Fatal::new(format!( - "git cat-file --batch-check exited {}: cannot tell which of {} reachable \ - object(s) are blobs, and reading none of them would report as a clean tree", - status.code().unwrap_or(-1), - shas.len() - ))); - } - let text = String::from_utf8_lossy(&answered); - for line in text.lines() { - let fields: Vec<&str> = line.split_whitespace().collect(); - if let [name, "blob", ..] = fields.as_slice() { - blobs.insert((*name).to_owned()); - } - } - Ok(blobs) + crate::git::blob_shas(root, shas) } /// Every blob a flip would serve, not every path HEAD still names. diff --git a/src/git.rs b/src/git.rs index 85de757..9a978f4 100644 --- a/src/git.rs +++ b/src/git.rs @@ -5,6 +5,7 @@ //! is safe; it has established nothing, and returning an empty answer would //! make that look like a pass. +use std::collections::BTreeSet; use std::path::{Path, PathBuf}; use std::process::Command; @@ -33,6 +34,102 @@ pub(crate) fn run(root: &Path, args: &[&str]) -> Result { }) } +/// Which of these objects git says are blobs, asked once rather than once each. +/// +/// The two pipes move at the same time, on two threads, and that is not a style +/// preference. `--batch-check` answers each object as it reads it, at roughly +/// fifty bytes an answer, so it fills its stdout pipe -- 64 KiB on Linux -- +/// somewhere near the thirteen-hundredth object and stops reading stdin. A +/// parent that writes the whole list first is then blocked on a full stdin pipe +/// while the child is blocked on a stdout pipe nobody is draining, and neither +/// ever moves again: no output, no exit code, the push simply stops. Every +/// repository these callers are meant for is far past that count, so writing +/// first is not a rare hang, it is the ordinary case. +/// +/// This lived twice, and the third caller is why it lives here instead: the +/// audit and the selection pass each grew their own writer thread while the +/// pre-push guard kept the version that hangs. One copy is the only shape in +/// which that cannot happen again. +pub(crate) fn blob_shas(root: &Path, shas: &[String]) -> Result> { + use std::io::{Read, Write}; + use std::process::Stdio; + + let mut blobs = BTreeSet::new(); + if shas.is_empty() { + return Ok(blobs); + } + + let mut child = Command::new("git") + .args(["cat-file", "--batch-check=%(objectname) %(objecttype)"]) + .current_dir(root) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .map_err(|error| Fatal::new(format!("git cat-file: {error}")))?; + let mut stdin = child + .stdin + .take() + .ok_or_else(|| Fatal::new("git cat-file: no stdin"))?; + let mut stdout = child + .stdout + .take() + .ok_or_else(|| Fatal::new("git cat-file: no stdout"))?; + + let mut answered: Vec = Vec::new(); + let written = std::thread::scope(|scope| { + let writer = scope.spawn(move || { + for sha in shas { + writeln!(stdin, "{sha}")?; + } + // Dropped here, and closing stdin is what tells `--batch-check` the + // list is finished. Without it the child waits for more input that + // is never coming and the read below never sees end of file. + drop(stdin); + Ok::<(), std::io::Error>(()) + }); + let drained = stdout.read_to_end(&mut answered); + // The writer's own error outranks the drain's: a child that died early + // shows up here as a broken pipe, and the drain merely stops. + writer.join().map_or_else( + |_| { + Err(std::io::Error::other( + "git cat-file: writer thread panicked", + )) + }, + |result| result.and_then(|()| drained.map(|_| ())), + ) + }); + written.map_err(|error| Fatal::new(format!("git cat-file: {error}")))?; + + let status = child + .wait() + .map_err(|error| Fatal::new(format!("git cat-file: {error}")))?; + // Reported rather than swallowed: no stdout means no known kinds, every + // caller's filter then keeps nothing, and a set of objects that could not be + // identified would read as a set with no blobs in it. + // + // A missing object is not this case. `--batch-check` writes " missing" + // and still exits 0, so a non-zero status means git itself could not run. + if !status.success() { + return Err(Fatal::new(format!( + "git cat-file --batch-check exited {}: cannot tell which of {} object(s) \ + are blobs, and reporting none of them would read as nothing to check", + status.code().unwrap_or(-1), + shas.len() + ))); + } + + let text = String::from_utf8_lossy(&answered); + for line in text.lines() { + let fields: Vec<&str> = line.split_whitespace().collect(); + if let [name, "blob", ..] = fields.as_slice() { + blobs.insert((*name).to_owned()); + } + } + Ok(blobs) +} + /// NUL-separated output, for the paths git will not quote. pub(crate) fn run_z(root: &Path, args: &[&str]) -> Result> { Ok(run(root, args)? @@ -135,4 +232,80 @@ mod tests { ("Ada Lovelace".to_owned(), "ada@example.test".to_owned()) ); } + + #[test] + fn several_thousand_objects_are_asked_about_without_deadlocking() { + // The proof for the pipe. `--batch-check` answers each object as it + // reads it, at roughly fifty bytes an answer, so 4000 objects is 160 KiB + // of stdin and 200 KiB back -- several times over the 64 KiB a pipe + // holds in each direction. A caller that wrote the whole list before + // reading a byte stopped somewhere past the fifteen-hundredth and never + // came back, which is what `guard::scope::keep_blobs` did to every push + // of a range this size. + // + // On a thread with a deadline, because a test that proves a deadlock is + // gone has to FAIL when it is not, and a test that hangs reports nothing + // at all -- it stops the suite with no failing test named. + let root = std::env::temp_dir().join(format!("uphold-git-batch-{}", std::process::id())); + std::fs::remove_dir_all(&root).ok(); + std::fs::create_dir_all(&root).unwrap(); + for args in [ + &["init", "-q", "-b", "main"][..], + &["config", "user.name", "Test"][..], + &["config", "user.email", "test@example.test"][..], + ] { + let status = Command::new("git") + .args(args) + .current_dir(&root) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .unwrap(); + assert!(status.success(), "git {args:?} failed"); + } + + // Written and staged in one `git add`, because the point of the test is + // the pipe and not the fixture: 4000 `hash-object` processes cost a + // minute of suite time to produce the same 4000 shas. + for index in 0..4000_u32 { + std::fs::write( + root.join(format!("blob-{index:05}.txt")), + format!("blob number {index}\n"), + ) + .unwrap(); + } + let status = Command::new("git") + .args(["add", "-A", "."]) + .current_dir(&root) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .unwrap(); + assert!(status.success(), "git add failed"); + let staged = run(&root, &["ls-files", "-s"]).unwrap(); + let shas: Vec = staged + .lines() + .filter_map(|line| line.split_whitespace().nth(1).map(str::to_owned)) + .collect(); + assert_eq!(shas.len(), 4000, "the fixture did not stage"); + + let (sender, receiver) = std::sync::mpsc::channel(); + let asked = root.clone(); + let listed = shas.clone(); + std::thread::spawn(move || { + sender.send(blob_shas(&asked, &listed)).ok(); + }); + let blobs = receiver + .recv_timeout(std::time::Duration::from_secs(60)) + .expect("`git cat-file --batch-check` did not answer: the pipes deadlocked") + .unwrap(); + + assert_eq!(blobs.len(), shas.len()); + std::fs::remove_dir_all(&root).ok(); + } + + #[test] + fn an_empty_list_asks_git_nothing() { + assert!(blob_shas(&std::env::temp_dir(), &[]).unwrap().is_empty()); + } } diff --git a/src/guard/scope.rs b/src/guard/scope.rs index 4587db2..3bf9c86 100644 --- a/src/guard/scope.rs +++ b/src/guard/scope.rs @@ -447,62 +447,22 @@ pub(crate) fn pushed_messages( } fn keep_blobs(root: &Path, candidates: Vec) -> Result> { - use std::io::Write; - use std::process::{Command, Stdio}; - if candidates.is_empty() { return Ok(candidates); } - - let mut child = Command::new("git") - .args(["cat-file", "--batch-check=%(objectname) %(objecttype)"]) - .current_dir(root) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .spawn() - .map_err(|error| Fatal::new(format!("git cat-file: {error}")))?; - { - let mut stdin = child - .stdin - .take() - .ok_or_else(|| Fatal::new("git cat-file: no stdin"))?; - for candidate in &candidates { - writeln!(stdin, "{}", candidate.sha) - .map_err(|error| Fatal::new(format!("git cat-file: {error}")))?; - } - } - let output = child - .wait_with_output() - .map_err(|error| Fatal::new(format!("git cat-file: {error}")))?; - // The status was never read here, and the failure was silent in the worst - // direction: no stdout means no known kinds, and the filter below then drops - // EVERY candidate and returns an empty list -- a push whose blobs could not - // be identified, reported as a push with no blobs in it. `read` a few lines - // down has always checked this; only this function did not. - // - // A missing object is not this case. `--batch-check` writes " missing" - // and still exits 0, so a non-zero status means git itself could not run. - if !output.status.success() { - return Err(Fatal::new(format!( - "git cat-file --batch-check exited {}: cannot tell which of {} object(s) \ - are blobs, and reporting none of them would read as a clean push", - output.status.code().unwrap_or(-1), - candidates.len() - ))); - } - let text = String::from_utf8_lossy(&output.stdout); - - let mut kinds: BTreeMap<&str, &str> = BTreeMap::new(); - for line in text.lines() { - let fields: Vec<&str> = line.split_whitespace().collect(); - if let [name, kind, ..] = fields.as_slice() { - kinds.insert(name, kind); - } - } + // The pumping, the closed stdin and the exit-status refusal all live in + // `git::blob_shas`. They used to live here too, in a version that wrote the + // whole list before reading a byte back -- which is the pipe deadlock the + // shared one exists to make unrepeatable, and which hung this guard on any + // pushed range past roughly fifteen hundred objects. + let shas: Vec = candidates + .iter() + .map(|candidate| candidate.sha.clone()) + .collect(); + let blobs = git::blob_shas(root, &shas)?; Ok(candidates .into_iter() - .filter(|candidate| kinds.get(candidate.sha.as_str()) == Some(&"blob")) + .filter(|candidate| blobs.contains(&candidate.sha)) .collect()) } From 649432abbca10223cf104bdafa128653854fb9cd Mon Sep 17 00:00:00 2001 From: Tong Date: Wed, 12 Aug 2026 19:56:18 +0900 Subject: [PATCH 2/4] Judge the messages a push publishes, not the last one anybody edited `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. --- docs/REFERENCE.md | 4 +- src/guard/message.rs | 55 +++++++++++---- tests/guard_recovered_halves.rs | 121 ++++++++++++++++++++++++++++++++ 3 files changed, 166 insertions(+), 14 deletions(-) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 38525e9..3a6464d 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -350,9 +350,9 @@ stamped on it, the range about to be pushed. | guard | refuses | |---|---| -| `prevent-ai-author` | a commit message carrying AI-authorship markers | +| `prevent-ai-author` | AI-authorship markers in the message being written — and at a push, in **every commit message the push publishes** | | `prevent-author-mismatch` | an identity that is not your global one | -| `prevent-unusual-unicode` | unusual characters in a commit message | +| `prevent-unusual-unicode` | unusual characters in the same set of messages | | `prevent-unusual-unicode-in-files` | characters that draw nothing, in committed content **and in the paths that carry it** | | `no-private-repo-names` | a private repository named in a public one's message | | `no-private-repo-names-staged` | the same, in the lines a commit adds | diff --git a/src/guard/message.rs b/src/guard/message.rs index fdd71f8..1afe09f 100644 --- a/src/guard/message.rs +++ b/src/guard/message.rs @@ -53,6 +53,37 @@ fn message_text(request: &Request<'_>) -> Result<(PathBuf, String)> { Ok((path, String::from_utf8_lossy(&bytes).into_owned())) } +/// Every message this run is actually about, labelled. +/// +/// At `pre-push` that is the messages of the commits being published, and NOT +/// `.git/COMMIT_EDITMSG`. Reading the fallback there is the same mistake the +/// paragraph above `message_text` describes, arriving by the other door: the +/// file exists, it holds whatever the last `git commit` wrote, and it is clean +/// -- so a push carrying a marker in a commit made by `git commit-tree`, a +/// rebase, a cherry-pick, `git am`, `--no-verify`, or a fast-forward out of a +/// hookless clone was reported as one guard passed, exit 0. +/// +/// `no-private-repo-names` already reads the pushed range for exactly this +/// reason; these two guards were the ones left asking the wrong file. +fn message_subjects(request: &Request<'_>) -> Result> { + if request.stage == super::Stage::PrePush { + return Ok(super::scope::pushed_messages( + request.root, + request.stage, + request.push_refs, + request.push_source, + )? + .into_iter() + .map(|(sha, body)| { + let short: String = sha.chars().take(12).collect(); + (format!("commit {short} (its MESSAGE)"), body) + }) + .collect()); + } + let (path, text) = message_text(request)?; + Ok(vec![(path.display().to_string(), text)]) +} + /// The judgment, over text that may never have been a file. pub(crate) fn ai_author_in(rule: &crate::config::Rule, label: &str, text: &str) -> Option { let mut found: Vec<&str> = Vec::new(); @@ -129,12 +160,12 @@ fn unusual_findings(label: &str, text: &str) -> Vec { } pub(crate) fn prevent_ai_author(request: &Request<'_>) -> Result> { - let (path, text) = message_text(request)?; - Ok(ai_author_in( - request.rule, - &path.display().to_string(), - &text, - )) + for (label, text) in message_subjects(request)? { + if let Some(refusal) = ai_author_in(request.rule, &label, &text) { + return Ok(Some(refusal)); + } + } + Ok(None) } /// Characters refused in a commit message. @@ -163,10 +194,10 @@ fn message_character_is_ordinary(character: char) -> bool { } pub(crate) fn prevent_unusual_unicode(request: &Request<'_>) -> Result> { - let (path, text) = message_text(request)?; - Ok(unusual_unicode_in( - request.rule, - &path.display().to_string(), - &text, - )) + for (label, text) in message_subjects(request)? { + if let Some(refusal) = unusual_unicode_in(request.rule, &label, &text) { + return Ok(Some(refusal)); + } + } + Ok(None) } diff --git a/tests/guard_recovered_halves.rs b/tests/guard_recovered_halves.rs index 31da167..50037ef 100644 --- a/tests/guard_recovered_halves.rs +++ b/tests/guard_recovered_halves.rs @@ -392,3 +392,124 @@ fn an_unrelated_literal_rule_does_not_remove_the_identity_fallback() { let output = scan_text(&root, subject.as_bytes(), home); assert_eq!(code(&output), 1, "{}", stderr(&output)); } + +// ── the message guards at pre-push ─────────────────────────────────── + +/// The two message guards, pinned at the stage that made them read the wrong +/// file. `no-private-repo-names-in-files` above already reads the pushed range; +/// these two asked `.git/COMMIT_EDITMSG` at every stage, so at pre-push they +/// judged whatever the last `git commit` happened to write. +const PUSHED_MESSAGES: &str = r#" +[rule.prevent-ai-author] +builtin = "prevent-ai-author" + +[rule.prevent-ai-author.git] +hooks = ["commit-msg", "pre-push"] + +[rule.prevent-unusual-unicode] +builtin = "prevent-unusual-unicode" + +[rule.prevent-unusual-unicode.git] +hooks = ["commit-msg", "pre-push"] +"#; + +fn pre_push(root: &Path, pushed: &str) -> Output { + Command::new(env!("CARGO_BIN_EXE_uphold")) + .args(["guard", "--stage", "pre-push"]) + .current_dir(root) + .env_remove("UPHOLD_ALLOW") + .env("PRE_COMMIT_REMOTE_NAME", "origin") + .env("PRE_COMMIT_LOCAL_BRANCH", "main") + .env("PRE_COMMIT_REMOTE_BRANCH", "refs/heads/main") + .env("PRE_COMMIT_TO_REF", pushed) + .env("PRE_COMMIT_FROM_REF", ZERO) + .stdin(Stdio::null()) + .output() + .unwrap() +} + +#[test] +fn an_attribution_marker_in_a_pushed_commit_is_refused_at_pre_push() { + // The whole of the bug in one fixture: the marker is in the commit being + // pushed, and `.git/COMMIT_EDITMSG` holds a LATER, clean message -- so a + // guard reading the fallback found nothing and reported "1 guard(s) + // passed", exit 0, while the marker went to the remote. + let root = repository(PUSHED_MESSAGES); + write(&root, "a.txt", "one\n"); + git(&root, &["add", "a.txt"]); + git( + &root, + &[ + "commit", + "-qm", + "Add the thing\n\nGenerated with Claude Code\n", + "--no-verify", + ], + ); + write(&root, "b.txt", "two\n"); + git(&root, &["add", "b.txt"]); + git( + &root, + &["commit", "-qm", "Add another thing", "--no-verify"], + ); + + let pushed = head(&root); + let output = pre_push(&root, &pushed); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + stderr(&output).contains("prevent-ai-author"), + "{}", + stderr(&output) + ); + // Named by commit, not by the path of a file it did not read. + assert!(stderr(&output).contains("MESSAGE"), "{}", stderr(&output)); +} + +#[test] +fn an_invisible_character_in_a_pushed_commit_is_refused_at_pre_push() { + let root = repository(PUSHED_MESSAGES); + write(&root, "a.txt", "one\n"); + git(&root, &["add", "a.txt"]); + git( + &root, + &["commit", "-qm", "Add the\u{200b}thing", "--no-verify"], + ); + write(&root, "b.txt", "two\n"); + git(&root, &["add", "b.txt"]); + git( + &root, + &["commit", "-qm", "Add another thing", "--no-verify"], + ); + + let pushed = head(&root); + let output = pre_push(&root, &pushed); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + stderr(&output).contains("prevent-unusual-unicode"), + "{}", + stderr(&output) + ); +} + +#[test] +fn a_clean_push_still_passes_when_the_last_edited_message_was_not() { + // The other direction, and the reason the fallback is not merely + // unnecessary: `.git/COMMIT_EDITMSG` outlives the commit it was written + // for. Reading it at pre-push refuses a push that publishes nothing wrong. + let root = repository(PUSHED_MESSAGES); + write(&root, "a.txt", "one\n"); + git(&root, &["add", "a.txt"]); + git(&root, &["commit", "-qm", "Add the thing", "--no-verify"]); + let pushed = head(&root); + + // Left behind by an attempt that was refused and never became a commit. + let git_dir = root.join(".git"); + std::fs::write( + git_dir.join("COMMIT_EDITMSG"), + "Something\n\nGenerated with Claude Code\n", + ) + .unwrap(); + + let output = pre_push(&root, &pushed); + assert_eq!(code(&output), 0, "{}", stderr(&output)); +} From 957b4c703b3cd23ba825ce5790998a107b1328f2 Mon Sep 17 00:00:00 2001 From: Tong Date: Wed, 12 Aug 2026 19:56:41 +0900 Subject: [PATCH 3/4] Read a not-text path as the name it is, not as a pattern `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. --- src/selection.rs | 73 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/src/selection.rs b/src/selection.rs index 6c9ae10..288c932 100644 --- a/src/selection.rs +++ b/src/selection.rs @@ -240,9 +240,19 @@ fn overrides_for(root: &Path, rule: &Rule, not_text: &[String]) -> Result Date: Wed, 12 Aug 2026 19:56:51 +0900 Subject: [PATCH 4/4] Let a guard own its file scope instead of failing the scan over it `[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. --- src/scan.rs | 24 +++++++++++---- tests/scan_cli.rs | 76 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 5 deletions(-) diff --git a/src/scan.rs b/src/scan.rs index 3be5074..ffa85ae 100644 --- a/src/scan.rs +++ b/src/scan.rs @@ -110,13 +110,27 @@ impl<'a> Scan<'a> { } failures.extend(match rule.builtin().unwrap_or_default() { "links-resolve" => self.link_failures(rule)?, - // Every other built-in reads something that is not the - // tree. Silently passing over it here would report a - // check that did not happen as one that did. + // A guard built-in's `[rule.files]` is not read by + // nothing: `guard::scope::in_file_scope` reads it, to + // scope the guard to part of the tree. So the question + // is which seam owns the rule, and `git.hooks` answers + // it -- a rule that names a hook runs there, and this + // scan is not its seam to fail from. + // + // Aborting here regardless is what made scoping a guard + // -- the supported way to narrow one -- kill content + // scanning for the WHOLE repository at exit 2, with a + // diagnosis that was not true of the rule it named. + _ if !rule.hooks().is_empty() => continue, + // With no hook either, nothing runs it and the keys + // really are read by nothing. Silently passing over + // that would report a check that did not happen as one + // that did. other => { return Err(Fatal::new(format!( - "rule {:?}: built-in {other:?} does not read files, so \ - its `files.*` keys would be read by nothing", + "rule {:?}: built-in {other:?} does not read files and names \ + no `git.hooks`, so nothing runs it and its `files.*` keys \ + would be read by nothing", rule.id ))) } diff --git a/tests/scan_cli.rs b/tests/scan_cli.rs index 045b643..674e074 100644 --- a/tests/scan_cli.rs +++ b/tests/scan_cli.rs @@ -1165,3 +1165,79 @@ fn the_effective_rules_are_what_inheritance_resolved_to() { "{text}" ); } + +// --- a guard's own file scope is not the scan's to fail on ----------------- + +#[test] +fn a_scoped_guard_does_not_abort_the_content_scan() { + // `[rule.files]` on a guard built-in is the supported way to narrow one: + // `guard::scope::in_file_scope` reads it. The scan aborted on it anyway, + // exit 2 for the WHOLE repository, with a diagnosis -- "would be read by + // nothing" -- that was not true of the rule it named. Scoping one guard + // switched off every content rule in the policy. + let root = workspace(); + write( + &root, + "policy/principles.toml", + r#" + [rule.no-todo] + message = "no TODO" + regexp = 'TODO' + + [rule.no-todo.files] + exclude = ["policy/**"] + + [rule.prevent-unusual-unicode-in-files] + builtin = "prevent-unusual-unicode-in-files" + + [rule.prevent-unusual-unicode-in-files.files] + include = ["src"] + + [rule.prevent-unusual-unicode-in-files.git] + hooks = ["pre-commit"] +"#, + ); + write(&root, "src/a.txt", "fine\nTODO: later\n"); + + let output = scan(&root); + assert_eq!( + code(&output), + 1, + "the scan should report the pattern rule, not abort: {}", + stderr(&output) + ); + assert!( + stderr(&output).contains("src/a.txt:2:TODO: later"), + "{}", + stderr(&output) + ); +} + +#[test] +fn a_guard_built_in_that_no_hook_runs_is_still_refused() { + // The other side of the same question. With `files.*` and no `git.hooks`, + // nothing runs the rule at either seam, so the keys really are read by + // nothing -- and passing over it would report a check that did not happen + // as one that did. + let root = workspace(); + write( + &root, + "policy/principles.toml", + r#" + [rule.prevent-unusual-unicode-in-files] + builtin = "prevent-unusual-unicode-in-files" + + [rule.prevent-unusual-unicode-in-files.files] + include = ["src"] +"#, + ); + write(&root, "src/a.txt", "fine\n"); + + let output = scan(&root); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!( + stderr(&output).contains("prevent-unusual-unicode-in-files"), + "{}", + stderr(&output) + ); +}