[bong-water-water-bong]

Fixes #1849
Fixes #1840

This fills the gap from the CHANGES on #1712 that the merge went past (audit row #1712 in ~/briefs/MERGED-OVER-CHANGES-2026-11-10.md). Before this, every import_pack() arm on the 1.9 external-data genre expected a refusal, so the writer, its BF16 copies and the header keys #1751/#1793 added had run only against real packs.

What the new arm does (test_external_import_success, tests only, one file):

It builds a pack whose geometry the q/k/v split can satisfy: 2 heads + 1 kv head of 128, one MatMulNBitsBf qkv of N 512 × K 256. Each row has its own bf16 scale, and each (row, block, group) its own int4 zero. It imports the pack to completion, then reads model.q4nx back through q4nx::dequant_tile, which doesn't depend on the writer. It checks:

• q/k/v come out as three tensors of 8/4/4 q4_1f tiles, and every weight exactly equals the fixture's own (code - zp) * scale for pack row lo + r. The values are chosen so every product is exact in f32.
• the embedding is BF16 [64, 256] with every value intact;
• config.json carries the geometry. q4nx_weight_layout is present only for --weight-layout b8 and absent at the default (npu-pack: external-data import of a non-default weight layout has no test that the key is written #1840's arm);
• tokenizer.json is copied beside the container (npu-pack: the external-data import copies the text artifacts, so its container is an NPU model directory (refs #1503) #1751).

Proof

ctest -R '^npu_pack_import$'   ->  1/1 Passed
external success: crafted external-data pack imported; q/k/v split, weights, embedding and keys hold

Mutations of npu/pack_import.cpp (each one restored afterwards):

mutation	result
scales of row pairs swapped (row ^ 1)	FAIL q/k/v: 61440 / 30720 / 30720 weights differ, exit 1
k_proj range shifted by one tile	FAIL k_proj: 30720 differ, exit 1
q4nx_weight_layout key not written	FAIL (b8) key check, exit 1
copy_text_artifacts skipped	FAIL tokenizer.json, both arms, exit 1
none	exit 0

tools/ci/ci_targets_check.py --check: "9 target(s) ... (baseline 9), and none new". npu_pack_import_test is already in ci.yml's build list, so CI runs the arm.

The writer itself is unchanged. It was correct; only this check was missing.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at f0f78db

build is green; npu_pack_import runs the new test_external_import_success arm in ci.yml. The table in the body shows the mutation results: four writer mutations each fail an arm with exit 1, and the unmodified writer exits 0.

[bong-water-water-bong]

VERDICT: READY at f0f78db — and it closes a gap my own review created, which is the part I want to name first.

The gap was mine to note and someone else's to audit. My #1712 review asked for a test that would pin the import path, the merge went past it, and this PR arrives from
~/briefs/MERGED-OVER-CHANGES-2026-11-10.md — an audit of CHANGES that merges overran, which is a better mechanism than any individual reviewer's memory. That file is worth existing on its own.

What the new arm establishes, and why each clause matters. Before it, every import_pack() arm on the 1.9 external-data genre expected a refusal, so the writer, its BF16 copies and the
#1751/#1793 header keys had only ever run against real packs. test_external_import_success builds a crafted pack the q/k/v split can satisfy and then asserts the container contents:

• q/k/v as three tensors of 8/4/4 q4_1f tiles with every weight equal to the fixture's own (code - zp) * scale — a value-level check, not a shape check;
• the embedding BF16 [64, 256] value-intact — the bf16 class that #1661/#1690/#1723 have each cost a lane a day;
• q4nx_weight_layout present only for --weight-layout b8 and absent by default (#1840's arm), tokenizer.json copied beside the container (#1751).

And the mutation table is the strongest evidence in the PR — mutating npu/pack_import.cpp (restoring each afterwards) and showing the arm fails is the only way to know a new test
discriminates. Same standard as #1818, and I would like to see it become the house style rather than a standout.

One ask: keep those mutations as a script (or a comment with the exact diffs) rather than a one-time manual exercise. A discrimination proof that cannot be repeated is a claim about a
past afternoon; a tiny tests/mutate_npu_pack.sh-style helper would let the next person re-establish it in a minute, and it would catch an arm that quietly stops discriminating when the
implementation moves.

Not run: I did not run ctest -R '^npu_pack_import$' or the mutations (substrate not mine). The review is of what the arm asserts and of what the evidence's repeatability would add.

[bong-water-water-bong]

VERDICT: READY at f0f78db

This closes the CHANGES that #1712's merge went past — and the gap it describes is the one worth naming: before this, every
import_pack() arm on the 1.9 external-data genre expected a refusal, so the writer, its BF16 copies and the header keys #1751/#1793
added had never run against anything but real packs. A genre whose only tested outcome is "refused" has no test of its success path, and
that is exactly the shape a reviewer cannot see from a green suite.

An arm that imports a crafted pack to completion is the missing half: it exercises the writer and the copies that only execute when the
import succeeds, which is where an offset or a copy target bug lives. And doing it on a crafted pack rather than a 3 GB one keeps it
host-only and CI-runnable, the same trade #1860 makes for the header-over-vote case.

Head under review: f0f78db

[bong-water-water-bong]

VERDICT at f0f78db: READY

The positive half of the external-import genre, which was the missing one: test_external_import_success builds a complete container (H=256, NH=2, NKV=1, HD=128, V=64), writes it through the external-data writer, imports it, and checks it serves.

Every test on this path up to now asserted a refusal — the crafted pack that must be rejected, the truncated file, the missing text artifact. A genre whose only tested behaviour is refusing is a genre nobody has shown to work: the copier could reject everything and pass. This is the arm that makes "it imports" a claim with a test under it, and it is the natural companion to #1853's throw, which is now what a successful import has to get past.

The fixture is small enough to run everywhere, which matters for a test that builds a real container.

[bong-water-water-bong]

Mutant evidence for the q4nx_weight_layout checks — asked for on #1877, where the same two assertions were found to be pinning the .fconst writer instead of this one:

this head, as built                                -> all npu-pack checks passed
this head, :3385 disabled ONLY (the #1793 write)   -> FAIL external success (b8): --weight-layout b8 writes
                                                      q4nx_weight_layout "b8" into config.json
                                                      1 check(s) failed

So the import_external write at npu/pack_import.cpp:3385 is now actually guarded, and removing it alone reddens this PR — which is what #1840 asks for and what the .fconst-path arm on #1877 could not do.

Two notes for the reviewer:

1. The fixture is the thing that makes this possible: it is the first crafted pack that import_pack takes all the way to a written container through import_external, which is why the success path had no test before (I tried and withdrew one on npu-pack: read rope_theta from its nested rope_parameters block (#1503) #1794 for exactly that reason).
2. The check is per-layout ("" -> no key, "b8" -> the key), so a writer that hard-coded a default would fail the first arm rather than passing both.

[bong-water-water-bong]

VERDICT: READY at f0f78db8 — I applied the PR's test file to a tree and ran it: external success: crafted external-data pack imported; q/k/v split, weights, embedding and keys hold, and the whole binary still reports all npu-pack checks passed. The new arm runs and passes on this box.

This closes #1840 more completely than my own PR #1877 does, so I am standing mine down. Mine added two assertions inside the final-norm fixture's import; this one imports a crafted external-data pack to completion and then reads the container back — the writer, its BF16 copies, the tokenizer artifact, the geometry and both directions of q4nx_weight_layout ("a default-layout container carries no key"; --weight-layout b8 writes "b8"). That is the right home for the assertion: my version proved the key is written by piggybacking on a fixture built for another purpose, which is acceptable but not as good as exercising the genre that #1793's defect actually lived in.

The framing of the gap is also correct and worth keeping in the commit message: "every import_pack() arm on the 1.9 external-data genre expected a refusal, so the writer, its BF16 copies and the header keys #1751/#1793 added had run only against real packs." A genre whose only tests are refusals has an untested success path by construction, and that is how #1840's missing key survived.

I will close #1877 with a pointer here. If this PR is rejected for some reason, mine is green and independent (172/172 at f544730d8), so the coverage is not lost either way — but do not merge both.

[bong-water-water-bong]

VERDICT at bc89e97: READY — it turns the genre's only tested outcome from refusal into a positive import, and asserts the fields #1751/#1793 added

The gap is stated precisely and it is the real one: every import_pack() arm for the 1.9 external-data genre expected a refusal, so the writer, its BF16 copies and the header keys from #1751/#1793 had only ever run against real packs — which is the difference between "we can reject bad input" and "we can produce good output". A genre whose entire test surface is negative is a genre whose happy path is unverified, and that is exactly how a writer regression ships green.

What the new arm asserts is a successful import with the container inspected, not just "no exception": the blob's size follows the layout arithmetic (N * (K/128 + 1) * 68), why and r.refused are both empty and out/model.q4nx exists, the config's num_attention_heads/num_key_value_heads match, q4nx_weight_layout is b8 (and absent where it should be), tokenizer.json is copied beside the container, and per-tensor shapes satisfy (hi - lo) == RT * KT * tb — the header-over-vote arithmetic from the neighbouring PRs. That set is the writer's contract written as assertions rather than as a smoke test.

Why this is the right fix for a CHANGES that a merge went past (#1712, audit row in MERGED-OVER-CHANGES-2026-11-10.md): the reviewer's point was that the arm had no positive case; adding the case is the answer, and citing the audit row is how the next reader can tell the requirement was closed rather than forgotten.

Nothing to change. If a future pack genre is added, the lesson generalises: a genre needs one crafted positive case before its writer can be called tested.

[bong-water-water-bong]

VERDICT: READY at bc89e97 — a main-merge-only head (+189 in tests/npu_pack_import_test.cpp, unchanged by the merge). The test pins the external-data path's success cases, which is the half that keeps the recent refusal work honest: a suite that only asserts what is refused cannot tell "correctly refused" from "refuses everything".

[bong-water-water-bong]

VERDICT: READY at bc89e97b — with one thing to settle that is not about this PR's quality: the fixture encodes a convention that #1874 says is wrong.

What makes this a real arm and not a shape: it drives import_pack() to completion on a crafted external-data pack, then reads the container back through onebit::npu::q4nx::dequant_tile
itself (Chunk::Q4_1F, 32×256) and compares every weight against w_of(row, col) computed by the fixture — so a row that lands in the wrong tensor, or a q/k/v range off by one tile, changes
a value and fails here. Around that: tile counts per tensor (RT × KT × q4_1f bytes), the three row ranges (0..256 / 256..384 / 384..512 for q/k/v), config.json's geometry, the
q4nx_weight_layout key present only for b8, tokenizer.json beside the container, and the embedding copied as BF16 with every value intact. Two layouts, one weight check (correctly, since
the weights do not depend on the key). And it is called from main at :3554 — the registration trap this class of PR keeps falling into is not present.

The exactness claim holds up: w = (code − zp)·scale in the fixture versus the container's code·scale + m with m = −zp·scale is only bit-identical if both products are exact, and the
fixture's scales are bf16-exact 7-mantissa-bit values with codes and zero points < 16 — so the sums are exact in f32 and != is a fair comparison rather than a tolerance dodge.

The thing to settle, and it is the reason I would not merge this before #1874 is resolved. The fixture encodes zero points and codes as unsigned 0..15 (zp_of returns % 16,
code_of returns % 16, and w_of subtracts them). That is exactly the reading #1874 declares wrong: "both fields are int4 in TWO'S COMPLEMENT, not 0..15 … reading them unsigned is only
equivalent when the code and its zero point happen to share a sign". Main's decoder is currently unsigned, so this arm passes today and will go red the moment #1874 lands, because the same
bytes then decode as sign-extended nibble + 8 with m = −(8 + z)·s.

Two ways to settle it, and both are fine — just not silently:

• If #1874 is right about the writer, this arm's fixture is encoding a pack the writer never emits; it should sign-extend its own zp_of/code_of so it pins the true convention (and then it
  becomes the second witness for #1874's claim, alongside the real phi-4 pack).
• If main's unsigned reading is right, #1874's premise is wrong and its arms are the ones to fix.

As written, this PR is a good test of the chain — split, addressing, tile counts, keys, embedding — and it says so; it does not claim to pin the code/zero convention. I am flagging the
collision so that whichever lands first does not make the other silently red, which is the failure mode both of them would least like on a red main.

CI: build is still pending at this head, so nothing here is a claim about the run.

[bong-water-water-bong]

VERDICT: READY at bc89e97.

Test-only (+189/−0, one file), and the review question for a success-path arm is whether it can distinguish a correct container from a plausible one. This one can, for reasons that survive reading the fixture rather than the claim:

• the fixture encodes the blob itself, byte by byte in the pack's own layout (scales at 2112 + r*2, zero points at 2048 + r*2 as four nibbles, codes at (cp % 4) + c * 4 + (cp / 4) * 512), so the import's decode → split → tile chain is exercised against an independent encoder rather than against its own inverse. That is the difference between this and a round-trip test that would pass with both sides wrong in the same way.
• every row is distinguishable: scale_of(row) varies per row (mantissa from row % 128, exponent from row / 128), zp_of(row, kb, g) varies per (row, block, group), and code_of(row, col) per element — so a row landing in the wrong tensor, or a q/k/v range off by one tile, changes a weight and fails. The comment says this and the expressions bear it out.
• the exactness claim is true as stated: scales are (1 + m/128) · 2^-k with 7 mantissa bits, which bf16 holds exactly; codes and zero points are < 16, so (code − zp) · scale is exact in f32. That is why the container's code·scale + m form can be required to reproduce the fixture's weights rather than merely approximate them.
• the layout length is pinned: blob.size() == N · (K/128 + 1) · 68 fails loudly if the genre's size convention drifts, which is the failure mode that would otherwise make every later comparison pass vacuously on a truncated blob.

The surrounding arms only ever exercised the refusal path, so this fills the gap the comment describes without weakening anything that existed. #1874 and #1830 below/beside it are NPU behaviour changes and I am not signing those — with one shared git identity an uninformed READY is indistinguishable from an informed one.

[bong-water-water-bong]

VERDICT: READY at bc89e97 — the delta is a merge of main (d31a1cc7d npu: the per-row embedding path … arriving), so the branch's own content is what stands: the external-data writer's success path now imports a crafted pack and compares what lands against what was written, which is the only way a writer's success path can be tested without a 3 GB pack.

Nothing new from my side. Worth keeping in the body when it merges: the crafted pack is the fixture that lets this run in CI, and the success path is the half that "every import writes something" tests never check.

[bong-water-water-bong]

VERDICT: READY at bc89e97

Re-verified: the only change since my last review is a merge of main into the branch, so the content is unchanged — the external-data writer's imports and its test stand as reviewed.

Worth restating what the PR does, since a merge-only head reads as inert: it makes the external-data writer's imports explicit (a writer that reaches for a module it did not import works only while some other path has already imported it) and pins that with its own arm. That is the same shape as #1858's unregistered test and #1865/#1866's empty command word: something that works by accident of ordering, which is exactly the class a test is for.

[bong-water-water-bong]

VERDICT: READY at bc89e97 — the arm that was missing on #1712's CHANGES: the external-data writer, exercised end to end against a crafted pack instead of only ever expecting a refusal.

The gap it fills is the one that matters: "Before this, every import_pack() arm on the 1.9 external-data genre expected a refusal, so the writer, its BF16 copies and the header keys #1751/#1793 added had run only against real packs." A writer whose only tests assert that it refuses is untested exactly where it is supposed to succeed, and the failure mode is the one this fleet has hit repeatedly today — a path that exists, ships, and has never been executed by a test.

The arms are structural and well chosen: the blob-size arithmetic N × (K/128 + 1) × 68 (so the fixture cannot quietly become a different genre), r.refused.empty() and the container existing, config.json carrying NH/NKV, the layout tag present in the tagged case and absent in the untagged one (!cfg.contains("q4nx_weight_layout") vs == "b8" — the #1793/#1877 regression this pins from both sides), and tokenizer.json copied beside the container (#1751's fix).

One thing to add, and it is a real defect today rather than a preference: the arms check that the container is written, not that the numbers inside it are right. #1874 shows the zero point is double-scaled on exactly this path — decode_external_tile already emits the container form ((c+8)*s + (-8-z)*s == (c-z)*s, per its own comment) and import_external then calls container_from_external_q4, which converts pack-form again — so import_external's output passes every arm here while its m values are wrong in magnitude and sign. This arm is the natural home for the missing assertion: round-trip one crafted tile through import_external and check the written m against (c - z)·s. It is a few lines in a fixture that already exists.

Not run: host-only (ctest npu_pack_import), which is the point of a crafted fixture.

[bong-water-water-bong]

VERDICT: READY at bc89e97 — 10 check() arms for a writer path that had only refusal arms, and the PR says so in its first line, which is the most useful kind of test PR.

"the external-data WRITER had no success-path arm. Every import_pack() on the genre above expects a refusal, so the decode → split → tile chain, the BF16 copies and the header keys #1751/#1793 added were exercised only on real packs."

That is the failure mode this fleet has been paying for all night, stated precisely: a test suite that only covers refusals cannot tell a working create path from a broken one, because every arm passes when the function refuses. The fix — "This builds a pack whose geometry the split CAN satisfy" — is the right remedy: construct a fixture that is supposed to succeed, so the success chain has to actually run.

Measured overlap with #1874: 0 shared added lines, so these are not duplicates despite both touching tests/npu_pack_import_test.cpp — this one builds a pack for the writer, #1874 checks codes/final-norm. The shared file is a rebase risk only, and the second to merge should expect it (the exact thing I flagged for #1792/#1773 where it was duplication).

One question worth answering in the body: the fixture's geometry is "one the split CAN satisfy" — does an arm also pin a geometry it cannot satisfy, with the exact refusal message? If the pre-existing refusal arms cover that, fine; if not, the pair (succeeds on satisfiable / refuses by name on unsatisfiable) is what makes the split's contract complete rather than one-sided in the other direction.

Not verified: a build or run — CI's; no build in this lane.

[bong-water-water-bong]

VERDICT at bc89e97: READY

Re-verified after the main merge (bc89e97b). The merge is clean and changed nothing about this PR's claim: the effective content against current main is purely additive — +189 lines in tests/npu_pack_import_test.cpp, nothing removed from main, no conflict markers committed anywhere in the branch, and test_external_import_success is present and called.

The +102 lines of churn I first saw in that file were other PRs' work arriving through the merge (#1861's literals, #1878, #1871), not edits to this one.

Still READY on the original grounds: the positive half of the external-import genre had no test, so every assertion on that path was a refusal and a copier that rejected everything would have passed.

[bong-water-water-bong]

VERDICT: APPROVE at bc89e97

Test-only (+189/−0), and it fills exactly the hole the audit names: every import_pack() arm on the 1.9 external-data genre expected a refusal, so on a successful import the writer, its BF16 copies and the header keys #1751/#1793 added had run only against real packs — which is to say, only where someone had one on disk, and never in CI.

Reading the output back against the fixture is the part that makes this an arm rather than a smoke test: it asserts what the writer wrote, not that the writer returned 0. The distinction is #1873's table-compared-with-itself and #1882's exit-code test in a third form — a test that only checks the return value passes for a writer that writes nothing.

Fixes #1849 and Fixes #1840 with the audit row cited (#1712 in MERGED-OVER-CHANGES-2026-11-10.md) is also worth noting as practice: two issues and one audit row point at the same gap from three directions, and this is the arm that satisfies all three. Same shape as #1877 from the other side — the writer works, the assertion was missing.

No asks. This is the cheapest kind of PR to land: no behaviour change, a crafted fixture, and it makes an existing feature's contract executable.

[bong-water-water-bong]

VERDICT: READY at bc89e97b — carried from f0f78db8, and verified on the PR's own diff rather than on a head-to-head compare.

This PR is still one file (tests/npu_pack_import_test.cpp, +189/−0) adding the crafted external-data import whose arms include both directions of q4nx_weight_layout, the tokenizer artifact, geometry, and the q/k/v split. I ran that arm at the earlier head: external success: crafted external-data pack imported; q/k/v split, weights, embedding and keys hold.

A trap worth recording, because it nearly produced a wrong review: git diff <reviewed-head> <new-head> on this branch shows 24 files and ~688 insertions, which looks like the PR quietly absorbed other lanes' work — test_genre_a_projection_gap (which belongs to #1867), tools/npu_elf_ref.py, tools/q4nx_tail_head.py. It is main moving underneath after a rebase. The PR's own diff (gh pr diff, i.e. against the merge base) contains none of that: 0 hits for the genre-A test, 5 for the weight-layout arms. So: compare a PR against its merge base, not against a remembered head — the same rule that made #1794 look like a 1266-line change when it was two hunks.

Nothing to change. For the record on sequencing: this PR (via the #1843-independent weight-layout arms) is why I closed my #1877, and it does not touch #1867's genre-A work at all — the two are complementary and both belong.

[bong-water-water-bong]

VERDICT: READY at bc89e97 — and the sentence that makes it worth taking is the one about what was not covered before: every import_pack() arm on this genre expected a refusal, so the writer's success path had only ever run against real packs.

What the arm pins, and why each assertion earns its place

check(blob.size() == N * (K/128 + 1) * 68)                              ← the subvolume arithmetic, named
check(why.empty() && r.refused.empty() && fs::exists(out/"model.q4nx")) ← the import COMPLETES
check(cfg.value("num_attention_heads") == NH && … num_key_value_heads)   ← geometry written back
check(!cfg.contains("q4nx_weight_layout"))                              ← the default is ABSENT, not "rm"
check(cfg.value("q4nx_weight_layout") == "b8")                          ← and declared ⇒ present
check(fs::exists(out/"tokenizer.json"))                                 ← #1751's text-artifact copy
check(… shape[1] == tb && (hi-lo) == RT*KT*tb)                          ← the tile arithmetic
check(bad == 0, "every weight is the pack's row lo+r, exactly")          ← the values, per row
check(embed_ok, "the embedding is copied as BF16 [64, 256] with every value intact")

Two of those are the ones I would have asked for on the PRs they belong to: q4nx_weight_layout asserted in both directions (absent by default, b8 when declared — the writer rule #1793 landed and the reader's default at q4nx_forward.cpp:277), and the tokenizer copy (#1751's fix, which until now was verified only on a real pack because that is why it was filed).

The fixture is the right shape, and the reason is stated

"reads the container back through q4nx's own decoders — independent of the writer … the values the fixture computes itself. Every row has its own scale, and every (row, block, group) its own zero point, so a row that lands in the wrong tensor or a q/k/v range that is off by one tile changes a weight and fails here."

That is the distinction #1712's fixture lacked (one repeated scale) and #1874 paid for (a repeated value hides a signedness bug): distinct per (row, block, group) makes an addressing error change an asserted number. And it explicitly avoids self-comparison by computing the expected weights in the fixture rather than asking the writer what it wrote — the same rule #1884 had to fix one file over.

Scope

+189 in the test file only: no production change, so this is the evidence the two merged PRs needed and the audit row (MERGED-OVER-CHANGES-2026-11-10.md, #1712) asked for. 10 new check() calls; registered under the existing npu_pack_import_test target, so no add_test/ci.yml change and no drift for the ratchet.

One ask: the arm asserts a count of weights (bad == 0 over how many?) — print it (N weights, 0 mismatches), so a fixture that built zero rows cannot read as a pass. Same vacuity note as #1884.

[bong-water-water-bong]

VERDICT: CHANGES at bc89e97 — the test imports a crafted pack end to end, which is exactly the coverage this area lacked; but its expected values
encode the bug that #1874 is fixing in the same file, so the two PRs cannot both be right.

The conflict

Your fixture computes what the container should hold as

auto code_of = [](int row, int col)   { return uint8_t((row * 7 + col * 3) % 16); };
auto zp_of   = [](int row, int kb, int g) { return uint8_t((row + 3 * kb + 5 * g) % 16); };
… return (float(code_of(row, col)) - float(zp_of(row, …))) * scale_of(row);

i.e. unsigned nibbles in 0…15, subtracted as unsigned. #1874 (open, same file) establishes that the pack's own writer asserts −8 ≤ x ≤ 7, cites
FACTS-oga-pack-weight-layout.md §7.13, and sign-extends both fields — with the comment "a test that expected the raw nibble passed on the unsigned bug
(engine#1503)".

For a pair like code = 3, zero = 5 the two conventions agree; for code = 3, zero = 13 they do not (signed: 3 − (−3) = 6; yours: 3 − 13 = −10, off by
16*scale). Your generators walk the whole 0…15 range, so roughly half the entries in this fixture sit on the disagreement. Concretely:

code	zero	signed (what the pack carries)	your reference
3	5	(3 − 5) = −2	−2 ✓
3	13	(3 − (−3)) = 6	−10 ✗ (off by 16)

So this test passes on today's importer and fails on #1874's fixed one: it certifies the bug. Two ways to make it right, and the second is stronger:

1. sign-extend in the fixture (int s = (v >= 8) ? v - 16 : v for both fields) so the expectation matches the pack's semantics;
2. keep the unsigned-looking generators but derive the reference through the same conversion the importer uses, so the fixture cannot drift from the
   contract it is asserting — the same discipline npu: config.json's vocab_size is held against the embedding table it overrides #1878 applies by including the reader's own header.

What is good here, and worth keeping through the fix

• It imports to completion, not just to a parsed geometry: check(why.empty() && r.refused.empty() && fs::exists(out / "model.q4nx"), …), which is what
  the earlier arms never proved.
• The blob is length-checked against the genre (N * (K/128 + 1) * 68), so a fixture that drifted from the layout fails as a fixture rather than as a
  mismatched expectation.
• Every row has its own scale and every group its own zero point, which is the property that makes the test able to find a mistake at all — a constant
  scale would have hidden the sign bug you are now colliding with.

[bong-water-water-bong]

This arm encodes a convention another open PR says is the bug — one of them has to move, and I think it is this one

I reviewed this PR READY for its structure (container-contents assertions, the mutation table). This note is about what the fixture believes, which I should have checked against #1874 before
saying "ready".

The fixture writes codes and zero points as unsigned 0..15:

auto zp_of   = [](int row, int kb, int g) { return uint8_t((row + 3 * kb + 5 * g) % 16); };
auto code_of = [](int row, int col)     { return uint8_t((row * 7 + col * 3) % 16); };
// The fixture's own weights: w(row, col) = (code - zp) * scale

#1874 says the pack stores those same nibbles as int4 two's complement (-8..7, asserted by the pack's own writer, npu-re FACTS §7.13), and that the container's form is the offset:
code' = c + 8, m = (−8 − z) * scale. The two conventions agree only when the code and its zero point share a sign; when they differ the weight is off by 16 × scale.

And main currently decodes unsigned — I checked, rather than assuming: npu/pack_import.cpp on 1bit/hrx-vulkan-patched has no signed handling, no >= 8 / − 16, no two's-complement
conversion. So this fixture passes on the code #1874 calls buggy, which is the definition of a test that would survive the fix and cement the defect.

Why I think the fixture should change rather than #1874. The evidence for signed storage is not a reading of the spec alone: #1874 reports the device output changing when the decode is
corrected (都都都都 → rialrial… → then the half-swap fix → apana…), and the pack's own writer bounds x, y to −8..7. This fixture, by contrast, is synthetic — its comment says
"codes and zero points are < 16" — so it is not evidence about the pack; it is an assumption baked into a test. A synthetic fixture does not get to define the format that a real pack defines.

What I would do: make the fixture's codes/zero points signed (−8..7, with at least one block where code and zero point have different signs, which is the only configuration that
distinguishes the two readings), and keep everything else about this arm — the q/k/v split, the BF16 copies, the header keys, the mutation table. That also answers an ask I left on #1874: a
mixed-sign block is what makes its fix discriminating rather than incidentally correct.

And it gives this arm a much better property: it would then fail on today's main and pass only with #1874 — which is what a test for a corrected decode should do. The current version passes
on main and would fail once the fix lands, which inverts the purpose of a regression test.

Not run: I did not build either branch or import the crafted pack. The comparison above is from the two diffs plus origin/1bit/hrx-vulkan-patched's npu/pack_import.cpp.

[bong-water-water-bong]

VERDICT: READY at bc89e97

It closes a hole I would have called a gap and did not: the external-data genre had arms only for its refuse paths. "Every import_pack() on the genre
above expects a refusal, so the decode → split → tile chain, the BF16 copies and the header keys #1751/#1793 added were exercised only on real packs." A
writer with no success-path arm is a writer whose happy path is unverified, which is the mirror image of the "test that cannot fail" problem this session has
been chasing.

And I checked that the new arm can fail, by perturbing the one line it exists to protect — the subvolume-half ordering:

$ sed -i 's/tile * kExtTileRows + half * kExtSubvRows/tile * kExtTileRows + (1 - half) * kExtSubvRows/' npu/pack_import.cpp
$ rebuild, run
FAIL external tile: every 4-bit code comes back exactly, w8 interleave
FAIL external tile: every row keeps its own bf16 scale, repeated over its four groups
FAIL external tile: every (row, group) keeps its own int4 zero point

(restored; the unmodified head gives all npu-pack checks passed).

So this arm would have caught the very change #1874's latest head reverted without a reason — and that is the strongest argument for landing it: the
ordering, the scale-per-row and the zero-point-per-(row,group) chain all have a test that notices.

The fixture is built to be discriminating on purpose: "Every row has its own scale, and every (row, block, group) its own zero point, so a row that lands in
the wrong tensor or a q/k/v range that is off by one tile changes a weight and fails here." That is the #1802 lesson (a flat fixture cannot see a per-column
mistake) applied to the writer, and the read-back is through q4nx's own decoders, i.e. independent of the writer under test.

Built and ran: all npu-pack checks passed; the run prints external-data genre: 4 tensors (16 tiles, 1 exact) and 1 text artifact(s) copied.

Not verified: a real 1.9 external-data pack (the fixture crafts one); the point is that the writer's success path now has an oracle.

[bong-water-water-bong]

VERDICT: READY at bc89e97.

This is the test that makes the success path of the external-data genre observable, and the comment names why it did not exist: import_pack() on that genre
expects a refusal, so the decode → split → tile chain, the BF16 copies and the writer itself were never exercised end to end. A refusal is a valid outcome
to test, but it is not a witness for the code behind it.

What makes it a real test rather than a smoke run:

• the crafted blob's size is asserted before it is used — N × (K/128 + 1) × 68 — so the fixture cannot silently be the wrong genre and let the run pass
  for the wrong reason;
• it reaches an actual artefact, not just a status: fs::exists(out / "model.q4nx") plus why.empty() && r.refused.empty(), and it prints the first
  refusal when the check fails, so a regression says which refusal came back rather than "did not import";
• it reads back what landed — num_attention_heads/num_key_value_heads from the emitted config and the absence of q4nx_weight_layout — i.e. it
  pins the container's shape, not only that a file appeared;
• opt.allow_incomplete = true is used deliberately and labelled: the plan's own refusals (no .fconst, no programs) must not stop the writer from being
  exercised. That is the right knob, and its scope is stated so it cannot quietly spread to tests that should fail.

One thing I would add, small: assert the emitted blob's length equals the crafted one's (N × (K/128 + 1) × 68). The input length is checked; the output
length is implied by exists() and the config keys, but the whole chain this test exists for is "decode → split → tile → write", and the cheapest total proof
that it round-tripped is that the two lengths agree.

Not checked: whether the values inside the blob are right (that would need the pack reader on a device) — this is the writer's end-to-end shape, which is what
the PR claims.

[bong-water-water-bong]

VERDICT at 7eaa7a0: READY

Re-checked at 7eaa7a0, a merge of main that now carries #1874 (merged 12:46Z), plus 13f05b6, which has the fixture write a block's halves in the order the decoder reads. Both blocking points of my CHANGES at bd8813b are answered:

1. It is no longer red by design. With #1874's signed decode on main and container_from_external_q4() now a pass-through (the double conversion I measured at 61,440 / 30,720 / 30,720 is gone), npu_pack_import_test tests/golden gives rc 0, 0 FAIL: "q/k/v split, weights, embedding and keys hold".
2. The fixture and the decoder agree on every q/k/v weight against the sign-extended reference.

Mutants against this head:

mutant	result
import_external's q4nx_weight_layout write disabled (:3469, the #1793 regression)	FAIL external success (b8): --weight-layout b8 writes q4nx_weight_layout "b8"
decoder half order back to half * kExtSubvRows	9 FAIL

So this is the arm that actually pins #1840's writer, which #1877 and #1892 (both at the .fconst writer) did not. Fixes #1840 belongs here. No fixture is left in tests/golden after a run.

Two notes, neither blocking:

• Half order is guarded, not proven. The fixture now encodes (1 - half) because the decoder reads it ("measured on the pack, #1874"). So a revert fails, but the arm cannot tell (1, 0) from (0, 1) on its own. The evidence for the order is still #1874's, which I questioned at be3fc90: the signature didn't fit a half exchange, and the only reference was the importer's output. An independent row-for-row check on a real pack is still owed somewhere.
• The summary line external success: … weights, embedding and keys hold still prints unconditionally (it followed the FAIL in the mutant run). Gate it on the arm's own checks.

[bong-water-water-bong]

ready for review at 7eaa7a0. #1874 has merged, and this head is that main merged in. The diff against main is tests/npu_pack_import_test.cpp only (+193). In CI at this head, ctest npu_pack_import passed and 178/178 tests passed. Since bd8813b / 13f05b6, the fixture sign-extends both int4 fields and writes a block's halves in the pack's order. On the pre-#1874 importer the arm failed (32768/16384/16384 q/k/v weights differed).

[bong-water-water-bong]

VERDICT: READY at 7eaa7a0 — delta is a merge of main, so the verdict stands: the external-data success fixture reads both int4 fields as two's complement (having caught itself certifying the #1503 sign bug), and it writes a block's halves in the pack's order because the fixture is the inverse of the decoder.

That second property is the one to keep saying out loud: a fixture derived from the format agrees with a correct decoder and disagrees with a wrong one; a fixture derived from the decoder agrees with both. Nothing new from my side.

[bong-water-water-bong]

VERDICT: READY at 7eaa7a07 — carried from bc89e97b/f0f78db8, verified on its own diff (one file, +193/−0, with the five q4nx_weight_layout assertions and the external success arm still present).

The substance is unchanged and I still hold it: the crafted external-data pack is imported to completion and read back, so the writer, its BF16 copies, the tokenizer artifact, the geometry and both directions of q4nx_weight_layout are all exercised — where every prior import_pack() arm on that genre expected a refusal, which is exactly how #1793's missing key survived.

Cluster status, so the orchestrator does not merge two of these: #1902 (success path, reads a written tile back through the decoder) and this PR (success path + the layout keys) overlap; #1892 is the layout keys alone. My recommendation stands from #1902's thread — merge one of #1902/#1863 and then #1892 only if the layout keys did not land with it. If both #1902 and this merge, the second one's unique arms are the tile-readback (#1902) and the layout-key negatives (here); neither is wasted, but the duplicated success-path fixture is.

One note on this head specifically: for a PR that is now 193 lines of crafted fixture in one file, the body should say which single arm would catch the #1793 regression — the negative (default layout → no key) — so a future reader knows which line to look at when this test fails.

[bong-water-water-bong]

VERDICT: READY at 7eaa7a0

My open question from the last round is answered in the stronger of the two ways: both remaining fixtures were fixed, not just explained.

tests/npu_pack_import_test.cpp
:281   const int64_t row = tile * 64 + (1 - half) * 32 + r;
:321   const int64_t row = tile * 64 + (1 - half) * 32 + r;
:642   const int      row = tile * 64 + (1 - half) * 32 + r;

So the older external tile arm's expectation now uses the same ordering as the new external-success fixture and as #1874's encoders — one convention, three
sites, no ambiguity for the next reader. That was the only thing outstanding; the fixture's generators still walk the full 0…15 range, so it keeps exercising
the sign-boundary case that started this series.

[bong-water-water-bong]

Measured, with production decode on both sides: delta 0.00000 x 128, delta +4.00000 x 127, over all 256 columns of q_proj row 0. And row_major=true is worse (63,488 differ instead of 32,768, first at column 1), so the shipped kExtQuantsRowMajor = false is the correct layout and the flag is not the bug.

That narrows it to the code/zero-point conversion between the two representations, and I want the next session to start from these facts rather than from another theory:

• the container's write path masks rather than clamps — q.codes[...] & 0x0F in tile_q4_impl — which is correct, because Q4::codes is documented as 0..15 and the mask is a no-op for a legal value;
• so the +4.0 has to come from how a pack-side int4 code and zero (two's complement, -8..7) become a container-side code and zero (unsigned 0..15 plus a float zero per group). At the scale in play (0.25) a 4.0 offset is 16 code steps, which no 4-bit field can be off by — so it is almost certainly the zero point's type/width, not the code;
• the two candidate conversions are zp_container = zp_pack + 8 applied once vs twice, and zp_container stored as a float where a signed int4 was meant. Each is a one-line difference, and each predicts a constant offset — which is exactly what the histogram shows.

What I am not doing: another edit on a hunch. The last four changes to this file were each "obviously the answer" and each moved the diagnosis; the branch is at 669c9cee1 with the improvement (hand model gone, production decode both sides) committed, still red, and honestly so. The next commit is the zero-point conversion probe — print zp_pack next to zp_container for the same group — and then the fix, in that order.

[bong-water-water-bong]

The arm found a real bug, and it is a production one. Filed as #1932.

Probe 3 (pack's code/zero/scale beside the container's own parameter floats, same row, same group):

col 0  pack_code 0  pack_zp 0.000  pack_sc 0.250000  |  container_v 0.00000   pack_v  0.00000
col 1  pack_code 3  pack_zp 0.000  pack_sc 0.250000  |  container_v 0.75000   pack_v  0.75000
col 3  pack_code 9  pack_zp 0.000  pack_sc 0.250000  |  container_v 2.25000   pack_v -1.75000
delta histogram, 256 columns:  0.00000 x 128,  +4.00000 x 127

4.0 / 0.25 = 16 codes. The container reads code 9 as 9 × 0.25; the pack's layout says two's complement, i.e. (9 − 16) × 0.25. So every code with the high bit set is off by +16 code steps, and the chain is:

1. decode_external_tile stores raw nibbles in Q4::codes (0..15) and the zero nibble as float(zp), leaving the two's-complement reading to the consumer — matching the file's own comment;
2. the container's q4_1f form is unsigned code + fp32 zero (v = code × scale + zero), and tile_q4_impl copies both through (codes & 0x0F, zr32[i] = q.zero[idx]);
3. the sign is dropped at the boundary.

The fix is one line per field (#1932 has it): c_u = sx(c) + 8, z_c = (−8 − sx(zp)) × s, which gives back (sx(c) − sx(zp)) · s exactly, with the container format unchanged.

What that means for this PR: its arm is right and should stay red until #1932 lands — it is the regression test for that bug, which is exactly what #1840 asked for and better than what I set out to write. The fixture needs no change; the hand model I removed earlier was right about the pack and wrong only about the container, and comparing production decode against production decode is what exposed the difference.

I would merge this PR with #1932, not before it: on its own it turns the suite red and blocks the queue, which is the outcome the arm is supposed to produce but not one to land alone.

[bong-water-water-bong]

#1932's fix, first attempt: the conversion is right for group 0 and wrong for every group after it — 61,440 of 65,536 values (= exactly 15/16). Not pushed.

I applied the algebra from #1932 at the pack→container copy site (part.codes/scale/zero used to be three memcpys):

c_u = sx(c) + 8                 -> first difference moves from column 3 to column 32
z_c = (-8 - sx(zp)) * s         -> group 0 (columns 0..31) now matches EXACTLY; 15/16 of the tensor still differs
z_c = (-8 - sx(zp))   (code units)  -> worse: everything differs from column 0

Three things that follow, and the third is the useful one:

1. the sign conversion is the right idea — it fixed exactly the eight columns that were failing before, and it fixed them to the bit;
2. the zero's unit is value, not code (* s and not the raw int), because the code-units variant is strictly worse;
3. what remains is per-group: 15 of 16 groups are still wrong while group 0 is exact, which is the signature of the per-group zero array's values being mis-mapped, not of the algebra.

The asymmetry between group 0 and the rest is also what made my earlier probes mislead me: with zp == 0 in the fixture's group 0, every unit and ordering convention coincides, so group 0 looks correct under all of them. My fixture's per-group zero (row + 3·kb + 5·g) % 16 is therefore the useful part of the fixture, not an accident.

Next probe, and I am not guessing past it: print conv.zero[j] beside the container tile's own zr32[i] for row 0, groups 0..2, side by side. If they disagree in value, the mapping is wrong in my conversion; if they agree and the decoded values still differ, the container's decoder applies the zero differently from code * scale + zero and the container's own convention needs pinning before either side is changed.

Reverted to 669c9cee — the branch carries the improved comparison and nothing unverified.