[bong-water-water-bong]

What

One header, npu/mtp/forward_base.h (the BaseModel adapter the live draft loop drives the 27B through), carries three
defects of the same family as the #811 drift. No behaviour change, no forward change, host only.

Root cause

1. The file comment describes a mechanism the code does not use. It said the adapter "takes a state snapshot before
   the verify batch and restores the accepted prefix afterwards", because "the forward has no GDN deferral yet". The
   code has used the deferred path since npu plan P6: the shared drafter protocol - tau-gated chain, 4-row verify with leaf fill, deferred GDN commit; acceptance per target in the leaderboard #805 P6 — begin_verify() stores each row's recurrence inputs,
   commit_verify_rows(accepted) applies the accepted prefix — and the in-body comment says so. The stale comment is
   the one a reader prices the loop with, and it prices it at a ~100 MB state copy per iteration that is not taken.
2. The members that comment left behind are dead: snap_ (unique_ptr<Q4nxNpuForward::StateSnapshot>) declared and
   never touched, snap_pos_ assigned and never read. StateSnapshot itself is real and used by the forward; the
   adapter just does not use it.
3. last_hidden_rows() was the class's only non-override method, and it is the single signature the drafter's
   advance hangs on. If the base's declaration ever drifts (a dropped const is the obvious one), the method silently
   stops overriding and reports no rows at all — which reads to the loop as "this base does not keep rows", i.e. the
   advance is skipped in silence. That is the exact failure mode that cost the live loop 1 token/pass for a day (npu: the MTP drafter runnable against the real base, and the head checked against the reference #811).

Measured

$ ninja mtp_acceptance            # ONEBIT_NPU=ON, main 6211c2fe
[2/2] Linking CXX executable mtp_acceptance          # clean
$ ctest -R mtp_draft
1/1 Test #31: mtp_draft ... Passed
$ tools/copyright.py --check                          # clean

Proof the third fix is a guard and not cosmetics — with override in place, dropping const from the accessor is now a
compile error, where before it was silent:

npu/mtp/forward_base.h:71:31: error: 'const std::vector<float>& onebit::npu::mtp::ForwardBase::last_hidden_rows()'
                              marked 'override', but does not override
FAILED: CMakeFiles/mtp_acceptance.dir/tools/mtp_acceptance.cpp.o

(file restored, rebuild green.)

Scope

npu/mtp/forward_base.h: +14/−11. Comment, override, two dead members, and the <memory> include that only existed
for snap_. Nothing in the loop, the head, the forward or the tests changes; no device.

---

Second commit (6f3c6d58): the review nit is right, and it is the same defect with the opposite sign

pi-prs: "a comment asserting a safety property that is not there". Checked in the source and it holds —
restore_state() (npu/forward/src/q4nx_forward.cpp:326-341) refuses only a plane of the wrong size
(give_whole) and a tokens > in.tokens extension. For tokens < in.tokens it restores the KV caches per position
to tokens and then installs the GDN conv/recurrent planes whole from the snapshot, so a prefix restore is
accepted and leaves KV at the prefix, recurrence at the snapshot's last position — precisely the mixed state the
sentence was written to warn about, not a refusal.

The comment now states that behaviour with its line reference instead of claiming a guard. This PR exists because a
comment asserted a design the code had replaced; a comment asserting a safety property nothing implements is the same
defect with the opposite sign, so it is fixed rather than merged with it.

Comment only — ninja mtp_acceptance rc 0, ctest -R mtp_draft 1/1, copyright --check clean; the adapter still
never calls snapshot_state()/restore_state().

[bong-water-water-bong]

VERDICT: READY

The override is the load-bearing line and I checked that it does what the comment says, by breaking it both ways rather than by reading it. BaseModel::last_hidden_rows() is virtual with a default body returning an empty vector (npu/mtp/draft.h:122-126), and the loop takes the advance from it (draft.h:341), so a base whose signature drifts reports no rows and the advance is skipped in silence.

Falsification, at this head: drift the signature the way a re-base would (const dropped on the member, override kept) ->

npu/mtp/forward_base.h:71:31: error: 'const std::vector<float>& onebit::npu::mtp::ForwardBase::last_hidden_rows()'
                              marked 'override', but does not override
build rc=1

and the same drift with override removed - which is main's code - builds and links with no diagnostic at all. So the PR turns the #811 failure mode into a build break, which is the only place it can be caught. The file was restored afterwards (git status clean) and rebuilt clean.

Also at this head: ninja mtp_acceptance -> 0 warnings, rc 0 (the same command the body ran), override on a base that has the virtual produces no -Woverloaded-virtual noise, and #include <memory> comes out with the unique_ptr member that used the only use of it.

One nit, and it is the same class of thing the PR is fixing - a comment asserting a safety property that is not there. The header now says "no prefix restore is attempted (restore_state() refuses those on a GDN model)". restore_state() (npu/forward/src/q4nx_forward.cpp:326-341) accepts tokens < in.tokens (a prefix) and restores KV per position while installing the GDN planes whole from the snapshot; its refusal is give_whole rejecting a plane of the wrong size, not a GDN guard and not a prefix check. So a prefix restore there is not refused, it is silently KV-at-prefix + GDN-at-snapshot - precisely the mixed state this comment exists to warn about. The adapter does not call it, so nothing is broken today; the sentence wants "restores the GDN planes whole, so a prefix restore is not what it does" rather than "refuses".

[bong-water-water-bong]

VERDICT: READY

Re-verdicted at 6f3c6d58. The one commit since 2b498833 is my nit, and it is fixed the way the code reads rather than by softening the claim:

// ... this adapter never restores: restore_state() ACCEPTS a prefix (`tokens < in.tokens`) and installs the GDN
// planes whole from the snapshot (`q4nx_forward.cpp:326-341`; its only refusals are a plane of the wrong size and
// a `tokens > in.tokens` extension), so a prefix restore here would leave KV at the prefix with the recurrence
// still at the snapshot's last position -- the mixed state this deferral exists to avoid.

That is exactly q4nx_forward.cpp:326-341: the three return false conditions are !ready_ || tokens <= 0 || tokens > in.tokens, in.per_pos != per, and the give_whole size checks, none of which looks at GDN or at the prefix relationship. The line references are right and the warning now says what the code does.

Everything from my 2b498833 review stands: ninja mtp_acceptance clean with 0 warnings, and the override falsification (const dropped on the member with override kept -> error: marked 'override', but does not override; the same drift without override, i.e. main's code, links silently).

[github-actions]

Preparing review...

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 811 - Partially compliant Compliant requirements: The MTP window needs host-side pieces stacked on npu: step_batch_all() -- per-row logits from one batched pass, for speculative verification #797 and npu: the draft loop commits a verify prefix instead of rolling back, and gates the chain on tau #798 The head is checked against the reference before any window opens The mtp_draft ctest remains green with pos threaded into DraftHead::draft Non-compliant requirements: The C++ head is measured against the Python reference with specific metrics (rel-L1, max|d|, cosine) Requires further human verification: Verification of the measured metrics (rel-L1 1.838e-04, max|d| 1.567e-02, cosine 0.999999985) requires hardware testing which cannot be assessed through code review alone 805 - Partially compliant Compliant requirements: The shared drafter protocol with tau-gated chain, 4-row verify with leaf fill, deferred GDN commit Acceptance per target in the leaderboard with gate and lane per the plan table Non-compliant requirements: Greedy text identical with public bars (74% first-draft, 2.63 tokens/pass) Requires further human verification: Verification of the public bars metrics (74% first-draft, 2.63 tokens/pass) requires hardware testing which cannot be assessed through code review alone 31 - Partially compliant Compliant requirements: A clean, simple documentation site at https://1bit-monster.github.io/engine/ Layout with top bar, sidebar, 760px text column, previous/next, and source links Style with system fonts, teal accent, styled tables and code, heading anchors, dark mode Build with tools/site.py, pinned python-markdown, and .github/workflows/pages.yml New files carry the full Apache notice Non-compliant requirements: None Requires further human verification: None

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Stale Documentation The file comment still describes a mechanism that is no longer used by the code. The comment incorrectly states that the adapter "takes a state snapshot before the verify batch and restores the accepted prefix afterwards", which is outdated since the deferred path has been in use since #805 P6. This can mislead developers about the actual implementation and performance characteristics. // The one non-obvious part is commit_verify(): a GDN layer's recurrence cannot be re-indexed, so the verify batch // DEFERS it -- begin_verify() stores each row's recurrence inputs instead of applying them, and // commit_verify_rows(accepted) applies exactly the accepted prefix and drops the rest (#805 P6). Nothing is // snapshotted per iteration and this adapter never restores: restore_state() ACCEPTS a prefix (`tokens < in.tokens`) // and installs the GDN planes whole from the snapshot (`q4nx_forward.cpp:326-341`; its only refusals are a plane of // the wrong size and a `tokens > in.tokens` extension), so a prefix restore here would leave KV at the prefix with // the recurrence still at the snapshot's last position -- the mixed state this deferral exists to avoid. The // forward's snapshot_state()/StateSnapshot API exists and this adapter deliberately does not use it. Dead Code The members snap_ and snap_pos_ are declared but never used. The snap_ member is a unique_ptr<Q4nxNpuForward::StateSnapshot> that is declared but never touched, and snap_pos_ is assigned but never read. These dead code elements should be removed to improve code clarity and maintainability. Q4nxNpuForward& fwd_; Missing Override The last_hidden_rows() method was previously not marked as override, which could lead to silent failures if the base class signature changes. This has been corrected to ensure proper override behavior and prevent the failure mode that previously cost 1 token/pass. const std::vector<float>& last_hidden_rows() const override { return fwd_.last_hidden_rows(); }

[bong-water-water-bong]

VERDICT: READY

Re-verdicted at 73be3d41. Only Merge branch 'main' since 6f3c6d58, and this PR's one file is byte-identical across the two heads (npu/mtp/forward_base.h), so nothing it says moved; the merge brings other lanes' work (#1142, #1138, #1140, #1139, #1136, #1134, #1133, #1132, #1131, #1130, #1124).

My nit stays fixed as reviewed at 6f3c6d58: restore_state() accepts a prefix and installs the GDN planes whole (q4nx_forward.cpp:326-341; its only refusals are tokens > in.tokens, a per_pos mismatch and a give_whole size), and the override on last_hidden_rows() still turns a drifted signature into error: marked 'override', but does not override while the same drift without it links silently.