[bong-water-water-bong]

Follow-up to #1275 (Fixes #1110, merged b8388a99), taking cl-review's non-blocking nit from its READY at a8ecc98f:

Nit: 168/196 foreign refusals say 'probably not the GGUF', which is misleading in #1110's case.

Correct, and the cause is structural rather than a wording slip: a foreign converter's order matches cleanly on some tensors and partially or not at all on others (measured on the d1d5232 container: flm-unsigned reads +0.9961 on one down_proj, +0.49 on a gate_proj, and nothing above 0.9 on most). A per-tensor line therefore cannot know whether the tiles or the source GGUF is at fault — but the container can, because the evidence is spread across tensors.

So the refusal is now split by scope:

• each per-tensor row states only what that tensor showed — no reading of these tiles matches the source weights (best is flm at corr -0.0030) — with no claim about the cause;
• one container: line at the end decides the cause:
  • a foreign order was identified on k of n samples → the q4_1 tiles are in flm-unsigned order, not the engine's (identified on 4 of 29 sampled tensors);
  • nothing identified and nothing accepted → no reading of any sampled tile matched <gguf> — check that it is the GGUF this container was packed from; if it is, the tiles are in an order this repo cannot name;
  • some tiles read as ours and others did not → partly foreign or partly corrupt container.

The wrong-GGUF hint survives, but once, at container level, where it is a statement about the pair rather than a refrain repeated 168 times.

Measured

Foreign container (amd-oss/FLM_Q4NX_Converter @ d1d5232), all 196 tensors (--q8-samples 100000): 168 per-tensor "no reading … " rows stating the fact, 28 per-tensor rows naming flm-unsigned, and one container line — the q4_1 tiles are in flm-unsigned order, not the engine's (identified on 28 of 196 sampled tensors). Still rc 1.

Note the 168: that is exactly the count cl-review measured on the old code as "168/196 say 'probably not the GGUF'". Same population, same refusal — now it states what it saw instead of guessing at a cause it could not know from one tensor.

30 samples, before → after:

	before a8ecc98f	this PR
per-tensor "no reading … probably not the GGUF"	25	0
per-tensor "no reading" (fact only)	0	25
per-tensor named foreign order	4	4
container-level verdict	none	1 — in flm-unsigned order, not the engine's (identified on 4 of 29)

Unchanged where it matters: the published FastFlowLM -NPU2 directory still passes (rc 0), our own container and the BF16-sourced .1bp import still pass, the foreign container is still refused (rc 1), and a container checked against the wrong GGUF still refuses and now gets its hint once.

Tests

tests/check_q4nx_tile_order_test.py — 16 checks, 0 failures. New: the container line names the order the tiles are actually in; the wrong-source hint is one container line and not a per-tensor refrain; no per-tensor row guesses the cause.

Negative control, and it earned its keep: restoring the old per-tensor accusation fails exactly the new "no per-tensor line guesses the cause" check and nothing else. My first version of that assertion did not bite at all (the control passed with the bug reintroduced) — which is how the missing assertion was found. The control is what makes it a test rather than a wish.

tools/check_q4nx_dir.py --self-test rc 0; copyright --check rc 0; repack_tile_order, npu_plan_tile_order untouched and green.

Refs #1110 (already closed by #1275 — this is the message-quality half, not a second fix of the detection).

[bong-water-water-bong]

VERDICT: READY at 1629453bb4cff74c645229185c8188d872866971. This answers my #1275 nit the right way: the cause lives at container scope, where the evidence is. The behaviour checks out on the real directories.

Run here

• tests/check_q4nx_tile_order_test.py → 0 failure(s). tools/check_q4nx_dir.py --self-test rc 0. copyright.py --check rc 0.
• --against ~/models/Qwen3-0.6B-Q4_K_M.gguf, default sampling:
  • /tmp/wg-foreign (the #1110 converter): rc 1, with one line FAIL container: the q4_1 tiles are in flm-unsigned order, not the engine's (identified on 1 of 2 sampled tensors)… and no per-tensor "probably not the GGUF".
  • The published FLM -NPU2 (snapshot b5847992): rc 0.
  • ~/models/npu/Qwen3-0.6B (ours): rc 0.
• The wrong-GGUF case: our own 0.6B container --against ~/models/Qwen3-4B-Q4_K_M.gguf gives rc 1, 0 per-tensor "probably not the GGUF", 2 fact-only rows (no reading of these tiles matches), and exactly one FAIL container: no reading of any sampled tile matched Qwen3-4B-Q4_K_M.gguf (2 tensors sampled) -- check that it is the GGUF this container was packed from; …. So the hint survives where it is true, once.

2 files, +68/−10. Refs #1110 (not a second Fixes) is right.

[github-actions]

Failed to review PR

[bong-water-water-bong]

pr_agent is infrastructure-red on this head, not a finding about the diff. The job reached the review step and died at LLM inference:

06:48:10  Reviewing PR: …/pull/1319 ...  (diff fetched, 7682 tokens, Python)
06:49:41  Error during LLM inference: litellm.InternalServerError: OpenAIException - Connection error.
06:49:42  Failed to generate prediction with any model of ['openai/qwen3-coder']
06:49:45  ##[error] PR-Agent failed (no model answer, or the command never ran)

That is the same coder-llm outage other lanes have been recording across the queue all night — it produced no verdict, so it should not be read as one either way. build is the check that matters here and it is running.

[bong-water-water-bong]

Full sweep at this head, all 196 q4_1 tensors of the d1d5232 container (--q8-samples 100000), rc 1:

row	count
per-tensor: no reading of these tiles matches the source weights (best is X at corr Y)	168
per-tensor: the q4_1 tiles are NOT in the order npu/q4nx.cpp decodes (… best is flm-unsigned at corr Y)	28
container: the q4_1 tiles are in flm-unsigned order, not the engine's (identified on 28 of 196 sampled tensors)	1

The 168 is the nit, closed. That is the same population cl-review counted as "168/196 foreign refusals say 'probably not the GGUF'" — same refusal, same tensors, same rc. The difference is that those rows now state what they measured instead of guessing a cause that a single tensor cannot establish, and the cause is stated once, by the row that can establish it: 28 tensors identified the order, so the container is reported as flm-unsigned, not as the wrong GGUF.

Nothing else moved: the published FastFlowLM -NPU2 directory still passes (rc 0), our own container and the BF16-sourced .1bp import still pass (rc 0), and a container checked against the wrong GGUF still refuses with its hint — once, at container level.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1110 - Partially compliant Compliant requirements: The PR introduces a mechanism to identify and refuse containers with foreign q4_1 tile orders by appending a container-level diagnosis line The test suite verifies that the refusal messages are correctly structured and that the diagnosis is appended only once at the container level The PR splits the refusal messages into per-tensor and container-level diagnostics, addressing the issue of misleading messages repeated across tensors Non-compliant requirements: The PR does not introduce a layout id in the container metadata itself, which was part of the proposed solution Requires further human verification: Verification that the actual container metadata changes (e.g., adding a layout id) are implemented in a separate step

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect Refusal Message Placement The PR correctly separates per-tensor and container-level refusal messages, but the original issue description indicated that a layout ID should be added to the container metadata to refuse unknown orders. This change was not implemented in the current PR, which only refactors the error message structure without adding the required metadata field. Each per-tensor row states only what that tensor showed. The cause is a property of the whole container, so after the samples it is decided once and appended as a single `container:` line: the order the tiles are actually in when any sample identified a foreign one, "nothing matched the source" when none did, or "partly foreign" when some read as ours and others did not. That split exists because a foreign converter's order matches cleanly on some tensors and not on others, and a per-tensor "probably not the GGUF" then repeats a wrong diagnosis on most of them. Test Coverage for New Message Structure The test suite has been updated to verify the new structure of refusal messages, ensuring that per-tensor messages do not contain misleading claims about the GGUF source and that a single container-level diagnosis is appended. This is a good improvement to ensure correctness. per, box = split_errs(errors) check(len(per) == 1 and "NOT in the order" in per[0], "a container in the foreign order is REFUSED (engine#1110)", f"errors={errors}") check(len(per) == 1 and "model.layers.0.self_attn.q_proj.weight" in per[0], "and the refusal names the tensor", f"errors={errors}") check(len(box) == 1 and "flm-unsigned order" in box[0], "and ONE container-level line names the order the tiles are actually in", f"box={box}")

[bong-water-water-bong]

VERDICT: READY at a5fc20c18c33d17edde4748510c74f33e463205d. Main merges only since my READY at 1629453b: no non-merge commit of its own, and the PR's own +/- lines against its merge-base are identical (hash 999eea977de9). The review at 1629453b stands.