Skip to content

refset, tools: mainline's Qwen3.6 MTP step taps as a reference set - #242

Closed
midagedev wants to merge 18 commits into
mainfrom
q35m2b
Closed

midagedev wants to merge 18 commits into
mainfrom
q35m2b

Conversation

@midagedev

@midagedev midagedev commented Oct 15, 2026 •

Copy link
Copy Markdown
Owner

Part of #30 (qwen35moe NextN MTP draft), step M2b-1: mainline's MTP graph as a tap-by-tap reference set. The next step (M2b-2) holds the card's draft step to it in gate_qwen35moe_mtp. Its one gate edit: gate_qwen35moe_mtp takes the draft file's name from refset::arch::qwen35moe::mtp::MODEL instead of a copy of it; no clause, band or pin moves. It builds on #236's tools/ref/ref_io.h (merged as d0cbd4b). Rebased onto main 73c307c4 at a4e71119; the PR's own diff equals the fold's at 2cfad01f on 4308ac0c (git patch-id --stable 0fd0a68a on both).

What

  • tools/ref/q35mtp_ref.cpp, a harness linked against mainline 36a73916:
    • It loads the file with load_mtp.
    • It runs the trunk over 64 prompt ids with the next-token rows kept (llama_set_embeddings_nextn).
    • Then, in an MTP context (LLAMA_CONTEXT_TYPE_MTP), it runs every step 0..=62 in order, with token ids[i+1], position i and the trunk's row h[i] (llama_batch_ext_set_embd_token, as common.cpp:2220 does; set_embd_state is a stub in this tree).
    • It dumps the named graph tensors (--tap, repeatable) at rows {0, 5, 31, 62}. The recipe's default is 17 taps, from mtp_hnorm to result_output, plus the router's logits, top-k, weights and the shared gate.
  • # mtp <layers> <steps> is written only after the MTP context ran every step, and # complete only by the final rename.
  • build-q35mtp-ref.sh records the source sha256, the commit and libllama's md5. dump-q35mtp.sh refuses a binary that differs from that record, stages, and runs under gpu-gate.sh.
  • crates/refset: the family mtp-qwen35moe-mainline (the A1 file, build 36a73916e) and the reader mtptaps (by tap name and row). The family check refuses by name a set of another file, build or architecture, or one without # mtp.

공통화한 것 (Commonized)

  • The family rides the existing Identity and check_set: one new arm, no second check.
  • tools/ref/ref_io.h (refset: Nemotron 3 Nano last-position logits, read from one reference set #236) is the one owner of the mainline and ik harnesses' input and output contract. q35mtp_ref.cpp uses its parse_i32, read_token_file, write_raw and the new write_bytes (write_raw is write_bytes of f32 rows). dump_ref, dump_mtp and dump_draft read --tokens-count with its parse_i64, so 64x is refused by name instead of read as 64. Each build record and run check hashes ref_io.h beside the harness source; the installed ik dumpers rebuild once (just build-ref-dump, build-ref-dump-mtp, build-ref-dump-draft) before their next dump.
  • The MTP taps tests take clefvis's testkit::scratch, the crate's one scratch-directory owner.
  • columns::need is the one owner of "a data row needs its column line" for the MTP, vision and visref readers (two local copies removed).
  • The reader takes the nemotron logits reader's shape (At, one(), the header loop).
  • The prompt is corpus-prose.ids' first 64 ids: the d1k set's first 64, the same md5.

Evidence

Cards: RTX A6000, driver 615.71.09, CUDA 13.4. Base origin/main e6050c5, head 0378d6e (parent 3915120; the trailer fold is 7939746, parent b177903; 1d97576 is the M2b-1 commit rebased).

nvidia-smi: NVIDIA GeForce RTX 3090, 615.71.09; NVIDIA RTX A6000, 615.71.09.

  • Dump: job s590 on the A6000. 42/42 layers offloaded, and the CUDA MTP context ran. gpu-gate.sh: q35mtp_dump ran 4 s (exit 0).

    • The set is in place: 71 files, # complete 68, # build 36a73916e, # mtp 1 63, # rows 0 5 31 62.
    • Tapped against untapped logits: worst |Δ| = 0 over the 63 steps, argmax equal 63/63. Tapping does not move the oracle.
  • Mainline's MTP acceptance (printed, not pinned), as gate (s) counts it: 11/16 of the last 16 steps (68.8 %), 47/63 over every step. Our card draft, gate (s): 12/16 (75 %). A CPU trial of the same harness gave 13/16 and 49/63.

  • just gate-refset: hw_refset: mtp-qwen35moe-mainline … PASS, 175 sets, 0 failed; cargo test -p bloomery-refset --lib 200 passed.

  • just refset-check: 338 sets, 0 failed. tools/check-recipes.sh: ok. recipes.py check --fill: 0 rows.

  • The dump and the FAIL-first ran before a machine reboot at 13:36. The gate runs are from before it; the reader tests were run again after it.

  • The review fold, job s672 on the A6000: q35mtp_ref rebuilt from this tree (the shared header in its source sha256; that run was before the move to ref_io.h). A trial dump to a scratch set ran: gpu-gate.sh: q35mtp_dump ran 4 s (exit 0). Its 70 data files are byte-identical to the published set, and the manifests are equal outside the flags and date lines. The scratch set was removed.

  • Malformed options, with the model path absent so that only the parse is under test:

    • The new q35mtp_ref (-c bad, -ngl 9x, -t '', --tokens-count 64x) and the new hidden_ref (-ngl bad, -ub 1.5, -c 99999999999, --tokens-count 64x, job s671) each exit 2 with <tool>: <opt> takes an integer, not '<value>'.
    • The old binaries go on to the model load: q35mtp_ref built from 0e4dc85 with -c bad and the installed hidden_ref with -ngl bad both reach failed to load /nonexistent.gguf, rc 1.
  • cargo test -p bloomery-refset --lib: 200 passed.

  • The second review fold (7939746): the # complete <tap rows> 0 trailer must count the tap rows the manifest holds and be its last line. The published set's trailer is # complete\t68\t0 over 68 tap rows. just refset-check after this fold: 338 sets, 0 failed, mtp-qwen35moe-mainline … PASS. dump-q35mtp.sh --cpu exits 64 unless Q35MTP_SET names a trial set, so the card's family set is never replaced by a CPU run (bash tools/ref/dump-q35mtp.sh --cpu → dump-q35mtp: --cpu writes a trial set; name it with Q35MTP_SET …, rc 64, before any profile or binary check). dump-q35mtp.sh holds a lock on the destination (<set>.lock, flock -n, exit 75 when another writer holds it) for the run, card or --cpu, through the install and the staging cleanup.

  • The move to ref_io.h (head f35de2fd, on refset: Nemotron 3 Nano last-position logits, read from one reference set #236's 433f8e3d), box CPU jobs, each binary built into a scratch directory:

    • dump_ref (build-dump.sh) and q35mtp_ref (build-q35mtp-ref.sh) built; --tokens-count 64x exits 2 with dump_ref: --tokens-count wants an integer from 1 to 9223372036854775807, got 64x and q35mtp_ref: --tokens-count wants an integer from 2 to 2147483647, got 64x.
    • dump_mtp and dump_draft: their build scripts rebuild the ik libraries under a timing lease, so they were compiled -fsyntax-only against their profiles' ik headers instead: both rc 0.
    • The NUL-in-a-record refusal is refset: Nemotron 3 Nano last-position logits, read from one reference set #236's ref_io.h with its ref_io_test case (just test-ref-io: ok).
  • cargo test -p bloomery-refset --lib: 210 passed.

  • Head 05031f98 (Shingler de8dba9 fold, on main d0cbd4bb):

    • dump_ref --tokens parses each field with parse_i32: --tokens bad,42 → dump_ref: --tokens wants an integer from 0 to 2147483647, got bad, rc 2; --tokens 64x the same (box build).
    • ref_io.h's write_raw(path, data, size, count) replaces dump_ref, dump_mtp and dump_draft's own writers; dump_mtp and dump_draft compile -fsyntax-only, rc 0.
    • q35mtp_ref --rows parses each piece with parse_i64: +5, 5 and 5, are refused by name, 0,5,31,62 reaches the model load (box build).
    • The tap file size check reads through columns::read_bytes. The reader test reads step 1, whose two argmaxes differ.
    • cargo test -p bloomery-refset --lib: 211 passed; just test-ref-io: ok.
    • Head 3d9df876: two cases hold file_name's backslash and NUL refusals (the two characters dropped → a tap file with a backslash red, killed; the clean run green).
  • Head de8dba95: the nemotron tests take testkit::scratch (tags prefixed nemotron-); TapSet::ids refuses an empty id line as nemotron.rs and ref_io.h do (the refusal dropped → an empty line among the prompt ids red, killed; the clean run green).

  • Head d39c851d: the nemotron reader takes columns::need; qwen35moe.sh's d1k step sets read MTP_IDS and MTP_IDS_SHA256, so the prose ids path and pin are written once; a case swaps two step rows' numbers (s.row != i dropped → red, killed; the clean run green).

  • Head a05aa083, restacked on refset: Nemotron 3 Nano last-position logits, read from one reference set #236's 07aa96b1: the nemotron logits read is columns::read_f32 plus refset: Nemotron 3 Nano last-position logits, read from one reference set #236's finite check; dump-q35mtp.sh takes ref-paths.sh's drop_incomplete_stage. cargo test -p bloomery-refset --lib: 211 passed.

  • Head 6da0dc73: columns::read_bytes and read_f32 are the one f32 file reader of the MTP step and nemotron sets; TapSet::read hands the trunk row to trunk_row. cargo test -p bloomery-refset --lib: 210 passed.

  • Head 2edf04b8: dump_ref, dump_mtp and dump_draft drop their own read_token_file; ref_io.h's reader runs in all of them. dump_ref built on the box: an ids file with a NUL inside a record → invalid token id '2', rc 2; --tokens-count 64x → rc 2. dump_mtp and dump_draft compile -fsyntax-only, rc 0. TapSet::read hands the trailer to check_trailer.

  • Head e6c2a4bd: columns::one and columns::file_name serve the MTP step and nemotron readers (file_name also refuses \\ and NUL); ref_io.h's write_ids writes both mainline harnesses' ids; dump_ref's zero-count and nemotron_ref's -ub checks that could not fire are gone. q35mtp_ref, nemotron_ref and dump_ref compile (-fsyntax-only on the box, rc 0); just test-ref-io: ok. Two reader cases added (a lost step row; a trunk file outside the set), each FAIL-first killed by its mutant, the clean run green.

  • Head 7a9ef349 (Shingler 3d9df87 fold): check_taps refuses a repeated # rows entry; columns::i32s_of is the one little-endian i32 decoder (dsref.rs and mtpref.rs copies removed, the tap read takes it). New cases: repeated rows hiding taps at a step that does not exist, no # rows and no tap rows, no # tokens_count line, no trunk row. FAIL-first in one --clean once run: the repeated-row guard, the empty-set guard and the tokens-count check each dropped → its case red, all killed; the clean run green. cargo test -p bloomery-refset --lib: 211 passed.

  • Head db5c9370: a case removes only the tap rows under # rows 0 2 (the guard's || made && → red, killed; the clean run green).

  • Head 2cfad01f (Shingler db5c937 fold, on main 4308ac0c): columns::read_f32 refuses a non-finite value by index, so every MTP tap and trunk file and the nemotron logits share one finite check (check_taps reads each f32 tap through it); build-q35mtp-ref.sh stops when git status on the tree fails; q35mtp_ref refuses to write a set whose tapped logits differ from the untapped ones or whose argmax differs at any step. Two cases: a NaN in a tap file (index 0), an infinity in the trunk file (index 3). FAIL-first (--clean once): the finite check dropped → both cases red, killed; the clean run green. cargo test -p bloomery-refset --lib: 216 passed. just gate-gpu-qwen35moe-mtp at 2cfad01f on the A6000 (job s825): gpu-gate.sh: gate_qwen35moe_mtp ran 9 s (exit 0), gate_qwen35moe_mtp: PASS. q35mtp_ref builds against llama.cpp 36a73916 (job s828, rc 0). On a4e71119: cargo test -p bloomery-refset --lib 217 passed (main added one); tools/check-recipes.sh: ok.

FAIL-first (tools/mutant-run.sh, each mutant alone)

  • F1, the reader defaults a missing # mtp: the_family_refuses_a_set_of_another_file_build_or_arch_or_without_the_mtp_context FAILED, killed.
  • F2, the build check commented out: the same test FAILED, killed. The clean run is green.
  • F3, the trunk's logit at id 7 set to NaN at position 0 (a copy tree, job s661 on the A6000): q35mtp_ref: non-finite logit nan at id 7, q35mtp_dump ran 3 s (exit 1), no set installed. Killed.
  • The reader's checks (one run, --clean once, the gate cargo test -p bloomery-refset --lib a_set_whose_files_or_rows_disagree_is_malformed). Each mutant was killed by its own case, and the clean run is green:
    • the step count check dropped;
    • the trunk length check dropped;
    • the "row is no step" arm dropped;
    • the tap count check dropped;
    • mtp.steps + 1 in u32 (an overflow panic).
  • The trailer checks (one run, --clean once, the same gate): the count check dropped → a tap name gone at every row, the trailer unchanged red; the last-line check dropped → a line after the trailer red. Both killed, the clean run green.
  • The reader's trailer zero field and step position (one run, --clean once): f[2] != "0" dropped → a trailer whose second field is not 0 red; s.pos != s.row dropped → a step at another position than its row red. Both killed, the clean run green.

Follow-up (not in this PR)

  • mtptaps.rs lets a repeated # rows, # model or # tokens_count header replace the earlier one (the writer emits each once; it takes a hand-edited manifest). Refusing a header already set, as step and tap rows do through seen, is a reader change of its own.
  • dump-q35mtp.sh removes the old set before it moves the new one in; a failed mv leaves the family without a set until the next dump. Moving the old set aside first is a change of its own.
  • nemotron.rs does not require its # complete trailer on the last line, as mtptaps.rs does; the shared trailer rule is one change with the scans below.
  • crates/refset has eleven At types and four # complete scans; one tagged At and one scan touch nine files, a change of their own.
  • build-dump-draft.sh records only the ik build in dump_draft.build, so dump-draft.sh does not refuse a dump_draft built before this PR (which reads --tokens-count 64x as 64). Rebuild it (just build-ref-dump-draft) before its next dump; recording the source and ref_io.h sha256 there, as build-dump.sh does, is a change of its own.

Pre-review CLEAN at 2cfad01f215003f4a72326ea3a389f90eca65752 (the same patch as a4e7111).

🤖 Generated with Claude Code

@github-actions github-actions Bot added host Host crates: qdot, threads, engram, engram-lab, runtime, sampler, levers, decision, vision, refset tools Tools, recipes, CI and the toolchain: tools/, justfile, .github/, Cargo labels Oct 15, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: MUST_FIX: invalid values reach completed sets, code is duplicated, and a comment names an absent consumer.

Must fix

1. Reject non-finite logits at the argmax owner

tools/ref/q35mtp_ref.cpp:134 Kind B: non-finite logits silently yield token ids. With x[0] = NaN, every comparison is false, so trunk target_next becomes 0 (366) and is published (513). Untapped NaNs are also ignored by std::max (435), leaving a plausible delta in the manifest (508).
Cost: protect these records before gates depend on them. Replace first_argmax:

static int first_argmax(const float * x, int n) {
    int best = 0;
    for (int i = 0; i < n; ++i) {
        if (!std::isfinite(x[i])) {
            fprintf(stderr, "q35mtp_ref: non-finite logit at id %d\n", i);
            std::exit(1);
        }
        if (x[i] > x[best]) best = i;
    }
    return best;
}

2. Reject malformed numeric options

tools/ref/q35mtp_ref.cpp:274 -c bad becomes zero, then the prompt length (303). -ngl bad becomes zero GPU layers (308). With otherwise valid inputs, inference can finish and publish a completed set instead of rejecting the option.
Cost: reject bad options before reference sets depend on this recipe. Replace all four atoi calls with integer:

static int integer(const char * s) {
    char * end = nullptr;
    errno = 0;
    const long v = strtol(s, &end, 10);
    if (end == s || *end || errno || v < INT_MIN || v > INT_MAX) {
        fprintf(stderr, "q35mtp_ref: invalid integer %s\n", s);
        std::exit(2);
    }
    return (int) v;
}

3. Share the existing oracle input and output contracts

tools/ref/q35mtp_ref.cpp:57 copies tools/ref/hidden_ref.cpp:70. write_bytes (84) also repeats write_raw (97) there: exact writes and close failures share a contract.
Cost: prevent policy drift (R14). Include this tools/ref/ref_common.h from both harnesses; delete the four local functions:

#pragma once
#include "llama.h"
#include <cerrno>
#include <climits>
#include <cstdint>
#include <cstdio>
#include <cstdlib>
#include <fstream>
#include <string>
#include <vector>

inline bool read_token_file(const std::string & path, long long count,
                            std::vector<llama_token> & ids) {
    std::ifstream in(path);
    if (!in) { fprintf(stderr, "ref: cannot read %s\n", path.c_str()); return false; }
    std::string line;
    while ((long long) ids.size() < count && std::getline(in, line)) {
        if (!line.empty() && line.back() == '\r') line.pop_back();
        char * end = nullptr;
        errno = 0;
        const long v = strtol(line.c_str(), &end, 10);
        if (end == line.c_str() || *end || errno || v < 0 || v > INT32_MAX) {
            fprintf(stderr, "ref: invalid token id in %s\n", path.c_str());
            return false;
        }
        ids.push_back((llama_token) v);
    }
    if ((long long) ids.size() != count) {
        fprintf(stderr, "ref: short prompt in %s\n", path.c_str());
        return false;
    }
    return true;
}
inline bool write_bytes(const std::string & path, const void * data, size_t bytes) {
    FILE * f = fopen(path.c_str(), "wb");
    if (!f) { fprintf(stderr, "ref: cannot write %s\n", path.c_str()); return false; }
    const bool ok = fwrite(data, 1, bytes, f) == bytes;
    if (fclose(f) != 0 || !ok) {
        fprintf(stderr, "ref: short write on %s\n", path.c_str());
        return false;
    }
    return true;
}

Replace the MTP ids and hidden-state write calls:

if (!read_token_file(tokens_file, n, ids)) return 2;
if (!write_bytes(out + "/result_norm.0.f32", rows.data(), rows.size() * sizeof(float))) return 1;

Hash the header too. In both build scripts:

SRC_SHA=$(cat "$SRC" "$HERE/tools/ref/ref_common.h" | sha256sum | cut -d' ' -f1)

In hidden.sh and dump-q35mtp.sh, respectively:

want_src=$(cat "$HERE/tools/ref/hidden_ref.cpp" "$HERE/tools/ref/ref_common.h" | sha256sum | cut -d' ' -f1)
want_src=$(cat tools/ref/q35mtp_ref.cpp tools/ref/ref_common.h | sha256sum | cut -d' ' -f1)

4. Reuse the scratch-directory owner

crates/refset/src/arch/qwen35moe/mtp/tests.rs:30 duplicates crates/refset/src/clefvis/testkit.rs:9, including cleanup and directory creation.
Cost: keep one cleanup policy (R14). Delete dir; replace the path import with:

use crate::clefvis::testkit::scratch as dir;
use std::path::Path;

5. Remove the nonexistent gate consumer

tools/ref/dump-q35mtp.sh:6 names gate_qwen35moe_mtp clause (m) as a consumer. That gate is absent.
Cost: correct coverage before users rely on it. Replace line 6:

# mtp-qwen35moe-mainline checks.

Checked

Confirmed both harness I/O pairs and the scratch owner. Both deduplication fixes reduce elements.
Traced invalid values to manifest publication. Tests inspected, not run. Inline replacements are valid on added lines. No existing gate pins changed. No escape-catalog match or hot path touched.

Notes

GPU evidence lacks the required nvidia-smi output.

🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)

Filed outside this PR: #243

Comment thread tools/ref/q35mtp_ref.cpp Outdated
// The first index of the largest value: the argmax a tie gives to the lower id.
static int first_argmax(const float * x, int n) {
int best = 0;
for (int i = 1; i < n; ++i) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reject non-finite logits before argmax turns them into ordinary token ids.

Suggested change
for (int i = 1; i < n; ++i) {
for (int i = 0; i < n; ++i) {
if (!std::isfinite(x[i])) { fprintf(stderr, "q35mtp_ref: non-finite logit at id %d\n", i); std::exit(1); }

Comment thread tools/ref/dump-q35mtp.sh Outdated
# the prompt's first MTP_PROMPT prose ids, fed teacher-forced (step i: token ids[i+1], position i, the trunk's h_nextn
# row i), every step run in order and the graph nodes MTP_TAPS dumped at the steps MTP_ROWS, by q35mtp_ref
# (tools/ref/q35mtp_ref.cpp), into $BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64 — the set the refset family
# mtp-qwen35moe-mainline checks and gate_qwen35moe_mtp's clause (m) reads.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tree has no gate_qwen35moe_mtp consumer.

Suggested change
# mtp-qwen35moe-mainline checks and gate_qwen35moe_mtp's clause (m) reads.
# mtp-qwen35moe-mainline checks.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: MUST_FIX: Incomplete sets pass validation, and concurrent dump writers can corrupt the oracle.

Must fix

1. Validate the completion counts

crates/refset/src/mtptaps.rs:211 discards both trailer fields. In the test fixture, delete both ffn_moe_topk manifest rows but retain # complete\t4\t0. The two remaining taps pass check_taps: it derives the expected names from the surviving rows at :378. check_family and Family::check_set then succeed, so refset-check reports PASS for an incomplete set.
Reject this before the set is used as an oracle. Replace this match arm:

"# complete" => {
    if f.len() != 3 || f[2] != "0"
        || number(f[1], &at)? as usize != set.taps.len()
        || text.lines().skip(i + 1).any(|l| !l.is_empty())
    {
        return Err(RefError::malformed(&at, "invalid completion trailer"));
    }
    set.complete = true;
}

Add a refusal case that removes one tap name at every selected row without changing the trailer.

2. Serialize writers to the destination

tools/ref/dump-q35mtp.sh:58 gives CPU and GPU runs the same staging directory. Start --cpu alongside a GPU dump to the same destination: CPU mode bypasses the card runner at :25, and both runs delete and write SET.staging. One can erase the other's files; interleaved writes can be installed under one run's manifest at :77–78.
Protect the reference data before concurrent runs corrupt it. Replace lines 57–60 with a destination lock held through EXIT cleanup and installation:

SET=${Q35MTP_SET:-$BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64}
mkdir -p "$(dirname "$SET")"
exec {set_lock}> "$SET.lock"
flock -n "$set_lock" || {
  echo "dump-q35mtp: $SET already has a writer" >&2
  exit 75
}
STAGE=$SET.staging
rm -rf "$STAGE"

3. Share the missing-column rule

crates/refset/src/mtptaps.rs:577 repeats crates/refset/src/vision.rs:256 and crates/refset/src/visref.rs:358: a data row requires its column header (R14).
Consolidating now removes two helper definitions and gives this validation rule one owner. Move the existing helper into columns.rs, replacing all three local definitions:

pub(crate) fn need<'a>(
    cols: &'a Option<Columns>,
    what: &str,
    at: &dyn Display,
) -> Result<&'a Columns, RefError> {
    cols.as_ref().ok_or_else(|| {
        RefError::malformed(at, format!("a {what} row before its column line"))
    })
}

Add this import in vision.rs and mtptaps.rs:

use crate::columns::need;

Add this import in visref.rs:

use crate::columns::need as need_cols;

Replace the three mtptaps.rs calls:

let c = need(&trunk_cols, "trunk", &at)?;
let r = need(&step_cols, "step", &at)?.row(line, &at)?;
let r = need(&tap_cols, "tap", &at)?.row(line, &at)?;

Checked

Duplicates: confirmed the three missing-column helpers; consolidation uses the existing columns module and reduces definitions and lines.
Complexity: diagnostics reports new TapSet::read at 46 and Family::check_set at 24→26; neither is a separate finding.
Silent failure: followed the ignored completion count through check_taps, check_family, Family::check_set, and the PASS output at refset_check.rs:47–57.
Tests, by inspection: the malformed-set cases do not remove an entire tap name or alter the completion count.
Gates: no existing bands or pins moved. Escape catalog: no exception changes these classifications.

Notes

Changed shell files pass bash -n. Rust and GPU tests were not run.

🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: MUST_FIX: malformed inputs still become valid token IDs or prompt lengths.

Must fix

1. Reject embedded NULs in token-file lines

tools/ref/ref_common.h:47 accepts \0bad as token 0 and 7\0bad as token 7. strtol stops at the embedded NUL, so checking *end does not check the whole string. Both harnesses feed the accepted ID to inference and can write a complete reference set.
Fixing the shared reader now prevents its callers from publishing more sets from malformed input. Replace the condition with:

        if (end == line.c_str() || end != line.c_str() + line.size() || errno != 0 ||
            v < 0 || v > INT32_MAX) {

Keep regression cases for both NUL-containing lines.

2. Close the malformed-option bug on the sibling dumpers

tools/ref/dump_mtp.cpp:618, tools/ref/dump_draft.cpp:529 and tools/ref/dump_ref.cpp:567 still parse --tokens-count 64x as 64. With valid remaining arguments, they run and publish a set instead of refusing the option. This is the same numeric-option defect this PR fixes in hidden_ref.
The new parser makes all three remaining repairs mechanical now. Include ref_common.h in each file and replace the assignments, respectively:

tokens_count = ref_integer("dump_mtp", "--tokens-count", argv[++i]);
tokens_count = ref_integer("dump_draft", "--tokens-count", argv[++i]);
tokens_count = std::max(0, ref_integer("dump_ref", "--tokens-count", argv[++i]));

Hash the header alongside the source in build-dump.sh / dump.sh and build-dump-mtp.sh / dump-mtp.sh, as the changed mainline runners do.

Non-blocking

⚠ Retain the malformed-option checks as an automated regression: tools/ref/ref_common.h:19 is exercised by the commands in the PR body, but the new Rust tests do not call it. Add those cases to the host checks. Replacing ref_integer with atoi must turn the check red.

Checked

Duplicates: 63 new definitions checked. All three dryer pairs share structure across different domains. The remaining ID-reader copies predate this change.
Complexity: TapSet::read is new at 51; its separate stages are headers, data rows and trailer validation. Family::check_set rises 24→26 through one additional identity arm.
Silent failure: six parser sites followed to consumers. The two surviving failure paths are above.
Tests (static mutation check): reversing byte order at crates/refset/src/mtptaps.rs:580, dropping the build check at crates/refset/src/mtptaps.rs:305, and dropping trailer validation at crates/refset/src/mtptaps.rs:214 each breaks a corresponding new test assertion.
Gates: no existing pins or thresholds moved. Destination locking, step positions, row coverage and trailer counts checked.
Escape catalog: no C1–C4 pattern matched. No engine hot path changed.

Notes

Validation was static; runtime tests and GPU gates were not run. The PR body supplies card-run evidence.

🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)

Comment thread tools/ref/ref_common.h Outdated
char * end = nullptr;
errno = 0;
const long v = strtol(line.c_str(), &end, 10);
if (line.empty() || *end != '\0' || errno != 0 || v < 0 || v > INT32_MAX) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An embedded NUL hides unparsed bytes; require a parsed integer spanning the whole line.

Suggested change
if (line.empty() || *end != '\0' || errno != 0 || v < 0 || v > INT32_MAX) {
if (end == line.c_str() || end != line.c_str() + line.size() || errno != 0 ||
v < 0 || v > INT32_MAX) {

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: MUST_FIX: malformed token-list input still reaches decode, and exact-write validation has multiple owners.

Must fix

1. Apply the numeric parser to the sibling token-list option

tools/ref/dump_ref.cpp:533 --tokens bad,42 becomes tokens 0,42; 64x becomes 64. With the required model and write options, these valid IDs reach llama_decode at line 779 and produce an oracle for a different prompt. This PR fixes the same malformed-number behavior in --tokens-count and token files; this sibling remains reachable.
Closing this sibling now prevents another reference-set repair. Replace lines 531–534:

const std::string list(argv[++i]);
for (size_t at = 0;;) {
    const size_t end = list.find(',', at);
    const std::string field = list.substr(at, end == std::string::npos ? end : end - at);
    int id = 0;
    if (!parse_i32("dump_ref", "--tokens", field.c_str(), 0, id)) return 2;
    tokens.push_back((llama_token) id);
    if (end == std::string::npos) break;
    at = end + 1;
}

2. Finish consolidating the exact-write contract

tools/ref/ref_io.h:88 repeats the open/write/close checks in tools/ref/dump_ref.cpp:203, tools/ref/dump_mtp.cpp:229 and tools/ref/dump_draft.cpp:175. All three now include this header. R14 requires one owner for these checks.
Consolidating them now removes three implementations before more callers attach. Add this overload to the header and delete those three local definitions; their callers stay unchanged:

inline bool write_raw(const std::string & path, const void * data, size_t size, size_t count) {
    return write_bytes(path, data, size * count);
}

Non-blocking

⚠ Check a step whose argmaxes differ: crates/refset/src/arch/qwen35moe/mtp/tests.rs:138 checks row 2, where both argmaxes are 8. Changing mtp_argmax to read target_next at crates/refset/src/mtptaps.rs:581 passes every new assertion.
Using the unequal row already in the fixture prevents this regression without another test. Replace the two lines:

let s = set.step(1).unwrap_or_else(|e| panic!("{e}"));
assert_eq!((s.token, s.pos, s.mtp_argmax, s.target_next), (12, 1, 6, 7));

Checked

Duplicates: the shared writer and all three local copies confirmed; the fix removes three definitions and adds one overload.
Complexity: diagnostics report new TapSet::read (41), new LogitsSet::read (24), and Family::check_set 24→28.
Silent failure: the token-list fallback reaches decode; the shared parser rejects malformed fields.
Tests: static mutation audit confirms that the existing unequal row catches the field-mapping error missed by the current assertion.
Gates: Rust/C++ suites and card gates were not rerun during validation.
Escape catalog: no C1–C4 pattern underlies the retained findings. These paths load reference sets or run reference tools.

🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)

Comment on lines +138 to +139
let s = set.step(2).unwrap_or_else(|e| panic!("{e}"));
assert_eq!((s.token, s.pos, s.mtp_argmax, s.target_next), (13, 2, 8, 8));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the existing row with unequal argmaxes so a swapped field mapping fails this assertion.

Suggested change
let s = set.step(2).unwrap_or_else(|e| panic!("{e}"));
assert_eq!((s.token, s.pos, s.mtp_argmax, s.target_next), (13, 2, 8, 8));
let s = set.step(1).unwrap_or_else(|e| panic!("{e}"));
assert_eq!((s.token, s.pos, s.mtp_argmax, s.target_next), (12, 1, 6, 7));

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: MUST_FIX: malformed manifests pass the family check, and the new reader adds a third i32 decoder.

Must fix

1. Reject repeated entries in # rows (kind A)

crates/refset/src/mtptaps.rs:395: in the existing three-step fixture, set # rows to 0 0 2, add both tap names at row 5 using the existing files, and set the trailer count to 6. All checks pass: six taps equal three declared rows times two names. refset-check reports PASS although step 5 does not exist.
Reject this before consumers depend on the accepted tap rows. Insert after the empty-set guard in check_taps:

if self.rows.iter().collect::<HashSet<_>>().len() != self.rows.len() {
    return Err(RefError::malformed(
        at.display(),
        "duplicate # rows entry",
    ));
}

Add this case to the existing malformed-set test. Removing the new guard must make it fail:

at(
    "repeated rows hide taps at a nonexistent step",
    &[
        "tap\tmtp_eh_proj\t5\tf32\t2\tmtp_eh_proj.r0.f32",
        "tap\tffn_moe_topk\t5\ti32\t2\tffn_moe_topk.r0.i32",
    ],
    &|d| swap(d, "# rows\t0\t2", "# rows\t0\t0\t2"),
    "duplicate # rows entry",
);

2. Reuse one little-endian i32 decoder (kind B, R14)

crates/refset/src/mtptaps.rs:504 repeats crates/refset/src/dsref.rs:573 and crates/refset/src/mtpref.rs:637. All three own the same four-byte little-endian conversion.
Consolidating now replaces two local helper definitions with one common definition and removes the new inline copy. Move the existing decoder to columns.rs:

pub(crate) fn i32s_of(b: &[u8]) -> Vec<i32> {
    b.as_chunks::<4>()
        .0
        .iter()
        .map(|c| i32::from_le_bytes(*c))
        .collect()
}

Delete both local i32s_of definitions and import use crate::columns::i32s_of; in their modules. Replace the inline return in tap_i32 with:

Ok(crate::columns::i32s_of(&bytes))

Checked

Duplicates: all three i32 decoders confirmed. The three dryer pairs share shape, not knowledge.
Complexity: TapSet::read is new at 41; headers and data-row dispatch are its separable stages. Family::check_set grows 26→28 through one ordinary identity arm.
Silent failure: two suppression sites followed. The test's .ok() is asserted; failed step lookup returns named Malformed.
Tests: static mutation review. Selecting row 0 at tap line 465 breaks the finished-set test; dropping check_build at line 279 breaks the identity-refusal test; dropping step consistency at line 344 breaks the malformed-set test. Repeated-row padding is uncovered.
Gates: no pins or thresholds moved; family FAIL-first evidence is supplied.
Escape catalog: C1–C4 do not apply to the retained findings. No engine hot path changed.

Notes

No local builds or test runs were performed. Test mutation results above are from code inspection.

🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)

Filed outside this PR: #261

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: MUST_FIX: invalid reference data and failed provenance checks can return success.

Must fix

1. Reject non-finite MTP reference values (kind A)

crates/refset/src/mtptaps.rs:410 and crates/refset/src/columns.rs:163: replace one f32 value in a tap or trunk file with NaN, keeping its length. TapSet::open succeeds and refset-check prints PASS. The writer rejects these values, but the reader admits them.
Close this before these sets become the next gate's oracle.

Replace columns::read_f32 with:

pub(crate) fn read_f32(path: &Path, want: u64, file: &str) -> Result<Vec<f32>, RefError> {
    let values: Vec<f32> = read_bytes(path, want, file)?
        .as_chunks::<4>()
        .0
        .iter()
        .map(|c| f32::from_le_bytes(*c))
        .collect();
    if let Some(i) = values.iter().position(|v| !v.is_finite()) {
        return Err(RefError::malformed(
            path.display(),
            format!("non-finite value at index {i}"),
        ));
    }
    Ok(values)
}

In check_taps, replace the file-check loop with:

        for t in &self.taps {
            match t.elem {
                Elem::F32 => { self.tap_f32(&t.name, t.row)?; }
                Elem::I32 => { read_bytes(&self.dir.join(&t.file), u64::from(t.n), &t.file)?; }
            }
        }

Remove the now-redundant finite check in nemotron::LogitsSet::logits; change its test's expected text to non-finite value at index 1. Add NaN/infinity cases for both MTP file kinds; removing the shared finite check must make them red.

2. Stop when the clean-tree query fails (kind A)

tools/ref/build-q35mtp-ref.sh:23: a corrupt or unreadable Git index makes git status exit 128 with empty stdout. The conditional succeeds as a clean-tree check; rev-parse HEAD can still succeed, so compilation and the commit-labelled build record continue. A shell reproduction returned 0 after the failed query.
Fix this before an unchecked oracle build is published. Replace line 23 with:

dirty=$(git_lcpp status --porcelain --untracked-files=no)
if [ -n "$dirty" ]; then

3. Use one Rust owner for the MTP model path (kind B, R14)

crates/refset/src/arch/qwen35moe/mtp.rs:12 repeats crates/gpu-gates/src/bin/gate_qwen35moe_mtp.rs:71. Both identify the same draft file, and the gate already depends on refset.
Remove the duplicate before the gate and reference family acquire separate file updates. Keep the family's constant; replace the gate's DRAFT_FILE definition with:

    use refset::arch::qwen35moe::mtp::MODEL as DRAFT_FILE;

Non-blocking

⚠ Enforce the untapped comparison: tools/ref/q35mtp_ref.cpp:381 computes distances, but a nonzero distance still reaches # complete and installation. Add a refusal before completion, or state the admissible difference and its evidence. The supplied run establishes zero only for that run.

Checked

Duplicates: confirmed the model-path copy and the gate's existing dependency on refset.
Complexity: TapSet::read dispatches manifest headers and rows; Family::check_set gains one identity arm.
Silent failure: followed the MTP reader through the family's check to refset-check's PASS output. A failed Git-status stub bypasses the original conditional (exit 0); the proposed assignment stops with exit 128.
Tests: the wrong-build fixture would fail if check_build were removed. The MTP tests contain no non-finite-value refusal case. Rust and GPU tests were not run during validation.
Gates: no existing pin or band moved; the PR reports FAIL-first runs for the family identity checks.
Escape catalog: no exact C1–C4 pattern; the lost command-substitution status is related to C1. No engine hot path changed.

🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)

Filed outside this PR: #265

Comment thread tools/ref/build-q35mtp-ref.sh Outdated
SRC=$HERE/tools/ref/q35mtp_ref.cpp
L=$MTP_LCPP
git_lcpp() { git -c safe.directory="$L" -C "$L" "$@"; }
if [ -n "$(git_lcpp status --porcelain --untracked-files=no)" ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A failed git status can have empty stdout and bypass this check; capture it separately so set -e stops the build.

Suggested change
if [ -n "$(git_lcpp status --porcelain --untracked-files=no)" ]; then
dirty=$(git_lcpp status --porcelain --untracked-files=no)
if [ -n "$dirty" ]; then

midagedev and others added 15 commits October 15, 2026 20:18
A mainline harness (q35mtp_ref) runs the target's trunk with its next-token
rows kept, then the in-file MTP block in its own context step by step, and
dumps the named graph tensors at chosen rows under a manifest that carries
`# mtp` only when that context ran. The refset family
mtp-qwen35moe-mainline reads them by tap name and row and refuses a set of
another file, build or architecture, or one without the MTP context.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ct, and refuse a bad option or logit

tools/ref/ref_common.h owns the numeric option parse, the ids file read and the
exact binary write for hidden_ref and q35mtp_ref; each build record's source
sha256 covers the header. A numeric option that is not a whole integer ends the
run with 2 (Closes #243). q35mtp_ref's argmax ends the run with 1 on a
non-finite logit, so no token or distance is written from such a row. The MTP
taps tests take clefvis's scratch directory.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…'s one pin

The family reads clefvis's QVIS_LCPP_BUILD, the commit of the mainline tree
q35mtp_ref links against, instead of a second constant for it. The step-count
check compares in u64, so a `# mtp` line at the integer's end is malformed,
not an overflow. Four reader checks gain a case each: a short MTP run, a trunk
of another length, a tapped row that is no step, and a tap at an unnamed row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… writer dumps a set

The `# complete <tap rows> 0` trailer must count the rows the manifest holds
and be its last line, so a set that lost a tap name at every row is
malformed instead of passing. dump-q35mtp.sh takes a lock on the destination
for the run, card or --cpu, through the install and the staging cleanup.
columns::need is the one owner of "a data row needs its column line" for the
MTP, vision and visref readers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… step's position

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The family's set is the card's. A --cpu run (another activation form) no
longer replaces it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a malformed count

dump_ref, dump_mtp and dump_draft parse --tokens-count with parse_i64, so
64x is refused by name instead of read as 64. Each build record and run check
hashes ref_io.h beside the harness source.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and an ids file write

columns::one and columns::file_name serve the MTP step and nemotron readers;
file_name also refuses a backslash and a NUL. ref_io.h's write_ids writes the
ids both mainline harnesses record. Two option checks that could not fire are
gone, and the MTP reader's tests cover a lost step row and a trunk file
outside the set.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…trailer check is its own function

dump_ref, dump_mtp and dump_draft drop their own read_token_file, so the
header's reader (NUL, sign and space refused) is the one that runs.
TapSet::read hands the trailer to check_trailer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he trunk row is its own function

columns::read_bytes and read_f32 read a set's value file by count and byte
length; the nemotron logits read takes them. TapSet::read hands the trunk row
to trunk_row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… pin is written once, and a step row's number is held

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… refuses an empty id line

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tes go through ref_io.h

dump_ref parses each --tokens field with parse_i32, so `bad,42` is refused by
name instead of read as `0,42`. ref_io.h's write_raw(path, data, size, count)
replaces the three dumpers' own writers. q35mtp_ref parses each --rows piece
with parse_i64. The tap file size check reads through columns::read_bytes. The
reader test reads the step whose two argmaxes differ.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…UL refusals

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
midagedev and others added 3 commits October 15, 2026 20:18
…decoder serves three readers

columns::i32s_of replaces dsref's and mtpref's copies and the MTP tap read's
inline decode. check_taps refuses a repeated row, so taps cannot hide at a
step that does not exist. Cases cover the repeated row, a set with no rows and
no taps, no # tokens_count line and no trunk row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… rows and no taps

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ble tree index stops the oracle build, and the MTP file has one owner

columns::read_f32 refuses a NaN or infinite value by index, so MTP tap and
trunk files and the nemotron logits share one finite check; check_taps reads
each f32 tap through it. build-q35mtp-ref.sh stops when git status fails.
gate_qwen35moe_mtp names its file through the MTP step family's MODEL.
q35mtp_ref refuses to write a set whose tapped run moved the logits.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added gpu The card engine: crates/gpu*, crates/app needs-gpu Changes code a card must check, and carries no gate run on a card labels Oct 15, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: MUST_FIX: an existing destination with a trailing slash causes the dump installer to delete both the old set and its replacement.

Must fix

1. Normalize the destination before deriving the staging and lock paths

tools/ref/dump-q35mtp.sh:62 Kind A. With an existing Q35MTP_SET=/data/trial/, STAGE becomes /data/trial/.staging and the lock is /data/trial/.lock. After a successful dump, rm -rf "$SET" at line 81 deletes the old set, the finished stage, and the lock file. The mv at line 82 fails because its source is gone; the EXIT trap cannot recover it.
Fix this assignment before reruns can erase installed reference data. Normalize the destination before deriving either path:

SET=$(realpath -m -- "${Q35MTP_SET:-$BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64}")

Checked

Duplicates: all three dryer pairs read; they represent distinct graph/element tags or step/case lookups.
Complexity: TapSet::read (new, 41) separates header/data parsing; check_taps (new, 17) checks coverage and files. Family::check_set (28 → 30) adds one dispatch arm.
Silent failure: the test's .ok() is asserted; unrecognized comment headers are skipped; first_argmax refuses non-finite logits by name.
Tests: static inspection confirms that the new NaN/infinity cases protect columns.rs:170. The inline replacement passes bash -n in the full script. Path derivation checked for trailing slashes and spaces; no model or GPU run performed during validation.
Gates: the existing GPU gate takes its model path from the shared constant; its bands and pins do not change. The PR reports FAIL-first results and GPU gate output.
Escape catalog: C1–C4 checked; none matches the finding.

Card check: gpu-evidence. This pull request changes crates/gpu-gates/src/bin/gate_qwen35moe_mtp.rs and carries a gate run on the author's card, which counts as verified.

🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)

Comment thread tools/ref/dump-q35mtp.sh
esac
done
if [ ${#taps[@]} = 0 ]; then for t in "${MTP_TAPS[@]}"; do taps+=(--tap "$t"); done; fi
SET=${Q35MTP_SET:-$BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With an existing destination ending in '/', the stage and lock are inside SET, so installation deletes the completed dump before mv.

Suggested change
SET=${Q35MTP_SET:-$BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64}
SET=$(realpath -m -- "${Q35MTP_SET:-$BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64}")

@github-actions github-actions Bot added gpu-evidence Verified on the author's card: the pull request carries the gate run and its output and removed needs-gpu Changes code a card must check, and carries no gate run on a card labels Oct 15, 2026
@midagedev

Copy link
Copy Markdown
Owner Author

Closing for now: this reference set is step M2b-1 of #30 and has no use until the draft it checks exists. The branch q35m2b stays; #30's checklist names it, and it is ported onto main in the same round as Body35's MtpBody, with the open review item (trailing-slash destination deletes both sets) fixed first. Bot-written (Claude Code, on the maintainer's instruction).

@midagedev midagedev closed this Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gpu The card engine: crates/gpu*, crates/app gpu-evidence Verified on the author's card: the pull request carries the gate run and its output host Host crates: qdot, threads, engram, engram-lab, runtime, sampler, levers, decision, vision, refset tools Tools, recipes, CI and the toolchain: tools/, justfile, .github/, Cargo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant