feat: GGUF v3 parser, IQ wire layouts, and T1 large MoE pilots (#7) - #44
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
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 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 API and validation
Inspection and real-GGUF pilots
Release and repository configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant inspect_gguf
participant load_gguf
participant GgufMetadata
participant ExpertExtraction
inspect_gguf->>load_gguf: load GGUF path
load_gguf->>GgufMetadata: read metadata and tensor inventory
inspect_gguf->>ExpertExtraction: extract first expert
ExpertExtraction-->>inspect_gguf: return expert tensor details
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Linear, Sentry Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
There was a problem hiding this comment.
Fixed: signed layout values such as general.alignment are now read with read_nonneg_layout_usize, which rejects negatives. Other signed KVs are preserved in the signed_numerics map.
| // 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 fixThere was a problem hiding this comment.
Fixed: parse_layout now calls validate_blocked_inner_dim, which requires dims[0] to be divisible by the dtype block size before accepting a quantized tensor.
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 👍 / 👎.
There was a problem hiding this comment.
Fixed: src/lib.rs re-exports Result at the crate root: pub use error::{ParserError, Result};.
| 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 👍 / 👎.
There was a problem hiding this comment.
Fixed: pilot_gguf_paths collects directory entries, sorts them, then truncates to the requested maximum.
| 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 👍 / 👎.
There was a problem hiding this comment.
Fixed: GgufMetadata::quantization() now reads the current general.file_type value from signed_numerics/numerics at call time, so manual map edits stay consistent.
| 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 👍 / 👎.
There was a problem hiding this comment.
Fixed: GgufMetadata::quantization() now reads the current general.file_type value from signed_numerics/numerics at call time, so manual map edits stay consistent.
There was a problem hiding this comment.
Correction: the inner-dimension validation is handled by validate_blocked_inner_dim in src/gguf/layout.rs, which checks tensor.dims[0] % DType::quant_block_size() == 0 before accepting any blocked-quant tensor layout.
![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
| 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
There was a problem hiding this comment.
Fixed: REVIEW.md heading structure was cleaned up and the markdownlint issue was resolved.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| // 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.
There was a problem hiding this comment.
Fixed: Result is re-exported from src/lib.rs as pub use error::{ParserError, Result};.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
![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>
There was a problem hiding this comment.
This Cubic finding was addressed in the latest commit; the Cubic AI reviewer check now passes.
| | 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>
There was a problem hiding this comment.
This Cubic finding was addressed in the latest commit; the Cubic AI reviewer check now passes.
| # 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>
There was a problem hiding this comment.
This Cubic finding was addressed in the latest commit; the Cubic AI reviewer check now passes.
| # 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
There was a problem hiding this comment.
This Cubic finding was addressed in the latest commit; the Cubic AI reviewer check now passes.
|
|
||
| # 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>
There was a problem hiding this comment.
This Cubic finding was addressed in the latest commit; the Cubic AI reviewer check now passes.
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 👍 / 👎.
There was a problem hiding this comment.
Fixed: rust-toolchain.toml stays on stable for local dev, and the CI msrv job installs and verifies 1.97.1 before running the gate.
- 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
- Simplify inspect_gguf resolve_path to preserve non-UTF-8 paths. - Validate extracted expert tensor bytes with byte_len_for_elements for all modeled dtypes, including Q/IQ layouts. - Add f32 KV round-trip coverage and remove redundant assert_ne. - Allow dead_code in shared test fixtures module. Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
- Set RUSTUP_TOOLCHAIN in Dockerfile and verify rustc version before build so rust-toolchain.toml's stable channel does not override MSRV. - Tighten MSRV grep in CI to require a trailing space, excluding future 1.97.10-style versions. - Fix markdownlint blanks-around-tables warnings and a host-specific path in REVIEW.md. Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Mirror real GGUF layout by padding each tensor's payload start offset to ALIGNMENT (32) in the test fixture, and clarify has_known_byte_layout docs. Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
- Make real_gguf_helpers_document_env set ENGRAM_MOE_SAMPLES and ENGRAM_GGUF to verify moe_sample_count/pilot_gguf_paths behavior. - Handle --help/-h/--version/-V and unknown flags in inspect_gguf. - Remove duplicate .beads/ entry from .gitignore. Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| pub fn numeric(&self, key: &str) -> Option<usize> { | ||
| self.numerics.get(key).map(|&v| v as usize) | ||
| let &v = self.numerics.get(key)?; | ||
| if v > i64::MAX as u64 { |
There was a problem hiding this comment.
WARNING: The new i64::MAX cutoff rejects valid unsigned UINT64 metadata on 64-bit targets.
numerics stores the original GGUF type only as a u64, so a value such as u64::MAX is indistinguishable here from a bit-preserved negative signed value. usize::try_from would accept that value on 64-bit, but this guard returns None, breaking numeric and arch_numeric for legitimate large unsigned metadata. Preserve the signedness/type or apply the negative check before erasing it.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed: GgufMetadata::numeric() checks signed_numerics for signed types and falls back to numerics with usize::try_from, so large valid UINT64 metadata is not rejected.
|
|
||
| // SAFETY: env mutation is isolated to this single-threaded test process. | ||
| unsafe { | ||
| env::set_var(ENV_MOE_SAMPLES, "7"); |
There was a problem hiding this comment.
WARNING: This test mutates the process-global environment without serializing the other tests.
The comment's "single-threaded" assumption is not enforced: #[test] functions run concurrently, and cargo test --include-ignored runs this helper alongside the two ignored pilot tests. Those tests can observe the temporary ENGRAM_GGUF and ENGRAM_MOE_SAMPLES values, while concurrent set_var/remove_var calls are exactly the process-global environment race that these unsafe APIs require callers to avoid. Use a process-wide lock, serialize the test process, or avoid mutating the process environment.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed: real_gguf_helpers_document_env no longer mutates the process environment; it calls the testable *_from helpers.
|
|
||
| // pilot_gguf_paths resolves a single ENGRAM_GGUF file. | ||
| let tmp = env::temp_dir().join(format!("engram_helpers_test_{}.gguf", process::id())); | ||
| fs::File::create(&tmp).expect("create temp file"); |
There was a problem hiding this comment.
WARNING: File::create can truncate an unrelated stale temporary file.
The filename is predictable and keyed only by the process ID, so a file left by an earlier failed run or created by another process can already exist under temp_dir(). File::create then truncates it before the assertion. Use a unique temporary directory/name and exclusive creation such as create_new, with cleanup guarded against failures.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed: temp files are created with create_new(true) and RAII guards clean them up.
| unsafe { | ||
| env::set_var(ENV_MOE_SAMPLES, "7"); | ||
| } | ||
| assert_eq!(moe_sample_count(), 7); |
There was a problem hiding this comment.
SUGGESTION: The replacement no longer tests expect_moe()'s default behavior.
The previous helper asserted that expect_moe() is false when ENGRAM_EXPECT_MOE is unset, but this version only exercises moe_sample_count and pilot_gguf_paths. A regression in the flag parser can therefore pass the always-on contract test unnoticed. Add a controlled unset/set assertion for expect_moe() while restoring the environment afterward.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed: the default behavior of expect_moe() is covered by expect_moe_from(None) in the refactored test.
| FROM rust:${RUST_VERSION}-slim | ||
|
|
||
| ARG RUST_VERSION | ||
| ENV RUSTUP_TOOLCHAIN=${RUST_VERSION} |
There was a problem hiding this comment.
WARNING: Persisting RUSTUP_TOOLCHAIN in the image changes downstream toolchain resolution.
This Dockerfile documents the image as a base for downstream crates, but ENV remains in the final image and overrides a descendant project's rust-toolchain.toml. Any downstream build using this image can be silently forced onto RUST_VERSION=1.97.1 instead of its declared compiler. Scope the variable to the verification RUN commands or explicitly unset it before exposing the base image.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed: RUSTUP_TOOLCHAIN is scoped to the build RUN only, so it does not leak into downstream image layers.
| if let Some(p) = args.next() { | ||
| if let Some(s) = p.to_str() { | ||
| if s == "--help" || s == "-h" || s == "--version" || s == "-V" { | ||
| return Err("help requested".into()); |
There was a problem hiding this comment.
SUGGESTION: The newly recognized version flags do not implement version output.
--version and -V return the same help requested error as --help; main then prints usage and exits with status 2, so callers receive neither a version nor a successful flag response. Either implement the expected help/version output and success status, or do not claim to handle the version flags.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed: inspect_gguf now supports --version / -V and prints the crate version from CARGO_PKG_VERSION.
| if s == "--help" || s == "-h" || s == "--version" || s == "-V" { | ||
| return Err("help requested".into()); | ||
| } | ||
| if s.starts_with('-') { |
There was a problem hiding this comment.
SUGGESTION: This new option check rejects valid model paths beginning with -.
A relative file such as -model.gguf was previously accepted as a positional path, but it is now rejected as an unknown option and there is no documented -- terminator or other escape path. Preserve arbitrary filesystem paths while handling flags, for example by supporting -- before the positional path.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed: resolve_path now supports -- so model paths starting with - can be passed after the terminator.
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- Track signed integer metadata in a separate map so numeric() rejects negatives without dropping valid large UINT64 values. - Replace recursive skip_value with an explicit stack, removing the hard 16-level nesting cap on valid metadata arrays. Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
Add *_from test helpers and test expect_moe, moe_sample_count, and pilot_gguf_paths against explicit inputs. Use unique temp paths with exclusive creation and RAII cleanup. Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
…olchain - Return Resolved::Help/Version and exit 0; support -- terminator for paths starting with -. - Scope RUSTUP_TOOLCHAIN to the build RUN in the Dockerfile so it does not leak into downstream images. Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Prevent negative signed file_type values from rendering as huge unsigned GGUF labels; add unit test coverage. Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Dockerfile`:
- Around line 34-38: Update the runtime Cargo command near the Dockerfile’s
final CMD to execute tests through the pinned ${RUST_VERSION} toolchain, using
rustup run rather than plain cargo; alternatively remove the command if the
image is intended to be build-only.
In `@tests/gguf_smoke.rs`:
- Line 238: Run rustfmt on the assertion involving
layout.metadata.float32("qwen2moe.rope_freq_base") so the long expression
follows standard formatting, and verify cargo fmt --check passes.
🪄 Autofix
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 Plus
Run ID: 2a31fd53-1fcb-4c02-aec2-9f0ba030b504
📒 Files selected for processing (11)
.github/workflows/ci.yml.gitignoreDockerfileREVIEW.mdexamples/inspect_gguf.rssrc/gguf/cursor.rssrc/gguf/layout.rssrc/gguf/tensor.rstests/common/mod.rstests/gguf_smoke.rstests/real_gguf.rs
💤 Files with no reviewable changes (1)
- .gitignore
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Build Docker Image (CPU-only)
- GitHub Check: qodana
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (5)
**/{Cargo.toml,Dockerfile,.github/workflows/*.yml}
📄 CodeRabbit inference engine (REVIEW.md)
When raising the Rust version floor, update Cargo.toml rust-version, the CI MSRV job, and the Docker Rust version together while keeping validate on stable.
Files:
Dockerfile.github/workflows/ci.yml
**/*.{rs,toml}
📄 CodeRabbit inference engine (REVIEW.md)
**/*.{rs,toml}: Keep engram-parser pure Rust and zero-dependency; never add myelin-accelerator or CUDA as a dependency, including optional dependencies.
Code must remain compatible with the declared MSRV of Rust 1.97.1 and the stable toolchain specified by rust-toolchain.toml.
Format Rust code with rustfmt and ensure cargo fmt --check passes.
Ensure cargo clippy --all-targets --all-features -- -D warnings passes without warnings.
GGUF on-wire ggml_type codes are metadata labels and packed-size information only; do not interpret them as a request for dequantization or compute.
Do not add benches or Criterion targets, and do not use cargo bench as this crate’s quality gate.
Files:
tests/common/mod.rsexamples/inspect_gguf.rstests/gguf_smoke.rssrc/gguf/cursor.rstests/real_gguf.rssrc/gguf/tensor.rssrc/gguf/layout.rs
**/*.rs
📄 CodeRabbit inference engine (REVIEW.md)
Limit engram-parser to GGUF v3 parsing, inventory, and raw MoE expert-byte extraction. Do not add CUDA, dequantization, mmap loading, or GGML computation.
Files:
tests/common/mod.rsexamples/inspect_gguf.rstests/gguf_smoke.rssrc/gguf/cursor.rstests/real_gguf.rssrc/gguf/tensor.rssrc/gguf/layout.rs
**/.github/workflows/*.yml
📄 CodeRabbit inference engine (REVIEW.md)
Keep CI aligned with the local quality gate: stable validate runs formatting, Clippy with warnings denied, build, tests, clean-tree checks, and coverage; the MSRV job uses Rust 1.97.1.
Files:
.github/workflows/ci.yml
tests/real_gguf.rs
📄 CodeRabbit inference engine (REVIEW.md)
Real-GGUF pilot tests are ignored, CPU-only, and path-gated through ENGRAM_GGUF or ENGRAM_MODEL_DIR; they must validate successful loading, non-empty tensors, in-range tensor bytes, and non-empty MoE projections when applicable.
Files:
tests/real_gguf.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: rmems/engram-parser
Timestamp: 2026-08-07T14:15:40.081Z
Learning: Keep production CUDA kernels/FFI in myelin-accelerator and GPU experiments or real-model pipelines in blackwell-kernel-lab; engram-parser must not depend on either repository.
Learnt from: CR
Repo: rmems/engram-parser
Timestamp: 2026-08-07T14:15:40.081Z
Learning: After the quality gate, the Git working tree must be clean; do not commit lcov.info, large GGUF weights, GPU profile dumps, or other generated artifacts.
🔇 Additional comments (15)
tests/real_gguf.rs (2)
126-152: Bound the directory walk before collecting candidates.
collect_ggufvisits and stores every matching file below depth 6 before it appliesENGRAM_GGUF_MAX. A large model directory can cause excessive filesystem work and allocation even when the selected result count is small. Preserve deterministic selection, but bound the traversal.
27-30: LGTM!Also applies to: 43-88, 90-92, 292-371
examples/inspect_gguf.rs (2)
24-72: LGTM!
169-199: LGTM!src/gguf/tensor.rs (1)
155-155: LGTM!Also applies to: 223-405, 573-609, 807-815
tests/gguf_smoke.rs (1)
223-226: LGTM!src/gguf/cursor.rs (2)
27-62: LGTM!Also applies to: 175-220, 260-377
226-237: 🗄️ Data Integrity & IntegrationNo change needed for
usizeconversion.The crate’s MSRV of Rust 1.97.1 targets 64-bit
usize; this conversion does not truncate a value such as4_294_967_328 + 32into32.> Likely an incorrect or invalid review comment.src/gguf/layout.rs (1)
13-155: LGTM!Also applies to: 242-242, 290-308, 462-464, 519-558
tests/common/mod.rs (1)
3-3: LGTM!Also applies to: 59-107
REVIEW.md (3)
136-136: LGTM!
216-216: LGTM!
353-353: LGTM!Dockerfile (1)
22-22: LGTM!.github/workflows/ci.yml (1)
85-91: LGTM!Also applies to: 98-109, 112-116
…olchain Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
…an_one_pilot Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
There was a problem hiding this comment.
1 issue found across 3 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="tests/real_gguf.rs">
<violation number="1" location="tests/real_gguf.rs:165">
P3: The default scan cap changed from 8 to 1 here, but REVIEW.md (the T1 pilot contract doc) still states the default is 8. Update REVIEW.md so the documented default matches the code, otherwise developers running tree scans will expect up to 8 files and silently get 1.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| return Vec::new(); | ||
| } | ||
|
|
||
| let max = max.and_then(|s| s.parse::<usize>().ok()).unwrap_or(1); |
There was a problem hiding this comment.
P3: The default scan cap changed from 8 to 1 here, but REVIEW.md (the T1 pilot contract doc) still states the default is 8. Update REVIEW.md so the documented default matches the code, otherwise developers running tree scans will expect up to 8 files and silently get 1.
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 165:
<comment>The default scan cap changed from 8 to 1 here, but REVIEW.md (the T1 pilot contract doc) still states the default is 8. Update REVIEW.md so the documented default matches the code, otherwise developers running tree scans will expect up to 8 files and silently get 1.</comment>
<file context>
@@ -141,12 +162,13 @@ fn pilot_gguf_paths_from(
}
- let max = max.and_then(|s| s.parse::<usize>().ok()).unwrap_or(8);
+ let max = max.and_then(|s| s.parse::<usize>().ok()).unwrap_or(1);
- // Collect the full candidate set first, then sort and cap — `read_dir`
</file context>
There was a problem hiding this comment.
This Cubic finding was addressed in the latest commit; the Cubic AI reviewer check now passes.
…W.md default cap Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
There was a problem hiding this comment.
1 issue found across 3 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="Dockerfile">
<violation number="1">
P2: The runtime test command no longer pins the toolchain. Previously `rustup run ${RUST_VERSION} cargo test` guaranteed the MSRV 1.97.1 toolchain at container runtime; the new exec-form `cargo` relies on the final image's default toolchain and any `rust-toolchain.toml` copied in by `COPY . .`. Since REVIEW.md pins `rust-toolchain.toml` to `stable` while the base image installs only 1.97.1, `docker run --rm engram-parser` may resolve to or attempt to download the stable channel at runtime rather than verifying on the MSRV — which can fail in offline/CI contexts. If the intent is to keep runtime verification on 1.97.1, restore an explicit pin (e.g. keep `ENV RUST_VERSION` and use `rustup run ${RUST_VERSION} cargo test ...`); otherwise ensure `rust-toolchain.toml` is excluded from the build context.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -15,10 +15,12 @@ | |||
| # | |||
There was a problem hiding this comment.
P2: The runtime test command no longer pins the toolchain. Previously rustup run ${RUST_VERSION} cargo test guaranteed the MSRV 1.97.1 toolchain at container runtime; the new exec-form cargo relies on the final image's default toolchain and any rust-toolchain.toml copied in by COPY . .. Since REVIEW.md pins rust-toolchain.toml to stable while the base image installs only 1.97.1, docker run --rm engram-parser may resolve to or attempt to download the stable channel at runtime rather than verifying on the MSRV — which can fail in offline/CI contexts. If the intent is to keep runtime verification on 1.97.1, restore an explicit pin (e.g. keep ENV RUST_VERSION and use rustup run ${RUST_VERSION} cargo test ...); otherwise ensure rust-toolchain.toml is excluded from the build context.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Dockerfile, line 45:
<comment>The runtime test command no longer pins the toolchain. Previously `rustup run ${RUST_VERSION} cargo test` guaranteed the MSRV 1.97.1 toolchain at container runtime; the new exec-form `cargo` relies on the final image's default toolchain and any `rust-toolchain.toml` copied in by `COPY . .`. Since REVIEW.md pins `rust-toolchain.toml` to `stable` while the base image installs only 1.97.1, `docker run --rm engram-parser` may resolve to or attempt to download the stable channel at runtime rather than verifying on the MSRV — which can fail in offline/CI contexts. If the intent is to keep runtime verification on 1.97.1, restore an explicit pin (e.g. keep `ENV RUST_VERSION` and use `rustup run ${RUST_VERSION} cargo test ...`); otherwise ensure `rust-toolchain.toml` is excluded from the build context.</comment>
<file context>
@@ -43,4 +42,4 @@ RUN chown -R appuser:appuser /app
USER appuser
-CMD rustup run ${RUST_VERSION} cargo test --release --all-features
+CMD ["cargo", "test", "--release", "--all-features"]
</file context>
There was a problem hiding this comment.
This Cubic finding was addressed in the latest commit; the Cubic AI reviewer check now passes.
…_numeric, fix md style Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
…agged by CodeScene Co-Authored-By: Raul Montoya Cardenas <montoyaraul34@gmail.com>
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 parsing coverage and provide clearer model and MoE inspection
What Changed
Q4_0_4_4and rejects it when its byte layout cannot be verified, instead of treating it asIQ3_M.general.file_typequantization fallback.Impact
✅ Wider GGUF model compatibility✅ Safer tensor bounds and malformed-file handling✅ Clearer MoE model inventory and extraction diagnostics💡 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.