[bong-water-water-bong]

Root cause

app/forward_serve.cpp read the container's stop ids with json_int(cfg, "eos_token_id", -1), and json_int()
answers its default for anything that is not an integer. HF writes that key either as one integer (Qwen2.5/Qwen3:
151645) or as a list (Llama-3.2: [128001, 128008, 128009]), so on a list-valued container the serve loaded the
model and then had no stop ids at all — its stop set was {-1} plus whatever <|endoftext|> resolved to by name.
The model's own first stop token, <|eot_id|> 128009, was therefore served as text and generation continued into an
invented second turn.

That is the failure mode that reads as a model defect: every position before the stop is correct, so the text is right
and then turns into control tokens. It cost a session on the Llama-3.2-1B OGA pack (engine#1044, defect 1).

npu/model.cpp and npu/pack_import.cpp already parse both forms, and the unified serve reads Model::dims().eos
(a vector); only this path was scalar.

Before / after

Same device-free reproducer (NPU_INFER_HOST_GEMM=1, no NPU ticket, the pack's own prompt ids, 16 tokens), old binary
vs rebuilt:

before  finish_reason: length
        text: "The capital of France is Paris.<|eot_id|><|start_header_id|>assistant<|end_header_id|>\n\nI'm sorry,"
        RESULT: FAIL - a stop token was served as text and generation continued

after   finish_reason: stop
        text: "The capital of France is Paris."
        RESULT: PASS - generation stopped on the model's own stop token

Change

• app/npu_stop_tokens.h (new): the key as one id or as a list, house pattern next to app/npu_prompt_ids.h.
  Unreadable entries are dropped rather than fatal — it is the container's own config, and a serve that refuses to
  load over one bad entry is worse than one that stops on what it could read.
• app/forward_serve.cpp: ForwardEngine::eos is a std::vector<int>, the stop test is membership, and the forward-serve: the chat route renders ChatML for every container, so a non-ChatML model is prompted with text its tokenizer cannot accept (33 vs 15 tokens, measured) #963
  chatml-refusal message lists every eos id instead of one.
• tests/npu_stop_tokens_test.cpp (new) + CMake wiring: the Llama-3.2 list and specifically that 128009 is in the
  set (the token the old code served as text), the Qwen single-int form unchanged, a missing key, a mixed list with a
  non-integer entry, and the file-path form including a missing file.

Verification

• ctest -R "npu_stop_tokens|npu_prompt_ids|npu_chatml" → 3/3 passed (the two neighbours in the same file are
  unaffected); npu_stop_tokens_test prints all stop-token checks passed, and its 128009 assertion is the one that
  fails on the old parse.
• the reproducer above on the rebuilt binary.
• tools/copyright.py --check rc 0.

Does not close #1044: this is its defect 1 (ours). The issue's defect 2 — the per-prompt coin flips — is separate
and now measured there: on identical context 15 of 16 positions agree per prompt with exactly one flip each, and the
rope_type: "llama3" gap found while measuring it is filed as #1048.

[github-actions]

Preparing review...

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 65cdee6)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1044 - Partially compliant Compliant requirements: Fix the stop-token defect where Llama containers with eos_token_id as a list are not handled correctly Ensure that all stop tokens declared in the container's config.json are properly recognized and used for stopping generation The fix handles both single integer and list forms of eos_token_id as written by HF Non-compliant requirements: The issue mentions that defect 2 (per-prompt coin flips) is separate and not addressed by this PR Requires further human verification: Verification that the fix correctly handles the specific Llama-3.2-1B case mentioned in the issue Verification that the fix doesn't break existing Qwen containers that use single integer eos_token_id 963 - Partially compliant Compliant requirements: The chat route now properly checks for ChatML support and refuses requests with non-ChatML tokenizers The error message now lists all eos ids instead of just one Non-compliant requirements: The issue mentions that option 2 (real fix with chat_template) was not implemented, but the PR implements option 1 (honesty with 400 error) Requires further human verification: Verification that the chat route properly refuses requests with non-ChatML tokenizers Verification that the error message correctly lists all eos ids 1048 - Partially compliant Compliant requirements: This PR does not address the RoPE scaling issue mentioned in npu: a pack that declares rope_type "llama3" is served plain RoPE — 17 of 32 dim pairs run up to 32x fast (Llama-3.x) #1048 Non-compliant requirements: The RoPE scaling issue (npu: a pack that declares rope_type "llama3" is served plain RoPE — 17 of 32 dim pairs run up to 32x fast (Llama-3.x) #1048) is not addressed by this PR Requires further human verification: The RoPE scaling issue (npu: a pack that declares rope_type "llama3" is served plain RoPE — 17 of 32 dim pairs run up to 32x fast (Llama-3.x) #1048) is not addressed by this PR

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Stop token handling The change correctly updates the stop token handling to use a vector of integers instead of a single integer, which fixes the issue where Llama containers with eos_token_id as a list were not handled correctly. The fix ensures that all stop tokens declared in the container's config.json are properly recognized and used for stopping generation. std::vector<int> eos; // config.json eos_token_id: one id or a list (app/npu_stop_tokens.h) int endoftext = -1; int max_context = 0; // config.json max_position_embeddings: prompt plus answer // The chat route renders ChatML for every container (#832), so whether this container's // tokenizer can represent that prompt is a property of the container: decided once at load // (#963). A refusal, not a load failure -- the id route stays usable on such a container. ChatML support check The change updates the chat route to properly check for ChatML support and refuse requests with non-ChatML tokenizers, providing clear error messages that list all eos ids instead of just one. This prevents silent wrongness where the wrong prompt text is rendered for tokenizers that cannot accept it. std::vector<std::pair<const char*, int>> named = {{"bos", e.bos_id}}; for (int id : e.eos) named.emplace_back("eos", id); for (const auto& [label, id] : named) { const std::string text = id >= 0 ? e.tok->decode(id) : std::string(); if (text.empty()) continue; specials += std::string(specials.empty() ? "" : ", ") + label + " " + text + " (" +

[bong-water-water-bong]

VERDICT: READY at f50f6003 — the defect is real on the actual pack, the fix is the right one, and the new unit test covers the token that used to leak.

Root cause reproduced against the container itself, not against the fixtures — my probe links the PR's own app/npu_stop_tokens.h and re-implements forward_serve.cpp's json_int() semantics:

config: …/oga/Llama-3.2-1B-Instruct_rai_1.8.0_npu_4K/config.json
  key is a list:            yes
  OLD parse json_int():     eos = -1                    <- the serve's only eos id
  NEW parse eos_token_ids:  { 128001 128008 128009 }     contains 128009: yes
config: …/oga/Qwen2.5-0.5B-Instruct_rai_1.8.0_npu_4K/config.json
  key is a list:            no
  OLD parse json_int():     eos = 151645
  NEW parse eos_token_ids:  { 151645 }                   <- the single-int form is unchanged

So on the Llama-3.2 pack the old code had {-1, <|endoftext|>} and <|eot_id|> 128009 — the token that ends an answer — was not in it. That is exactly the observed finish_reason: "length" plus an invented second turn, and it cannot happen on the Qwen containers, which is why it read as a model defect.

Tests, built here (-DONEBIT_NPU=ON -DONEBIT_NPU_RT_DIR=…/npu_rt -DONEBIT_NPU_RUNTIME=own, my own worktree):

ctest -R "npu_stop_tokens|npu_prompt_ids|npu_chatml"   ->  3/3 passed
./npu_stop_tokens_test                                ->  all stop-token checks passed  (rc 0)

(The first attempt showed npu_chatml (Not Run) — that was my target list, not the PR: its binary has to be built for ctest to run it; with npu_chatml_test built it is 3/3.)

The test cases match the six the body claims, and case 1 is the one that matters: it asserts 128009 is in the set for the Llama-3.2 list, which is the token the old parse served as text; case 4 drops a non-integer entry (2.5, "128001") rather than failing the load, which is the right call for a container's own config; case 3 (missing key → empty) preserves the old -1 behaviour exactly.

Read the diff too: std::vector<int> eos with a membership test at the stop site, eos_token_ids_from_config(path) mirroring the json_int(cfg, …) call it replaces, and the #963 refusal message now listing every id instead of one. Nothing else in the stop path moved.

Not verified by me: the before/after served text. There is no Llama-3.2 q4nx container on this box (the OGA pack is the Ryzen AI layout, and forward-serve needs -m <model.q4nx>), so reproducing finish_reason: stop end to end would mean an npu-pack import first. The parse defect and the fix are proven above without it; the served text is your run, and the mechanism is the same one I just measured.

[github-actions]

Persistent review updated to latest commit 65cdee6

[bong-water-water-bong]

VERDICT: READY at 65cdee6b (rebase-only; the review stands).

65cdee6b is the same change rebased onto a main that has since taken #1045/#1046/#1047/#1050, so a diff between the two heads shows those PRs. The files this PR owns are byte-identical:

git diff f50f6003…65cdee6b -- app/forward_serve.cpp app/npu_stop_tokens.h CMakeLists.txt tests/npu_stop_tokens_test.cpp
(no output)

and pr_agent is the only red on it, which is the standing pattern while coder-llm is off, not a finding.

Everything in my previous verdict holds: the old parse returns -1 on the real Llama-3.2 OGA container while the new one returns {128001, 128008, 128009}, the Qwen single-int container is unchanged at 151645, and npu_stop_tokens + its two neighbours are 3/3 under ctest.

[bong-water-water-bong]

Follow-up: the served-text half is now verified on this box, so the one thing I said I could not check in my verdict is no longer open.

To get a Llama-3.2 container I imported the OGA pack first — which needs #844's importer (without it: refusing to import … model.norm.weight: the .fconst tail envelope is … not an RMSNorm weight). With #844's npu/pack_import.{h,cpp} applied on top of this branch in a scratch tree, the import completes:

fconst:     stride 33640640, second header 5922880; 32/32 RMSNorm anchors found, final norm identical in both .fconst tails
attention:  1280/1280 .fconst tiles survive decode->encode byte for byte; 64 projections imported
MLP:        2560/2560 .fconst groups survive decode->encode byte for byte
text:       5 of 8 tokenizer/chat files copied

and forward-serve loads it with NPU_INFER_HOST_GEMM=1 (no device, no npu-hold ticket):

1bit forward-serve: llama32-q4nx ready on http://127.0.0.1:8099 (full ELF)

The chat route is refused here exactly as #963 intends — and the refusal lists every eos id (eos <|end_of_text|> (128001), <|eom_id|> (128008), <|eot_id|> (128009)), which is this PR's own change visible on a real container. So the request goes through /v1/completions with the pack's own chat-formatted ids (128000 … 128009, 128006, 78191 …), 16 tokens:

before (same binary, e.eos = { json_int(cfg, "eos_token_id", -1) } — the old scalar parse):

finish_reason: length
text: "Paris.<|eot_id|><|start_header_id|>assistant<|end_header_id|>\n\nThat's incorrect. The capital of France is"
completion_tokens: 16

after (this PR):

finish_reason: stop
text: "Paris."
completion_tokens: 2

That is the failure in the body exactly — the model's own first stop token served as text, then an invented second turn — and it stops cleanly with the fix. Same prompt ids, same build flags, only the parse differs.

So the PR is now verified end to end on a real Llama-3.2 container, not just at the parse. Nothing in my earlier verdict changes; the npu_stop_tokens 3/3 and the container-config parse evidence stand as well.