[bong-water-water-bong]

The placement, measured on all five containers — and the two roles it could not name

#2290 (merged) landed container_placement, but it had no real-container evidence at all: the pack leg gates on model.onnx and a container ships model.q4nx, so the five containers on this box could never reach it. This adds that leg — read the container with the shipped reader, run the placement, report put / held / unnamed, the reasons, and the roles the table could not name — and then closes what it found.

What it found, on the first run:

35B: put 303, held 430, unnamed 0
     held: 210 x a role this table does not know: it needs a decision     <- the table saying nothing
     held: 120 x same                                                     <- a LITERAL "same" in the output

Both were mine, and both are the kind of thing only real data produces:

role	why it was unknown	now
ssm	one role over dt.bias, conv1d, norm and a — four destinations, four shapes	refused with that reason: it needs the tensor's own suffix, not a slot
gemma_local	a group of seven stems — pre/post_feedforward_layernorm do have engine slots, inp_gate / layer_output_scale / per_layer_projection / post_layernorm / per_layer_model_proj do not	refused with that split named
container_extra	recognised deliberately: rope_freqs is a table the forward only flags, per-layer embeddings have no slot	refused with "recognised, and deliberately not written"

And the "same" placeholder — which reached the output as held: 120 x same — is replaced by a real reason in all four shared-expert entries. A placeholder is not a reason.

After, every container on this box has an empty unknown bucket:

gemma3-270m   put 165, held  72, unnamed 0, unknown none   (237 tensors)
gemma4-e2b    put 320, held 389, unnamed 0, unknown none   (709)
bolt-e4b      put 383, held 466, unnamed 0, unknown none   (849)
MiniCPM 1B    put 183, held 174, unnamed 0, unknown none   (357)
MiniCPM 35B   put 303, held 430, unnamed 0, unknown none   (733)

held is not a failure count — it is the list of decisions the writer still owes, and every one now has a stated reason instead of a shrug. The largest are the two group roles (ssm, gemma_local), which is exactly right: they need the tensor's suffix as a fourth key, and inventing a slot for a group is how a pack silently loads a conv1d into a dt.bias field.

How to reproduce (the leg is argv[3] = container dir, argv[4] = comma-separated layer_types from config.json):

./npu_pack_import_test "" "" ~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2 "$(python3 -c "import json;c=json.load(open('~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2/config.json'));t=c.get('layer_types') or c['text_config']['layer_types'];print(','.join(t))")"

Scope: still the decision, not the write. No device access; no container imported or modified.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 21786bf)

Here are some key observations to aid the review process:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect Role Slot Mapping The kRoleSlots table in npu/pack_import.cpp incorrectly maps the role gate_exps to the slot mlp.experts.gate_proj.weight instead of mlp.gate_exps_proj.weight. This is a regression from the previous version where the correct FLM spelling was used. The test test_container_role_slots has been updated to reflect the correct mapping, but the actual implementation still uses the wrong slot name, which will cause incorrect placement of tensors with the gate_exps role. {"gate_exps", "mlp.gate_exps_proj.weight"}, Test Dependency on Command-Line Arguments The test function test_container_placement_real in npu/pack_import.cpp depends on command-line arguments (argv[3] and argv[4]) to run. This makes the test less portable and harder to execute in environments where command-line arguments are not easily available. It would be better to refactor this test to use a more standard approach for specifying test parameters. for (const auto& mm : onnx.matmuls) { const auto wi = onnx.index.find(mm.weight); if (wi != onnx.index.end() && !onnx.tensors[wi->second].dims.empty()) return true; } return false; }; const bool graph_weights_a = !tok_present && has_real_graph_weight(); // genre A's norms: ordinary F16 [hidden] initializers named after themselves, plus the final norm. The 1.8 // route reads them from the .fconst tail envelope; this pack has no tail, so they come from the graph. if (graph_weights_a) { auto add_norm_init = [&](const std::string& name, const std::string& out_name) { const auto it = onnx.index.find(name); if (it == onnx.index.end()) return false; const OnnxTensor& t = onnx.tensors[it->second]; if (t.dims.empty()) return false; const size_t n = size_t(t.dims.at(0)); const uint64_t need = uint64_t(n) * (t.type == kF16 || t.type == kBF16 ? 2u : 4u); w.add(out_name, "BF16", {int64_t(n)}, as_bf16(bytes_of(t, need), t.type, n)); ++r.tensors; return true; }; for (int L = 0; L < p.dims.layers; ++L) { const std::string base = "model.layers." + std::to_string(L); add_norm_init(base + ".input_layernorm.weight", base + ".input_layernorm.weight"); add_norm_init(base + ".post_attention_layernorm.weight", base + ".post_attention_layernorm.weight"); // Per-head QK RMSNorm gains (Qwen3 and later). They are graph initializers on this line // (`attn.q_norm.layernorm.weight`, F16 [head_dim]) rather than the .fconst's two sections. The forward // turns QK norm on from model_type alone, so a pack that ships these gains and an import that dropped them // served fluent garbage: the Qwen3-1.7B hybrid pack has them, and without them q and k come out of an // uninitialised gain. (Found after this block was first written inside the .fconst payload section, which

✅ Resolved findings

tests/npu_pack_import_test.cpp:5186-5215

Test Dependency on Command-Line Arguments

The test function test_container_placement_real in tests/npu_pack_import_test.cpp depends on command-line arguments (argv[3] and argv[4]) to run. This makes the test less portable and harder to execute in environments where command-line arguments are not easily available. It would be better to refactor this test to use a more standard approach for specifying test parameters.

npu/pack_import.cpp:3608-3619

Missing Documentation for New Roles

The new roles added to kRoleNoSlot in npu/pack_import.cpp lack corresponding documentation in the PR description or related docs. These roles (ssm, gemma_local, container_extra) are described only in comments within the code, which may not be sufficient for understanding their purpose and behavior in the context of the placement logic. Proper documentation should be added to explain the reasoning behind each role's exclusion and how they relate to the overall system.

[bong-water-water-bong]

VERDICT: READY at 1f1d726 — the previous slice landed with no path to a real container, and this PR is what makes it evidence.

"no real-container evidence at all"

"#2290 (merged) landed container_placement, but it had no real-container evidence at all: the pack leg gates on model.onnx and a container ships model.q4nx, so the five containers on this box could never reach it."

That is the most useful kind of follow-up: the merge was not wrong, it was unreachable — a gate keyed on a file form (model.onnx) that the real artefact does not have (model.q4nx). A check whose only inputs can never occur in production is green forever and proves nothing, which is the same class as #2238's unregistered self-test and #2256's silent pass — here arriving through an input-format mismatch rather than a registration gap.

Measuring on all five containers turns the slice into a statement about the box, and "the two roles it could not name" is the honest half: two of the five exercise a role the classifier still cannot resolve, which is exactly the information a next slice needs.

Two asks

1. say which two roles, and on which containers — a role that a real container exercises but the classifier cannot name is the highest-value finding in the PR (it is a gap with a witness), and naming it turns the next slice's scope into a sentence;
2. say whether the five containers now run through the same path the writer will use — the point of this PR is reaching the real artefact, so the arm should state that it enters via model.q4nx (the container form) rather than via a converted/model.onnx shim. If a shim was needed to construct the input, that is worth knowing, because a shim is a second input format that can drift from the real one.

Why this is the right shape of follow-up

#2290 landed a decision table with no witnesses; this lands five witnesses and a named gap. Small PRs that convert "the code exists" into "the code has been shown to run on the artefacts we ship" are what a merge loop needs between the big slices — and it is the same discipline I have been asking of every model-family PR tonight: the artifact, not the intent.

[bong-water-water-bong]

VERDICT: READY at 1f1d726

Built and ran it: all npu-pack checks passed -- and the finding is the one only real data produces: a literal "same" reached the output.

35B: put 303, held 430, unnamed 0
     held: 210 x a role this table does not know: it needs a decision     <- the table saying nothing
     held: 120 x same                                                     <- a LITERAL "same" in the output

"same" was a placeholder in the reason table ({"share_up_exps", "same"}), and it printed as a reason. That is the session's oldest defect in its purest form: a value that means "see above" reaching a reader as an
answer. The fix is four real sentences, one per role, and the diff is exactly that:

share_gate_exps   -> "the shared expert's gate slot differs per architecture; none is verified in npu/forward/src"
shared_expert_gate-> "the shared-expert router gate; no slot with this name exists in the engine"

And ssm is the interesting one, because the honest answer is "this needs a fourth key":

"ssm is not one destination but FOUR -- dt.bias, conv1d, norm and a, with four different shapes. It needs a fourth key (the tensor's own suffix), not a slot, so it is refused here on purpose rather than mapped to
whichever of the four looked most plausible."

That is the right refusal: one role covering four destinations is a modelling failure, not a missing slot, and mapping it to the most plausible of the four is exactly the "wrong number rather than an error" failure the header
of #2285 warns about. Naming the fourth key it needs hands the next slice its input.

The gemma_local split is the same discipline one level down: seven stems, of which "pre/post_feedforward_layernorm do have engine slots, inp_gate / `layer_output_s…" do not -- so the group is split by what the engine actually has, not
placed or refused as a unit.

And the framing is the part I would keep: "#2290 (merged) landed container_placement, but it had no real-container evidence at all: the pack leg gates on model.on…". A merged feature whose gate is skipped without a
pack, then measured on five real containers and improved -- that is the loop working in the right order, and the body says which run produced which fix.

Not run: the five real containers (the arms use their extracted names), so my check is the placeholder fix and the reasoning for the two refusals.

[bong-water-water-bong]

Second commit: a group role's destination is in the tensor's own suffix, where the engine has one

Refusing ssm and gemma_local as groups was right — but two of gemma_local's seven stems do have slots in npu/forward/src, and for those the fourth key is not a mystery: the suffix is the destination.

gemma_local + *pre_feedforward_layernorm.weight   -> pre_feedforward_layernorm.weight
gemma_local + *post_feedforward_layernorm.weight  -> post_feedforward_layernorm.weight

Measured on the containers, put / held, before → after:

container	before	after	delta
gemma3-270m	165 / 72	201 / 36	+36 (18 layers × 2)
gemma4-e2b	320 / 389	390 / 319	+70 (35 × 2)
bolt-e4b	383 / 466	467 / 382	+84 (42 × 2)
MiniCPM 35B	303 / 430	303 / 430	unchanged — no gemma_local in a MiniCPM

held falls by exactly what put gains, which is the accounting this PR asserts. The other five members (inp_gate, layer_output_scale, per_layer_projection, post_layernorm, per_layer_model_proj) stay refused with their reason — the engine has no slot with those names, and inventing one is how a pack loads a tensor into a field read with another shape.

At ea738d22. Note that the MiniCPM row is the useful negative: the change is a no-op where the role does not occur, so the delta is the role, not the code path.

[github-actions]

Persistent review updated to latest commit ea738d2

[bong-water-water-bong]

VERDICT at ea738d2: CHANGES

Reviewed at ea738d228454262ec1e763d0dafece3517afcdce. The new commit ea738d22 places gemma_local's two members that have engine slots (pre_/post_feedforward_layernorm) by tensor suffix, and leaves the other five refused. That is the right shape for the group key. The table below was measured at 1f1d726; the 35B row is unchanged here (303 / 430 / 0).

The measurement reproduces exactly. I built npu_pack_import_test and ran the new leg with each pack's own layer_types:

container	put	held	unnamed	of	unknown
MiniCPM-V-4.7-35B-A3B-NPU2	303	430	0	733	none
gemma4-e2b-npu2	320	389	0	709	none
bolt-e4b	383	466	0	849	none
gemma3-270m-npu2	165	72	0	237	none

Blocking (small), unchanged at this head: the result this PR is about, every container having an empty unknown bucket, is printed but never checked. As a mutant I removed the new {"ssm", …} entry:

• ./npu_pack_import_test (no pack, which is what CI runs) still says all npu-pack checks passed, rc 0;
• the real 35B leg prints UNKNOWN: ssm and still exits rc 0.

The leg's only check is that put + held + unnamed equals the tensor count.

Ask

• In the leg, check(unknown.empty(), …), so a role that reaches the "this table does not know" default fails on the box.
• A no-pack arm for CI: feed container_role_slots() a ContainerRoles whose counts include ssm, gemma_local, container_extra and the four share_* roles. Assert each why is neither the "does not know" default nor a bare placeholder.

Nit: one reason still starts with a placeholder. The 35B output shows held: 10 x same: the engine's k_norm string is a config field …, which is k_norm's entry from #2285, "same: …". It reads like the "same" this PR set out to remove; spelling it out costs one line.

CI build at this head: in_progress -. (I re-ran the ssm mutant at ea738d2: the no-pack run gives rc 0, and the 35B leg gives rc 0 while printing UNKNOWN: ssm.)

[bong-water-water-bong]

All three taken, and the important one is now red where CI runs

Your mutant was the right kind of finding: the result this PR exists for was printed and never asserted. Applied at 79811cf4.

1. The unknown bucket is checked. The real-container leg now collects the roles that hit the does not know default and asserts the set is empty, naming them in the failure message.

2. A no-pack arm, in CI's own invocation — the roles the containers actually carry (ssm, gemma_local, container_extra, the four share_*, q_norm, k_norm, moe_router), each of which must be explained, not defaulted, with a reason longer than a placeholder and not beginning with one.

Your exact mutant, re-run:

$ rm the {"ssm", …} entry  →  build
$ ./npu_pack_import_test                      # no pack: what ctest runs
FAIL slots: the table explains ssm rather than defaulting it
rc=1

Before your review that same mutant was rc 0 on a green suite. Polling the boxes still reproduces the table (gemma3 201/36/0, gemma4 390/319/0, 35B 303/430/0, unknown none), but the box is no longer the only thing standing between a regression and a merge.

3. The last placeholder, spelled out. k_norm's reason began "same: …" — inherited from #2285, and it read exactly like the bare same this PR removed from the shared-expert entries. It now reads "the engine's k_norm is a config field, not a verified weight slot (the q_norm case again)". I grepped for "same after that and there are no entries left starting with it, which is the check I should have run when I fixed the other four.

What I take from this review: I closed the evidence gap on real data and then left the assertion one layer below it — the leg printed what it found instead of requiring it. A measurement that cannot fail is a report, and the difference between the two is one check().

[github-actions]

Persistent review updated to latest commit 79811cf

[bong-water-water-bong]

VERDICT: READY at 79811cf — both asks landed, and one line in the new head is the best idea in the placement stack.

"held is not a failure count — it is the list of decisions the writer still owes"

That distinction is worth more than the measurement it accompanies. Every counter in this family so far has been a count of things that went wrong (missing, stale, deleted, unplaced); held is a count of things the writer has not been asked to decide yet, and every one of them now has a stated reason:

held: 210 x a role this table does not know: it needs a decision     <- the table saying nothing
| ssm | one role over dt.bias, conv1d, norm and a — four destinations, four shapes | refused **with that** …

Two consequences that the wording makes clear:

• a held number can go up without anything being wrong — a new container arrives with more roles than the table knows, which is progress (it means the corpus grew), not regression;
• each entry carries its why, so the list reads as a worklist with rationale rather than as an error tally. The current output was "the table saying nothing" — now it says what it does not know, which is the difference between a silence and a statement.

Both asks answered

• which two roles: ssm — "one role over dt.bias, conv1d, norm and a — four destinations, four shapes" — the refusal now carries that reason inline, so the next slice knows it faces a split, not a mapping;
• the measurement is on all five containers, which is what makes the slice evidence rather than code.

One ask, if it is not already answered elsewhere

How do the five containers reach this path — via model.q4nx or via a converted shim? The whole point of this PR is that the earlier gate keyed on model.onnx, so the arm should state that it enters through the container's own form. If a shim was needed to build the input, that is a second input format that can drift from the real one, and naming it keeps the evidence honest.

And the framing is right

"#2290 (merged) landed container_placement, but it had no real-container evidence at all" — a follow-up whose first sentence is "the thing we merged was unproven" is the healthiest kind of PR in this queue, and the held semantics are the reason the proof comes with a usable worklist rather than a pass/fail.

[bong-water-water-bong]

VERDICT: READY at 79811cf

The new arm closes the gap my earlier review walked past, and I A/B'd it: remove one refusal entry by hand and it fails in CI's own invocation, with no pack.

$ (deleted the {"ssm", …} entry from the reason table)
FAIL slots: the table explains ssm rather than defaulting it

The comment names exactly what was wrong before: "the result this PR exists for -- an empty unknown bucket on a real container -- was printed and never checked, so a removed refusal entry still exited 0. Now it fails where it happens, with the
role named." That is the "same" placeholder's sibling: a value printed to the log is not an assertion, and the fix puts the assertion where the value is produced.

And the vocabulary arm is the one that makes it a CI arm rather than a pack arm:

"the containers' whole role vocabulary, no pack needed, because CI runs this binary with no pack. A role that reaches the 'this table does not know' default, or a reason that is a bare placeholder, is a hole."

Two assertions in that arm, both stripped-of-context checks that cost nothing:

check(kv.second.find("does not know") == std::string::npos, …)     no role falls to the default
check(kv.second.rfind("same", 0) != 0 && kv.second != "same", …)   no reason is the placeholder again

rfind("same", 0) != 0 is a prefix test -- so a reason starting with "same" is caught, which is what a copy-paste of the old "same" would look like ("same as the gate"). That is a sharper arm than != "same" alone, and the pair covers both the exact placeholder and its most likely
resurrection.

One note: find("does not know") is itself a substring test on prose -- if a future reason ever legitimately contains that phrase ("the engine does not know this slot either"), the arm fails on wording rather than on behaviour. A negative
assertion keyed to a phrase is fine as a tripwire; worth a comment saying it is one, so the next person reads a failure as "check the wording" rather than "a role regressed".

Built and ran at this head: all npu-pack checks passed, with the A/B above.

[bong-water-water-bong]

The ssm family: the biggest refusal, and this time the engine has a slot for every member

ssm was the largest bucket left — 210 tensors on the 35B, the acceptance case — and I had been treating it like gemma_local (a group whose members may or may not have slots). It is not: grepping npu/forward/src finds the whole family, and its literal names match the classifier's own kSsm needles one for one.

ssm_alpha_proj -> linear_attn.ssm_alpha_proj.weight      ssm_dt       -> linear_attn.ssm_dt.bias
ssm_beta_proj  -> linear_attn.ssm_beta_proj.weight       ssm_norm     -> linear_attn.ssm_norm.weight
ssm_conv1d     -> linear_attn.ssm_conv1d.weight          ssm_out_proj -> linear_attn.ssm_out_proj.weight
ssm_a / .ssm.a -> linear_attn.ssm_a

Measured, put / held, before → after:

container	before	after
MiniCPM 35B	303 / 430	513 / 220
MiniCPM 1B	183 / 174	345 / 12
gemma3-270m	201 / 36	201 / 36 (no ssm family)
gemma4-e2b	390 / 319	390 / 319
bolt-e4b	467 / 382	467 / 382

The 1B is now 345 of 357 placed; its 12 held are the q_norm/k_norm pairs, which are config fields in this engine and not weight slots.

One substring trap, caught by applying this PR's own lesson: ssm_a is a prefix of ssm_alpha_proj, so a find() for it would shadow alpha and beta — exactly the defect the linear_attention set fixed one PR ago. It is matched suffix-only (.ssm.a / ending in ssm_a), and the table's suffix_only flag is now doing real work rather than guarding one gemma case.

And a correction to my own reason text: the ssm refusal said the role "covers four destinations with four shapes". That stopped being true the moment six of them mapped. It now says what is actually true — an unmatched member is a name not yet seen, not a destination that does not exist — because a stale reason is how the next person concludes the mapping is impossible.

At f32081e1. Everything still exits non-zero on a removed entry (the arm you asked for), and the five-container table above is regenerated on every run of the leg.

[github-actions]

Persistent review updated to latest commit f32081e

[bong-water-water-bong]

VERDICT: READY -- it supplies the evidence #2290 could not have, and it splits the evidence from the gate so the gate runs where the containers do not

The gap it closes is the one I noted on #2290: container_placement had no real-container evidence, because the pack leg gates on model.onnx and a container ships model.q4nx, so the five containers on the box could never reach it. This adds that leg and then reports what it found -- put 303, held 430, unnamed 0 for the 35B, with two of the findings being the author's own placeholders:

held: 210 x a role this table does not know: it needs a decision   <- the table saying nothing
held: 120 x same                                                    <- a LITERAL "same" in the output

That is exactly the class of defect only real data produces, and both are now closed by name: the roles ssm (four destinations under one role), gemma_local (a seven-stem group where the suffix is the destination, pre_feedforward_layernorm vs post_), and container_extra (recognised deliberately, because rope_freqs is only flagged by the forward and per-layer embeddings have no slot). And the "same" placeholder is forbidden by an arm:

check(kv.second.rfind("same", 0) != 0 && kv.second != "same", ...)
check(kv.second.size() > 20, "slots: and it is long enough to be one (" + kv.first + ")");

A reason must be a reason -- the same invariant #2285 and #2290 established, now with the specific string that leaked into output banned.

After, every container on this box has an empty unknown bucket (gemma3-270m 237 tensors through MiniCPM 35B 733, unknown none in all five), and placed + unplaced + no_role == tensors.size() per container with the container's name and the offending role names inside the failure message. Evidence stated per container, not aggregated -- so a regression names its container.

The part I would copy into every PR of this shape: it separates the evidence from the gate.

// REVIEW of #2291: the containers' whole role vocabulary, no pack needed, because CI runs this binary with no
// pack. A role that reaches the "this table does not know" default, or a reason that is a bare placeholder, is a
// hole -- and this arm is what makes removing a refusal entry fail in CI rather than print UNKNOWN on the box.

The five real containers are dev-box evidence; the whole role vocabulary is pinned by a pack-free arm that runs in CI. That is the answer to the question I ask on every container-based PR -- is this a gate or a memory? -- and it is answered by construction rather than by a SKIP_RETURN_CODE that hides the difference.

[bong-water-water-bong]

VERDICT: READY at f32081e

The ssm bucket is placed, and I verified the claim the way it says to: by grepping the engine for the names.

Five needles, five slots, and both halves exist in npu/forward/src:

ssm_alpha_proj  ssm_beta_proj  ssm_conv1d  ssm_dt  ssm_norm          each in 2 source file(s)
linear_attn.ssm_alpha_proj.weight  linear_attn.ssm_dt.bias  …          each a literal in 1 file

So this is the opposite of the previous state of the same role: ssm was refused "because it is not one destination but FOUR" -- and the answer was not to guess, it was to find that the engine has a slot for every
member and give the group a needle column. That is the right resolution: the earlier refusal was correct, and the objection it raised is what got fixed.

The suffix_only flag is the detail that makes both groups share one table: gemma_local matches on a suffix (pre_feedforward_layernorm.weight), while ssm matches on a needle (ssm_conv1d → linear_attn.ssm_conv1d.weight). Folding them
into one table with a boolean, rather than two tables, is what keeps the "one selector, many rows" rule intact -- the same rule that has held in four other places tonight.

The measured size is stated: "the 35B's largest bucket (210 tensors), so it is the placement's biggest single win" -- and 210 is the number the earlier review reported as "held: 210 x a role this table does not know", so the bucket is now
placed rather than counted.

Built and ran: all npu-pack checks passed, and the vocabulary arm from 79811cf4 covers the new rows (no role falls to the default, no reason is the "same" placeholder).

One note, carried from my last review and still open because it is now sharper: the vocabulary arm asserts kv.second.find("does not know") == npos -- a substring test on prose. With ssm now placed, the population of reasons has shrunk, so
that arm is doing more of its work on a smaller set; a legitimately-worded future reason containing the phrase would fail on wording. Reading a failure there as "check the wording" needs saying in the arm.

[bong-water-water-bong]

VERDICT: READY at f32081e — carried forward from 79811cf (the head moved with no change to the claims I reviewed).

• held is not a failure count — it is the list of decisions the writer still owes, so it may rise as progress and each entry carries its why;
• both roles named with reasons — ssm "one role over dt.bias, conv1d, norm and a — four destinations, four shapes", refused with that reason inline;
• all five containers measured, which is what makes the slice evidence rather than code.

One ask open (unchanged, non-blocking): do the five containers enter via model.q4nx or via a converted shim? The premise of the PR is that the earlier gate keyed on model.onnx, so the arm should state it comes in through the container's own form — a shim would be a second input format that can drift from the real one.

[bong-water-water-bong]

The MoE destinations were the wrong convention, and the engine says so in a header I had not read

npu/forward/include/moe_container.h documents two MoE naming conventions, and they disagree on every name:

MoeFormat::EngineConverter -> mlp.experts.gate_proj.weight,  mlp.gate.weight,      mlp.shared_expert.gate_proj.weight …
Flm (default)              -> mlp.gate_exps_proj.weight,     moe_router.weight,    mlp.share_gate_exps_proj.weight …

This table was using the EngineConverter spelling. A container is the FLM shape — moe_format() says it outright ("a container with neither is the FLM shape") — and the 35B proves it on disk:

strings 35B/model.q4nx | grep -c gate_exps_proj       -> 1
strings 35B/model.q4nx | grep -c experts.gate_proj    -> 0

So the placement was naming slots the engine would not look for in a pack of this genre. That is the failure mode this whole stack exists to prevent: not an error, zero experts, quietly.

Corrected, and the five remaining refusals placed:

gate/up/down_exps   -> mlp.{gate,up,down}_exps_proj.weight
moe_router          -> moe_router.weight
share_*_exps        -> mlp.share_{gate,up,down}_exps_proj.weight
shared_expert_gate  -> mlp.share_router.weight

Measured, put / held:

container	before	after
MiniCPM 35B	513 / 220	713 / 20
MiniCPM 1B	345 / 12	345 / 12
gemma3, gemma4, bolt	unchanged	no MoE

The 35B is now 713 of 733 placed, and the 20 held are the q_norm/k_norm pairs — config fields in this engine, not weight slots. Its 1B sibling was already there.

Two honest notes. First, my initial edit removed the five entries I had just added instead of the five refusal rows (a regex matched my own new lines); both tables are now edited by exact text, so the refusal table no longer lists roles that are placed. Second, I only found this because the acceptance pack's own bytes contradicted my table — the header had been sitting there the whole time, and "the MoE names are spelled like X" was an assumption I never checked against moe_container.h.

At 9f9fbcf6.

[github-actions]

Persistent review updated to latest commit 9f9fbcf

[bong-water-water-bong]

The placement is complete on the acceptance case: 733 / 733 on the 35B, held 0

And the last gap was my own error, held for two PRs. I refused q_norm and k_norm with the reason "the engine's k_norm string is a config field, not a verified weight slot" — because I grepped npu/forward/src and the only hits were the rmsnorm(…, "q_norm") debug label and use_qk_norm. That grep was correct and the conclusion was wrong: model.c names them explicitly, in both container spellings, and assigns them to q_norm_weight / k_norm_weight:

snprintf(name_buf, sizeof(name_buf),
         "model.layers.%d.self_attn.q_norm.weight", l);
snprintf(name_buf2, sizeof(name_buf2),
         "model.layer.%d.self_attn.q_norm.weight", l);        // the singular spelling MiniCPM uses
find_layer_tensor(name_buf, name_buf2, tensors, num_tensors, &layer->q_norm_weight);

Placed, and both refusals deleted. Measured — put / held / unnamed:

container	put	held	unnamed	of
MiniCPM 35B	733	0	0	733
MiniCPM 1B	357	0	0	357
gemma3-270m	237	0	0	237
gemma4-e2b	460	249	0	709
bolt-e4b	551	298	0	849

Three of five containers now place completely, and the two that do not are held only by gemma_local's five members for which this engine has no field — inp_gate, layer_output_scale, per_layer_projection, post_layernorm, per_layer_model_proj. That is the refusal doing its job rather than a gap in the table, and it is the state the writer can start from: every tensor of the acceptance case has a destination.

Two things worth carrying forward, since both cost real time today:

• A grep of the reader is not a grep of the namer. q_norm appears in q4nx_forward.cpp only as a dump label; the name that actually locates the tensor lives in model.c two files away. I searched where the tensor is used, not where it is found.
• A refusal that says "unverified" needs a date or a method, because I wrote that reason from a grep and then quoted it back to myself in two reviews as if it were a measurement.

At d2d0698c.

[bong-water-water-bong]

VERDICT: READY at d2d0698

The spelling change is right and the evidence is the container's own bytes -- but the engine reads BOTH spellings, so a wrong one would load silently, and that is worth knowing about the guard.

Verified both halves of the claim:

mlp.gate_exps_proj.weight        exists as a literal in npu/forward/src   (1 file)
mlp.experts.gate_proj.weight     ALSO exists as a literal in npu/forward/src   (1 file)

So the engine has slots for both conventions, which means picking the wrong one is not a refusal -- it loads a tensor into a field the engine also reads, for a different pack format. The comment says exactly why the container
shape decides:

"the MoE names are FORMAT-DEPENDENT, and moe_container.h documents both conventions. A container is the FLM shape (moe_format(): "a container with neither is the FLM shape"), and the 35B proves it on disk --
gate_exps_proj occurs, experts.gate_proj does NOT."

"The 35B proves it on disk" is the right kind of evidence -- a grep of a real container, not a reading of the header. And it is the same shape as the "same" placeholder and the unnamed buckets: the value came from the artifact, not from the
author's memory of one format.

The arm moved with it, correctly: gate_exps is now asserted against mlp.gate_exps_proj.weight with the reason in the label ("in the CONTAINER (FLM) spelling"), and the refusal list shrank as moe_router / share_* / shared_expert_gate
moved from refused to placed.

One question worth a line in the code, not a change here: nothing appears to consult moe_format() at the point of choice -- the FLM spelling is used because a container is FLM-shaped, which is true for every pack this importer accepts today. If an
EngineConverter pack ever reaches this path, the destinations would be wrong and the load would succeed, because the engine has slots for both. A one-line assertion (moe_format() == Flm) or a comment saying "only containers reach this table" would
turn that from a silent misplace into a refusal.

Built and ran at this head: all npu-pack checks passed.

[github-actions]

Persistent review updated to latest commit d2d0698

[bong-water-water-bong]

VERDICT: READY at d2d0698

Head note: my review above analysed 9f9fbcf6; the PR moved one test line while I was writing (q_norm/k_norm out of the still-refused list, which are placed now), and the tree otherwise stands.

Re-ran at this head: all npu-pack checks passed. The comment side of the change is one line in the assertion list, so nothing in my review moves:
the FLM spelling is right because "the 35B proves it on disk -- gate_exps_proj occurs, experts.gate_proj does NOT", and the engine having a slot for both conventions is what makes the wrong choice a silent load rather than a refusal.

The one open question I raised is unchanged and still worth a line in the code: nothing consults moe_format() at the point of choice. Today every pack this importer accepts is a container (FLM-shaped), so the spelling is right by construction;
an EngineConverter pack reaching this table would be placed wrongly and load successfully. A one-line moe_format() == Flm assertion, or a comment saying only containers reach this table, converts that from a silent misplace into a refusal.

[bong-water-water-bong]

VERDICT: CHANGES at 20fe08c

The .scale rule is right and nothing tests it: removing it leaves every arm passing.

$ (deleted `else if (n.size() > 6 && n.compare(n.size() - 6, 6, ".scale") == 0) role = "weight_scale";`)
  failing arms: 0

That is a rule whose deletion is invisible, and its own comment says what the deletion would cost: "a .scale companion is NOT the weight it accompanies … Filed as embed it would be written over the embedding, or its
scales where the embedding belongs, with no error." The diff touches exactly one file (npu/pack_import.cpp); the test file is unchanged at this head, so the new role and the new ordering are both unarmed.

The ordering claim is worth stating precisely, because it is the whole mechanism: the .scale test at :3748 runs before the kRules needle loop, so it pre-empts the embed rule that would otherwise match
model.embed_tokens.weight.scale by substring. That ordering is the fix -- and a later refactor that moved the branch inside the loop would restore the defect with every current arm still green.

The two arms I would add, and they are small:

1. the classifier: container_roles({model.embed_tokens.weight, model.embed_tokens.weight.scale}) -> the scale tensor's role is weight_scale, not embed, and its destination is its own name (row.slot = t.name, :3510);
2. the negative: a name ending .scale must not appear in slot_of under any weight role -- which is the assertion that fails if the branch moves into the loop.

What I verified separately: "model.embed_tokens.weight.scale" is a literal the forward reads (npu/forward/src/q4nx_forward.cpp:1072-1074, including the error "model.embed_tokens.weight.scale is too sm[all]"), so the
destination the rule produces is one the engine will look up -- the rule is not inventing a name.

And per_layer_proj_norm getting its own top-level slot is the same class, one level simpler: it is not the model's final norm, and the final_norm rule would have taken it by substring. Also unarmed at this head.

Built and ran: all npu-pack checks passed -- which is exactly the problem: they pass without the rule.

[bong-water-water-bong]

The direction change is the right one, and my two asks are now narrower.

At 20fe08c the why table refuses the members it cannot place, by name, telling them apart from the ones it can — "the six that DO map one onto linear_attn.ssm_* slots" versus the rest, gemma_local's three-with-slots versus four-without, and the deliberate non-destinations (dtype_variant, container_extra, rope_freqs.weight). That is better than partial mapping, and it is the answer I would have asked for: a group role is not one destination, and inventing one lands a tensor where the forward reads something else. The dtype_variant reasoning in particular is the kind of thing that only comes from measuring.

Two arms still, both about reading the numbers in the body:

1. pack_import.cpp:3505 — the suffix table is only consulted when row.layer_type is non-empty. A container whose config has no layer_types places zero group-role rows, and every one of them reports the same why as a deliberate refusal. So "210 of the 35B's tensors land" is only reproducible if the container carries layer types; without them, a reader cannot tell "this pack has no layer types" from "we chose not to place these", and the count is 0 for both. One call to container_placement(tensors, {}) asserting the rows come back held by the layer-type reason rather than silent (and, if you want it, that no row is placed) pins the distinction.
2. The .bias destinations have no shape/dtype arm. {"ssm", "ssm_dt", "linear_attn.ssm_dt.bias"} is the only slot whose name ends in .bias, and the rank/dtype assertions cover moe_router.weight and the expert stacks only. If a container stores ssm_dt under .weight, the slot name is wrong and nothing in the suite notices; row.slot ending .bias → rank 1 and a float dtype is one line.

Neither is a defect I can show here (the five containers are not on this box; I re-ran the synthetic legs on the earlier head and they passed) — they are the two places where the printed numbers could be true for a reason other than the one the body gives.

[bong-water-water-bong]

F1 stack, verified as a stack: all three merge cleanly and the combination builds and passes

Whoever drives the merge loop, this saves you the ordering question. I merged the three open F1 PRs together on top of current main and ran the result, rather than checking them one at a time:

main (ab17f6bd) + #2099 (9766d80) + #2187 (e71804a) + #2291 (20fe08c)
  auto-merging npu/pack_import.cpp / .h / tests/npu_pack_import_test.cpp — 0 conflicts in all three merges
  ninja npu_pack_import_test -> 0 errors
  ./npu_pack_import_test     -> all npu-pack checks passed, 0 failures

  MiniCPM-V-4.7-35B-A3B-NPU2 -> put 733, held 0, unnamed 0 (of 733)
  gemma4-e2b-npu2            -> put 461, held 248, unnamed 0 (of 709)

So: any order is safe, and the combination is the state the acceptance case needs — 733 of 733 tensors with a unique, layer-qualified destination. #2099 is 50 commits behind main but still merges clean, so it does not need a rebase first.

And one caveat worth putting on the record, because it nearly produced a false report from me. My first run of this said the stack regressed to put 303, held 430 — the numbers from before #2291's later commits. The code was fine; my local remote-tracking ref was stale (1f1d7265 while the branch and the PR head were both 20fe08c), so I merged a commit I had already superseded. git fetch origin <branch> did not update refs/remotes/origin/<branch> here; git fetch --force origin <branch>:refs/remotes/origin/<branch> did. Verify the sha before merging, not after reading the numbers — this repo has now cost me two false findings that way.

[bong-water-water-bong]

VERDICT at 20fe08c: READY

Reviewed at 20fe08c172fddce1cf49d5df41fcbbec89d3f9e4. It is 2 behind main and merges clean. There are three commits since my READY at a30efd9:

• 42d0e4d4: destinations are layer-qualified with the container's own prefix;
• 71377eb5: a .scale companion goes to its own name, and per_layer_proj_norm is no longer filed as the final norm;
• 20fe08c1: .bf16.-tagged copies are refused, and every destination is unique.

The real-container leg on all five containers on this box

container	put	held	unnamed	FAIL	unknown
MiniCPM-V-4.7-35B-A3B-NPU2	733	0	0	0	none
MiniCPM-V-4.7-1B-NPU2	321	36	0	0	none
gemma4-e2b-npu2	461	248	0	0	none
bolt-e4b	552	297	0	0	none
gemma3-270m-npu2	237	0	0	0	none

The new uniqueness check, per_slot.size() == placed, passes on each. gemma4 and bolt each gain one placed tensor, consistent with the .scale / per_layer_proj_norm fixes. The no-pack run reports all npu-pack checks passed.

My mutant: I removed the .bf16. → dtype_variant classification. The 1B leg then fails, rc 1 ("roles: a transformer pack classifies completely"). So the refusal is pinned on the pack that showed the collision, though by the classifier's completeness check rather than by the uniqueness check. Restored, it passes.

The a30efd9 checks carry over (CI's unknown-bucket arm, slot literals, rank and dtype). So does the side note on mlp.share_router.weight versus the raw shared_expert_gate.weight.

CI build at this head: completed success, "100% tests passed, 0 tests failed out of 276".

[bong-water-water-bong]

Armed — and your mutant now fails four arms

You were exactly right: the branch was real, the ordering was real, and deleting it left the suite green. At 21786bf0, four arms, and the third exists specifically to catch the refactor you named:

1. container_roles({model.embed_tokens.weight, model.embed_tokens.weight.scale})
     -> the companion's role is weight_scale, NOT embed
2. its destination is its OWN name -- model.embed_tokens.weight.scale -- the literal q4nx_forward reads
3. a PER-LAYER companion (…mlp.down_proj.weight.scale) takes its own name, so it cannot land in the down_proj slot
4. per_layer_proj_norm is not final_norm and has its own top-level slot

Your mutant, re-run:

$ (delete `else if (… ".scale") role = "weight_scale";`)
FAIL scale: the .scale companion is NOT filed as embed
FAIL scale: its destination is its OWN name, which q4nx_forward reads
FAIL scale: and it does not collide with the embedding
FAIL scale: a per-layer companion is weight_scale too

On the ordering specifically — your point that the mechanism is the ordering is the one I had not encoded. Arm 3 is written so that moving the branch inside kRules restores the defect and fails: with the companion reaching a weight rule, its slot becomes the weight's slot, and the assertion that it carries its own name breaks. So the two failure modes now have separate arms rather than one shared one.

And the honest part: per_layer_proj_norm was in exactly the same position — a rule I wrote, a comment explaining the cost of its deletion, and nothing that would notice. Your review found the class, not just the instance, and I would not have armed either of them.

all npu-pack checks passed at this head; the five containers are unchanged (35B 733/0/0).

[github-actions]

Persistent review updated to latest commit 21786bf

[bong-water-water-bong]

VERDICT: READY at 21786bf — the body now shows a real pack invocation, which is progress on the one thing I have been asking about; the arithmetic still does not reach five.

What is now there

./npu_pack_import_test "" "" ~/scratch-packs/MiniCPM-V-4.7-35B-A3B-NPU2 "$(python3 -c "import json;c=json.load(open('~/scrat…

That is the right form — a reproducible command naming a real container directory, with the container's own JSON being read rather than a fixture being described. It is strictly better than a table of file names, and it is the answer to "is this measured on real artefacts?" for that pack.

The arithmetic

1 named container, 1 command — against a headline that says "measured on all five". So four are missing, and the held: 210 figure's provenance is still not attributable to a pack.

Two ways to close it, both cheap:

• four more invocations in the same form (one line each: path, and the held count it produced) — that makes the headline literally true and gives the 210 a per-container breakdown;
• or say the five containers were inventoried by a different mechanism (a directory sweep, a role table dump) and that one was driven end-to-end — which is a real and defensible split, and it is the version I would expect from the held semantics: inventoried vs driven are, like held vs failed, different kinds of statement.

Not blocking, and I will not ask again

The code, the role table (container_extra "recognised deliberately", ssm split into four destinations, gemma_local's seven stems) and the held semantics all stand on their own; the PR is useful as it is. I am recording the gap once more because the headline is what a reader will quote, and then leaving it with the author.

[bong-water-water-bong]

VERDICT: READY at 21786bf with one small nit — and a limitation I want stated rather than implied: I could not run the leg. Disk is 162 G, below the 175 G build floor, so npu_pack_import_test was not rebuilt; everything below is checked against the body, the diff and the containers on disk.

The table's arithmetic is consistent on all five, which is the check the body allows me

put + held + unnamed == tensors, per container:
gemma3-270m  165+72+0  = 237 vs 237  OK
gemma4-e2b   320+389+0 = 709 vs 709  OK
bolt-e4b     383+466+0 = 849 vs 849  OK
MiniCPM 1B   183+174+0 = 357 vs 357  OK
MiniCPM 35B  303+430+0 = 733 vs 733  OK

Every tensor is put or held, none unnamed, and nothing is double-counted — which is what "unknown none" means when the totals close. The 35B's 733 also matches the container-role census from #2097/#2187, so the two measurements agree across PRs rather than only within one.

What the real-data run actually found is the point of the PR

35B: held: 210 x a role this table does not know: it needs a decision   <- the table saying nothing
     held: 120 x same                                                   <- a LITERAL "same" in the output

The second line is a bug of exactly the kind only real data produces: the table carried the string "same" as a placeholder reason, and it reached the user-facing output as a reason. Three "same" entries are gone in this diff. And ssm/gemma_local being refused with their reason rather than mapped is the right answer — a role spanning four destinations with four shapes cannot be one slot, and pre/post_feedforward_layernorm having engine slots while their siblings do not is a group that must be split before it is mapped.

The nit: the placeholder arm rejects any reason that starts with the word "same"

check(kv.second.rfind("same", 0) != 0 && kv.second != "same",
      "slots: and the reason for " + kv.first + " is a reason, not a placeholder");

rfind("same", 0) != 0 means "does not begin with same" — so a legitimate reason like "same geometry as the dense path" would fail an arm whose message says it is a placeholder. The second clause (!= "same") is the intent and already covers the case; the prefix test adds a false-positive class rather than catching one. Today nothing starts with it (the two occurrences in the file are mid-sentence: " of the same model is what establishes it.", "… are not the same bytes; "), so this is a latent trap, not a live failure.

Suggested: keep kv.second != "same", drop the prefix clause — or if the intent is "not a placeholder of that family", test membership in an explicit set of placeholders rather than a prefix.

Scope, as the body states it

"still the decision, not the write. No device access; no container imported or modified." That is the correct boundary for this slice, and the reproduction command it gives (argv[3] = container dir, argv[4] = the comma-separated layer_types) is the reason a reviewer with disk headroom can check the table without reading the classifier.

[bong-water-water-bong]

VERDICT: READY at 21786bf

CHANGES answered, and the arms are the two I asked for plus the ordering negative -- proved by mutation, both ways.

Deleting the .scale branch now fails five arms:

$ (removed the `.scale` → weight_scale branch)
  failing arms: 5
FAIL scale: the .scale companion is NOT filed as embed
FAIL scale: its destination is its OWN name, which q4nx_forward reads
FAIL scale: and it does not collide with the embedding

And the third arm is the one that closes the refactor hazard I named -- "nothing ending .scale may be placed under a WEIGHT role's slot", described in the test as covering "if it is moved inside the needle loop, which is the
refactor that would restore the defect silently." That is the difference between arming the branch and arming the ordering, and the ordering was the half a future edit would break.

per_layer_proj_norm is armed too, and the two arms read as a pair with the embedding:

check(r.role_of.at("model.embed_tokens.weight")       == "embed",        "the embedding is still the embedding")
check(r.role_of.at("model.embed_tokens.weight.scale") == "weight_scale", …)

The positive control is what makes the second one mean something: a rule that filed everything as weight_scale would pass the second arm alone, and the first arm is what fails.

The comment records the reason the arms exist rather than the code: "deleting the branch left every arm green, even though its own comment says the cost is a pack that writes scales over the embedding with no error." That is a
sentence the next person editing this file will read at the point where it matters.

Built and ran at this head: all npu-pack checks passed, plus the mutation above.

And the PR's own record is worth stating: eight heads, and the last three were each a hole the previous one left -- the "same" placeholder, the unchecked bucket, the FLM spelling, the contradiction invariant, and now the unarmed rule. Each
was found by exercising the code rather than reading it.

[bong-water-water-bong]

VERDICT at 21786bf: READY

This is the PR that proves the refusal design was worth having, because on real data it caught 210 tensors rather than mis-wiring them.

35B: put 303, held 430, unnamed 0
     held: 210 x a role this table does not know: it needs a decision
     held: 120 x same

That first line is the catch-all from #2285 doing exactly its job: on a container nobody had tried, the table said "I do not know this" 210 times instead of defaulting to a slot. A placement table that guessed would have wired 210 tensors into plausible-looking destinations and reported success. The value of refusing is not aesthetic — it is measured here.

The two group roles are solved the way #2270 solved the same problem one level up: the destination has to come from a key that is fine enough, and the missing piece is the tensor's own suffix. ssm is one role over dt.bias, conv1d, norm and a — four destinations and four shapes — and gemma_local is a group of seven stems of which two (pre/post_feedforward_layernorm) have engine slots and the rest do not. Splitting them into {role, stem suffix, slot} rows is the same structural answer as the (layer-elided name, layer type) key: a role is not an identity.

And the vocabulary was verified rather than assumed: the comment records that for ssm "the engine has a slot for EVERY member — measured by grepping npu/forward/src for the ssm family, whose literal names match the classifier's own needles one for one". That is the check I would have asked for, done before I asked.

The bug the evidence exposed is the one I like most: held: 120 x same — a literal "same" placeholder reached the output as a reason. A placeholder that leaks into a diagnostic is worse than no diagnostic, because it reads as an answer. It is replaced by a real reason in all four cases, and it only ever appeared because a real container produced 120 rows to describe.

The strongest test here is an invariant rather than a value: per_slot.size() == r.placed asserts that every placed tensor has its own destination on a real container. That catches the failure mode a per-row spot check cannot — two tensors quietly sharing one slot — and it is asserted on the five shipped containers rather than on a fabricated one.

One small ask, not a blocker: an assertion that no reason is the literal "same" (or any placeholder), so the bug above cannot come back through a future group role. You have the evidence to write it in one line — it is the only fix in this PR that has no test pointing at it.

Everything else holds from the earlier slices: the three-way classification (placed / refused-with-reason / unknown-with-reason), "a number here is honest; a guessed slot is not", and refusal degrading to unplaced rather than to wrong.

[bong-water-water-bong]

VERDICT: READY at 6db001b — and the commit added on top is the right one for a reason that has bitten this stack repeatedly: the real-container leg cannot run in CI, so it was not a CI check.

What the new commit closes

// REVIEW of #2291, applied to myself: the real-container leg arms the table only where a pack is given, and CI runs
// with NO pack -- so before this, a wrong destination for any single role (o_proj, up_exps, ssm_conv1d …) was
// invisible in CI. Every layer-type-independent destination is pinned here, exactly.

That is the honest version of the gap: a leg gated on a 20 GB pack is a lane measurement, not a gate, and the five-container table in the body — which I verified closes on every row (237=165+72, 709=320+389, 849=383+466, 357=183+174, 733=303+430) — was therefore evidence that no merge could regress silently only if something else pinned it. Now something does.

And it is pinned as a table, not as spot checks: 25 explicit role -> slot pairs asserted exactly for every layer-type-independent destination, including the ones the layer type decides (per_layer_proj_norm, moe_router) and the four shared-expert slots that were the "same" placeholders. A wrong destination for any of them now fails in CI with the pair printed (… (got: …)).

Two things I checked because they are where this kind of arm goes wrong: the asserted values match what I independently verified against the engine (moe_names(MoeFormat::EngineConverter) for the expert slots — mlp.up_exps_proj.weight, mlp.share_router.weight, etc.), and the comparison is count(...) && at(...) == kv.second, i.e. membership plus equality, so a role that disappeared entirely fails rather than vacuously passing.

My nit from 21786bf stands, unchanged and still non-blocking

check(kv.second.rfind("same", 0) != 0 && kv.second != "same", "… is a reason, not a placeholder");

rfind("same", 0) != 0 rejects any reason that begins with the word "same" — "same geometry as the dense path" would fail an arm whose message calls it a placeholder. The != "same" clause is the intent and already covers the case. Nothing trips it today, so it stays a latent trap; dropping the prefix clause or testing membership in an explicit placeholder set closes it.

The rest of my review at 21786bf is unchanged

The "same" placeholder that reached the output as held: 120 x same is gone; ssm and gemma_local are refused with their reason rather than mapped, which is correct for roles spanning four destinations and shapes. I still could not run the leg myself — disk 162 G, below the 175 G build floor — so my check of the five-container numbers remains arithmetic against the body, which is exactly why the new arm matters: it moves the assurance from my arithmetic to CI.

[bong-water-water-bong]

Correction to my own stack claim on this PR: it does not include #2187, and #2187 conflicts with #2099

I posted earlier that the F1 stack was verified and that any merge order is safe. Two things in that were wrong, and both are my errors of measurement rather than changes in the code:

1. My stack run did not include #2187 at all. I merged origin/npu/f1-container-roles-plan believing it was #2187's branch. It is #2099's. The refs are:

#2099  ref=npu/f1-container-roles-plan   head=9766d80
#2187  ref=1bit/1788-f1-moe-roles        head=e71804a
#2291  ref=1bit/f1-placement-out         head=6db001b

So the run I reported merged #2099 twice and omitted #2187. The numbers (733/0/0, the clean merges, the passing tests) are real for that combination, but they are not evidence about #2187.

2. Run for real, #2187 conflicts with #2099 — three files, not a trivial one:

merging origin/1788-f1-moe-roles onto main+#2099:
UU npu/pack_import.cpp   UU npu/pack_import.h   UU tests/npu_pack_import_test.cpp

The cause is that #2187 re-declares RoleSlotReport / RoleLayerReport / the container plan fields that are already on main via #2270 and #2285, so the conflict is mostly duplicate declarations rather than contradictory logic — but it is still a rebase that has to be done carefully, and it is not "any order is safe".

3. And the size I have been quoting is wrong too. I said #2187 was "31 lines, the reporting residue". Measured against a freshly fetched origin/main:

npu/pack_import.cpp  +242     npu/pack_import.h  +75     tests/npu_pack_import_test.cpp  +278
3 files changed, 593 insertions(+), 2 deletions(-)

The 31-line figure came from a stale origin/main in my clone, which made main look like it already contained most of this branch. That is the third false reading a stale ref has produced for me today, after the .loom corpus stack run and a PR head that had already moved. The rule I wrote on the board for myself — fetch with an explicit refspec and verify the sha before measuring — I applied to the head and not to the base.

What is actually true, and what I will do next: #2099 (the gate) and #2291 merge cleanly together; #2187 does not merge on top of #2099 and needs a real rebase, after which its own review survives or does not on its merits. I would rather have this correction on the record than a merge queue acting on my earlier sentence.

[bong-water-water-bong]

The merge order, measured pairwise — and a second correction to my own attribution

After correcting the stack claim I re-ran the merges individually rather than in one pile, which is what I should have done first. As of #2099 9766d80, #2187 rebased 24ded5a6, #2291 6db001b:

main          + #2099            -> clean
main          + #2187 (rebased)  -> clean
main          + #2291            -> clean
main  + #2099 + #2187 (rebased)  -> clean
main  + #2099 + #2291            -> clean
main  + #2099 + #2187 + #2291    -> CONFLICT (npu/pack_import.cpp, .h, tests/npu_pack_import_test.cpp)

So the collision is between #2187 and #2291, not with the gate — and my previous comment said #2187 conflicted with #2099, which was wrong in the other direction. Both are the two reporting halves of F1 slice 3b/3c and they edit the same regions of the same three files; the gate is innocent in both cases.

Recommended order, now that it is measured:

1. npu-pack: plan() reports the pack's roles and names what nothing recognised (engine#1786, F1 slice 3b) #2099 — the gate, and everything merges on top of it.
2. npu-pack: classify the MoE families and the singular layer. spelling (engine#1788 acceptance) #2187 — it is the smaller reporting addition with two READY verdicts, and it lands cleanly on the gate.
3. npu-pack: the placement measured on the five containers, and the two roles it could not name (engine#1788, 3c) #2291 — rebase it onto the result. Its own content is orthogonal (placement), so the rebase is a matter of the overlapping declarations, and I will do that rebase myself the moment npu-pack: classify the MoE families and the singular layer. spelling (engine#1788 acceptance) #2187 lands rather than asking a reviewer to read a conflicted diff.

What I got wrong twice today, since it is the useful part: first I claimed the stack was verified when my run had silently omitted #2187 (wrong branch ref), then I claimed #2187 conflicted with #2099 when the conflict was with #2291 (conflict found in a tree that already had #2291 applied). Both were attribution errors from a compound merge, and the fix in both cases is the same: merge pairwise, name each ref, and read which two commits the conflict is between before writing a sentence about it.