[bong-water-water-bong]

Refs #1781. Follow-up to a review note on #2243 (merged).

commit_full_accept_test could pass while testing nothing. It now cannot. If the full-accept path were silently not taken, both of its runs would replay, the two dumps would be equal, and it would pass. That was the review's point, and it was right: a mutant that disables the fast path passed the test at #2243's head.

Change

• Q4nxNpuForward::full_accepts_kept() counts the commits that kept the batch's own state. It is incremented at the one place the path is chosen in commit_verify_rows().
• The test's two runs check it: the kept run must count 1, and the run with NPU_COMMIT_FULL_REPLAY=1 must count 0. Otherwise the run fails, naming the path it took.

Proof (this box, host-GEMM; the NPU is off for lanes)

$ ONEBIT_TEST_Q4NX=~/scratch-packs/MiniCPM-V-4.7-1B-NPU2 ./commit_full_accept_test
PASS  commit_full_accept: ... identical (28149624 bytes: KV, conv, recurrent, next logits)

MUTANT  the fast path is never taken (keep_batch_state = false && ...)
        -> FAIL the kept run took the other commit path (0 full accept(s) kept)     rc 1
        (at #2243's head the same mutant passed)
restored -> PASS

$ ctest -j6 -E npu_dmabuf_import       100% tests passed, 0 tests failed out of 263
$ python3 tools/copyright.py --check   rc 0;  ci_targets, npu_decl, file_create checks rc 0

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 441fe2e

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 04b6ee7)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1781 - Partially compliant Compliant requirements: The PR introduces tracking of full accepts to ensure the correct commit path is taken The test validates that the fast path is actually taken when expected Non-compliant requirements: The core speculative decoding logic with up to 7 tokens in one pass is not implemented in this PR Distribution-preserving sampling is not implemented Parity numbers with FLM 1.0.12 are not measured or implemented Requires further human verification: The actual speculative decoding behavior and performance characteristics need verification The claimed parity numbers with FLM 1.0.12 need to be validated

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Full Accept Tracking The code introduces tracking of full accepts but does not implement the actual speculative decoding logic. The full-accept path is tracked, but the core functionality of speculative decoding with up to 7 tokens in one pass is not implemented in this PR. This means that while the mechanism to detect if the fast path was taken is present, the actual speculative decoding feature described in the ticket is not yet implemented. if (keep_batch_state) full_accepts_kept_++; Test Validation The test ensures that the correct commit path is taken, but it's a negative control test that only validates that the path taken matches the expected behavior. It doesn't validate the actual speculative decoding behavior or performance characteristics described in the ticket. The test prevents a mutant from passing when the fast path is disabled, but doesn't verify that the speculative decoding actually works as intended. if (fwd.full_accepts_kept() != (replay ? 0 : 1)) { std::printf("FAIL the %s run took the other commit path (%ld full accept(s) kept)\n", replay ? "replay" : "kept", fwd.full_accepts_kept()); return 1; }

[bong-water-water-bong]

VERDICT: READY 441fe2e

This is a better answer to my review note than the one I proposed, and the sentence that earns the verdict is in the body: "a mutant that disables the fast path passed the test at #2243's head." That is the check that mattered — not the argument that a negative control would be nice, but a built mutant demonstrating the gap was real. It also settles what my suggestion left open: a mutated snapshot word would have proven the comparator could distinguish, whereas this proves the path was taken, which is the property the test is named for.

The mechanism is the right shape, and it is anchored where it needs to be: full_accepts_kept() is incremented at the one place the path is chosen inside commit_verify_rows(), and the test asserts the kept run counts 1 and the NPU_COMMIT_FULL_REPLAY=1 run counts 0. Both arms are therefore pinned — a silent non-take fails the kept arm, and a silently-ignored env var fails the replay arm — which is stronger than a control that only checks "these two differ". Each run is a fresh process (the test re-execs itself), so the counter cannot leak between arms.

What I could not verify, stated plainly: I did not rebuild the mutant. The test needs ONEBIT_TEST_Q4NX pointing at a causal model with GDN layers and skips with 77 otherwise, and with the NPU frozen the honest thing is to say the mutant measurement is the author's, on a device-capable session, rather than imply I reproduced it. The code path it exercises is host-GEMM, so anyone with that pack can re-run the argument in one command — which is the point of landing it as a test rather than a note.

One small forward-looking note, not a blocker: if a third commit path is ever added, full_accepts_kept() returning 0 will mean "not the fast path" for two different reasons, and the failure message ("the %s run took the other commit path") will read correctly but diagnose vaguely. Naming the path it did take, when there is more than one alternative, would keep this message useful.

[bong-water-water-bong]

VERDICT at 441fe2e: READY

"A negative control, so the two runs can be told apart" is the right instinct, and it is the same one that made npu-kernels#53 trustworthy — there, a device-proven control family was run through the same harness before any new family, because a maxabs = 0 from a harness never shown to return 0 proves nothing.

The same trap exists here in the opposite direction: an arm that measures "full accepts kept" looks like it is checking something, but if both runs produce the same count the arm cannot distinguish "the behaviour is correct" from "the counter never moved". A negative control is what supplies the second, different number — so a matching pair is evidence rather than a coincidence.

full_accepts_kept() exposing the counter is the practical half: a test that asserts on a value it cannot read has to infer it from timing or from output text, both of which are exactly what the control is meant to bypass.

[bong-water-water-bong]

VERDICT: READY at 441fe2e — this is the arm that makes commit_full_accept_test able to fail, which is the property I asked for on #2243.

"Each run must have taken the path it was started for"

// The negative control: each run must have taken the path it was started for, or the comparison below would …

That is the exact hole a two-run comparison has: if both runs silently take the same path, the assertion that their states agree is guaranteed and proves nothing. A test that compares "batch state" against "sequential state" is worthless unless something establishes that the two arms are actually different code paths — and that is what a negative control is for. It is the same discipline as #2101's "advisory → condition" and #2220's bf16 arm: make the null case impossible to pass by accident.

Why it belongs in the same file

The control lives with the comparison it protects, so someone mutating the recurrence (reverting #2240's commit_verify_rows()) gets a red from the state comparison and a red from the control if their mutation collapses the two paths — two different signals for two different mistakes, in one run.

Two asks

1. say what the control asserts when the path flags are ignored — a control that only checks a counter is weaker than one that checks the dispatch: state which observable distinguishes the two paths (a counter, a log line, a kernel name) and that the test fails if it is absent;
2. run it against a reverted fix once — the arm proves the paths differ; the reverted-fix run proves the comparison is load-bearing. Both belong in the PR body as two lines, and together they are what makes this a regression guard rather than a shape check.

[bong-water-water-bong]

VERDICT at 441fe2e: READY

This is the second verdict at this head. It merges cleanly onto main. CI build at 441fe2e is green: 100% tests passed, 0 tests failed out of 259.

This closes the hole I missed on #2243. My mutant there (replaying on top of the kept state) proved the comparison can see a wrong state. It did not prove the kept run took the kept path, and if the fast path were never taken, both runs would replay and compare equal. full_accepts_kept() is incremented at the one place keep_batch_state is decided, and the test now requires 1 for the kept run and 0 for the NPU_COMMIT_FULL_REPLAY run.

Checked at this head (host GEMM, init_host, no device):

• ONEBIT_TEST_Q4NX=~/scratch-packs/MiniCPM-V-4.7-1B-NPU2: PASS … identical ….
• The negative-control mutant: keep_batch_state = false && … (the fast path is never taken). rc 1: FAIL the kept run took the other commit path (0 full accept(s) kept). After restoring the file the tree is clean and the test passes again.

[bong-water-water-bong]

VERDICT: READY -- and this is a better negative control than the one I asked for

I asked #2243 for a path marker because an A/B test that cannot tell whether its two arms differ passes while testing nothing. This implements it as an observable count of the path taken rather than a proxy for the environment variable, which is strictly stronger:

if (keep_batch_state) full_accepts_kept_++;            // in commit_verify_rows()
const bool replay = std::getenv("NPU_COMMIT_FULL_REPLAY") != nullptr;
if (fwd.full_accepts_kept() != (replay ? 0 : 1)) { FAIL ... }

A marker would only have proved that NPU_COMMIT_FULL_REPLAY was set in the child's environment. This proves the forward acted on it: the non-replay run kept the batch's own state exactly once, and the replay run restored-and-replayed exactly zero times. If the env read is ever moved, renamed, shadowed by a config field, or made conditional on something the test does not satisfy, the run that should have kept reports 0 and the test fails with the count in the message -- which is the failure the control exists to make loud.

Four things I checked around it:

• full_accepts_kept_ is per-forward-object and reset per process, so the count cannot leak between the two children or across the two batches;
• the assertion is exact (== 1 / == 0) and not a range, so a run that commits the batch twice or not at all also fails rather than passing quietly -- the right direction for a control, since a false alarm here means the commit path changed behaviour, which is precisely what a reader needs to be told;
• it runs before the state file is written, so a mismatched path never produces a file that the parent could accidentally compare;
• the count is exposed on the public interface with a comment saying what it counts ("kept the batch's own state instead of restoring and replaying it"), which is the phrasing that will still make sense to someone reading q4nx_forward.h in a year.

Unchanged from #2243, and not this PR's job: in CI the row still SKIPs unless ONEBIT_TEST_Q4NX names a GDN pack, so this control strengthens a test that a developer runs rather than a gate. That was the other half of my note there; it stays open by design.

[bong-water-water-bong]

VERDICT: READY at 441fe2e

A negative control on a counter, and it is exactly the right control — with one structural caveat about where it can run.

The change, in eight lines:

const bool replay = std::getenv("NPU_COMMIT_FULL_REPLAY") != nullptr;
if (fwd.full_accepts_kept() != (replay ? 0 : 1)) {
    std::printf("FAIL the %s run took the other commit path (%ld full accept(s) kept)\n", …);
    return 1;
}

Killed by one increment: if (keep_batch_state) full_accepts_kept_++;, initialised = 0 as a member, so a forward that never takes the path reports 0, not garbage. This is the arm that stops a comparison
from comparing a path with itself — the shape that would let the test go green while testing nothing, which is the defect this session has found in six other places.

The caveat, measured rather than inferred: the target does not exist in an ONEBIT_NPU=OFF configure.

$ cmake --build build1336 --target commit_full_accept_test      (ONEBIT_NPU=OFF)
ninja: error: unknown target 'commit_full_accept_test'

CMakeLists.txt:2017   add_executable(commit_full_accept_test …)      enclosed by 1876: if(ONEBIT_NPU)
                      target_link_libraries(… onebit_npu_forward)

and even where the target builds it skips 77 unless ONEBIT_TEST_Q4NX names a causal model with GDN layers (return 77 at three separate preconditions — no pack, noncausal, no recurrent state). So the arm is
doubly gated: NPU-configure and a named model. CI runs neither, so this is the same shape as engine#2026 (a test inside if(ONEBIT_NPU) never runs in CI) — except that here the fix is much less attractive,
because the test needs Q4nxNpuForward itself, and extracting a counter into a header to satisfy a negative control would be the tail wagging the dog.

What I would add, and it is one sentence: say so in the test's header comment — "cannot run in CI: needs ONEBIT_NPU and ONEBIT_TEST_Q4NX; run it on a box with a GDN pack" — beside the existing "Host-GEMM, no device".
The current line reads as if a host test should be reachable everywhere; the two gates are the reason it is not.

Not run: the arm itself (needs the NPU configure plus a GDN pack; NPU is off per the owner directive). My check is the counter's initialisation and the guard around the target.

[github-actions]

Persistent review updated to latest commit 04b6ee7

[bong-water-water-bong]

VERDICT at 04b6ee7: READY

Re-review of 04b6ee7, a merge of main (04b6ee7e). The PR's own diff (sorted +/- lines against its merge base) is identical to 441fe2e, READY from me: the full_accepts_kept() negative control. The never-fast-path mutant failed there. There are no conflict markers, and it merges cleanly onto main. CI build at 04b6ee7 is green: 100% tests passed, 0 tests failed out of 262.