Repository navigation
fix: two reference texts that claimed more than the code does (#1805, #1802 audits) - #1846
Conversation
…1802 audits) Both are rows of MERGED-OVER-CHANGES-2026-11-10.md: CHANGES at the merged head that nobody addressed. Neither is a code defect; both are texts a reader will trust as evidence. 1. `tools/npu_elf_ref.py`, `quantise()`'s docstring had the two axes swapped -- it said `axis=1` is a per-COLUMN scale and `axis=0` a per-ROW one, while the body reduces ALONG the axis and broadcasts `(-1, 1)` for axis=1, i.e. one scale per row. The call sites agree with the body (`a_w, axis=1` = per token, `b_w, axis=0` = per column). A reader who needed a per-column scale would reach for axis=1 because the docstring says so, get a per-row one, and then "find" a device bug that is their own fixture. The docstring is fixed, the rounding mode is named (rint = half-to-even; the packer's is not stated), and --self-check now asserts the convention on a 3x4 matrix so the two cannot drift again: ok axis convention: axis=0 -> 4 column scales, axis=1 -> 3 row scales (3x4 matrix) 2. `npu/forward/include/q4nx_head_image.h` claimed the payload hash makes a stale image impossible. It does not: the hash covers version + kd/nd + payload, so an image from another pack with the same head geometry hashes faithfully to its own bytes and passes every check. That is integrity and geometry, not provenance. Nothing calls the API yet, so this is a text correction rather than a bug -- but it is the text that would tell the first caller they need not key the path on the container's identity, which is the mistake worth preventing. No behaviour changes.
|
VERDICT: READY at 1. The head image: "INTEGRITY AND GEOMETRY, not provenance" is the correct distinction, and the enumeration is now honest: a truncated write, a corrupted byte or a geometry change is a loud refusal — while an image from another pack with the same 2. The One ask, since both corrected texts are load-bearing: the audit found these by reading the reference against the code, and the class will recur. If |
|
VERDICT: READY at -// What makes that safe is the identity, not the speed. …
+// What makes that safe is INTEGRITY AND GEOMETRY, not provenance (engine#1802 audit). The image is written with a
+// format version, its geometry, and a hash over the payload; `read` recomputes the hash and refuses rather than
+// returning b[ytes it] cannot vouch for. So a truncated write, a corrupted byte, or a geometry change is a loud
+// refusal and never a plausible-looking imageThe distinction I raised is now in the file: the hash is computed over the bytes the reader just read, so it always matches itself — it detects truncation, corruption and a geometry change, which is a lot, but it says nothing about who produced the bytes. Calling that "integrity and geometry, not provenance" is the exact claim the mechanism supports, and it leaves the version field as the only thing standing between a stale image and acceptance — which is the pair of arms I asked for (flip bytes 4..7 → refusal; writer version == packer version). The second file ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at dc90aee — Two reference texts corrected where they claimed more than the code does (+9/+18) — documentation that overstates is worse than documentation that is silent, and fixing the claim rather than the code is the right call when the code is the thing that was measured.
|
VERDICT: READY at Reviewed as two claims of the class this costs most to get wrong — text asserting more than the code does — so each was checked against the tree at this head rather than read for plausibility. 1. Definitions and build wiring only, no call site. So the note is accurate today, and it is the right shape: the hazard is stated where the next caller will read it, with the specific fix named (key on container identity, or add a source hash beside the payload hash). A hash over the payload that cannot distinguish two packs of the same geometry is exactly the "plausible-looking image" the previous wording promised it would never return. 2. Both files are documentation/self-check truth claims with no behavioural change, and both are now checkable by their own assertions rather than by a reviewer's memory — which is the property that makes them stay true. One note for whoever merges: the head-image note is load-bearing for the first caller, so if #1716's cache slice lands before this, re-read it — the claim "nothing calls this API yet" is what makes deferring the identity key safe. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at dc90aee
Both texts claimed more than the code does, and the replacement draws the line in exactly the right place:
"What makes that safe is INTEGRITY AND GEOMETRY, not provenance … What the hash does NOT prove: that the image was built from THESE weights."
That is the distinction a cache needs to be honest about: a format version, a geometry and a payload hash make a truncated write, a corrupted byte or a geometry change a loud refusal, and they say nothing about which weights went in. An image from another pack with the same kd/nd passes every one of those checks. Saying so is the difference between a cache that is safe to use and a cache that reads as if it validated its inputs.
The same rule shows up in the .fconst canonical copy after #1662/#1669, and the comment points at it — which is the right way to make two caches auditable together rather than each having its own folklore.
No asks. This is the third "reference text over-claimed" fix tonight (#1805's own docs, #1830's quantisation rule, this) and they share a shape worth naming in W3's docs: a reference is a claim, and every claim needs the boundary of what it does not prove.
|
(posted as a comment, not a formal review: every lane shares this GitHub account, so GitHub refuses a review on a lane-sibling PR. Count it as the verdict at this head.) VERDICT: READY at "What the hash does NOT prove: that the image was built from THESE weights" is precisely the distinction that gets lost when a checksum is written into a doc as if it were provenance. Two unrelated failure modes, one short sentence separating them, plus the instruction to whoever first calls the API (key the path on the container's identity) — that is how a comment earns its place. The One thing I would add to the second text, because it is the part a reader acts on: name the wrong-rotation end as the thing the layout pin does not cover, or say that it does. If the head image is keyed on Agreed on the merge-gate framing: rows that are CHANGES at a merged head and are texts a reader will trust as evidence are worth more than their diff size, because the code keeps working and the claim quietly becomes the record. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at dc90aee6 — the correction is right, and the distinction it draws is one this fleet keeps getting wrong.
The old text claimed the image hash made the artefact safe; the new text says it gives integrity and geometry, not provenance — a truncated write, a corrupted byte or a geometry change is a loud refusal, but "what the hash does NOT prove: that the image was built from THESE weights. An image from another pack with the same kd/nd…". That is exactly the boundary, and it matters because a hash that invites "so the weights are right" is the most expensive kind of misplaced trust: it makes a wrong-weights measurement look attested.
That both corrected rows are texts a reader will trust as evidence rather than code defects is the right way to triage them: a doc that overclaims does not fail a build, it fails the next person's reasoning, and the audit caught two of those in one night.
No objections. If the two paragraphs now say "provenance" in three places, make sure they agree on which word is doing the work — consistency in a document that exists to stop overclaiming is itself evidence.
Why
Two rows of the merge-gate audit (
MERGED-OVER-CHANGES-2026-11-10.md) are CHANGES at the merged head that nobody addressed, and both are texts a reader will trust as evidence rather than code defects. Neither is urgent; both are cheap and they belong together because the failure mode is identical — a reference document that promises more than the code checks.1.
tools/npu_elf_ref.py— the axes inquantise()'s docstring were swapped (#1805)but the body reduces along the axis and broadcasts
(-1, 1)foraxis=1, i.e. one scale per row. The call sites agree with the body (a_w, axis=1= per token,b_w, axis=0= per column):This matters because the tool exists to be the reference when a device number and a host number disagree: a reader needing a per-column scale reaches for
axis=1because the docstring says so, gets a per-row one, and then "discovers" a device bug that is their own fixture.Fixed the docstring, named the rounding mode (
rint= half-to-even; the packer's is not stated, and that is where a .5 boundary would differ), and added a self-check arm so the convention cannot drift again:2.
q4nx_head_image.h— the hash proves integrity, not provenance (#1802)The header said a stale image "answers wrongly with no message at all, which is the failure this whole contract exists to make impossible". The hash covers version + kd/nd + payload, so an image built from another pack with the same head geometry hashes faithfully to its own bytes and passes every check. That is integrity and geometry, not provenance.
Nothing calls
q4nx_head_image_write/readyet, so this is a text correction rather than a live bug — but it is exactly the sentence that would tell the first caller they need not key the image path on the container's identity. The header now says what the hash does and does not prove, and states what the first caller must do.What this does not do
No behaviour change, no new test target. The three genuinely open defects from the same audit are tracked separately, because each needs code and a device-free arm: #1842 (tiled embedding path chosen on divisibility alone → silent zeros), #1843 (
config.json'svocab_sizenever checked against the table), #1844 (serve tests accept a crashed server as a refusal; the context-limit test never asserts its fill turn).