feat: GGUF v3 parser, IQ wire layouts, and T1 large MoE pilots (#7) - #44
feat: GGUF v3 parser, IQ wire layouts, and T1 large MoE pilots (#7)#44rmems wants to merge 13 commits into
Conversation
…lpers Add 32 GGML type constants, ggml_type_label() function, expanded DType enum with byte length calculations for IQ3_S and IQ3_M, metadata helper methods (quantization, block_count, expert_count, etc.), public metadata value type constants, comprehensive tests (26 passing), and updated README with full documentation. Clippy clean with -D warnings. Resolves #7.
Align with corinth-canal ggml mapping. HF IQ3_M is a preset, not type 31. Wire 31 maps to DType::Other(31) with no known byte_len (fail closed).
Restore main README structure with origin/modularization section and correct Q4_0_4_4 (type 31) semantics.
Add first-class IQ dtype sizes (llama.cpp/GGUF block table), fix IQ3_S to 110 B/block, derive quantization from general.file_type when needed, expand smoke tests for quant stacked experts, and refresh local gitignore.
Optional ENGRAM_EXPECT_MOE hard-fails when no experts are discovered; ENGRAM_MOE_SAMPLES extracts more than the first pair for large MoE T1.
Avoid failing the always-on helper test when ENGRAM_EXPECT_MOE or ENGRAM_MOE_SAMPLES are set during local large-model pilots.
ZAYA1 Q8 and OLMoE F16 need single-file ENGRAM_GGUF runs with headroom above full-file load size; EXPECT_MOE hardens MoE discovery locally.
Bump crate version to 0.2.0 and rust-version/CI/Docker MSRV to 1.97.1. Add rust-toolchain.toml (stable) and inspect_gguf example; align REVIEW/README.
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:XXL
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis release updates the crate to version 0.2.0, expands GGUF dtype and metadata support, adds inspection and real-model validation tools, and aligns CI, Docker, toolchain, and documentation settings with Rust 1.97.1. ChangesGGUF 0.2.0 functionality
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant inspect_gguf
participant load_gguf
participant GgufMetadata
participant MoEExtraction
inspect_gguf->>load_gguf: load GGUF path
load_gguf->>GgufMetadata: read metadata and tensor inventory
inspect_gguf->>MoEExtraction: request first expert pair
MoEExtraction-->>inspect_gguf: return expert tensor details
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR successfully implements GGUF v3 parser enhancements with comprehensive dtype support, MoE extraction capabilities, and robust metadata helpers. The implementation demonstrates solid engineering practices with proper bounds checking, overflow protection, and extensive test coverage.
Key Strengths
- Comprehensive dtype support: Full coverage of GGML types including F32, F16, BF16, Q4_0-Q8_1, Q2_K-Q8_K, IQ1_S-IQ4_XS, and integer types (I8-I64, F64)
- Proper wire type handling: Correctly identifies wire type 31 as
Q4_0_4_4(historical) and maps it toDType::Other(31), preventing confusion with IQ3_M - Robust error handling: Consistent overflow checks, bounds validation, and descriptive error messages
- Well-tested: Comprehensive unit tests for dtype conversions, byte length calculations, and edge cases
- Zero dependencies: Pure Rust implementation achieving the stated design goal
Architecture Changes
- Renamed constants:
VT_*→GGUF_VALUE_TYPE_*(improved clarity and public API consistency) - Enhanced metadata: Added quantization fallback logic, MoE-specific helpers, and float type support
- Expanded DType enum: From 7 variants to 28+ with proper block size documentation
Testing Coverage
✅ T0 (CI): All unit tests, smoke tests, and doctests passing
✅ T1 (Local): Validated with real MoE models (ZAYA1-8B Q8_0, OLMoE-1B-7B F16)
The changes are production-ready and ready to merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
coderabbitai
Bot
added
documentation
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| GGUF_VALUE_TYPE_INT8 => self.read_i8_as_u64(), | ||
| GGUF_VALUE_TYPE_UINT16 => self.read_u16_as_u64(), | ||
| GGUF_VALUE_TYPE_INT16 => self.read_i16_as_u64(), | ||
| GGUF_VALUE_TYPE_UINT32 => self.read_u32_as_u64(), | ||
| GGUF_VALUE_TYPE_INT32 => self.read_i32_as_u64(), | ||
| GGUF_VALUE_TYPE_UINT64 => self.read_u64(), | ||
| GGUF_VALUE_TYPE_INT64 => self.read_i64_as_u64(), |
There was a problem hiding this comment.
Suggestion: Signed metadata values are converted to u64 using two's-complement casting, so a negative value such as an INT32 alignment becomes a huge positive usize. read_metadata_section then accepts that value as the alignment, and align_up can overflow or compute an invalid tensor data offset, causing a panic or incorrect tensor locations. Reject negative signed values when coercing to an unsigned layout value, especially for general.alignment. [type error]
Severity Level: Major ⚠️
- ❌ Malformed GGUF files can panic during tensor-offset finalization.
- ❌ Valid tensor extraction can use wrapped or out-of-bounds offsets.
- ⚠️ `load_gguf()` accepts invalid alignment metadata before failure.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/gguf/cursor.rs
**Line:** 136:142
**Comment:**
*Type Error: Signed metadata values are converted to `u64` using two's-complement casting, so a negative value such as an `INT32` alignment becomes a huge positive `usize`. `read_metadata_section` then accepts that value as the alignment, and `align_up` can overflow or compute an invalid tensor data offset, causing a panic or incorrect tensor locations. Reject negative signed values when coercing to an unsigned layout value, especially for `general.alignment`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| // Q2_K: 84, Q3_K: 110, Q4_K: 144, Q5_K: 176, Q6_K: 210, Q8_K: 292 | ||
| Self::Q2_K => blocked_byte_len(n_elements, 256, 84), | ||
| Self::Q3_K => blocked_byte_len(n_elements, 256, 110), | ||
| Self::Q4_K => blocked_byte_len(n_elements, 256, 144), |
There was a problem hiding this comment.
Suggestion: The blocked-size check uses only the total element count, but GGML quantization blocks are formed along the innermost dimension. A tensor shaped [128, 2] has 256 total elements and therefore passes this check for Q4_K, even though each row has only 128 elements and cannot form a valid 256-element block. layout.rs then accepts the tensor and extract_expert derives contiguous byte strides from the incorrect size. Pass the tensor dimensions into the sizing/validation path and require the innermost dimension to be block-aligned before computing the byte length. [logic error]
Severity Level: Major ⚠️
- ⚠️ Malformed Q4_K tensors pass directory validation.
- ❌ MoE extraction returns invalid expert payload boundaries.
- ⚠️ Corrupt inputs can be interpreted as valid quantized weights.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/gguf/tensor.rs
**Line:** 368:368
**Comment:**
*Logic Error: The blocked-size check uses only the total element count, but GGML quantization blocks are formed along the innermost dimension. A tensor shaped `[128, 2]` has 256 total elements and therefore passes this check for `Q4_K`, even though each row has only 128 elements and cannot form a valid 256-element block. `layout.rs` then accepts the tensor and `extract_expert` derives contiguous byte strides from the incorrect size. Pass the tensor dimensions into the sizing/validation path and require the innermost dimension to be block-aligned before computing the byte length.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 2 high |
| CodeStyle | 2 minor |
| Complexity | 5 medium |
🟢 Metrics 93 complexity · 10 duplication
Metric Results Complexity 93 Duplication 10
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c59c306b2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| pub use error::{ParserError, Result}; | ||
| pub use gguf::{DType, GgufLayout, GgufMetadata, Tensor, f16_bits_to_f32, load_gguf, parse_bytes}; | ||
| // Re-export commonly used types at the crate root for convenience. |
There was a problem hiding this comment.
Downstream code importing engram_parser::Result no longer compiles because this replaces the previous pub use error::{ParserError, Result} with a ParserError-only re-export. The same change still documents Result as part of the public API in README.md, and the changelog does not announce its removal, so this appears accidental rather than an intentional 0.2 API break.
Useful? React with 👍 / 👎.
| for entry in entries.flatten() { | ||
| if out.len() >= max_files { | ||
| break; |
There was a problem hiding this comment.
When ENGRAM_MODEL_DIR contains more GGUF files than ENGRAM_GGUF_MAX, this stops at the cap while iterating fs::read_dir, whose order is unspecified. Sorting out afterward only orders the arbitrarily selected subset, so identical T1 pilot commands can exercise different checkpoints and produce different results; collect and sort candidates before applying the cap.
Useful? React with 👍 / 👎.
| self.quantization_from_file_type | ||
| .as_deref() | ||
| .unwrap_or("unknown") |
There was a problem hiding this comment.
When callers create GgufMetadata::default() and populate its public numerics map, or mutate general.file_type after parsing, quantization() ignores the current value because it reads this private cache, which is initialized only by parse_layout. This makes the documented general.file_type fallback return "unknown" or a stale label for otherwise valid public metadata state; derive or refresh the fallback from the current map instead.
Useful? React with 👍 / 👎.
| fn blocked_byte_len(n_elements: usize, block_size: usize, bytes_per_block: usize) -> Option<usize> { | ||
| if !n_elements.is_multiple_of(block_size) { | ||
| return None; | ||
| } | ||
| (n_elements / block_size).checked_mul(block_bytes) | ||
| let n_blocks = n_elements / block_size; | ||
| n_blocks.checked_mul(bytes_per_block) |
There was a problem hiding this comment.
For the newly supported blocked dtypes, divisibility of the total element count is insufficient because GGML blocks cannot span rows: the innermost dimension must itself be divisible by the dtype's block size. For example, a Q4_0 tensor with dims [16, 1, 2] passes this check because its total is 32, is assigned an 18-byte payload, and can then be split into two invalid 9-byte expert slices; calculate the row layout from dims[0] or reject such shapes.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Around line 29-40: Update the standard development artifact patterns in
.gitignore to explicitly ignore the lcov.info coverage report generated by the
documented gate, ensuring it cannot leave the working tree dirty or be staged.
In `@examples/inspect_gguf.rs`:
- Around line 23-138: Split the self-contained reporting logic out of main into
print_dtype_histogram, print_moe_report, and print_tensor_sample, preserving all
existing output and behavior. Add print_raw_tensor for shared MoE tensor
formatting, update main to call these helpers, and import GgufLayout and
RawTensor as required by their signatures.
In `@REVIEW.md`:
- Around line 109-110: Update REVIEW.md to resolve the markdownlint warnings:
replace the setext heading around Lines 109-110 with a plain paragraph or ATX
heading, and insert blank lines after the tables ending around Lines 212 and 348
before the following separators.
In `@rust-toolchain.toml`:
- Around line 4-6: Set the toolchain to Rust 1.97.1 in rust-toolchain.toml
(lines 4-6), and ensure Dockerfile (lines 18-20) and .github/workflows/ci.yml
(lines 94-99) explicitly use the same version via RUSTUP_TOOLCHAIN=1.97.1 or
cargo +1.97.1 for their Cargo commands.
In `@src/gguf/cursor.rs`:
- Around line 27-39: Add a concise Rust doc comment to each public GGUF
value-type constant from GGUF_VALUE_TYPE_UINT8 through GGUF_VALUE_TYPE_FLOAT64,
describing the corresponding GGUF value type. Keep the existing constant names
and numeric values unchanged.
In `@src/gguf/tensor.rs`:
- Around line 12-15: Update the module documentation near ggml_type_label to
avoid claiming coverage of every dtype in GGUF v3; state instead that the
constants represent the dtypes this crate models. Do not alter the existing GGML
type table or label behavior unless adding the remaining label-only constants is
specifically intended.
- Around line 154-155: Update the documentation in the tensor type comment to
link specifically to DType::Other and format its u32 payload as raw u32 text,
resolving the intra-doc link without changing the documented behavior.
In `@src/lib.rs`:
- Around line 45-46: Align the public Result API by updating src/lib.rs:45-46 to
re-export Result alongside ParserError from error, preserving README.md:119-122
as documented; no direct README.md change is needed.
In `@tests/gguf_smoke.rs`:
- Around line 321-350: Update the GGUF smoke test’s metadata fixtures to include
a real qwen2moe.rope.freq_base KvValue::F32 entry, then assert
layout.metadata.float32 returns 10000.0. Replace each duplicated manual GGUF
byte-building block near the affected tests with build_gguf(&kv, &[]), including
the copies around the later test cases, and remove the now-unneeded
#[allow(dead_code)] on KvValue::F32.
In `@tests/real_gguf.rs`:
- Around line 86-124: Reduce CodeScene complexity by extracting the per-role
assertions and per-path body from real_gguf_moe_extract_when_present, preserving
the existing assertions and pilot-loop behavior. In collect_gguf, extract entry
scanning and file/directory partitioning into a helper such as
partition_entries, then retain max_files/depth checks and recursion in
collect_gguf while preserving ordering and behavior.
- Around line 75-81: Change the ENV_MAX fallback in the test’s
environment-variable parsing from 8 to 1, while preserving the existing override
behavior when ENGRAM_GGUF_MAX is explicitly set. Keep collect_gguf and its other
arguments unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c5570a2-0fab-4cbe-b953-99b8be9b6084
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/ci.yml.gitignoreCHANGELOG.mdCargo.tomlDockerfileREADME.mdREVIEW.mdexamples/inspect_gguf.rsrust-toolchain.tomlsrc/gguf/cursor.rssrc/gguf/layout.rssrc/gguf/mod.rssrc/gguf/tensor.rssrc/lib.rstests/gguf_smoke.rstests/real_gguf.rs
| fn main() -> ExitCode { | ||
| let path = match resolve_path() { | ||
| Ok(p) => p, | ||
| Err(msg) => { | ||
| eprintln!("{msg}"); | ||
| eprintln!("usage: cargo run --example inspect_gguf -- <model.gguf>"); | ||
| eprintln!(" or: ENGRAM_GGUF=<model.gguf> cargo run --example inspect_gguf"); | ||
| return ExitCode::from(2); | ||
| } | ||
| }; | ||
|
|
||
| if !path.is_file() { | ||
| eprintln!("not a file: {}", path.display()); | ||
| return ExitCode::from(1); | ||
| } | ||
|
|
||
| let t0 = Instant::now(); | ||
| let layout = match load_gguf(&path) { | ||
| Ok(l) => l, | ||
| Err(e) => { | ||
| eprintln!("load_gguf failed: {e}"); | ||
| return ExitCode::from(1); | ||
| } | ||
| }; | ||
| let parse_ms = t0.elapsed().as_secs_f64() * 1000.0; | ||
|
|
||
| println!("path: {}", path.display()); | ||
| println!("parse_ms: {parse_ms:.2}"); | ||
| println!("architecture: {}", layout.metadata.architecture()); | ||
| println!("quantization: {}", layout.metadata.quantization()); | ||
| println!("alignment: {}", layout.alignment); | ||
| println!("tensor_count: {}", layout.tensors.len()); | ||
| println!("block_count: {:?}", layout.metadata.block_count()); | ||
| println!("expert_count: {:?}", layout.metadata.expert_count()); | ||
| println!("expert_used: {:?}", layout.metadata.expert_used_count()); | ||
| println!("embed_len: {:?}", layout.metadata.embedding_length()); | ||
|
|
||
| // Dtype histogram (first 16 labels by frequency). | ||
| let mut counts: Vec<(String, usize)> = { | ||
| use std::collections::HashMap; | ||
| let mut m: HashMap<String, usize> = HashMap::new(); | ||
| for t in layout.tensors.values() { | ||
| *m.entry(ggml_type_label(t.ggml_type).to_owned()) | ||
| .or_default() += 1; | ||
| } | ||
| let mut v: Vec<_> = m.into_iter().collect(); | ||
| v.sort_by(|a, b| b.1.cmp(&a.1).then_with(|| a.0.cmp(&b.0))); | ||
| v | ||
| }; | ||
| if counts.len() > 16 { | ||
| counts.truncate(16); | ||
| } | ||
| println!("dtype_hist: {counts:?}"); | ||
|
|
||
| let experts = list_experts(&layout); | ||
| println!("moe_pairs: {} (block,expert)", experts.len()); | ||
| if !experts.is_empty() { | ||
| let show = experts.len().min(8); | ||
| println!("moe_pairs_hd: {:?}", &experts[..show]); | ||
|
|
||
| let (b, e) = experts[0]; | ||
| let t1 = Instant::now(); | ||
| match extract_expert(&layout, b, e) { | ||
| Ok(w) => { | ||
| let extract_ms = t1.elapsed().as_secs_f64() * 1000.0; | ||
| println!( | ||
| "extract ({b},{e}): complete={} extract_ms={extract_ms:.2}", | ||
| w.is_complete() | ||
| ); | ||
| if let Some(g) = w.gate.as_ref() { | ||
| println!( | ||
| " gate: dims={:?} bytes={} dtype={:?} stacked={}", | ||
| g.dims, | ||
| g.bytes.len(), | ||
| g.dtype, | ||
| g.stacked_slice | ||
| ); | ||
| } | ||
| if let Some(u) = w.up.as_ref() { | ||
| println!( | ||
| " up: dims={:?} bytes={} dtype={:?}", | ||
| u.dims, | ||
| u.bytes.len(), | ||
| u.dtype | ||
| ); | ||
| } | ||
| if let Some(d) = w.down.as_ref() { | ||
| println!( | ||
| " down: dims={:?} bytes={} dtype={:?}", | ||
| d.dims, | ||
| d.bytes.len(), | ||
| d.dtype | ||
| ); | ||
| } | ||
| } | ||
| Err(err) => println!("extract ({b},{e}) failed: {err}"), | ||
| } | ||
| } | ||
|
|
||
| // Sample a few tensor names (sorted) for inventory smoke. | ||
| let mut names: Vec<_> = layout.tensors.keys().cloned().collect(); | ||
| names.sort(); | ||
| let n = names.len().min(12); | ||
| println!("tensor_names_hd ({n}/{}):", names.len()); | ||
| for name in &names[..n] { | ||
| let t = &layout.tensors[name]; | ||
| println!( | ||
| " {name}: dims={:?} type={} byte_len={}", | ||
| t.dims, | ||
| ggml_type_label(t.ggml_type), | ||
| t.byte_len | ||
| ); | ||
| } | ||
|
|
||
| ExitCode::SUCCESS | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Split main to clear the CodeScene Complex Method violation.
The CodeScene gate fails on this file with a health score of 9.41 against a required 10.00, and it names main as the Complex Method offender. main currently performs path resolution, file validation, loading, metadata printing, dtype histogram building, MoE extraction reporting, and tensor-name sampling.
Three blocks are self-contained and extract without any behavior change: the histogram at Lines 61-75, the MoE report at Lines 77-120, and the name sample at Lines 122-135.
♻️ Proposed extraction
- // Dtype histogram (first 16 labels by frequency).
- let mut counts: Vec<(String, usize)> = {
- use std::collections::HashMap;
- let mut m: HashMap<String, usize> = HashMap::new();
- for t in layout.tensors.values() {
- *m.entry(ggml_type_label(t.ggml_type).to_owned())
- .or_default() += 1;
- }
- let mut v: Vec<_> = m.into_iter().collect();
- v.sort_by(|a, b| b.1.cmp(&a.1).then_with(|| a.0.cmp(&b.0)));
- v
- };
- if counts.len() > 16 {
- counts.truncate(16);
- }
- println!("dtype_hist: {counts:?}");
-
- let experts = list_experts(&layout);
- println!("moe_pairs: {} (block,expert)", experts.len());
- if !experts.is_empty() {
+ print_dtype_histogram(&layout);
+ print_moe_report(&layout);
+ print_tensor_sample(&layout);
+
+ ExitCode::SUCCESS
+}Add the helpers below main:
fn print_dtype_histogram(layout: &GgufLayout) {
use std::collections::HashMap;
let mut m: HashMap<&'static str, usize> = HashMap::new();
for t in layout.tensors.values() {
*m.entry(ggml_type_label(t.ggml_type)).or_default() += 1;
}
let mut counts: Vec<_> = m.into_iter().collect();
counts.sort_by(|a, b| b.1.cmp(&a.1).then_with(|| a.0.cmp(b.0)));
counts.truncate(16);
println!("dtype_hist: {counts:?}");
}
fn print_raw_tensor(role: &str, t: &RawTensor) {
println!(
" {role}: dims={:?} bytes={} dtype={:?} stacked={}",
t.dims,
t.bytes.len(),
t.dtype,
t.stacked_slice
);
}
fn print_moe_report(layout: &GgufLayout) {
let experts = list_experts(layout);
println!("moe_pairs: {} (block,expert)", experts.len());
let Some(&(b, e)) = experts.first() else {
return;
};
println!("moe_pairs_hd: {:?}", &experts[..experts.len().min(8)]);
let t1 = Instant::now();
match extract_expert(layout, b, e) {
Ok(w) => {
let extract_ms = t1.elapsed().as_secs_f64() * 1000.0;
println!(
"extract ({b},{e}): complete={} extract_ms={extract_ms:.2}",
w.is_complete()
);
for (role, opt) in [
("gate", w.gate.as_ref()),
("up ", w.up.as_ref()),
("down", w.down.as_ref()),
] {
if let Some(t) = opt {
print_raw_tensor(role, t);
}
}
}
Err(err) => println!("extract ({b},{e}) failed: {err}"),
}
}
fn print_tensor_sample(layout: &GgufLayout) {
let mut names: Vec<_> = layout.tensors.keys().cloned().collect();
names.sort();
let n = names.len().min(12);
println!("tensor_names_hd ({n}/{}):", names.len());
for name in &names[..n] {
let t = &layout.tensors[name];
println!(
" {name}: dims={:?} type={} byte_len={}",
t.dims,
ggml_type_label(t.ggml_type),
t.byte_len
);
}
}This needs GgufLayout and RawTensor added to the import at Line 21.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/inspect_gguf.rs` around lines 23 - 138, Split the self-contained
reporting logic out of main into print_dtype_histogram, print_moe_report, and
print_tensor_sample, preserving all existing output and behavior. Add
print_raw_tensor for shared MoE tensor formatting, update main to call these
helpers, and import GgufLayout and RawTensor as required by their signatures.
Source: Pipeline failures
| There is no `rustfmt.toml` in this repo; defaults are fine. | ||
| --- |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Resolve the reported markdownlint warnings.
markdownlint-cli2 reports a setext heading at Lines 109-110 and missing blank lines after the tables ending at Lines 212 and 348. Convert the note to a plain paragraph or ATX heading, and add blank lines before the following separators.
Also applies to: 212-213, 348-349
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 109-109: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@REVIEW.md` around lines 109 - 110, Update REVIEW.md to resolve the
markdownlint warnings: replace the setext heading around Lines 109-110 with a
plain paragraph or ATX heading, and insert blank lines after the tables ending
around Lines 212 and 348 before the following separators.
Source: Linters/SAST tools
| [toolchain] | ||
| channel = "stable" | ||
| components = ["rustfmt", "clippy", "llvm-tools-preview"] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
docker build --no-cache \
--build-arg RUST_VERSION=1.97.1 \
-t engram-parser-msrv .
docker run --rm --workdir /app engram-parser-msrv \
sh -lc 'rustup show active-toolchain; rustc --version'Repository: Limen-Neural/engram-parser
Length of output: 208
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- tracked files ---'
git ls-files -- rust-toolchain.toml Dockerfile .github/workflows/ci.yml Cargo.toml
printf '%s\n' '--- rust-toolchain.toml ---'
cat -n rust-toolchain.toml
printf '%s\n' '--- Dockerfile relevant lines ---'
cat -n Dockerfile | sed -n '1,45p'
printf '%s\n' '--- CI relevant lines ---'
cat -n .github/workflows/ci.yml | sed -n '75,125p'
printf '%s\n' '--- Rust version declarations and toolchain uses ---'
rg -n --hidden -g '!target' -g '!node_modules' \
'rust-version|RUST_VERSION|rust-toolchain|rustup (default|show)|dtolnay/rust-toolchain|cargo \+[0-9]|rustc --version|cargo (check|test|clippy|fmt|build)' \
Cargo.toml rust-toolchain.toml Dockerfile .github/workflowsRepository: Limen-Neural/engram-parser
Length of output: 156
🌐 Web query:
rustup override precedence RUSTUP_TOOLCHAIN rust-toolchain.toml official documentation
💡 Result:
Rustup determines the active toolchain by evaluating several override mechanisms in a strict order of precedence [1][2][3]. When multiple overrides are present, Rustup selects the first toolchain defined by the following list [1][4]: 1. Toolchain override shorthand (e.g., cargo +beta) [1][2][3] 2. RUSTUP_TOOLCHAIN environment variable [1][5][2] 3. Directory override (set via rustup override command) [1][2][3] 4. rust-toolchain.toml file (or the legacy rust-toolchain file) [1][2][6] 5. Default toolchain [1][2][3] Proximity-based behavior: There is an important nuance regarding directory overrides and rust-toolchain.toml files. Both of these methods are discovered by searching upwards from the current working directory to the filesystem root [1][2]. When both are encountered during this search, the one found closer to the current directory takes precedence [1][2][3]. For example, a rust-toolchain.toml file in a child directory will take precedence over a directory override configured in a parent directory [1][2]. Notes: - The toolchain specified by a higher-precedence method will be used, and lower-precedence methods are ignored for the purpose of selecting the toolchain [1][7]. - While the official documentation lists the order above, some users have noted that certain rustup commands (such as rustup show) may exhibit behavior that differs from the primary toolchain resolution process used by proxied tools like cargo and rustc [8]. - If both rust-toolchain.toml and the legacy rust-toolchain file exist in the same directory, rust-toolchain is ignored in favor of rust-toolchain.toml (though the legacy file is still supported for backward compatibility) [1][2].
Citations:
- 1: https://rust-lang.github.io/rustup/overrides.html
- 2: https://github.com/rust-lang/rustup/blob/4b2c0919/doc/user-guide/src/overrides.md
- 3: https://github.com/rust-lang/rustup/blob/060192bd8089b4866f1571d85e85ca0264323233/README.md
- 4: https://deepwiki.com/rust-lang/rustup/3.3-toolchain-naming-and-resolution
- 5: https://rust-lang.github.io/rustup/environment-variables.html
- 6: https://rust-lang.github.io/rustup/overrides.html?highlight=toolchain
- 7: Document (and perhaps change) interaction between
rust-toolchain.tomland other override sources for components and targets rust-lang/rustup#3483 - 8: Is "Overrides" section of "The rustup book" correct? rust-lang/rustup#4150
🌐 Web query:
official rust Docker image rustup default toolchain rust:${VERSION}-slim rust-toolchain.toml directory override
💡 Result:
The official Rust Docker images (e.g., rust:1.x.x-slim) include rustup by default, which manages toolchains within the container environment [1][2][3]. Because rustup is active, it automatically respects rust-toolchain.toml files found in your working directory [4][5]. If your project contains a rust-toolchain.toml file, cargo and rustc will prioritize the toolchain specified in that file over the Docker image's default toolchain [4][5]. This can lead to unexpected behavior, such as rustup attempting to download additional toolchains at runtime if the specified channel is not already installed in the image [6][7]. To prevent this automatic override and ensure your build uses exactly the toolchain pre-installed in the Docker image, you can explicitly set the RUSTUP_TOOLCHAIN environment variable when running your commands [4][7]. By setting this variable to the toolchain version provided by the image (e.g., export RUSTUP_TOOLCHAIN=$(rustc --version | cut -d' ' -f2)), rustup will bypass the search for rust-toolchain.toml and use the specified toolchain [4][7]. Alternatively, you can temporarily rename or remove the rust-toolchain.toml file during the build process if strict isolation is required [4][7].
Citations:
- 1: https://hub.docker.com/_/rust
- 2: https://github.com/rust-lang/docker-rust/
- 3: https://github.com/rust-lang/docker-rust/blob/dd106de2954f52f336c3d2c1326ae778c51830f3/stable/bullseye/Dockerfile
- 4: https://rust-lang.github.io/rustup/overrides.html
- 5: https://rust-lang.github.io/rustup/overrides.html?highlight=rust-toolchain.toml
- 6: Is there no way to get a regular cargo that isn't rustup's cargo?? rust-lang/docker-rust#212
- 7: Fix docker build toolchain override from rust-toolchain.toml (#50) solana-foundation/solana-verifiable-build#275
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- rust-toolchain.toml ---'
cat -n rust-toolchain.toml
printf '%s\n' '--- Dockerfile ---'
cat -n Dockerfile | sed -n '1,45p'
printf '%s\n' '--- CI ---'
cat -n .github/workflows/ci.yml | sed -n '75,125p'
printf '%s\n' '--- version declarations and commands ---'
rg -n --hidden -g '!target' -g '!node_modules' \
'rust-version|RUST_VERSION|rust-toolchain|rustup (default|show)|dtolnay/rust-toolchain|cargo \+[0-9]|rustc --version|cargo (check|test|clippy|fmt|build)' \
Cargo.toml rust-toolchain.toml Dockerfile .github/workflowsRepository: Limen-Neural/engram-parser
Length of output: 156
🌐 Web query:
rustup official documentation toolchain override precedence RUSTUP_TOOLCHAIN rust-toolchain.toml
💡 Result:
Rustup determines which toolchain to use by checking several sources in a specific order of precedence. It selects the first one encountered in the following sequence [1][2]: 1. Toolchain override shorthand: The command-line argument (e.g., cargo +beta) [1][2]. 2. RUSTUP_TOOLCHAIN environment variable: A toolchain specified via this environment variable [1][2]. 3. Directory override: Set via the rustup override command [1][2]. 4. rust-toolchain.toml file: A file located in the current or a parent directory [1][2]. 5. Default toolchain: The global default toolchain [1][2]. Regarding directory overrides and the rust-toolchain.toml file, Rustup searches for them by walking up the directory tree toward the filesystem root [1][3]. Because of this, proximity to the current directory takes precedence; a rust-toolchain.toml file or directory override found in a directory closer to your current location will be preferred over one found further up the tree [1][3]. For file-based overrides (directory overrides and rust-toolchain.toml), if both are encountered at the same level in the directory tree during the search, the directory override (set via the rustup override command) is prioritized over the rust-toolchain.toml file [1][3].
Citations:
- 1: https://rust-lang.github.io/rustup/overrides.html?highlight=toolchain
- 2: https://rust-lang.github.io/rustup/overrides.html
- 3: https://rust-lang.github.io/rustup/overrides.html?highlight=rust-toolchain.toml
🌐 Web query:
official Rust Docker image rustup default toolchain rust version image directory override rust-toolchain.toml
💡 Result:
Official Rust Docker images are built with rustup pre-installed [1][2][3]. Because rustup manages toolchains dynamically, it prioritizes configurations found in the directory tree over the default toolchain installed in the image [4][5]. When a project contains a rust-toolchain.toml file, running cargo or rustc inside a container will cause rustup to detect this file and automatically switch to, or download, the specified toolchain [4][6][7]. This behavior frequently results in unexpected toolchain downloads or build inconsistencies during Docker builds [8][6][7]. To manage or override this behavior in Docker environments, you can use the following methods: 1. RUSTUP_TOOLCHAIN Environment Variable: Setting this variable forces cargo and other rustup-proxied commands to use the specified toolchain, effectively ignoring rust-toolchain.toml or other directory overrides [8][9]. For example: ENV RUSTUP_TOOLCHAIN=stable 2. Excluding Files: Including rust-toolchain.toml in your.dockerignore file prevents the file from being copied into the container, ensuring rustup does not detect it [7]. 3. Directory Overrides: While rustup supports directory-level overrides via rustup override, these are typically less practical in non-interactive container environments compared to environment variables [4][5][9]. For deterministic builds that rely strictly on the pre-installed toolchain within the image, setting RUSTUP_TOOLCHAIN is the most direct approach to disable rustup's automatic discovery mechanism [8].
Citations:
- 1: https://hub.docker.com/_/rust
- 2: https://github.com/rust-lang/docker-rust/
- 3: https://hub.docker.com/r/rustlang/rust/dockerfile
- 4: https://rust-lang.github.io/rustup/overrides.html
- 5: https://rust-lang.github.io/rustup/overrides.html?highlight=rust-toolchain.toml
- 6: Is there no way to get a regular cargo that isn't rustup's cargo?? rust-lang/docker-rust#212
- 7: Integration tests runner downloads rustc again vectordotdev/vector#21923
- 8: Fix docker build toolchain override from rust-toolchain.toml (#50) solana-foundation/solana-verifiable-build#275
- 9: https://rust-lang.github.io/rustup/environment-variables.html?highlight=ram
🏁 Script executed:
#!/usr/bin/env bash
set -u
for f in rust-toolchain.toml Dockerfile Cargo.toml .github/workflows/ci.yml; do
printf '\n--- %s ---\n' "$f"
if [ -f "$f" ]; then
case "$f" in
Dockerfile) sed -n '1,45p' "$f" ;;
.github/workflows/ci.yml) sed -n '75,130p' "$f" ;;
*) cat "$f" ;;
esac
else
printf 'MISSING\n'
fi
doneRepository: Limen-Neural/engram-parser
Length of output: 3762
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- targeted references ---'
for f in Cargo.toml Dockerfile .github/workflows/ci.yml; do
[ -f "$f" ] || continue
grep -nE 'rust-version|RUST_VERSION|rust-toolchain|rustup|dtolnay/rust-toolchain|cargo \+|cargo (check|test|clippy|fmt|build)|rustc' "$f" || true
doneRepository: Limen-Neural/engram-parser
Length of output: 156
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- workflow commands around MSRV job ---'
sed -n '85,125p' .github/workflows/ci.yml 2>/dev/null || trueRepository: Limen-Neural/engram-parser
Length of output: 1241
🌐 Web query:
dtolnay/rust-toolchain GitHub action README toolchain input rustup default override rust-toolchain.toml
💡 Result:
The dtolnay/rust-toolchain GitHub Action is designed for concise usage and relies on standard rustup behavior to manage toolchains [1][2]. Regarding the interaction between the action and rust-toolchain.toml files: 1. Interaction with rust-toolchain.toml: The action installs the specified toolchain using rustup default [3]. It does not explicitly call rustup override set [4][5]. Because rustup has a defined hierarchy for toolchain selection, a rust-toolchain.toml file located in your repository will take precedence over the global default set by this action [6][5]. Therefore, the action effectively "installs" the toolchain, but the project's local rust-toolchain.toml file will continue to control which toolchain is actually used during cargo operations within that directory [6][5]. 2. Handling toolchain inputs: If you provide an explicit toolchain input to the action (e.g., with: toolchain: stable), this input overrides the toolchain version that would otherwise be selected [5]. Note that this does not necessarily prevent a rust-toolchain.toml file from overriding the toolchain later, but the action will have successfully installed the toolchain requested via the input [5]. 3. Best practices for overrides: If you need to force the use of a specific toolchain regardless of a rust-toolchain.toml file, you can utilize the action's output [4]. The action provides an output named name, which contains the toolchain string [1][2]. You can use this to explicitly run commands with the intended toolchain [4]: - uses: dtolnay/rust-toolchain@master id: toolchain with: toolchain: nightly - run: cargo +${{steps.toolchain.outputs.name}} build Alternatively, you can use rustup to set an override manually in your workflow after the action has finished [4]: - uses: dtolnay/rust-toolchain@master id: toolchain - run: rustup override set ${{steps.toolchain.outputs.name}} - run: cargo build In summary, the action respects standard rustup mechanisms. It installs the requested toolchain as the default, but existing rust-toolchain.toml files will still take precedence as per rustup's native override hierarchy [6][5].
Citations:
- 1: https://github.com/dtolnay/rust-toolchain
- 2: https://github.com/dtolnay/rust-toolchain/blob/master/README.md
- 3: https://github.com/dtolnay/rust-toolchain/blob/master/action.yml
- 4:
defaultvsoverridedtolnay/rust-toolchain#29 - 5: ci: workflow jobs override rust-toolchain.toml pin via toolchain: stable input clouatre-labs/aptu#1141
- 6: https://rust-lang.github.io/rustup/overrides.html?highlight=toolchain
🌐 Web query:
"dtolnay/rust-toolchain" "RUSTUP_TOOLCHAIN"
💡 Result:
In the context of the dtolnay/rust-toolchain GitHub Action, RUSTUP_TOOLCHAIN is not an input parameter of the action itself, but rather an environment variable that interacts with how rustup behaves during your workflow [1][2]. When using dtolnay/rust-toolchain, the action installs a specific Rust toolchain [1]. However, rustup typically prioritizes a rust-toolchain.toml (or rust-toolchain) file in your repository if one exists [3][4][2]. If you have a rust-toolchain.toml file but want your CI workflow to use a different toolchain (e.g., in a build matrix), setting the RUSTUP_TOOLCHAIN environment variable can be used to override that file [4][2][5]. Because RUSTUP_TOOLCHAIN has a higher priority than the rust-toolchain.toml file, setting it ensures that cargo and other proxied tools use the toolchain you specify via the environment variable, effectively ignoring the file [3][4][5][6]. This technique is commonly used to prevent rustup from re-resolving or overriding the toolchain choice during CI execution, which can sometimes cause flakiness or unintended behavior [2][5]. To use this in a GitHub workflow, you can set it at the job or step level [2][5]: jobs: test: runs-on: ubuntu-latest env: RUSTUP_TOOLCHAIN: ${{ matrix.rust }} steps: - uses: dtolnay/rust-toolchain@master with: toolchain: ${{ matrix.rust }} In this configuration, the dtolnay/rust-toolchain action installs the toolchain, and RUSTUP_TOOLCHAIN ensures that subsequent cargo commands honor that specific version regardless of any rust-toolchain configuration files present in the repository [4][2][5].
Citations:
- 1: https://github.com/dtolnay/rust-toolchain
- 2: https://github.com/payjoin/rust-payjoin/blob/c73733ffd729c47889117f5d42a42809649bff42/.github/workflows/rust.yml
- 3: https://internals.rust-lang.org/t/priority-of-directory-override-and-rust-toolchain-file/13224
- 4: fix CI job for building on MSRV rust-osdev/x86_64#510
- 5: stescobedo92/hide-env-keys@510f709
- 6: https://rust-lang.github.io/rustup/environment-variables.html
🏁 Script executed:
#!/usr/bin/env bash
set -u
url='https://raw.githubusercontent.com/dtolnay/rust-toolchain/29eef336d9b2848a0b548edc03f92a220660cdb8/action.yml'
curl --fail --silent --show-error --max-time 20 "$url" | sed -n '1,220p'Repository: Limen-Neural/engram-parser
Length of output: 6765
Ensure Docker and the MSRV job use Rust 1.97.1.
rust-toolchain.toml overrides the Docker image toolchain and the default set by dtolnay/rust-toolchain. Set RUSTUP_TOOLCHAIN=1.97.1 in both environments, or invoke each Cargo command with cargo +1.97.1.
📍 Affects 3 files
rust-toolchain.toml#L4-L6(this comment)Dockerfile#L18-L20.github/workflows/ci.yml#L94-L99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust-toolchain.toml` around lines 4 - 6, Set the toolchain to Rust 1.97.1 in
rust-toolchain.toml (lines 4-6), and ensure Dockerfile (lines 18-20) and
.github/workflows/ci.yml (lines 94-99) explicitly use the same version via
RUSTUP_TOOLCHAIN=1.97.1 or cargo +1.97.1 for their Cargo commands.
Source: Coding guidelines
| pub const GGUF_VALUE_TYPE_UINT8: u32 = 0; | ||
| pub const GGUF_VALUE_TYPE_INT8: u32 = 1; | ||
| pub const GGUF_VALUE_TYPE_UINT16: u32 = 2; | ||
| pub const GGUF_VALUE_TYPE_INT16: u32 = 3; | ||
| pub const GGUF_VALUE_TYPE_UINT32: u32 = 4; | ||
| pub const GGUF_VALUE_TYPE_INT32: u32 = 5; | ||
| pub const GGUF_VALUE_TYPE_FLOAT32: u32 = 6; | ||
| pub const GGUF_VALUE_TYPE_BOOL: u32 = 7; | ||
| pub const GGUF_VALUE_TYPE_STRING: u32 = 8; | ||
| pub const GGUF_VALUE_TYPE_ARRAY: u32 = 9; | ||
| pub const GGUF_VALUE_TYPE_UINT64: u32 = 10; | ||
| pub const GGUF_VALUE_TYPE_INT64: u32 = 11; | ||
| pub const GGUF_VALUE_TYPE_FLOAT64: u32 = 12; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check crate-root lint attributes and undocumented public items.
set -euo pipefail
echo "=== crate-root attributes ==="
fd -t f 'lib.rs' src | xargs -r sed -n '1,20p'
echo
echo "=== undocumented pub items in src ==="
fd -e rs . src --exec awk '
/^[[:space:]]*(\/\/\/|\/\/!|#\[)/ { doc=1; next }
/^[[:space:]]*pub (const|fn|struct|enum|mod|type) / { if (!doc) print FILENAME": "FNR": "$0 }
{ doc=0 }
' {}Repository: Limen-Neural/engram-parser
Length of output: 2219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== lint configuration ==="
rg -n --hidden -g '!target' 'missing_docs|deny\(warnings\)|forbid\(warnings\)|warn\(missing_docs\)' . || true
echo
echo "=== public exports and constant definitions ==="
rg -n -C 3 'GGUF_VALUE_TYPE_|pub use|pub mod' src/gguf src/lib.rs
echo
echo "=== package metadata and lint sections ==="
cat Cargo.tomlRepository: Limen-Neural/engram-parser
Length of output: 10500
Document the public GGUF value-type constants.
These constants are re-exported from src/lib.rs but have no doc comments. Add one /// comment for each constant. The crate does not enable missing_docs, so this is an API documentation improvement, not a build requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gguf/cursor.rs` around lines 27 - 39, Add a concise Rust doc comment to
each public GGUF value-type constant from GGUF_VALUE_TYPE_UINT8 through
GGUF_VALUE_TYPE_FLOAT64, describing the corresponding GGUF value type. Keep the
existing constant names and numeric values unchanged.
Source: Learnings
| /// Types not explicitly enumerated are captured by [`DType::Other(u32)`] | ||
| /// which preserves the raw code for callers to dispatch on. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate rustdoc intra-doc links that include a tuple-variant payload.
set -euo pipefail
rg -nP '\[`[A-Za-z_:]+\([a-z0-9]+\)`\]' --type=rustRepository: Limen-Neural/engram-parser
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd -t f 'tensor\.rs$' . | head -n 1)
printf '%s\n' "FILE=$file"
sed -n '1,190p' "$file"
printf '%s\n' '--- DType declarations and links ---'
rg -n -C 3 'DType::Other|enum DType|Other\s*\(' --type rust .
printf '%s\n' '--- manifest and quality-gate configuration ---'
fd -t f '(Cargo\.toml|rust-toolchain.*|Makefile|justfile|\.github)' . | sort | head -n 80Repository: Limen-Neural/engram-parser
Length of output: 17019
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd -t f 'tensor\.rs$' . | head -n 1)
printf '%s\n' "FILE=$file"
sed -n '120,175p' "$file"
printf '%s\n' '--- DType declarations and links ---'
rg -n -C 3 'DType::Other|enum DType|Other\s*\(' --type rust .
printf '%s\n' '--- manifests and toolchain files ---'
fd -t f 'Cargo\.toml|rust-toolchain.*|Makefile|justfile' . | sortRepository: Limen-Neural/engram-parser
Length of output: 11636
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if ! command -v rustdoc >/dev/null 2>&1; then
printf '%s\n' 'rustdoc unavailable'
exit 0
fi
rustdoc --version
tmpdir=$(mktemp -d)
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/probe.rs" <<'RS'
#![deny(rustdoc::broken_intra_doc_links)]
/// A dtype.
pub enum DType {
/// An opaque dtype.
Other(u32),
}
/// [`DType::Other(u32)`]
pub fn with_payload() {}
/// [`DType::Other`]
pub fn without_payload() {}
RS
set +e
rustdoc --edition 2021 --crate-name probe "$tmpdir/probe.rs" -o "$tmpdir/doc" \
>"$tmpdir/stdout" 2>"$tmpdir/stderr"
status=$?
set -e
printf '%s\n' "rustdoc_exit=$status"
cat "$tmpdir/stderr"Repository: Limen-Neural/engram-parser
Length of output: 184
🌐 Web query:
rustdoc intra-doc link tuple enum variant payload DType::Other(u32) unresolved link syntax
💡 Result:
To link to an enum variant in rustdoc, the standard syntax is to use the full path to the variant: [MyEnum::MyVariant] [1][2]. If the link is not resolving correctly, it is often because rustdoc cannot distinguish the variant from other items with similar names, or because the specific anchor syntax is required [3][4]. For linking to enum variants, you can use the #variant. anchor syntax if the standard path resolution fails: [MyVariant]: MyEnum#variant.MyVariant [5] Regarding tuple variant fields (e.g., DType::Other(u32)), rustdoc's support for linking directly to individual tuple fields within an enum variant is limited. While Rustdoc supports documenting tuple struct fields [6], it does not currently provide a native, stable intra-doc link syntax to reference a specific field (like 0) inside a tuple variant [7][8]. If you receive an "unresolved link" error, consider these troubleshooting steps: 1. Disambiguation: Use the variant@ prefix to explicitly tell rustdoc you are linking to a variant [9]: [variant@DType::Other] 2. Verify Path: Ensure the path is reachable from the module where the documentation comment is written. If the item is in a different crate, ensure that crate is in your documentation's scope (i.e., it must be a dependency and documented) [10]. 3. Anchor Links: If you are linking to a location within the same documentation page, you can use Markdown fragment identifiers (e.g., [Link Text](#variant.Other)), though this is less robust than standard intra-doc links [5][2]. Because tuple fields in enum variants are often treated as anonymous or "free-floating" items in generated rustdoc metadata [7], they are not consistently addressable via standard intra-doc link paths. If you need to link to the payload specifically, you may need to use a standard URL link to the generated documentation page for that enum [10].
Citations:
- 1: https://rust-lang.github.io/rfcs/1946-intra-rustdoc-links.html
- 2: https://stackoverflow.com/questions/31582064/how-to-link-to-other-fns-structs-enums-traits-in-rustdoc
- 3: https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html
- 4: https://doc.rust-lang.org/beta/rustdoc/lints.html
- 5: https://stackoverflow.com/questions/73316074/rustdoc-link-to-enum-variant
- 6: Add support for tuple struct field documentation rust-lang/rust#87451
- 7: [rustdoc-json] Free-floating
struct_fielditems with enum tuple struct variants rust-lang/rust#92945 - 8: Rustdoc should pick up docs for tuple struct and enum fields rust-lang/rust#42615
- 9: Add
field@andvariant@doc-link disambiguators rust-lang/rust#130587 - 10: https://users.rust-lang.org/t/how-do-intra-doc-links-work/138250
Fix the unresolved intra-doc link. Link to [DType::Other] and render the payload as raw u32 text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gguf/tensor.rs` around lines 154 - 155, Update the documentation in the
tensor type comment to link specifically to DType::Other and format its u32
payload as raw u32 text, resolving the intra-doc link without changing the
documented behavior.
Source: Learnings
| // Re-export commonly used types at the crate root for convenience. | ||
| pub use error::ParserError; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the definition and re-export status of the public `Result` alias.
set -euo pipefail
echo "=== definition ==="
rg -nP --type=rust '^\s*pub type Result' src
echo
echo "=== crate-root re-exports from error ==="
rg -nP --type=rust -C2 'pub use error::' src/lib.rs
echo
echo "=== references to the root path ==="
rg -nP --type=rust '\bengram_parser::Result\b'
echo
echo "=== changelog mentions ==="
fd -t f 'CHANGELOG.md' --exec rg -n -i 'result|breaking' {}Repository: Limen-Neural/engram-parser
Length of output: 475
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== error alias and crate-root exports ==="
cat -n src/error.rs | sed -n '82,98p'
cat -n src/lib.rs | sed -n '38,55p'
echo
echo "=== README API list and usage examples ==="
cat -n README.md | sed -n '108,130p'
rg -n -C2 'engram_parser::Result|use engram_parser::.*Result|Result<' README.md src tests examples 2>/dev/null || true
echo
echo "=== package version and changelog ==="
rg -n '^(version|name)\s*=' Cargo.toml
if fd -t f '^CHANGELOG\.md$' . | head -1 | read -r changelog; then
cat -n "$changelog" | sed -n '1,100p'
else
echo "No CHANGELOG.md found"
fi
echo
echo "=== repository-wide Result references ==="
rg -n '\bResult\b' --glob '!target/**' --glob '!Cargo.lock' | head -120 || trueRepository: Limen-Neural/engram-parser
Length of output: 17309
Align the public Result API. Result<T> remains public under engram_parser::error, but src/lib.rs does not re-export it at the crate root while README.md lists it as a public API. Restore pub use error::{ParserError, Result}; or remove Result from the README.
📍 Affects 2 files
src/lib.rs#L45-L46(this comment)README.md#L119-L122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib.rs` around lines 45 - 46, Align the public Result API by updating
src/lib.rs:45-46 to re-export Result alongside ParserError from error,
preserving README.md:119-122 as documented; no direct README.md change is
needed.
| let kv = [ | ||
| ("general.architecture", KvValue::Str("qwen2moe")), | ||
| ("general.quantization_type", KvValue::Str("Q4_K_M")), | ||
| ("qwen2moe.block_count", KvValue::U32(28)), | ||
| ("qwen2moe.expert_count", KvValue::U32(64)), | ||
| ("qwen2moe.expert_used_count", KvValue::U32(8)), | ||
| ("qwen2moe.embedding_length", KvValue::U32(2048)), | ||
| ("qwen2moe.attention.head_count", KvValue::U32(16)), | ||
| ("general.name", KvValue::Str("Qwen2-MoE-A2.7B")), | ||
| ]; | ||
|
|
||
| // Build custom GGUF with f32 metadata | ||
| let mut out = Vec::new(); | ||
| out.extend_from_slice(&GGUF_MAGIC); | ||
| push_u32(&mut out, GGUF_VERSION); | ||
| push_u64(&mut out, 0); // no tensors | ||
| push_u64(&mut out, kv.len() as u64); | ||
|
|
||
| for (key, value) in &kv { | ||
| match value { | ||
| KvValue::U32(v) => push_kv_u32(&mut out, key, *v), | ||
| KvValue::Str(v) => push_kv_string(&mut out, key, v), | ||
| KvValue::F32(v) => push_kv_f32(&mut out, key, *v), | ||
| } | ||
| } | ||
|
|
||
| // Align | ||
| while out.len() % ALIGNMENT as usize != 0 { | ||
| out.push(0); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Exercise the f32 metadata path, and reuse build_gguf.
Two problems overlap here.
First, no test writes an f32 KV pair. The kv array contains only Str and U32 entries, so push_kv_f32 and VT_F32 are dead in this test, KvValue::F32 needs #[allow(dead_code)] at Line 93, and capture_f32_kv in src/gguf/layout.rs stays uncovered. Line 369 only asserts the missing-key case. Add a real f32 entry and assert float32 returns it.
Second, Lines 333-350 reimplement build_gguf byte for byte. build_gguf(&kv, &[]) emits the identical sequence and already handles KvValue::F32. The same duplication exists at Lines 380-396 and Lines 409-424, and it has already diverged: only this copy handles the F32 arm, while the other two copies use unreachable!().
♻️ Proposed refactor
#[test]
fn metadata_helper_methods() {
- // Value type for f32
- const VT_F32: u32 = 6;
-
- fn push_kv_f32(out: &mut Vec<u8>, key: &str, v: f32) {
- push_string(out, key);
- push_u32(out, VT_F32);
- out.extend_from_slice(&v.to_le_bytes());
- }
-
let kv = [
("general.architecture", KvValue::Str("qwen2moe")),
("general.quantization_type", KvValue::Str("Q4_K_M")),
("qwen2moe.block_count", KvValue::U32(28)),
("qwen2moe.expert_count", KvValue::U32(64)),
("qwen2moe.expert_used_count", KvValue::U32(8)),
("qwen2moe.embedding_length", KvValue::U32(2048)),
("qwen2moe.attention.head_count", KvValue::U32(16)),
("general.name", KvValue::Str("Qwen2-MoE-A2.7B")),
+ ("qwen2moe.rope.freq_base", KvValue::F32(10000.0)),
];
- // Build custom GGUF with f32 metadata
- let mut out = Vec::new();
- out.extend_from_slice(&GGUF_MAGIC);
- push_u32(&mut out, GGUF_VERSION);
- push_u64(&mut out, 0); // no tensors
- push_u64(&mut out, kv.len() as u64);
-
- for (key, value) in &kv {
- match value {
- KvValue::U32(v) => push_kv_u32(&mut out, key, *v),
- KvValue::Str(v) => push_kv_string(&mut out, key, v),
- KvValue::F32(v) => push_kv_f32(&mut out, key, *v),
- }
- }
-
- // Align
- while out.len() % ALIGNMENT as usize != 0 {
- out.push(0);
- }
-
- let layout = parse_bytes(out, "mem://metadata".into()).expect("parse");
+ let layout = parse_bytes(build_gguf(&kv, &[]), "mem://metadata".into()).expect("parse");Then assert the f32 round trip:
assert_eq!(layout.metadata.float32("qwen2moe.rope.freq_base"), Some(10000.0));Apply the same build_gguf(&kv, &[]) replacement at Lines 380-396 and Lines 409-424, and drop #[allow(dead_code)] at Line 93.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/gguf_smoke.rs` around lines 321 - 350, Update the GGUF smoke test’s
metadata fixtures to include a real qwen2moe.rope.freq_base KvValue::F32 entry,
then assert layout.metadata.float32 returns 10000.0. Replace each duplicated
manual GGUF byte-building block near the affected tests with build_gguf(&kv,
&[]), including the copies around the later test cases, and remove the
now-unneeded #[allow(dead_code)] on KvValue::F32.
| let max = env::var(ENV_MAX) | ||
| .ok() | ||
| .and_then(|s| s.parse::<usize>().ok()) | ||
| .unwrap_or(8); | ||
|
|
||
| let mut out = Vec::new(); | ||
| collect_gguf(&root, 0, 6, max, &mut out); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Lower the default ENGRAM_GGUF_MAX or document the RAM cost.
load_gguf reads each file fully into a Vec<u8>. When only ENGRAM_MODEL_DIR is set, this defaults to 8 files, and both pilot tests then load each one in sequence. The README at Line 156 tells the operator to use one ENGRAM_GGUF per process and to keep free RAM at or above the file size. The directory-scan default works against that guidance and can run 8 multi-gigabyte loads in a single process.
Set the default to 1, so a multi-file scan becomes an explicit opt-in.
🛡️ Proposed change
+ // One file per process by default: `load_gguf` reads the whole file into
+ // memory, so scanning a tree of multi-GB weights must be opt-in.
let max = env::var(ENV_MAX)
.ok()
.and_then(|s| s.parse::<usize>().ok())
- .unwrap_or(8);
+ .unwrap_or(1);Based on learnings: "Run real-GGUF pilots locally one file per process; do not scan multiple multi-gigabyte files because load_gguf reads the entire file into memory."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let max = env::var(ENV_MAX) | |
| .ok() | |
| .and_then(|s| s.parse::<usize>().ok()) | |
| .unwrap_or(8); | |
| let mut out = Vec::new(); | |
| collect_gguf(&root, 0, 6, max, &mut out); | |
| // One file per process by default: `load_gguf` reads the whole file into | |
| // memory, so scanning a tree of multi-GB weights must be opt-in. | |
| let max = env::var(ENV_MAX) | |
| .ok() | |
| .and_then(|s| s.parse::<usize>().ok()) | |
| .unwrap_or(1); | |
| let mut out = Vec::new(); | |
| collect_gguf(&root, 0, 6, max, &mut out); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/real_gguf.rs` around lines 75 - 81, Change the ENV_MAX fallback in the
test’s environment-variable parsing from 8 to 1, while preserving the existing
override behavior when ENGRAM_GGUF_MAX is explicitly set. Keep collect_gguf and
its other arguments unchanged.
Source: Learnings
| fn collect_gguf( | ||
| dir: &Path, | ||
| depth: usize, | ||
| max_depth: usize, | ||
| max_files: usize, | ||
| out: &mut Vec<PathBuf>, | ||
| ) { | ||
| if out.len() >= max_files || depth > max_depth { | ||
| return; | ||
| } | ||
| let Ok(entries) = fs::read_dir(dir) else { | ||
| return; | ||
| }; | ||
| let mut dirs = Vec::new(); | ||
| for entry in entries.flatten() { | ||
| if out.len() >= max_files { | ||
| break; | ||
| } | ||
| let path = entry.path(); | ||
| if path.is_file() { | ||
| if path | ||
| .extension() | ||
| .and_then(|e| e.to_str()) | ||
| .is_some_and(|e| e.eq_ignore_ascii_case("gguf")) | ||
| { | ||
| out.push(path); | ||
| } | ||
| } else if path.is_dir() { | ||
| dirs.push(path); | ||
| } | ||
| } | ||
| dirs.sort(); | ||
| for d in dirs { | ||
| collect_gguf(&d, depth + 1, max_depth, max_files, out); | ||
| if out.len() >= max_files { | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Split the two functions that fail the CodeScene gate.
The CodeScene gate fails on this file with a health score of 7.72 against a required 10.00. It names Complex Method and Bumpy Road Ahead in real_gguf_moe_extract_when_present and collect_gguf.
real_gguf_moe_extract_when_present nests a per-path loop, a per-expert loop, and a per-role loop with branching inside all three. collect_gguf interleaves the file filter, the directory collection, and the recursion in one body with four separate bound checks.
Extract the per-role assertions and the per-path body from the test, and extract the entry partition from collect_gguf. Neither extraction changes behavior.
♻️ Proposed extraction
+fn assert_raw_tensor(role: &str, t: &engram_parser::RawTensor) {
+ assert!(!t.bytes.is_empty(), "{role} empty bytes");
+ assert!(!t.dims.is_empty(), "{role} empty dims");
+ let elems: usize = t.dims.iter().product();
+ match t.dtype {
+ DType::F16 | DType::BF16 => assert_eq!(t.bytes.len(), elems * 2),
+ DType::F32 => assert_eq!(t.bytes.len(), elems * 4),
+ _ => {}
+ }
+}
+
+/// Returns `true` when at least one MoE pair was extracted for `path`.
+fn scan_one_pilot(path: &Path, samples: usize) -> bool {
+ let t0 = Instant::now();
+ let layout = load_gguf(path).expect("load");
+ let load_ms = t0.elapsed().as_secs_f64() * 1000.0;
+
+ let experts = list_experts(&layout);
+ eprintln!(
+ "moe_scan {} arch={} quant={} expert_meta={:?} pairs={} load_ms={load_ms:.1}",
+ path.display(),
+ layout.metadata.architecture(),
+ layout.metadata.quantization(),
+ layout.metadata.expert_count(),
+ experts.len(),
+ );
+
+ if experts.is_empty() {
+ eprintln!("skip MoE (none discovered): {}", path.display());
+ return false;
+ }
+
+ for &(b, e) in experts.iter().take(samples.min(experts.len())) {
+ let w = extract_expert(&layout, b, e).unwrap_or_else(|err| {
+ panic!("extract_expert({}, {b}, {e}): {err}", path.display());
+ });
+ assert!(
+ w.gate.is_some() || w.up.is_some() || w.down.is_some(),
+ "{}: empty extract for ({b},{e})",
+ path.display()
+ );
+ for (role, opt) in [
+ ("gate", w.gate.as_ref()),
+ ("up", w.up.as_ref()),
+ ("down", w.down.as_ref()),
+ ] {
+ if let Some(t) = opt {
+ assert_raw_tensor(role, t);
+ }
+ }
+ eprintln!(
+ "OK moe {} pair=({b},{e}) complete={} stacked_gate={}",
+ path.display(),
+ w.is_complete(),
+ w.gate.as_ref().map(|g| g.stacked_slice).unwrap_or(false),
+ );
+ }
+
+ if let Some(n) = layout.metadata.expert_count() {
+ assert!(n > 0, "{}: expert_count metadata is 0", path.display());
+ }
+ true
+}real_gguf_moe_extract_when_present then reduces to the pilot loop plus the final hard check. For collect_gguf, extract the entry partition:
fn partition_entries(dir: &Path) -> (Vec<PathBuf>, Vec<PathBuf>) {
let mut files = Vec::new();
let mut dirs = Vec::new();
let Ok(entries) = fs::read_dir(dir) else {
return (files, dirs);
};
for entry in entries.flatten() {
let path = entry.path();
if path.is_dir() {
dirs.push(path);
} else if path
.extension()
.and_then(|e| e.to_str())
.is_some_and(|e| e.eq_ignore_ascii_case("gguf"))
{
files.push(path);
}
}
files.sort();
dirs.sort();
(files, dirs)
}Also applies to: 190-269
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/real_gguf.rs` around lines 86 - 124, Reduce CodeScene complexity by
extracting the per-role assertions and per-path body from
real_gguf_moe_extract_when_present, preserving the existing assertions and
pilot-loop behavior. In collect_gguf, extract entry scanning and file/directory
partitioning into a helper such as partition_entries, then retain
max_files/depth checks and recursion in collect_gguf while preserving ordering
and behavior.
Source: Pipeline failures
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 17 files
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="REVIEW.md">
<violation number="1" location="REVIEW.md:143">
P3: Running the documented local coverage command creates an unignored `lcov.info`, conflicting with the clean-tree workflow and allowing accidental commits. Consider ignoring this generated file or writing/removing it outside the checkout.</violation>
<violation number="2" location="REVIEW.md:287">
P2: These RAM minima are below the documented measured peaks, so following them can OOM the T1 pilots. Raise them above 17.7/25.8 GiB (plus margin), or add `--test-threads=1` to the pilot commands and revalidate lower serialized-test requirements.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:158">
P3: The documented “Large MoE” invocation never sets these variables for the test that consumes them; the following example ignores both. Showing the variables on a `real_gguf_moe` test command would make the advertised hard-fail and sampling configuration effective.</violation>
</file>
<file name="tests/real_gguf.rs">
<violation number="1" location="tests/real_gguf.rs:145">
P2: Running the two T1 pilots as documented will load every multi-GB GGUF twice simultaneously (once per test fn) in the same process, since both `real_gguf_parse_inventory` and `real_gguf_moe_extract_when_present` independently `require_pilots()` + `load_gguf()`, and identical non-`#[ignore]`... both are `#[ignore]` so cargo runs them in parallel threads. For ZAYA1-8B/OLMoE this roughly doubles the already-2x peak RSS and can OOM (worst on a tree scan with ENGRAM_GGUF_MAX files). Consider running pilots under `--test-threads=1` (note it in the doc comment/README), or hoisting the load so the two tests share one layout.</violation>
</file>
<file name="tests/gguf_smoke.rs">
<violation number="1" location="tests/gguf_smoke.rs:343">
P3: The f32 metadata support added in this PR isn't actually covered: no KvValue::F32 is placed in the test's kv map, so the F32 arm / push_kv_f32 never run and float32(key) is only checked for a missing key returning None. Add a real f32 KV (e.g. "qwen2moe.rope_freq_base") and assert float32 returns Some(<value>) so the positive capture path (and the survived #[allow(dead_code)] variant) are actually exercised.</violation>
</file>
<file name="rust-toolchain.toml">
<violation number="1" location="rust-toolchain.toml:5">
P2: The newly added `rust-toolchain.toml` pins `channel = "stable"`, but that file silently overrides the pinned toolchains used by the CI `msrv` job and the Docker build, so the MSRV gate no longer actually runs on 1.97.1. Rustup resolves the active toolchain in priority order, and a repo-local `rust-toolchain.toml` outranks the default toolchain that `dtolnay/rust-toolchain` sets. That means in `.github/workflows/ci.yml`'s `msrv` job, `cargo fmt/clippy/build/test` will run on stable instead of the installed 1.97.1, and in the `Dockerfile` (which builds the `rust:1.97.1-slim` image) cargo will try to use a `stable` toolchain that isn't even present in the image — potentially failing the build or, in CI, silently skipping the real MSRV verification. Recommend forcing the pinned toolchain in both places (e.g. set `RUSTUP_TOOLCHAIN=1.97.1` for the msrv job/container, or remove `rust-toolchain.toml` before `cargo` invocations) so the MSRV guarantee is actually exercised.</violation>
</file>
<file name="examples/inspect_gguf.rs">
<violation number="1" location="examples/inspect_gguf.rs:118">
P2: A discovered expert can fail extraction while this command still exits successfully, making scripted pilot runs miss the failure. Consider writing the error to stderr and returning a nonzero `ExitCode` from this branch.</violation>
</file>
<file name=".gitignore">
<violation number="1" location=".gitignore:34">
P2: Environment templates such as `.env.example` and `.env.template` can no longer be added normally because `.env*` treats them as secrets. Consider explicit negations for safe template files while continuing to ignore real environment files.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| let paths = require_pilots(); | ||
| for path in paths { | ||
| let t0 = Instant::now(); | ||
| let layout = load_gguf(&path).unwrap_or_else(|e| { |
There was a problem hiding this comment.
P2: Running the two T1 pilots as documented will load every multi-GB GGUF twice simultaneously (once per test fn) in the same process, since both real_gguf_parse_inventory and real_gguf_moe_extract_when_present independently require_pilots() + load_gguf(), and identical non-#[ignore]... both are #[ignore] so cargo runs them in parallel threads. For ZAYA1-8B/OLMoE this roughly doubles the already-2x peak RSS and can OOM (worst on a tree scan with ENGRAM_GGUF_MAX files). Consider running pilots under --test-threads=1 (note it in the doc comment/README), or hoisting the load so the two tests share one layout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/real_gguf.rs, line 145:
<comment>Running the two T1 pilots as documented will load every multi-GB GGUF twice simultaneously (once per test fn) in the same process, since both `real_gguf_parse_inventory` and `real_gguf_moe_extract_when_present` independently `require_pilots()` + `load_gguf()`, and identical non-`#[ignore]`... both are `#[ignore]` so cargo runs them in parallel threads. For ZAYA1-8B/OLMoE this roughly doubles the already-2x peak RSS and can OOM (worst on a tree scan with ENGRAM_GGUF_MAX files). Consider running pilots under `--test-threads=1` (note it in the doc comment/README), or hoisting the load so the two tests share one layout.</comment>
<file context>
@@ -0,0 +1,290 @@
+ let paths = require_pilots();
+ for path in paths {
+ let t0 = Instant::now();
+ let layout = load_gguf(&path).unwrap_or_else(|e| {
+ panic!("load_gguf({}) failed: {e}", path.display());
+ });
</file context>
| | Model | Path (this machine) | Size | Min free RAM | | ||
| |-------|---------------------|------|--------------| | ||
| | jina F16 (smoke) | `~/.models/jinaai/…/v5-nano-text-matching-F16.gguf` | ~0.4 GiB | ≥ 2 GiB | | ||
| | ZAYA1-8B Q8_0 | `~/.models/gguf/Abiray/ZAYA1-8B-GGUF/ZAYA1-8B-Q8_0.gguf` | ~8.83 GiB | ≥ 12 GiB available | |
There was a problem hiding this comment.
P2: These RAM minima are below the documented measured peaks, so following them can OOM the T1 pilots. Raise them above 17.7/25.8 GiB (plus margin), or add --test-threads=1 to the pilot commands and revalidate lower serialized-test requirements.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At REVIEW.md, line 287:
<comment>These RAM minima are below the documented measured peaks, so following them can OOM the T1 pilots. Raise them above 17.7/25.8 GiB (plus margin), or add `--test-threads=1` to the pilot commands and revalidate lower serialized-test requirements.</comment>
<file context>
@@ -0,0 +1,373 @@
+| Model | Path (this machine) | Size | Min free RAM |
+|-------|---------------------|------|--------------|
+| jina F16 (smoke) | `~/.models/jinaai/…/v5-nano-text-matching-F16.gguf` | ~0.4 GiB | ≥ 2 GiB |
+| ZAYA1-8B Q8_0 | `~/.models/gguf/Abiray/ZAYA1-8B-GGUF/ZAYA1-8B-Q8_0.gguf` | ~8.83 GiB | ≥ 12 GiB available |
+| OLMoE-1B-7B F16 | `~/.models/gguf/allenai/OLMoE-1B-7B-0125-Instruct-GGUF/OLMoE-1B-7B-0125-Instruct-F16.gguf` (symlink → Downloads) | ~12.89 GiB | ≥ 18 GiB available |
+
</file context>
| # channel = stable always tracks the latest stable release. | ||
| # MSRV (Cargo.toml rust-version / CI msrv job) is the *minimum* supported version. | ||
| [toolchain] | ||
| channel = "stable" |
There was a problem hiding this comment.
P2: The newly added rust-toolchain.toml pins channel = "stable", but that file silently overrides the pinned toolchains used by the CI msrv job and the Docker build, so the MSRV gate no longer actually runs on 1.97.1. Rustup resolves the active toolchain in priority order, and a repo-local rust-toolchain.toml outranks the default toolchain that dtolnay/rust-toolchain sets. That means in .github/workflows/ci.yml's msrv job, cargo fmt/clippy/build/test will run on stable instead of the installed 1.97.1, and in the Dockerfile (which builds the rust:1.97.1-slim image) cargo will try to use a stable toolchain that isn't even present in the image — potentially failing the build or, in CI, silently skipping the real MSRV verification. Recommend forcing the pinned toolchain in both places (e.g. set RUSTUP_TOOLCHAIN=1.97.1 for the msrv job/container, or remove rust-toolchain.toml before cargo invocations) so the MSRV guarantee is actually exercised.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At rust-toolchain.toml, line 5:
<comment>The newly added `rust-toolchain.toml` pins `channel = "stable"`, but that file silently overrides the pinned toolchains used by the CI `msrv` job and the Docker build, so the MSRV gate no longer actually runs on 1.97.1. Rustup resolves the active toolchain in priority order, and a repo-local `rust-toolchain.toml` outranks the default toolchain that `dtolnay/rust-toolchain` sets. That means in `.github/workflows/ci.yml`'s `msrv` job, `cargo fmt/clippy/build/test` will run on stable instead of the installed 1.97.1, and in the `Dockerfile` (which builds the `rust:1.97.1-slim` image) cargo will try to use a `stable` toolchain that isn't even present in the image — potentially failing the build or, in CI, silently skipping the real MSRV verification. Recommend forcing the pinned toolchain in both places (e.g. set `RUSTUP_TOOLCHAIN=1.97.1` for the msrv job/container, or remove `rust-toolchain.toml` before `cargo` invocations) so the MSRV guarantee is actually exercised.</comment>
<file context>
@@ -0,0 +1,6 @@
+# channel = stable always tracks the latest stable release.
+# MSRV (Cargo.toml rust-version / CI msrv job) is the *minimum* supported version.
+[toolchain]
+channel = "stable"
+components = ["rustfmt", "clippy", "llvm-tools-preview"]
</file context>
| # Real GGUF pilots (xai-dissect style; not CI — needs weights on disk) | ||
| # Full-file load (no mmap): one ENGRAM_GGUF per process; free RAM ≥ file size + margin | ||
| ENGRAM_GGUF=~/.models/gguf/.../model.gguf cargo test --test real_gguf -- --ignored --nocapture | ||
| # Large MoE: ENGRAM_EXPECT_MOE=1 ENGRAM_MOE_SAMPLES=3 (see REVIEW.md T1 large MoE) |
There was a problem hiding this comment.
P3: The documented “Large MoE” invocation never sets these variables for the test that consumes them; the following example ignores both. Showing the variables on a real_gguf_moe test command would make the advertised hard-fail and sampling configuration effective.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 158:
<comment>The documented “Large MoE” invocation never sets these variables for the test that consumes them; the following example ignores both. Showing the variables on a `real_gguf_moe` test command would make the advertised hard-fail and sampling configuration effective.</comment>
<file context>
@@ -122,8 +151,18 @@ cargo test --all-features
+# Real GGUF pilots (xai-dissect style; not CI — needs weights on disk)
+# Full-file load (no mmap): one ENGRAM_GGUF per process; free RAM ≥ file size + margin
+ENGRAM_GGUF=~/.models/gguf/.../model.gguf cargo test --test real_gguf -- --ignored --nocapture
+# Large MoE: ENGRAM_EXPECT_MOE=1 ENGRAM_MOE_SAMPLES=3 (see REVIEW.md T1 large MoE)
+cargo run --example inspect_gguf -- ~/.models/gguf/.../model.gguf
</file context>
</details>
```suggestion
# Large MoE (see REVIEW.md T1 large MoE)
ENGRAM_GGUF=~/.models/gguf/.../model.gguf ENGRAM_EXPECT_MOE=1 ENGRAM_MOE_SAMPLES=3 \
cargo test --test real_gguf real_gguf_moe -- --ignored --nocapture
| match value { | ||
| KvValue::U32(v) => push_kv_u32(&mut out, key, *v), | ||
| KvValue::Str(v) => push_kv_string(&mut out, key, v), | ||
| KvValue::F32(v) => push_kv_f32(&mut out, key, *v), |
There was a problem hiding this comment.
P3: The f32 metadata support added in this PR isn't actually covered: no KvValue::F32 is placed in the test's kv map, so the F32 arm / push_kv_f32 never run and float32(key) is only checked for a missing key returning None. Add a real f32 KV (e.g. "qwen2moe.rope_freq_base") and assert float32 returns Some() so the positive capture path (and the survived #[allow(dead_code)] variant) are actually exercised.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/gguf_smoke.rs, line 343:
<comment>The f32 metadata support added in this PR isn't actually covered: no KvValue::F32 is placed in the test's kv map, so the F32 arm / push_kv_f32 never run and float32(key) is only checked for a missing key returning None. Add a real f32 KV (e.g. "qwen2moe.rope_freq_base") and assert float32 returns Some(<value>) so the positive capture path (and the survived #[allow(dead_code)] variant) are actually exercised.</comment>
<file context>
@@ -296,3 +306,435 @@ fn expert_out_of_range() {
+ match value {
+ KvValue::U32(v) => push_kv_u32(&mut out, key, *v),
+ KvValue::Str(v) => push_kv_string(&mut out, key, v),
+ KvValue::F32(v) => push_kv_f32(&mut out, key, *v),
+ }
+ }
</file context>
|
|
||
| # correct: subcommand after cargo (same as CI) | ||
| cd ~/Limen-Neural/engram-parser | ||
| cargo llvm-cov --all-targets --all-features --locked --lcov --output-path lcov.info |
There was a problem hiding this comment.
P3: Running the documented local coverage command creates an unignored lcov.info, conflicting with the clean-tree workflow and allowing accidental commits. Consider ignoring this generated file or writing/removing it outside the checkout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At REVIEW.md, line 143:
<comment>Running the documented local coverage command creates an unignored `lcov.info`, conflicting with the clean-tree workflow and allowing accidental commits. Consider ignoring this generated file or writing/removing it outside the checkout.</comment>
<file context>
@@ -0,0 +1,373 @@
+
+# correct: subcommand after cargo (same as CI)
+cd ~/Limen-Neural/engram-parser
+cargo llvm-cov --all-targets --all-features --locked --lcov --output-path lcov.info
+
+# human-readable summary only (no lcov file)
</file context>
Rephrase CHANGELOG/README/REVIEW and crate docs so ggml_type codes mean labels + packed byte sizes for GGUF, not dequant or a ggml runtime.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
REVIEW.md (1)
20-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the quality-gate command independent of the checkout path.
Line 27 hard-codes
~/Limen-Neural/engram-parser. The command fails when the repository is checked out elsewhere. Resolve the repository root dynamically or instruct users to run the command from the current repository root.Proposed documentation fix
- cd ~/Limen-Neural/engram-parser + cd "$(git rev-parse --show-toplevel)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@REVIEW.md` around lines 20 - 28, Update the “Full quality gate” instructions in REVIEW.md to remove the hard-coded checkout path in the cd command. Make the command work from any checkout by dynamically resolving the repository root, or replace it with an instruction to run from the current repository root while preserving the Cargo.toml prerequisite.src/gguf/tensor.rs (1)
349-389: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject non-row-aligned blocked tensor shapes.
read_tensor_entryvalidates only the total element count. AQ4_0tensor with shape[16, 2]therefore passes as one 18-byte block, although GGML requiresdims[0]to be divisible by the block size. Validatedims[0]before calculatingbyte_len.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gguf/tensor.rs` around lines 349 - 389, Update read_tensor_entry to validate the first dimension before calculating byte_len, rejecting blocked quantized tensors whose dims[0] is not divisible by their quantization block size. Preserve valid row-aligned shapes and ensure the validation occurs before byte_len_for_elements is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@REVIEW.md`:
- Around line 20-28: Update the “Full quality gate” instructions in REVIEW.md to
remove the hard-coded checkout path in the cd command. Make the command work
from any checkout by dynamically resolving the repository root, or replace it
with an instruction to run from the current repository root while preserving the
Cargo.toml prerequisite.
In `@src/gguf/tensor.rs`:
- Around line 349-389: Update read_tensor_entry to validate the first dimension
before calculating byte_len, rejecting blocked quantized tensors whose dims[0]
is not divisible by their quantization block size. Preserve valid row-aligned
shapes and ensure the validation occurs before byte_len_for_elements is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42d66fa5-9f0d-4c8a-a182-8f92125280f2
📒 Files selected for processing (5)
CHANGELOG.mdREADME.mdREVIEW.mdsrc/gguf/tensor.rssrc/lib.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6403b3284e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # channel = stable always tracks the latest stable release. | ||
| # MSRV (Cargo.toml rust-version / CI msrv job) is the *minimum* supported version. | ||
| [toolchain] | ||
| channel = "stable" |
There was a problem hiding this comment.
In the inspected .github/workflows/ci.yml MSRV job, the action installs 1.97.1 but lines 108–118 invoke unqualified cargo; this checked-in directory toolchain file makes those commands resolve stable instead of the action's default toolchain. This is reproducible with rustup show active-toolchain from the repository, while rustup override --help confirms that directory overrides control cargo invocations, so the job silently tests latest stable rather than the advertised MSRV. Export RUSTUP_TOOLCHAIN=1.97.1 for the job or invoke cargo +1.97.1 explicitly.
Useful? React with 👍 / 👎.
- Restore crate-root Result re-export (accidental 0.2 break) - Reject negative signed GGUF numerics used as layout values - Validate quant block alignment on innermost dim (dims[0]) - Derive quantization() from live file_type map (no stale cache) - Sort pilot GGUF candidates before ENGRAM_GGUF_MAX cap - Force RUSTUP_TOOLCHAIN=1.97.1 on CI msrv job despite stable override - Ignore lcov.info; label load_ms; DRY DType::label; README K-quants
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gguf/cursor.rs`:
- Around line 167-192: Update capture_numeric_kv and the signed read helpers
such as read_i8_as_u64, read_i16_as_u64, read_i32_as_u64, and read_i64_as_u64 so
negative signed metadata is preserved or ignored without error instead of being
converted through nonneg_i64_as_u64. Restrict non-negative validation to layout
fields such as general.alignment, retain the negative-alignment regression test,
and add coverage using a negative vendor-key fixture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76b352e3-ea83-4107-8a17-f51cc95daf14
📒 Files selected for processing (11)
.github/workflows/ci.yml.gitignoreREADME.mdREVIEW.mdexamples/inspect_gguf.rssrc/gguf/cursor.rssrc/gguf/layout.rssrc/gguf/tensor.rssrc/lib.rstests/gguf_smoke.rstests/real_gguf.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7120d77120
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| u64::try_from(v).map_err(|_| { | ||
| invalid_layout( | ||
| path, | ||
| format!("signed GGUF numeric value {v} is negative; expected non-negative"), |
There was a problem hiding this comment.
When a valid GGUF contains any negative signed scalar KV, capture_kv routes it through read_numeric_as_u64, and this helper now rejects the entire checkpoint. GGUF metadata is extensible and its signed value types are not restricted to counts or alignment, so a custom field such as an INT32 value of -1 must not make load_gguf fail; apply the non-negative check only to fields such as general.alignment, and preserve or skip other negative signed metadata.
Useful? React with 👍 / 👎.
| pub fn has_known_byte_layout(self) -> bool { | ||
| !matches!(self, Self::Other(_)) | ||
| } |
There was a problem hiding this comment.
Replacing this portion of the public DType API removes is_float() and element_size(), so downstream users of 0.1 stop compiling after upgrading. has_known_byte_layout() is not an equivalent replacement—it returns true for integer and blocked-quant types and provides no element width—and the changelog announces other API changes but not these removals; retain the two compatibility methods unless this source break is intentional and documented.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| /// (GGUF enum): `0 → "F32"`, `1 → "F16"`, otherwise `"GGUF(n)"`. | ||
| /// Returns `"unknown"` when neither is present. Derived at call time | ||
| /// so `Default` + public map edits stay consistent. | ||
| pub fn quantization(&self) -> String { |
There was a problem hiding this comment.
WARNING: Breaking API change — quantization() now returns String instead of &str
The return type was changed from &str to String to support the dynamic
general.file_type fallback. This breaks any downstream code that
pattern-matches on the return value (e.g., match metadata.quantization() { "F32" => ... }), which no longer compiles because String does not
match &str patterns in match arms. Consider returning Cow<'_, str>
to preserve backward compatibility while still allowing the fallback to
allocate.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| fn collect_gguf(dir: &Path, depth: usize, max_depth: usize, out: &mut Vec<PathBuf>) { | ||
| if depth > max_depth { |
There was a problem hiding this comment.
WARNING: collect_gguf no longer caps during directory walk
The refactor removed the max_files early-termination guard. The function
now always traverses the entire directory tree up to max_depth=6 before
sorting and truncating. For large model directories with thousands of
.gguf files, this causes unbounded memory growth and wasted I/O compared
to the old behavior, which stopped walking once max files were found.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| assert_eq!(ggml_type_label(21), "IQ3_S"); | ||
| assert_eq!(ggml_type_label(30), "BF16"); | ||
| assert_eq!(ggml_type_label(31), "Q4_0_4_4"); | ||
| assert_ne!(ggml_type_label(31), "IQ3_M"); |
There was a problem hiding this comment.
SUGGESTION: Redundant assert_ne! after assert_eq!
Line 452 (assert_ne!(ggml_type_label(31), "IQ3_M")) is redundant
because line 451 already asserts assert_eq!(ggml_type_label(31), "Q4_0_4_4").
If the positive assertion passes, the negative one is guaranteed to pass.
Remove the redundant check to strengthen the test.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 7120d77)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7120d77)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Reviewed by laguna-s-2.1:free · Input: 109K · Output: 15K · Cached: 28.5K |
…DType helpers Negative signed rejection applies only to general.alignment (vendor signed KVs accepted). Restore DType::is_float / element_size for 0.1 API surface. Document quantization() String return in CHANGELOG.
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
codeant-ai
Bot
added
size:XXL
There was a problem hiding this comment.
Gates Failed
(2 new files with code health below 10.00)
(2 files with Low Cohesion, Bumpy Road Ahead, Deep, Nested Complexity)
(4 files with Large Assertion Blocks, Complex Method, Overall Code Complexity)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| New code is healthy | Violations | Code Health Impact | |
|---|---|---|---|
| real_gguf.rs | 4 rules | 8.34 | Suppress |
| inspect_gguf.rs | 1 rule | 9.41 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| gguf_smoke.rs | 1 critical rule | 10.00 → 7.55 | Suppress |
| real_gguf.rs | 2 critical rules | 8.34 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| gguf_smoke.rs | 1 advisory rule | 10.00 → 7.55 | Suppress |
| real_gguf.rs | 2 advisory rules | 8.34 | Suppress |
| tensor.rs | 1 advisory rule | 9.39 → 8.28 | Suppress |
| inspect_gguf.rs | 1 advisory rule | 9.41 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| const VT_STRING: u32 = 8; | ||
|
|
||
| // Dtypes. | ||
| // Dtypes (GGUF wire type ids). |
There was a problem hiding this comment.
❌ New issue: Low Cohesion
This module has at least 6 different responsibilities amongst its 29 functions, threshold = 3
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gguf/cursor.rs`:
- Around line 188-221: Update read_nonneg_layout_usize so GGUF_VALUE_TYPE_BOOL
uses the one-byte reader, matching read_numeric_as_u64, while
GGUF_VALUE_TYPE_UINT64 continues using read_u64. Preserve the existing handling
for all other layout value types and return conversion behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46ecd6b7-252d-4393-85a9-fe707d9432a3
📒 Files selected for processing (5)
CHANGELOG.mdsrc/gguf/cursor.rssrc/gguf/layout.rssrc/gguf/tensor.rstests/gguf_smoke.rs
| /// Read a non-negative layout value (e.g. `general.alignment`). | ||
| /// | ||
| /// Rejects signed negatives so they do not wrap into huge alignments. | ||
| /// Other signed KV pairs should use [`Self::read_numeric_as_u64`] instead. | ||
| pub(crate) fn read_nonneg_layout_usize(&mut self, value_type: u32) -> Result<usize> { | ||
| let v = match value_type { | ||
| GGUF_VALUE_TYPE_UINT8 => self.read_u8()? as u64, | ||
| GGUF_VALUE_TYPE_UINT16 => self.read_u16()? as u64, | ||
| GGUF_VALUE_TYPE_UINT32 => self.read_u32()? as u64, | ||
| GGUF_VALUE_TYPE_UINT64 | GGUF_VALUE_TYPE_BOOL => self.read_u64()?, | ||
| GGUF_VALUE_TYPE_INT8 => { | ||
| let s = self.read_u8()? as i8; | ||
| nonneg_signed(self.path, i64::from(s))? | ||
| } | ||
| GGUF_VALUE_TYPE_INT16 => { | ||
| let s = self.read_i16()?; | ||
| nonneg_signed(self.path, i64::from(s))? | ||
| } | ||
| GGUF_VALUE_TYPE_INT32 => { | ||
| let s = self.read_i32()?; | ||
| nonneg_signed(self.path, i64::from(s))? | ||
| } | ||
| GGUF_VALUE_TYPE_INT64 => { | ||
| let s = self.read_i64()?; | ||
| nonneg_signed(self.path, s)? | ||
| } | ||
| other => { | ||
| return Err(self.unsupported(format!( | ||
| "expected integer GGUF value for layout field, got type {other}" | ||
| ))); | ||
| } | ||
| }; | ||
| Ok(v as usize) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fix the BOOL byte-width mismatch in read_nonneg_layout_usize.
Line 197 groups GGUF_VALUE_TYPE_BOOL with GGUF_VALUE_TYPE_UINT64 and reads 8 bytes with self.read_u64()?. GGUF encodes BOOL as 1 byte, and this same file already treats it correctly elsewhere: read_numeric_as_u64 maps GGUF_VALUE_TYPE_BOOL to self.read_u8_as_u64() (1 byte).
If a GGUF file encodes general.alignment as BOOL, this reads 7 extra bytes that belong to the next KV pairs. The cursor then silently misaligns for all subsequent metadata parsing, since read_exact only checks total-length bounds, not per-value-type correctness. This is the only call site of this function (src/gguf/layout.rs, read_metadata_section, line 280), so the effect is a full-file metadata misparse for this rare but spec-legal input.
Split the BOOL arm to read 1 byte, consistent with read_numeric_as_u64.
🐛 Proposed fix for the BOOL byte-width mismatch
- GGUF_VALUE_TYPE_UINT64 | GGUF_VALUE_TYPE_BOOL => self.read_u64()?,
+ GGUF_VALUE_TYPE_UINT64 => self.read_u64()?,
+ GGUF_VALUE_TYPE_BOOL => self.read_u8()? as u64,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Read a non-negative layout value (e.g. `general.alignment`). | |
| /// | |
| /// Rejects signed negatives so they do not wrap into huge alignments. | |
| /// Other signed KV pairs should use [`Self::read_numeric_as_u64`] instead. | |
| pub(crate) fn read_nonneg_layout_usize(&mut self, value_type: u32) -> Result<usize> { | |
| let v = match value_type { | |
| GGUF_VALUE_TYPE_UINT8 => self.read_u8()? as u64, | |
| GGUF_VALUE_TYPE_UINT16 => self.read_u16()? as u64, | |
| GGUF_VALUE_TYPE_UINT32 => self.read_u32()? as u64, | |
| GGUF_VALUE_TYPE_UINT64 | GGUF_VALUE_TYPE_BOOL => self.read_u64()?, | |
| GGUF_VALUE_TYPE_INT8 => { | |
| let s = self.read_u8()? as i8; | |
| nonneg_signed(self.path, i64::from(s))? | |
| } | |
| GGUF_VALUE_TYPE_INT16 => { | |
| let s = self.read_i16()?; | |
| nonneg_signed(self.path, i64::from(s))? | |
| } | |
| GGUF_VALUE_TYPE_INT32 => { | |
| let s = self.read_i32()?; | |
| nonneg_signed(self.path, i64::from(s))? | |
| } | |
| GGUF_VALUE_TYPE_INT64 => { | |
| let s = self.read_i64()?; | |
| nonneg_signed(self.path, s)? | |
| } | |
| other => { | |
| return Err(self.unsupported(format!( | |
| "expected integer GGUF value for layout field, got type {other}" | |
| ))); | |
| } | |
| }; | |
| Ok(v as usize) | |
| } | |
| /// Read a non-negative layout value (e.g. `general.alignment`). | |
| /// | |
| /// Rejects signed negatives so they do not wrap into huge alignments. | |
| /// Other signed KV pairs should use [`Self::read_numeric_as_u64`] instead. | |
| pub(crate) fn read_nonneg_layout_usize(&mut self, value_type: u32) -> Result<usize> { | |
| let v = match value_type { | |
| GGUF_VALUE_TYPE_UINT8 => self.read_u8()? as u64, | |
| GGUF_VALUE_TYPE_UINT16 => self.read_u16()? as u64, | |
| GGUF_VALUE_TYPE_UINT32 => self.read_u32()? as u64, | |
| GGUF_VALUE_TYPE_UINT64 => self.read_u64()?, | |
| GGUF_VALUE_TYPE_BOOL => self.read_u8()? as u64, | |
| GGUF_VALUE_TYPE_INT8 => { | |
| let s = self.read_u8()? as i8; | |
| nonneg_signed(self.path, i64::from(s))? | |
| } | |
| GGUF_VALUE_TYPE_INT16 => { | |
| let s = self.read_i16()?; | |
| nonneg_signed(self.path, i64::from(s))? | |
| } | |
| GGUF_VALUE_TYPE_INT32 => { | |
| let s = self.read_i32()?; | |
| nonneg_signed(self.path, i64::from(s))? | |
| } | |
| GGUF_VALUE_TYPE_INT64 => { | |
| let s = self.read_i64()?; | |
| nonneg_signed(self.path, s)? | |
| } | |
| other => { | |
| return Err(self.unsupported(format!( | |
| "expected integer GGUF value for layout field, got type {other}" | |
| ))); | |
| } | |
| }; | |
| Ok(v as usize) | |
| } |
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 192-192: src/gguf/cursor.rs#L192
Method read_nonneg_layout_usize has a cyclomatic complexity of 14 (limit is 8)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gguf/cursor.rs` around lines 188 - 221, Update read_nonneg_layout_usize
so GGUF_VALUE_TYPE_BOOL uses the one-byte reader, matching read_numeric_as_u64,
while GGUF_VALUE_TYPE_UINT64 continues using read_u64. Preserve the existing
handling for all other layout value types and return conversion behavior.
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/gguf/cursor.rs">
<violation number="1" location="src/gguf/cursor.rs:197">
P1: A boolean-typed `general.alignment` consumes eight bytes instead of its one-byte GGUF payload, desynchronizing subsequent KV/tensor parsing. Decode `GGUF_VALUE_TYPE_BOOL` with `read_u8()` as the other cursor paths do.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| GGUF_VALUE_TYPE_UINT8 => self.read_u8()? as u64, | ||
| GGUF_VALUE_TYPE_UINT16 => self.read_u16()? as u64, | ||
| GGUF_VALUE_TYPE_UINT32 => self.read_u32()? as u64, | ||
| GGUF_VALUE_TYPE_UINT64 | GGUF_VALUE_TYPE_BOOL => self.read_u64()?, |
There was a problem hiding this comment.
P1: A boolean-typed general.alignment consumes eight bytes instead of its one-byte GGUF payload, desynchronizing subsequent KV/tensor parsing. Decode GGUF_VALUE_TYPE_BOOL with read_u8() as the other cursor paths do.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/gguf/cursor.rs, line 197:
<comment>A boolean-typed `general.alignment` consumes eight bytes instead of its one-byte GGUF payload, desynchronizing subsequent KV/tensor parsing. Decode `GGUF_VALUE_TYPE_BOOL` with `read_u8()` as the other cursor paths do.</comment>
<file context>
@@ -175,9 +185,39 @@ impl<'a> GgufCursor<'a> {
+ GGUF_VALUE_TYPE_UINT8 => self.read_u8()? as u64,
+ GGUF_VALUE_TYPE_UINT16 => self.read_u16()? as u64,
+ GGUF_VALUE_TYPE_UINT32 => self.read_u32()? as u64,
+ GGUF_VALUE_TYPE_UINT64 | GGUF_VALUE_TYPE_BOOL => self.read_u64()?,
+ GGUF_VALUE_TYPE_INT8 => {
+ let s = self.read_u8()? as i8;
</file context>
| GGUF_VALUE_TYPE_UINT64 | GGUF_VALUE_TYPE_BOOL => self.read_u64()?, | |
| GGUF_VALUE_TYPE_UINT64 => self.read_u64()?, | |
| GGUF_VALUE_TYPE_BOOL => self.read_u8()? as u64, |
1 participant
User description
Summary
ggml_typecodes): labels + packedbyte_lenonly — not GGML dequant, kernels, or ggml runtime.Cargo.toml, CImsrv, Dockerfile);rust-toolchain.tomlon stable.tests/real_gguf.rs,examples/inspect_gguf.rs) withENGRAM_EXPECT_MOE/ENGRAM_MOE_SAMPLES.REVIEW.md(T0/T1/T2; large MoE RAM budget).Charter (what this PR is not)
Test plan / results
T0 (CI-equivalent, always-on)
cargo fmt --checkcargo clippy --all-targets --all-features -- -D warningscargo build --all-featurescargo test --all-featuresResult (2026-08-02): all green — lib units +
tests/gguf_smoke.rs(19) +real_gguf_helpers_document_env+ doctests; ignored T1 not run in T0. Package engram-parser v0.2.0.T1 real GGUF pilots (local only — not CI)
Full-file
load_gguf(no mmap). OneENGRAM_GGUFper process.~/.models/gguf/Abiray/ZAYA1-8B-GGUF/ZAYA1-8B-Q8_0.ggufzaya/ Q8_0complete=falsepartial roles)~/.models/gguf/allenai/…/OLMoE-…-F16.gguf(symlink → Downloads)olmoe/ F16complete=true, stacked)Logs (host only):
/tmp/engram-t1-zaya-test.log,/tmp/engram-t1-olmoe-test.log.Peak RSS for T1 ≈ 2× file size when inventory + MoE tests share one process.
Milestone
0.2.0 — ship of canonical GGUF + MSRV 1.97.1
Agent
Grok Build: Grok 4.5 (high)
CodeAnt-AI Description
Expand GGUF v3 parsing and MoE model inspection
What Changed
Q4_0_4_4and rejects it when its payload size cannot be determined.Impact
✅ Supports more GGUF tensor formats✅ Safer handling of malformed model files✅ Clearer GGUF and MoE model inventories💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.