From 188970b2250f4a344c06cc8ff4e18ff5d24e0c5a Mon Sep 17 00:00:00 2001 From: Tong Date: Wed, 12 Aug 2026 23:04:07 +0900 Subject: [PATCH] Let a text-capable guard stand in front of a command Three repositories wrote `command.before` on `no-private-repo-names` and the loader refused all three, on grounds that were true and unhelpful: a built-in is not an `exec`, and `shim::run` consulted only `exec` rules. They were not writing it by mistake. That guard reads the commit message at every git hook, and a repository whose own prose cites its issues gets every one of those citations refused -- GroundGate says so in the policy file, in a paragraph explaining why the rule deliberately carries no `git.hooks`. So the seam it belongs at is the command that publishes text to a forge, and there was no field to say that in. Every alternative was worse: `git.hooks = ["commit-msg"]` refuses the citations, `git.hooks = ["manual"]` refuses them at CI instead, and no place at all is refused at load. So the shim consults text-capable built-ins as well as `exec` checkers, scoped by the same `command.before`, through the same dispatch `uphold guard --text` uses -- a guard that judged a commit message one way and a pull-request body another would be two rules under one id. The loader allows the field for those three built-ins and still refuses it for anything that reads an index, an identity or a push range, with a message that now names which ones can. And the "this shim is checked by nothing" refusal counts a built-in as a checker, having previously refused a policy whose shim was checked by a guard rather than by a script. The fleet goes from 68 of 71 repositories reconciling to 71 of 71. The three were not misconfigured; the vocabulary was short a word. --- docs/REFERENCE.md | 12 ++++++- src/config.rs | 87 ++++++++++++++++++++++++++++++++++++++++------- src/guard/mod.rs | 42 ++++++++++++++++------- src/shim.rs | 23 ++++++++++++- tests/shim_cli.rs | 68 ++++++++++++++++++++++++++++++++++++ 5 files changed, 205 insertions(+), 27 deletions(-) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 4561d49..4ec6172 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -55,7 +55,17 @@ does not run.** Both halves are checked at load. A rule naming two checks is refused, because one of them would be read by nothing while looking enforced. A rule naming no place is refused, because it runs nowhere and that reads exactly like a rule -that passes. +that passes. `command.before` is refused on a check no shim can consult — the +shim consults `exec` checkers and the built-ins that can judge arbitrary text +(`prevent-ai-author`, `prevent-unusual-unicode`, `no-private-repo-names`), and +anything else reads an index, an identity or a push range and has nothing to say +about a pull-request body. + +A text-capable built-in with `command.before` and no `git.hooks` is a deliberate +shape, not an omission. `no-private-repo-names` reads the commit message at +every git hook, and a repository whose own prose cites its issues would have +every one of those citations refused — so the seam it belongs at is the command +that publishes text to a forge, and only that one. Exit codes: `0` clean, `1` violations, `2` the check could not be made. diff --git a/src/config.rs b/src/config.rs index 7ab14bf..256492d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -903,13 +903,33 @@ impl Rule { // it: "nothing says where it runs" is SATISFIED by the very field that // cannot be used, so the one check that exists to find a rule with no // place is the check this rule slips past. - if check != Check::Exec && self.command.is_some() { + // A text-capable built-in stands in front of a command too, and it is + // the ONLY seam some of them belong at: `no-private-repo-names` reads a + // commit message at every git hook, which refuses the issue citations a + // repository's own prose is full of, so a repository that wants it over + // a pull-request body and nowhere else has no other field to say it in. + // Three wrote `command.before` on the built-in independently while this + // refused all three. + let text_capable = self + .builtin() + .is_some_and(|builtin| crate::guard::TEXT_GUARDS.contains(&builtin)); + if check != Check::Exec && !text_capable && self.command.is_some() { + let built_in_note = if check == Check::Builtin { + format!( + "\nThe built-ins that can judge the text a command publishes are {}; \ + any other reads an index, an identity or a push range, and has nothing \ + to say about a pull-request body.", + crate::guard::TEXT_GUARDS.join(", ") + ) + } else { + String::new() + }; return Err(Fatal::new(format!( - "rule {:?}: only an `exec` checker stands in front of a command, so \ - `command.before` on a `{check}` rule would be read by nothing and would \ - look like configuration that works.\n\ + "rule {:?}: a `{check}` rule cannot stand in front of a command, so \ + `command.before` here would be read by nothing and would look like \ + configuration that works.\n\ A rule that searches the tree says so with `files.*`, and a built-in \ - that fires at a git hook says so with `git.hooks`.", + that fires at a git hook says so with `git.hooks`.{built_in_note}", self.id ))); } @@ -1299,9 +1319,18 @@ fn validate_shims(policy_path: &Path, rules: &[Rule], shims: &[crate::shim::Shim } } + // Both kinds count. An `exec` checker is a program this repository names; a + // text-capable built-in is one the binary carries, and `shim::run` consults + // both. Counting only the first refused a policy whose shim was checked -- + // by a guard rather than by a script -- as a shim checked by nothing. let checked: BTreeSet<&str> = rules .iter() - .filter(|rule| rule.is(Check::Exec)) + .filter(|rule| { + rule.is(Check::Exec) + || rule + .builtin() + .is_some_and(|builtin| crate::guard::TEXT_GUARDS.contains(&builtin)) + }) .filter_map(|rule| rule.command.as_ref()) .flat_map(|where_| where_.before.iter()) .filter_map(|line| line.split_whitespace().next()) @@ -1607,18 +1636,20 @@ mod tests { #[test] /// `command.before` on a check no shim can consult. /// - /// The third member of the same family, and the one that was missing. - /// `shim::run` filters its checkers to `exec` rules, so a built-in whose + /// The third member of the same family. `shim::run` consults `exec` + /// checkers and text-capable BUILT-INS, so a rule that is neither and whose /// only declared place is `command.before` is consulted by nothing and runs /// nowhere -- and the "nothing says where it runs" refusal is satisfied by /// the very field that cannot be used, so the check meant to catch a rule /// with no place is the one this rule walked past. fn a_command_place_the_check_cannot_use_is_refused() { - // The regexp case carries `files.*` too: it is a rule that really does - // run, by the scan, and the `command.before` beside it is the part that - // reaches nothing. + // A built-in that reads the index, an identity or a push range has + // nothing to say about the text a command publishes. The regexp case + // carries `files.*` too: it is a rule that really does run, by the scan, + // and the `command.before` beside it is the part that reaches nothing. for check in [ - "builtin = \"prevent-ai-author\"", + "builtin = \"prevent-public-push\"", + "builtin = \"prevent-unusual-unicode-in-files\"", "message = \"no\"\nregexp = \"TODO\"\nfiles.include = [\".\"]", ] { let error = policy_from(&format!( @@ -1632,6 +1663,38 @@ mod tests { } } + #[test] + /// A text-capable built-in may stand in front of a command. + /// + /// It is the only seam some of them belong at. `no-private-repo-names` + /// reads a commit message at every git hook, which refuses the issue + /// citations a repository's own prose is full of -- so a repository that + /// wants it over a pull-request body and nowhere else has no other field to + /// say it in. Three wrote `command.before` on the built-in independently + /// while the loader refused all three, on the true-but-unhelpful grounds + /// that a built-in is not an `exec`. + fn a_text_capable_builtin_may_stand_in_front_of_a_command() { + for builtin in crate::guard::TEXT_GUARDS { + let loaded = policy_from(&format!( + "[[shim]]\ncommand = \"gh\"\nmatch = [\"pr:create\"]\n\ + text_flags = [\"-b\"]\n\n\ + [rule.stands-in-front]\nbuiltin = \"{builtin}\"\n\n\ + [rule.stands-in-front.command]\nbefore = [\"gh\"]\n" + )); + assert!(loaded.is_ok(), "{builtin}: {:?}", loaded.err()); + let policy = loaded.unwrap(); + let rule = policy + .rules + .iter() + .find(|rule| rule.id == "stands-in-front"); + assert!( + rule.is_some(), + "{builtin}: the rule did not survive the load" + ); + assert_eq!(rule.unwrap().seams(), vec!["shim"], "{builtin}"); + } + } + /// A `command` table that names no command line is a place that selects /// nothing, and the "where does it run" check reads it as a place. #[test] diff --git a/src/guard/mod.rs b/src/guard/mod.rs index 7e068b4..7cb1bf8 100644 --- a/src/guard/mod.rs +++ b/src/guard/mod.rs @@ -169,25 +169,41 @@ pub(crate) fn over_text( ) -> Result> { let mut refusals = Vec::new(); for rule in policy.of_check(Check::Builtin) { - let Some(builtin) = rule.builtin() else { - continue; - }; - if !TEXT_GUARDS.contains(&builtin) || bypassed(&rule.id) { - continue; - } - let found = match builtin { - "prevent-ai-author" => message::ai_author_in(rule, label, text), - "prevent-unusual-unicode" => message::unusual_unicode_in(rule, label, text), - "no-private-repo-names" => names::in_text(root, rule, label, text)?, - _ => None, - }; - if let Some(refusal) = found { + if let Some(refusal) = text_refusal(root, rule, label, text)? { refusals.push(refusal); } } Ok(refusals) } +/// One text-capable guard's verdict over one piece of text. +/// +/// `None` where the rule is not a text guard, or is bypassed, or found nothing +/// -- three different reasons a caller does not have to tell apart, because a +/// guard with nothing to say about a pull-request body is not a guard that +/// passed it. Extracted so the shim seam consults exactly the same dispatch +/// `uphold guard --text` does: a text guard that judged a commit message one +/// way and a PR body another would be two rules under one id. +pub(crate) fn text_refusal( + root: &Path, + rule: &Rule, + label: &str, + text: &str, +) -> Result> { + let Some(builtin) = rule.builtin() else { + return Ok(None); + }; + if !TEXT_GUARDS.contains(&builtin) || bypassed(&rule.id) { + return Ok(None); + } + Ok(match builtin { + "prevent-ai-author" => message::ai_author_in(rule, label, text), + "prevent-unusual-unicode" => message::unusual_unicode_in(rule, label, text), + "no-private-repo-names" => names::in_text(root, rule, label, text)?, + _ => None, + }) +} + /// Run one guard. pub(crate) fn evaluate(request: &Request<'_>) -> Result> { let id = request.rule.id.as_str(); diff --git a/src/shim.rs b/src/shim.rs index 3ce16da..6fbcf3d 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -1117,9 +1117,19 @@ pub(crate) fn run(root: &Path, policy: &Policy, name: &str, argv: &[OsString]) - // also asked about a branch name on `git push` and a tarball on `npm // publish` -- because the only thing selecting it was `kind = "command"`, // which says nothing about which command. + // Two kinds stand in front of a command, and both are scoped by the same + // `command.before`. An `exec` checker is a program this repository names. + // A text-capable BUILT-IN is one the binary already carries -- and a + // repository that wanted `no-private-repo-names` over a pull-request body + // had no way to say so: the guard reads a commit message at every git hook, + // which refuses the issue citations a repository's own prose is full of, so + // the seam it belongs at is this one and only this one. Three repositories + // wrote `command.before` on the built-in independently and the loader + // refused all three, on the true-but-unhelpful grounds that a built-in is + // not an `exec`. The field means what they meant now. let checkers: Vec<&Rule> = policy .before_command(name, &words) - .filter(|rule| rule.is(Check::Exec)) + .filter(|rule| rule.is(Check::Exec) || rule.is(Check::Builtin)) .collect(); let mut refusals: Vec = Vec::new(); @@ -1152,6 +1162,17 @@ pub(crate) fn run(root: &Path, policy: &Policy, name: &str, argv: &[OsString]) - if crate::guard::bypassed(&rule.id) { continue; } + if rule.is(Check::Builtin) { + // The same dispatch `uphold guard --text` runs, so a + // guard cannot judge a commit message one way and a + // pull-request body another under one id. + if let Some(refusal) = + crate::guard::text_refusal(root, rule, subject.kind, &subject.value)? + { + refusals.push(refusal.report); + } + continue; + } if let Some(refusal) = consult(root, rule, subject)? { refusals.push(format!("{refusal}\n{}", rule.message())); } diff --git a/tests/shim_cli.rs b/tests/shim_cli.rs index da83847..285155a 100644 --- a/tests/shim_cli.rs +++ b/tests/shim_cli.rs @@ -385,3 +385,71 @@ fn an_argument_that_is_not_text_reaches_the_command_it_was_typed_for() { stdout(&output) ); } + +/// A text-capable built-in standing in front of a command, with no `exec` +/// checker anywhere and no git hook. +/// +/// The seam some guards belong at and could not name. `no-private-repo-names` +/// reads a commit message at every git hook, which refuses the issue citations +/// a repository's own prose is full of -- so a repository that wants it over a +/// pull-request body and NOWHERE else had no field to say it in, and three +/// wrote `command.before` on the built-in independently while the loader +/// refused all three. +const BUILTIN_CHECKER: &str = r#" +[[shim]] +command = "faux" +match = ["pr:create"] +text_flags = ["-t", "--title", "-b", "--body"] +scope = "always" + +[rule.no-private-repo-names] +builtin = "no-private-repo-names" +visibility = "public" +private_owners = ["acme-private"] + +[rule.no-private-repo-names.command] +before = ["faux"] +"#; + +#[test] +fn a_text_capable_builtin_refuses_the_body_it_stands_in_front_of() { + let root = workspace(BUILTIN_CHECKER); + let output = shim( + &root, + &[ + "faux", + "pr", + "create", + "-b", + "this fixes acme-private/thing", + ], + ); + assert_eq!(code(&output), 1, "{}", stdout(&output)); + assert!( + stderr(&output).contains("acme-private"), + "{}", + stderr(&output) + ); + // Refused means not published: the real command must not have run. + assert!(!stdout(&output).contains("faux ran"), "{}", stdout(&output)); +} + +#[test] +fn a_clean_body_reaches_the_real_command_through_a_builtin_checker() { + let root = workspace(BUILTIN_CHECKER); + let output = shim(&root, &["faux", "pr", "create", "-b", "an ordinary change"]); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!(stdout(&output).contains("faux ran"), "{}", stdout(&output)); +} + +#[test] +fn a_builtin_checker_satisfies_the_shim_that_would_otherwise_check_nothing() { + // The load refuses a shim no checker names, because a command collected and + // consulted by nothing runs anyway -- an invocation that passed because + // nothing looked at it. A built-in is a checker, and counting only `exec` + // rules refused a policy whose shim WAS checked, by a guard rather than a + // script. + let root = workspace(BUILTIN_CHECKER); + let output = shim(&root, &["faux", "--version"]); + assert_eq!(code(&output), 0, "{}{}", stdout(&output), stderr(&output)); +}