Add resumable skippy quantize workflows - #900
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds skippy-quantize and llama-quant-ffi, removes direct-return TCP wiring from prompt/bench/runtime clients, and extends native MTP correctness and benchmark flows with ABI, report, and orchestration changes. Changesskippy-quantize and llama-quant-ffi: new quantization toolchain
Direct-return TCP server removal across prompt, bench, and correctness clients
Native MTP N1 correctness testing and verify-span-local benchmark
skippy-model-package artifact hook fix and SPD project handoff document
Possibly related issues
Possibly related PRs
Suggested reviewers
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (23)
crates/skippy-correctness/src/native_mtp_openai/remote.rs-65-69 (1)
65-69: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHarden SSH/SCP invocations to avoid orchestration hangs.
Line 65 and the helpers at Line 208-242 run remote commands without non-interactive/timeout SSH options. In unattended runs this can block indefinitely on auth/host-key prompts or long network stalls.
Suggested fix
- let output = Command::new("ssh") - .arg(host) + let output = Command::new("ssh") + .args(["-o", "BatchMode=yes", "-o", "ConnectTimeout=10"]) + .arg(host) .arg(command) .output() .with_context(|| format!("start remote stage 1 on {host}"))?; fn ssh_success(host: &str, remote_command: &str) -> Result<()> { let status = Command::new("ssh") + .args(["-o", "BatchMode=yes", "-o", "ConnectTimeout=10"]) .arg(host) .arg(remote_command) .status() .with_context(|| format!("run ssh command on {host}"))?; fn scp_to(host: &str, local_path: &Path, remote_path: &str) -> Result<()> { let status = Command::new("scp") + .args(["-o", "BatchMode=yes", "-o", "ConnectTimeout=10"]) .arg(local_path) .arg(format!("{host}:{remote_path}")) .status() .with_context(|| format!("copy {} to {host}:{remote_path}", local_path.display()))?; fn scp_from(host: &str, remote_path: &str, local_path: &Path) -> Result<()> { let status = Command::new("scp") + .args(["-o", "BatchMode=yes", "-o", "ConnectTimeout=10"]) .arg(format!("{host}:{remote_path}")) .arg(local_path) .status() .with_context(|| format!("copy {host}:{remote_path} to {}", local_path.display()))?;Also applies to: 208-242
🤖 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 `@crates/skippy-correctness/src/native_mtp_openai/remote.rs` around lines 65 - 69, The SSH command invocation in the remote stage execution needs to be hardened with non-interactive and timeout options to prevent indefinite blocking on authentication prompts or network stalls. Add SSH arguments like -o BatchMode=yes for non-interactive mode and -o ConnectTimeout with an appropriate timeout value to the Command::new("ssh") invocation at line 65, and apply the same hardening to all SSH/SCP command invocations in the helper functions located at lines 208-242 to ensure consistent behavior across all remote command execution paths.crates/skippy-correctness/src/native_mtp_openai/mod.rs-65-70 (1)
65-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire non-loopback stage1 networking when
--stage1-ssh-hostis used.Line 65-70 validates remote mode flags, but it doesn’t validate addresses. With current defaults (
stage1_bind_addr=127.0.0.1:19172,stage1_endpoint_addrunset), enabling--stage1-ssh-hostmakes stage0 target loopback and remote stage1 is unreachable by default.Suggested fix
if args.stage1_ssh_host.is_some() && args.stage1_remote_stage_server_bin.is_none() { bail!("--stage1-remote-stage-server-bin is required with --stage1-ssh-host"); } + if args.stage1_ssh_host.is_some() { + let stage1_bind = args.stage1_bind_addr; + let stage1_endpoint = args.stage1_endpoint_addr.unwrap_or(stage1_bind); + if stage1_bind.ip().is_loopback() || stage1_endpoint.ip().is_loopback() { + bail!( + "--stage1-bind-addr and --stage1-endpoint-addr must be non-loopback when --stage1-ssh-host is set" + ); + } + }🤖 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 `@crates/skippy-correctness/src/native_mtp_openai/mod.rs` around lines 65 - 70, When --stage1-ssh-host is specified, the validation logic needs to check that the networking addresses are configured for remote access. Add a validation check in the section after line 70 (following the existing args.stage1_ssh_host validations) to ensure that when args.stage1_ssh_host is some value, the stage1_bind_addr must not be a loopback address (127.0.0.1) and stage1_endpoint_addr must be set to a non-loopback value. This prevents the incompatible default configuration where stage0 would target loopback while attempting to reach a remote stage1 server.crates/skippy-correctness/src/runner.rs-2956-2962 (1)
2956-2962: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreflight selects non-GGUF
stage_modelfiles and can fail valid runs.Line 2956 currently picks
runtime.stage_modelwhenever it is a file. In artifact-slice usage,--stage-modelcan be a manifest file, so Lines 2964-2969 then try GGUF metadata/tensor inspection on a non-GGUF path and fail before execution. Gate this path to GGUF files (or resolve to an actual GGUF) before callingnative_mtp_artifact_summary.💡 Suggested fix
fn native_mtp_preflight_model_path(runtime: &RuntimeArgs) -> &Path { runtime .stage_model .as_deref() - .filter(|path| path.is_file()) + .filter(|path| { + path.is_file() + && path + .extension() + .and_then(|ext| ext.to_str()) + .is_some_and(|ext| ext.eq_ignore_ascii_case("gguf")) + }) .unwrap_or(runtime.model.as_path()) }Also applies to: 2964-2969
🤖 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 `@crates/skippy-correctness/src/runner.rs` around lines 2956 - 2962, The `native_mtp_preflight_model_path` function currently accepts any file in `runtime.stage_model` without validating it is actually a GGUF file. This causes downstream GGUF metadata inspection to fail when a manifest file is passed as `--stage-model`. Modify the filter condition in `native_mtp_preflight_model_path` to check that the selected path is specifically a GGUF file (not just any file) before returning it. If `stage_model` is not a GGUF file, it should fall back to `runtime.model.as_path()` instead. This ensures only valid GGUF paths are passed to the code at lines 2964-2969 that performs tensor inspection.crates/skippy-correctness/src/runner.rs-957-998 (1)
957-998: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftExtract the duplicated decode/verification flow into a shared helper.
The staged decode + optional second-step verification logic is duplicated across Line 957+ and Line 1276+. Consolidating this will reduce drift and keep these orchestration functions within maintainable complexity boundaries.
As per coding guidelines: "
**/*.rs: Do not add Rust methods or functions over the configured Clippy line-count limit...andDo not add Rust code over the configured cognitive-complexity limit. Prefer small, named decision helpers...`"Also applies to: 1276-1317
🤖 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 `@crates/skippy-correctness/src/runner.rs` around lines 957 - 998, Extract the duplicated staged decode and verification flow into a shared helper function in the runner.rs file. Identify the common pattern used in both the initial decode block (around line 957) and the second verification block (around line 1276) that involves calling binary_decode_message with specific BinaryDecodeMessageArgs, write_stage_message, recv_reply, and ensure_reply_kind functions. Create a helper function that accepts parameters like the stream, session, wire_dtype, token_id, decode_step, activation_width, request_id, and session_id, and returns the predicted token and optional timing information. Replace both duplicated code sections with calls to this new helper function, passing the appropriate parameters for each context (decode_step 0 for the first call, decode_step 1 for the verification call).Source: Coding guidelines
crates/skippy-quantize/scripts/compare-reference-quantization.py-65-97 (1)
65-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail fast when the invocation would run zero checks.
Right now the script can exit 0 after writing a report even when neither conversion nor quantization actually ran (for example, missing both
--checkpointand--quant-input, or--checkpointwithout a converter and no quant input). Add argument validation so “no-op success” cannot happen.Suggested fix
def parse_args() -> argparse.Namespace: parser = argparse.ArgumentParser() @@ - return parser.parse_args() + args = parser.parse_args() + if args.checkpoint is None and args.quant_input is None: + parser.error("provide --checkpoint and/or --quant-input") + if ( + args.checkpoint is not None + and args.python_converter is None + and args.quant_input is None + ): + parser.error( + "--checkpoint requires --python-converter when --quant-input is not provided" + ) + return args🤖 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 `@crates/skippy-quantize/scripts/compare-reference-quantization.py` around lines 65 - 97, The main function can exit successfully without performing any actual work when neither conversion nor quantization runs. Add argument validation immediately after parse_args() in the main function to ensure at least one meaningful check would execute. Validate that either args.checkpoint is not None or args.quant_input is not None, and raise an error or print a usage message if neither is provided. This validation should fail fast before any work is attempted, preventing the script from writing an empty report and returning success when no actual checks were performed..agents/skills/hf-quant-and-layer-package-jobs/SKILL.md-42-75 (1)
42-75: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--max-memoryin the llama-api examples will fail at runtime.Line 45 and Line 73 pair
--backend llama-apiwith--max-memory 32G, but the native llama quantize path rejects--max-memory(crates/skippy-quantize/src/native_quantize.rs, Line 159). These documented commands currently fail before execution.Suggested doc correction
target/release/skippy-quantize run-quant \ --manifest /tmp/skippy-quantize.json \ --backend llama-api \ - --max-memory 32G \ --work-dir /tmp/skippy-quantize-work \ --spool-dir /tmp/skippy-quantize-output \ --record-dir /tmp/skippy-quantize-records \ --json-event-file /tmp/skippy-quantize-status.json \ --json-event-interval-seconds 120 \ --json-event-window 8 @@ target/release/skippy-quantize quant-job \ --source /mnt/bf16 \ --source-prefix BF16 \ --target /mnt/quant \ --target-prefix <quant-selector> \ --output-basename <model>-<quant-selector> \ --quant <quant-selector> \ --tensor-type-file /mnt/recipe/tensor-types.txt \ --window-size 1 \ --manifest /tmp/skippy-quantize.json \ --backend llama-api \ - --max-memory 32G \ --dry-run🤖 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 @.agents/skills/hf-quant-and-layer-package-jobs/SKILL.md around lines 42 - 75, The documentation contains two command examples (in the run-quant command block and the quant-job command block) that pair `--backend llama-api` with `--max-memory 32G`, but the llama quantize backend explicitly rejects the `--max-memory` flag, causing these documented commands to fail at runtime. Remove the `--max-memory 32G` parameter from both the skippy-quantize run-quant command and the skippy-quantize quant-job command that use `--backend llama-api` to make the documented examples executable.crates/skippy-quantize/src/plan_convert.rs-43-50 (1)
43-50: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate
--stream-buffer-bytesbefore entering streaming paths.Line 47/58/78 forward
stream_buffer_bytesdirectly, but this command never rejects0. Add the same invariant used by other convert entrypoints to prevent invalid streaming behavior.💡 Proposed fix
-use anyhow::Result; +use anyhow::{Result, ensure}; ... pub(crate) fn run_plan_convert(args: PlanConvertArgs) -> Result<()> { + ensure!( + args.stream_buffer_bytes > 0, + "--stream-buffer-bytes must be greater than zero" + ); let mut plan = inspect_hf_checkpoint(&args.source, args.max_memory, args.staging_fraction)?;Also applies to: 58-59, 78-79
🤖 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 `@crates/skippy-quantize/src/plan_convert.rs` around lines 43 - 50, The run_plan_convert function forwards stream_buffer_bytes to verify_hf_checkpoint_tensor_streams without validating that it is a valid non-zero value, which can lead to invalid streaming behavior. Add validation logic to check that args.stream_buffer_bytes is greater than zero before entering the streaming path (before the condition checking args.verify_streaming). This validation should reject zero or invalid values similar to how other convert entrypoints handle this parameter to prevent invalid streaming operations on lines 49-51 and any other locations where stream_buffer_bytes is forwarded to streaming functions.crates/skippy-quantize/src/main.rs-679-687 (1)
679-687: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
quantize-layer-packagesilently drops quantization options that change output artifacts.This path accepts
QuantRunnerArgsbut the generated hook command ignores output-affecting flags (for example--imatrix,--include-weights/--exclude-weights,--tensor-type,--output-tensor-type,--token-embedding-type,--prune-layers,--override-kv). That can produce different quantized artifacts than requested with no error.💡 Minimal safe fix (reject unsupported flags until fully forwarded)
fn quantize_layer_package(args: QuantizeLayerPackageArgs) -> Result<()> { @@ ensure!( !args.runner.print_only, "quantize-layer-package does not support --print-only; use quant-job --preflight-only first" ); + ensure!( + args.runner.imatrix.is_none() + && args.runner.include_weights.is_empty() + && args.runner.exclude_weights.is_empty() + && args.runner.output_tensor_type.is_none() + && args.runner.token_embedding_type.is_none() + && args.runner.tensor_type.is_empty() + && args.runner.prune_layers.is_none() + && args.runner.override_kv.is_empty(), + "quantize-layer-package currently does not forward all quantization override flags; remove overrides or add forwarding in hook generation" + );Also applies to: 781-799
🤖 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 `@crates/skippy-quantize/src/main.rs` around lines 679 - 687, The quantize_layer_package function accepts QuantRunnerArgs but silently ignores output-affecting quantization flags like imatrix, include-weights, exclude-weights, tensor-type, output-tensor-type, token-embedding-type, prune-layers, and override-kv, which can produce different artifacts than requested without error. Add ensure! checks (similar to the existing window_size and dry_run validations) to validate that these unsupported flags are not being passed, and reject them with descriptive error messages. Apply the same validation logic to all locations where this issue occurs, including the other instance referenced at lines 781-799.crates/skippy-quantize/src/artifacts.rs-99-117 (1)
99-117: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDon’t treat same-size target shards as equivalent content.
The resume shortcut currently trusts size equality only. A stale or wrong shard with identical length is accepted and silently propagated.
Suggested fix
fn publish_file(source: &Path, target: &Path) -> Result<()> { @@ if target.exists() { let target_len = target @@ ensure!( source_len == target_len, "target shard already exists with different size: {} source_bytes={} target_bytes={}", target.display(), source_len, target_len ); + ensure!( + files_equal(source, target)?, + "target shard already exists with same size but different content: {}", + target.display() + ); print_info(format!( "Publish target already exists: {} ({})", target.display(), format_bytes(source_len) )); return Ok(()); } @@ } + +fn files_equal(a: &Path, b: &Path) -> Result<bool> { + const BUF: usize = 1024 * 1024; + let mut fa = fs::File::open(a).with_context(|| format!("open {}", a.display()))?; + let mut fb = fs::File::open(b).with_context(|| format!("open {}", b.display()))?; + let mut ba = vec![0_u8; BUF]; + let mut bb = vec![0_u8; BUF]; + loop { + let na = std::io::Read::read(&mut fa, &mut ba).with_context(|| format!("read {}", a.display()))?; + let nb = std::io::Read::read(&mut fb, &mut bb).with_context(|| format!("read {}", b.display()))?; + if na != nb { + return Ok(false); + } + if na == 0 { + return Ok(true); + } + if ba[..na] != bb[..nb] { + return Ok(false); + } + } +}🤖 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 `@crates/skippy-quantize/src/artifacts.rs` around lines 99 - 117, The resume shortcut logic in the target shard validation currently only compares file sizes between source and target, which is insufficient because two files with identical length may have different content. Instead of relying solely on the `source_len == target_len` equality check, implement content verification by computing and comparing checksums or hashes of both the source and target files. This ensures that stale or corrupted shards with the same byte length are not silently accepted as equivalent content.crates/skippy-quantize/src/output.rs-289-339 (1)
289-339: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid temp-file collisions between periodic and manual snapshot writes.
write_nowand the background writer can callwrite_snapshotconcurrently, but both use the same temp filename. That introduces rename races and intermittent write errors.Suggested fix
fn write_snapshot(path: &Path, state: &Arc<Mutex<JsonEventState>>) -> Result<()> { @@ - let temp = path.with_extension(format!( - "{}.tmp", - path.extension() - .and_then(|extension| extension.to_str()) - .unwrap_or("json") - )); + let temp = path.with_extension(format!( + "{}.{}.{}.tmp", + path.extension() + .and_then(|extension| extension.to_str()) + .unwrap_or("json"), + std::process::id(), + unix_timestamp_ms() + ));🤖 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 `@crates/skippy-quantize/src/output.rs` around lines 289 - 339, The write_snapshot function creates a temp file with a fixed naming pattern that doesn't account for concurrent calls. When both the periodic writer thread (from spawn_periodic_writer) and the write_now function call write_snapshot simultaneously using the same path, they both try to write to the same temp filename, causing a rename race condition. Modify the temp filename generation to include a unique identifier (such as a random component, UUID, or thread ID) so that concurrent calls to write_snapshot generate different temp filenames before atomically renaming them to the final path.crates/skippy-quantize/src/backend.rs-208-216 (1)
208-216: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate quant backends on actual runtime capability, not just enum kind.
This check currently accepts
llama-api/skippy-abieven when their runtime is unavailable, so invalid backend choices fail later instead of at preflight.Suggested fix
pub fn ensure_quant_backend(kind: BackendKind) -> Result<()> { - ensure!( - matches!(kind, BackendKind::LlamaApi | BackendKind::SkippyAbi), - "backend {} cannot quantize GGUFs yet: {}", - kind.as_str(), - capabilities(&[]).skippy_abi.reason - ); + let caps = capabilities(&[]); + match kind { + BackendKind::LlamaApi => ensure!( + caps.llama_api.llama_quantize, + "backend {} is unavailable: {}", + kind.as_str(), + caps.llama_api.reason + ), + BackendKind::SkippyAbi => ensure!( + caps.skippy_abi.llama_quantize, + "backend {} is unavailable: {}", + kind.as_str(), + caps.skippy_abi.reason + ), + BackendKind::NativeRust => ensure!( + false, + "backend {} cannot quantize GGUFs yet", + kind.as_str() + ), + } Ok(()) }🤖 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 `@crates/skippy-quantize/src/backend.rs` around lines 208 - 216, The ensure_quant_backend function currently only validates the enum kind against allowed types (BackendKind::LlamaApi and BackendKind::SkippyAbi) but does not check if these backends are actually available at runtime. Fix this by using the capabilities function to get the runtime capabilities for the provided backend kind and verify that the specific backend has the required capability enabled (not just checking the enum type with matches!). This ensures that backends pass validation only if they are both the correct enum type AND actually available at runtime, catching invalid backend choices at preflight instead of later in execution.crates/skippy-quantize/src/hf_checkpoint.rs-359-367 (1)
359-367: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winConstrain indexed shard paths to stay under the checkpoint root.
weight_mapentries are joined directly into filesystem paths, so absolute paths or..segments can escape the checkpoint directory and make this command read arbitrary local files.Suggested fix
fn discover_indexed_safetensors(source: &Path) -> Result<Vec<PathBuf>> { @@ - let mut files = index + let source_root = source + .canonicalize() + .with_context(|| format!("canonicalize {}", source.display()))?; + let mut files = index .weight_map .values() - .map(|name| source.join(name)) + .map(|name| { + let candidate = source.join(name); + let canonical = candidate + .canonicalize() + .with_context(|| format!("canonicalize {}", candidate.display()))?; + ensure!( + canonical.starts_with(&source_root), + "indexed shard path escapes checkpoint root: {}", + candidate.display() + ); + Ok(canonical) + }) + .collect::<Result<BTreeSet<_>>>()? - .collect::<BTreeSet<_>>() .into_iter() .collect::<Vec<_>>();🤖 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 `@crates/skippy-quantize/src/hf_checkpoint.rs` around lines 359 - 367, The code in this block joins weight_map entries directly into filesystem paths without validating that they remain within the checkpoint directory boundaries. This creates a security vulnerability where entries containing absolute paths or `..` segments can escape the checkpoint root directory. To fix this, validate each file name from weight_map.values() before joining it with source: check that each name is relative (not absolute) and does not contain path components like `..` that would escape the directory, then normalize the resulting joined path and verify it stays within the source directory using canonicalization or path comparison techniques before including it in the files collection.crates/skippy-quantize/src/imatrix.rs-142-166 (1)
142-166: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate length-prefixed fields before allocating.
The parser trusts file-provided
entry_count,name_len,value_count, dataset length, and GGUF string length before allocation. A corrupt imatrix can request hugeVecallocations beforeread_exactfails.Proposed direction
+fn checked_remaining_len(reader: &Cursor<Vec<u8>>, len: u64, what: &str) -> Result<usize> { + let len = usize::try_from(len).with_context(|| format!("{what} length does not fit usize"))?; + let position = usize::try_from(reader.position()).context("reader position does not fit usize")?; + let remaining = reader.get_ref().len().saturating_sub(position); + ensure!(len <= remaining, "{what} extends past end of imatrix file"); + Ok(len) +} + fn read_gguf_string(reader: &mut Cursor<Vec<u8>>) -> Result<String> { let len = read_u64(reader)?; - let mut bytes = vec![0_u8; len as usize]; + let len = checked_remaining_len(reader, len, "GGUF string")?; + let mut bytes = vec![0_u8; len]; reader.read_exact(&mut bytes)?; String::from_utf8(bytes).context("GGUF string is not UTF-8") }Apply the same check before allocating legacy names, values, and dataset bytes.
Also applies to: 186-192, 476-480
🤖 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 `@crates/skippy-quantize/src/imatrix.rs` around lines 142 - 166, Add validation checks for length-prefixed fields before allocating memory to prevent potential out-of-memory attacks from corrupted files. In the read_legacy_imatrix_entry function, add an ensure! check for name_len before allocating the name_bytes vector to verify it is within a reasonable maximum size. Similarly, validate value_count before allocating the values vector. Apply the same validation pattern in read_legacy_imatrix_trailer for the dataset length and any other length-prefixed allocations to ensure all file-provided lengths are checked for reasonableness before Vec allocation occurs.crates/skippy-quantize/src/gguf_template.rs-27-35 (1)
27-35: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMirror
mtp_num_hidden_layersin emitted MTP metadata.Line 28 treats
mtp_num_hidden_layersas a valid MTP declaration, but Lines 141-143 and 56-62 only count/emitnum_nextn_predict_layers. A config that only usesmtp_num_hidden_layerscan therefore select MTP tensors while writing a base-only{arch}.block_count, leavingblk.*tensor indices beyond the advertised metadata.Proposed fix
pub(crate) fn mtp_layer_start_from_hf_config(source: &Path) -> Result<Option<u32>> { let config = read_hf_config(source)?; - let Some(nextn_layers) = optional_u32(&config, "num_nextn_predict_layers") - .or_else(|| optional_u32(&config, "mtp_num_hidden_layers")) - else { + let Some(_nextn_layers) = optional_mtp_layer_count(&config) else { return Ok(None); }; - if nextn_layers == 0 { - return Ok(None); - } required_u32(&config, "num_hidden_layers").map(Some) } @@ if options.include_mtp { - push_if_u32( - &mut metadata, - arch, - "nextn_predict_layers", - &config, - "num_nextn_predict_layers", - ); + if let Some(nextn_layers) = optional_mtp_layer_count(&config) { + metadata.push(GgufKv::u32( + &format!("{arch}.nextn_predict_layers"), + nextn_layers, + )); + } } @@ let block_count = required_u32(config, "num_hidden_layers")? + if options.include_mtp { - optional_u32(config, "num_nextn_predict_layers").unwrap_or(0) + optional_mtp_layer_count(config).unwrap_or(0) } else { 0 }; @@ fn optional_u32(config: &Value, key: &str) -> Option<u32> { @@ } + +fn optional_mtp_layer_count(config: &Value) -> Option<u32> { + optional_u32(config, "num_nextn_predict_layers") + .or_else(|| optional_u32(config, "mtp_num_hidden_layers")) + .filter(|count| *count > 0) +}Also applies to: 55-63, 140-146
🤖 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 `@crates/skippy-quantize/src/gguf_template.rs` around lines 27 - 35, The metadata emission code in lines 56-62 and 140-146 only checks for `num_nextn_predict_layers` when writing the MTP block count, but the input parsing in the nextn_layers block accepts either `num_nextn_predict_layers` or `mtp_num_hidden_layers`. Mirror the same fallback logic from the nextn_layers variable assignment (checking `num_nextn_predict_layers` first, then falling back to `mtp_num_hidden_layers`) in both metadata emission sections so that configs using only `mtp_num_hidden_layers` will correctly emit the appropriate block count metadata.crates/skippy-quantize/src/gguf_writer.rs-317-329 (1)
317-329: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject duplicate GGUF tensor names after mapping.
Different HF names can normalize to the same GGUF name, such as prefixed/unprefixed shared tensors or duplicated shard entries. The writer currently emits both entries, producing an ambiguous GGUF tensor table.
Proposed fix
for group in expert_groups.into_values() { tensors.push(group.into_tensor_source()?); } tensors.sort_by(|a, b| a.name.cmp(&b.name)); + for pair in tensors.windows(2) { + ensure!( + pair[0].name != pair[1].name, + "duplicate GGUF tensor name {} after tensor name mapping", + pair[0].name + ); + } Ok(tensors) }🤖 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 `@crates/skippy-quantize/src/gguf_writer.rs` around lines 317 - 329, The current implementation collects tensors from safetensor files and expert groups into the tensors vector, then sorts by name, but does not check for duplicate GGUF tensor names that may result from different HF names mapping to the same GGUF name. After the tensors.sort_by call, add validation logic to detect and reject any duplicate tensor names by iterating through the sorted tensors vector and ensuring each entry has a unique name. If duplicates are found, return an error that identifies the conflicting names to aid debugging.crates/skippy-quantize/src/imatrix.rs-222-226 (1)
222-226: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject GGUF
chunk_countvalues that do not fit the FFI field.Line 225 casts
u32toi32, so values abovei32::MAXsilently become negative before being exposed throughNativeImatrix::chunk_count().Proposed fix
Ok(LoadedImatrix { entries, dataset: metadata.datasets.into_iter().next(), - chunk_count: metadata.chunk_count.unwrap_or(0) as i32, + chunk_count: metadata + .chunk_count + .map(i32::try_from) + .transpose() + .context("imatrix.chunk_count does not fit int32")? + .unwrap_or(0), })🤖 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 `@crates/skippy-quantize/src/imatrix.rs` around lines 222 - 226, The chunk_count field is casting a u32 value directly to i32 without validation, which causes silent overflow when the value exceeds i32::MAX. Before the cast in the LoadedImatrix construction, validate that metadata.chunk_count.unwrap_or(0) does not exceed i32::MAX, and if it does, return an appropriate error. Only proceed with the i32 cast if the validation passes.crates/skippy-quantize/src/imatrix.rs-419-424 (1)
419-424: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply include and exclude filters together.
When both filters are provided, Line 420 ignores
include_weightsand keeps every non-excluded tensor. That can feed unrelated imatrix entries into quantization.Proposed fix
fn include_exclude_match( name: &str, include_weights: &[String], exclude_weights: &[String], ) -> bool { - if !exclude_weights.is_empty() { - return !exclude_weights.iter().any(|filter| name.contains(filter)); - } - if !include_weights.is_empty() { - return include_weights.iter().any(|filter| name.contains(filter)); - } - true + let included = + include_weights.is_empty() || include_weights.iter().any(|filter| name.contains(filter)); + let excluded = exclude_weights.iter().any(|filter| name.contains(filter)); + included && !excluded }🤖 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 `@crates/skippy-quantize/src/imatrix.rs` around lines 419 - 424, The filter logic currently returns early when exclude_weights is not empty, preventing include_weights from being evaluated when both filters are provided. Fix this by restructuring the logic to first check if the name should be excluded (return false if it matches any exclude filter), then check if an include filter exists and return false if the name does not match any include filter, and finally return true if neither exclusion condition is met. This ensures both filters are applied together: a tensor is only included if it passes both the exclude check (not in exclude list) and the include check (in include list if one is provided).crates/llama-quant-ffi/build.rs-25-44 (1)
25-44: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not reuse native archives without validating the build inputs.
Line 94 returns as soon as archive files exist, so changes to native build inputs can keep linking stale artifacts, especially with a reused
LLAMA_STAGE_BUILD_DIR. The script context shows additional inputs such as native flags, CUDA architectures, ROCm targets, and VMM settings that are not all tracked here. Letscripts/build-llama.sh’s stamp decide freshness, or compare the same stamp inputs before returning.Proposed direction
fn print_rerun_envs() { for key in [ "LLAMA_STAGE_BUILD_DIR", "LLAMA_STAGE_LIB_DIR", "LLAMA_STAGE_LINK_MODE", @@ "LLVMInstallDir", "VULKAN_SDK", + "LLAMA_STAGE_GGML_NATIVE", + "SKIPPY_GGML_NATIVE", + "LLAMA_STAGE_CUDA_ARCHITECTURES", + "SKIPPY_CUDA_ARCHITECTURES", + "LLAMA_STAGE_AMDGPU_TARGETS", + "SKIPPY_AMDGPU_TARGETS", + "GGML_CUDA_NO_VMM", + "LLAMA_STAGE_USE_SCCACHE", + "SKIPPY_USE_SCCACHE", + "CMAKE_BUILD_TYPE", ] { println!("cargo:rerun-if-env-changed={key}"); } } @@ - if required_static_archives_exist(build_dir) { + if required_static_archives_exist(build_dir) && !native_auto_build_enabled() { return; }If always invoking the scripts when auto-build is enabled is too eager, replace the second hunk with an explicit build-stamp comparison using the same inputs as
scripts/build-llama.sh.Also applies to: 94-96
🤖 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 `@crates/llama-quant-ffi/build.rs` around lines 25 - 44, The print_rerun_envs function in build.rs is incomplete and does not track all necessary build inputs that affect native compilation. The build script at line 94 returns early when archive files exist without validating that all build inputs remain unchanged, allowing stale artifacts to be reused. Expand the print_rerun_envs function to include tracking for all build inputs mentioned in the build process such as native flags, CUDA architectures, ROCm targets, and VMM settings that scripts/build-llama.sh uses. Ensure these environment variables and configuration inputs are added to the array of keys being monitored with cargo:rerun-if-env-changed directives so that the build system will properly detect when native build inputs change and invalidate cached archives.crates/llama-quant-ffi/build.rs-139-149 (1)
139-149: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake native auto-build opt-in.
Line 148 makes plain Cargo builds clone/build patched llama.cpp when archives are missing. That can break offline builds and unexpectedly turn
cargo check/CI into a long native build; the existing panic text already describes enabling auto-build explicitly.Proposed fix
fn native_auto_build_enabled() -> bool { for key in ["SKIPPY_LLAMA_AUTO_BUILD", "MESH_LLM_AUTO_BUILD_LLAMA"] { if let Ok(value) = std::env::var(key) { return !matches!( value.to_ascii_lowercase().as_str(), "0" | "false" | "no" | "off" ); } } - true + false }🤖 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 `@crates/llama-quant-ffi/build.rs` around lines 139 - 149, The native_auto_build_enabled() function currently defaults to returning true when neither SKIPPY_LLAMA_AUTO_BUILD nor MESH_LLM_AUTO_BUILD_LLAMA environment variables are set, making auto-build opt-out by default. This causes unexpected long native builds during plain Cargo builds. Change the final return statement from true to false so that auto-build is opt-in instead of opt-out, requiring users to explicitly enable it via environment variables rather than having it enabled by default.Cargo.toml-50-57 (1)
50-57: 📐 Maintainability & Code Quality | 🟠 MajorAdd the new workspace crates to workflow filters, publish scripts, and xtask consistency checks.
The root workspace now includes
crates/skippy-quantizeandcrates/llama-quant-ffiinCargo.toml, and they are correctly listed inscripts/affected-crates.shandscripts/plan-clippy-batches.sh. However, per the coding guidelines for workspace crate additions, the following files still need to be updated in the same commit:
- Workflow filters in
.github/workflows/filesscripts/publish-crates.shtools/xtask/src/main.rsfor repo-consistency expectations🤖 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 `@Cargo.toml` around lines 50 - 57, The new workspace crates skippy-quantize and llama-quant-ffi have been added to Cargo.toml but are missing from other configuration files required for proper workspace management. Add these two new crate names to the workflow filters in all relevant .github/workflows files, include them in the scripts/publish-crates.sh script, and update tools/xtask/src/main.rs to include them in the repo-consistency expectations to ensure they are properly tracked across all build, test, and release processes.Source: Coding guidelines
crates/skippy-quantize/src/manifest.rs-59-65 (1)
59-65: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrite manifests atomically to protect resumability.
Directly writing the target path can leave a truncated/corrupt manifest if interrupted mid-write. For resumable jobs, this should use temp-file + atomic rename semantics.
🤖 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 `@crates/skippy-quantize/src/manifest.rs` around lines 59 - 65, The write_manifest function directly writes to the target path, which can result in a truncated or corrupted manifest if the write operation is interrupted. Instead, implement atomic writes by writing to a temporary file first and then atomically renaming it to the target path. Create a temporary file in the same directory as the target path (to ensure they are on the same filesystem for atomic rename operations), serialize and write the manifest to this temp file, then use atomic rename to move the temp file to the final path specified in the path parameter. This ensures that if an interruption occurs during write, the original manifest file remains intact and jobs can resume cleanly.crates/skippy-quantize/src/splits.rs-211-218 (1)
211-218: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnforce split-window bounds inside
stage_source_window.This path currently accepts invalid windows and proceeds (e.g.,
first_split = 0orlast_split > total), which can stage an unintended shard set instead of hard-failing.Proposed fix
pub fn stage_source_window( source: &Path, source_prefix: &str, first_source_shard: &Path, stage_path: &Path, window: SplitWindow, total: u32, ) -> Result<PathBuf> { + validate_split_window(window, total)?; remove_dir_if_exists(stage_path)?; let stage_root = prefixed_path(stage_path, source_prefix);Also applies to: 224-243
🤖 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 `@crates/skippy-quantize/src/splits.rs` around lines 211 - 218, Add validation for the split-window bounds at the beginning of the stage_source_window function to enforce that the window parameter contains valid bounds before processing. The validation should check that first_split is at least 1, last_split does not exceed the total parameter, and first_split is less than or equal to last_split. If any of these conditions are violated, the function should return an appropriate error instead of proceeding with staging the shard set.crates/skippy-quantize/src/locking.rs-70-73 (1)
70-73: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not silently disable manifest locking on non-Unix platforms.
Returning
Ok(())here permits concurrent writers with no exclusion, which can race manifest reads/writes. If cross-platform locking is not implemented yet, fail fast instead of giving a false safety signal.Safer interim behavior
#[cfg(not(unix))] fn lock_file(_file: &File) -> Result<()> { - Ok(()) + anyhow::bail!("manifest locking is not implemented on this platform") }🤖 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 `@crates/skippy-quantize/src/locking.rs` around lines 70 - 73, The lock_file function guarded by #[cfg(not(unix))] silently returns Ok(()) without actually implementing any file locking mechanism, which masks the lack of proper exclusion and allows concurrent writes to potentially race. Instead of returning Ok(()), make this function return an error to fail fast and prevent false safety assumptions until proper cross-platform locking is implemented. Replace the Ok(()) with an appropriate error result that clearly indicates locking is not available on this platform.
🟡 Minor comments (3)
crates/skippy-protocol/src/binary/codec.rs-50-68 (1)
50-68: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep scalar and sideband predictions consistent.
This exported helper can encode
predicted = 42withpredicted_tokens = []or[7, ...], producing a split-brainStageReplyafterrecv_reply. Preserve the old invariant by requiring the sideband to be non-empty and start withpredicted.Proposed fix
pub fn send_reply_predicted_with_tokens_and_stats( mut writer: impl Write, predicted: i32, predicted_tokens: &[i32], stats: StageReplyStats, ) -> io::Result<()> { + if predicted_tokens.is_empty() { + return Err(invalid_input("predicted token reply requires at least one token")); + } + if predicted_tokens[0] != predicted { + return Err(invalid_input( + "predicted token sideband must start with predicted token", + )); + } if predicted_tokens.len() > MAX_STAGE_PREDICTED_TOKENS { return Err(invalid_input("too many predicted tokens")); }🤖 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 `@crates/skippy-protocol/src/binary/codec.rs` around lines 50 - 68, The send_reply_predicted_with_tokens_and_stats function currently allows predicted_tokens to be empty or contain values inconsistent with the predicted parameter, which creates a split-brain state. Add validation after the MAX_STAGE_PREDICTED_TOKENS length check to ensure that predicted_tokens is non-empty and that its first element equals the predicted value, returning an invalid_input error with an appropriate message if either validation fails.crates/skippy-quantize/scripts/compare-reference-quantization.py-109-110 (1)
109-110: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
--nthreadsas a positive integer at parse time.Accepting arbitrary strings here turns input mistakes into downstream command failures and can be misclassified as tolerable matching failures.
Suggested fix
- parser.add_argument("--nthreads", default="8") + parser.add_argument("--nthreads", type=int, default=8) @@ - return parser.parse_args() + args = parser.parse_args() + if args.nthreads <= 0: + parser.error("--nthreads must be greater than zero") + return args🤖 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 `@crates/skippy-quantize/scripts/compare-reference-quantization.py` around lines 109 - 110, The --nthreads argument currently accepts any string value with a default of "8" without type validation, causing input errors to manifest as downstream failures instead of immediate validation errors. Modify the parser.add_argument call for --nthreads to include a type parameter that converts the input to an integer and a validation check (via choices or a custom type function) to ensure only positive integers are accepted, so invalid inputs are rejected at parse time rather than during execution.crates/skippy-quantize/src/native_quantize.rs-259-277 (1)
259-277: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve user tensor-pattern casing when building overrides.
Line 273 lowercases the pattern before FFI handoff. That can change regex meaning (
\S→\s,\W→\w) and silently apply overrides to the wrong tensors.Suggested fix
- patterns.push(CString::new(raw_pattern.to_ascii_lowercase())?); + patterns.push(CString::new(raw_pattern)?);🤖 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 `@crates/skippy-quantize/src/native_quantize.rs` around lines 259 - 277, The pattern is being converted to lowercase with to_ascii_lowercase() before being added to the patterns vector, which can change regex meaning and cause overrides to be silently applied to the wrong tensors. Remove the to_ascii_lowercase() call from the line where patterns.push(CString::new(...)) is invoked, and instead pass raw_pattern directly to preserve the user's original tensor-pattern casing.
🧹 Nitpick comments (3)
SPD_SKIPPY_PROJECT.md (2)
140-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce repetitive sentence openers in the limitations list.
Lines 140–142 open three consecutive bullets with "Skippy/Skippy" subject positions, which reduces readability. Consider restructuring to vary the subject or tense.
- Skippy/Rust does not execute the SPD head. - Skippy/Rust does not load tensor values into an executable SPD head yet. - Skippy does not yet expose live hidden-state taps for SPD. + The SPD head has not yet been executed by Skippy/Rust. + Tensor loading for executable SPD head support is not yet implemented. + Skippy does not yet expose live hidden-state taps for SPD.🤖 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 `@SPD_SKIPPY_PROJECT.md` around lines 140 - 142, The three consecutive limitation bullets in the SPD_SKIPPY_PROJECT.md file all begin with "Skippy/Rust" or "Skippy does", creating repetitive subject positions that hurt readability. Restructure these three bullets (the ones describing SPD head execution, tensor value loading, and hidden-state taps) by varying their sentence structure and subject positions. Consider combining related points, using passive voice alternatives, or restructuring the phrasing to eliminate the repetitive opener pattern while maintaining clarity about what Skippy currently cannot do.
217-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify artifact licensing and handoff status in the layout section.
The "Artifact Layout Target" section proposes an eventual package structure but doesn't explicitly state whether the trained SPD head
.safetensorsfile can be published or should remain internal. Milestone 5 (lines 395–401) mentions licensing constraints, but this guidance should be surfaced earlier here to prevent misunderstandings during reproduction.Consider adding a note like:
+ **Publishing guidance:** Trained SPD head `.safetensors` files may be subject to base model licensing (e.g., Qwen). Before publishing a trained head, confirm compatibility with the base model's license. Serving manifests and metrics may be published even if the trained weights cannot.🤖 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 `@SPD_SKIPPY_PROJECT.md` around lines 217 - 241, Add clarification to the "Artifact Layout Target" section about the licensing status and handoff constraints for the trained SPD head `.safetensors` file. Specifically, insert a note that explicitly states whether the `.safetensors` artifact can be published or should remain internal, and reference the licensing constraints mentioned in Milestone 5 to make this guidance visible earlier in the document and prevent misunderstandings during reproduction efforts.crates/skippy-quantize/src/main.rs (1)
1049-1201: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit the window-runner orchestration into phase helpers.
run_convert_window_once_with_manifestandrun_quant_window_once_with_manifesteach combine selection, planning, staging, execution, publishing, and cleanup in one block. Breaking these into named phase helpers will reduce cognitive load and make failure-path tests much easier to maintain.As per coding guidelines: "Do not add Rust methods or functions over the configured Clippy line-count limit. Split long logic into semantically named helpers..." and "Do not add Rust code over the configured cognitive-complexity limit."
Also applies to: 1235-1443
🤖 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 `@crates/skippy-quantize/src/main.rs` around lines 1049 - 1201, The function `run_convert_window_once_with_manifest` (and similarly `run_quant_window_once_with_manifest`) is too long and has high cognitive complexity, combining selection, planning, staging, execution, publishing, and cleanup in a single block. Extract each distinct phase into semantically named helper functions such as selecting the window, building the conversion plan, preparing the output directory, executing the conversion command, and publishing the results. This will reduce cognitive load, improve readability, and make testing failure paths much easier to maintain.Source: Coding guidelines
| let element_count = input.len() / source_dtype.byte_size() as usize; | ||
| let output_len = element_count | ||
| .checked_mul(target_dtype.byte_size() as usize) | ||
| .context("converted chunk byte length overflow")?; | ||
| let mut output = Vec::with_capacity(output_len); | ||
| for index in 0..element_count { | ||
| let value = read_float_element(input, source_dtype, index); | ||
| write_float_element(&mut output, target_dtype, value); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Reject misaligned chunks instead of silently dropping bytes.
element_count uses floor division, so non-aligned chunk sizes truncate trailing bytes and corrupt tensor conversion output.
Suggested fix
pub(crate) fn convert_float_chunk<W: Write>(
@@
) -> Result<u64> {
- let element_count = input.len() / source_dtype.byte_size() as usize;
+ let source_bytes = source_dtype.byte_size() as usize;
+ anyhow::ensure!(
+ input.len() % source_bytes == 0,
+ "chunk size {} is not aligned to source dtype width {}",
+ input.len(),
+ source_bytes
+ );
+ let element_count = input.len() / source_bytes;🤖 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 `@crates/skippy-quantize/src/float_convert.rs` around lines 72 - 80, The
calculation of element_count using integer division silently drops trailing
bytes that don't form a complete element when the input length is not evenly
divisible by the source data type size. Add a validation check after calculating
element_count to ensure input.len() is evenly divisible by
source_dtype.byte_size() as usize, and use .context() to return an error if
there are misaligned bytes instead of allowing them to be truncated.
|
Closing in favor of the clean quant-only PR: #901 |
1 participant
Summary
Validation
Summary by CodeRabbit
skippy-quantizeRust CLI for native GGUF conversion/quantization with spooling, validation, and status reporting.verify-span-localcommand for local inference/span verification reporting.