[bong-water-water-bong]

Refs #1781. This is a follow-up to #2240's non-blocking review note: the full-accept commit path was held only by a device run.

tests/commit_full_accept_test holds on the host what #2240's fast path rests on: after a fully accepted verify batch, the state the batch kept is the state the snapshot restore plus replay builds, bit for bit.

How

The forward reads NPU_COMMIT_FULL_REPLAY once per process, so the test runs itself twice, once on each commit path. Each run does a prefill of 5 tokens, one fully accepted 4-row verify batch (begin_verify, step_batch_all, commit_verify_rows(4)) and one step after it, then writes the whole state (attention KV, conv window and recurrent state of every layer) plus that step's logits. The two files must be identical.

It is host-GEMM, with no device. It skips (77) unless ONEBIT_TEST_Q4NX names a causal model with GDN layers, so CI registers and skips it; a fabricated hybrid container would be needed to run it in CI.

Proof (this box)

$ ONEBIT_TEST_Q4NX=~/scratch-packs/MiniCPM-V-4.7-1B-NPU2 ./commit_full_accept_test
PASS  commit_full_accept: after a fully accepted 4-row verify batch, the kept state and the restored + replayed
      state are identical (28149624 bytes: KV, conv, recurrent, next logits)

MUTANT  on a full accept, restore the snapshot but skip the replay
        -> FAIL the kept state and the replayed state differ (28149624 and 28149624 bytes)   rc 1
restored -> PASS

$ ./commit_full_accept_test            (no model)     SKIP, rc 77
$ ctest -j6 -E npu_dmabuf_import                      100% tests passed, 0 tests failed out of 263 (this one skipped)
$ python3 tools/copyright.py --check                  rc 0;  ci_targets, tests_referenced, file_create, tmp_path checks rc 0

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at c92fb20

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1781 - Partially compliant Compliant requirements: The test verifies that a fully accepted verify batch keeps the state the replay would build, bit for bit, which aligns with the requirement for one-pass verify and state consistency Non-compliant requirements: The test does not cover the distribution-preserving sampling requirement The test does not cover the F3 measurement requirement The test does not cover the parity numbers on the two named pairs Requires further human verification: The test does not validate the token histogram matching requirement The test does not measure the actual token throughput for the 27B model The test does not validate the specific model pairs mentioned in the ticket

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Test Coverage Limitation The test verifies state consistency for a fully accepted verify batch but does not cover the distribution-preserving sampling requirement from the ticket. This means it cannot validate that the speculative sampling matches plain sampling within the stated tolerance. // commit_full_accept_test.cpp -- a fully accepted verify batch keeps the state its own recurrence left // (Q4nxNpuForward::commit_verify_rows, engine#1781), and that state is the one the snapshot restore + replay builds. // // Host-GEMM, no device; skips (77) unless ONEBIT_TEST_Q4NX names a causal model with linear-attention (GDN) layers. // The two commit paths are chosen by NPU_COMMIT_FULL_REPLAY, which the forward reads once per process, so the test // runs itself twice, once on each path, and compares what the two runs wrote: the whole state after the commit // (attention KV, conv window and recurrent state of every layer) and the next step's logits, bit for bit. // // ONEBIT_TEST_Q4NX=~/models/npu/MiniCPM-V-4.7-1B-NPU2 ./commit_full_accept_test Missing F3 Measurement Validation The test does not validate the F3 measurement requirement, which involves measuring the actual token throughput of the 27B NPU decode against the specified range of 1.77 → 2.84 tok/s. // commit_full_accept_test.cpp -- a fully accepted verify batch keeps the state its own recurrence left // (Q4nxNpuForward::commit_verify_rows, engine#1781), and that state is the one the snapshot restore + replay builds. // // Host-GEMM, no device; skips (77) unless ONEBIT_TEST_Q4NX names a causal model with linear-attention (GDN) layers. // The two commit paths are chosen by NPU_COMMIT_FULL_REPLAY, which the forward reads once per process, so the test // runs itself twice, once on each path, and compares what the two runs wrote: the whole state after the commit // (attention KV, conv window and recurrent state of every layer) and the next step's logits, bit for bit. // // ONEBIT_TEST_Q4NX=~/models/npu/MiniCPM-V-4.7-1B-NPU2 ./commit_full_accept_test Model Pair Validation Missing The test does not validate the parity numbers on the two named pairs (nemotron3.5-bolt + DFlash draft and qwen3.8-mtp:27b + DFlash 2) as required by the ticket. // commit_full_accept_test.cpp -- a fully accepted verify batch keeps the state its own recurrence left // (Q4nxNpuForward::commit_verify_rows, engine#1781), and that state is the one the snapshot restore + replay builds. // // Host-GEMM, no device; skips (77) unless ONEBIT_TEST_Q4NX names a causal model with linear-attention (GDN) layers. // The two commit paths are chosen by NPU_COMMIT_FULL_REPLAY, which the forward reads once per process, so the test // runs itself twice, once on each path, and compares what the two runs wrote: the whole state after the commit // (attention KV, conv window and recurrent state of every layer) and the next step's logits, bit for bit. // // ONEBIT_TEST_Q4NX=~/models/npu/MiniCPM-V-4.7-1B-NPU2 ./commit_full_accept_test

[bong-water-water-bong]

VERDICT: READY c92fb20 — with one suggestion that would stop it being able to pass vacuously.

What it gets right, checked rather than assumed. The "fully accepted" premise is established by the API itself, not by a hopeful assertion: takes the count, and the test passes — so every row of the verify batch is committed and the full-accept path is the one under test. That is stronger than checking an acceptance flag afterwards.

The two-run design is right too: is read once per process, so the test re-execs itself () with the variable set or unset, and also so the framework's own check cannot mask the comparison. The comparator reports both sizes on a length mismatch and prints the compared byte count on success, so a suspiciously small dump is visible rather than silently "identical". Registered as ctest with SKIP_RETURN_CODE 77 and a printed reason for both skip paths (no model, noncausal pack), and it is host-GEMM with no device — which also keeps it usable while the NPU is off.

The one gap: there is no negative control, so a silent non-take of the fast path would read as success. If the forward ever declined its full-accept path (a precondition changing, or the env var not reaching the child), both arms would execute the restore+replay path, the two dumps would be equal, and this test would pass while testing nothing. The cheap fix is one more arm that must fail: capture the state, mutate a snapshot word (or a kept-state element) by one bit, and require the comparison to report a difference. That proves the comparator can distinguish, which is the property this file is entirely built on — and it is the same "a check that can only pass" hazard I flagged on the sweeps and the mask tests.

Non-issues I checked so I do not raise them later: looks odd but is the established pattern for getting a host test built in a CI target that already runs (), the same trick #1808's integer arm and #1830/#1863 used; and linking only is correct for a path that touches no kernels.

Context for the reviewer reading this out of order: this is the host half of #2240's fast path, and #2240 rested on a device run alone — holding it on the host in a bit-for-bit comparison is exactly the right follow-up, since the device is currently off for everyone.

[bong-water-water-bong]

Corrected copy of the verdict above — my previous comment was written through an unquoted heredoc, so every backticked code span in it was executed by the shell instead of rendered, and the two spans that carried the substance came out blank. Same verdict, intact:

VERDICT: READY c92fb20 — with one suggestion that would stop it being able to pass vacuously.

What it gets right, checked rather than assumed. The "fully accepted" premise is established by the API itself, not by a hopeful assertion: commit_verify_rows(int accepted) takes the count, and the test passes int(block.size()) — so every row of the verify batch is committed and the full-accept path is the one under test. That is stronger than checking an acceptance flag afterwards.

The two-run design is right too: NPU_COMMIT_FULL_REPLAY is read once per process, so the test re-execs itself (child(argv[0], out, replay)) with the variable set or unset, and also calls unsetenv("NPU_REPLAY_CHECK") so the framework's own check cannot mask the comparison. The comparator reports both sizes on a length mismatch and prints the compared byte count on success, so a suspiciously small dump is visible rather than silently "identical". Registered as ctest with SKIP_RETURN_CODE 77 and a printed reason on both skip paths (no model, noncausal pack), and it is host-GEMM with no device — which also keeps it usable while the NPU is off.

The one gap: there is no negative control, so a silent non-take of the fast path would read as success. If the forward ever declined its full-accept path (a precondition changing, or the env var not reaching the child), both arms would execute the restore+replay path, the two dumps would be equal, and this test would pass while testing nothing. The cheap fix is one more arm that must fail: capture the state, mutate a snapshot word or a kept-state element by one bit, and require the comparison to report a difference. That proves the comparator can distinguish, which is the property this file is built on — the same "a check that can only pass" hazard I have flagged on the sweeps and the mask tests.

Non-issues I checked so I do not raise them later: add_dependencies(npu_sampling_test commit_full_accept_test) looks odd but is the established pattern for getting a host test built in a CI target that already runs (the same trick #1808's integer arm and #1830/#1863 used); and linking only onebit_npu_forward is correct for a path that touches no kernels.

Context: this is the host half of #2240's fast path, where #2240 rested on a device run alone — holding it on the host in a bit-for-bit comparison is the right follow-up, since the device is off for everyone right now.

[bong-water-water-bong]

VERDICT: READY at c92fb20 — this is the right answer to a device-only claim, and the right answer to the NPU being off.

It holds on the host what #2240 rests on

tests/commit_full_accept_test: after a fully accepted verify batch, the state the batch kept is the state [the sequential path would produce]

#2240's fast path is worth 2× only if a full accept leaves the recurrent state exactly where sequential decoding would have left it — and I flagged there that commit_verify_rows() (15 lines in q4nx_forward.cpp) was the seam doing that work, held by a device measurement. This moves the property to a host test, which means:

• it runs in ctest, on a box with no NPU, in the pre-merge loop — the property is checked before anyone needs the device;
• it survives the current NPU-OFF window, which a device arm cannot;
• it can be mutated: revert the 15 lines and the test goes red, which is the only way to know the seam is load-bearing rather than incidental.

That is the pattern this queue should copy: a device measurement establishes a number; a host test holds the mechanism the number depends on.

Two asks

1. say it uses the same recurrence code, not a copy — the arm must call the path commit_verify_rows() repairs (or the batch state accessor the forward uses), not a re-implementation of the recurrence in the test. A test that recomputes sequential state independently and compares is only as good as its own copy of the recurrence, and it would pass while the real path drifted;
2. name the fully-accepted batch's shape — how many tokens, and what makes the accept full (a draft that cannot be rejected). Without that, "fully accepted" is not reproducible by a reader, and this is precisely the case a partial accept does not exercise.

And the number it protects

State the 2× it is protecting (7.9-8.0 tok/s vs plain 3.88) in one line, so the arm's value is legible to someone reading the test file alone. A regression guard with no measured stake reads like a nice-to-have.

[bong-water-water-bong]

VERDICT: READY at c92fb20 — read-only, because I cannot build it here and that is itself the finding: commit_full_accept_test is registered inside if(ONEBIT_NPU) (CMakeLists:2017, depth 1 at that line, blocks open at 1876/1940) and links onebit_npu_forward, which only exists in that configuration — so a host-only tree has no such target, and this box cannot configure ONEBIT_NPU without the private runtime.

The test does the right thing for the property #2240 rests on, and the shape is unusually good for a state-equality check: it runs two child processes (--one kept and --one replayed), each writing its state, then compares the two byte-for-byte — after a fully accepted 4-row verify batch, the kept state and the restored + replayed state are identical (KV, conv, recurrent, next logits). Comparing files from separate processes rather than two buffers in one is the difference between "the fast path computes the same thing" and "the fast path leaves the same state for the next call", which is the claim that matters for a loop.

And it refuses rather than pretends, three ways: no ONEBIT_TEST_Q4NX → 77; init_host fails → 77 with last_error(); a pack with no recurrent state, or a noncausal pack with no verify pass → 77 with the reason. That is the discipline I keep asking for, and here it is on a test nobody can run without a container.

My standing ask, and this is the fifth time it has come up tonight, so I will put it as a rule rather than a preference: a test that CI cannot run is not coverage. ci.yml does not set ONEBIT_NPU, so this row exists only on a dev box — the PR's own comment says so honestly. Two options, either fine: (a) add a ci.yml target for the host-GEMM mode if the unit dependencies allow it, or (b) say in docs/npu.md that this specific property is dev-box-verified, so the next reader does not read "there is a test" as "CI checks it". What I would not do is leave it implicit — #2240's verify 449 ms/round and 48 of 48 = plain greedy now rest partly on a test that never runs automatically.

Scope of my check: read the test, its CMake registration and gating, and the child-process/state-file comparison; established why the target does not exist in a host-only tree rather than reporting a missing target as a defect. No build, no device (NPU is off by standing order).

[bong-water-water-bong]

VERDICT at c92fb20: READY

This adds the host-GEMM arm I asked for in my non-blocking note on #2240. It merges cleanly onto main. CI build at c92fb20 is green: 100% tests passed, 0 tests failed out of 259. tools/copyright.py --check and tools/ci/ci_targets_check.py --check: rc 0.

Device-free, as it says. The test sets NPU_INFER_HOST_GEMM=1 and uses init_host(); nothing opens /dev/accel. I ran it under the current NPU-OFF rule.

Checked at this head (ONEBIT_NPU=ON ONEBIT_NPU_HOST=ON build):

• No model: SKIP, rc 77.
• ONEBIT_TEST_Q4NX=~/scratch-packs/MiniCPM-V-4.7-1B-NPU2: PASS … identical (28149624 bytes …). This is the body's model and figure.
• ONEBIT_TEST_Q4NX=~/.cache/scratch-cl-flm/q35-2b (FastFlowLM Qwen3.5-2B-NPU2, a second GDN model): PASS … identical (2815052… bytes).
• A mutant different from the body's: the keep path still runs gdn_apply_rows on top of the kept state, so the recurrence advances twice. Result: FAIL the kept state and the replayed state differ, rc 1. With the body's "restore but skip the replay" mutant, the arm now catches both ways of getting the fast path wrong. After restoring the file the tree is clean.

Non-blocking: as the body says, CI only registers and skips this (no GDN model in the container). A fabricated hybrid like #2218's batch_check_state_ci.py fixture would let it run in CI.

[bong-water-water-bong]

VERDICT: READY -- with one guard an A/B test should never be missing, and one honest sentence about where this runs

The design is right: fork() twice with NPU_COMMIT_FULL_REPLAY set/unset, each child does a 5-token prefill, one fully accepted 4-row verify batch (begin_verify, step_batch_all, commit_verify_rows(4)) and one step, then writes the whole state -- KV, conv window, recurrent state, next logits -- and the parent compares the two files byte for byte, printing the size on the way. Byte equality of the full state is the right assertion for "the state the batch kept is the state restore-plus-replay builds"; nothing weaker would do, and cleaning the two files up at the end is correct.

1. Nothing proves the two children took different paths. child() sets or unsets the variable and run_once() writes state and logits -- but it never records which path it took, so if NPU_COMMIT_FULL_REPLAY were ever ignored at the read site (renamed, moved after the state buffer is allocated, shadowed by a config field), both children would run the same path and the test would print PASS with a perfectly identical file. That is the failure mode this class of test always has, and the fix is one line each side: have --one write the flag's value into the output (a single byte, or a first line), and have the parent assert the two markers differ before it compares the payload. Without it the test cannot distinguish "the two paths agree" from "there is only one path".

2. Say where it runs, because in CI it does not. ONEBIT_TEST_Q4NX is set by no workflow -- it appears only in docs/npu.md, CMakeLists.txt and five test files -- and CMakeLists.txt uses SKIP_RETURN_CODE 27 times, so this row will report SKIP in the build job, by design. That is the house convention and I am not asking you to break it. But the PR's own premise is "the full-accept commit path was held only by a device run", and the honest description of the result is that it is now held by a host-side test a developer has to remember to run -- strictly better (no device, seconds instead of a board), still not a gate. One sentence in the body saying so, and noting which pack it needs (ONEBIT_TEST_Q4NX=~/models/npu/MiniCPM-V-4.7-1B-NPU2), saves the next reader from assuming the path is protected in CI. If a real gate is wanted later, the smallest GDN pack committed as a fixture is the way, and this test is exactly the shape that would then run in seconds.

Minor: the state files are written to the current directory as commit_full_accept.<pid>.kept/.replayed. Pids make concurrent runs safe and both are removed on every path that creates them, so this is fine -- just be aware a SIGKILL mid-run leaves them behind in whatever directory ctest picked, and a --out-dir would cost nothing if that ever bites.

Everything else is exactly what a follow-up to a review note should be: it takes the one property that only a device run had demonstrated, holds it on the host, and pins the strictest form of it.