Repository navigation
fix(diff-guard): the root allowlist could not see a stray directory - #2052
Merged
Merged
Conversation
#2036 compares tracked files whose path has no separator. A stray *directory* at the root has no such path -- `.commitmsg/m.txt` contains a separator, so the filter discards it as "not at the root" and the only thing actually at the root is a name that never appears in `ls-files` output at all. That is not hypothetical. `.commitmsg/` reached `main` as faedea4 and was removed three minutes later by 490b846 "Remove accidentally tracked PR artifacts". #2036's own header cites it. Naming an instance is not the same as testing its shape: every arm I validated #2036 with used a file. Compare first path segments instead, so a root file contributes itself and a nested file contributes its top-level directory. The allowlist gains the 20 tracked root directories and the diagnostics label which unlisted entries are directories, because the remedy differs (`git rm -r --cached`). Walking every root path ever added on `main` and subtracting what is still here turns up two more instances nobody had reported -- .msg.txt (3967533) and .wa64.log (83a51bf). Six, not four; both headers now say so, and both record how long each sat on `main` rather than only that it happened. Battery 12/12 driving the script extracted from the workflow YAML. The arm that matters is the control: the same stray directory against #2036's guard and #2036's allowlist returns 0 and prints "Root is exactly the 16 allowlisted file(s)" -- it does not merely fail to complain, it affirmatively reports a clean root. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2052 +/- ##
==========================================
+ Coverage 80.31% 80.87% +0.56%
==========================================
Files 409 425 +16
Lines 190562 210133 +19571
Branches 190562 210133 +19571
==========================================
+ Hits 153046 169945 +16899
- Misses 32082 34517 +2435
- Partials 5434 5671 +237
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…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>
Owner
Author
|
Independent review (Opus, adversarial, read-only): REQUEST CHANGES — two MAJOR, two MINOR, three NIT. Every finding reproduced before fixing. Pushed M1 — the walk had the same blind spot as the guard. Accepted, and it's the worst possible place to have made it.The reviewer ran my own stated method and got nine removed root segments where I reported six. The one that matters: Why I missed it, reproduced: # what I ran for #2052's table
awk '/^@/{c=$1} !/^@/ && NF && $0 !~ /\// {print ...}'
^^^^^^^^^^^^^^ paths with no separatorA stray directory's files all contain a separator, so every one is discarded. That is character-for-character the defect this PR exists to fix, in the instrument used to argue for it. I wrote "found by walking every root path ever added, not by collecting what people reported" — and the walk could not see the shape I was claiming to have discovered. Redone on first segments:
Seven entries in six incidents — On the other three the reviewer surfaced, excluded with reasons in the file rather than silently:
M2 —
|
b1b3d1e indented `root-files:` by four spaces instead of two while rewriting the comment above it. That is valid YAML -- the key becomes a member of `deletion-ratio` rather than a sibling of it -- so nothing reports a syntax error. GitHub simply stops seeing a job: yaml.safe_load(...)['jobs'].keys() -> ['deletion-ratio'] The push-event run for b1b3d1e failed with "This run likely failed because of a workflow file issue" and zero jobs. The pull_request check list did not turn red: `Deletion ratio` and `Root file allowlist` simply *vanished* from it, leaving fourteen green checks and no indication the guard was gone. An invalid workflow removes checks rather than failing them, so the surviving signal is an absence, and `gh pr checks` cannot express an absence. The battery did not catch it either, which is the more useful half. It located the script by splitting the YAML text on " root-files:" -- and that also matches " root-files:", because the four-space form contains the two-space form as a substring. It extracted 90 correct lines from a job that no longer existed and reported 15/15. Driving the real script text is not the same as driving the real workflow. Extraction now goes through yaml.safe_load and asserts the job exists under `jobs`, carries the expected `name:`, and has exactly one `run:` step. Two arms added: the extractor accepts the current file, and refuses the four-space mutant. The old text-split extractor accepts that mutant silently -- verified, not assumed. Battery 17/17. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Correction, and the most useful thing in this PR is now the mistake rather than the fix. I shipped a commit that deleted the guard, and CI reported fourteen green checks
yaml.safe_load(open('.github/workflows/diff-guard.yml'))['jobs'].keys()
# ['deletion-ratio'] <- root-files is not a jobNo syntax error. Nothing to read as a warning. What the signal looked like, which is the part worth internalisingThe push-event run failed — "This run likely failed because of a workflow file issue", zero jobs. But on the pull request:
I noticed only because I knew exactly which two checks this branch must have, and counted them. That does not generalise. diff <(git show origin/main:.github/workflows/X.yml | python3 -c "import yaml,sys;print('\n'.join(yaml.safe_load(sys.stdin)['jobs']))") \
<(python3 -c "import yaml;print('\n'.join(yaml.safe_load(open('.github/workflows/X.yml'))['jobs']))")The battery didn't catch it either, and that half is worseEvery arm claimed to drive "the script extracted from the workflow YAML, not a copy". It located the script by splitting on the literal I had been treating "drives the real text" as equivalent to "drives the real thing". It isn't. The text was real; the job was gone, and the text carries no evidence of that. Extraction now goes through Why this keeps happening to me specificallyThree times in two days now, the defect has been in the instrument, resolving against itself instead of against the subject:
Each time the reading looked like a reading. Battery 17/17, with the two structural arms added. Both |
justinchuby
marked this pull request as ready for review
August 25, 2026 03:16
This was referenced Aug 25, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 25, 2026
## What `cargo test --lib <name> -- --exact` with a **bare** test name matches nothing when the test lives in a module. It prints `running 0 tests`, `test result: ok`, and **exits 0**. To a script reading the exit code that is indistinguishable from the test passing. Inside a mutation battery it is indistinguishable from *the mutant surviving* — and that is the dangerous direction, because every arm then reports "your test does not cover this". You go write coverage you already have, or conclude a guard is unreachable and delete it. ## Measured, not argued ``` cargo test -q --lib a_continuing_turn_is_admitted -- --exact running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 2 filtered out exit 0 cargo test -q --lib tests::a_continuing_turn_is_admitted -- --exact running 1 test test result: ok. 1 passed; 0 failed; 0 ignored; 1 filtered out exit 0 ``` Two properties make it silent rather than noisy, both measured: 1. **A renamed test and a test that never existed produce byte-identical output.** So a battery repeats the same verdict forever after a rename — there is no moment at which it announces that it stopped measuring. 2. **Whether a bare name matches depends only on module nesting.** A `#[test]` at crate root *is* its own full path and matches; the same name inside `mod tests` does not. One battery can therefore hold working arms and vacuous arms simultaneously, which reads as *partial coverage* rather than as a broken instrument. ## This is the third independent discovery in this repo | where | form | |---|---| | `crates/onnx-runtime-hostmon/tests/agrees_with_hostlock_sh.rs` (#2026) | guarded in code, with a comment naming the exact failure mode | | `docs/benchmarks/2026-08-21-int4-packed-nibble-avx2.md:313` (#1628) | recorded after the battery reported **seven of seven** mutations undetected | | #1982 | paid for again, in full | The knowledge existed both earlier times. It was in a benchmark report and a test comment — places nobody writing a battery is going to look. That is the actual defect this PR repairs: not the trap, which is well understood, but that finding it a fourth time was the expected outcome. ## The check this recommends is a count, not a string ```sh n=$(cargo test -q --lib "$FILTER" -- --exact --list 2>/dev/null | grep -c ': test$') [ "$n" -eq 1 ] || { echo "FILTER-DRIFT: '$FILTER' selected $n, expected 1"; exit 2; } ``` `--list` enumerates matches **without running them**, so "did the filter resolve" is separable from "what did it find". Asserting `1 passed` in the run output is the same idea and is what `agrees_with_hostlock_sh.rs` does. I measured its limits rather than assuming them: it false-alarms the moment a filter selects more than one test — a module filter over two tests prints `2 failed`, which contains neither `1 passed` nor `1 failed` — and it cannot separate a broken filter from a real failure. Fine where exactly one test is selected by construction, as in that file; not a general rule. ## Changes - **`.github/skills/measurement-discipline/SKILL.md`** — new failure mode **§9**, in the file's existing house style (incident, evidence, `**Check:**`). Also adds the vacuity-arm rule: the only arm whose expected result you know independently of the code under test, and therefore the only one that can report that the apparatus is lying. Frontmatter `source:` updated. - **`RULES.md` §8** — one bullet, cross-referencing §9. - **`docs/research/testing/00-integration-stress-design.md:150`** — this doc gave `cargo test ... -- --exact <scenario>` as *the* reproduction recipe, i.e. the fragile form, in a tracked document. Corrected to the full module path. This is `measurement-discipline`'s own "correct the record where the claim lives" applied to itself. ## Validation - Frontmatter parses (`yaml.safe_load`); failure-mode numbering verified contiguous 1..9. - Both new relative links resolved to existing files on disk. - Docs-only: verified against `ci.yml`'s own `is_docs_path()` classifier — all three paths classify docs, `docs_only=true`. - Ran the `root-files` guard from #2052 locally: 36 entries, no strays. - No code, no test, no workflow changes. ## Credit The trap in its current form is Gaff's write-up; the earlier two records are #1628's and #2026's. My contribution is the measurement of *why* it is silent (byte-identical outputs; module-nesting dependence), the falsification of the `1 passed` form's generality, and putting it where the next person will actually hit it. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
pushed a commit
that referenced
this pull request
Aug 25, 2026
The wiki section this PR adds said 'three former over-reads' and listed three. There are five, and since then two more holes of a different kind (a nested job key deletes a check rather than failing it, per #2052). A note that undercounts its own subject is the same defect the note is about: the text and the thing it describes drifted apart, and nothing red flagged it. Corrected to five, named individually, with the completeness caveat stated rather than implied; plus the vanished-job section, the Windows sole-executor fact that motivates it, and the residual the guard cannot close. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 25, 2026
#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>
This was referenced Aug 26, 2026
Workflow job-integrity gate reads only ci.yml, leaving 23 workflow files and 35 jobs unguarded
#2206
Closed
justinchuby
added a commit
that referenced
this pull request
Aug 26, 2026
Closes #2206. ## The hole #2023 added `_stray_job_attribute`, which refuses a job key indented one level too far. That is a good guard: the mis-indent is *valid YAML*, so the job silently becomes a key of the job above it and GitHub stops producing its checks rather than failing them. No pass/fail gate can see an absence. It is reached only through `workflow_jobs()`, which reads one file: ```python WORKFLOW = ROOT / ".github" / "workflows" / "ci.yml" # line 17 ``` 24 workflow files, 44 jobs. `ci.yml` holds 9. **23 files and 35 jobs were unguarded.** ## Measured before writing the fix Nesting `root-files` two spaces deeper in `diff-guard.yml` — the root-allowlist job #2052 was filed to fix — leaves valid YAML, and `yaml.safe_load` reports the jobs GitHub will run as `['deletion-ratio']`: | gate | with the root allowlist silently deleted | |---|---| | `verify` | `rc=0` — `workspace lint coverage ok: 55 tested` | | `verify-required-tier` | `rc=0` — `windows ORT coverage ok: 6 package(s)` | | `self-test` | `rc=0` — `51/51 arms behaved as stated` | `ci.yml` is not exposed to this, but not because the gate covers it — branch protection blocks on a missing required check, and `verify_required_tier` refuses when `REQUIRED_JOB_NAMES` names a job that no longer exists. Both are inventories of `ci.yml`. **A workflow file containing no required job has neither**: `diff-guard.yml`, `hostlock.yml`, `miri.yml`, `weight-cache-guard.yml`, `wiki-lint.yml`, `visualizer-test.yml`. ## Fix, in two halves that are separately necessary **1. Scope.** The scan now covers every `.github/workflows/*.yml` and `*.yaml`, while still running from `rust-quality`. Scope and host are independent choices and only the *host* has to be required. All 24 files parse clean today, so the widening costs nothing now — `_JOB_ATTRIBUTES` is the complete documented job-key set, so false-positive risk is low. **2. Inventory.** A stray-key rule fires on the *mechanism* of accidental nesting, so it is structurally blind to a job deleted outright — the file simply has fewer jobs and nothing about it is malformed: ``` CAUGHT nest root-files 2 deeper -> 'deletion-ratio' contains 'root-files', ... SURVIVED delete root-files block -> parsed as ['Deletion ratio'] ``` `WORKFLOW_JOB_INVENTORY` pins the file set and each file's job names. The maintenance cost is real and deliberate; the refusal prints the exact block to paste. ## Mutation results, judged on what each refusal names | arm | nest | delete | |---|---|---| | baseline (this PR) | CAUGHT | CAUGHT | | scope cut back to `ci.yml` | SURVIVED | SURVIVED | | inventory comparison removed | CAUGHT | SURVIVED | My **first** battery scored the scope arm as CAUGHT. It was wrong, and worth stating: cutting the scope to `ci.yml` while leaving the 24-entry inventory in place makes the gate exit 1 with 23 unrelated *"recorded but not on disk"* complaints — refusing without ever having looked at the mutant. Scored on exit status it reads as a passing arm and would have certified a property the change does not have. The table above narrows the inventory with the scope, and requires the refusal to name `root-files` / `Root file allowlist`. Self-test arms driving `_integrity_failures` are also run end-to-end through `verify_workflow_integrity` via `--simulate-deleted-job`, because the defect this gate exists for lives in *which files get opened*, and a helper handed the right dict by hand cannot see a file that was never read. ## Two anti-vacuity controls - **An exact inventory must be accepted.** A suite of refusal arms is passed perfectly by a gate that refuses everything. Mutation `_integrity_failures` → always refuse: the control arm fails. - **The file list is resolved against `os.listdir`.** `verify_workflow_integrity` derives *both* sides of its file comparison from `workflow_files()`, so a file that helper silently skipped would be absent from `on_disk` and from `parsed` alike and read as agreement — the instrument resolving against itself. Mutation `workflow_files()` → skip `diff-guard.yml`: 3 arms fail. - The success line reports counts (`24 file(s), 44 job(s)`) and the gate refuses when it read zero jobs, so `ok` can never mean "I saw nothing" — the same shape as `windows ORT coverage ok: 0 package(s) via []`. ## Validation - `verify`, `verify-required-tier`, `verify-workflow-integrity` all `rc=0` - `self-test` **59/59** (was 51/51) - `python3 -m py_compile` clean; `yaml.safe_load` confirms `ci.yml` still defines 9 jobs - 8-arm battery + the 4-arm content-judged battery above; every mutation asserted to have applied before it was scored, every restore md5-verified - Purely additive: +334 / −0 ## Residual, stated rather than closed Nesting `rust-quality` itself deletes the gate that would catch it. That case is covered by branch protection, because a required check that stops reporting blocks the merge — which is exactly why the gate is *hosted* in a required lane even though its *scope* is now every file. A ruleset edit that drops a required check remains unobservable from inside the repo, as `REQUIRED_JOB_NAMES` already documents. The inventory will conflict on any PR that adds or renames a job. That is the price of seeing deletions; resolve by running `verify-workflow-integrity` and pasting the block it prints, never by choosing a side. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 27, 2026
…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>
This was referenced Aug 27, 2026
justinchuby
pushed a commit
that referenced
this pull request
Aug 27, 2026
`.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>
justinchuby
added a commit
that referenced
this pull request
Aug 27, 2026
) ## 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
A stray directory has no such path.
.commitmsg/m.txtcontains a separator, so it is discarded as "not at the root" — and the thing that is at the root,.commitmsg, never appears inls-filesoutput at all. The guard doesn't fail to complain; it affirmatively reports a clean root.Not hypothetical:
.commitmsg/reachedmainasfaedea4d1, removed three minutes later by490b846c3"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:
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:
main.msg.txt39675330b→bbc193117.commitmsg/faedea4d1→490b846c3.goldens/faedea4d1→490b846c3.wa64.log83a51bfa6→c07acaa78.commitmsge42fa9470(#1881) →398cff8e5(#1999).pris_v4.log589d48ffd(#1951) →7a6482c83(#1975).body.md79196f89d(#2026) →54625db9d(#2036)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), andabresults— 131h onmain, the longest of any, but added by adocs(benchmarks): record the … resultcommit that says it is recording a result. Intentional-when-added is the line;.wa64.logrode in on atest(cpu):commit that never mentions it..wa64.logstill matters beyond the count: it predates.pris_v4.logby two days, so/*.login #1975 was reactive to the second log incident.No authorship attributed — squash-merge rewrites
%anto 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.
Root is exactly the 36 allowlisted entr(ies).(directory)Root is exactly the 16 allowlisted file(s)..goldens/, the second directory instance(symlink)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
git diff --cached --name-only, which renders non-ASCII as"caf\303\251.txt"while the guard usescore.quotePath=false. It reported the non-ASCII arm VACUOUS against a setup that had worked. Opus caught exactly this in chore: enumerate what belongs at the repository root #2036's guard; it reappeared in the thing measuring the guard.Also fixed, from review
command 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 advisedgit rm -r --cached.-Ltested first.Scope
CI-config only —
.github/workflows/diff-guard.ymland.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.