Repository navigation
Conversation
github-actions
Bot
added
host
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: MUST_FIX: invalid data passes validation, and reference I/O has duplicate owners.
Must fix
1. Validate token values before approving their file
crates/refset/src/nemotron.rs:215 counts nonempty lines. Five garbage lines pass as token IDs; refset-check prints PASS. The happy fixture also has IDs 4 and 5 outside its vocabulary of 4.
Fix this before consumers rely on the set. Replace the count line:
if p.n_ids == 0 || p.n_vocab == 0 {
return Err(RefError::malformed(ids.display(), "empty prompt or vocabulary"));
}
for (i, line) in text.lines().enumerate() {
let id: u32 = crate::columns::parse_field(line, "token id", &At(&ids, i + 1))?;
if id >= p.n_vocab {
return Err(RefError::malformed(ids.display(), format!("token id {id} outside vocabulary {}", p.n_vocab)));
}
}
let n = text.lines().count();Use 0\n1\n2\n3\n0\n in the happy fixture and assert refusal of a malformed ID.
2. Check the completion count
crates/refset/src/nemotron.rs:110 accepts every # complete line. Delete a prompt row while retaining # complete\t4\t0: the smaller set still passes. Malformed counts also pass.
Prevent missing gate cases. Declare let mut complete_rows = None; before the loop, replace the arm, and check after the loop:
"# complete" => {
if f.len() != 3 || f[2] != "0" {
return Err(RefError::malformed(&at, "want # complete\t<count>\t0"));
}
complete_rows = Some(crate::columns::parse_field::<usize>(f[1], "complete count", &at)?);
set.complete = true;
}
// After the loop, before Ok(set):
if complete_rows != Some(set.prompts.len()) {
return Err(RefError::malformed(path.display(), "completion count differs from prompt rows"));
}Add a mutation case that removes a prompt row without changing the trailer.
3. Refuse invalid numeric options
tools/ref/nemotron_ref.cpp:147 uses atoi. With otherwise valid arguments, -c garbage selects the prompt length and -t garbage selects default threads. The dump proceeds with bad input.
Validate before callers rely on these defaults. Replace the three numeric branches with:
} else if (more && (strcmp(argv[i], "-c") == 0 || strcmp(argv[i], "-ub") == 0 || strcmp(argv[i], "-t") == 0)) {
const char * flag = argv[i];
const char * value = argv[++i];
char * end = nullptr;
errno = 0;
const long n = strtol(value, &end, 10);
if (end == value || *end != '\0' || errno != 0 || n < 0 || n > INT_MAX) {
fprintf(stderr, "nemotron_ref: %s wants an integer, got %s\n", flag, value);
return 2;
}
if (strcmp(flag, "-c") == 0) n_ctx = (int) n;
else if (strcmp(flag, "-ub") == 0) n_ubatch = (int) n;
else threads = (int) n;Keep the following } else { branch.
4. Give reference file I/O one owner (R14)
tools/ref/nemotron_ref.cpp:35 repeats the token parser and raw writer at tools/ref/hidden_ref.cpp:70. Both own token bounds and short I/O.
Share before either copy receives another fix. Put this in tools/ref/ref_io.h, include it in both harnesses, and delete both local parsers and writers:
#pragma once
#include "llama.h"
#include <cerrno>
#include <climits>
#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);
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 id = strtol(line.c_str(), &end, 10);
if (end == line.c_str() || *end != '\0' || errno != 0 || id < 0 || id > INT32_MAX) {
fprintf(stderr, "%s: invalid token ID\n", path.c_str());
return false;
}
ids.push_back((llama_token) id);
}
if (!in.is_open() || in.bad() || (long long) ids.size() != count) {
fprintf(stderr, "%s: cannot read %lld token IDs\n", path.c_str(), count);
return false;
}
return true;
}
inline bool write_raw(const std::string & path, const float * data, size_t count) {
FILE * f = fopen(path.c_str(), "wb");
if (!f) { perror(path.c_str()); return false; }
const bool ok = fwrite(data, sizeof(float), count, f) == count;
if (fclose(f) != 0 || !ok) {
fprintf(stderr, "%s: short write\n", path.c_str());
return false;
}
return true;
}Replace Nemotron's call with if (!read_token_file(p.path, p.n, p.ids)) return 2;. Include the header in both build/run source hashes. Four functions become two; lines and names decrease.
Checked
Duplicates: C++ I/O copies confirmed; no dryer pairs.
Complexity: diagnostics report LogitsSet::read 20 and Family::check_set 24→26 (one arm).
Silent failure: invalid IDs, counts and numeric options are accepted.
Tests: model/build, missing trailers and logit values are covered; ID and trailer-count validation is absent. Static inspection only.
Gates: pin reused; no thresholds moved. Escape catalog: no matching finding.
Notes
The PR supplies no FAIL-first evidence for the new tests.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
Filed outside this PR: #240
There was a problem hiding this comment.
Since the last review:
- Token validation: still open for empty lines (crates/refset/src/nemotron.rs:238); malformed numbers and out-of-vocabulary IDs are refused.
- Completion count: fixed.
- Numeric options: fixed.
- Shared reference I/O: fixed.
Verdict: MUST_FIX: malformed token records still pass validation or produce a successful reference dump.
Must fix
1. Refuse empty token lines
crates/refset/src/nemotron.rs:238 skips an empty record. With the existing fixture, 0\n\n1\n2\n3\n0\n still passes the five-ID check, so refset-check reports PASS. The shared C++ reader refuses the same file.
Close the validation gap before this set becomes a gate input. Replace continue; with:
return Err(RefError::malformed(At(&ids, i + 1), "empty token id"));Add the empty-line fixture as a refusal case.
2. Check consumption of the entire C++ token record
tools/ref/ref_io.h:57 checks only *end == '\0'. A file record containing bytes 1\x00garbage\n survives getline; strtol consumes only 1, and this check accepts it. Both harnesses decode token 1. Nemotron writes a clean 1\n to its output and completes, hiding the malformed source.
Fix the shared owner before more callers inherit this acceptance. Replace the two-line condition with:
if (line.empty() || line[0] < '0' || line[0] > '9' || end != line.c_str() + line.size() ||
errno != 0 || id < 0 || id > INT32_MAX) {Add a binary token-file fixture containing the embedded NUL and assert refusal.
Checked
Duplicates: 12 new functions in the incremental diff. Both mainline harnesses use the shared I/O owner; existing IK readers are explicitly deferred. Dryer reported no pairs.
Complexity: crapper flags LogitsSet::read (24), with added trailer-validation branches, and Family::check_set (26), whose family arm is unchanged this round. Complexity alone yields no finding.
Silent failure: traced empty records to refset-check's PASS branch and embedded NULs through the shared reader to the dump harnesses.
Tests, static mutation review: parsing invalid IDs as zero or changing >= to > makes the Rust refusal assertions fail. Dropping the end check in ref_io.h:26 makes the C++ trailing-junk assertion fail. Empty/NUL records are absent from the tests.
Gates: no thresholds or pins moved. Escape catalog C1–C4: no matching defect. Changed paths are reference readers and tools, outside engine hot paths.
Notes
Validation was static; no suites were run. The PR supplies Rust FAIL-first results and a green run for the new C++ test.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
| let mut n = 0usize; | ||
| for (i, line) in text.lines().enumerate() { | ||
| if line.is_empty() { | ||
| continue; |
There was a problem hiding this comment.
An empty record is skipped, so a five-ID file with an extra blank line passes validation. Return the named error.
| continue; | |
| return Err(RefError::malformed(At(&ids, i + 1), "empty token id")); |
| if (line.empty() || line[0] < '0' || line[0] > '9' || *end != '\0' || errno != 0 || id < 0 || | ||
| id > INT32_MAX) { |
There was a problem hiding this comment.
An embedded NUL ends strtol before the record ends, so malformed input is accepted as its numeric prefix.
| if (line.empty() || line[0] < '0' || line[0] > '9' || *end != '\0' || errno != 0 || id < 0 || | |
| id > INT32_MAX) { | |
| if (line.empty() || line[0] < '0' || line[0] > '9' || end != line.c_str() + line.size() || | |
| errno != 0 || id < 0 || id > INT32_MAX) { |
The ref-nemotron-logits family reuses the tokenizer build pin. A set is read by column name and refused when its trailer, model path, or build pin does not match.
ik has no nemotron_h. nemotron_ref links the pinned mainline build and writes four CPU prompts into ref-nemotron-logits. CUDA stays hidden.
The logits family runs the tokenizer family's model function. The dump compares the live mainline md5s with the build record instead of repeating the pin. The profile no longer restates the prompt list the dump script owns, and a prompt check reads its logits through logits.
…e name has one check The build script records the md5s it linked and does not repeat the tokenizer family's pin. A manifest name that leaves the set directory is columns::file_name, which the dequant reader already applied.
…t does not parse is refused A token line that is not a whole id, or an id outside the vocabulary, is malformed. The # complete count is the prompt rows. hidden_ref and nemotron_ref share one parser: a numeric option is the whole argument, an integer in range, or the harness names the flag and the text and exits 2. The build record hashes the .cpp and then ref_io.h. just test-ref-io compiles and runs that parser's checks. No card and no llama.cpp.
…aders The C++ reader takes a token record only when strtol consumed every byte of it, so a NUL inside the line no longer ends the record early. The nemotron set reader refuses an empty id line, as the C++ reader does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Verdict: MUST_FIX: the reader accepts non-finite reference logits, and the change adds duplicate validation and cleanup logic.
Must fix
1. Reject non-finite logits before accepting the set
crates/refset/src/nemotron.rs:218 A file containing NaN or infinity with the correct byte length returns Ok. check_prompt discards that vector, so Family::check_set succeeds and refset-check prints PASS. The writer refuses these values.
Rejecting them here prevents invalid reference data from reaching later comparisons. Insert before Ok(out):
if let Some(i) = out.iter().position(|v| !v.is_finite()) {
return Err(RefError::malformed(
path.display(),
format!("non-finite logit at index {i}"),
));
}Add a fixture with a non-finite lane and assert a named Malformed error.
2. Keep staging cleanup in one owner
tools/ref/dump-nemotron.sh:78 is an exact copy of tools/ref/dump.sh:257 and tools/ref/dump-mtp.sh:181 (R14).
Every cleanup change now needs three edits. Move this definition to the existing tools/ref/ref-paths.sh, which all three scripts source; delete their local definitions and keep their traps. This removes two definitions without adding a file or parameter.
drop_incomplete_stage() {
[ -d "$STAGE" ] || return 0
grep -qs '^# complete' "$STAGE/MANIFEST.tsv" && return 0
rm -rf "$STAGE"
echo "[staging] removed the incomplete $STAGE" >&2
}3. Use the shared integer parser for prompt counts
tools/ref/nemotron_ref.cpp:44 repeats the range parser in tools/ref/ref_io.h:16 (R14). They already disagree: a prompt count +5 is accepted here and rejected by the shared parser.
Reuse prevents further divergence and replaces four lines with three. Replace lines 44–47:
const std::string count(c1 + 1, c2);
long long n = 0;
if (!parse_i64("nemotron_ref", "--prompt count", count.c_str(), 1, 1000000, n)) return false;Non-blocking
⚠ Exercise the wrapper used by both harnesses: tools/ref/ref_io_test.cpp:19 tests parse_i64 directly. Mutating parse_i32 to dest = atoi(value); return true; leaves every assertion green and restores acceptance of invalid flags. Add before the file tests:
int n32 = 123;
expect(!parse_i32("ref", "-c", "garbage", 0, n32) && n32 == 123, "i32 rejects garbage");⚠ Test column lookup with reordered columns: crates/refset/src/arch/nemotron_h/mod.rs:61 always uses the reader's field order. Replacing name lookup with positional parsing passes the success test. Replace the fixture's format expression:
let mut text = format!(
"# model\t{model}\n# build\t{build}\n# arch\tnemotron_h\n\
# prompt columns\tname\tn_vocab\tn_ids\tlogits\tids\n\
prompt\tp\t4\t5\tp.logits.f32\tp.ids\n"
);Checked
Duplicates: both reported copies confirmed; cleanup consolidation removes two definitions, and prompt counts reuse an existing parser. Dryer reported zero pairs.
Complexity: diagnostics report LogitsSet::read new at 24 and Family::check_set 24→26. The latter adds one dispatch arm.
Silent failure: non-finite logits reach check_prompt, Family::check_set, and the PASS branch in refset-check.
Tests: static inspection confirms no parse_i32 call in the C++ suite and fixed column order in every Nemotron fixture. The two described mutants evade those tests.
Gates: no existing pin, tolerance or test was removed or relaxed.
Escape catalog: no C1–C4 match in the changed shell code. No engine hot path changed.
Notes
Runtime tests and dump commands were not run in this review.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
Filed outside this PR: #258
| expect(accepted("-t", "16", 0, INT_MAX, 16), "threads"); | ||
| expect(accepted("-c", "2147483647", 0, INT_MAX, INT_MAX), "int max"); | ||
| expect(rejected("-c", "2147483648", 0, INT_MAX), "past int max"); | ||
|
|
There was a problem hiding this comment.
Exercise parse_i32 so the suite fails if the wrapper accepts invalid flags.
| int n32 = 123; | |
| expect(!parse_i32("ref", "-c", "garbage", 0, n32) && n32 == 123, "i32 rejects garbage"); |
…g cleanup and prompt count parse have one owner The nemotron reader refuses a NaN or infinite logit by index. ref-paths.sh owns drop_incomplete_stage for dump.sh, dump-mtp.sh and dump-nemotron.sh. nemotron_ref reads a prompt's count with parse_i64, so +5 is refused. The tests read the prompt columns in another order and call parse_i32. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Since the last review:
- Non-finite logits: fixed at crates/refset/src/nemotron.rs:218.
- Duplicate staging cleanup: fixed at tools/ref/ref-paths.sh:268; all three callers reuse it.
- Duplicate prompt-count parser: fixed at tools/ref/nemotron_ref.cpp:44.
- Integer-wrapper coverage: fixed at tools/ref/ref_io_test.cpp:30.
- Reordered-column coverage: fixed at crates/refset/src/arch/nemotron_h/mod.rs:61.
Verdict: CLEAN: All previous findings are fixed, and no new must-fix finding remains.
Non-blocking
⚠ Record the new test's FAIL-first run: crates/refset/src/arch/nemotron_h/mod.rs:170.
The body reports red/green results for four earlier refusal tests, but none for this new test. Add the commands and output showing failure before the finite-value guard and success after it. This makes the regression evidence reviewable.
Checked
Duplicates: 1 new test and 1 relocated function checked; no duplicate remains. Dryer reported zero pairs.
Complexity: diagnostics flag LogitsSet::read at 24 (parser) and Family::check_set 24→26 (dispatch arm). Neither function changes in this increment.
Silent failure: 2 changed refusal paths followed. Non-finite logits propagate Malformed through check_set to FAIL; invalid prompt counts produce a named refusal.
Tests: static mutation checks show that deleting the finite-value guard, replacing parse_i32 with atoi, dropping its output assignment, or restoring positional column reads makes an assertion fail. All four changed shell files pass bash -n.
Gates: no pin or tolerance moved; no test removed or relaxed.
Escape catalog: no C1–C4 match. NaN/infinity, prompt-count boundaries and cleanup availability were checked. No engine hot path changes.
Notes
Rust and C++ runtime tests were not run in this review.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
1 participant
What
Nemotron 3 Nano (
nemotron_h) last-position logits, read from one reference set. The family row, the reader, and the mainline CPU dump land together on7362ad52.The public reader is
Identity::NemotronLogits,nemotron::LogitsSet::open/logits,ARCH = "nemotron_h", andLOGITS_SET = "ref-nemotron-logits".runsistokenizer::nemotron_model. The build pin is owned by the family row:llama-tokenize 5df95d3a942effc971ff170432d840fd libllama.so 3575b1f39e7773379fec011d39f32a77. A set from another model, another build, or without its trailer is refused by name.The set is four prompts,
code5,code29,prose64, andprose512(5, 29, 64, and 512 ids, BOS included). Trailer# complete 4 0. Each logits file is 524288 bytes. The dump is mainline withn_gpu_layers=0.A token id that does not parse or sits outside the vocabulary, a
# completecount that is not the prompt rows, and a numeric option that is not a whole integer in range are refused by name.hidden_refandnemotron_refshare that parser. The build record hashes the harness.cppand thenref_io.h.Closes #240
The ik dumpers
dump_ref,dump_mtpanddump_draftkeep their own token readers. Moving them ontoref_io.his separate work.Gates·Evidence
cargo test -p bloomery-refset: 202 passed, 0 failed, 3 ignored.just check-recipes: exit 0.tools/check-comments.sh: exit 0.tools/check-levers.sh: ok.just build-ref-nemotron(no card, predicted 90s): exit 0.just dump-ref-nemotron(no card, predicted 600s): exit 0. The dump ran before the later script folds; the recorded build id still matches the family pin. CUDA stayed hidden (n_gpu_layers=0).just refset-check: exit 0.ref-nemotron-logitsPASS. Summary: 338 set(s), 0 failed.just test-ref-io(no card) exited 0. stdout:ref_io_test: ok. stderr printed the recipe line and the named refusals the test requires:-c garbage,12x,12,8,-1,+8,-ub 0, an empty--tokens-count,2147483648, a token linegarbage, a short token file, and a missing directory.7362ad52against its base was CLEAN (must-fix 0). Three non-blocking warns were left unfolded. That commit is the parent of the refusal commit.nemotron_htests were red before the reader check (box, card none, exit 101: 5 passed, 4 failed) and green after (9 passed).95be4fecagainst origin/main: CLEAN, must-fix 0, warn 2. Incremental reviewer; the validator did not finish. One commit on7362ad52.The bloomery AI assistant wrote this.