Skip to content

hostmon: a host that cannot take the lock must not read as an idle one - #2026

Merged
justinchuby merged 2 commits into
mainfrom
squad/leon-hostmon-unusable
Aug 24, 2026
Merged

justinchuby merged 2 commits into
mainfrom
squad/leon-hostmon-unusable

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

What

onnx_runtime_hostmon::hostlock::read reported Free on a host where the lock directory does not exist and cannot be created. Every benchmark row taken there was stamped host_lock=free.

That is a fail-open, and it is the same one #1989 fixed in hostlock.sh — reintroduced one layer down, where it is the more durable half:

on an unusable host, before this PR
hostlock.sh UNUSABLE, and acquire/run refuse with exit 7
hostlock::read Free → host_lock=free on every emitted row

The script at least stops. A false free is a claim about conditions that outlives the run: it says "nobody had declared the machine", when the true statement is "nobody could have — including whoever was running beside this measurement". Weeks later it reads as reassurance.

Closes #1997.

Measured, on origin/main

The reader resolves the lock dir correctly (#1942) and then opens <dir>/meta. Path resolution succeeds into a mode-500 parent (it has x), finds no hl entry, and returns ENOENT — which classify_io maps to Free. So the arm that produces the wrong answer is the one whose comment says "Absent is a measurement: nobody has taken the lock." It is a measurement only when absence was a choice.

What changed

  • LockState::Unusable / LockField::Unusable, printed unusable. Never is_protected(). Unusable vs Free across the window is changed — a host that becomes usable mid-run did change custody in the only sense that matters.
  • dir_problem ports lock_dir_problem from the script, including the two things that were got wrong there first and would have been got wrong again here:
    • it asks the nearest existing ancestor, not the lock dir, because publishing stages at a sibling and mv -Ts it into place;
    • it requires W_OK and X_OK. A mode-0600 directory is writable and still cannot hold an entry (mkdir 0600-parent/sub → EACCES). A -w-only check passes a host on which nothing can be created.
  • It calls access(2) rather than reading st_mode, because the script's test -w resolves to faccessat. Reimplementing the rule from mode bits would disagree with the writer on uid, supplementary groups, ACLs and read-only mounts — on precisely the hosts where the answer is interesting.
  • state_at keeps the script's ordering: an existing lock directory outranks any question about writability. Reporting unusable for a directory a peer has already published into would relabel real contention as a broken config, and send an agent off to fix its own machine while somebody else's benchmark runs. (This is the mutant that survived the first battery on the shell side.)
  • scripts/ort_ab/README.md gains the unusable row in the host_lock= value table.

Anti-vacuity

Six mutations, each killed by at least one named cell:

mutation killed by
absence is always Free a_host_that_cannot_take_the_lock_reads_unusable_in_both_implementations
W_OK only, no X_OK same (the mode-0600 cell)
dir problem outranks a published holder a_published_lock_outranks_a_host_that_could_not_have_created_it
"exists and is not a directory" arm removed the plain-file cell
field maps Unusable → Free an_unusable_host_does_not_print_as_a_free_one
Display prints unusable as free same

The differential test compares against the script's real porcelain (state=UNUSABLE, state=FREE, state=HELD), not against my expectation of it — a reader that agreed with a comment and not with the writer is the failure that whole file exists to prevent. It carries a creatable-path control, so a guard that degenerated into a blanket refusal cannot pass, and it asserts the acquire refusal alongside the state.

Validation

  • cargo test -p onnx-runtime-hostmon — 29 + 10 + 31 + 1 + 2 pass, 0 fail.
  • cargo clippy -p onnx-runtime-hostmon --all-targets clean; cargo fmt --check clean.
  • scripts/hostlock_test.sh — 341/341, unchanged (the script is not touched).
  • No benchmark was run for this PR, so it needed no host lock and took none.

Scope: one crate plus one README row. No EP, kernel or scheduling behaviour.

…le one

`hostlock::read` mapped `ENOENT` on the metadata file to `LockState::Free`,
so on a host where the lock directory does not exist *and cannot be created*
-- an unwritable or unsearchable parent, or a plain file sitting at the lock
path -- every benchmark row was stamped `host_lock=free`. `free` is a claim
about the host: "nobody had declared the machine". The true statement is
"nobody could have, including whoever was running beside this measurement".

`hostlock.sh` grew an `UNUSABLE` state and exit 7 for exactly this case
(#1989), and the reader kept the fail-open one layer down, where it is the
more durable of the two: the script at least stops, while a false `free`
persists in the emitted row and reads as reassurance weeks later.

- `LockState::Unusable` / `LockField::Unusable`, printed as `unusable`,
  never `is_protected`, and `Changed` against `Free` -- a host that becomes
  usable mid-window did change custody in the only sense that matters.
- `dir_problem` ports `lock_dir_problem` from the script, including asking
  the *nearest existing ancestor* (publishing stages at a sibling and
  `mv -T`s, so that is the directory that must accept entries) and requiring
  `W_OK` **and** `X_OK` -- a mode-0600 parent is writable and still cannot
  hold an entry. It uses `access(2)` because the script's `test -w` does:
  reimplementing it from `st_mode` would disagree with the writer on uid,
  supplementary groups, ACLs and read-only mounts, i.e. on precisely the
  hosts where the answer is interesting.
- `state_at` keeps the script's ordering: an existing lock directory wins
  over any question about writability. Answering `unusable` for a directory
  a peer has already published into would relabel live contention as a
  broken config and send an agent off to fix its own machine while somebody
  else's benchmark runs.

The differential test asserts both sides against the script's real porcelain
rather than against my expectation of it, with a creatable-path control so a
blanket refusal cannot pass, and the acquire refusal asserted alongside the
state. Six mutations -- absence always free, `W_OK`-only, problem-outranks-
holder, the not-a-directory arm removed, and both launderings of the field
into `free` -- are each killed by at least one cell.

Closes #1997.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.46%. Comparing base (0a87860) to head (f132259).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
crates/onnx-runtime-hostmon/src/hostlock.rs 92.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2026      +/-   ##
==========================================
+ Coverage   80.28%   80.46%   +0.18%     
==========================================
  Files         409      425      +16     
  Lines      189627   208841   +19214     
  Branches   189627   208841   +19214     
==========================================
+ Hits       152234   168049   +15815     
- Misses      31970    35135    +3165     
- Partials     5423     5657     +234     
Flag Coverage Δ
cli-ort-linux 72.51% <ø> (?)
cli-ort-windows 72.01% <ø> (?)
mlas 85.10% <ø> (?)
offline 80.60% <92.00%> (+0.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
crates/onnx-runtime-hostmon/src/hostlock.rs 92.30% <92.00%> (-0.73%) ⬇️

... and 79 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.

…fixtures

Review found a seventh mutation the battery missed: deleting the
`probe.is_dir()` arm of `dir_problem` left every cell green. Every fixture
put a real directory above the lock, or put the non-directory at the *leaf*,
where the earlier check catches it and returns before the ancestor walk ever
runs -- so the branch that ports the script's `[ ! -d "$p" ]` was untested.
With it gone, a lock under an *executable* regular file falls through to
`access(W_OK|X_OK)`, which returns 0 on a file, and the reader answers Free
where the script answers UNUSABLE. The execute bit is what makes the fixture
load-bearing: a mode-644 ancestor is caught by `X_OK` failing, so it would
have pinned nothing.

Also stops the tests stranding fixtures. They chmod directories to 0500/0600
and rely on `Drop` to restore them, which covers panics and not kills; a
SIGKILL leaves a *non-empty* mode-500 directory, and `remove_dir_all` fails
on it with EACCES. That error was discarded, so the next run inherited the
fixture and failed somewhere unrelated -- a stale artifact wearing the
costume of a regression. `lock_dir` and `ScratchLock` now restore modes
before removing, and the recovery has a cell that first asserts the bare
removal really does fail, so it cannot quietly stop pinning anything.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby

Copy link
Copy Markdown
Owner Author

Both items landed in f1322597a.

SHOULD FIX — the seventh mutant. Confirmed and fixed, and your diagnosis of why it survived is the part worth recording: every fixture either put a real directory above the lock, or put the non-directory at the leaf, where dir.exists() && !dir.is_dir() catches it and returns before the ancestor walk runs at all. So the arm that ports the script's [ ! -d "$p" ] had no input that reached it. The new cell puts a regular file at an intermediate ancestor and — the load-bearing detail — chmod 0700s it, because a mode-644 file is caught by X_OK failing and would have pinned nothing. Verified by mutation: with the block deleted the new cell fails; restored, it passes.

That makes two branches in this design that were deletable-while-green, both found by review rather than by me, and both the same shape: the guard's early return hid the branch behind it. The -x half of the permission check in #1989 was the first. I have added "for each return in a guard, name the input that reaches the code after it" to how I build these batteries.

NIT — the stranded fixture. Taken, and it is more than cosmetic: the two suites here run in the same CARGO_TARGET_TMPDIR, so one stranded non-empty mode-500 directory poisons subsequent runs and surfaces as acquire failed in a test that has nothing to do with permissions. lock_dir and ScratchLock::drop now restore_modes before removing. The recovery has its own cell, and that cell first asserts the bare remove_dir_all really does fail — otherwise, the day the hard case stops being hard, it would keep passing while pinning nothing, which is the failure mode this file exists to avoid.

Cleared items noted, especially the two I would not have checked myself: that both sides use real-uid access and therefore share the CAP_DAC_OVERRIDE blind spot identically (agreement is what matters here, not omniscience), and the LOCK_DIR == "/" divergence — real, unreachable, and now the only one known.

Validation on f1322597a: cargo test -p onnx-runtime-hostmon 29 + 11 + 31 + 1 + 2, 0 failed; clippy --all-targets and fmt --check clean; scripts/hostlock_test.sh 341/341 (script untouched). Mutation battery re-run: M2, M3, M7, M8 all killed, no survivors. No benchmark was run, so no host lock was taken.

@justinchuby
justinchuby merged commit 79196f8 into main Aug 24, 2026
17 checks passed
@justinchuby
justinchuby deleted the squad/leon-hostmon-unusable branch August 24, 2026 21:05
justinchuby added a commit that referenced this pull request Aug 24, 2026
## 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.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 25, 2026
…sm (#2038)

`.body.md` at the repository root is my PR description for #2026. It
reached `main` in 79196f8 because I wrote it into the worktree root
(`/tmp` is off-limits here) and staged with `git add -A` — the dotfile
sat above ten test files in `git status` and I read past it.

Harmless in the sense that nothing reads it; not harmless in the sense
that the next person to `ls -a` has to establish that.

Two changes:

- `git rm .body.md`.
- `.gitignore` gains `.leon_*` for anything new, plus the specific names
already scattered across my worktrees (`.body.md`, `.resp*.md`,
`.prbody*.md`, `.merge.log`, `.mutate*`, `.suite.log`, `.selftest.log`),
so an aborted run cannot leave something stageable behind.

There is precedent immediately above it in the same file: Roy added
`.rg_*` / `.rv_*` / `.roy_*` and then `roy_validate.log` separately,
with the comment *"the summary did not [match `.rv_*`], so it was easy
to commit by accident with `add -A`"*. Same defect, same fix.

Verified `git check-ignore` matches `.leon_x`, `.merge.log` and
`.resp2.md`, and that no tracked file in the tree is caught by the new
patterns.

No `--admin`, no bypass — `merge_when_green.sh` will wait for the
required contexts.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 25, 2026
…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>
justinchuby added a commit that referenced this pull request Aug 25, 2026
…tion

Independent review returned REQUEST CHANGES on five findings. All five were
reproduced before being fixed; two of them are defects in the section's own
reasoning rather than typos.

Citations (both MAJOR). The hostmon guard was cited as #2026 and the
benchmark harness-bug record as #1628. Both wrong, and wrong by the same
mechanism: I used `git log -1 -- <file>`, which resolves to the file's most
recent commit, not to the commit that introduced the line. `git log -S` on
the exact text gives #1950 for the guard (a7003c9) and #1619 for the
record (fb38341); #2026 does not touch the guard at all (0 hunks). For a
section whose entire value is that the prior records were unfindable, wrong
pointers are the defect itself, so this is now a **Check:** in the text.

The `1 passed` claim was backwards (MINOR). I wrote that the substring
check false-alarms when a filter selects more than one test -- the safe
direction. Measured, it fails in the dangerous one:

  11 passing tests -> "11 passed; 0 failed" contains "1 passed" -> accepted
  2 selected, 1 failing -> "1 passed; 1 failed"  contains "1 passed" -> accepted

Both are false greens; the second accepts a run holding a real failure.
The section now states this, explains why the check is nonetheless sound in
agrees_with_hostlock_sh.rs (window_probe_child is a crate-root `#[test]`,
so exactly one test is selected by construction), and draws the conclusion
the measurements actually support: the two checks compose. The listing pins
the selection to one, which is what makes reading the run output sound
afterwards. Neither half is sufficient alone.

`--list` counts `#[ignore]`d tests (MINOR). `tests::an_ignored_test` lists
with the same `: test` suffix and resolves to n=1 while the run executes
nothing (`0 passed; 0 failed; 1 ignored`). The listing proves the name
resolves, never that the arm ran -- which is why the run-output half is not
optional. Noted in the text. My first probe of this claim returned n=0 and
appeared to refute it; the probe had used a bare name and selected nothing,
i.e. it fell into the trap being documented, so the refutation was the
apparatus failing rather than the claim.

Design doc (NIT): "a bare name matches nothing" is false for a crate-root
test, the asymmetry §9 is careful about. Qualifier restored.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 added a commit that referenced this pull request Aug 25, 2026
…t let it

#2150 (b054ff6) carried .roy-scratch/pr.md into main. That file is the
working copy of #2150's own pull-request description -- a build artefact of
writing the PR, not repo content.

The cause is a near-miss in .gitignore: my scratch patterns are `.roy_*`
(underscore) and I named the directory `.roy-scratch` (hyphen), so `add -A`
swept it in. Widening the pattern to `.roy-*` covers both spellings.

This is the third time a PR-authoring scratch file has reached main by this
exact route, and the second in my own namespace:

  roy_validate.log     added 74756f0, removed 9e32e2d
                       ("drop the accidentally committed validation transcript")
  .body.md             reached main in #2026 (79196f8), Leon's namespace
  .roy-scratch/pr.md   this one

Both prior cases are already memorialised as comments in .gitignore, and
Leon's names the mechanism: `git add -A` "does not distinguish a dotfile at
the root from repo content, and a name like `.body.md` looks plausible enough
in `git status` to skim past." That matches why this one also survived two
reviews -- a stray *added* file reads as intentional in a diff, because a
reviewer checks whether changed lines are correct and an unfamiliar new path
is not a changed line.

I caught it from `rm -rf .roy-scratch` reporting the file as tracked-deleted
rather than untracked -- a cleanup step, not a review step. The check that
would have caught it is confirming the PR's file list is the set of files I
meant to change, which costs one command and is now in my pre-merge sequence.

I found this while #2150's checks were still running and pushed a fix to that
branch, but auto-merge fired on the previous head first. Nothing was bypassed;
the race is between a push and an armed auto-merge, which is why this is a
follow-up rather than an amendment.

No production code, tests, or documentation are touched.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 25, 2026
…t let it (#2156)

Removes `.roy-scratch/pr.md` from `main` and widens the `.gitignore`
pattern that failed to catch it.

**No production code, tests, or documentation are touched.** One stray
file deleted, three lines added to `.gitignore`.

## What happened

#2150 (`b054ff63e`) carried `.roy-scratch/pr.md` into the tree. That
file is the working copy of #2150's own PR description — an artefact of
writing the pull request, not repo content.

## Why it got in

My scratch ignore patterns are `.roy_*` — underscore. I named the
directory `.roy-scratch` — hyphen. `.roy_*` does not match
`.roy-scratch/`, so `git add -A` swept it in. The fix widens the pattern
to `.roy-*` so both spellings are covered.

## This is the third time, not the first

I originally wrote this up as the second instance of one failure mode.
Reviewing my own claim against the history, it is the **third**
repo-wide, and the second in my own namespace — and both priors are
already memorialised as comments in the very file I am editing:

| file | fate |
|---|---|
| `roy_validate.log` | added `74756f0d9`, removed `9e32e2d84` — *"drop
the accidentally committed validation transcript"* |
| `.body.md` | reached main in #2026 (`79196f89d`), Leon's namespace |
| `.roy-scratch/pr.md` | this one |

That changes the character of the fix. A one-off argues for deleting a
file; three occurrences across two namespaces argue that the ignore
patterns are the wrong shape — they enumerate *known* scratch names
instead of reserving a namespace, so every new scratch filename is a
fresh chance to miss.

Leon's comment already names the mechanism precisely:

> `git add -A` does not distinguish a dotfile at the root from repo
content, and a name like `.body.md` looks plausible enough in `git
status` to skim past.

## Why it survived two reviews

Worth stating rather than quietly deleting the file. A stray **added**
file reads as intentional in a diff: a reviewer assesses whether changed
lines are correct, and an unfamiliar new path is not a changed line — it
looks like a deliberate addition whose contents happen to be prose.
Leon's note is the same observation from an independent author, which
suggests it is a property of the review surface rather than of any one
reviewer. I missed it too.

I caught it from `rm -rf .roy-scratch` reporting the file as
tracked-deleted (` D`) rather than untracked — a *cleanup* step, not a
review step. The check that would have caught it:

```
gh api repos/OWNER/REPO/pulls/N/files --jq '.[].filename'
```

One command, confirming the PR's file list is the set of files I meant
to change. It is now part of my pre-merge sequence, and I ran it on this
PR before requesting review.

## Why a follow-up and not an amendment

I found this while #2150's required checks were still running and pushed
the fix to that branch — but auto-merge fired on the previous head
first, so the correction missed by about two minutes.

Nothing was bypassed: `--squash --auto` waited for `Fast (Linux x86_64)`
and `Rust quality` as required. The hazard is narrower and worth
recording — **once auto-merge is armed, the head can merge at any
moment, so the branch is no longer a safe place to stage a correction.**
Staging a fix there is a race against your own merge. A follow-up PR is
the only reliable move.

## Review

Opus review returned no blocking issues, and confirmed adversarially
that:

- `.roy-*` is **not** too broad. No tracked path has a `.roy-`
component. The nearest miss,
`.squad/decisions/inbox/roy-session-kv-cache.md`, has no leading dot and
is unmatched — verified with `git check-ignore`.
- Leaving the pattern unanchored is consistent with its neighbours
(`.rg_*`, `.rv_*`, `.roy_*`, all unanchored). The root-anchored block
later in the file is anchored for a stated reason that does not
transfer: those names are ordinary nouns a nested crate might want,
whereas `.roy-` is a namespace prefix.
- Nothing in the repo references the deleted path — the only occurrence
of `roy-scratch` after this change is the new `.gitignore` comment.

The review also caught a factual error in my first write-up: I said the
`roy_validate.log` comment sits three lines *above* the `.roy_*`
patterns. It sits one line *below* them. Corrected here and in the
commit message. Fitting, in a PR about a claim that no one checked.

## Verification

- `git ls-tree -r --name-only origin/main | grep roy-scratch` → present
before, absent after.
- Ignore fix confirmed live rather than assumed: this PR's own scratch
directory is on disk right now and `git check-ignore -v
.roy-scratch/body.md` attributes it to the new `.roy-*` rule, with `git
status --porcelain` clean.
- Diff against `main` is exactly the two files above.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hostmon reader still reports free for a lock dir that cannot exist, now that hostlock.sh says UNUSABLE

1 participant