Context
Bundles the 5 deferred MEDIUM/LOW findings from the Agent R review of Spanker PR #15 (which merged the bindgen integration + 5 review nits + 10 new tests). None merge-blocking but worth a focused cleanup.
Tasks
MEDIUM 1 — Use checked_mul in test allocator helper
src/backends/ggml/tests/mock_matmul.rs alloc_operands uses bare * arithmetic on usize. The production helpers (expected_a_bytes, expected_b_bytes, expected_out_bytes) correctly use checked_mul. The test helper signals the wrong idiom for a future contributor copying it.
Fix: replace bare * with checked_mul().unwrap(), OR call the production helpers directly.
MEDIUM 2 — Re-export OUTPUT_ELEM_BYTES for test usage
OUTPUT_ELEM_BYTES is declared as a local const in the integration test file (4 = sizeof::()). Today it duplicates the crate-level value with no enforcement that they match.
Fix: re-export OUTPUT_ELEM_BYTES as pub (the crate-level is already pub(crate)), import it in the integration test. Or use std::mem::size_of::<f32>() inline + comment.
MEDIUM 3 — matmul_q4_k_returns_not_implemented silent skip
The test silently returns when /dev/null open fails, emitting only eprintln!. A skipped test that produces no visible failure is indistinguishable from a passing test in CI summaries.
Fix: refactor so the stub test doesn't need a file-system handle (the stub ignores ctl entirely). Pass a mock SpankerControl if API allows, or document the skip path explicitly with cargo test ignored marker.
LOW 4 — Document bindgen.layout_tests(false)
build.rs calls .layout_tests(false) which suppresses bindgen's compile-time size/align assertions. Safe today (only struct spanker_version is bound and its layout is manually asserted in the cross-mirror test) but the rationale is undocumented.
Fix: one-line comment in build.rs explaining: layout assertions would require the test runner to link against libclang; the mirror test in lib.rs covers size_of::<ffi::spanker_version>() instead.
LOW 5 — Pin bindgen patch version
Cargo.toml declares bindgen = "0.69" (allows 0.69.x). Cargo.lock pins 0.69.5. Bindgen patch releases have historically changed generated-code layout.
Fix: pin to =0.69.5 OR document the current approach (# Cargo.lock is the source of truth for bindgen patch version).
Acceptance
- All 5 changes in one PR
- Tests still 42/42 pass
cargo clippy --workspace --all-targets -- -D warnings clean
cargo fmt --check --all clean
Refs
Authored by Agent R (Reviewer).
Context
Bundles the 5 deferred MEDIUM/LOW findings from the Agent R review of Spanker PR #15 (which merged the bindgen integration + 5 review nits + 10 new tests). None merge-blocking but worth a focused cleanup.
Tasks
MEDIUM 1 — Use
checked_mulin test allocator helpersrc/backends/ggml/tests/mock_matmul.rsalloc_operandsuses bare*arithmetic onusize. The production helpers (expected_a_bytes,expected_b_bytes,expected_out_bytes) correctly usechecked_mul. The test helper signals the wrong idiom for a future contributor copying it.Fix: replace bare
*withchecked_mul().unwrap(), OR call the production helpers directly.MEDIUM 2 — Re-export
OUTPUT_ELEM_BYTESfor test usageOUTPUT_ELEM_BYTESis declared as a localconstin the integration test file (4 = sizeof::()). Today it duplicates the crate-level value with no enforcement that they match.Fix: re-export
OUTPUT_ELEM_BYTESaspub(the crate-level is alreadypub(crate)), import it in the integration test. Or usestd::mem::size_of::<f32>()inline + comment.MEDIUM 3 —
matmul_q4_k_returns_not_implementedsilent skipThe test silently
returns when/dev/nullopen fails, emitting onlyeprintln!. A skipped test that produces no visible failure is indistinguishable from a passing test in CI summaries.Fix: refactor so the stub test doesn't need a file-system handle (the stub ignores
ctlentirely). Pass a mockSpankerControlif API allows, or document the skip path explicitly withcargo testignored marker.LOW 4 — Document
bindgen.layout_tests(false)build.rscalls.layout_tests(false)which suppresses bindgen's compile-time size/align assertions. Safe today (onlystruct spanker_versionis bound and its layout is manually asserted in the cross-mirror test) but the rationale is undocumented.Fix: one-line comment in
build.rsexplaining: layout assertions would require the test runner to link against libclang; the mirror test inlib.rscoverssize_of::<ffi::spanker_version>()instead.LOW 5 — Pin bindgen patch version
Cargo.tomldeclaresbindgen = "0.69"(allows 0.69.x).Cargo.lockpins 0.69.5. Bindgen patch releases have historically changed generated-code layout.Fix: pin to
=0.69.5OR document the current approach (# Cargo.lock is the source of truth for bindgen patch version).Acceptance
cargo clippy --workspace --all-targets -- -D warningscleancargo fmt --check --allcleanRefs
Authored by Agent R (Reviewer).