Fleet audit: exit 0 on checks that could not look, two deadlocks, and a proposal for guard base sets

[HackingGate]

Findings from a fleet-wide audit of uphold and its 65 consuming repositories. Consumer repositories are private, so they are referred to here by count rather than by name.

Every item below was independently re-verified by a second reviewer prompted to refute it. Two candidate findings were dropped as unsupported and are not listed.

The recurring shape is a check that could not look reporting a pass. That is the failure explicit-unknown exists to name, and it is currently reachable in eight places.

A. Exit-code discipline: a check that could not look reports clean

• src/selection.rs:201 -- walk errors are silently dropped. walker.build().flatten() discards every Err (permission denied, IO error, loop detection, ignore-file parse failure). Those subtrees are neither searched nor reported, and the short file list is indistinguishable from a genuinely small tree. Iterate without .flatten() and either return Fatal or collect a could-not-read list that forces Exit::Broken, the way audit.rs already does.
• src/pins.rs:197 -- a hook pin that could not be checked is reported as a guard that passed. remote_refs returns Ok(None) with the comment "it is never a pass". The caller makes it a pass: unchecked pins are only eprintln'd and never reach report, so stale() returns Ok(None) at line 264.
• src/audit.rs:221 -- audit --for-publication can never exit 0. forge_conversations unconditionally pushes the "comment edit history could not be read" note into unreadable, so the Exit::Broken branch at line 365 always wins and lines 375-376 are dead code. docs/REFERENCE.md:457-459 documents a clean path that is unreachable. Split the always-true structural caveat from measured unreadability.
• src/selection.rs:198 -- scan silently skips tracked files that a .gitignore matches, including the user's global one. The walker sets .git_ignore(true).git_global(true).git_exclude(true).parents(true), but git ignore rules do not apply to already-tracked files. A tracked, committed, ignore-matched file is invisible to every content rule and is not counted in the skipped-paths report. Select from the index (git ls-files -z) instead.
• src/selection.rs:205 -- a files.include pointing outside the repository silently selects zero files and reports clean. Entries failing strip_prefix(&self.root) are dropped with no counter and no message.
• src/text.rs:48 -- scan --text decodes stdin lossily and reports a pass over bytes it could not read. from_utf8_lossy without a word, while scan.rs:596-607 refuses a non-UTF-8 file on the grounds that "clean would mean unexamined". Verified: printf 'caf\xe9 latin1 bytes\n' | uphold scan --text - prints "policy checks passed (text)".
• src/config.rs:863 -- command.before is accepted on non-exec rules and read by nothing. validate refuses files.* on a fileless check and git.hooks on a non-builtin, but has no mirror here, while shim::run filters candidates to Check::Exec. A builtin whose only declared place is command.before runs nowhere and reports clean.
• src/shim.rs:633 -- a [[shim]] with no checker rule execs the command with nothing checked and says nothing. Nothing at load cross-checks [[shim]].command against any command.before, the way an unknown builtin is refused at config.rs:877.

B. Hangs and panics

• src/selection.rs:57 -- uphold scan deadlocks on any repository with more than a few thousand tracked files. not_text_paths writes the whole git ls-files -z output into git check-attr --stdin -z text on the calling thread and only then calls wait_with_output(). Once check-attr fills its stdout pipe it stops reading stdin, our write_all fills the stdin pipe, and neither side drains.
• src/shim.rs:538 -- consult deadlocks on a large subject when the checker writes more than a pipe buffer. Blocking write_all before wait_with_output() with stdout and stderr both piped and undrained. Reproduced with a 200 KB -F body. Also stop discarding the write result with .ok().
• src/report.rs:49 -- report::dedent slices at a non-char-boundary and panics (exit 101) instead of reporting the violation. indent is a byte count minimised across non-blank lines, but trim_start strips Unicode whitespace, so the minimum can land inside a multi-byte character of another line. Whitespace-only lines are excluded from the minimum yet still sliced, which is a second way in.
• src/main.rs:102 -- non-UTF-8 argv panics (exit 101) instead of exiting 2. env::args() panics on any non-Unicode argument, and the shim is designed to sit in front of git, gh and npm, which are routinely handed such paths. Use args_os() and keep OsString for the passthrough.

C. Root and scope resolution

• src/main.rs:82 -- discover walks past the repository boundary. It pops directories until a policy/principles.toml appears, with no stop at a .git boundary. Run inside a repository that has no policy, uphold scan|guard|audit|shim climbs to the enclosing superproject, loads its policy, and adopts its directory as root -- so the report names files that are not in the repository the command was run in. Stop the walk at the first directory containing .git.
• src/main.rs:186 -- --policy infers the repository root as the policy file's grandparent, with nothing checking the layout. A policy at the repository root makes root the repository's parent; a policy one level below / makes root be /.

D. Guard correctness

• src/guard/names.rs:540 -- no-private-repo-names-staged runs git diff --cached --unified=0 without --no-ext-diff --no-textconv. git diff honours diff.external and per-path textconv from global and system config, so a common difftastic or delta setup emits EXTERNAL a.txt ... and no + lines at all, and the guard reports a pass. Add --no-ext-diff --no-textconv --no-color -U0 and -c core.quotepath=false.
• src/guard/names.rs:539 -- in_staged ignores the rule's [rule.files] scope and drops the file and line from its findings. It concatenates every added line into one blob labelled "staged changes" and never calls scope::in_file_scope, unlike in_tracked (names.rs:560) and unicode::in_files (unicode.rs:234).
• src/text.rs:63 -- scan --text loses the host-identity fallback as soon as the repository declares any forbidden-literals rule. The fallback exists, per its own docstring, so the guard is not absent "in exactly the places nobody thought to configure it". The test is for the check kind, not for that rule. Test for forbidden_literals = "running-os-identity" instead.

E. Shim: transparency of the passthrough

• src/shim.rs:213 -- shim consumes stdin for -F - and never replays it, so the real command publishes an empty body. The comment on lines 208-212 promises the replay and calls the alternative "worse than no guard": "A guard that silently eats the body it approved is worse than no guard." No replay exists; run() execs with a plain .status().
• src/shim.rs:684 -- shim forks instead of exec'ing, so signal death flattens to exit 1 and signals sent to the shim are not forwarded. A Ctrl-C'd git push looks like an ordinary failure. Use CommandExt::exec() with arg0, which is what the module docstring already describes.
• src/shim.rs:154 -- Shim::matches reads argv[0]/argv[1] positionally, so any global flag before the subcommand silently disables the shim. gh --repo owner/name issue create -t "..." yields verb --repo, noun owner/name; git -c user.name=x push origin topic yields verb -c. Neither matches, so the whole checking block is skipped and the command execs unexamined.

F. Audit surface

• src/audit.rs:232 -- audit scans only HEAD's tree. A private-to-public flip republishes every reachable object, so a name or credential committed and then deleted before HEAD is served forever by the forge's blob and commit-view endpoints, and survives the default-branch rewrite the audit is meant to trigger. Scan the reachable blob set via git rev-list --objects plus cat-file --batch-check.
• src/audit.rs:203 -- audit never reads issue or PR titles, nor PR review bodies, and silently truncates at 200 items. The title is the exact field the shim guards on gh issue create -t (shim.rs:695). Add title to the --json list, add a pass over --json reviews plus the review-comment API, and compare the returned count against the cap instead of truncating in silence.

G. Reconciler (uphold_check.py)

Worth stating up front, because it changes the shape of the fix: there is no uphold check subcommand. src/main.rs USAGE offers scan, guard, audit, rules and shim only, and grep -n "upheld.toml\|enforce" src/*.rs src/guard/*.rs returns nothing. The reconcile exists only in Python, and 65 repositories pin it. It cannot simply be deleted; its inventory half needs to move into the binary.

• uphold_check.py:284 -- pinning only uphold-check is accepted as proof that every uphold rule runs. published_hook_ids() includes the reconciler's own id, and runs_principles returns True as soon as any published id is pinned. A repository pinning nothing but the reconciler prints "reconciled 1 enforcement claim". Exclude the reconciler's own id from the evidence set.
• uphold_check.py:313 -- a claim on a guard reconciles green even when no hook installs the stage that guard fires at. _rule_ids() reads only the [rule.*] keys and discards git.hooks. A repository pinning only uphold-scan while declaring a pre-push guard reconciles clean. Carry git.hooks through and map each stage to the hook id that installs it.
• uphold_check.py:359 -- the reconciler re-implements config::load and drops inherit.paths, refusing rules the engine actually runs. Rust's Inherit has three fields (sets, paths, disabled_rules); the Python reads only sets. Fix by adding uphold rules --effective [--json] to the binary and deleting content_policy_rules().
• uphold_check.py:101 -- any 4-space-indented bare YAML key in lefthook.yml becomes a claimable "rule". LEFTHOOK_COMMAND matches any valueless mapping key at indent 4, including the configs: key that README.md:47-54 tells every lefthook consumer to write verbatim. A claim on a rule named configs reconciles green.
• scripts/check_hook_pins.py:256 -- hook-pins-resolve (Python) and no-stale-hook-pins (Rust) both run at pre-push and can return contradictory verdicts on the same pin. src/pins.rs:20-25 says the line-scanning version existed only because it shipped as language: script and that "a binary has no such constraint". It was never removed. Delete the script and its hook registration, and re-point the explicit-unknown claim in policy/upheld.toml:41-47.
• uphold_check.py:206 -- read_text catches only OSError, so a non-UTF-8 config exits 1 with a traceback instead of 2. UnicodeDecodeError derives from ValueError. Every caller is a could-not-look path, and the contract reads exit 1 as "a claim is false".
• uphold_check.py:911 -- unvalidated [review] fields crash with a traceback and exit 1. no_subject_here is carefully validated; max_lines, include_domains and emit are not. max_lines = "nine hundred" raises ValueError.
• uphold_check.py:953 -- --review --emit writes outside the repository and crashes on a missing parent directory. emit = ["../ESCAPED.md"] reports "wrote ../ESCAPED.md" and creates the file one level above the repository.
• uphold_check.py:671 -- --coverage counts orphan claims in the numerator it just reported as orphans. enforced intersects with claimable but never with supplied.
• scripts/check_hook_pins.py:201 -- a repos entry whose rev: precedes its repo: is silently dropped. Moot if the script is deleted per the item above.
• scripts/check_hook_pins.py:354 -- unreadable configs are never reported when any pin is also missing; the return EXIT_MISSING precedes the unreadable loop.
• tests/test_uphold_check.py -- 106 tests pass and none cover any divergence above. The suite exercises inherit.sets but never inherit.paths, and asserts a lefthook consumer reconciles using the very fixture whose configs: key is a phantom rule. Add a differential test that runs the binary and the Python over one fixture repository and asserts the rule-id sets are equal.

H. Distribution

• src/pins.rs:57 -- no-stale-hook-pins hard-fails (exit 2) on any lefthook-only consumer. read_pins() unconditionally opens .pre-commit-config.yaml and error::read_to_string turns ENOENT into a Fatal, so a consumer who followed the documented lefthook install path dies at every pre-push. Reproduced against the release binary.
• src/pins.rs:57 and .github/dependabot.yml:27 -- lefthook remotes[].ref pins are invisible to every pin checker and to Dependabot. The one version a lefthook consumer pins is read by nothing. Either teach the pin readers about lefthook.yml remotes:, or state the exclusion in README.md:43-58.
• hooks/lefthook.yml:42 -- lefthook consumers run the full reconciler on every commit. No glob and no files, while .pre-commit-hooks.yaml:17-21 states the opposite as design intent and enforces it with a files regex.
• .github/workflows/test.yml:256 -- the "no Rust toolchain needed" claim is asserted in README.md:20-21 and .pre-commit-hooks.yaml:10-13 and tested nowhere. The runner-parity job installs the toolchain unconditionally for all three matrix legs. Gate the step on the lefthook leg, or drop the claim.
• scripts/consumer_check.sh:137 -- three published ids are pinned by the parity harness and never executed. uphold-check never matches after the seed commit, uphold-guard-merge needs a real merge that no question makes, and uphold-guard-manual is never invoked.

I. Rules that did not fully survive the port

These are halves of already-ported rules, not new rules. The absorbed upstreams are archived, so this is the last chance to recover them from the source.

• no-private-repo-names-in-files -- the path itself is never scanned, only blob content. The upstream scanned five things out of the resolved scope: blob text, symlink target, the path for every entry kind, the blobs a pushed range introduced, and the messages. The same one-line change also makes -staged cover renames.
• no-private-repo-names at pre-push -- the commit messages a push publishes are read by nothing. The upstream had a --messages mode precisely because commit-msg only fires when git commit writes a message: not for commit-tree, rebase, cherry-pick, git am, --no-verify, or a fast import.
• no-private-repo-names-staged -- a committed .gitattributes holding * -diff or *.csv binary reduces plain-ASCII files to "Binary files differ", and the check reports a pass. The upstream had a second pass for exactly this; it did not survive.
• prevent-unusual-unicode-in-files -- the filename is not scanned ("the path itself is scanned before its content, because a filename is committed text"), and an undecodable blob is skipped in silence.
• no-stale-hook-pins -- root config only, where the upstream read every .pre-commit-config.yaml in the work tree ("a pin in sub/.pre-commit-config.yaml is a pin a run touches"); a missing repos: key reads as zero pins rather than as unreadable.
• shared scope resolver -- gitlink entries (mode 160000) reach git cat-file blob and abort the tree-wide guards with exit 2. index_blobs pushes every git ls-files -s -z record without looking at the mode. Carry the mode on Blob rather than inferring blob-ness later.
• shim -- the PR or issue body composed in an editor is unmediated. The upstream installed itself as the command's own editor variable (GH_EDITOR / GLAB_EDITOR), ran the real editor, then read the file back and consulted the same checkers. uphold only warns.
• shim -- the shipped glab shim is inert. policy/principles.toml:248-256 declares scope = "public-target", but forge_visibility() hard-codes gh api repos/... --jq .visibility, which cannot answer for a GitLab remote. Dispatch the visibility resolver on the shim's command or the remote host, and preserve the internal != public rule.
• unmanaged-pins -- the set does not match cargo install --git ... --tag vX.Y.Z or a lefthook remotes: ref:, which are the two pins uphold's own install instructions create.
• test tier -- the upstream's promotion-corpus parity test was not ported. It asserted every promoted base rule still matches lines derived mechanically from the local rules it replaced, on the grounds that "a rule that stops matching produces no output at all: the gate goes green and stays green".

J. Proposal: bundle the guards as base sets, and ship the trigger first

Measured across the 65 consuming repositories, five guard declarations are byte-identical after stripping comments, with zero variation:

rule	stages	identical in
prevent-ai-author	["commit-msg"]	65/65
prevent-unusual-unicode	["commit-msg"]	65/65
no-merge-commit	["pre-commit"]	65/65
no-local-merge	["pre-merge-commit"]	65/65
prevent-public-push	["pre-push"]	65/65
no-stale-hook-pins	["manual"]	43/45
prevent-unusual-unicode-in-files	4 stages	43/45

That is one decision and 64 transcriptions. policy/base/ is already the preset mechanism (src/config.rs:29-59, resolved by [inherit] sets); nothing restricts a set to content checks, and parse() is the same either way. The two outliers are the argument: two repositories hand-rolled the binary's codepoint class as a regexp literal under the same rule id, declared no git.hooks, and both still reconcile green -- a writable copy of a compiled-in check, invisible to every check uphold has.

Current set adoption, however, is the reason to be careful: process-residue 48, unmanaged-pins 43, credentials 34, broken-links 6, host-identity 1, captured-fixtures 0. 17 repositories carry no [inherit] at all. host-identity was promoted specifically to end 29 hand-copies; 1 repository inherits it and all 29 copies survive, across 16 distinct files.include lists, none of them the bundled ["."]. Shipping four more sets into that vacuum produces four more unadopted sets.

Ship order, where the order is the design:

• 1. A new builtin no-hand-copied-base-rule, appended to process-residue (already at 48/65). It fires only on a local rule carrying an id a bundled set owns, from a set the repository does not inherit -- a deliberate shadow of an inherited set stays silent, since that is the documented override. Refusal names the id, the owning set, and the coverage delta. Registers at manual for one release so it is a report before it is a gate.
• 2. Independent of any set work: emit the derived-owner note on the allow path in src/guard/push.rs, not only inside the refusal block at lines 88-97. 50 of 65 repositories run prevent-public-push with no owner pinned, and the guard is silent about being in the mode its own comment calls "tautological for the one remote most likely to be wrong". Exit 0, a note, not a gate.
• 3. Four guard sets -- commit-message-residue (the two commit-msg guards), unreviewed-history (no-local-merge + no-merge-commit), invisible-characters (the tree-wide unicode guard, separate because it is slow), stale-pins (separate because it reaches the network) -- plus a fifth, unowned-push, carrying prevent-public-push with a new owner_required = true field so inheriting the set never decides who you are. Naming law from docs/adr/0001:120-136 applies: named by what it refuses.
• 4. Same release, mandatory: warn when a repository shadows a bundled id with a different check field (regexp where the set ships builtin). Without it, adding invisible-characters makes the two forked copies permanently invisible behind a set they silently shadow, and the preset hides the defect it was added to remove.
• 5. Same release: set provenance in guard refusal output, e.g. refused by 'no-merge-commit' [set: unreviewed-history]. A guard arriving from a config line that named no git hook is astonishment unless the refusal says where it came from.

Explicitly not built, and ROADMAP.md:60-63 should be rewritten in the same commit to say which is which:

• [inherit] in policy/upheld.toml. Rejected on the merits, and additionally unimplementable as specified: [rule.X] is a keyed table, but [[enforce]] is an array of tables in which one rule id legitimately carries several principles. Six declarations already do this, including this repository's own (catalog-tests under both explicit-unknown and single-authoritative-source). There is no shadow-by-id to build; the only available override semantics would silently drop one of two legitimate claims. Separately, uphold check verifies the rule half and cannot verify the principle half at all, so presetting it would report green over an assertion no human made and no machine checked -- and these claims are exported outward via --oscal.
• A generator or --init template that copies bytes into consumer repositories. An inherited set is re-read from the binary every run, so "this repository is running a stale copy" is not a state that can exist; a generator reintroduces it.
• A take-everything shorthand. src/config.rs:252-258 records that use_default = true was removed for exactly this reason.

Risks to hold, if the above is taken:

• Sets ship compiled in, so a rev bump can change behaviour with zero diff in the consuming tree. With guards inside, a rule added to an existing set starts refusing commits in up to 65 repositories with nothing in any tree to review. Constraint: sets are additive-only within a major version, and a new guard gets a new set name rather than joining an existing one. Nothing enforces this today, and uphold rules --set NAME needs to be diffable across versions before step 3 ships.
• Adopting host-identity is not a no-op and must not be sold as one. The bundled rule scans ["."] while all 29 copies scan a strict subset, quietly avoiding vendored trees, target/, and test corpora. Per-repository landing, not a fleet sweep.
• If step 3 ships and steps 2 and 4 slip, the outcome is worse than doing nothing: the two forked reimplementations become permanently hidden behind a set they shadow, and 50 repositories keep a silent derived-owner guard while looking newly tidy.

Not covered

The catalog itself -- principles/*.toml schema consistency across the 24 records, overlap or unfalsifiability between principles, and whether name-index.json can drift from its source -- was not reached. That reviewer failed on a connection error and returned nothing.

[HackingGate]

Worked through this against the tree. The headline finding is about the audit itself, so it goes first.

This was written against a pre-1.1.1 checkout

Thirty-three of the items listed here are already closed in the released tree, by 0a2da20 ("Make a check that could not look say so, everywhere it can happen") and e437d86. Each was re-verified with file:line evidence before being set aside, rather than fixed a second time:

• A / B / C, src/selection.rs and src/main.rs — the not_text_paths deadlock (a scoped writer thread drains stdin while the parent reads stdout, with a 3000-path regression test that fails rather than wedges), .flatten() (gone; walk errors are collected and force Exit::Broken), index selection (git ls-files -z is primary, the walk is a fallback with every ignore source disabled), the out-of-repo files.include (a refusal naming the offending spec), the argv panic, discover climbing past .git, and the --policy grandparent inference.
• F, src/audit.rs — reachable_blobs walks rev-list --objects; read_conversation asks for title,body,comments; --for-publication has a reachable clean path.
• G, the reconciler — moot in the shape described. The Python holds no policy reader at all now; the reconcile moved into src/check.rs in fc596df, and scripts/check_hook_pins.py was deleted in 0a2da20. read_text catches UnicodeDecodeError, [review] fields are validated, --emit refuses a path that escapes the root, --coverage intersects with supplied.

If you re-run the fleet audit, pointing it at a current checkout is the first change to make.

What was still live, and landed in #18

• no-private-repo-names-staged named the file but never the line. It reads the patch hunk by hunk now. Auditing the sibling call sites for the --no-ext-diff --no-textconv defect found two more, plus a filter that dropped an added line whose text begins ++.
• no-stale-hook-pins passed a tree in which zero hook configurations were found. Exit 2 now. "Configurations read, none names a remote pin" is a distinct note — repo: local, repo: meta and a lefthook config with no remotes: block pin nothing, and the two should not read alike.
• A [[shim]] no checker rule claims collected a subject, checked it with nothing, and ran the command anyway.
• no-published-host-identity stood in front of npm publish while the shim matched pack:* — the tarball's metadata is built by pack.
• Section H's Dependabot half: no ecosystem reads a lefthook config, so the exclusion is now stated rather than papered over.

#12 shipped in the same PR: the four Go ids are published for both runners, gofmt written once as test -z "$(gofmt -l .)", and consumer_check.sh gained a ninth question that drives all four — publishing an id the parity harness never runs would recreate the hole.

One regression, caught by consumer verification

The new shim reading printed a false could-not-look line on every ordinary command — git log -1 --oneline reported "-1 sits before the subcommand". An unclassifiable option leaves the subcommand in doubt only where the two readings disagree about which word it is. Fixed before merge. Worth stating as a general rule: a refusal a reader sees on every command is a refusal they stop reading, which costs the one invocation where it was true.

Still open, and why

• Sections I and J — the rule halves that did not survive the port, and the base-sets proposal. Design work, not defects. Section J's own risk analysis (ship steps 2 and 4 or do not ship step 3) still holds and is the right gate.
• src/audit.rs:482 — the blob read side still spawns one git cat-file blob <sha> per blob, with no cap, deadline or progress. The deadlock half is genuinely fixed; this is latency and observability, and a slow run is indistinguishable from a hung one.
• src/audit.rs:214 — exit 0 is still unreachable for a repository with genuinely zero pull requests. A successful refs/pull/*/head fetch that brings back nothing is pushed into unreadable. git ls-remote origin 'refs/pull/*/head' would separate "the forge retains none" (a fact, clean) from "the fetch matched nothing" (unread).
• src/audit.rs:216 — a missing \ line continuation leaves fourteen literal spaces in report output.

Verified on consumers

Read-only against five repositories on one machine — 39-submodule superproject at 36G, plus four others. scan, check and guard exit 0 on all five, byte-identical before and after. Pre-push over 744 commits: 0.30s, no hang. A submodule with .git and no policy refuses rather than borrowing the superproject's. The lefthook-only install path was driven on a fixture, since no consumer here carries a top-level lefthook.yml — that remains the least-exercised path in the matrix.