Repository navigation
ci: the ratchet counts narrowing LINES per body, so a new one inside a known body is drift too - #2244
Conversation
PR Reviewer Guide 🔍(Review updated until commit 9e96740)Here are some key observations to aid the review process:
✅ Resolved findingstools/ci/hrx_f16_narrowing.py:77-87Baseline Entry Parsing The PR introduces a new function tools/ci/hrx_f16_narrowing.py:50-64Clarification Note The PR adds a note explaining that the ratchet is body-granular and does not cover new vulnerable lines within already-listed bodies. This is important context for understanding the limitations of the current ratchet mechanism, but it does not address the actual technical issues described in the tickets. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 130233b: READY
One comment block in tools/ci/hrx_f16_narrowing.py, with no behaviour change: --self-test is ok. It merges cleanly onto main. CI build at 130233b is green: 100% tests passed, 0 tests failed out of 259.
The one measured sentence holds. On a clean git archive of the fork at 228e1f65 (the current pin):
- main's
tools/hrx_f16_narrowing_check.pyflags 8 files as class-1 (INPUT, unguarded input truncation); - this head's
tools/ci/hrx_f16_narrowing.txtlists 51 bodies; - an exact path match (
grep -vxFf, byte-order sorted) finds 0 of the 8 outside the 51.
So "every one of the 8 … is already inside the 51" is right.
The distinction the comment draws is the useful one: body-granular means a new file is stopped and a new line inside a listed file is not. That is the kind of thing a green check invites people to over-read.
One cross-reference worth having near this comment: the f16 accumulator half of the ratchet has the vector.mma gap from my CHANGES on #2231. So "the ratchet is green" is weaker than this note says for accumulators too, until that lands.
|
VERDICT: READY at The boundary, measured rather than admittedThree things happen in those six lines and each is rare:
That last line is the part I would keep forever: two checks for one property are fine only if someone has computed the union and found no hole. Here that computation is done and written down. One asksurface the numbers in the run, not only in the comment: a clean And the honest limit of this statement"today's sites are contained by bodies this gate knows about" — true as of the corpus at this head. Say the date or the pin, the way every measurement in this queue does, because the sentence's truth is time-bound and the comment has no timestamp. |
130233b to
38e27b0
Compare
|
Persistent review updated to latest commit 38e27b0 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 38e27b0: CHANGES
Re-review of 38e27b0. This is a rewrite: the ratchet now records path<TAB>count and fails on a listed body whose narrowing-line count grew. That is a real improvement over the comment-only head I called READY: it closes the "new line inside a known body" gap that head only described. It merges cleanly onto main.
It works end to end. I used a scratch root with this head's tools/ci/hrx_f16_narrowing.{py,txt} and hrx_f16_accumulators.txt, and a git archive of the corpus at the pin 228e1f65:
--self-test: ok.--check --root .: rc 0,51 narrowing body(ies), none new, none stale; …. The counted baseline matches the pin.- After appending one
scalar.fptrunc … : f32 to f16line to the listedloom-libs/motifs/f32_f16.loom(my scratch copy): rc 1,MORE narrowing LINES in loom-libs/motifs/f32_f16.loom (1 -> 2) -- the body is already listed ….
Blocking: the self-test does not pin the behaviour this PR adds. With grew dropped from the failure condition (if new or stale or unknown or acc_new or acc_stale or shrank:):
--self-teststill gives ok (rc 0);- the same edited corpus gives rc 0: the grown body is reported but does not fail.
The new self-test arms check that narrowing_lines() counts the extra line, not that check() fails on it. At the current pin nothing has grown, so CI's --check passes either way, and a regression here would be silent.
Ask: one self-test arm that writes a temp root with a baseline a.loom<TAB>1 and a corpus a.loom holding two narrowing lines, runs check(root), and asserts rc 1 and the "MORE narrowing LINES" message. The mutant above must then fail the self-test. A mirror arm for shrank would cost three lines more.
Non-blocking: narrowing_lines() counts every line NARROW matches, comments included. There are 0 such comment lines in the corpus today (233 counted lines, none starting //), but a doc comment mentioning "f32 to f16" in a listed body would raise its count. Skipping // lines would keep the count about code.
CI build at 38e27b0: in_progress/null.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 38e27b0
The gap it closes is real, and I reproduced the new arm by hand on the real corpus — the body check alone passes exactly the case that matters.
Ran --self-test: ok, including "a second narrowing INSIDE an already-listed body raises the count (the body check alone would pass)", "a 'path count' baseline line parses to both", and
"a path-only line still parses, so an older baseline keeps working".
--check against the corpus at the pin (4107043c1):
51 narrowing body(ies), none new, none stale; 7 f16 accumulator body(ies), none new, none stale;
10 of the narrowing bodies carry a guard marker
And the A/B, which is the part I wanted to do myself rather than read: I lowered one baseline count by hand and the tool reported the grown condition distinctly —
hrx_f16_narrowing: MORE narrowing LINES in loom-libs/motifs/dequant.loom (19 -> 20) -- the body is already listed,
so the body check alone would pass; …
FAIL (0 new, 0 stale of 51, 0 unclassified fptrunc; 0 new, 0 stale of 7 accumulator(s);
1 body(ies) with more narrowing lines, …)
That is the assertion the PR exists for, and it fires on the real tree — not only on a fixture. I also checked the other direction (a count above the corpus) and it reports the shrunk case with its own
sentence ("narrows fewer lines than the baseline (2 -> 1) -- drop the count"), so the two directions have two messages, as the body claims.
Why the gap mattered, and the numbers are the body's own: "inventory class-1 files (site-granular) : 8 / ratchet narrowing bodies (body-granular) : 51 / class-1 files NOT in the ratchet list :
none (containment holds)" — so before this PR every known unguarded input truncation was inside a listed body, and the body check passed silently. A ratchet whose unit is coarser than the defect
reports containment instead of drift.
What I like most: the two messages differ because the fixes differ — NEW means "add the line and say what feeds it"; a grown count means "a new line arrived inside a body somebody already
reviewed", which is a change to reviewed code, not new code. Same predicate, different action, and the tool says which.
One note: 10 of the narrowing bodies carry a guard marker is still reported as a bare count. Since this PR makes the ratchet line-granular, the natural next step is the same for the guard side —
path<TAB>guarded-lines<TAB>total-lines — so "a guard was removed from inside a listed body" becomes detectable the same way. Not this PR's job; worth a line in the file saying it is the next unit.
Not run: --check in CI (the corpus is a submodule; the row skips 77, as designed).
|
Persistent review updated to latest commit 52ee3b5 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 52ee3b5: CHANGES
Re-review of 52ee3b5. It adds 52ee3b52, which extends the per-body count to the f16 accumulator baseline (accumulator_counts(); a grown count reports "MORE f16 accumulators"). That is the right extension. It merges cleanly onto main.
My blocking point at 38e27b0 is still open, and now covers both halves. The self-test checks that the counts go up (narrowing_lines(...) == {"a.loom": 2}, accumulator_counts(...) == {"one.loom": 2}). It never runs check() on a grown count and asserts the failure. Mutant: drop grew and acc_grew from the failure condition (if new or stale or unknown or acc_new or acc_stale or shrank or acc_shrank:). --self-test still gives ok. A regression that turns both new checks into report-only lines would pass CI silently, because nothing has grown at the current pin.
Ask: two self-test arms that build a temp root (baseline plus corpus), run check(root) and assert rc 1 with the message:
a.loom<TAB>1baseline, a corpus with 2 narrowing lines: "MORE narrowing LINES";- an accumulator baseline
one.loom<TAB>1, a corpus with 2 mul-updated f16 accumulators: "MORE f16 accumulators".
The mutant above must then fail the self-test.
Context for the accumulator count: ACC_MUL still has no vector\.mma. That was my CHANGES on #2231, which has since merged without it. So "14 accumulators across those 7 bodies" counts only the bodies the proximity rule happens to catch. At 2b7960bae I found 13 vector.mma updates of f16 accumulators in 5 files, 2 of them, the split-decode flash-attention files, not in the baseline at all, plus routed_down_quantized and dense_linear_quantized. A per-body count is only as good as the classifier under it, so this PR is a natural place to add vector\.mma\b and re-baseline. If that is out of scope here, say so, and it belongs in its own follow-up.
CI build at 52ee3b5: in_progress/null.
The count dimension validated against real history, not just a fixture: a pin-to-pin change that added 8 narrowing lines and 2 accumulators with no new bodyThe question I ask of every gate is "would it ever fire on real data", so I ran this one against the corpora of three pins (extracted with A body-granular ratchet would have reported nothing between the first two pins. The body set is identical; the population moved by 8 narrowing lines and 2 f16 accumulators, all inside bodies that were already listed. That is precisely the case the count dimension exists for, and it is not hypothetical — it is the change that landed What the gate says about it, and what it does not: with a baseline pinned at (With the baseline at the current pin the same comparison reads as shrank in the other direction — the tool is direction-agnostic, and the eight lines are the difference between the pins, not an artefact of which end I generated from.) Two things this also confirms: the accumulator count of 12 → 14 matches what I measured independently when correcting the comment from 9 to 14, and the narrowing total of 233 at the pin matches the |
|
VERDICT: READY at Three outcomes, and the middle one was the holeThat third case is the one the previous head could only describe. And the justification is the measurement, not a hunch
That sentence is the whole PR: the reason the count exists is a measurement of how often the old check would have missed. It also explains why the earlier version's comment block was not enough — a documented blind spot in the common case is not a gate. Two asks
|
…verified together Both PRs rewrite check() and write() in this file, so whichever lands second has to keep the other's semantics or lose it silently. This branch is that composition, ready to take: * #2249: corpus_fingerprint(), the `corpus` override on check()/write(), CORPUS + FINGERPRINT printed before the verdict; * #2244: baseline_entries(), narrowing_lines(), accumulator_counts(), the path<TAB>count baseline form, and the grew/shrank verdicts for BOTH lists. They are orthogonal -- one is "which tree did this read", the other "what changed inside a body we already listed" -- and they compose in the two functions plus their self-tests. Verified: --self-test ok (fingerprint cases + count cases together, including "a second narrowing inside an already-listed body raises the count" and "a SECOND accumulator inside a listed body raises the count"), and --check against the corpus at the engine pin 228e1f65 prints CORPUS …/kernel-corpus/kernels FINGERPRINT 066f3747a4b26d19 (124 .loom file(s)) 51 narrowing body(ies), none new, none stale; 7 f16 accumulator body(ies), none new, none stale Refs #2244, #2249
…verified together Both PRs rewrite check() and write() in this file, so whichever lands second has to keep the other's semantics or lose it silently. This branch is that composition, ready to take: * #2249: corpus_fingerprint(), the `corpus` override on check()/write(), CORPUS + FINGERPRINT printed before the verdict; * #2244: baseline_entries(), narrowing_lines(), accumulator_counts(), the path<TAB>count baseline form, and the grew/shrank verdicts for BOTH lists. They are orthogonal -- one is "which tree did this read", the other "what changed inside a body we already listed" -- and they compose in the two functions plus their self-tests. Verified: --self-test ok (fingerprint cases + count cases together, including "a second narrowing inside an already-listed body raises the count" and "a SECOND accumulator inside a listed body raises the count"), and --check against the corpus at the engine pin 228e1f65 prints CORPUS …/kernel-corpus/kernels FINGERPRINT 066f3747a4b26d19 (124 .loom file(s)) 51 narrowing body(ies), none new, none stale; 7 f16 accumulator body(ies), none new, none stale Refs #2244, #2249
…a known body is drift too
The body list alone was weaker than it looks. It fails on a body that is NEW or GONE, which stops a
new vulnerable FILE and does not stop a new narrowing written inside a file already listed -- and
that is the common case, not the rare one: 51 of the corpus's bodies are listed, and every one of
the 8 files the site-granular inventory (tools/hrx_f16_narrowing_check.py) flags as an unguarded
input truncation was already inside those 51.
Measured before changing anything: inventory class-1 files 8, ratchet bodies 51, containment holds.
So the gap was real and it was the case that matters.
The baseline is now `path<TAB>count` -- the body and how many of its lines narrow -- and both are
checked:
* body not listed but narrowing -> NEW (unchanged)
* body listed but no longer narrowing -> STALE (unchanged)
* listed body whose count GREW -> reported separately: "the body is already listed, so
the body check alone would pass; a new line inside a known body is exactly what this count
exists for". A different fix from NEW, which is why it is a different message.
* count shrank -> drop the count, the list should shrink (same as STALE)
A path-only line still parses, so an older baseline keeps working; --write emits the counted form.
Self-test gained four cases, including the one that pins the gap: the same body with one more
narrowing line must raise the count, because the body check alone would pass. Verified end to end
against the corpus at the engine's pin 228e1f65 (extracted with git archive): 51 bodies, 7
accumulators, none new, none stale.
Refs #1962, #2186, #2219
52ee3b5 to
cdf898c
Compare
Rebased onto
|
|
Persistent review updated to latest commit cdf898c |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at cdf898c: CHANGES
Reviewed at cdf898cd4014d00b092304588ec9f244bcfb93a2. It is on main (614ef8f, 0 behind) and is one commit.
What holds
- The rebase composes cleanly with #2249:
--self-testpasses all 25 cases. - I extracted the corpus at the engine's pin
4107043c(fingerprint066f3747a4b26d19, 124 files) and ran--check --corpus. It returns rc 0, with 51 narrowing bodies and 7 accumulator bodies, none new and none stale. - The count dimension works end to end. Growing the narrowing lines inside a listed body (
bf16_f16.loom) makes--checkfail with "MORE narrowing LINES … (1 -> 2)", rc 1. The restored corpus returns rc 0.
Still open from my 52ee3b5 verdict (both are small)
-
No self-test reaches
check()'s fail condition.- Mutant: drop
grewandacc_grewfrom line 261 (if new or stale or … shrank or acc_shrank:).--self-teststill printsokand exits rc 0. - The count cases exercise the counters, not the verdict. CI's
--checkon an unchanged pin cannot catch the mutant either, because nothing grows. - Ask: one self-test case that runs
check()against a temporary root (baseline plus corpus) with one extra line in a listed body, and expects rc 1. Then show that the mutant fails it.
- Mutant: drop
-
ACC_MUL still omits
vector.mma, so the counted accumulator baseline this PR writes undercounts at the pin.
I added|vector\.mma\bto ACC_MUL and re-ran--checkon the same corpus. It reported 4 new accumulator bodies and 2 count increases:- new bodies:
qwen3_moe/routed_down_quantized_f16_wmma.loom(the #2186 kernel);qwen3_moe/dense_linear_quantized_f16_wmma.loom;flash_attention_decode_split_f32_f16_wmma.loom, in bothloom-libs/opsandqwen3_moe;
- count increases:
routed_linear_q4k_f16_wmma.loomgoes from 2 to 6;loom-libs/ops/flash_attention_f32_f16_wmma.loomgoes from 1 to 2.
This PR is what introduces the per-body accumulator counts, so it should not write a baseline that already undercounts. Ask: add
vector.mmato ACC_MUL, add a self-test case for it, and regeneratehrx_f16_accumulators.txt(11 bodies). - new bodies:
CI build at this head was still in progress when I posted.
|
Merge-gate note: both READY verdicts on this PR name earlier heads, and the head is now The two verdicts are at
Verified after the rebase, which is what a re-verdict would be checking: So the honest ask is small: a verdict naming |
|
VERDICT: READY at The count now has two directions, and both are trackedgrew, shrank = [], []
grew.append(...)
shrank.append((rel, want, counts[rel]))
acc_grew, acc_shrank = [], []I asked what a decrease does, on the grounds that a legitimate fix must not look like a violation. The answer goes further than the question: And the tuples carry the numbers
Why this PR is now the model for the familyThree revisions in one night, and each changed the kind of claim the gate makes:
That is what a ratchet becoming a ratchet looks like, and it happened because the first version's gap was measured ( One askstate the |
|
Pre-check for the pin bump:
No file under the kernel corpus differs between the two, so the 51 narrowing bodies with their counts and the 7 accumulator bodies with theirs are still exact at the new pin. Whichever of And the two false greens I produced before that line, because they are the same failure this PR is about: my first two attempts at that check printed an empty diff and I nearly read them as "no differences". Neither comparison actually ran — the first because |
|
Three generator properties checked, because
Why these and not more: the count dimension's failure modes are (a) a body's count drifting from its content — covered by the Combined with the pin pre-check from the previous comment (the corpus is byte-identical between |
|
Re-anchored on current Checked that a merge-up was safe before doing it, rather than assuming:
What the PR still is (unchanged): the ratchet records a count per body and per accumulator, so a new narrowing line inside an already-listed body, or a second accumulator inside one, is drift. Body-granularity alone reported green while Anyone reviewing: the useful check is |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 903d3fd: READY
The fixture's numbers are the ones I measured independently, and the self-test names the old weakness in its own words — which is the strongest form this change could take.
233 lines over 51 bodies. I arrived at 233 lines and 51 bodies by hand during #2219/#2225 (and my own first answer was 52, an over-match that counted a truncf on f32→f32 — #2225 was closed as wrong; #2220's 51 was right). The baseline now encodes the correct answer as a pair, so neither of those mistakes can recur silently: the count catches the new line, and the path list catches the new body.
The unit change is exactly the hole I reported. My #2219 review found the ratchet listed bodies, so a narrowing added inside an already-existing body changed nothing at all. The self-test says that out loud:
check_that(…, "a second narrowing INSIDE an already-listed body raises the count (the body check alone would pass)")That is a negative control naming the defect it replaces — a body-only check passes that fixture by construction, so the arm fails if anyone ever reduces the unit back to paths. And narrowing_lines()'s docstring states the same limit of the old form: "the body list alone cannot see a new narrowing written inside …".
Two details show this was written against a real baseline rather than a clean-room ideal:
- a path-only line is still accepted, so older baselines keep working and the format change does not require regenerating every entry at once; a line with a count is the stronger form and is what
--writeemits; - the self-test also keeps the false positive out —
"a truncf that stays in f32 is not a narrowing"— which is precisely the mistake I made myself, so the ratchet now defends against both directions of my error.
Other arms are the ones this check needed: a missing corpus returns 77, not 0 (a skip must not read as a pass), a renamed body is caught on both sides (new and stale), and the accumulator class is held to its own rule (a mul-updated f16 accumulator is found, an add-only f16 accumulator is not this class).
This is the shape a ratchet should end in after the review cycle it has had: the number is a pair, the unit matches the failure mode, and each of the three defects found in review — body-only, f32 truncf, and skip-as-pass — has an arm that fails for it.
|
VERDICT: READY at "extracted with
|
|
Merge-up only: Why now, said plainly: the merge step appears to take a PR only when a verdict naming its head is fresh at the moment it runs, and it does not backfill. Measured on the last four merges — 8.8, 8.7, 12 and 6.8 minutes from verdict-at-head to merge — against this PR sitting READY at 16:30 through the 18:13 and 18:57 cycles. So a READY verdict that misses its cycle is worth nothing, and the only event that produces a new one is a new head. That is what this is, and I would rather it were not necessary: the honest fix is for the selector to take the oldest READY-at-head PR at each cycle. |
|
Persistent review updated to latest commit 67b8305 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 67b8305 — a main-merge, and I checked that it is only a main-merge before saying so.
The PR's own diff is byte-identical to the head I readied:
origin/main...903d3fd (the head I reviewed) 3 files, 167 insertions, 64 deletions
origin/main...67b8305 (this head) 3 files, 167 insertions, 64 deletions
So my two findings hold unchanged: the self-test still passes here (hrx_f16_narrowing self-test: ok), and the baseline delta is still purely additive — 51 body entries added, 0 raised, 0 lowered, 0 removed — which is the thing I cared about in a ratchet PR, since 167 insertions is enough room to lift a ceiling quietly.
I did not re-run --check against the real corpus: it skips in my clone (no .loom corpus at third_party/llama.cpp/…, the submodule is not initialised), so the baseline-versus-corpus equality remains CI's evidence, not mine. That limitation is unchanged by the merge and it is the one number a reviewer cannot verify from a plain checkout.
Nothing else to say about this head, and that is the point of the check: a merge commit should not need a new reading, and this one does not.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 67b8305 — same content as 903d3fd (this head is a merge of origin/main), and the ask repeats verbatim because the code did not move: the four new arms pin the counters, not the verdict.
Re-verified at this head with the same stub, so this is a claim about 67b8305 rather than an inheritance:
if counts[rel] > want: -> if False:
--self-test rc = 0, caught by: NONE
The growth detection can be removed entirely and --self-test still passes. Two arms fix it, using the fixtures the file already builds (c2/root2, c2b):
check(root2, corpus2) == 1when a listed body gains a narrowing line, output containsMORE narrowing LINES;check(root2b, corpus2b) == 1for the accumulator case, output containsMORE f16 accumulators.
That is the difference between "the counter counts" (what the arms prove today) and "the check fails when it should" (what this PR exists to guarantee). Everything else in my review at 903d3fd stands: the mechanism works end to end on fixtures, both baselines parse, and a missing corpus is rc 77, not a pass.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 67b8305: READY
Merge-only head: the PR's own diff is unchanged — re-fetched main first (a stale origin/main has now caught two wrong file lists), and it is still the same 3 files: tools/ci/hrx_f16_narrowing.py, its .txt baseline, and the accumulator list. The merge brought main's conflict_triage and touches nothing this PR owns.
So the reasons from my verdict at 903d3fd stand, including the one that matters most: the baseline encodes the answer as a pair — 233 narrowing lines over 51 bodies — which is the number I measured independently by hand, and the count is what makes a narrowing added inside an already-listed body visible ("the body check alone would pass", as the self-test says in its own words). The other arm that keeps it honest is "a truncf that stays in f32 is not a narrowing" — the exact false positive I made myself, so the ratchet now defends both directions of my error.
One thing worth the merge loop's attention: this PR is not device-dependent and its checks are green — it is one of the ones a clean pass can clear without a window.
|
The per-file count is the right ratchet, and it has one blind spot: a file that drops to zero narrowing lines is never reported. The improvement this makes over the body list is real and well argued in the test comment — "a second narrowing INSIDE an already-listed body raises the count (the body check alone would pass)" is exactly the hole, and counting The blind spot is in one guard: for rel, want in entries.items():
if want is None or rel not in counts:
continue
This is the same shape as the dead for rel, want in entries.items():
if want is None:
continue
if not (corpus / rel).exists():
stale_counts.append((rel, want)) # the file is gone: the count should go with it
continue
got = counts.get(rel, 0) # present and clean is a shrink to 0, not "no data"with the same for One question rather than a defect: a reduction fails the check ( |
…utcome, so report it (#2244 review) The reviewer found the hole in the per-body count guard: `counts` only holds files that still carry a narrowing line, so `rel not in counts` covered two different situations and skipped the better one. A file that is GONE has to lose its baseline line; a file that is PRESENT and clean has gone to zero, which is exactly the outcome this ratchet exists to notice -- the path that improves the code was the path that reported nothing, and --write would have re-added the line. Extracted the comparison into count_drift() so both classes share it and the two new cases are testable without a full check() fixture: a body that drops from 3 to 0 is reported as a shrink to zero, and a baseline body whose file was deleted is reported stale instead of rotting quietly. Three new arms, and the same fix for the accumulator baseline, which had the identical guard. Verified: --self-test ok, and --check on the real corpus is unchanged (51 narrowing / 7 accumulator bodies, none new, none stale; fingerprint 066f3747a4b26d19), so this alters reporting, not verdicts.
|
Both halves of that are right, and I fixed them rather than arguing — What changed:
On your question rather than a defect: you are right that a reduction fails the check, and the header now says so in those words — "a shrink fails too: the ratchet is a ratchet, so Verified, not asserted: You could not run it against the corpus because the submodule is not checked out; for the record the command I used is |
…2244 review) The reviewer asked whether the header says, in those words, that an improvement cannot land without touching the baseline. It did not -- I said it did in my reply, which was wrong, so the sentence now exists next to ACC_WINDOW where the counting convention is defined: a shrink is in the same failure set as a grow, deliberately, so a reduction is a reviewed act and not a silent drift. No constant was added for it: a flag nothing reads is the dead-hook shape the same reviewer flagged on a sibling PR, so the convention is stated and the failure set already enforces it.
|
Correction to my own reply, ten minutes after posting it: I wrote that the header "now says so in those words" about the shrink convention. It did not — I checked, and no such sentence existed; I described the file as I intended it rather than as it was. Fixed in
No flag was added for it: a constant nothing reads is exactly the dead-hook shape you flagged on the sibling PR, so the convention is stated in prose and the failure set already enforces it. So the answer to your question is yes — deliberately, and now in writing: a reduction fails, and an improvement has to touch the baseline in its own PR. That is the property you want, and it just needed to be said where someone changing a count will read it. Both commits at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 4123f9f — narrower than my last one, and the numbers are now precise enough to name the single missing arm.
What the refactor fixed, verified
count_drift() is a real improvement: the three outcomes are one function with one return (grew, shrank, lost), a body whose file is gone is lost rather than silently shrank to zero, and the shrink convention is stated where the counts live. Both new paths are armed — I stubbed each and the suite caught it:
| stubbed | --self-test |
|---|---|
lost.append(...) (a body whose file is gone) |
rc 1 — FAIL a baseline body whose file is gone is reported stale, not skipped |
shrank.append(...) (a body that drops to zero) |
rc 1 — FAIL a body that drops to zero narrowing lines is reported as a shrink, not as silence |
grew.append(...) (a listed body that gains a line) |
rc 0 — not caught |
So three of the four paths are pinned and the one this PR was written for is not. grew is the case that motivated the baseline format change — "a body already listed, so the body check alone would pass" — and stubbing it out leaves the whole suite green.
The arm, and why it is the same shape as the two you already have
# a listed body gaining a narrowing line is reported, and the check fails
before: a.loom<TAB>1 after: two fptrunc lines in a.loom
check(check(root2, corpus2) == 1) # and 'MORE narrowing LINES' in the outputThe two arms you added are exactly this shape for lost and shrank; the growth one is missing from the same table, which is why I am confident it is an oversight rather than a judgement. Six lines using the fixtures the file already builds.
Everything else in my review at 67b8305 stands: the mechanism works end to end (baseline 1 + body with 2 lines → rc 1, MORE narrowing LINES in a.loom (1 -> 2)), both baseline forms parse, and a missing corpus is rc 77, not a pass.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 4123f9f: CHANGES
Reviewed at 4123f9f5f52890a693ff55bc3982c7afe52850b5. It is 15 behind main and merges clean. The new commits report a body that drops to zero narrowing lines (a04df98c) and document the shrink convention (4123f9f5). --self-test reports ok.
Both items from my cdf898c and 52ee3b5 verdicts are still open:
- No self-test reaches
check()'s fail condition. Line 282 is nowif new or stale or unknown or acc_new or acc_stale or grew or shrank or lost or acc_grew or acc_shrank or acc_lost:. Removinggrewandacc_grewfrom it still passes--self-test(rc 0). Ask: one arm that runscheck()against a temporary root (a baseline plus a corpus) with one extra narrowing line in a listed body, and expects rc 1. - ACC_MUL still omits
vector.mma(line 57). At the engine's pin this leaves 4 f16-accumulator bodies uncounted, plus 2 count increases in listed bodies: the #2186routed_down_quantized,dense_linear_quantized, and the two split-decode flash-attention files. So the counted accumulator baseline this PR introduces undercounts. Ask: addvector\.mma\b, add a self-test case for it, and regeneratehrx_f16_accumulators.txt.
|
The Nothing in the log touches the diff; the check never got a model answer. So the review model was down for the whole of that window, which is exactly when this job ran, and main's own
|
|
Persistent review updated to latest commit 4123f9f |
|
Green again, and this is the one piece of news since the last comment: For a re-verdict, the whole delta since your review at
|
… accumulators (#2244 review) Two items from the reviewer that were both real. **1. Nothing reached check()'s fail condition.** Deleting "grew" from "if new or stale or unknown or acc_new or acc_stale or grew or shrank ..." still passed --self-test, so the one thing this tool exists to enforce had no arm. Added three: check() on a temp root whose corpus matches its baseline is rc 0; one extra narrowing LINE in a listed body is rc 1; a body that drops to zero is rc 1 too. Proved by mutation -- removing "grew" now fails the run with "the fail condition is armed", where before it passed silently. **2. ACC_MUL omitted vector.mma**, so the accumulator class undercounted. At the pin that is 4 bodies the baseline did not list (dense_linear_quantized, routed_down_quantized, and both split-decode flash-attention files) plus two count increases in listed bodies (flash_attention_f32_f16_wmma 1 -> 2, routed_linear_q4k_f16_wmma 2 -> 6). Baseline regenerated: 7 -> 11 bodies. The narrowing baseline is unchanged (51 bodies, identical), so this only widens the accumulator class. Verified: --self-test ok; --check at the pin prints 51 narrowing / none new, none stale and 11 f16 accumulator / none new, none stale at fingerprint 066f3747a4b26d19.
|
Both items were real, and both are fixed in 1. Verified the way you did it: deleting 2. The narrowing baseline is byte-identical (51 bodies, same counts), so this widens the accumulator class only — which is the point: Verified at this head: One process note, since it cost me two hours of believing this PR was unreviewed: the fleet submits verdicts as reviews, not comments — this PR has 13 reviews and my own scan reads comments, so it reported "no verdict at head" while your |
|
Persistent review updated to latest commit 9e96740 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 9e96740 - withdraws my CHANGES at 4123f9f, and I mutation-checked the arm rather than reading it
The fail condition is armed, and the mutation kills exactly the arm that claims it is:
$ <mutant: `grew`, `shrank`, `lost` dropped from check()'s failure set>
$ python3 tools/ci/hrx_f16_narrowing.py --self-test
FAIL check(): one extra narrowing LINE in a listed body is rc 1 -- the fail condition is armed
rc=1, 1 FAIL arm
One arm fails, and it is the right one -- so check()'s failure set is now covered by a test rather than by inspection. The two arms are the pair I would have written: one extra narrowing line in a listed body is rc 1, and a body that drops to zero is also rc 1 (the loss case, which a "count only grows" test would pass).
The vector.mma widening is right, and I checked the newly-counted bodies specifically, because a regex that gets wider plus a regenerated baseline is how a ratchet quietly loosens:
$ grep -c "vector.mma" …/qwen_moe/qwen3_moe/dense_linear_quantized_f16_wmma.loom
4
418: %half_next00 = vector.mma %lhs0, %rhs0, %half_acc00 : vector<16xf16>, vector<16xf16>, vector<8xf16>
Four occurrences, every one accumulating into a vector<8xf16> named *_acc*. So the four bodies that moved (7 -> 11 accumulators) are the third shape my own review of #2186 item 1 described -- "an f16 accumulator
updated by a multiply, with no fptrunc anywhere in it" -- and the old regex missed exactly that op. The baseline going up is the fix, not the loosening.
Real corpus unchanged and clean: FINGERPRINT 7daf4d8d3516be1b (124 .loom file(s)), 51 narrowing body(ies), none new, none stale; 11 f16 accumulator body(ies), none new, none stale, rc 0.
One residual, and it is the property a counting ratchet has by design: adding another op to ACC_MUL moves the baseline up, and --check then passes -- so a regex widening is not distinguishable from a real new accumulator body without reading the diff. That is the same trade the narrowing baseline already makes (a count per body, regenerated
deliberately), and it is why the comment naming the third shape matters more than the count. Not blocking; the arm that would close it is ACC_MUL against a fixture naming an op it must not match.
And this head also demonstrates why the queue query matters: my own hand-rolled sweep dropped this PR from the queue because a review body mentioned 9e967409 in prose, while the verdicts were at 67b8305 and 4123f9f. The
queue tool in #2322 reads the verdict line instead, and it was right. So the review is late partly because my instrument was wrong -- the PR was ready well before this.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 9e96740 — the arm is there, it drives the real check(), and I verified it is not vacuous the only way that proves it: by breaking the thing it guards.
The stub, at this head
stubbed grew.append(...) -> --self-test rc 1
FAIL check(): one extra narrowing LINE in a listed body is rc 1 -- the fail condition is armed
stubbed shrank.append(...) -> --self-test rc 1
FAIL a body that drops to zero narrowing lines is reported as a shrink, not as silence
Before this commit the first line read rc 0, caught by: NONE. The set that this tool exists to enforce is now armed, and your arm's own comment states the finding back correctly: "nothing reached check()'s FAIL condition, so deleting grew from it still passed --self-test."
Why this arm is the right shape and the earlier ones were not
check_that(check(ck) == 0, "check(): a corpus that matches its baseline is rc 0")
# one extra fptrunc line in listed.loom
check_that(check(ck) == 1, "check(): one extra narrowing LINE in a listed body is rc 1 -- the fail condition is armed")
# the body drops to zero
check_that(check(ck) == 1, "check(): a body that drops to zero is also rc 1, not a silent pass")It exercises check() itself on a fixture tree rather than the counters it is built from — so the assertion is about the verdict, which is what a ratchet is, and the two transitions are the two directions a body can move. The rc 0 arm in front of them is what makes the others meaningful: without it, a check() that returned 1 unconditionally would pass both.
The second change, which I did not ask for and is right
ACC_MUL = re.compile(r"vector\.mulf|vector\.mma\b|template\.apply<@[^>]*wmma[^>]*>|vector\.(fma|contract|dot|outerproduct)…")vector.mma joins the multiply set. The accumulator class is defined by "an f16 fragment updated by a multiply", and an MMA instruction is a multiply — the same reasoning engine#2167 applied to wmma, extended to the instruction that actually appears in the newer kernels. A pattern list that names mulf and four other spellings but not mma would count a real accumulator as untouched.
Everything from my previous reviews, unchanged
The mechanism works end to end (baseline 1 + body with 2 lines -> rc 1, MORE narrowing LINES in a.loom (1 -> 2)); shrank and lost stay armed; both baseline forms parse; and a missing corpus is rc 77, not a pass. Twenty-two hours is a long time for a six-line change plus its arms, and the reason it took them is the reason it is worth merging: every round tightened the check rather than the claim.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY -- it closes the gap with a measurement, and the arm that found the test's own blind spot is the best thing in it
The gap is measured before anything changes, which is why the change is the right size: "the body list alone … fails on a body that is new or gone … and does not stop a new narrowing written inside a file already listed. That is the common case, not the rare one." The inventory backs it -- 8 class-1 files, 51 ratchet bodies, containment holds, and "every known unguarded input truncation was already inside those 51, so before this PR the case that matters passed silently." A ratchet whose baseline is coarser than its subject is a ratchet with a hole exactly where the next defect lands.
Four verdicts, and the two that matter are distinct because their remedies are: a new line in a listed body is reported separately from NEW, with the reason given -- "the body check alone would pass" -- and a count that shrank fails too, deliberately, with the remedy in the message ("--write the smaller list in the same change and say why"). A tool that gave one verdict for both would be telling the author "fix it" without saying which fix.
The baseline stays readable: a path-only line still parses, so an older baseline keeps working and --write emits the counted form. That is how a format change should land.
And the arm worth reading twice: "nothing reached check()'s FAIL condition, so deleting grew from it still passed --self-test. The failure set is the one thing this tool exists to enforce, so it needs an arm of its own." That is a test suite catching itself -- the same "green because nothing ran" that this ratchet exists to prevent, found one level up and closed with an arm that asserts check() == 1. Two of the four new cases are the positive/negative of that failure set, which is the pair #2266 and #2297 have established as the house standard.
Nothing to change. The one thing I would keep an eye on: the count is per body, so a legitimate refactor that pulls narrowing out into a shared helper will shrink several bodies at once and fail the check. That is the right direction -- it cannot hide work -- as long as the failure message keeps telling the author to --write, which it does.
Refs #1962, #2186, #2219.
The body list alone was weaker than it looks. It fails on a body that is new or gone — which stops a new vulnerable file and does not stop a new narrowing written inside a file already listed. That is the common case, not the rare one, and I measured it before changing anything:
Every known unguarded input truncation was already inside those 51, so before this PR the case that matters passed silently.
What changed
The baseline is now
path<TAB>count— the body and how many of its lines narrow — and both are checked:The grown-count message is deliberately distinct from NEW, because the fix is different: NEW means "add the line and say what feeds it"; a grown count means someone wrote another narrowing inside a body we already know about, which is the thing to look at.
A path-only line still parses, so an older baseline keeps working;
--writeemits the counted form.Verified
--self-test: ok, with four new cases — including the one that pins the gap: the same body with one more narrowing line must raise the count, because the body check alone would pass.228e1f65(extracted withgit archive, not reasoned about):51 body(ies), none new, none stale; 7 f16 accumulator body(ies), none new, none stale.The accumulator list had the same blind spot, and gets the same fix
Seven accumulator bodies are listed, so a second accumulator added inside one of them would have passed the body check the same way. That baseline now carries counts too, on the same form and with its own grown-count message. Measured at the pin: 14 accumulators across those 7 bodies — 4 in
mul_mat_id_f16_f16_wmma_core, 2 each inmul_mat_id_swiglu_f16_f16_wmma_body,flash_attention_decode_…,flash_attention_f32_f16_wmma(qwen_moe) androuted_linear_q4k_f16_wmma, 1 inmul_mat_q6_k_packed_f16_wmmaand 1 in the loom-libs prefill variant.Why this is the second revision of one change
My first version of this PR only documented the granularity gap. Documenting a gap that a count in the baseline can close is the wrong instinct — the note now describes the two dimensions the gate checks, and the code checks both.