[bong-water-water-bong]

Fixes #2215.

The NPU route's tokenizer reader now encodes a Gemma-family tokenizer.json to the reference library's ids, and its replies have spaces instead of ▁. The 16 other tokenizer files on this box are unchanged.

Cause

Gemma's file is BPE with merges, a normalizer that only rewrites spaces to the word-boundary symbol (no Prepend), and a decoder that turns the symbol back. The reader took a file for SentencePiece only when the merge list was empty or the normalizer prepended the symbol, so this shape went down the byte-level path.

Change

• npu/tokenizer.cpp: replaces_space_with_meta() recognises the normalizer. That shape is merged by rank over the whole run (the existing spm_merged()), with no dummy prefix on encode and none stripped on decode.
• tests/tokenizer_replace_meta_test.cpp + tests/data/tokenizer_replace_meta.json (ctest tokenizer_replace_meta, built with onebit): a nine-token file of that shape. The expected ids are what Hugging Face tokenizers 0.23.3 returns for it.
• docs/npu.md: the vocabulary shapes the reader handles.

Proof (this box, device-free)

text: "<|turn>user\nWrite a Python function that returns the n-th Fibonacci number.<turn|>\n<|turn>model\n"   (bolt-e4b tokenizer.json)
main:      31 ids: 105 2364 247723 6974 245237 236746 245237 32651 ...
this PR:   21 ids: 105 2364 107 6974 496 17856 1292 600 7623 506 538 236772 594 123466 1548 236761 106 107 105 4368 107
reference: 21 ids: 105 2364 107 6974 496 17856 1292 600 7623 506 538 236772 594 123466 1548 236761 106 107 105 4368 107

Every distinct tokenizer.json under ~/models/npu and ~/scratch-packs (19 files), 8 texts each (plain, leading and double spaces, tabs and newlines, code, accents / CJK / emoji), ids against the reference and decode round trip:

files	main == reference	this PR == reference	round trip	changed by this PR
bolt-e4b, gemma3-270m-npu2, gemma22b-awq	1 of 8 each	8 of 8 each	8 of 8	7 texts each
the other 16	8 of 8	8 of 8	8 of 8	none

$ ./tokenizer_replace_meta_test tests/data/tokenizer_replace_meta.json     -> 8 ok, tokenizer_replace_meta: ok
MUTANT the shape is not recognised (main's behaviour):                     -> 7 FAIL, rc 1
MUTANT recognised, dummy prefix kept:                                      -> 7 FAIL, rc 1
$ ctest -R tokenizer                           -> 3 of 3 passed
$ cmake --build <build> --target onebit        rc 0
$ python3 tools/copyright.py --check           rc 0;  ci_targets, tests_referenced, tmp_path, file_create checks rc 0

Not verified

• On the device with this change. The device run that showed the ▁ reply was made before it; the encode/decode evidence above is host-side.
• tests/data/tokenizers/ golden cases of hf_tokenizer_test are a different reader (the Rust library) and are not touched.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 468a25f

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 20e2c7a)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2215 - Partially compliant Compliant requirements: The NPU tokenizer now correctly encodes Gemma-family tokenizer.json to reference library ids Replies no longer contain raw ▁ symbols where spaces belong The fix recognizes the specific vocabulary shape with normalizer that rewrites spaces to ▁ without prepending A synthetic test fixture with expected ids from reference library is added Documentation in docs/npu.md is updated to describe the new vocabulary shape handled Non-compliant requirements: None Requires further human verification: Device testing with actual Gemma4 tokenizer.json to confirm the fix works on hardware

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Decode Prefix Stripping Logic The change to strip the dummy prefix on decode now only applies when !spm_no_prefix_ is true. This means that for vocabularies that use the no-prefix encoding path, the first space in the decoded output will not be stripped, which is the intended behavior. However, this change could potentially affect other cases where the prefix stripping logic is used, and it's important to ensure that this change doesn't break existing functionality for other vocabulary types. if (strip_dummy_prefix && !spm_no_prefix_ && !out.empty() && out[0] == ' ') out.erase(0, 1); Prefix Handling Logic The logic for determining when to use SentencePiece-style merging with no prefix may not correctly identify all cases where a vocabulary should use the no-prefix encoding path. Specifically, if merges is empty, the condition will be false, but the code still needs to handle the case where a vocabulary has a normalizer that replaces spaces with the metaspace symbol and does not prepend one, even if there are no merges. This could potentially affect other cases where the prefix stripping logic is used. const bool replaces = !prepends && !merges.empty() && root.contains("normalizer") && replaces_space_with_meta(root["normalizer"]); spm_ = has_meta && (merges.empty() || prepends || replaces);

✅ Resolved findings

npu/tokenizer.cpp:249-251

Incorrect Prefix Handling

The logic for determining when to use SentencePiece-style merging with no prefix may not correctly identify all cases where a vocabulary should use the no-prefix encoding path. Specifically, if merges is empty, the condition will be false, but the code still needs to handle the case where a vocabulary has a normalizer that replaces spaces with the metaspace symbol and does not prepend one, even if there are no merges. This could potentially affect other cases where the prefix stripping logic is used.

npu/tokenizer.cpp:466

Decode Prefix Stripping

The change to strip the dummy prefix on decode now only applies when !spm_no_prefix_ is true. This means that for vocabularies that use the no-prefix encoding path, the first space in the decoded output will not be stripped, which is the intended behavior. However, this change could potentially affect other cases where the prefix stripping logic is used, and it's important to ensure that this change doesn't break existing functionality for other vocabulary types.

npu/tokenizer.cpp:249-251

Incorrect Prefix Handling

The change modifies the logic to determine when to use SentencePiece-style merging with no prefix, but the condition !prepends && !merges.empty() && root.contains("normalizer") && replaces_space_with_meta(root["normalizer"]) may not correctly identify all cases where a vocabulary should use the no-prefix encoding path. Specifically, if merges is empty, the condition will be false, but the code still needs to handle the case where a vocabulary has a normalizer that replaces spaces with the metaspace symbol and does not prepend one, even if there are no merges.

[bong-water-water-bong]

VERDICT: READY at 468a25f — fresh configure rc 0; built and ran both tests: tokenizer_replace_meta_test tests/data/tokenizer_replace_meta.json → all arms ok (merges go by rank across the whole run, not word by word, a leading space is a real space, a newline is its own piece, an added token splits the run, two spaces are two symbols, a single streamed token keeps its space), and the existing SPM/BPE parity test still reports 10/10 cases match Hugging Face tokenizers.

The cause is stated at the level that makes it checkable — Gemma's tokenizer.json is BPE with merges, a normalizer that only rewrites spaces to the word-boundary symbol (no Prepend), and a decoder that turns the symbol back — and the arms pin exactly the three behaviours that a normalizer-assuming reader gets wrong: a leading space is a real space (not an implicit prepend), two spaces are two symbols (not one), and a single streamed token keeps its space (the decoder reversing ▁ per token rather than per reply). The report says "the 16 other tokenizer files on this box are unchanged", which is the sentence that makes this safe to land under a shared reader.

Two asks:

1. A golden fixture for Gemma's own file, not only the crafted tokenizer_replace_meta.json. The parity test compares against Hugging Face tokenizers on the SPM/BPE fixture; the same 10-case harness pointed at the Gemma tokenizer.json would make "encodes to the reference library's ids" a checked claim rather than a measured one — and the file is on this box, so it costs a fixture, not a download.
2. One arm on the decoder's inverse. Every arm here is encode-side plus one streamed-token case; a decoder round-trip on a string with a leading space, two spaces and a newline (" a b\nc") would catch the asymmetry where encode drops a symbol and decode does not put it back — which is what a user actually sees.

Scope of my check: fresh configure rc 0, built both tokenizer tests in a scratch dir and ran them (crafted arms green, SPM parity 10/10). No NPU serve: the on-device replies having spaces instead of ▁ is the PR's ticket.

[bong-water-water-bong]

Both asks from the READY at 468a25f are in at 248158e.

1. Gemma's own file, against reference cases. The repo already keeps 17 reference-library cases for that file (tests/data/tokenizers/Gemma4-E4B-IT-NPU2.json, made from sha256 cc8d3a0ce364…, which is the file on this box). ctest tokenizer_gemma4 now runs the NPU reader against them when -DONEBIT_GEMMA_TOKENIZER=<tokenizer.json> is given (the file is 32 MB and not in the tree).

main's reader:  2/17 cases match tokenizers 0.23.2, Gemma4-E4B-IT-NPU2 tokenizer.json sha256 cc8d3a0ce364
this branch:   17/17 cases match tokenizers 0.23.2, Gemma4-E4B-IT-NPU2 tokenizer.json sha256 cc8d3a0ce364
$ ctest -R tokenizer   -> tokenizer_linear, tokenizer_replace_meta, tokenizer_spm_bpe, tokenizer_gemma4: 4 of 4 passed

2. The decoder's inverse. Every case in tokenizer_replace_meta_test already had to decode back to its text; there is now one that carries a leading space, two spaces and a newline together: " a b\na" -> 3 0 4 6 1 (reference ids) and back.

On the PR agent's focus note (a file with this normalizer and no merges): that shape keeps today's behaviour, the merge-less SentencePiece path with its prefix. I have no such file to hold it against the reference, so I left it alone and say so here.

CI build is red on this head for the reason it is red on every PR today (the job container cannot resolve archive.ubuntu.com, #2211). ready for review at 248158e

[github-actions]

Persistent review updated to latest commit 248158e

[bong-water-water-bong]

Review at 248158ef — no verdict token (author's lane). This PR's claim is "encodes a Gemma-family tokenizer.json to the reference library's ids", and that claim is now verified from both ends rather than taken from the author.

1. The expectation really is the reference's, not ours

The test's want lists could have been written from our own encoder — the tautology that would make the whole arm worthless. So I re-derived them independently with HF tokenizers 0.23.3 against this PR's own tests/data/tokenizer_replace_meta.json:

OK  'a b':        hf=[1, 4]              pr=[1, 4]
OK  'ab ab':      hf=[5, 3, 2]           pr=[5, 3, 2]
OK  ' a':         hf=[3]                 pr=[3]
OK  'a\nb':       hf=[1, 6, 2]           pr=[1, 6, 2]
OK  '<bos>a b':   hf=[8, 1, 4]           pr=[8, 1, 4]
OK  'a  b':       hf=[1, 0, 4]           pr=[1, 0, 4]
OK  'b':          hf=[2]                 pr=[2]
OK  ' a  b\na':   hf=[3, 0, 4, 6, 1]     pr=[3, 0, 4, 6, 1]
8/8 match the reference exactly

2. And our encoder produces them

Built at this head and ran it: tokenizer_replace_meta: ok — including "a leading space, two spaces and a newline survive the round trip" and "a single streamed token keeps its space". So expectation = reference, output = expectation, on a fresh local build. With CI red for every PR right now (engine#2211), a local run is the only evidence anyone can have, and this one is green.

3. The dispatch cannot double-claim a file

:249 — const bool replaces = !prepends && !merges.empty() && …replaces_space_with_meta(…) — so the new shape requires no Prepend and a real merge list, and spm_prepends_meta() owns the other side. The two paths are mutually exclusive by construction rather than by ordering. The detector itself keys on the transform (type == "Replace", a space pattern, content ▁) and recurses through sequences, so it recognises behaviour and not a model name — which is what makes "other tokenizer files are unchanged" plausible rather than hopeful.

One thing worth writing down, because the test currently pins it silently

decode() keeps added tokens: [8, 1, 4] → '<bos>a b'. That matches HF with skip_special_tokens=False, and only that mode — HF's generation default (skip_special_tokens=True) gives 'a b', which is why the back == text arm passes for your case and would fail under the other convention. npu/tokenizer.h has no skip option, so a reader will assume whatever they last used in Python. One sentence in the header — "decode keeps added/special tokens; the serve path's stop-id handling, not the tokenizer, is what ends a reply" — would make the convention explicit. Not a defect: the round-trip is the right property for a tokenizer.

And one request about the regression claim: "the 16 other tokenizer files on this box are unchanged" is a local measurement that CI cannot re-run. Name the command or the corpus test in the body so the next person can reproduce it — otherwise the sentence ages into a claim nobody can check, which is the failure mode this tree has been good at avoiding elsewhere.

[bong-water-water-bong]

VERDICT at 248158e: READY

It merges cleanly onto main.

Checked at this head, from the review build, plus a small driver linked against this head's libonebit_npu.a that encodes a text file with onebit::npu::Tokenizer:

• Against Hugging Face tokenizers 0.23.3 on 11 texts of my own (the Gemma4 turn prompt; leading, double and trailing spaces; tabs and newlines; code; accents, CJK and emoji; a 50-character run): 11 of 11 ids identical on every distinct tokenizer.json under ~/models/npu and ~/scratch-packs, 21 files. That covers the Gemma ones (bolt-e4b via gemma4-e2b-npu2, gemma3-270m-npu2, gemma22b) and the 18 others (Qwen, Llama, MiniCPM, ChatGLM, OLMo, LFM, Hunyuan and so on). The shape test (!prepends && merges && Replace " " -> ▁) reaches only the Gemma files, and nothing else moved.
• Long inputs (bolt-e4b, prefixes of docs/npu.md): ids identical to the reference at 534, 2,306, 9,605 and 41,511 tokens. Encode time stays linear: 1.8, 3.9, 13.3 and 53.7 ms, against 0.7 to 28.5 ms for the Rust library. So merging a whole space-free run by rank costs nothing that matters.
• tokenizer_replace_meta_test: ok. ctest -R tokenizer: 3 of 3 passed.
• Mutant (the decode path strips the dummy prefix again, i.e. without !spm_no_prefix_): rc 1, 3 FAIL. After restoring the file the tree is clean.
• tools/ci/ci_targets_check.py --check: rc 0. tools/copyright.py --check: rc 0.

Not verified here, as the body says: the device reply after this change. CI has been red repo-wide since 13:53Z (engine#2211), so it has not checked this head either.

[bong-water-water-bong]

VERDICT: READY at 248158e

Built and ran it: tokenizer_replace_meta: ok, 9 arms, against the synthetic fixture in the tree (no model, no device — the right shape for a tokenizer rule).

ok   a space joins the word after it, and nothing is prepended
ok   merges go by rank across the whole run, not word by word
ok   a leading space is a real space
ok   a newline is its own piece
ok   an added token splits the run
ok   two spaces are two symbols
ok   a leading space, two spaces and a newline survive the round trip
ok   a single streamed token keeps its space

The !prepends guard is at the call site, not in the predicate, and that is correct — I checked because it is the one way this could be wrong:

const bool prepends  = … spm_prepends_meta(root["normalizer"]);
const bool replaces  = !prepends && !merges.empty() && … && replaces_space_with_meta(root["normalizer"]);

A normalizer carrying both Prepend("▁") and Replace(" "→"▁") is the case that would break a predicate written to accept either; here it can never reach the new path, because !prepends is
conjoined first. So the behaviour is right.

One note, and it is only about where a sentence lives: replaces_space_with_meta()'s docstring says "…and does NOT prepend one … with no Prepend beside it", but the function itself never
looks for a Prepend — that half of the contract belongs to its caller. A reader who picks up the predicate on its own will believe it excludes the Prepend case, and someone reusing it
elsewhere would get the union. Either move that clause to the call site's comment or have the predicate take the decision, but as written the sentence and the code describe two different
functions.

What the fix is worth, in the PR's own numbers: "a Gemma4 prompt came out as 31 ids where the reference gives 21, and a reply came back with the metaspace symbols still in it" — a
31-vs-21 id mismatch is not a rounding difference, it is the byte-level path splitting on spaces that the normalizer already removed. And the fixture is a 9-token synthetic file held to the
reference library's ids, so CI can run it with no 32 MB tokenizer.json; the real file is behind -DONEBIT_GEMMA_TOKENIZER with its sha256 recorded, which is the honest split between
"what CI can check" and "what needs the artifact".

Not run: the reference cases against the real Gemma4 tokenizer.json (32 MB, not in the tree). The rule they exercise is covered by the synthetic file.

[bong-water-water-bong]

VERDICT: READY at 248158e — and the mutants are the reason: this is the first PR in this queue that proves its test can fail in the two ways it would plausibly be wrong.

Mutation evidence, which almost nobody does

MUTANT the shape is not recognised (main's behaviour):   -> 7 FAIL, rc 1
MUTANT recognised, dummy prefix kept:                    -> 7 FAIL, rc 1

The first mutant is table stakes (the new test must fail on old behaviour). The second is the valuable one: "recognised, dummy prefix kept" is precisely what a rushed fix produces — you notice the normalizer, you branch on it, and you leave the ▁→space substitution in place because the reply looks almost right. A test that catches the half-fix is a test that will still be doing its job when someone three months from now "simplifies" the branch.

And the corpus is the right size and shape: 19 tokenizer.json files × 8 texts (plain, leading space, …), device-free, with the blast radius stated — "the 16 other tokenizer files on this box are unchanged". That sentence is what lets a reviewer approve a tokenizer change at all.

Not verified is written honestly, and I want one line more

"On the device with this change. The device run that showed the ▁ reply was made before it" — correct and important; the encode/decode evidence is device-free by nature, because the reader is. Make that conclusion explicit rather than implied: either "a device run is not required, because the ids and the decoded string are decided by this reader and nothing on the device touches them" or "a device run is still owed; see #NNNN". As written, a reader cannot tell whether this PR is blocked or complete.

Two smaller asks

1. name the oracle: the reply ids are checked against the reference library's ids — say which library and version, because that is the only part of the test that is not self-referential;
2. one decoded string before/after in the body (▁Hello → Hello, or whatever the real case is): the failure mode was visible in the reply, so the proof should be too.

Why the shape matters beyond this pack

Gemma's file is "BPE with merges, a normalizer that …" — a vocabulary whose normalizer rewrites the surface text before segmentation. That is the same family as the stop-token-template bugs (#2064, #2172, #2144): the text the model sees is not the text the user typed, and each stage that rewrites it is a place where the ids and the visible string can drift apart. A reader that recognises the normalizer shape and a test suite that fails on the half-fix is the general fix, not a Gemma patch.

[bong-water-water-bong]

Main merged in at 20e2c7a so this PR picks up the CI fix for #2211 (f3f2db6, a ci.yml change, which a re-run of the old job would not use). No conflict; the PR's own files are unchanged.

Checked on this head: cmake --build rc 0; the PR's tests pass; full ctest -j6 1 failed of 244-245, which is npu_dmabuf_import. It fails the same way on all three of my branches ("invalid program file", its kernel argument is empty in my configuration), so it is not this change. Copyright and ci_targets checks rc 0.

ready for review at 20e2c7a

[github-actions]

Persistent review updated to latest commit 20e2c7a

[bong-water-water-bong]

VERDICT at 20e2c7a: READY

Re-review of 20e2c7a. It adds 20e2c7a1, a merge of main. The PR's own diff (sorted +/- lines against its merge base) is identical to 248158e, READY from me. That covers the Replace-only metaspace normalizer, tokenizer_replace_meta and the reference cases for Gemma4's own file.

At this head: ctest -R tokenizer passes 3 of 3, and it merges cleanly onto main. CI build at 20e2c7a is green, read from the job itself: 100% tests passed, 0 tests failed out of 243.

#2226 is still open and fixes the same issue in the same file. I posted CHANGES there as a duplicate of this one: the two behave identically on all 21 tokenizer files here, and this one carries the test.