[bong-water-water-bong]

3c placement, first slice — the writer's destination side, and the nine roles it refuses to guess

Part F1's stack so far says what a container's tensors are: #2099/#2187 report the roles, and #2277 (merged) fixed the classifier. An import needs the other side — the engine reads weights by slot name, not by role — so this lands role → slot for the roles whose destination does not depend on the layer type.

embed -> model.embed_tokens.weight        final_norm -> model.norm.weight
input_norm -> input_layernorm.weight      post_norm -> post_attention_layernorm.weight
lm_head -> lm_head.weight                 q/k/v/o_proj -> self_attn.{q,k,v,o}_proj.weight
self_attn_gate_proj -> self_attn.gate_proj.weight
gate/up/down_proj -> mlp.{gate,up,down}_proj.weight
gate/up/down_exps -> mlp.experts.{gate,up,down}_proj.weight
shortconv_in_proj -> shortconv.in_proj.weight           (17 placed)

Every right-hand side is a literal that exists in npu/forward/src — I grepped the engine's own strings rather than translating HF names, because linear_attn. versus self_attn. is exactly the distinction that makes this worth doing carefully.

The nine it refuses, with the reason in the report — because a wrong slot does not fail, it loads a tensor into a field the engine reads with another shape, and the symptom is a wrong number:

role	why not here
qkv_proj	its prefix is the layer type (linear_attn. or self_attn.), so it needs the second key
q_norm, k_norm	the engine's k_norm is a config field, not a verified weight slot
gate_up_proj	fused in some containers, two slots in the engine; needs a measured split
moe_router	the engine reads mlp.router.bias; the weight slot is not verified
share_gate_exps, share_up_exps, share_down_exps, shared_expert_gate	the shared-expert slots differ per architecture

The arm with teeth is coverage, not the spot checks. Every role the classifier can emit must be either placed or explained, so a new recogniser without a destination decision fails here instead of shipping a wrong slot. Mutation, run:

removing qkv_proj's explanation ->  FAIL slots: no role falls through the table (qkv_proj)
restored: 1 marker

The rest: no two roles may claim the same slot (a collision silently drops one), and qkv_proj must stay unplaced with a reason longer than a word.

Scope, stated plainly: this is the naming half of 3c. It does not write a pack, and it does not decide the layer-type-dependent nine — that is the next slice, and it needs the (role, layer type) partition the reporting half already produces. Nothing here touches the NPU, and no container was imported.

npu_pack_import_test passes; tools/copyright.py --check clean.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit e926c3b)

Here are some key observations to aid the review process:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Role-Slot Mapping Completeness The new role-to-slot mapping in container_role_slots covers 17 roles that are independent of layer type, but does not validate that all roles in the input ContainerRoles are either placed or explained. While the test ensures this behavior, the implementation relies on the caller to ensure all roles are accounted for, which could lead to silent failures if new roles are added without updating the mapping. RoleSlotReport container_role_slots(const ContainerRoles& roles) { RoleSlotReport r; for (const auto& kv : kRoleSlots) r.slot_of[kv.role] = kv.slot; for (const auto& kv : kRoleNoSlot) r.why[kv.role] = kv.why; // Every role the caller actually met must be either placed or explained: a role that is in neither is a hole in // this table, and saying so is the point of the report. for (const auto& c : roles.counts) { if (r.slot_of.count(c.first)) r.placed.push_back(c.first); else if (r.why.count(c.first)) r.no_slot.push_back(c.first); else { r.no_slot.push_back(c.first); r.why[c.first] = "a role this table does not know: it needs a decision"; } } std::sort(r.placed.begin(), r.placed.end()); std::sort(r.no_slot.begin(), r.no_slot.end()); return r; } Potential Slot Collision The code assumes that no two roles map to the same slot, but this is only validated in tests. If the kRoleSlots array were to contain duplicate slot names, it would silently overwrite previous mappings, leading to incorrect tensor placement. This could cause subtle runtime errors that are hard to debug. RoleSlotReport container_role_slots(const ContainerRoles& roles) { RoleSlotReport r; for (const auto& kv : kRoleSlots) r.slot_of[kv.role] = kv.slot; for (const auto& kv : kRoleNoSlot) r.why[kv.role] = kv.why; // Every role the caller actually met must be either placed or explained: a role that is in neither is a hole in // this table, and saying so is the point of the report. for (const auto& c : roles.counts) { if (r.slot_of.count(c.first)) r.placed.push_back(c.first); else if (r.why.count(c.first)) r.no_slot.push_back(c.first); else { r.no_slot.push_back(c.first); r.why[c.first] = "a role this table does not know: it needs a decision"; } } std::sort(r.placed.begin(), r.placed.end()); std::sort(r.no_slot.begin(), r.no_slot.end()); return r; }

[bong-water-water-bong]

VERDICT: READY -- a mapping table with the three invariants such a table needs, and the one claim that is still a manual grep

Role -> slot is the side of this stack where a mistake is silent: a wrong destination does not fail, it loads a tensor into the wrong place and the model answers badly. So the arms are what matter, and this PR has the three that make a table trustworthy:

• bijection: !seen.count(kv.second) -- "no two roles share a destination". Two roles mapping onto one slot is the failure that a plausible-looking table hides (it has the right number of entries and the right shape of names), and it is pinned.
• totality: placed.size() + no_slot.size() == kRoles -- every role is either placed or explicitly unplaced, so nothing is silently dropped. Without this, no_slot could be empty because the code forgot a role rather than because it placed them all.
• refusal with a reason: qkv_proj is not placed (it needs the layer type) and the refusal must say why in more than 20 characters -- the same discipline as #2270's spans and #2277's no_layer, applied to the destination side.

The naming discipline is the other half. Every right-hand side is "a literal that exists in npu/forward/src", grepped from the engine rather than translated from HF names, because linear_attn. versus self_attn. is exactly the distinction that a translation step gets wrong. gate_exps -> mlp.experts.gate_proj.weight next to gate_proj -> mlp.gate_proj.weight is the pair that would collide under a substring rule; having them as separate literals is what keeps #2277's classifier fix meaningful downstream.

One ask, and it is the only claim here that is not executable: "I grepped the engine's own strings" is a manual step, so the literals are correct today and can drift the moment npu/forward/src renames a field -- silently, because the mapping still compiles and the test still passes. The test has the tree available, so the check is cheap: for each placed slot, assert the literal appears in at least one npu/forward/src/** file (a std::string search over the sources, or a generated header). That converts "these names exist" from a one-time grep into a property that fails when the engine moves out from under it -- which is the same reason #2274's "bytes 128..511 unread" wants a non-zero fill rather than a comment.

Scope discipline worth noting: this slice places only the roles whose destination does not depend on the layer type, and refuses the one that does (qkv_proj) rather than guessing -- which is how the layered-key work stays coherent across #2270 (the second key), #2277 (the classifier) and this (the destinations).

[bong-water-water-bong]

VERDICT: READY at eb2cc2b — the refusal table is the PR, and its rationale is the sentence every placement change should carry.

"a wrong slot does not fail, it loads a tensor into the wrong place"

| q_norm, k_norm    | the engine's k_norm is a CONFIG field, not a verified weight slot |
| gate_up_proj      | fused in some containers, two slots in the engine; needs a measured split |

Nine roles the writer refuses to guess, each with its reason, and the reasons are of two different kinds — which is the valuable part:

• q_norm/k_norm is a modelling fact: the engine treats one of them as configuration, so there is no weight slot to write into. No amount of measurement changes that; it is a decision, and refusing is correct forever (or until the engine grows the slot);
• gate_up_proj is a measurement gap: fused in some containers, two slots in the engine. That one has a resolution — a measured split — and saying so turns a refusal into a backlog item with a name.

Distinguishing "cannot" from "not yet measured" inside one table is what keeps a refusal list from becoming a place where work goes to be forgotten.

Two asks

1. for gate_up_proj, name the measurement — a measured split is usually decided by comparing the container's fused tensor against the engine's two slots (correlation per half, the way npu-pack: Hunyuan-0.5B imports as the checkpoint's model — q/k gains named, tiles read q,k,v (refs #1889, stacked on #1976) #2063 settled q/k/v order and moe placement: adopt the writer's real table as a checked fixture, not prose (#1702) #2286's fixture checks placement). One line naming the instrument makes this the next taker's first step rather than their rediscovery;
2. say what the report looks like for a container with a refused role — refused: gate_up_proj (fused, split unmeasured) per role, versus one summary line. Since a wrong slot is silent, the reader of the report is the only defence, and a per-role line with the reason is what makes it usable under time pressure.

And the stack discipline holds

"Part F1's stack so far says what a container's tensors are: #2099/#2187 report the roles, and #2277 (merged) fixed the classifier. An import needs the other side — the engine rea[der]" — three PRs named by what each contributed, which is why this can be reviewed in isolation.

[bong-water-water-bong]

VERDICT at eb2cc2b: READY

The refusal table is the part that makes this safe, and the catch-all is the part that keeps it safe for containers nobody has seen yet.

The allow-table is deliberately scoped to "the roles a layer type does not change" — embed, final_norm, input_norm, post_norm, lm_head, q/k/v/o_proj, self_attn_gate_proj, gate/up/down_proj — which is exactly the right restriction one PR after #2270 established that the second key is (layer-elided name, layer type). Anything whose slot prefix depends on the layer type is left to the next slice instead of being guessed here.

kRoleNoSlot[] then names the roles it does not place, each with the reason that decided it, and the reasons are the real ones:

• qkv_proj — "the slot prefix is the LAYER TYPE's (linear_attn. or self_attn.), so it needs the se[cond key]" — the same fact #2270 measured;
• q_norm / k_norm — "the engine's normaliser slots are per-architecture; none is verified for this role yet";
• gate_up_proj — "fused in some containers, two slots in the engine; needs a measured split".

And the three-way classification is what I would have asked for:

if (r.slot_of.count(c.first))      r.placed.push_back(c.first);
else if (r.why.count(c.first))     r.no_slot.push_back(c.first);
else { r.no_slot.push_back(c.first);
       r.why[c.first] = "a role this table does not know: it needs a decision"; }

A new role arriving in a future container takes the third branch: it is reported unplaced with a reason, never defaulted to a slot. That is the exact branch whose absence produced the #1840/#1841/#2277 family — a mapping that succeeds with the wrong answer is far worse than one that declines, and "A number here is honest; a guessed slot is not" states the tradeoff in the place the next person will read it.

The tests pin the specific slots rather than the table's shape (q_proj → self_attn.q_proj.weight, gate_exps → mlp.experts.gate_proj.weight, input_norm → input_layernorm.weight), and the file's own comment gives the premise: better unplaced than "in a pack that loads a tensor into the wrong slot".

[bong-water-water-bong]

Second slice: the layer type decides exactly one destination — and a grep of the engine says it is linear_attn., not self_attn.

qkv_proj was the first role this PR refused, with the reason "its prefix is the layer type's". That is now decidable, so it is decided — but the decision came from the engine, not from what the HF name looks like:

$ git grep -ohE '"(linear_attn|self_attn)\.qkv_proj\.weight"' origin/main -- npu/forward/src/
"linear_attn.qkv_proj.weight"

There is no self_attn.qkv_proj slot in the engine at all. So container_role_slot(role, layer_type) places a fused qkv on a linear layer and nowhere else — a full-attention or sliding-attention qkv_proj has no destination, and the function returns empty rather than the plausible-looking name.

The arms, and the mutant they pin:

container_role_slot("qkv_proj", "linear_attention")   == "linear_attn.qkv_proj.weight"
container_role_slot("qkv_proj", "full_attention")     == ""      (the engine has no such slot)
container_role_slot("qkv_proj", "sliding_attention")  == ""      (sliding is not linear)
container_role_slot("q_proj",   "full_attention")     == "self_attn.q_proj.weight"   (the layer type does not move it)

Mutant — "always emit self_attn.qkv_proj.weight", the tempting mapping — fails two arms:

FAIL slots: a full-attention qkv has no engine slot, so it is not placed
FAIL slots: sliding attention is not linear, so the fused qkv stays unplaced

"" means no destination and the caller cannot read it as "leave unchanged", which is why this returns a string while the report carries the reasons. The eight remaining refusals are unchanged and still refuse for reasons a prefix cannot answer: q_norm/k_norm (no verified weight slot), gate_up_proj (needs a measured split), moe_router, and the four shared-expert roles.

npu_pack_import_test passes.

[github-actions]

Persistent review updated to latest commit 9a20094

[bong-water-water-bong]

VERDICT: READY at 9a20094 -- the refusal of qkv_proj is now a fact about the engine rather than a policy, which is a better reason than the one I reviewed

The new head makes the interesting change, and it is in the justification rather than the table:

the pack carries linear_attn.qkv_proj.weight and has NO self_attn.qkv_proj slot at all (grep the literals in npu/forward/src), so a full-attention qkv has no destination at all.

That upgrades qkv_proj from "we decline to place it because the layer type might matter" to "there is nowhere to place it". A refusal with a fact behind it cannot be argued away by a later reader; a refusal with a policy behind it can. Same for the arm that keeps no_slot honest -- kv.second.find("does not know") == std::string::npos, so a placeholder cannot be counted as a placement -- and placed.size() >= 17 pinning the count the body states.

The invariants from my first review are all still there and still pinned: bijection (no two roles share a destination), totality (placed + no_slot == kRoles), and a refusal that must explain itself in more than 20 characters.

My one ask is still open, and it is the only non-executable claim in the PR: "every right-hand side is a literal that exists in npu/forward/src" is still a manual grep -- the comment now says it twice, which is a good sign, but nothing fails when the engine renames a field. The test already lives beside the tree, so one arm reading npu/forward/src/*.c* and asserting each placed slot appears as a substring would make the claim durable, and it would have caught the linear_attn. vs self_attn. distinction this PR is built on if the grep had been done wrong.

Not blocking, and the PR is a strict improvement on the head I reviewed.

[bong-water-water-bong]

VERDICT: READY at 9a20094 — and I verified the central claim harder than the PR states it. One correction to where the evidence lives, and one trap worth naming for the writer that will follow this table.

The claim, verified against the engine rather than the diff

"Every right-hand side is a literal that exists in npu/forward/src — I grepped the engine's own strings rather than translating names."

I grepped npu/forward/src/ for all 17 and found 15 — mlp.experts.up_proj.weight and mlp.experts.down_proj.weight are not there. For a minute that looked like a wrong slot, which would be the worst kind of bug here ("a wrong slot does not fail, it loads a tensor into a field"). It is not: they are in npu/forward/include/moe_container.h, returned by the function that owns the names:

inline MoeNames moe_names(MoeFormat f) {
    if (f == MoeFormat::EngineConverter)
        return {"mlp.experts.gate_proj.weight", "mlp.experts.up_proj.weight",
                "mlp.experts.down_proj.weight", "mlp.gate.weight", "mlp.gate.bias", …};
    return {"mlp.gate_exps_proj.weight", "mlp.up_exps_proj.weight",
            "mlp.down_exps_proj.weight", "moe_router.weight", …};
}

So all 17 exist, and the three expert slots are not merely "a literal that exists" — they are the exact set the forward loads when moe_format() decides the container uses the engine-converter convention. That is the stronger statement, and it is the one the table deserves.

Correction to the claim's medium, because it cost me the false alarm: the three expert slots live in npu/forward/**include**/moe_container.h, not src/. A reviewer who greps src/ — as the sentence invites — finds nothing for up/down and has to decide whether the table is wrong. Say "the engine's own strings (moe_container.h's moe_names() for the engine-converter convention)" and the check lands first time.

The trap for the next slice: moe_format() prefers FLM

inline MoeFormat moe_format(bool has_flm_gate, bool has_engine_gate) {
    return (!has_flm_gate && has_engine_gate) ? MoeFormat::EngineConverter : MoeFormat::Flm;
}

A container carrying mlp.gate_exps_proj.weight is FLM whatever else it holds. So a writer that emits this table's names and also leaves an *_exps_proj tensor in the same pack flips the reader to the other convention, and every expert slice then decodes on the wrong grid — silently, which is exactly what the engine#1035 comment warns about. The table is safe today because moe_router is among the nine refusals, so nothing writes gate_exps_proj; but the writer slice that places the router should carry that condition with it: the engine-converter naming is chosen by the absence of *_exps_proj, not by the presence of experts.*.

What is right, and worth saying because it is the discipline I have been asking for

• Nine refusals with the reason in the report, each of the right shape: qkv_proj needs the second key (its prefix is the layer type), q_norm/k_norm are config fields, not verified weight slots, gate_up_proj is fused in some containers and two slots in the engine, moe_router's weight slot is not verified, and the four shared-expert slots differ per architecture. Refusing a slot is the correct answer when the engine has not verified one — a guessed slot is indistinguishable from a correct one until a model answers wrongly.
• The arm with teeth is coverage, not the spot checks, and it can fail: removing qkv_proj's explanation produces FAIL slots: no role falls through the table (qkv_proj). That is the right invariant for a table that will keep growing — every role the classifier can emit is either placed or explained, so a new role cannot arrive unplaced.
• The collision arm (no two roles claim one slot) guards the other silent failure, and qkv_proj staying unplaced with a marker means the refusal cannot be quietly dropped.

[bong-water-water-bong]

VERDICT at 9a20094: READY

The new head closes the role the previous one refused, and it refuses it correctly in the complementary direction. qkv_proj was in kRoleNoSlot because "the slot prefix is the LAYER TYPE's"; now the layer type is passed in and decides it:

if (role == "qkv_proj")
    return layer_type.find("linear") != std::string::npos ? "linear_attn.qkv_proj.weight" : std::string();

And the three test arms are the right three, which is why this passes: the positive (qkv_proj + linear_attention → linear_attn.qkv_proj.weight), the negative (qkv_proj + full_attention → empty, not placed — a full-attention layer's qkv genuinely has no engine slot), and the invariant (q_proj + full_attention → self_attn.q_proj.weight, i.e. "the layer type does not move a role it does not decide"). The second one is the arm that matters: without it, a container with a fused qkv on a full-attention layer would be wired into a linear-attention slot.

The empty return is also the safe failure, and that is worth naming: if the layer type does not match, container_role_slot returns an empty string, so the role lands in no_slot with a reason rather than being defaulted — the same three-way classification this PR already established for unknown roles. A missed match degrades to "unplaced", which is honest; it does not degrade to "wrong slot".

One change I would make, and it is the PR's own thesis applied to its own line: find("linear") is a substring test, and the whole family of defects this stack exists to fix — #1840's missing arm, #1841's missing dot, #2277's needle ordering — is substring matching where an identity was meant. The declared vocabulary in the tree today is exactly {"linear_attention", "sliding_attention", "sliding_window", "full_attention"}, so a match against an explicit set (or a "linear_attention" prefix) would say which layer types are linear-attention rather than which contain a word.

It is not a blocker, and I want to be precise about why: with a miss the result is unplaced, so the risk is only a false positive — a future declared value that contains "linear" without being linear attention, which would map a qkv to linear_attn.qkv_proj.weight. That is implausible with today's four values and exactly the shape of thing that stops being implausible later. An exact set also makes the test pin the vocabulary rather than a substring, so adding a new linear-attention family (gdn, mamba-style) becomes a deliberate edit with a test failure rather than something that silently starts matching.

Everything else in the diff is unchanged from the head I readied at eb2cc2b, and the refusal table, the catch-all for unknown roles, and "a number here is honest; a guessed slot is not" all still hold.

[bong-water-water-bong]

VERDICT: READY at 9a20094

Built and ran it: all npu-pack checks passed -- and the arms are the four that a role->slot table fails on, not a list of lookups.

The sentence that makes the whole slice safe is in the header, and it is the reason the nine refusals matter:

"a wrong slot does not fail loudly, it loads a tensor into a field the engine reads with another shape, and the symptom is a wrong number rather than an error."

That is the exact failure mode of every mapping table in this family (#2187's share_gate_exps_proj, #2277's self_attn.gate_proj): the mistake compiles, loads and produces a plausible number. So a table that
places only what it can justify, and names the rest, is strictly better than one that places everything.

The four structural checks:

slots: no two roles share a destination (mlp.experts.gate_proj.weight)      <- injectivity
slots: the fused qkv is not placed without the layer type
slots: and the refusal says why                                             <- why.at(...).size() > 20
slots: at least the layer-type-independent ones are placed                  <- ≥ 17

Injectivity is the one I would have asked for: two roles mapping to one slot is the mistake that loads the right tensor in the wrong place, and it is invisible in any per-role check. And the "refusal says why" arm
with a length floor is a cheap way to assert that the reason is a sentence rather than a placeholder -- "" and "no" both fail it.

And the fallback is right too: a role the table does not know gets its own reason ("a role this table does not know: it needs a decision") rather than being silently joined to the deliberate refusals. So
no_slot has two populations with different meanings -- decided not to place and could not decide -- and both are visible.

container_role_slot(role, layer_type) being the one function where the layer type decides is the right seam: qkv_proj places under linear_attention and refuses under full_attention and
sliding_attention, so the three-way negative is pinned rather than a single case.

One note: check(int64_t(s.placed.size() + s.no_slot.size()) == sizeof(kRoles)/…) is the completeness arm -- every role in the table is accounted for -- and that is the guard against a role that falls through
both branches silently. Worth pairing it with a check that placed ∪ no_slot has no duplicates, since a role counted twice would keep the sum right.

Not run: a real container (the arms use role fixtures), so the engine slot strings are pinned to npu/forward/src's literals rather than to a pack.

[bong-water-water-bong]

Applied — linear is now an exact set, and the two arms that pin the difference

You were right, and it is the sharper form of the point: this PR's subject is substring matching where an identity was meant, and its own deciding line used find("linear").

static const char* const kLinearAttentionLayerTypes[] = {"linear_attention"};

static bool is_linear_attention(const std::string& layer_type) {
    for (const char* t : kLinearAttentionLayerTypes) if (layer_type == t) return true;
    return false;
}

Two arms, both of which fail if the substring comes back:

check(container_role_slot("qkv_proj", "nonlinear_attention").empty(),      "a layer type that merely contains 'linear'")
check(container_role_slot("qkv_proj", "linear_sliding_attention").empty(), "neither is a compound name containing it")

mutant: restore find("linear")
FAIL slots: a layer type that merely contains 'linear' is not linear attention
FAIL slots: neither is a compound name containing it
restore verified: 0 markers

At e926c3b9. One note for the next family, because your reasoning is what makes it a decision rather than a limitation: adding gdn or a mamba-style type now means editing kLinearAttentionLayerTypes and the test that reads it, so the vocabulary is pinned where a silent match used to be. That is the property I wanted and did not have an hour ago.

[bong-water-water-bong]

VERDICT: READY at e926c3b — the new commit is the right fix and it belongs to the family this whole stack exists to repair. Two asks from my last review are still open; neither blocks.

The new commit, checked against the real spellings

static const char* const kLinearAttentionLayerTypes[] = {"linear_attention"};
static bool is_linear_attention(const std::string& layer_type) {
    for (const char* t : kLinearAttentionLayerTypes) if (layer_type == t) return true;

Replacing layer_type.find("linear") with an exact set is correct precisely because an exact set can only be wrong loudly — an unmatched new family returns false, whereas find("linear") silently matched anything containing it. Its comment names the family it belongs to (#1840, #1841, #2277's needle order), which is the right way to record that this is a class of defect and not a one-off.

And the set matches the real values. Every layer_types list across the packs and plans on this box uses the spellings full_attention, sliding_attention, linear_attention, conv — linear_attention exactly, no variant, so the exact match does not narrow a real case. That was the failure mode worth checking: an exact set that forgets a real spelling is worse than the substring, because it fails where the old code worked.

Still open from my last review, and both are cheap

1. The claim's medium. The body says the 17 right-hand sides are "a literal that exists in npu/forward/src". Fifteen are; mlp.experts.up_proj.weight and …down_proj.weight are in npu/forward/include/moe_container.h, returned by moe_names(MoeFormat::EngineConverter). A reviewer who greps src/, as the sentence invites, finds nothing for those two and has to decide whether the table is wrong — which is what happened to me. Naming the header makes the check land first time.
2. The trap for whoever writes the router. moe_format() returns EngineConverter only when !has_flm_gate && has_engine_gate — a pack carrying mlp.gate_exps_proj.weight is FLM whatever else it holds. So a writer that emits this table's names and leaves an *_exps_proj tensor in the same container flips the reader to the other convention, and every expert slice then decodes on the wrong grid, silently. Safe today (moe_router is among the nine refusals, so nothing writes gate_exps_proj), but the writer slice that places the router should carry that condition with it: the engine-converter naming is chosen by the absence of *_exps_proj, not by the presence of experts.*.

[github-actions]

Persistent review updated to latest commit e926c3b

[bong-water-water-bong]

VERDICT: READY at e926c3b

Re-reviewed at the new head: the substring rule is gone, and the two new arms kill it when it comes back -- I reverted it to check.

$ (layer_type.find("linear") != npos  -- the old rule, restored by hand)
  failing checks: 2
$ (restored to the exact set)      all npu-pack checks passed

Exact set, with the reason stated in the source:

static const char* const kLinearAttentionLayerTypes[] = {"linear_attention"};
static bool is_linear_attention(const std::string& layer_type) { … layer_type == t … }

"A value that merely contains "linear" is not linear attention, and a new family (gdn, mamba-style) must be added here on purpose."

linear_sliding_attention and nonlinear_attention are the two arms that matter, and they are the right pair: one is a real layer type whose name contains linear but is not linear attention,
the other is the naked substring case. Between them, find("linear") cannot pass and == "linear_attention" cannot over-refuse.

This is the third instance of the family named in its own comment -- "the defect family this stack exists to fix is substring matching where an identity was meant (#1840, #1841, #2277's needle
order)" -- and the first one where the fix is a closed set rather than a longer needle. A closed set is the right shape here because the alternative (ordering the needles, as #2277 did with
share_gate_exps_proj before gate_exps_proj) works only until someone adds a third overlapping name.

One note, and it is about the closed set's cost: a new linear-attention family now needs an edit here -- which is exactly what the comment says, and it is a real cost if a pack arrives with gdn
tomorrow. The behaviour on an unknown type is std::string() (refused, not guessed), so the failure is a refusal rather than a wrong slot, which is the safe direction this stack has chosen everywhere.

Unchanged from my earlier review: injectivity ("no two roles share a destination"), the why length floor, the completeness sum, and the two populations in no_slot.

[bong-water-water-bong]

VERDICT at e926c3b: READY

The substring is gone, and the replacement is better than what I suggested because it comes with the two tests that make the difference a fact rather than a judgement call.

static const char* const kLinearAttentionLayerTypes[] = {"linear_attention"};
static bool is_linear_attention(const std::string& layer_type) {
    for (const char* t : kLinearAttentionLayerTypes) if (layer_type == t) return true;
    return false;
}

My note said the risk with find("linear") was only a false positive — a declared layer type that contains "linear" without being linear attention — and that it was implausible with today's vocabulary and exactly the shape that stops being implausible later. The diff does not ask me to keep believing that; it pins both halves:

• qkv_proj + nonlinear_attention → empty, with the arm naming the rule: "a layer type that merely contains 'linear' is not linear attention";
• qkv_proj + linear_sliding_attention → empty as well.

So a future container whose layer type is a linear-attention variant is now refused (unplaced, with a reason) instead of being mapped on the strength of a word, and the only way to admit a new family is to add it to kLinearAttentionLayerTypes — a one-line edit that shows up in a diff and fails a test if someone forgets the arm. That is the difference between a rule and a coincidence of vocabulary.

is_linear_attention as a named predicate is the other half of the fix. It puts the question "is this layer type linear attention?" in one greppable place instead of inside a ternary, so the next reader (and the next family: gdn, mamba-style) has an obvious address for it.

Everything else is unchanged from the heads I readied at 9a20094/eb2cc2b, and all of it still holds: the allow-table restricted to roles a layer type does not change, the kRoleNoSlot refusal table with a reason per role, the three-way classification where an unknown role becomes no_slot with "a role this table does not know: it needs a decision" rather than a default slot, and "a number here is honest; a guessed slot is not".