From 55ca8a01e5435aa15c461a8a5cc846053753d6ee Mon Sep 17 00:00:00 2001 From: Tong Date: Wed, 12 Aug 2026 23:47:05 +0900 Subject: [PATCH 1/3] Stop auditing pull refs the forge no longer serves `refs/audit/pull/*` is a destination this subcommand writes and nothing else touches, and the fetch that fills it did not prune. So a ref left by an earlier run outlived the pull request it named -- and outlived the remote it came from, once origin was repointed at a different repository. Every commit and every blob under one was then walked as something this forge serves, and reported as would-be-republished. The branch half was already pruned, for the reason stated two functions up: a report that cries wolf about the unpublishable is one nobody finishes reading. This clone was carrying forty such refs, and the run over them buried the one finding that is real under sixty that are not -- 769 surfaces read, against 284 once the refspec prunes its own destination. --- src/audit.rs | 12 +++++++ tests/audit_publication_cli.rs | 59 ++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/src/audit.rs b/src/audit.rs index b0fe11d..9b90327 100644 --- a/src/audit.rs +++ b/src/audit.rs @@ -168,6 +168,17 @@ fn history(root: &Path) -> Result> { /// untouched and a clone does not carry them by default -- so an audit that /// only read local refs would report clean over exactly the surface that /// survives the fix. +/// +/// Pruned, for the reason `refresh_origin` states about branches and this half +/// did not honour: `refs/audit/pull/*` is a local destination that nothing else +/// writes and nothing else deletes, so a ref left by an earlier run outlives the +/// pull request it named -- and outlives the remote it came from, if `origin` +/// was ever repointed. Every commit under it was then read as one this forge +/// serves and reported as `would be republished`, which is the "cries wolf about +/// the unpublishable" failure named a few lines up: findings whose only fix is +/// to delete something the reader would discover was never published there. +/// `--prune` with an explicit refspec prunes that refspec's destination, so a +/// stale ref goes on the next run rather than being audited forever. fn retained_pull_refs(root: &Path) -> Result<(Vec, Vec)> { let mut surfaces = Vec::new(); let mut unreadable = Vec::new(); @@ -175,6 +186,7 @@ fn retained_pull_refs(root: &Path) -> Result<(Vec, Vec)> { .args([ "fetch", "-q", + "--prune", "origin", "+refs/pull/*/head:refs/audit/pull/*", ]) diff --git a/tests/audit_publication_cli.rs b/tests/audit_publication_cli.rs index 1751982..35ef8c8 100644 --- a/tests/audit_publication_cli.rs +++ b/tests/audit_publication_cli.rs @@ -241,3 +241,62 @@ hooks = ["pre-commit"] "a literal owner outside the first variant was never objected to:\n{report}" ); } + +/// A `refs/audit/pull/*` ref the forge no longer serves is not audited. +/// +/// That destination is written by this subcommand and by nothing else, so a ref +/// left by an earlier run stays until something prunes it -- including after +/// `origin` is repointed at a different repository, which is when every ref +/// under it names a pull request the current forge never had. Read unpruned, +/// those commits are reported as `would be republished`, and the reader's only +/// fix is to delete something that was never published where the report says it +/// was. It is the same defect the branch half was fixed for, on the ref set that +/// half does not cover. +#[test] +fn a_pull_ref_the_forge_no_longer_serves_is_not_audited() { + let root = repository(); + // A real remote, because the fetch is what prunes: with no origin at all the + // subcommand reports the surface unreadable and never walks a ref. + let origin = + std::env::temp_dir().join(format!("uphold-publication-origin-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&origin); + git(&root, &["init", "-q", "--bare", origin.to_str().unwrap()]); + git( + &root, + &["remote", "add", "origin", origin.to_str().unwrap()], + ); + + std::fs::write(root.join("a.txt"), "nothing to see\n").unwrap(); + git(&root, &["add", "-A"]); + git(&root, &["commit", "-qm", "one", "--no-verify"]); + git(&root, &["push", "-q", "origin", "main"]); + + // A commit the remote does not have, parked under the audit's own + // destination the way a previous run against another forge would leave it. + std::fs::write(root.join("STALE.md"), "PrivateOrg\n").unwrap(); + git(&root, &["add", "-A"]); + git( + &root, + &[ + "commit", + "-qm", + "a pull request on some other forge", + "--no-verify", + ], + ); + let stale = Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(&root) + .output() + .unwrap(); + let stale = String::from_utf8_lossy(&stale.stdout).trim().to_owned(); + git(&root, &["update-ref", "refs/audit/pull/9", &stale]); + git(&root, &["reset", "-q", "--hard", "HEAD~1"]); + + let output = audit(&root); + let report = text(&output); + assert!( + !report.contains("STALE.md") && !report.contains("some other forge"), + "a pruned pull ref was still read as a surface this forge serves:\n{report}" + ); +} From de22f40b255165385c0b76217861f29a0fda59ef Mon Sep 17 00:00:00 2001 From: Tong Date: Wed, 12 Aug 2026 23:47:25 +0900 Subject: [PATCH 2/3] Let a command nothing declares here simply run The link is on PATH for the whole machine; a `[[shim]]` is a line in one repository's policy. So the undeclared case is the ordinary one, not the exception -- every directory outside a participating repository reaches it, and so does every participating repository that declares a shim for some other command. Sixty-six of the seventy-one policies in the fleet declare none at all. While that answer was an error, installing the link the way the documentation describes made `git` exit 2 nearly everywhere it was typed, including outside a repository entirely. What gets installed after that is nothing, which loses the seam in the repositories that did declare one -- and that is what had happened: the links still pointed at the predecessor engine, because pointing them here broke the command. An absent declaration is a place the rule does not run, the same way an absent `[git]` table is. It is a reading rather than a failure to read, so it is not exit 2 by the rule that governs those; a policy that exists and cannot be READ still is, because the declaration nobody could read might have been the one. Asked for by name rather than run as the command, an undeclared shim stays an error: nothing is standing in front of anything and the caller asked. --- docs/REFERENCE.md | 10 +++++ src/main.rs | 28 ++++++++++--- src/shim.rs | 84 +++++++++++++++++++++++++++++++++----- tests/shim_cli.rs | 100 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 208 insertions(+), 14 deletions(-) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 4ec6172..be87dcf 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -478,6 +478,16 @@ exists precisely to skip `commit-msg`. about to publish, and **execs through**. Put a link named for the command on PATH ahead of the real one and `argv[0]` does the rest. +That link is on PATH for the whole machine, while a `[[shim]]` is a line in one +repository's policy — so **where nothing declares the command, the command +simply runs**: no policy in this directory, or a policy that declares a shim for +some other command. Neither is a could-not-look, so neither is exit `2`; the +policy was read and it said this command is not one it stands in front of. A +policy that exists and cannot be *read* still exits `2`, because the +declaration that could not be read might have been the one. Asked for by name — +`uphold shim faux …` — an undeclared command is still an error, since nothing is +standing in front of anything and the caller asked. + ```toml [[shim]] command = "gh" diff --git a/src/main.rs b/src/main.rs index 2ce8b3b..9cca270 100644 --- a/src/main.rs +++ b/src/main.rs @@ -201,7 +201,7 @@ fn run() -> Result { .file_name() .filter(|name| !name.is_empty() && name.to_str() != Some("uphold")) { - return shim_command(text_of(name)?, &arguments); + return shim_command(text_of(name)?, &arguments, shim::Invoked::AsTheCommand); } let Some((first, rest)) = arguments.split_first() else { @@ -250,7 +250,7 @@ fn run() -> Result { let (name, shimmed) = rest .split_first() .ok_or_else(|| Fatal::new(format!("shim needs a command\n\n{USAGE}")))?; - shim_command(text_of(name)?, shimmed) + shim_command(text_of(name)?, shimmed, shim::Invoked::ByName) } other => Err(Fatal::new(format!( "unknown subcommand {other:?}\n\n{USAGE}" @@ -626,9 +626,27 @@ fn effective_rules_command(as_json: bool) -> Result { Ok(Exit::Clean) } -fn shim_command(name: &str, argv: &[OsString]) -> Result { +fn shim_command(name: &str, argv: &[OsString], invoked: shim::Invoked) -> Result { let working = std::env::current_dir()?; - let (root, policy_path) = discover(&working).ok_or_else(|| no_policy_here(&working))?; + // No policy where the command was typed means no repository here declares + // anything to stand in front of it. Run as the command, that is the command + // running: the link is on PATH for the whole machine -- `/tmp`, somebody + // else's checkout, a shell that never enters a participating repository -- + // and refusing there protects nothing, breaks `git` everywhere, and gets + // the link removed, which is how the seam is lost in the repositories that + // DID declare it. Asked for by name, it is still an error, because the + // caller asked this repository for a shim it does not have. + // + // A policy that exists and cannot be read is a different answer and still + // fatal both ways: `config::load` below says so, because a declaration that + // could not be read might have been the one standing in front of this + // command. + let Some((root, policy_path)) = discover(&working) else { + return match invoked { + shim::Invoked::AsTheCommand => shim::exec_through(name, argv), + shim::Invoked::ByName => Err(no_policy_here(&working)), + }; + }; let policy = config::load(&root, &policy_path)?; // The shimmed command's arguments stay bytes all the way to the exec. On // Unix an argument is an arbitrary byte string -- `git add` on a file named @@ -638,7 +656,7 @@ fn shim_command(name: &str, argv: &[OsString]) -> Result { // command that runs is the command that was typed. Where the shim has // something to CHECK, it refuses the untranslatable argument itself, in the // words of what it could not read. - shim::run(&root, &policy, name, argv) + shim::run(&root, &policy, name, argv, invoked) } fn main() { diff --git a/src/shim.rs b/src/shim.rs index 6fbcf3d..681d9d4 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -790,6 +790,45 @@ fn file_identity(path: &Path) -> Option { path.canonicalize().ok() } +/// How this process was reached, which is the only thing that differs when +/// nothing here declares the command. +/// +/// Both are the same seam and the same reading. One is a command being run -- +/// through a link named for it, on a PATH that spans the whole machine -- and +/// the other is a question asked about this repository, typed with the answer +/// in mind. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Invoked { + /// Through a link named for the command: argv[0] decided. + AsTheCommand, + /// As `uphold shim `, with the command named as an argument. + ByName, +} + +/// Run the command with nothing standing in front of it. +/// +/// The transparent path, for the two answers that are not "check this": no +/// policy where the command was typed, and a policy that declares no shim for +/// it. Both are readings rather than failures to read, and a shim installed for +/// the whole machine meets them constantly -- every directory outside a +/// participating repository is one of them. +/// +/// No stdin is replayed because none was collected: reading it belongs to the +/// checking path, and a command whose text nothing here reads must be handed +/// the descriptor it was given rather than a copy of what this process drained +/// out of it. +pub(crate) fn exec_through(name: &str, argv: &[OsString]) -> Result { + let own = std::env::current_exe().ok(); + let Some(real) = real_command(name, own.as_deref()) else { + return Err(Fatal::new(format!( + "nothing here stands in front of {name}, and there is no {name} on PATH to run" + ))); + }; + let mut command = Command::new(&real); + command.args(argv); + hand_off(&mut command, name, None) +} + /// The real command, found by walking PATH past ourselves. /// /// "Past ourselves" is a question about the FILE, not about the directory. A @@ -1083,7 +1122,13 @@ fn hand_off(command: &mut Command, name: &str, stdin: Option<&[u8]>) -> Result Result { +pub(crate) fn run( + root: &Path, + policy: &Policy, + name: &str, + argv: &[OsString], + invoked: Invoked, +) -> Result { let words: Vec = argv .iter() .map(|argument| argument.to_string_lossy().into_owned()) @@ -1102,14 +1147,35 @@ pub(crate) fn run(root: &Path, policy: &Policy, name: &str, argv: &[OsString]) - .map(|shim| (shim.command.as_str(), shim)) .collect(); let Some(shim) = shims.get(name) else { - return Err(Fatal::new(format!( - "no shim declares the command {name:?}; this policy declares {}", - if shims.is_empty() { - String::from("none") - } else { - shims.keys().copied().collect::>().join(", ") - } - ))); + // Nothing here declares this command. The reading is the same either + // way -- an absent declaration is a place the rule does not run, the + // same way an absent `[git]` table is, and it is not a could-not-look, + // so it is not exit 2 by the rule that governs those. What differs is + // what was asked. + // + // Run AS the command, the answer lets it run. The link is on PATH for + // the whole machine while a `[[shim]]` is a line in one repository's + // policy, so refusing an undeclared command meant `git` exiting 2 in + // every repository that had not declared one, and in every directory + // that is not a repository at all. What gets installed after that is + // nothing, which loses the seam everywhere rather than where it was + // undeclared. + // + // Asked for BY NAME, the answer is an error: `uphold shim faux ...` + // names a shim this repository does not have, nothing is standing in + // front of anything, and the caller is entitled to hear that rather + // than watch a typo run. + return match invoked { + Invoked::AsTheCommand => exec_through(name, argv), + Invoked::ByName => Err(Fatal::new(format!( + "no shim declares the command {name:?}; this policy declares {}", + if shims.is_empty() { + String::from("none") + } else { + shims.keys().copied().collect::>().join(", ") + } + ))), + }; }; // Only the rules that name THIS command line. A checker used to be diff --git a/tests/shim_cli.rs b/tests/shim_cli.rs index 285155a..cb0ad20 100644 --- a/tests/shim_cli.rs +++ b/tests/shim_cli.rs @@ -255,6 +255,12 @@ fn the_bypass_names_the_checker_it_switched_off() { assert!(stdout(&output).contains("faux ran:")); } +/// Asked BY NAME for a shim this repository does not have, and told so. +/// +/// The other half of this pair lets the same reading through, and the two are +/// not in tension: what differs is what was asked. `uphold shim unknown ...` is +/// a question typed with an answer in mind, and nothing is standing in front of +/// anything, so the caller hears it rather than watching a typo run. #[test] fn a_command_no_shim_declares_is_refused_rather_than_silently_passed_through() { let root = workspace(POLICY); @@ -267,6 +273,100 @@ fn a_command_no_shim_declares_is_refused_rather_than_silently_passed_through() { ); } +/// A link is on PATH for the whole machine; a `[[shim]]` is a line in one +/// repository's policy. +/// +/// So the undeclared case is the ordinary one, not the exception: every +/// directory outside a participating repository reaches it, and so does every +/// participating repository that declares a shim for some OTHER command. While +/// that answer was an error, installing the link as the documentation describes +/// made the command exit 2 nearly everywhere it was typed -- and what gets +/// installed after that is nothing, which loses the seam in the repositories +/// that did declare it. +#[test] +fn a_command_this_policy_does_not_declare_still_runs_when_the_link_is_the_command() { + let root = workspace(POLICY); + // The real command, behind a link named for it. Two directories, because one + // cannot hold two files of the same name -- and the link has to come first. + std::fs::create_dir_all(root.join("front")).unwrap(); + let stub = root.join("bin/undeclared"); + std::fs::write(&stub, "#!/bin/sh\necho \"undeclared ran: $*\"\n").unwrap(); + let mut permissions = std::fs::metadata(&stub).unwrap().permissions(); + std::os::unix::fs::PermissionsExt::set_mode(&mut permissions, 0o755); + std::fs::set_permissions(&stub, permissions).unwrap(); + let link = root.join("front/undeclared"); + std::os::unix::fs::symlink(env!("CARGO_BIN_EXE_uphold"), &link).unwrap(); + + let path = format!( + "{}:{}:{}", + root.join("front").display(), + root.join("bin").display(), + std::env::var("PATH").unwrap_or_default() + ); + let output = Command::new(&link) + .args(["publish", "--now"]) + .current_dir(&root) + .env("PATH", path) + .env_remove("UPHOLD_ALLOW") + .output() + .unwrap(); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!( + stdout(&output).contains("undeclared ran: publish --now"), + "{}{}", + stdout(&output), + stderr(&output) + ); +} + +/// No policy where the command was typed is not a repository refusing; it is a +/// directory that declares nothing. +/// +/// `/tmp`, somebody else's checkout, a shell that never enters a participating +/// repository -- a machine-wide link meets these far more often than it meets a +/// declaration. Refusing here protects nothing and breaks the command +/// everywhere. +#[test] +fn a_directory_with_no_policy_at_all_does_not_break_the_command() { + let root = workspace(POLICY); + // Outside `root`, deliberately: a subdirectory of it would find the policy + // by walking up, which is the case this test is not about. + let elsewhere = std::env::temp_dir().join(format!( + "uphold-shim-no-policy-{}-{}", + std::process::id(), + root.file_name().unwrap().to_string_lossy() + )); + let _ = std::fs::remove_dir_all(&elsewhere); + std::fs::create_dir_all(&elsewhere).unwrap(); + + // Named for the stub, so PATH resolution finds the link first and the real + // command second, exactly as an install puts them. + std::fs::create_dir_all(root.join("front")).unwrap(); + let front = root.join("front/faux"); + std::os::unix::fs::symlink(env!("CARGO_BIN_EXE_uphold"), &front).unwrap(); + + let path = format!( + "{}:{}:{}", + root.join("front").display(), + root.join("bin").display(), + std::env::var("PATH").unwrap_or_default() + ); + let output = Command::new(&front) + .args(["pr", "create", "-t", "anything"]) + .current_dir(&elsewhere) + .env("PATH", path) + .env_remove("UPHOLD_ALLOW") + .output() + .unwrap(); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!( + stdout(&output).contains("faux ran:"), + "{}{}", + stdout(&output), + stderr(&output) + ); +} + #[test] fn a_checker_that_could_not_look_is_not_a_pass() { // Exit 2 is the third answer, and folding it into either of the others is From c38b6eea16fc82241cd7a84a3108abd6075b9b5b Mon Sep 17 00:00:00 2001 From: Tong Date: Wed, 12 Aug 2026 23:47:54 +0900 Subject: [PATCH 3/3] Say that this repository is public, where the guard reads it `visibility` is the whole scope test for the three name guards, and it still said `private` a day after the flip. A rule that believes its repository is private refuses nothing, silently, and reports a clean tree while doing it -- so a declaration left behind by a visibility change is not a stale comment, it is the guard switched off. That is how the one finding the publication audit now reports was written: a commit message naming a private organisation, on a branch pushed after the flip, past a commit-msg guard that had been told it had nothing to protect. Each variant carries its own scope, so all three say it. The comment says which line to edit when a repository is flipped, and records what the flip already published: the squashed commit on the default branch is clean, the retained head ref of the pull request that merged it is not, and no rewrite reaches that one. --- policy/principles.toml | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/policy/principles.toml b/policy/principles.toml index 3a25e1d..e2eadd5 100644 --- a/policy/principles.toml +++ b/policy/principles.toml @@ -129,20 +129,28 @@ git.hooks = ["pre-push"] [rule.no-private-repo-names] builtin = "no-private-repo-names" # `visibility` is declared rather than looked up, and it is a settled fact -# rather than a placeholder: publication was considered and declined, so this -# repository stays private and the guard stays out of scope. +# rather than a placeholder: this repository was published, so the guard is in +# scope on every commit written here. +# +# It said `private` for a day after the flip, and that is how the one finding +# below was written. The declaration is the whole scope test -- a rule that +# believes its repository is private refuses nothing, silently, and reports a +# clean tree while doing it -- so a declaration left behind by a visibility +# change is not a stale comment, it is the guard switched off. Flipping a +# repository means editing this line in the same change. # # Declared rather than looked up because a lookup makes every commit wait on a -# forge, and because the answer is a decision rather than a reading. The line to -# change if that decision is revisited is this one -- and the check to run -# before changing it is `uphold audit --for-publication`, which asks what a -# flip would republish under the visibility it is flipping TO. Every guard here -# asks "is the target public NOW", so nothing else re-examines a name that was -# correctly allowed at write time. +# forge, and because the answer is a decision rather than a reading. The check +# to run before changing it is `uphold audit --for-publication`, which asks what +# a flip would republish under the visibility it is flipping TO. Every guard +# here asks "is the target public NOW", so nothing else re-examines a name that +# was correctly allowed at write time. # -# It has an answer today, and it is not clean: two retained pull-request head -# refs carry a private organisation's name, and no history rewrite reaches them. -visibility = "private" +# What the flip published, and what no rewrite reaches: commit 188970b2, on the +# retained head ref of the pull request that merged it, names a private +# organisation in its message. The squashed commit on the default branch does +# not. The remaining copy is the forge's to remove, not a rewrite's. +visibility = "public" # Declared, not looked up. A forge lookup answers for `owner/repo`; these are # organisations, and the form that got past a hand audit of this repository was # an organisation named on its own, in a sentence about what it carries. A @@ -160,12 +168,15 @@ git.hooks = ["commit-msg"] [rule.no-private-repo-names-staged] builtin = "no-private-repo-names-staged" -visibility = "private" +# The same settled fact as the variant above, and it has to be written again +# here: each variant carries its own scope, so one left at `private` is one seam +# standing down while its siblings refuse. +visibility = "public" git.hooks = ["pre-commit"] [rule.no-private-repo-names-in-files] builtin = "no-private-repo-names-in-files" -visibility = "private" +visibility = "public" # Not pre-commit: this asks the forge about every distinct name anywhere in the # tree, not the few a commit touches, and a check that adds twenty seconds to # every commit is one somebody deletes. That trade-off is a line in this file