chore: enumerate what belongs at the repository root by justinchuby · Pull Request #2036 · justinchuby/onnx-genai

justinchuby · 2026-08-24T21:31:43Z

What

.body.md — the PR body for #2026 — is tracked on main right now. This removes it and adds a Diff guard job that makes the root an enumerated set rather than a pattern-matching problem.

Why not another .gitignore pattern

It is the fourth file of this class, and it landed after the two most recent fixes for it:

file	arrived	removed by
`.commitmsg/`	—	`490b846c3`
`.commitmsg`	#1881	#1999
`.pris_v4.log` (1364-line cargo transcript)	#1951	#1975
`.body.md`	#2026, today	this PR

Every repair added one more pattern. The block's own comments predict the failure twice — "the name matching none of the patterns above and nothing stopped the next one" and "the file was named outside every pattern above" — and then it happened again, hours later, to a file whose intended name (.pr-body.md) is already listed. Someone typed a shorter one.

Three non-converging iterations is enough evidence. A pattern list has to predict the next filename; an allowlist does not.

The check

Asserts the repository root is exactly .github/root-file-allowlist.txt. It lives beside deletion-ratio because it guards the same blind spot from the other end: that job catches a change too large for anyone to read, this one catches a single added line invisible in a diff of hundreds. Both are cases where the shape of the change, not its content, is the signal.

Adding a legitimate root file = add a line to the list in the same PR. That is deliberately not a label: a label is ephemeral, a line in a tracked file is a permanent record of the decision, which is the property the existing .gitignore comments were reaching for.

Validation

Six arms, driving the script extracted from the workflow YAML rather than a copy — a seam that reimplements the check passes whether or not CI runs the same thing (per #1897):

arm	expect	result
clean root	accept	`rc=0`, "Root is exactly the 16 allowlisted file(s)"
stray root file (`.body.md`)	reject	`rc=1`, names the file
nested `crates/…/.body.md`	accept	`rc=0` — not over-broad
stale allowlist entry	reject	`rc=1` — the list cannot rot into a rubber stamp
allowlist file missing	reject	`rc=1` — fails closed, no vacuous pass
new root file, allowlisted in-PR	accept	`rc=0` — escape hatch works

Two of those are the ones I would have skipped if I were not being careful. Missing-allowlist matters because the natural implementation returns success when it cannot find its own input — the guard would go permanently green the moment someone moved the file. Stale-entry matters because an entry that outlives its file silently pre-approves that exact filename, so a rotting list is worse than none.

.gitignore also gets /.body.md, verified both directions (root ignored, nested not, shadows zero tracked files). That line is explicitly the trailing half of the repair — it is the layer that has failed four times, kept because it stops the local git add -A, not because it is the fix.

Not claimed

This does not stop a stray file in a subdirectory; the root is where the evidence is (4/4), and a repo-wide version would need a scratch-file heuristic, which is the guessing game this PR is trying to end.

`.body.md` -- the PR body for #2026 -- reached `main` today, hours after two separate fixes to the .gitignore block written for exactly this class. It is the fourth file of that class: .commitmsg/ removed by 490b846 .commitmsg re-added by #1881, removed by #1999 .pris_v4.log 1364-line cargo transcript, #1951, removed by #1975 .body.md PR body for #2026, removed here Each repair added one more pattern to .gitignore, and each time the next file was named outside every pattern written for the previous one. The comments in that block say so in as many words -- twice. Three non-converging iterations of the same fix is enough evidence to stop predicting filenames. Adds a `Diff guard` job asserting the repository root is exactly the files listed in .github/root-file-allowlist.txt. It sits beside the deletion-ratio job because it guards the same blind spot from the other end: that job catches a change too large to read, this one catches a single added line invisible in a diff of hundreds. Adding a legitimate root file means adding a line to the list in the same PR -- unlike a label, that leaves a permanent record of the decision. The /.body.md ignore line is kept as the trailing half of the repair. Verified against six arms, driving the script extracted from the workflow rather than a copy of it: a clean root accepts; a stray root file rejects; a nested file of the same name accepts, so the rule is not over-broad; a stale allowlist entry rejects, so the list cannot rot into a rubber stamp; a missing allowlist fails closed rather than passing vacuously; and a new root file that is allowlisted in the same PR accepts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Independent review found the escape hatch does not work for non-ASCII filenames, which is the one thing the check must never get wrong: adding the file to the allowlist is the documented way to add a root file. Under git's default core.quotePath=true, `git ls-files` reports a non-ASCII name in an octal-escaped, quoted form. Reproduced: a root `café.txt` correctly added to the allowlist was rejected, and the two diagnostics contradicted each other -- reporting `"caf\303\251.txt"` as unlisted and `café.txt` as a stale entry in the same run. Fixed with `git -c core.quotePath=false ls-files`. Also from the review: - An allowlist containing only comments failed closed but printed *zero* diagnostics, because the empty `grep` aborted the script under `set -e` before any message. A red X with no explanation is a bad guard even when the verdict is right. Now checked explicitly, with a message. This also keeps an empty list from reaching `comm`, where two empty inputs compare equal and would pass vacuously. - Entries are stripped of CR and surrounding whitespace. A CRLF checkout would otherwise mismatch every entry at once, and trailing whitespace on an entry is invisible in review but silently never matches. .gitattributes pins the file to LF so the CR case does not arise in the first place. Battery is now ten arms, all driving the script extracted from the workflow: the six from the original commit still pass, plus non-ASCII-allowlisted (accepts, was rejecting), comments-only (rejects with 3 diagnostic lines, was 0), CRLF allowlist (accepts) and trailing-whitespace entry (accepts). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

justinchuby · 2026-08-24T22:06:48Z

Independent review response — `a7524bf00`

Verdict was APPROVE WITH NITS, one MAJOR. I reproduced every finding before fixing it rather than taking the report on trust, and all four actionable ones were real.

M1 — the escape hatch did not work for non-ASCII filenames (fixed)

The worst possible place for this bug: adding the file to the allowlist is the documented way to add a root file, so the one path a contributor is told to take was broken.

Under git's default core.quotePath=true, git ls-files renders a non-ASCII name octal-escaped and quoted. Reproduced with a root café.txt correctly added to the allowlist — rejected, with two diagnostics that contradict each other in the same run:

::error::Root-level file(s) not on the allowlist:
::error::  "caf\303\251.txt"      <- what git reported
::error::Allowlist entr(ies) for files not present at the root:
::error::  café.txt               <- the entry the contributor correctly added

So it simultaneously said "your file is unlisted" and "your entry is a stale rubber stamp" about the same file. Fixed with git -c core.quotePath=false ls-files; that arm now returns rc=0.

I want to be clear about why I missed it: all six of my original arms used ASCII filenames. I tested the guard's logic thoroughly and its input encoding not at all. That is the same failure I have been describing in other people's guards all week — exercising the axis you thought of rather than the one the defect lives on.

m1 — failing closed with no diagnostic (fixed)

A comments-only allowlist exited 1, which is the right verdict, but printed zero ::error:: lines: the empty grep aborted the script under set -e before any message could be emitted. Verified: rc=1, error-lines=0. A red X with no explanation is a bad guard even when its verdict is correct. Now checked explicitly with a message — error-lines=3.

This also closes a latent vacuity the reviewer flagged: an empty list must never reach comm, where two empty inputs compare equal and pass.

m2 / n1 — CRLF and whitespace (fixed)

A CRLF checkout would put a stray \r on every entry and mismatch all sixteen at once — a spectacular and hard-to-read failure. Entries are now stripped of CR and surrounding whitespace, and .gitattributes pins the file to LF so it does not arise. This repo already pins its other byte-compared files for exactly this reason.

Not changed, with reasons

n3 (labeled/unlabeled triggers inherited from the sibling job) — harmless idempotent re-runs; per-job triggers would need splitting the workflow. Agreed, leave.
n4 (no push trigger) — same limitation as deletion-ratio. main requires PRs, so the gap is theoretical; narrowing it belongs to a change that moves both jobs, not this one.
roy_validate.sh — the reviewer flagged it as scratch-adjacent. I checked: tracked deliberately via perf(cpu): consume packed int4 nibbles directly in accuracy-4 decode (1.2-2.4x) #1619/perf(cpu): remove per-block bookkeeping from the int4 acc4 decode kernel (1.69x t=1, 1.38x block 64) #1628, and .gitignore covers its logs. Correct to grandfather in for this PR's snapshot; whether a personal repro script belongs at root is a maintainer call, not a hygiene bug, and folding it in here would hide a judgement inside a mechanical change.

Battery: 10 arms, all green

arm	expect	before	after
clean root	accept	✅	✅
stray root file	reject	✅	✅
nested same name	accept	✅	✅
stale entry	reject	✅	✅
allowlist missing	reject	✅	✅
new root file allowlisted in-PR	accept	✅	✅
non-ASCII, allowlisted	accept	❌ `rc=1`	✅ `rc=0`
comments-only allowlist	reject with a message	⚠️ silent	✅ 3 lines
CRLF allowlist	accept	—	✅
trailing-whitespace entry	accept	—	✅

All driven by the script extracted from the workflow YAML, not a copy of it.

codecov · 2026-08-24T22:50:49Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.98%. Comparing base (398cff8) to head (a7524bf).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2036      +/-   ##
==========================================
+ Coverage   80.25%   80.98%   +0.72%     
==========================================
  Files         409      425      +16     
  Lines      189791   209135   +19344     
  Branches   189791   209135   +19344     
==========================================
+ Hits       152319   169361   +17042     
- Misses      32048    34115    +2067     
- Partials     5424     5659     +235

Flag	Coverage Δ
cli-ort-linux	`72.51% <ø> (?)`
cli-ort-windows	`72.01% <ø> (?)`
mlas	`85.20% <ø> (?)`
offline	`81.13% <ø> (+0.88%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 67 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

justinchuby · 2026-08-24T23:55:09Z

Merged as 54625db9d. Post-merge verification, since a guard that only ever ran on its own PR is a guard nobody has watched work.

The stale-base check, which mattered here more than usual. Per Pris's finding on #1817, my green was taken against a frozen merge base — from the job's own log, Merge a7524bf00 into 6a4b22eb1. main had advanced 5 commits past that by merge time. That is normally a reason to shrug; here it isn't, because this guard's subject is literally "the set of files at the repository root on main". A stale base is the one thing that could have let it pass on a tree nobody was proposing to merge.

So I checked the axis rather than the clock — 3-way cat-file -e on both operands first, then --is-ancestor:

base IS ancestor of current main; main advanced by 5 commit(s)
root blobs on current main not in allowlist:  .body.md   (this PR deletes it)
stale allowlist entries:                      (none)
root files touched by the 5 new commits:      (none)

The green transfers because the measured axis is provably unchanged across the gap — not because 18 lanes were green.

The guard, run on the merged tree, driven from the YAML rather than a copy (--is-ancestor's lesson applied to myself: a seam that reimplements the thing passes whether or not production uses it):

arm	rc	note
merged `main` as-is	0	`Root is exactly the 16 allowlisted file(s).`
novel root file `.pr-notes-v2.md`	1	matched by no `.gitignore` rule
`.body.md` force-added past its own ignore line	1	defence-in-depth
`docs/.body.md` (nested, same name)	0	root-scoped, as intended

Arm 2 is the whole thesis: a name no pattern could have predicted is still caught. Four instances and three pattern-fixes says prediction doesn't converge; an allowlist doesn't have to predict anything.

One arm of mine was vacuous and I nearly published it. My first attempt reinstated .body.md with a plain git add and got rc=0 — which I could have written up as "correct, root is clean". It wasn't correct for the right reason: git add was refused by the /.body.md ignore line this same PR added, so nothing ever reached the index and the guard was handed an empty change. The assertion passed; the experiment had no power. git diff --cached --name-only between staging and asserting is what turns that into a real arm, and it's now in every arm above.

Unrelated but re-checked on the new head: verify_cuda_test_honesty.py --self-test rc=0, --source-scan rc=0 over 84 CUDA test targets (unchanged).

— Holden

…as stale Independent review found the evidence half of this PR wrong in the way it was most embarrassing to be wrong: the walk that produced the list had the same blind spot as the guard it was written to fix. `git log --diff-filter=A --name-only` filtered to paths without a separator reports stray *files*. A stray directory's files all contain a separator, so they are discarded -- the identical mistake #2036's comparison makes. Redone on first path segments, the walk finds .goldens/, added by faedea4 and removed by 490b846 "Remove accidentally tracked PR artifacts", the same commit pair as .commitmsg/. A directory removed as a PR artifact is the most on-point instance this PR could have, and it was invisible to the method that argued for it. Seven entries in six incidents, not six. .commitmsg's lifetime was stated as ~37h. It is 19.8h (e42fa94 -> 398cff8); 29.5h if the directory add is used as the start, which conflates two incidents. Nothing yields 37h. Corrected, and every remaining figure re-measured from committer timestamps. `site`, `third_party` and `abresults` also appear in the corrected walk and are excluded with reasons rather than silently: the first two are announced reorganizations, and abresults was added by a `docs(benchmarks):` commit that says it is recording a result -- intentional when added, which is the line. Two behaviour fixes from the same review: - An entry listed twice left one copy unpaired in `comm`, reported as "not present at the root" for a name that plainly is. `actual` was deduplicated and the allowlist was not. Dedupe both, and emit a warning naming the duplicate, which is the actual mistake. - `[ -d ]` follows symlinks, so a root symlink to a directory was labelled "(directory)" and advised `git rm -r --cached`. Test `-L` first. Also recorded what comparing first segments costs: the guard sees root children only, so scratch under an already-blessed directory is invisible to it. Battery 15/15, including both new arms and .goldens/ as a second directory instance. All twelve cited SHAs verified present and ancestors of main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…2052) Follow-up to #2036, merged earlier today. **The guard it shipped cannot see a stray *directory* at the root — and two of the seven historical instances are directories, one of them cited in #2036's own header.** ## The gap #2036 compares tracked files whose path contains no separator: ```bash git ls-files | grep -v '/' ``` A stray directory has no such path. `.commitmsg/m.txt` contains a separator, so it is discarded as "not at the root" — and the thing that *is* at the root, `.commitmsg`, never appears in `ls-files` output at all. The guard doesn't fail to complain; it affirmatively reports a clean root. Not hypothetical: `.commitmsg/` reached `main` as `faedea4d1`, removed three minutes later by `490b846c3` *"Remove accidentally tracked PR artifacts"*. #2036's header lists it, with the trailing slash. **I enumerated the instance and then validated against six arms that all used a file.** Naming an instance is not testing its shape. ## The fix Compare **first path segments** — a root file contributes itself, a nested file contributes its top-level directory: ```bash git -c core.quotePath=false ls-files | sed 's#/.*##' | LC_ALL=C sort -u ``` The allowlist gains the 20 tracked root directories (36 entries), and unlisted entries are labelled `(directory)` or `(symlink)` where they are one, because the remedy differs. ## The inventory, corrected by review I claimed six instances "found by walking every root path ever added, not by collecting what people reported". **The walk had the same blind spot as the guard** — it filtered to paths without a separator, so it could not see a stray directory either. Redone on first segments: | entry | added → removed | on `main` | |---|---|---| | `.msg.txt` | `39675330b` → `bbc193117` | 1.6h | | **`.commitmsg/`** | `faedea4d1` → `490b846c3` | 3m | | **`.goldens/`** | `faedea4d1` → `490b846c3` | 3m | | `.wa64.log` | `83a51bfa6` → `c07acaa78` | 17.2h | | `.commitmsg` | `e42fa9470` (#1881) → `398cff8e5` (#1999) | 19.8h | | `.pris_v4.log` | `589d48ffd` (#1951) → `7a6482c83` (#1975) | 1.3h | | `.body.md` | `79196f89d` (#2026) → `54625db9d` (#2036) | 2.7h | **Seven entries, six incidents** — `.commitmsg/` and `.goldens/` arrived and left together. `.goldens/` is the most on-point instance available (a directory removed as a "PR artifact") and my method could not see it. Excluded, with reasons recorded in the file rather than silently: `site` (moved to onnx-genai-wiki, #1488), `third_party` (oneDNN removal), and `abresults` — 131h on `main`, the longest of any, but added by a `docs(benchmarks): record the … result` commit that says it is recording a result. Intentional-when-added is the line; `.wa64.log` rode in on a `test(cpu):` commit that never mentions it. `.wa64.log` still matters beyond the count: it predates `.pris_v4.log` by two days, so `/*.log` in #1975 was reactive to the *second* log incident. No authorship attributed — squash-merge rewrites `%an` to the merging account, so it reads identically for all seven and says nothing about who staged the file. ## Validation — 15/15 Driving the script **extracted from the workflow YAML**, never a copy. | arm | rc | |---|---| | clean root | 0 — `Root is exactly the 36 allowlisted entr(ies).` | | **stray root directory → new guard** | **1**, labelled `(directory)` | | **same stray → #2036 guard + #2036 allowlist** | **0** — `Root is exactly the 16 allowlisted file(s).` | | `.goldens/`, the second directory instance | 1 | | duplicate allowlist entry → not reported stale | 0, warning names it | | root symlink | 1, labelled `(symlink)` | | stray root file / stale entry / comments-only / missing list | 1 / 1 / 1 / 1 | | nested file under an allowlisted dir | 0 | | non-ASCII root file, allowlisted | 0 | | new root directory allowlisted in-PR / not | 0 / 1 | | CRLF allowlist | 0 | Row 3 is the control and must pair **both** of #2036's halves. My first attempt paired the old guard with the *new* allowlist: it returned 1 and looked like coverage, but the 1 came from 20 directory entries reading as stale. ## Two instrument bugs in my own battery 1. **Wrong control**, above — a control that changes two things measures neither. 2. **The staging check had the defect the guard was fixed for.** Each arm asserts its input reached the index before believing the output, but that check used bare `git diff --cached --name-only`, which renders non-ASCII as `"caf\303\251.txt"` while the guard uses `core.quotePath=false`. It reported the non-ASCII arm VACUOUS against a setup that had worked. Opus caught exactly this in #2036's guard; it reappeared in the thing measuring the guard. ## Also fixed, from review - An entry listed twice left one copy unpaired in `comm` and was reported as "not present at the root" for a name that is. Deduped both sides; duplicates now raise a `::warning::` naming them. - `[ -d ]` follows symlinks, so a root symlink to a directory was labelled a directory and advised `git rm -r --cached`. `-L` tested first. - Recorded the cost of first-segment comparison: the guard sees **root children only**. Scratch under an already-blessed directory is invisible to it. ## Scope CI-config only — `.github/workflows/diff-guard.yml` and `.github/root-file-allowlist.txt`. No Rust, no runtime behaviour, no test changes. Adding a root entry, file or directory, means adding it to the allowlist in the same PR; the error message says so. --------- Co-authored-by: holden <holden@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

#2081) Closes #2077. `Detect change scope` classifies a PR as docs-only and skips **both required checks** — `Fast (Linux x86_64)` and `Rust quality`: ```yaml fast-linux: { needs: changes, if: needs.changes.outputs.docs_only != 'true' } rust-quality: { needs: changes, if: needs.changes.outputs.docs_only != 'true' } ``` Two markdown files under `docs/` are compiled into Rust with `include_str!`, so **a pure markdown edit can break `cargo test` while classifying as docs**, skipping the checks that would catch it. ``` crates/onnx-genai-metadata/tests/capability_catalogue.rs:9 include_str!(".../docs/genai/INFERENCE_METADATA_DECISIONS.md") crates/onnx-runtime-ep-cuda/src/kernels/mod.rs:1686 include_str!(".../docs/execution/CUDA_COVERAGE.md") ``` ## Falsified, not argued Both edits below change **only** the `.md` — `git diff --name-only` returns one path, so `docs_only=true`. Baseline on `0ce253f4a` is `3 passed; 0 failed`. **Rename an HTML comment marker.** The test locates its table with `.expect(...)`: ``` panicked at crates/onnx-genai-metadata/tests/capability_catalogue.rs:12:10: capability catalogue start marker test result: FAILED. 2 passed; 1 failed ``` **Delete one catalogue table row:** ``` assertion `left == right` failed: update the normative capability catalogue when the built-in vocabulary changes left: {..., "input_presence", "linear_effects", ...} right: {..., "input_presence", "kv_cache", "linear_effects", ...} test result: FAILED. 2 passed; 1 failed ``` The second is the *exact* edit that test exists to police. The test is skipped by the class of change it was written for. ## The fix is derived, not a special case The property is not "these two paths are special", it is **a file whose bytes are compiled into an artifact is source, whatever its extension**. So the set is scanned, not listed, and cannot go stale when a third embed is added (Rule 10): ```sh if embed_hits="$(git grep -I -n -oE 'include_(str|bytes)!\s*\(\s*"[^"]+"' -- '*.rs' 2>/dev/null)"; then : else [ $? -eq 1 ] || embed_scan_ok=false; fi ``` then resolved relative to each including file with `realpath -m --relative-to`. 143 embedded targets today, of which exactly two are docs-classified. A hardcoded two-path exception would be correct today and silently wrong on the third embed — the same shape as the `.gitignore` pattern list that #2036 replaced after four consecutive predictions failed. Documented limitation: only literal-path embeds are visible. A computed path (`concat!`/`env!`) is not, and stays classified as docs. ## The dangerous way to get this wrong, which I hit in my own patch My first draft used a bare assignment. Under the default `bash -e` step shell, `v="$(git grep ...)"` **exits the step** when grep matches nothing — and a dead `changes` job *skips* `fast-linux` and `rust-quality`, and **a skipped required check satisfies the ruleset**. The fail-open form of this fix is worse than the bug it fixes. ``` bash -e -c 'v="$(grep zzz /dev/null)"; echo REACHED' -> rc=1, REACHED never printed bash -e -c 'if v="$(grep zzz /dev/null)"; then :; ...' -> rc=0, REACHED ``` A `git grep` error (rc>1) forces `docs_only=false`, matching the classifier's existing fail-closed-on-ambiguity contract. I did not change that contract; the file is careful about it and says so. ## Validation The battery drives the classifier **extracted from `ci.yml` via `yaml.safe_load`** — never a text split, because #2052 shipped a battery that text-matched a job name and therefore could not distinguish a job from a key nested inside another job — against **real commits**, under `bash -e` to match GitHub's step shell. ``` == structural == job list identical to main: True (9 jobs) fast-linux / rust-quality byte-identical to main: True 'changes' step count unchanged: True (2 -> 2) only changed job: ['changes'] == behaviour (want | new | main-control) == [OK] plain root doc want=true new=true main=true [OK] plain nested doc want=true new=true main=true [OK] EMBEDDED metadata doc want=false new=false main=true <-- changed [OK] EMBEDDED cuda doc want=false new=false main=true <-- changed [OK] EMBEDDED marker rename want=false new=false main=true <-- changed [OK] embedded + plain doc want=false new=false main=true <-- changed [OK] rust source want=false new=false main=false [OK] doc + rust source want=false new=false main=false [OK] LICENSE want=true new=true main=true 9/9, 4 arms change verdict vs main (0 would mean the fix is a no-op) == shell arms == unmutated control rc=0 docs_only=true grep finds nothing (rc=1) rc=0 docs_only=true survives bash -e grep errors (bad regex) rc=0 docs_only=false fails closed ``` Every arm is paired with **main's implementation as the control**, so an arm cannot pass by the harness failing to exercise anything — the four `<-- changed` rows are the proof the battery has power. ## Blast radius This edits a required workflow. An invalid workflow does not fail its checks, it **removes** them — observed on #2052, where a four-space indent made one job a key inside another and the PR showed 14 green with both diff-guard checks simply absent, satisfying `pending=0 && failed=[]`. That is why the structural gate above parses the YAML and diffs the job list rather than reading the diff. `changes` is the only modified job; both required jobs are byte-identical to `main`. Note this PR is **not** docs-only, so it exercises the full required set on itself. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: holden <holden@squad>

…ident `.roy-scratch/pr.md` reached main in #2150 (b054ff6). It is a `--body-file` staging artifact -- and not even the body that shipped: it is the superseded first draft, whose central claim ("the reserve half is not the identity, and it bites at exactly one width") the merged PR body explicitly retracts. A tracked, greppable, unmarked copy of a corrected claim is worse than plain scratch. `Root file allowlist` reported it correctly, named it as a directory, and printed the remedy. It has been red on main and on every PR opened since -- 0.9h at the time of this commit -- which is the reason to take the fix now rather than the reason to wait for its owner. Three changes: - remove the file; - add `/.*-scratch/` to .gitignore. This is the guessing half of the repair and it is labelled as such: five artifacts of this class have now reached main and each was named outside every pattern written for the last one; - add the incident row to both copies of the log (the allowlist header and the diff-guard comment) and bump their counts to eight entries in seven incidents. A count that stops being true is how the log stops being read. The header noted that #2036 closed a directory-shaped blind spot while only *citing* an instance of it, never testing one. `.roy-scratch/` is the first directory incident since, and the check named it: that is the positive control the earlier fix never got, so it is recorded next to the claim it settles. Verified by running the workflow's own logic against this tree (pass, root is exactly the 36 allowlisted entries), against origin/main (reports exactly `.roy-scratch`), and against an injected `.gaff-probe/` (reports it) -- so the pass is the fix, not a check that stopped discriminating. `git check-ignore` confirms the new pattern covers root `.roy-scratch/` and `.pris-scratch/` and leaves `nested/.foo-scratch/` tracked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@roy

…ident (#2161) ## What `.roy-scratch/pr.md` reached `main` in #2150 (`b054ff63e`, 22:20:23Z). This removes it, and records the incident in the two places that keep the log. `Root file allowlist` has been **red on `main` and on every PR opened since** — it inherits, so it is currently red on #2157 and on anything else branched after 22:20Z. Fixing it is not a judgement on #2150; the check fired correctly and named the exact file and remedy. ## Why it is worth a PR rather than a `git rm` The file is a `--body-file` staging artifact — and **not the body that shipped**. It is the superseded first draft: | | claim about the reserve half | |---|---| | committed `.roy-scratch/pr.md` | "The reserve half **is not** the identity, and it bites at exactly one width." | | #2150's merged body | "The reserve half **is** the identity on that mask too. **This is a correction to the first version of this PR**…" | So `main` currently carries a tracked, greppable, root-level copy of a technical claim its own author retracted, with nothing marking it stale. That is worse than ordinary scratch: ordinary scratch is merely noise. ## The three changes 1. **Remove the file.** 2. **`.gitignore`: `/.*-scratch/`** — root-anchored, per the `/*.log` convention. The comment labels this as *the guessing half* of the repair, because that is what the record shows it is: five artifacts of this class have now reached `main` (`.msg.txt`, `.commitmsg`, `.pris_v4.log`, `.body.md`, `.roy-scratch/pr.md`) and **each was named outside every pattern written for the previous one** — `.body.md` landed hours after two separate fixes to that very block, and the name it should have had (`.pr-body.md`) was already ignored. 3. **Record the incident** in `.github/root-file-allowlist.txt`'s header *and* the `diff-guard.yml` comment, and bump both counts: seven entries / six incidents → **eight / seven**. Both files state their count in prose; a count that quietly stops being true is how a log stops being read. Per #2036's precedent the row carries the introducing and removing commits and the dwell time. ## One thing this settles rather than asserts The allowlist header says #2036 shipped a directory-shaped blind spot **while citing an instance of it in its own header** — "naming an instance is not the same as testing its shape". `.roy-scratch/` is the **first directory incident since that gap was closed**, and the check named it, as a directory, with the right remedy: ``` ::error::Root-level entr(ies) not on the allowlist: ::error:: .roy-scratch/ (directory) ``` That is the positive control the earlier fix never got, so it is recorded next to the claim it settles. ## Verification The workflow's logic run verbatim against three trees — because "the check passes now" is worth nothing unless the check still fails on something: | tree | expectation | result | |---|---|---| | this branch | pass | `Root is exactly the 36 allowlisted entr(ies).` | | `origin/main` (pre-fix) | report the cause, and only it | `unlisted: .roy-scratch` | | this branch + injected `.gaff-probe/x.md` | still discriminating | `UNLISTED: .gaff-probe`, rc=1 | And `git check-ignore -v` on the new pattern: `.roy-scratch/pr.md` and `.pris-scratch/a.md` both ignored at line 90; `nested/.foo-scratch/b.md` **not** ignored (rc=1), which is the documented root-only scope. No other path in the tree references `.roy-scratch` (`grep -rn`, excluding the three files changed here). ## Notes - @roy — this is your draft; if you would rather keep it, keep it *somewhere untracked* and say so and I will close this. It is a superseded draft of a merged PR's body, so I have assumed it is disposable. - Base is `7cb5d98df`. Expect `Mobius metadata packages (signal)` red — that is #2154, inherited from `main`, unrelated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

justinchuby · 2026-08-26T14:14:24Z

Post-merge validation of the guard this PR added. The enumeration is sound, but it landed in the advisory tier, so it cannot block the thing it was built to block.

The guard is not a required check

The job is root-files / display name Root file allowlist, in .github/workflows/diff-guard.yml:120-126, consuming .github/root-file-allowlist.txt:130. All of that is present on main and working.

The required contexts come from the active ruleset "merge rules" (~DEFAULT_BRANCH, strict=false), and are exactly two:

REQUIRED: Fast (Linux x86_64)
REQUIRED: Rust quality

Root file allowlist is not among them. Neither is Deletion ratio.

Why that is not theoretical

Auto-merge waits only on required checks, and it is armed routinely here — on #2023 it was enabled at 15:18:14Z and the PR merged at 17:11:29Z with 0 reviews. An advisory red does not stop that.

The precedent is on the record: #1982 merged with CLI ORT (Linux x86_64) and CLI ORT (Windows x86_64) both FAILURE. CLI ORT is likewise absent from the required set. So a red advisory check demonstrably does not block a merge in this repository — that is exactly how #1982 put a failing shape_dispatch_gate on main.

Applied here: a fifth root-level file of the .body.md class can land on main with Root file allowlist red, which is the outcome this PR exists to prevent. The .gitignore half still works, because that layer acts before the commit; it is the enumerating half — the half added precisely because pattern lists kept failing to predict the next name — that is unenforced.

Worth noting the placement rationale in the PR was that it "sits beside deletion-ratio". That is true, and deletion-ratio is itself advisory — so the neighbour the placement was reasoned from is also unable to block. Same class as #2015 (closed by #2023) and #2138.

An instrument warning, because I nearly published a false confirmation

My first pass "confirmed" this with gh pr view --json statusCheckRollup and its isRequired field. That field is void in this repository. It reports advisory for Fast (Linux x86_64) and Rust quality — the two genuinely required contexts — on merged and open PRs alike:

open PR #2216:  advisory  Fast (Linux x86_64)
                advisory  Rust quality

It returns the same value regardless of the property it names, so it cannot support either conclusion. The likely cause is that required-ness here comes from a ruleset, while isRequired reflects classic branch protection, which this repo does not use.

I caught it only because I independently knew those two contexts are required — a control I happened to be holding, not one I designed. The sound source is:

gh api repos/justinchuby/onnx-genai/rulesets/<id> \
  --jq '.rules[]|select(.type=="required_status_checks")
        |.parameters.required_status_checks[].context'

Anyone reasoning about required-vs-advisory from statusCheckRollup.isRequired — in a script, a gate, or a review — is reading a field that is constant here.

@holden

…2244) One line of context each for three directories, measured rather than assumed. ## What I found Five entries sit at the repository root that are **untracked and matched by no ignore rule**: ``` .ort_upstream/ .squad/ .validation-worktrees/ .worktrees/ hogpids.txt ``` `git add -A` in a primary checkout stages **69 files** from `.validation-worktrees` alone. `hogpids.txt` is the one I'd flag to a human reader: it has no leading dot, so in `git status` it reads as ordinary repository content rather than as scratch. This is the untracked precursor to the class that has now arrived four times (`.commitmsg/`, `.commitmsg`, `.pris_v4.log`, `.body.md`). Those were all *files someone created and forgot*. These three are **directories every agent creates by convention** — throwaway worktrees for validating a PR against latest `main`, and a vendored upstream checkout — so unlike a filename, they are enumerable and don't require predicting anything. ## This does not replace the allowlist job — verified, not assumed @holden's `Root file allowlist` is the backstop and it works. I extracted the script structurally (`yaml.safe_load` → `jobs['root-files']`, asserting exactly one `run:` step, per the lesson from #2052) and drove it: | arm | rc | | |---|---|---| | clean tree | 0 | `Root is exactly the 36 allowlisted entr(ies).` | | scratch file staged, **before** this change | 1 | `.validation-worktrees/ (directory)` | | `git add -f` past the **new** ignore rules | **1** | still refused, same message | | only `.gitignore` staged (control) | 0 | passes | The third row is the one that matters: an ignore rule that *hid* a force-added file from the guard would be worse than the problem. It doesn't — `ls-files` reports the root segment of a tracked path regardless of ignore state. So this is a layer in front of the backstop, not a replacement for it. Each staging arm asserts `git diff --cached --name-only` before believing any rc, because an acceptance arm can otherwise be satisfied by its own setup silently failing — which is exactly how a `git add` refused by an ignore rule produced a misleading rc=0 in #2036. ## Deliberately not included - **`hogpids.txt`** — another agent's run artifact. It should be deleted by its owner, not permanently ignored; writing a rule for it would be predicting filenames again, which is the approach #2036 retired. - **`.squad/`** — whether the team root is meant to be committed is a policy question, not a chore. Flagging it rather than deciding it: right now it is one `git add -A` away from being committed, and I don't think that's intended either way by accident. @justinchuby, worth a decision. Root-anchored (`/.worktrees/`, not `.worktrees/`) because a crate could legitimately hold a nested fixture directory of any of these names; only the repository root is scratch. Co-authored-by: pris <pris@squad.local> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

`.review/` was found untracked in two worktrees. It is caught by neither existing pattern: * `/.*-scratch/` requires a `-scratch` name, and `.review` is not one. * `/*.log` is anchored to the repository root, so `.review/pr.log` -- one directory down -- matches nothing. Verified with `git check-ignore -v`: before this change `.review/pr.log` returns no match while a root `pr.log` matches `/*.log`. The `Root file allowlist` check does catch a committed `.review/` (it compares first path segments, so directories are visible since #2052 -- #2036 compared files only and was blind to exactly this), but only once it is on main, by which point every open PR has inherited the red. This is the prevention half; the allowlist stays the backstop for a name nobody predicted. Negative control: a nested `.log` outside `.review/` is still trackable, so fixtures and crates that legitimately carry one are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

) ## What Adds `/.review/` to `.gitignore`. ## Why this one is not covered `.review/` was found untracked in at least two worktrees. Neither existing wildcard reaches it: | pattern | why it misses | |---|---| | `/.*-scratch/` | requires a `-scratch` name; `.review` is not one | | `/*.log` | anchored to the repository root, so `.review/pr.log` — one directory down — matches nothing | Measured before the change: ``` $ git check-ignore -v .review/pr.log (no match) $ git check-ignore -v pr.log .gitignore:98:/*.log pr.log ``` That asymmetry is the whole bug: the root-anchoring that makes `/*.log` safe for fixtures is exactly what lets the same filename through one directory down. After: ``` $ git check-ignore -v .review/pr.log .review/body.md .gitignore:99:/.review/ .review/pr.log .gitignore:99:/.review/ .review/body.md ``` **Negative control** (the half that matters for any broadening change): a nested `.log` *outside* `.review/` is still trackable, and a `sub/.review/pr.log` deeper in the tree is deliberately **not** swallowed, so a fixture or crate that legitimately carries either is unaffected. `git add -A --dry-run` stages 0 `.review` paths after the fix. Nothing currently tracked matches: `git ls-files | grep -i review` returns only `.github/skills/reviewer-protocol/…` and `*-review.md`, none of which live under a root `.review/`. Root-anchoring is deliberate and house-consistent — every scratch pattern in this block (`/.body.md`, `/.*-scratch/`, `/*.log`, `/.merge.log`) is anchored, for the stated reason that an unanchored rule could hide a legitimately-tracked nested directory. ## Why bother, given the allowlist exists `Root file allowlist` does catch a committed `.review/`: it compares first path segments, so directories are visible — since **#2052** (`e974224dd`, "the root allowlist could not see a stray directory"). Note that **#2036 is the counter-example, not the fix**: it compared files only and was blind to precisely this case, as `diff-guard.yml` says in its own comment. But the check fires only *after* the directory is on `main`, and by then every open PR has inherited the red. That is the sequence that played out with `.roy-scratch/pr.md` in #2150, which then needed two separate removal PRs (#2156 and #2161) because delete/delete is not a merge conflict and nothing warned either author. So this is the prevention half. The allowlist stays the backstop for a name nobody predicted — the two are not substitutes, and the comment block in `.gitignore` says so. ## Review Independent Opus review: no MUST-FIX. One SHOULD-FIX — the provenance citation originally read `#2036` where the directory-visible comparison actually landed in `#2052`. Corrected in both the commit message and this body. In a file whose entire value is accurate incident provenance, citing the one PR that had the opposite behaviour was worth a re-run to fix. ## Scope One `.gitignore` entry plus its comment. No code, no CI config, no behaviour change. `.gitignore` rules never apply to already-tracked files, and no tracked file lives under a root `.review/`. Co-authored-by: holden <holden@squad.local> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

justinchuby and others added 2 commits August 24, 2026 21:30

justinchuby marked this pull request as ready for review August 24, 2026 23:47

justinchuby merged commit 54625db into main Aug 24, 2026
18 checks passed

justinchuby deleted the squad/root-file-allowlist branch August 24, 2026 23:48

This was referenced Aug 25, 2026

ci(miri): fail a Miri step that ran zero tests instead of reporting ok #2048

Merged

fix(diff-guard): the root allowlist could not see a stray directory #2052

Merged

This was referenced Aug 25, 2026

docs-only CI classifier skips both required checks for markdown that is compiled into Rust #2077

Closed

fix(ci): markdown that is compiled into Rust is not a docs-only change #2081

Merged

justinchuby mentioned this pull request Aug 25, 2026

chore: remove .roy-scratch/ from the root, and record the seventh incident #2161

Merged

justinchuby mentioned this pull request Aug 26, 2026

chore: ignore the local-only working directories every agent creates #2244

Merged

This was referenced Aug 27, 2026

Shared checkout is 486 commits stale: hostlock.sh does not exist there, and sequential 'acquire' fails open (runs saturating work unlocked) #2162

Open

chore: ignore .review/, the scratch directory both wildcards miss #2288

Merged

Navigation Menu

Repository navigation

chore: enumerate what belongs at the repository root - #2036

chore: enumerate what belongs at the repository root#2036
justinchuby merged 2 commits into
mainfrom
squad/root-file-allowlist

justinchuby commented Aug 24, 2026

Uh oh!

justinchuby commented Aug 24, 2026

Uh oh!

codecov Bot commented Aug 24, 2026 •

edited

Loading

Uh oh!

Uh oh!

justinchuby commented Aug 24, 2026

Uh oh!

justinchuby commented Aug 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

justinchuby commented Aug 24, 2026

What

Why not another .gitignore pattern

The check

Validation

Not claimed

Uh oh!

justinchuby commented Aug 24, 2026

Independent review response — a7524bf00

M1 — the escape hatch did not work for non-ASCII filenames (fixed)

m1 — failing closed with no diagnostic (fixed)

m2 / n1 — CRLF and whitespace (fixed)

Not changed, with reasons

Battery: 10 arms, all green

Uh oh!

codecov Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

justinchuby commented Aug 24, 2026

Uh oh!

justinchuby commented Aug 26, 2026

The guard is not a required check

Why that is not theoretical

An instrument warning, because I nearly published a false confirmation

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Independent review response — `a7524bf00`

codecov Bot commented Aug 24, 2026 •

edited

Loading