From f2121dee10dd5bc7b020b044cf06fe3e89aefe7d Mon Sep 17 00:00:00 2001 From: HackingGate Date: Wed, 9 Sep 2026 22:33:11 +0900 Subject: [PATCH 1/3] A declared visibility answers the shim, and a forge's silence says which silence it was Two changes to the one seam, and the second is the one that was actually reported. The shim's `public-target` scope ran `gh api repos// --jq .visibility` on every push, including in a repository whose policy states `visibility = "private"` at the top of the file. The guard path has always read that declaration first; this seam never read it at all. It does now, for one destination only: the one `resolve_target` took from `origin` because nothing on the command line named another. A `target_flags` value is a destination somebody typed and a typed `owner/repo` carries no host, so under a `--hostname` or a `GH_HOST` the same two names are a different forge's repository with its own visibility -- and two matching path segments cannot tell those apart. Every named destination goes to the forge exactly as it did before, including the ones that name this repository. The second change is why the first was not enough. Skipping that lookup left `no-stale-visibility` to make the next call, which is correct -- it exists to catch a repository that went public while the policy still says private, and it must reach the network to do it. What was wrong was what it said when the network did not answer. `guard::names::lookup` already separated a 404 from everything else; `no_stale_visibility` then collapsed both into one message explaining what a 404 means, so an account four minutes into a rate limit read a paragraph about deleted and renamed repositories. The shim seam drew no line at all: any non-zero exit was one indistinguishable `None`. So a failed forge call is classified once, in `shim::Silence` -- NotFound, RateLimited, Refused -- and the shim seam, `no-stale-visibility` and `prevent-public-push` all read it, because three classifiers would be free to disagree about one exit code and only one of them could be right. A rate limit now names the wait, asked of `gh api rate_limit`, the one endpoint GitHub exempts from the limit it is reporting. No verdict moves. Every one of these is still exit 2, still not a pass, and still not cached -- `src/config.rs` says why, and a cache that outlives the run is a stale answer with a longer life. The point is that a reader who cannot tell a rate limit from a deleted repository reaches for `UPHOLD_ALLOW` either way. Claude-Session: https://claude.ai/code/session_01HertdiAvdNKGyjR91jvRUM --- docs/REFERENCE.md | 76 ++++- src/guard/names.rs | 44 ++- src/guard/push.rs | 54 ++-- src/guard/visibility.rs | 31 +- src/shim.rs | 628 ++++++++++++++++++++++++++++++++++++++-- tests/guard_cli.rs | 87 ++++++ tests/shim_cli.rs | 257 ++++++++++++++++ 7 files changed, 1099 insertions(+), 78 deletions(-) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index d1e9ba2..7b4d8dc 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -465,6 +465,23 @@ checkers down exactly where the lookup failed. That is exit `2` before the command runs, and `unresolved = "run"` on the `[[shim]]` table is the opt-out — see [the shims](#uphold-shim--the-shims). +The rows above are what the forge said, and the shim reaches them only where it +had to ask at all. A repository that declares its own `visibility` in its policy +file has answered the question already, so `public-target` reads the declaration +and asks nobody -- `public` puts the checks in scope, `private` and `internal` +stand them down, and no `gh` runs. That declaration speaks for this repository +and for no other, so it is read for one destination only: the one the shim took +from `origin` because nothing on the command line named another. A `gh -R +owner/repo` goes to the forge exactly as it always did, and it does so **even +where the name it carries is this repository's own** -- a typed `owner/repo` +has no host in it, so under a `--hostname` or a `GH_HOST` the same two names +are a different forge's repository with its own visibility, and two matching +path segments cannot tell the two apart. So the `2` above is still the answer +for a named destination the forge could not be asked about, and for a repository +that declares nothing; what is bought offline is the case the cost was actually +in -- the push that names nothing and means `origin`, which no longer needs an +authenticated `gh` to be told what the policy already said. + So **`gh` must be authenticated wherever these rules run**, CI included. In a GitHub Actions job that means `GH_TOKEN: ${{ github.token }}`; the job token reads this repository and public ones and answers `404` for everything else, @@ -1161,7 +1178,7 @@ stamped on it, the range about to be pushed. | `no-merge-commit` | a commit finishing a merge or a squash merge | | `no-stale-hook-pins` | a pin left behind its upstream, or naming no ref — in `.pre-commit-config.yaml` **and** lefthook `remotes:`, at any depth in the tree; a pin it **could not check** is exit `2` | | `no-hand-copied-base-rule` | a rule this policy writes out by hand under an id a bundled set already ships, from a set it does not inherit. Reads the **policy**, not the tree. At `pre-commit` only what the change adds; at `manual` the whole sweep | -| `no-stale-visibility` | a declared `private` the forge no longer serves. Reads the **declaration** and the forge, not the tree; a forge that did not answer is exit `2` and never "confirmed private" | +| `no-stale-visibility` | a declared `private` the forge no longer serves. Reads the **declaration** and the forge, not the tree; a forge that did not answer is exit `2` and never "confirmed private", and it says which silence it met — a 404 reads differently from a rate limit | Declared like any other rule, in the same file and the same id namespace. **`git.hooks` is the whole registration.** @@ -1504,8 +1521,27 @@ nested form is the enum's own spelling; the flatter one this page used to show never parsed.) `collect = "git-refs"` replaces the argv walk for `git`, whose published text is positional. -**A scope that could not be evaluated is not a scope that said no.** -`public-target` is the one predicate that asks somebody else, and `gh` +**`public-target` asks this repository before it asks anybody else.** The +policy's own top-level `visibility` is a statement of the fact the predicate +needs, so where there is one it settles the question offline: `public` is in +scope, `private` and `internal` are out of it, and nothing is spawned. The +narrowing that makes reading it safe is that a declaration is about ONE +repository, and the only destination this seam can be sure of is the one it +derived itself. So the declaration is read where a `target_flags` value is +absent -- where the destination came from `origin` -- and nowhere else. A named +destination goes to the forge whatever it names: `gh -R other/repo` obviously, +and `gh -R this-owner/this-repo` too, because the name carries no host and +`--hostname` and `GH_HOST` both make `owner/repo` a different repository on a +different forge. A tree whose `origin` cannot be read has no destination for a +declaration to be about, and a policy that declares nothing changes nothing. +The guard seam does the same thing one step less strictly (see +`guard::names::target_is_public`), and the difference is deliberate: a guard +reads text belonging to this repository, while a shim stands in front of a +command that may be publishing somewhere else entirely. + +**A scope that could not be evaluated is not a scope that said no.** Where the +question does reach a forge, `public-target` is the one predicate that asks +somebody else, and `gh` unauthenticated, a rate limit, no network, or a repository with no `origin` all answer nothing. That was read as "out of scope", which stood every checker behind the table down — including `prevent-unowned-target`, whose own contract @@ -1518,6 +1554,31 @@ reach `public-target` — neither the table's own `scope` nor the `command.scope of any rule naming the command — because a parameter nothing reads is configuration that looks like it works. +**A silence is told apart from the other silences.** "The forge did not say" was +one sentence for four different situations, and the one thing the reader needed +in order to act was the one thing it left out: a 404, an unauthenticated `gh`, a +rate limit and a `gh` that is not installed are four instructions, not one. So a +failed forge call is classified once — in `shim::Silence`, which the shim seam, +`no-stale-visibility` and `prevent-public-push` all read, because three +classifiers would be free to disagree about one exit code and only one of them +could be right — and the refusal carries the cause: + +```text +uphold shim: the forge did not say whether acme/widget is public, so whether the +`public-target` checks apply here could not be established. The forge is +rate-limiting this client; the budget resets in about 15 minutes. This is not an +answer about the repository -- it is the same silence for a public one and a +private one. +``` + +The reset time is the one question this tool asks in reply to a failure, and it +asks the one endpoint GitHub exempts from the limit it is reporting, so the ask +cannot deepen the hole it is describing. **None of this changes a verdict.** +Every one of these is still exit `2`, still not a pass, and still not a cached +answer — a wait somebody can sit out is worth naming precisely because the +alternative on offer is `UPHOLD_ALLOW`, and a reader who cannot tell a rate limit +from a deleted repository reaches for the bypass either way. + **An alias is expanded before the `match` list is consulted.** `match` names verbs literally, and every one of these commands lets a person rename one: `git -c alias.p=push p origin HEAD:refs/heads/x`, a persisted `[alias] p = @@ -1671,9 +1732,12 @@ than the seam. The fourth is uphold getting out of its own way, and it is documented here rather than left implicit because a reader who meets the line deserves to know -what set it. A `public-target` scope asks the forge whether the destination is -public, and it asks by running `gh api repos// --jq .visibility`; -a `git-remote` target asks by running `git remote get-url origin`. PATH answers +what set it. A `public-target` scope asks whether the destination is public, and +where this repository has not declared its own `visibility` -- or where the +command named a destination, which a declaration does not speak for -- it asks +by running `gh api repos// --jq .visibility`; a `git-remote` target +asks by running `git remote get-url origin`, which is how an unnamed destination +is resolved in the first place and so runs either way. PATH answers `gh` and `git` with the shim, so **every question this tool asks on the way to a verdict is a command it stands in front of**. On 2026-09-02 that closed into a loop against the released binary inside this repository's own checkout: the diff --git a/src/guard/names.rs b/src/guard/names.rs index 3b81765..3ce3f37 100644 --- a/src/guard/names.rs +++ b/src/guard/names.rs @@ -42,6 +42,7 @@ use super::{Refusal, Request, Stage}; use crate::config::{Policy, Rule}; use crate::error::{Fatal, Result}; use crate::git; +use crate::shim::Silence; /// A repository name, with what a sentence put on the end taken off. /// @@ -96,6 +97,15 @@ pub(crate) enum Visibility { pub(crate) struct Resolved { pub visibility: Visibility, pub canonical: Option, + /// Why there was no answer, where there was none. + /// + /// `Visibility` says only that the check did not happen; this says which of + /// the four ways it did not, in the words the forge used. A caller that + /// refuses over an unchecked claim owes the reader that much -- "the forge + /// did not say" is the same sentence for a deleted repository and for an + /// account that is over its budget for the next eleven minutes, and only one + /// of those is worth waiting out. + pub silence: Option, } /// Any `host.tld/owner/repo` or its scp-like `host.tld:owner/repo`, with the @@ -383,28 +393,34 @@ pub(crate) fn lookup(cache: &mut BTreeMap, owner: &str, repo: Resolved { visibility, canonical, + silence: None, } } - // The forge said no. WHICH no it said is the whole question, and it is - // in stderr: `gh` writes `gh: Not Found (HTTP 404)` for a name it will - // not show us, and `gh: Bad credentials (HTTP 401)` for a client it - // will not talk to. Only the first is a fact about the name. + // The forge said no. WHICH no it said is the whole question, and + // [`Silence`] is where that line is drawn -- once, for this seam and for + // the two others that ask a forge the same kind of question. Only a + // `NotFound` is a fact about the name. // // Anything else -- 401, 403 and a rate limit, 5xx, a status line that // is not there, or no `gh` at all -- is the check not happening, and // reporting that as an inconclusive finding is how an unauthenticated // run passed every name in the tree. - Ok(output) => Resolved { - visibility: if String::from_utf8_lossy(&output.stderr).contains("(HTTP 404)") { - Visibility::Unknown - } else { - Visibility::Unavailable - }, - canonical: None, - }, - Err(_) => Resolved { + Ok(output) => { + let silence = Silence::of("gh", &output); + Resolved { + visibility: if silence == Silence::NotFound { + Visibility::Unknown + } else { + Visibility::Unavailable + }, + canonical: None, + silence: Some(silence), + } + } + Err(error) => Resolved { visibility: Visibility::Unavailable, canonical: None, + silence: Some(Silence::unreachable("gh", &error)), }, }; cache.insert(key, resolved.clone()); @@ -626,6 +642,7 @@ fn judge( Resolved { visibility: Visibility::Private, canonical: None, + silence: None, } } else { lookup(&mut cache, &owner, &repo) @@ -1306,6 +1323,7 @@ mod tests { Resolved { visibility, canonical: canonical.map(str::to_owned), + silence: None, } } diff --git a/src/guard/push.rs b/src/guard/push.rs index 6698be1..d87280e 100644 --- a/src/guard/push.rs +++ b/src/guard/push.rs @@ -32,7 +32,7 @@ use super::{Refusal, Request}; use crate::config::{Policy, Rule}; use crate::error::Result; use crate::git; -use crate::shim::Forge; +use crate::shim::{Forge, Silence}; /// Which seam is asking, for the two phrases that differ between them. /// @@ -132,23 +132,13 @@ pub(crate) enum Owned { Yes, /// The forge answered, and neither is true. No, - /// No answer: no `gh` on PATH, no credentials, no network, or output that - /// is neither a yes nor a no. Carries the first line of what `gh` said, so - /// the reader is told which of those it was. + /// No answer: no `gh` on PATH, no credentials, a rate limit, no network, or + /// output that is neither a yes nor a no. Carries one whole sentence saying + /// which of those it was, because "the forge could not be asked" is a + /// different instruction to the reader in each case. CouldNotAsk(String), } -/// The first line of what a failed `gh` wrote, which is the part worth printing. -fn first_line(stderr: &[u8]) -> String { - let text = String::from_utf8_lossy(stderr); - let line = text.lines().next().unwrap_or_default().trim().to_owned(); - if line.is_empty() { - String::from("it said nothing") - } else { - line - } -} - /// Ask the forge whether the operator owns this destination, once per run. /// /// Only ever reached for a destination the allow-list has already refused, so @@ -181,11 +171,13 @@ fn ask_forge(owner: &str, key: &str) -> Owned { Ok(output) if output.status.success() => { String::from_utf8_lossy(&output.stdout).trim().to_owned() } - Ok(output) => return Owned::CouldNotAsk(first_line(&output.stderr)), - Err(error) => return Owned::CouldNotAsk(format!("gh could not be run ({error})")), + Ok(output) => return Owned::CouldNotAsk(Silence::of("gh", &output).sentence()), + Err(error) => { + return Owned::CouldNotAsk(Silence::unreachable("gh", &error).sentence()); + } }; if login.is_empty() { - return Owned::CouldNotAsk(String::from("`gh api user` printed no login")); + return Owned::CouldNotAsk(String::from("`gh api user` exited 0 and printed no login.")); } // GitHub logins are case-insensitive, and a pin written in the case the // profile page shows is the same account as one written in the case a url @@ -205,18 +197,22 @@ fn ask_forge(owner: &str, key: &str) -> Owned { // `permissions` block that is not there answers `null`. Neither // is a no about ownership. other => Owned::CouldNotAsk(format!( - "`gh api repos/{key}` answered {other:?}, which is neither true nor false" + "`gh api repos/{key}` answered {other:?}, which is neither true nor false." )), } } - // A 404 here is a definite no, and only because the request above - // succeeded: the client works and is authenticated, and a forge that - // will not show this repository to that identity is not a forge saying - // the identity administers it. Every other failure -- 401, 403, a rate - // limit, 5xx -- is the check not happening. - Ok(output) if String::from_utf8_lossy(&output.stderr).contains("(HTTP 404)") => Owned::No, - Ok(output) => Owned::CouldNotAsk(first_line(&output.stderr)), - Err(error) => Owned::CouldNotAsk(format!("gh could not be run ({error})")), + // Classified once, by the one classifier this binary has for the + // question -- see [`Silence`]. A 404 here is a definite no, and only + // because the request above succeeded: the client works and is + // authenticated, and a forge that will not show this repository to that + // identity is not a forge saying the identity administers it. Every + // other failure -- 401, 403, a rate limit, 5xx -- is the check not + // happening, and each of them says so in its own words. + Ok(output) => match Silence::of("gh", &output) { + Silence::NotFound => Owned::No, + silence => Owned::CouldNotAsk(silence.sentence()), + }, + Err(error) => Owned::CouldNotAsk(Silence::unreachable("gh", &error).sentence()), } } @@ -412,8 +408,8 @@ pub(crate) fn unowned( Some(Owned::CouldNotAsk(why)) => { return Err(crate::error::Fatal::new(format!( "rule {:?}: {report}\n\nThe forge could not be asked whether you own {name}, \ - so the allow-list is the only answer there is and it is not a whole one: \ - {why}. Could not look is not a pass. Authenticate `gh`, or name the \ + so the allow-list is the only answer there is and it is not a whole one. \ + {why} Could not look is not a pass. Authenticate `gh`, or name the \ destination on the rule, or bypass this run deliberately with \ UPHOLD_ALLOW={}.", rule.id, rule.id diff --git a/src/guard/visibility.rs b/src/guard/visibility.rs index fff7237..b860c39 100644 --- a/src/guard/visibility.rs +++ b/src/guard/visibility.rs @@ -32,6 +32,7 @@ use super::{Refusal, Request}; use crate::config::visibility_is_public; use crate::error::{Fatal, Result}; use crate::git; +use crate::shim::Silence; /// Refuse a declared privacy the forge has stopped serving. /// @@ -103,7 +104,8 @@ pub(crate) fn no_stale_visibility(request: &Request<'_>) -> Result Ok(Some(Refusal { id: id.clone(), report: format!( @@ -119,14 +121,17 @@ pub(crate) fn no_stale_visibility(request: &Request<'_>) -> Result Err(Fatal::new(format!( + // one that was deleted or renamed. + Visibility::Unknown => Err(Fatal::new(format!( "{id}: the forge did not say whether {owner}/{repo} is public, so the declared \ {declared:?} was not checked. A 404 is a private repository, a deleted one, a \ renamed one, and a request that carried no credentials -- this rule can \ @@ -134,5 +139,19 @@ pub(crate) fn no_stale_visibility(request: &Request<'_>) -> Result Err(Fatal::new(format!( + "{id}: the forge was not able to say whether {owner}/{repo} is public, so the \ + declared {declared:?} was not checked. {}\n\nThis rule can disprove a claim of \ + privacy and can never confirm one, so it does not read silence as agreement. \ + Could not look is not a pass -- bypass this run deliberately with \ + UPHOLD_ALLOW={id}.", + resolved.silence.as_ref().map_or_else( + || String::from("The forge gave no reason."), + Silence::sentence + ) + ))), } } diff --git a/src/shim.rs b/src/shim.rs index f064c3d..c05c872 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -1504,10 +1504,17 @@ impl Shim { /// pull-request body is worth refusing whatever the destination -- says so /// beside its `command.before` rather than inheriting the table's idea of /// the question. + /// + /// The policy is a parameter for the sake of one arm: `public-target` is a + /// question this repository may already have answered in writing, and a + /// predicate that asks a forge anyway is a network call -- and a token, and + /// a rate limit -- spent on a fact that was sitting in `principles.toml` + /// the whole time. See [`declared_standing`]. pub(crate) fn scope_holds( &self, scope: &Scope, root: &Path, + policy: &Policy, collected: &Collected, argv: &[String], ) -> Result { @@ -1525,12 +1532,26 @@ impl Shim { apply here could not be established", ))); }; + // What this repository says about itself, before anybody is + // asked. Read here and not before the resolver, because a tree + // with no `origin` has no destination for a declaration to be + // about, and that is the resolver's answer above rather than + // this one's. + if let Some(standing) = declared_standing(root, policy, collected)? { + return Ok(standing); + } match self.visibility(root, &target).as_deref() { - Some("public") => Ok(Standing::Holds), - Some(_) => Ok(Standing::DoesNotHold), - None => Ok(Standing::CouldNotTell(format!( + Ok("public") => Ok(Standing::Holds), + Ok(_) => Ok(Standing::DoesNotHold), + // The cause, in the words the forge used. It used to be + // dropped here: a rate limit, an unauthenticated `gh` and a + // deleted repository were one sentence, and the one thing + // the reader needed in order to act was the one thing it + // did not say. + Err(silence) => Ok(Standing::CouldNotTell(format!( "the forge did not say whether {target} is public, so whether the \ - `public-target` checks apply here could not be established" + `public-target` checks apply here could not be established. {}", + silence.sentence() ))), } } @@ -1601,8 +1622,13 @@ impl Shim { /// neither public to the internet nor private. Only `public` is treated as /// public by the caller, so a forge that grows a fourth word does not /// quietly become one of the three. - fn visibility(&self, root: &Path, target: &str) -> Option { - match self.forge(root)? { + fn visibility(&self, root: &Path, target: &str) -> std::result::Result { + let Some(forge) = self.forge(root) else { + return Err(Silence::Refused(String::from( + "no forge client is known for that host, so there was nobody to ask", + ))); + }; + match forge { Forge::GitHub => forge_field( "gh", &["api", &format!("repos/{target}"), "--jq", ".visibility"], @@ -1686,25 +1712,234 @@ impl Forge { } } +/// What the policy already says about this destination, where it speaks for it. +/// +/// `Some` is a declaration that settles the question offline, and it is the same +/// declaration `guard::names::target_is_public` reads: a repository whose +/// `principles.toml` says `visibility = "private"` has stated the fact, and a +/// shim that ran `gh api repos//` anyway needed an authenticated +/// `gh` on every `git push` to learn it a second time. When GitHub rate-limits +/// the account that lookup fails, the failure is a [`Standing::CouldNotTell`], +/// and `unresolved = "refuse"` -- the default -- stops every push in every tree +/// over a fact nobody had to ask for. +/// +/// `None` sends the caller to the forge, and the narrowing that makes this safe +/// is the destination it will read the declaration for. A declaration is about +/// ONE repository, so this reads it only where the destination is the one +/// [`Shim::resolve_target`] took from `origin` -- which is to say where nothing +/// on the command line named a destination at all. A `target_flags` value is a +/// destination somebody typed, and a typed `owner/repo` carries no host: `-R +/// acme/widget` under a `--hostname` or a `GH_HOST` is a repository on another +/// forge that happens to share a name with this one, and there is nothing in +/// two matching path segments that can tell it apart from this tree. So every +/// named destination goes to the forge exactly as it did before, including the +/// ones that name this repository, and what is bought offline is the case the +/// cost was actually in: the push that names nothing and means `origin`. +fn declared_standing( + root: &Path, + policy: &Policy, + collected: &Collected, +) -> Result> { + if collected + .target + .as_deref() + .is_some_and(|named| !named.is_empty()) + { + return Ok(None); + } + // Lazily, and only here. Reading it is allowed to run `visibility_from`, which + // is a process, and a predicate that spawned one for a target the declaration + // does not speak for would trade the lookup this exists to avoid for another + // one. + let Some(declared) = policy.declared_visibility(root)? else { + return Ok(None); + }; + // Held to the three spellings before it arrives -- a written one at load, a + // `visibility_from` answer as it is read -- so `None` here is unreachable + // rather than a fourth word being guessed at. It falls through to the forge + // regardless: a word this seam cannot read is not an answer this seam may + // invent. + Ok( + crate::config::visibility_is_public(&declared).map(|public| { + if public { + Standing::Holds + } else { + Standing::DoesNotHold + } + }), + ) +} + +/// Why a forge CLI gave no answer, told apart by what it printed. +/// +/// One classifier rather than three. `guard::names::lookup`, `guard::push` and +/// [`forge_field`] all ask a forge a question that can fail four ways, and all +/// three used to draw their own line -- or, at this seam, none at all: a +/// non-zero exit was one indistinguishable `None`, so a rate limit, an +/// unauthenticated `gh` and a deleted repository arrived at the user as the same +/// sentence about a 404. Three classifiers would be free to disagree about one +/// `Output` and only one of them could be right. +/// +/// The line is in stderr, because `gh api` writes the API's error BODY to +/// stdout: `gh: Not Found (HTTP 404)` for a name it will not show us, `gh: Bad +/// credentials (HTTP 401)` for a client it will not talk to, and an "API rate +/// limit exceeded" for a client it has stopped talking to for now. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum Silence { + /// The forge answered, and the answer was that it will show us no + /// repository by this name. A fact about the name -- though not ONE fact: + /// private, deleted, renamed, mistyped, and a request that carried no + /// credentials all land here. + NotFound, + /// The client is over its budget. Not a fact about the name at all, and the + /// one silence that ends by itself -- so it carries the wait where the forge + /// would say. + RateLimited { resets_in_minutes: Option }, + /// Anything else: no credentials, no network, no `gh`, a 5xx, a status line + /// that is not there. The first line of what the tool said, because a + /// message that names the cause is the difference between a check somebody + /// fixes and a check somebody bypasses. + Refused(String), +} + +impl Silence { + /// Classify a failed run of `program`. + /// + /// The rate-limit arm asks one further question, and it is the only question + /// in this binary asked in reply to a failure: `gh api rate_limit` is the + /// endpoint GitHub exempts from the limit it is reporting, so the ask cannot + /// deepen the hole it is describing. `None` for the wait where even that did + /// not answer -- the limit is still the fact, and the clock is the courtesy. + pub(crate) fn of(program: &str, output: &std::process::Output) -> Self { + let stderr = String::from_utf8_lossy(&output.stderr); + if stderr.contains("(HTTP 404)") { + return Self::NotFound; + } + if stderr.to_lowercase().contains("rate limit") { + return Self::RateLimited { + resets_in_minutes: rate_limit_minutes(program), + }; + } + Self::Refused(first_line(&stderr).unwrap_or_else(|| { + format!( + "`{program}` exited {}", + output + .status + .code() + .map_or_else(|| String::from("on a signal"), |code| code.to_string()) + ) + })) + } + + /// The forge could not be run at all -- no `gh` on PATH, no permission to + /// exec it. Distinct from a `gh` that ran and refused, and reported that way. + pub(crate) fn unreachable(program: &str, error: &std::io::Error) -> Self { + Self::Refused(format!("`{program}` could not be run ({error})")) + } + + /// The clause a caller puts after "the forge did not say ...". + /// + /// One sentence per cause, and the point of having them is that they are + /// not interchangeable: a rate limit is waited out, an unauthenticated + /// client is fixed, a 404 is five different repositories at once. A caller + /// with a better sentence for its own question is free to write one -- + /// `guard::visibility` does, because "no repository by that name" means + /// something specific when the name is your own `origin`. + pub(crate) fn sentence(&self) -> String { + match self { + Self::NotFound => String::from( + "The forge will show no repository by that name, which is a private one, a \ + deleted one, a renamed one, and a request that carried no credentials.", + ), + Self::RateLimited { resets_in_minutes } => resets_in_minutes.map_or_else( + || { + String::from( + "The forge is rate-limiting this client. This is not an answer about \ + the repository -- it is the same silence for a public one and a \ + private one.", + ) + }, + |minutes| { + format!( + "The forge is rate-limiting this client; the budget resets in about \ + {minutes} minute{}. This is not an answer about the repository -- it \ + is the same silence for a public one and a private one.", + if minutes == 1 { "" } else { "s" } + ) + }, + ), + Self::Refused(what) => format!("The forge was asked and said: {what}"), + } + } +} + +/// Minutes until the client's forge budget resets, where the forge will say. +fn rate_limit_minutes(program: &str) -> Option { + if program != "gh" { + return None; + } + let output = inner_tool(program) + .args(["api", "rate_limit", "--jq", ".rate.reset"]) + .output() + .ok()?; + if !output.status.success() { + return None; + } + let reset: u64 = String::from_utf8_lossy(&output.stdout) + .trim() + .parse() + .ok()?; + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .ok()? + .as_secs(); + // Rounded UP, so "0 minutes" is never printed for a wait somebody still has + // to sit through. A reset already in the past is a clock that disagrees with + // the forge's, and a wait nobody should be told to take. + Some(reset.saturating_sub(now).div_ceil(60)).filter(|minutes| *minutes > 0) +} + +/// The first non-empty line of what a tool printed, trimmed. +fn first_line(text: &str) -> Option { + text.lines() + .map(str::trim) + .find(|line| !line.is_empty()) + .map(str::to_owned) +} + /// Run a forge CLI and read one word out of what it printed. /// /// `field` names a JSON key to pull out where the CLI cannot be asked to do it; /// `None` means the whole of stdout is the answer. -fn forge_field(program: &str, args: &[&str], field: Option<&str>) -> Option { - let output = inner_tool(program).args(args).output().ok()?; +fn forge_field( + program: &str, + args: &[&str], + field: Option<&str>, +) -> std::result::Result { + let output = match inner_tool(program).args(args).output() { + Ok(output) => output, + Err(error) => return Err(Silence::unreachable(program, &error)), + }; if !output.status.success() { - return None; + return Err(Silence::of(program, &output)); } let text = String::from_utf8_lossy(&output.stdout); - let value = match field { - Some(field) => json_string_field(&text, field)?, - None => text.trim().to_owned(), - }; - let value = value.trim().to_lowercase(); + let value = field.map_or_else( + || Some(text.trim().to_owned()), + |field| json_string_field(&text, field), + ); + let value = value.unwrap_or_default().trim().to_lowercase(); // An empty answer is not an answer. `--jq` on a field that is not there // prints a blank line and exits 0, and treating that as a visibility would - // be a lookup that did not happen wearing the face of one that did. - (!value.is_empty() && value != "null").then_some(value) + // be a lookup that did not happen wearing the face of one that did. It is + // its own silence rather than a refusal, because a run that SUCCEEDED and + // said nothing is not a forge that would not talk to us. + if value.is_empty() || value == "null" { + return Err(Silence::Refused(format!( + "`{program}` exited 0 and printed no visibility" + ))); + } + Ok(value) } /// Parse the document and read a TOP-LEVEL key, rather than scan for a needle. @@ -2313,10 +2548,11 @@ impl ScopeMemo { shim: &Shim, scope: &Scope, root: &Path, + policy: &Policy, collected: &Collected, argv: &[String], ) -> Result { - match self.standing(shim, scope, root, collected, argv)? { + match self.standing(shim, scope, root, policy, collected, argv)? { Standing::Holds => Ok(true), Standing::DoesNotHold => Ok(false), Standing::CouldNotTell(why) => match shim.unresolved { @@ -2343,6 +2579,7 @@ impl ScopeMemo { shim: &Shim, scope: &Scope, root: &Path, + policy: &Policy, collected: &Collected, argv: &[String], ) -> Result { @@ -2355,7 +2592,7 @@ impl ScopeMemo { if let Some(answer) = self.answers.get(&key) { return Ok(answer.clone()); } - let answer = shim.scope_holds(scope, root, collected, argv)?; + let answer = shim.scope_holds(scope, root, policy, collected, argv)?; if let (Standing::CouldNotTell(why), Unresolved::Run) = (&answer, shim.unresolved) { eprintln!("uphold shim: {why}, and the command ran anyway. This is not a pass."); } @@ -2468,6 +2705,7 @@ fn edit_and_check(root: &Path, policy: &Policy, name: &str, argv: &[String]) -> shim, effective_scope(rule, shim), root, + policy, collected, &opened_for, )? { @@ -2522,6 +2760,7 @@ fn edit_and_check(root: &Path, policy: &Policy, name: &str, argv: &[String]) -> shim, effective_scope(inner, shim), root, + policy, collected, &opened_for, ), @@ -2782,9 +3021,16 @@ pub(crate) fn run( ))); } collected = shim.collect(root, &words)?; - let in_scope = scopes.holds(shim, &shim.scope, root, &collected, &words)?; + let in_scope = scopes.holds(shim, &shim.scope, root, policy, &collected, &words)?; for rule in &checkers { - if scopes.holds(shim, effective_scope(rule, shim), root, &collected, &words)? { + if scopes.holds( + shim, + effective_scope(rule, shim), + root, + policy, + &collected, + &words, + )? { any_applies = true; } } @@ -2850,7 +3096,14 @@ pub(crate) fn run( // The rule's own scope where it wrote one, the table's where it // did not -- the same reading the text path takes, answered // from the same memo. - if !scopes.holds(shim, effective_scope(rule, shim), root, &collected, &words)? { + if !scopes.holds( + shim, + effective_scope(rule, shim), + root, + policy, + &collected, + &words, + )? { continue; } if let Some(refusal) = crate::guard::target_refusal( @@ -2895,7 +3148,14 @@ pub(crate) fn run( // The rule's own scope where it wrote one, the table's // where it did not -- answered from the memo, so a // destination is looked up once however many rules ask. - if !scopes.holds(shim, effective_scope(rule, shim), root, &collected, &words)? { + if !scopes.holds( + shim, + effective_scope(rule, shim), + root, + policy, + &collected, + &words, + )? { continue; } // `subjects` narrows every kind of checker the same way: @@ -2952,6 +3212,7 @@ pub(crate) fn run( shim, effective_scope(inner, shim), root, + policy, &collected, &words, ) @@ -3082,6 +3343,22 @@ mod tests { line.split_whitespace().map(str::to_owned).collect() } + /// A policy that declares nothing, which is the state every case that is + /// not about a declaration is in: `public-target` then asks the forge + /// exactly as it always did. + fn undeclared() -> Policy { + Policy::default() + } + + /// A policy that says what this repository is, the way `principles.toml` + /// says it. + fn declaring(visibility: &str) -> Policy { + Policy { + visibility: Some(String::from(visibility)), + ..Policy::default() + } + } + fn named(shim: &Shim, line: &str) -> bool { matches!(shim.reading(&argv(line)), Reading::Named) } @@ -3280,6 +3557,7 @@ mod tests { npm.scope_holds( &Scope::PublicRegistry, &dir, + &undeclared(), &Collected::default(), &argv("publish") ) @@ -3298,6 +3576,7 @@ mod tests { npm.scope_holds( &Scope::PublicRegistry, Path::new("."), + &undeclared(), &Collected::default(), &argv("publish --dry-run") ) @@ -3595,6 +3874,7 @@ mod tests { gh().scope_holds( &Scope::Always, Path::new("."), + &undeclared(), &Collected::default(), &argv("pr create") ) @@ -3618,6 +3898,7 @@ mod tests { .scope_holds( &Scope::PublicTarget, Path::new("."), + &undeclared(), &Collected::default(), &argv("pr create") ) @@ -3645,7 +3926,297 @@ mod tests { assert_eq!(unknown_host.forge(&dir), None); assert!(matches!( unknown_host - .scope_holds(&Scope::PublicTarget, &dir, &collected, &argv("pr create")) + .scope_holds( + &Scope::PublicTarget, + &dir, + &undeclared(), + &collected, + &argv("pr create") + ) + .unwrap(), + Standing::CouldNotTell(why) if why.contains("did not say whether acme/widget is public") + )); + } + + /// A repository whose `origin` points at `url`, and whose forge no resolver + /// knows. + /// + /// The host is deliberately unrecognised, which is what makes "the forge + /// was not asked" an assertion rather than a hope: the only answer the + /// lookup can give here is [`Standing::CouldNotTell`], so a definite + /// standing coming back out is proof that the declaration answered and + /// nothing was spawned. The `gh` marker file the CLI tests use asserts the + /// same thing from outside; this asserts it where a stray `gh` on the + /// runner's PATH cannot reach. + fn repository_pointing_at(kind: &str, url: &str) -> PathBuf { + let dir = crate::fixture::scratch(kind); + std::fs::create_dir_all(&dir).unwrap(); + for args in [ + &["init", "-q", "-b", "main"][..], + &["remote", "add", "origin", url][..], + ] { + let status = Command::new("git") + .args(args) + .current_dir(&dir) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .unwrap(); + assert!(status.success(), "git {args:?} failed"); + } + dir + } + + /// A shim whose command names no forge, so nothing here can reach one. + fn no_forge_cli() -> Shim { + let mut shim = gh(); + shim.command = String::from("faux"); + shim + } + + #[test] + fn a_declared_private_visibility_answers_without_asking_the_forge() { + // The defect, as it was reported: a repository whose `principles.toml` + // says `visibility = "private"` made a network call on every `git push` + // to learn a fact the policy states offline, and when the account was + // rate-limited the lookup failed and the default `unresolved` refused + // every push in every tree. + let dir = repository_pointing_at( + "shim-declared-private", + "https://forge.example/acme/widget.git", + ); + assert_eq!( + no_forge_cli() + .scope_holds( + &Scope::PublicTarget, + &dir, + &declaring("private"), + &Collected::default(), + &argv("pr create") + ) + .unwrap(), + Standing::DoesNotHold + ); + // And the same fixture with nothing declared, which is where that + // answer would otherwise have had to come from. + assert!(matches!( + no_forge_cli() + .scope_holds( + &Scope::PublicTarget, + &dir, + &undeclared(), + &Collected::default(), + &argv("pr create") + ) + .unwrap(), + Standing::CouldNotTell(why) if why.contains("did not say whether acme/widget is public") + )); + } + + #[test] + fn a_declared_public_visibility_puts_the_checks_in_scope_offline() { + // The other half, and it is the half that must not be lost: standing + // the checks DOWN on a declaration nobody read would be the failure + // this change could cause, so the `public` word has to reach `Holds` + // without a forge exactly as `private` reaches `DoesNotHold`. + let dir = repository_pointing_at( + "shim-declared-public", + "https://forge.example/acme/widget.git", + ); + assert_eq!( + no_forge_cli() + .scope_holds( + &Scope::PublicTarget, + &dir, + &declaring("public"), + &Collected::default(), + &argv("pr create") + ) + .unwrap(), + Standing::Holds + ); + } + + #[test] + fn a_declaration_about_this_repository_says_nothing_about_another_one() { + // Where the shim is stricter than the guard, and it has to be: `-R + // other-owner/their-repo` is an explicit foreign destination, and this + // policy's `visibility` is a statement about this tree. Reading it at + // somebody else's repository would stand the checks down over a fact + // about a different place entirely. + let dir = repository_pointing_at( + "shim-declared-foreign", + "https://forge.example/acme/widget.git", + ); + let elsewhere = Collected { + target: Some(String::from("other-owner/their-repo")), + ..Collected::default() + }; + assert!(matches!( + no_forge_cli() + .scope_holds( + &Scope::PublicTarget, + &dir, + &declaring("private"), + &elsewhere, + &argv("pr create") + ) + .unwrap(), + Standing::CouldNotTell(why) + if why.contains("did not say whether other-owner/their-repo is public") + )); + } + + /// One failed run of `sh`, standing in for a forge CLI that said this. + /// + /// A real `Output` rather than a hand-built one, because the field this + /// classifier reads is stderr and the thing worth asserting is that a + /// process's stderr reaches it. + fn said(stderr: &str, code: i32) -> std::process::Output { + Command::new("sh") + .args(["-c", &format!("printf '%s' \"$1\" >&2; exit {code}")]) + .arg("sh") + .arg(stderr) + .output() + .expect("sh runs") + } + + #[test] + fn a_404_is_the_one_silence_that_is_about_the_name() { + assert_eq!( + Silence::of("glab", &said("glab: Not Found (HTTP 404)", 1)), + Silence::NotFound + ); + } + + #[test] + fn a_rate_limit_is_not_reported_as_a_404() { + // The report this came from: a rate-limited push was refused with a + // paragraph explaining what a 404 means, which is neither what happened + // nor anything the reader could act on. `glab` rather than `gh` so the + // reset is not looked up -- the classification is what is under test. + let silence = Silence::of( + "glab", + &said("glab: API rate limit exceeded for user ID 1. (HTTP 403)", 1), + ); + assert_eq!( + silence, + Silence::RateLimited { + resets_in_minutes: None + } + ); + assert!( + silence.sentence().contains("rate-limiting this client"), + "{}", + silence.sentence() + ); + assert!( + !silence.sentence().contains("404"), + "{}", + silence.sentence() + ); + } + + #[test] + fn an_unauthenticated_client_is_reported_in_the_words_it_used() { + let silence = Silence::of("glab", &said("glab: Bad credentials (HTTP 401)", 1)); + assert_eq!( + silence, + Silence::Refused(String::from("glab: Bad credentials (HTTP 401)")) + ); + assert!( + silence.sentence().contains("Bad credentials"), + "{}", + silence.sentence() + ); + } + + #[test] + fn a_forge_that_said_nothing_at_all_is_still_named_by_its_exit() { + // A tool that fails silently used to arrive as an empty reason inside a + // sentence that promised one. The exit code is the only fact there is, + // so it is the fact reported. + assert_eq!( + Silence::of("glab", &said("", 7)), + Silence::Refused(String::from("`glab` exited 7")) + ); + } + + #[test] + fn a_destination_somebody_typed_is_asked_about_even_where_it_names_this_repository() { + // The narrowing, asserted at the case that looks safest. `-R + // acme/widget` inside `acme/widget` names what appears to be this + // repository, and two matching path segments are still not this + // repository: a typed `owner/repo` carries no host, so under a + // `--hostname` or a `GH_HOST` the same two names are a different forge's + // repository with its own visibility. Nothing here can tell those apart, + // so nothing here answers for either -- the forge does, as it always + // did. + let dir = repository_pointing_at( + "shim-declared-named", + "https://forge.example/acme/widget.git", + ); + let named = Collected { + target: Some(String::from("acme/widget")), + ..Collected::default() + }; + assert!(matches!( + no_forge_cli() + .scope_holds( + &Scope::PublicTarget, + &dir, + &declaring("private"), + &named, + &argv("pr create") + ) + .unwrap(), + Standing::CouldNotTell(why) if why.contains("did not say whether acme/widget is public") + )); + } + + #[test] + fn an_internal_declaration_is_not_a_public_one() { + // `internal` is public to everyone with an account on the instance, + // which is neither public to the internet nor private -- and only + // `public` puts the `public-target` checks in scope here. + let dir = repository_pointing_at( + "shim-declared-internal", + "git@forge.example:acme/widget.git", + ); + assert_eq!( + no_forge_cli() + .scope_holds( + &Scope::PublicTarget, + &dir, + &declaring("internal"), + &Collected::default(), + &argv("pr create") + ) + .unwrap(), + Standing::DoesNotHold + ); + } + + #[test] + fn a_declaration_needs_an_origin_to_be_about_anything() { + // No `origin` is nothing to compare the target against, so there is no + // establishing that the declaration speaks for this destination and the + // forge is where the question goes -- unchanged from before. + let dir = crate::fixture::scratch("shim-declared-no-origin"); + std::fs::create_dir_all(&dir).unwrap(); + let elsewhere = Collected { + target: Some(String::from("acme/widget")), + ..Collected::default() + }; + assert!(matches!( + no_forge_cli() + .scope_holds( + &Scope::PublicTarget, + &dir, + &declaring("private"), + &elsewhere, + &argv("pr create") + ) .unwrap(), Standing::CouldNotTell(why) if why.contains("did not say whether acme/widget is public") )); @@ -3669,6 +4240,7 @@ mod tests { .scope_holds( &Scope::PublicRegistry, &dir, + &undeclared(), &Collected::default(), &argv("publish") ) @@ -3683,7 +4255,13 @@ mod tests { }; assert_eq!( npm() - .scope_holds(&Scope::PublicRegistry, &dir, &internal, &argv("publish")) + .scope_holds( + &Scope::PublicRegistry, + &dir, + &undeclared(), + &internal, + &argv("publish") + ) .unwrap(), Standing::DoesNotHold ); @@ -3708,6 +4286,7 @@ mod tests { &npm(), &scope, &dir, + &undeclared(), &Collected::default(), &argv("publish") ) @@ -3719,6 +4298,7 @@ mod tests { &npm(), &Scope::PublicRegistry, &dir, + &undeclared(), &Collected::default(), &argv("publish") ) diff --git a/tests/guard_cli.rs b/tests/guard_cli.rs index 6a5a852..1874ba9 100644 --- a/tests/guard_cli.rs +++ b/tests/guard_cli.rs @@ -1302,6 +1302,93 @@ fn an_origin_with_no_owner_and_repository_in_it_is_not_a_checked_claim() { assert!(text.contains("Could not look is not a pass"), "{text}"); } +// The two cases that DO reach a forge, through the fixture's own `gh`. Both are +// exit 2 and both leave the declaration unchecked -- the verdict was never the +// question. What is asserted here is that they do not say the same thing: for a +// while they did, and an account four minutes into a rate limit was handed a +// paragraph about what a 404 means. + +/// A `gh` that is over its budget, and a `rate_limit` endpoint that says when. +/// +/// The reset is built at call time because the message counts forward from now. +/// GitHub exempts `rate_limit` from the limit it reports, which is what makes +/// asking it in reply to a 403 something other than digging. +fn gh_is_rate_limited(root: &Path, resets_in_seconds: u64) { + let reset = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() + + resets_in_seconds; + gh_says( + root, + &format!( + "case \"$*\" in\n\ + 'api rate_limit --jq .rate.reset') echo {reset} ;;\n\ + *) echo 'gh: API rate limit exceeded for user ID 1. (HTTP 403)' >&2; exit 1 ;;\n\ + esac\n" + ), + ); +} + +#[test] +fn a_rate_limited_forge_is_not_reported_as_a_repository_that_is_not_there() { + let root = repository( + "visibility = \"private\"\n\n[rule.no-stale-visibility]\n\ + builtin = \"no-stale-visibility\"\n\n\ + [rule.no-stale-visibility.git]\nhooks = [\"pre-push\"]\n", + ); + git( + &root, + &[ + "remote", + "add", + "origin", + "https://github.com/acme/widget.git", + ], + ); + gh_is_rate_limited(&root, 15 * 60); + + let output = push_guard(&root, &[]); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + let text = stderr(&output); + assert!(text.contains("rate-limiting this client"), "{text}"); + // Counted forward and rounded up, so the reader is told how long to wait + // rather than left to compare two epochs. + assert!(text.contains("resets in about 15 minutes"), "{text}"); + // The wording that used to be printed here, and the reason this test exists. + assert!(!text.contains("A 404 is a private repository"), "{text}"); + assert!(text.contains("UPHOLD_ALLOW=no-stale-visibility"), "{text}"); +} + +#[test] +fn a_forge_that_will_show_no_such_repository_still_gets_the_404_reading() { + // The other half: splitting the arms must not cost the case the original + // wording was right for. A 404 over a repository declared private is the + // ordinary answer to an unauthenticated request, and saying so is the whole + // value of the paragraph. + let root = repository( + "visibility = \"private\"\n\n[rule.no-stale-visibility]\n\ + builtin = \"no-stale-visibility\"\n\n\ + [rule.no-stale-visibility.git]\nhooks = [\"pre-push\"]\n", + ); + git( + &root, + &[ + "remote", + "add", + "origin", + "https://github.com/acme/widget.git", + ], + ); + gh_says(&root, "echo 'gh: Not Found (HTTP 404)' >&2\nexit 1\n"); + + let output = push_guard(&root, &[]); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + let text = stderr(&output); + assert!(text.contains("A 404 is a private repository"), "{text}"); + assert!(!text.contains("rate-limiting"), "{text}"); +} + // ── where a push is allowed to go ──────────────────────────────────── /// A pre-push guard told nothing but its flags. diff --git a/tests/shim_cli.rs b/tests/shim_cli.rs index a19f8f0..b0785f5 100644 --- a/tests/shim_cli.rs +++ b/tests/shim_cli.rs @@ -2623,6 +2623,263 @@ fn a_gitlab_project_id_is_the_same_destination_with_its_separator_escaped() { ); } +// ── a visibility this repository already declared ──────────────────── + +/// A `public-target` shim over a repository that says what it is. +/// +/// The `visibility` line is the whole point of the fixture: it is the same +/// declaration `guard::names::target_is_public` has always read, written where +/// a policy writes it, and the question it answers is exactly the one the shim +/// was sending to a forge. +const DECLARED_POLICY: &str = r#" +visibility = "private" + +[rule.no-published-markers] +message = "remove the marker" +exec = "uphold guard --text -" + +[rule.no-published-markers.command] +before = ["faux"] + +[rule.prevent-ai-author] +builtin = "prevent-ai-author" + +[rule.prevent-ai-author.git] +hooks = ["commit-msg"] + +[[shim]] +command = "faux" +match = ["pr:create"] +text_flags = ["-t", "--title"] +target_flags = ["-R", "--repo"] +target = "git-remote" +scope = "public-target" +"#; + +/// The workspace above with an `origin`, and a `gh` that records being run. +/// +/// The stub fails rather than answering, and writes its marker before it does: +/// "asked, and could not be told" and "never asked" are then two different +/// states on disk rather than one exit code, which is what makes a forge call +/// that should not have happened visible at all. It is also what a rate-limited +/// account looks like, which is the failure this behaviour was reported from. +fn declared_workspace(policy: &str, origin: &str) -> PathBuf { + let root = workspace(policy); + stub( + &root, + "gh", + "#!/bin/sh\necho asked >> \"$(dirname \"$0\")/../gh.asked\"\nexit 1\n", + ); + Command::new(support::real_git()) + .args(["remote", "add", "origin", origin]) + .current_dir(&root) + .stdout(Stdio::null()) + .status() + .unwrap(); + root +} + +fn forge_was_asked(root: &Path) -> bool { + root.join("gh.asked").exists() +} + +#[test] +fn a_declared_private_visibility_answers_the_scope_with_no_forge_call() { + // The report: a repository whose policy says `private` still ran `gh api + // repos//` on every `git push`, and when the account was + // rate-limited the lookup failed, the standing was could-not-tell, and the + // default `unresolved` refused every push in every tree -- over a fact the + // policy had stated offline the whole time. + let root = declared_workspace(DECLARED_POLICY, "https://github.com/acme/widget.git"); + let output = shim( + &root, + &["faux", "pr", "create", "-t", "Generated with Claude Code"], + ); + // Out of scope, so the checks do not apply and the command runs -- which is + // the policy's own answer about its own destination, not a check skipped. + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!(stdout(&output).contains("faux ran:"), "{}", stdout(&output)); + assert!( + !forge_was_asked(&root), + "the forge was asked about a repository whose policy had already answered" + ); +} + +#[test] +fn a_declared_public_visibility_puts_the_checks_in_scope_with_no_forge_call() { + // The half that must not be lost. Reading the declaration is only safe if + // `public` reaches the checkers exactly as a forge saying `public` did: + // standing checks down on a word nobody read would be the failure this + // change could cause. + let root = declared_workspace( + DECLARED_POLICY + .replace("visibility = \"private\"", "visibility = \"public\"") + .as_str(), + "https://github.com/acme/widget.git", + ); + let output = shim( + &root, + &["faux", "pr", "create", "-t", "Generated with Claude Code"], + ); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + !stdout(&output).contains("faux ran:"), + "{}", + stdout(&output) + ); + assert!( + !forge_was_asked(&root), + "the forge was asked about a repository whose policy had already answered" + ); +} + +#[test] +fn a_declaration_about_this_repository_is_not_read_at_somebody_elses() { + // Where the shim is stricter than the guard, and it has to be: `-R + // other-owner/their-repo` is an explicit foreign destination, and what this + // policy says about this tree says nothing whatever about that one. So the + // forge is asked, it cannot answer, and a question that could not be asked + // has never been a pass here. + let root = declared_workspace(DECLARED_POLICY, "https://github.com/acme/widget.git"); + let output = shim( + &root, + &[ + "faux", + "pr", + "create", + "-R", + "other-owner/their-repo", + "-t", + "An ordinary title", + ], + ); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!( + !stdout(&output).contains("faux ran:"), + "{}", + stdout(&output) + ); + assert!( + stderr(&output).contains("other-owner/their-repo"), + "{}", + stderr(&output) + ); + assert!( + forge_was_asked(&root), + "a foreign destination was answered out of this repository's own declaration" + ); +} + +#[test] +fn a_repository_that_declares_nothing_still_asks_the_forge() { + // The unchanged path, asserted rather than assumed: a policy with no + // `visibility` line has said nothing, and silence is not an answer this + // seam may supply on the repository's behalf. + let root = declared_workspace( + DECLARED_POLICY + .replace("visibility = \"private\"\n", "") + .as_str(), + "https://github.com/acme/widget.git", + ); + let output = shim(&root, &["faux", "pr", "create", "-t", "An ordinary title"]); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!( + !stdout(&output).contains("faux ran:"), + "{}", + stdout(&output) + ); + assert!( + forge_was_asked(&root), + "nothing was declared and nothing was asked either" + ); +} + +#[test] +fn a_rate_limited_forge_says_so_where_the_scope_could_not_be_established() { + // The shim seam drew no line at all: every non-zero exit from `gh` was one + // indistinguishable "the forge did not say", so the sentence that reached + // the user over a rate limit was the same sentence a deleted repository + // would have produced -- and neither named the thing to do about it. + let root = workspace( + DECLARED_POLICY + .replace("visibility = \"private\"\n", "") + .as_str(), + ); + let reset = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() + + 8 * 60; + stub( + &root, + "gh", + &format!( + "#!/bin/sh\ncase \"$*\" in\n\ + 'api rate_limit --jq .rate.reset') echo {reset} ;;\n\ + *) echo 'gh: API rate limit exceeded for user ID 1. (HTTP 403)' >&2; exit 1 ;;\n\ + esac\n" + ), + ); + Command::new(support::real_git()) + .args([ + "remote", + "add", + "origin", + "https://github.com/acme/widget.git", + ]) + .current_dir(&root) + .stdout(Stdio::null()) + .status() + .unwrap(); + + let output = shim(&root, &["faux", "pr", "create", "-t", "An ordinary title"]); + // Still exit 2. The verdict was never the question -- a scope that could not + // be evaluated is not a scope that said no, and that is unchanged. + assert_eq!(code(&output), 2, "{}", stderr(&output)); + let text = stderr(&output); + assert!( + text.contains("did not say whether acme/widget is public"), + "{text}" + ); + assert!(text.contains("rate-limiting this client"), "{text}"); + assert!(text.contains("resets in about 8 minutes"), "{text}"); +} + +#[test] +fn a_typed_destination_naming_this_repository_is_still_asked_about() { + // The narrowing at the case that looks safest, and the reason the shim is + // stricter than the guard even here. `-R acme/widget` inside `acme/widget` + // names what appears to be this repository -- but a typed `owner/repo` + // carries no host, so under a `--hostname` or a `GH_HOST` those same two + // names are a different forge's repository with its own visibility, and two + // matching path segments cannot tell the two apart. So the declaration + // answers only for the destination `origin` supplied, and everything + // somebody typed goes to the forge exactly as it did before. + let root = declared_workspace(DECLARED_POLICY, "https://github.com/acme/widget.git"); + let output = shim( + &root, + &[ + "faux", + "pr", + "create", + "-R", + "acme/widget", + "-t", + "Generated with Claude Code", + ], + ); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!( + !stdout(&output).contains("faux ran:"), + "{}", + stdout(&output) + ); + assert!( + forge_was_asked(&root), + "a typed destination was answered out of this repository's own declaration" + ); +} + // A shim that can be handed its own probe. // // On 2026-09-02 the released binary fork-bombed a workstation from inside this From 5104bc21ac82a8d4891ff9d3964dc638cbfbea4c Mon Sep 17 00:00:00 2001 From: HackingGate Date: Wed, 9 Sep 2026 22:34:23 +0900 Subject: [PATCH 2/3] Match the reference page's own em dashes in the prose this added Claude-Session: https://claude.ai/code/session_01HertdiAvdNKGyjR91jvRUM --- docs/REFERENCE.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 7b4d8dc..f0eda94 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -468,18 +468,18 @@ see [the shims](#uphold-shim--the-shims). The rows above are what the forge said, and the shim reaches them only where it had to ask at all. A repository that declares its own `visibility` in its policy file has answered the question already, so `public-target` reads the declaration -and asks nobody -- `public` puts the checks in scope, `private` and `internal` +and asks nobody — `public` puts the checks in scope, `private` and `internal` stand them down, and no `gh` runs. That declaration speaks for this repository and for no other, so it is read for one destination only: the one the shim took from `origin` because nothing on the command line named another. A `gh -R owner/repo` goes to the forge exactly as it always did, and it does so **even -where the name it carries is this repository's own** -- a typed `owner/repo` +where the name it carries is this repository's own** — a typed `owner/repo` has no host in it, so under a `--hostname` or a `GH_HOST` the same two names are a different forge's repository with its own visibility, and two matching path segments cannot tell the two apart. So the `2` above is still the answer for a named destination the forge could not be asked about, and for a repository that declares nothing; what is bought offline is the case the cost was actually -in -- the push that names nothing and means `origin`, which no longer needs an +in — the push that names nothing and means `origin`, which no longer needs an authenticated `gh` to be told what the policy already said. So **`gh` must be authenticated wherever these rules run**, CI included. In a @@ -1528,7 +1528,7 @@ scope, `private` and `internal` are out of it, and nothing is spawned. The narrowing that makes reading it safe is that a declaration is about ONE repository, and the only destination this seam can be sure of is the one it derived itself. So the declaration is read where a `target_flags` value is -absent -- where the destination came from `origin` -- and nowhere else. A named +absent — where the destination came from `origin` — and nowhere else. A named destination goes to the forge whatever it names: `gh -R other/repo` obviously, and `gh -R this-owner/this-repo` too, because the name carries no host and `--hostname` and `GH_HOST` both make `owner/repo` a different repository on a @@ -1733,8 +1733,8 @@ than the seam. The fourth is uphold getting out of its own way, and it is documented here rather than left implicit because a reader who meets the line deserves to know what set it. A `public-target` scope asks whether the destination is public, and -where this repository has not declared its own `visibility` -- or where the -command named a destination, which a declaration does not speak for -- it asks +where this repository has not declared its own `visibility` — or where the +command named a destination, which a declaration does not speak for — it asks by running `gh api repos// --jq .visibility`; a `git-remote` target asks by running `git remote get-url origin`, which is how an unnamed destination is resolved in the first place and so runs either way. PATH answers From dabc95871bda8b24bd4d6910aabfc2af61332d3a Mon Sep 17 00:00:00 2001 From: HackingGate Date: Wed, 9 Sep 2026 22:48:22 +0900 Subject: [PATCH 3/3] Say what the comments hold, and not what they replaced uphold's own content policy on the branch that changed it: `no-before-after-narrative-in-source` on the CouldNotTell arm, and `no-user-data-measurement` on a rounding comment that quoted a figure. Claude-Session: https://claude.ai/code/session_01HertdiAvdNKGyjR91jvRUM --- src/shim.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/shim.rs b/src/shim.rs index c05c872..e98dbcc 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -1543,11 +1543,10 @@ impl Shim { match self.visibility(root, &target).as_deref() { Ok("public") => Ok(Standing::Holds), Ok(_) => Ok(Standing::DoesNotHold), - // The cause, in the words the forge used. It used to be - // dropped here: a rate limit, an unauthenticated `gh` and a - // deleted repository were one sentence, and the one thing - // the reader needed in order to act was the one thing it - // did not say. + // The cause, in the words the forge used. A rate limit, an + // unauthenticated `gh` and a deleted repository are three + // different instructions to the reader, and a refusal + // naming none of them is one the reader can only bypass. Err(silence) => Ok(Standing::CouldNotTell(format!( "the forge did not say whether {target} is public, so whether the \ `public-target` checks apply here could not be established. {}", @@ -1893,9 +1892,9 @@ fn rate_limit_minutes(program: &str) -> Option { .duration_since(std::time::UNIX_EPOCH) .ok()? .as_secs(); - // Rounded UP, so "0 minutes" is never printed for a wait somebody still has - // to sit through. A reset already in the past is a clock that disagrees with - // the forge's, and a wait nobody should be told to take. + // Rounded UP, so a wait somebody still has to sit through is never reported + // as no wait at all. A reset already in the past is a clock that disagrees + // with the forge's, and a wait nobody should be told to take. Some(reset.saturating_sub(now).div_ceil(60)).filter(|minutes| *minutes > 0) }