diff --git a/crates/apr-cli/src/commands/inspect.rs b/crates/apr-cli/src/commands/inspect.rs index f3dc223705..84f9fe92d3 100644 --- a/crates/apr-cli/src/commands/inspect.rs +++ b/crates/apr-cli/src/commands/inspect.rs @@ -215,6 +215,20 @@ pub(crate) fn run( /// GGUF/SafeTensors inspect via RosettaStone /// Print rosetta inspection report as JSON. fn output_rosetta_json(path: &Path, report: &aprender::format::rosetta::InspectionReport) { + if let Ok(json) = serde_json::to_string_pretty(&rosetta_json(path, report)) { + println!("{json}"); + } +} + +/// The `apr inspect --json` document for a GGUF/SafeTensors file. +/// +/// `metadata` is every key in the report, unfiltered: #3733's gate reads a +/// model's declared context and memory arithmetic from it, and a second GGUF +/// reader is refused (`check_no_hand_rolled_parsers.sh`). +fn rosetta_json( + path: &Path, + report: &aprender::format::rosetta::InspectionReport, +) -> serde_json::Value { let mut json_map = serde_json::Map::new(); json_map.insert( "file".to_string(), @@ -262,10 +276,7 @@ fn output_rosetta_json(path: &Path, report: &aprender::format::rosetta::Inspecti .collect::>() .into(); json_map.insert("metadata".to_string(), metadata); - - if let Ok(json) = serde_json::to_string_pretty(&json_map) { - println!("{json}"); - } + serde_json::Value::Object(json_map) } /// Print rosetta inspection report as rich text. diff --git a/crates/apr-cli/src/commands/inspect_tests.rs b/crates/apr-cli/src/commands/inspect_tests.rs index 8ad8204569..a2757cf464 100644 --- a/crates/apr-cli/src/commands/inspect_tests.rs +++ b/crates/apr-cli/src/commands/inspect_tests.rs @@ -5,6 +5,38 @@ mod inspect_tests { use super::*; use std::path::Path; + // ======================================================================== + // rosetta_json (#3733): --json shows every metadata key, unfiltered + // ======================================================================== + + #[test] + fn rosetta_json_emits_every_metadata_key() { + use aprender::format::rosetta::{FormatType, InspectionReport}; + let keys = [ + "general.architecture", + "qwen35.context_length", + "qwen35.full_attention_interval", + "quantize.imatrix.file", + "zzz_never_heard_of.depth", + "tokenizer.ggml.model", + ]; + let report = InspectionReport { + format: FormatType::Gguf, + file_size: 1, + metadata: keys.iter().map(|k| ((*k).to_string(), "v".to_string())).collect(), + tensors: vec![], + total_params: 0, + quantization: None, + architecture: Some("qwen35".to_string()), + }; + let v = rosetta_json(Path::new("m.gguf"), &report); + let shown = v["metadata"].as_object().expect("metadata object"); + assert_eq!(shown.len(), keys.len()); + for k in keys { + assert!(shown.contains_key(k), "--json dropped {k}"); + } + } + // ======================================================================== // validate_path // ======================================================================== diff --git a/crates/aprender-core/src/format/gguf/api.rs b/crates/aprender-core/src/format/gguf/api.rs index 490cd15ae3..eba45f1764 100644 --- a/crates/aprender-core/src/format/gguf/api.rs +++ b/crates/aprender-core/src/format/gguf/api.rs @@ -339,9 +339,11 @@ pub fn load_gguf_raw>(path: P) -> Result { // Contract: apr-inspect-metadata-propagation-v1 F-INSPECT-META-001 (paiml/aprender#622). // Propagate raw GGUF KV metadata to downstream consumers (inspect/rosetta) so they // can display authentic on-disk keys instead of fabricated ML-shorthand names. + // #3733: EVERY header key, including those outside the reader's parse allowlist. let raw_metadata: BTreeMap = reader .metadata .iter() + .chain(&reader.display_only_metadata) .map(|(k, v)| (k.clone(), gguf_value_display(v))) .collect(); diff --git a/crates/aprender-core/src/format/gguf/api_tests.rs b/crates/aprender-core/src/format/gguf/api_tests.rs index 069d46f1c2..7fdbcc11a6 100644 --- a/crates/aprender-core/src/format/gguf/api_tests.rs +++ b/crates/aprender-core/src/format/gguf/api_tests.rs @@ -449,3 +449,6 @@ fn test_gguf_model_config_clone() { #[path = "api_tests_raw_tensor_rope.rs"] mod api_tests_raw_tensor_rope; + +#[path = "api_tests_all_keys_3733.rs"] +mod api_tests_all_keys_3733; diff --git a/crates/aprender-core/src/format/gguf/api_tests_all_keys_3733.rs b/crates/aprender-core/src/format/gguf/api_tests_all_keys_3733.rs new file mode 100644 index 0000000000..6c7e1cf37b --- /dev/null +++ b/crates/aprender-core/src/format/gguf/api_tests_all_keys_3733.rs @@ -0,0 +1,149 @@ +//! #3733: `apr inspect --json` must show EVERY key the GGUF header carries. +//! +//! The reader's parse allowlist (`tokenizer.`, `general.`, six arch prefixes) +//! decided what inspect could show, so a Qwen3.5 file listed 24 of its 46 header +//! keys and none of the 18 `qwen35.*` ones: `qwen35.` does not start with +//! `qwen3.`. The fixture below uses that prefix, a prefix nothing has heard of, +//! and the `quantize.*` group real quantized files carry. + +use super::*; +use crate::format::gguf::GGUF_MAGIC; +use crate::format::rosetta::RosettaStone; + +/// Every key the fixture's header carries. The count assertions compare against +/// THIS list, which is exactly what `export_tensors_to_gguf` writes. +fn header() -> Vec<(String, GgufValue)> { + vec![ + ( + "general.architecture".into(), + GgufValue::String("qwen35".into()), + ), + ( + "general.name".into(), + GgufValue::String("pygmy-hybrid".into()), + ), + ("qwen35.context_length".into(), GgufValue::Uint32(262_144)), + ("qwen35.block_count".into(), GgufValue::Uint32(32)), + ( + "qwen35.full_attention_interval".into(), + GgufValue::Uint32(4), + ), + ( + "qwen35.attention.head_count_kv".into(), + GgufValue::Uint32(4), + ), + ("qwen35.attention.key_length".into(), GgufValue::Uint32(256)), + ( + "qwen35.rope.dimension_sections".into(), + GgufValue::ArrayInt32(vec![11, 11, 10, 0]), + ), + ("qwen35.rope.freq_base".into(), GgufValue::Float32(1.0e7)), + ( + "quantize.imatrix.file".into(), + GgufValue::String("imatrix.gguf".into()), + ), + ("zzz_never_heard_of.depth".into(), GgufValue::Uint64(7)), + ( + "tokenizer.ggml.model".into(), + GgufValue::String("gpt2".into()), + ), + ] +} + +fn fixture() -> NamedTempFile { + let file = NamedTempFile::with_suffix(".gguf").expect("temp gguf"); + let tensors = vec![GgufTensor { + name: "token_embd.weight".to_string(), + shape: vec![4, 4], + dtype: GgmlType::F32, + data: vec![0u8; 64], + }]; + let mut bytes = Vec::new(); + export_tensors_to_gguf(&mut bytes, &tensors, &header()).expect("export gguf"); + std::fs::write(file.path(), &bytes).expect("write gguf"); + file +} + +fn assert_every_header_key(shown: &BTreeMap, layer: &str) { + let header = header(); + let missing: Vec<&str> = header + .iter() + .map(|(k, _)| k.as_str()) + .filter(|k| !shown.contains_key(*k)) + .collect(); + assert!( + missing.is_empty(), + "{layer} dropped header keys: {missing:?}" + ); + assert_eq!( + shown.len(), + header.len(), + "{layer} must show exactly the header's KV count" + ); +} + +#[test] +fn raw_metadata_carries_every_header_key() { + let file = fixture(); + let raw = load_gguf_raw(file.path()).expect("load"); + assert_every_header_key(&raw.raw_metadata, "load_gguf_raw"); + assert_eq!(raw.raw_metadata["qwen35.context_length"], "262144"); + assert_eq!( + raw.raw_metadata["qwen35.rope.dimension_sections"], + "[len=4]" + ); + assert_eq!(raw.raw_metadata["zzz_never_heard_of.depth"], "7"); +} + +/// The path `apr inspect` takes for a GGUF (`run_rosetta_inspect`). +#[test] +fn rosetta_inspect_carries_every_header_key() { + let file = fixture(); + let report = RosettaStone::new().inspect(file.path()).expect("inspect"); + assert_every_header_key(&report.metadata, "RosettaStone::inspect"); +} + +/// The keys are shown, not consumed: the config accessors still read only the +/// allowlisted map, so an architecture the loaders do not know gets no config it +/// never had (a widened accessor would hand `apr import` a qwen35 hidden size). +#[test] +fn display_only_keys_do_not_reach_the_config_accessors() { + let file = fixture(); + let reader = GgufReader::from_file(file.path()).expect("read"); + assert!(reader + .display_only_metadata + .contains_key("qwen35.context_length")); + assert!(!reader.metadata.contains_key("qwen35.context_length")); + assert_eq!(reader.context_length(), None); + assert_eq!(reader.num_layers(), None); + + // `keep_all` (the shard merge) still puts everything in `metadata`. + let full = GgufReader::from_file_full(file.path()).expect("read full"); + assert!(full.display_only_metadata.is_empty()); + assert_eq!(full.metadata.len(), header().len()); +} + +/// Parsing (not skipping) keys outside the allowlist must not turn a hostile +/// array count into an allocation or an out-of-bounds index. +#[test] +fn an_array_count_past_the_end_of_the_file_is_an_error_not_a_panic() { + for elem_type in [4u32, 5, 6, 8] { + let key = "zzz.hostile"; + let mut data = Vec::new(); + data.extend_from_slice(&GGUF_MAGIC.to_le_bytes()); + data.extend_from_slice(&3u32.to_le_bytes()); + data.extend_from_slice(&0u64.to_le_bytes()); // tensors + data.extend_from_slice(&1u64.to_le_bytes()); // one KV + data.extend_from_slice(&(key.len() as u64).to_le_bytes()); + data.extend_from_slice(key.as_bytes()); + data.extend_from_slice(&9u32.to_le_bytes()); // array + data.extend_from_slice(&elem_type.to_le_bytes()); + data.extend_from_slice(&(u64::MAX / 2).to_le_bytes()); + data.extend_from_slice(&[0u8; 16]); + let err = GgufReader::from_bytes(data).expect_err("a count past EOF must be refused"); + assert!( + err.to_string().contains("overruns"), + "type {elem_type}: {err}" + ); + } +} diff --git a/crates/aprender-core/src/format/gguf/builder.rs b/crates/aprender-core/src/format/gguf/builder.rs index 5aca8570aa..91f5c7bcd1 100644 --- a/crates/aprender-core/src/format/gguf/builder.rs +++ b/crates/aprender-core/src/format/gguf/builder.rs @@ -161,10 +161,13 @@ fn test_from_bytes_tensor_at_max_dims() { } // ======================================================================== -// skip_metadata_value Tests (via from_bytes with non-parsed keys) +// Keys outside the parse allowlist (via from_bytes with non-parsed keys). +// They were skipped by length arithmetic until #3733; they are now decoded into +// `display_only_metadata`, so `apr inspect` shows them, and still kept out of +// `metadata`, which the config accessors read. // ======================================================================== -/// Build a GGUF with metadata that will be skipped (key prefix not in parsed set) +/// Build a GGUF with one metadata key outside the parse allowlist fn build_gguf_with_skipped_metadata(key: &str, value_type: u32, value_bytes: &[u8]) -> Vec { let mut data = Vec::new(); @@ -175,7 +178,7 @@ fn build_gguf_with_skipped_metadata(key: &str, value_type: u32, value_bytes: &[u data.extend_from_slice(&1u64.to_le_bytes()); // metadata_count = 1 // Metadata KV: key with prefix that does NOT match tokenizer./general./llama./qwen2./phi./mistral. - // so it will be skipped via skip_metadata_value + // so it lands in display_only_metadata (#3733) data.extend_from_slice(&(key.len() as u64).to_le_bytes()); data.extend_from_slice(key.as_bytes()); data.extend_from_slice(&value_type.to_le_bytes()); @@ -189,6 +192,7 @@ fn test_skip_metadata_value_uint8() { let data = build_gguf_with_skipped_metadata("custom.u8", 0, &[42u8]); let reader = GgufReader::from_bytes(data).expect("skip uint8"); assert!(!reader.metadata.contains_key("custom.u8")); + assert!(reader.display_only_metadata.contains_key("custom.u8")); } #[test] @@ -196,6 +200,7 @@ fn test_skip_metadata_value_int8() { let data = build_gguf_with_skipped_metadata("custom.i8", 1, &[0xFEu8]); let reader = GgufReader::from_bytes(data).expect("skip int8"); assert!(!reader.metadata.contains_key("custom.i8")); + assert!(reader.display_only_metadata.contains_key("custom.i8")); } #[test] @@ -203,6 +208,7 @@ fn test_skip_metadata_value_uint16() { let data = build_gguf_with_skipped_metadata("custom.u16", 2, &1000u16.to_le_bytes()); let reader = GgufReader::from_bytes(data).expect("skip uint16"); assert!(!reader.metadata.contains_key("custom.u16")); + assert!(reader.display_only_metadata.contains_key("custom.u16")); } #[test] @@ -210,6 +216,7 @@ fn test_skip_metadata_value_int16() { let data = build_gguf_with_skipped_metadata("custom.i16", 3, &(-500i16).to_le_bytes()); let reader = GgufReader::from_bytes(data).expect("skip int16"); assert!(!reader.metadata.contains_key("custom.i16")); + assert!(reader.display_only_metadata.contains_key("custom.i16")); } #[test] @@ -217,6 +224,7 @@ fn test_skip_metadata_value_bool() { let data = build_gguf_with_skipped_metadata("custom.flag", 7, &[1u8]); let reader = GgufReader::from_bytes(data).expect("skip bool"); assert!(!reader.metadata.contains_key("custom.flag")); + assert!(reader.display_only_metadata.contains_key("custom.flag")); } #[test] @@ -229,6 +237,7 @@ fn test_skip_metadata_value_string() { let data = build_gguf_with_skipped_metadata("custom.str", 8, &value_bytes); let reader = GgufReader::from_bytes(data).expect("skip string"); assert!(!reader.metadata.contains_key("custom.str")); + assert!(reader.display_only_metadata.contains_key("custom.str")); } #[test] @@ -236,6 +245,7 @@ fn test_skip_metadata_value_uint64() { let data = build_gguf_with_skipped_metadata("custom.u64", 10, &999u64.to_le_bytes()); let reader = GgufReader::from_bytes(data).expect("skip uint64"); assert!(!reader.metadata.contains_key("custom.u64")); + assert!(reader.display_only_metadata.contains_key("custom.u64")); } #[test] @@ -243,6 +253,7 @@ fn test_skip_metadata_value_int64() { let data = build_gguf_with_skipped_metadata("custom.i64", 11, &(-1i64).to_le_bytes()); let reader = GgufReader::from_bytes(data).expect("skip int64"); assert!(!reader.metadata.contains_key("custom.i64")); + assert!(reader.display_only_metadata.contains_key("custom.i64")); } #[test] @@ -251,6 +262,7 @@ fn test_skip_metadata_value_float64() { build_gguf_with_skipped_metadata("custom.f64", 12, &std::f64::consts::E.to_le_bytes()); let reader = GgufReader::from_bytes(data).expect("skip float64"); assert!(!reader.metadata.contains_key("custom.f64")); + assert!(reader.display_only_metadata.contains_key("custom.f64")); } #[test] @@ -259,6 +271,7 @@ fn test_skip_metadata_value_unknown_type() { let data = build_gguf_with_skipped_metadata("custom.unk", 99, &[0u8; 4]); let reader = GgufReader::from_bytes(data).expect("skip unknown"); assert!(!reader.metadata.contains_key("custom.unk")); + assert!(reader.display_only_metadata.contains_key("custom.unk")); } #[test] diff --git a/crates/aprender-core/src/format/gguf/reader.rs b/crates/aprender-core/src/format/gguf/reader.rs index 198931c5a5..1e23f114a8 100644 --- a/crates/aprender-core/src/format/gguf/reader.rs +++ b/crates/aprender-core/src/format/gguf/reader.rs @@ -86,6 +86,28 @@ fn read_metadata_array(data: &[u8], offset: usize) -> Result<(GgufValue, usize)> let count = read_u64(data, offset + 4)? as usize; let mut consumed = 12; // type (4) + count (8) + // #3733: every header key is now parsed (keys outside the allowlist used to + // be skipped by length arithmetic alone), so a count taken from the file must + // not size an allocation or index past the end. The span is checked first; + // a string needs at least its 8-byte length, which bounds that count too. + let available = data.len().saturating_sub(offset + consumed); + let min_elem = match elem_type { + 0..=1 | 7 => 1, + 2..=3 => 2, + 8 | 10..=12 => 8, + _ => 4, + }; + if count + .checked_mul(min_elem) + .is_none_or(|need| need > available) + { + return Err(AprenderError::FormatError { + message: format!( + "GGUF metadata array of {count} elements (type {elem_type}) overruns the file" + ), + }); + } + match elem_type { 8 => { let mut strings = Vec::with_capacity(count); @@ -107,13 +129,7 @@ fn read_metadata_array(data: &[u8], offset: usize) -> Result<(GgufValue, usize)> 5 => { let mut values = Vec::with_capacity(count); for _ in 0..count { - let v = i32::from_le_bytes([ - data[offset + consumed], - data[offset + consumed + 1], - data[offset + consumed + 2], - data[offset + consumed + 3], - ]); - values.push(v); + values.push(read_i32_le(data, offset + consumed)?); consumed += 4; } Ok((GgufValue::ArrayInt32(values), consumed)) @@ -121,25 +137,13 @@ fn read_metadata_array(data: &[u8], offset: usize) -> Result<(GgufValue, usize)> 6 => { let mut values = Vec::with_capacity(count); for _ in 0..count { - let v = f32::from_le_bytes([ - data[offset + consumed], - data[offset + consumed + 1], - data[offset + consumed + 2], - data[offset + consumed + 3], - ]); - values.push(v); + values.push(read_f32_le(data, offset + consumed)?); consumed += 4; } Ok((GgufValue::ArrayFloat32(values), consumed)) } _ => { - let elem_size = match elem_type { - 0..=1 | 7 => 1, - 2..=3 => 2, - 10..=12 => 8, - _ => 4, - }; - consumed += count * elem_size; + consumed += count * min_elem; Ok((GgufValue::ArrayUint32(vec![]), consumed)) } } @@ -261,42 +265,6 @@ pub(crate) fn read_metadata_value( } } -/// Skip a metadata value (we don't need to parse all metadata types) -fn skip_metadata_value(data: &[u8], offset: usize, value_type: u32) -> Result { - match value_type { - 0..=1 | 7 => Ok(1), // Uint8, Int8, Bool - 2..=3 => Ok(2), // Uint16, Int16 - 8 => { - // String - let len = read_u64(data, offset)? as usize; - Ok(8 + len) - } - 9 => { - // Array - need to read element type and count, then skip elements - let elem_type = read_u32(data, offset)?; - let count = read_u64(data, offset + 4)? as usize; - let elem_size = match elem_type { - 0..=1 | 7 => 1, // Uint8, Int8, Bool - 2..=3 => 2, // Uint16, Int16 - 8 => { - // Array of strings - need to iterate - let mut skip = 12; // type (4) + count (8) - for _ in 0..count { - let (_, slen) = read_string(data, offset + skip)?; - skip += slen; - } - return Ok(skip); - } - 10..=12 => 8, // Uint64, Int64, Float64 - _ => 4, // Uint32, Int32, Float32, Unknown - }; - Ok(12 + count * elem_size) - } - 10..=12 => Ok(8), // Uint64, Int64, Float64 - _ => Ok(4), // Uint32, Int32, Float32, Unknown (4 bytes default) - } -} - /// Parsed GGUF file for import #[derive(Debug)] pub struct GgufReader { @@ -312,6 +280,16 @@ pub struct GgufReader { pub data_offset: usize, /// Metadata key-value pairs (extracted from GGUF) pub metadata: BTreeMap, + /// Every header key OUTSIDE the parse allowlist, kept for inventory only + /// (`apr inspect`). Empty when the reader was built with `keep_all`. + /// + /// #3733: the allowlist (`tokenizer.`, `general.`, and six arch prefixes) + /// decided what `apr inspect --json` could show, so a Qwen3.5 file lost all + /// 22 of its `qwen35.*` keys, `context_length` among them: `qwen35.` does not + /// start with `qwen3.`. The keys live here rather than in `metadata` because + /// the config accessors read `metadata`, and widening THEM would hand every + /// unlisted architecture a config the import path has never been given. + pub display_only_metadata: BTreeMap, } /// Tensor metadata from GGUF file diff --git a/crates/aprender-core/src/format/gguf/reader_parsing.rs b/crates/aprender-core/src/format/gguf/reader_parsing.rs index 9f7a0f5a25..5b8d5ea65a 100644 --- a/crates/aprender-core/src/format/gguf/reader_parsing.rs +++ b/crates/aprender-core/src/format/gguf/reader_parsing.rs @@ -26,9 +26,9 @@ impl GgufReader { Self::from_bytes_keep(data, false) } - /// Parse GGUF from bytes. When `keep_all` is true EVERY metadata key is - /// retained; otherwise only `tokenizer.` / `general.` / known-arch keys are - /// parsed (the rest skipped for efficiency). + /// Parse GGUF from bytes. When `keep_all` is true EVERY metadata key goes to + /// `metadata`; otherwise only `tokenizer.` / `general.` / known-arch keys do, + /// and the rest go to `display_only_metadata` (#3733). No key is dropped. pub fn from_bytes_keep(data: Vec, keep_all: bool) -> Result { if data.len() < 24 { return Err(AprenderError::FormatError { @@ -78,6 +78,7 @@ impl GgufReader { // Parse metadata section (extract vocabulary and other tokenizer data) let mut offset = 24; let mut metadata = BTreeMap::new(); + let mut display_only_metadata = BTreeMap::new(); for _ in 0..metadata_kv_count { // Read key let (key, key_len) = read_string(&data, offset)?; @@ -103,8 +104,10 @@ impl GgufReader { metadata.insert(key, value); offset += value_len; } else { - // Skip other metadata for efficiency - let value_len = skip_metadata_value(&data, offset, value_type)?; + // #3733: kept, not skipped. `apr inspect` must show every key the + // header carries; only the config accessors are kept to the list. + let (value, value_len) = read_metadata_value(&data, offset, value_type)?; + display_only_metadata.insert(key, value); offset += value_len; } } @@ -164,6 +167,7 @@ impl GgufReader { tensors, data_offset, metadata, + display_only_metadata, }) } diff --git a/crates/aprender-core/src/format/gguf/tests/core_tensor_export.rs b/crates/aprender-core/src/format/gguf/tests/core_tensor_export.rs index cd2531114e..c06dc5be17 100644 --- a/crates/aprender-core/src/format/gguf/tests/core_tensor_export.rs +++ b/crates/aprender-core/src/format/gguf/tests/core_tensor_export.rs @@ -121,6 +121,7 @@ fn make_test_reader(metadata: std::collections::BTreeMap) -> tensors: vec![], data_offset: 0, metadata, + display_only_metadata: std::collections::BTreeMap::new(), } } diff --git a/docs/audits/quorum-PMAT-3733.json b/docs/audits/quorum-PMAT-3733.json new file mode 100644 index 0000000000..57c43acfd9 --- /dev/null +++ b/docs/audits/quorum-PMAT-3733.json @@ -0,0 +1,178 @@ +{ + "ticket": "PMAT-3733", + "base": "52f43da71", + "base_resolved": "52f43da71", + "base_note": "no origin/52f43da71 exists; judged against the local ref", + "head": "7dad33b28bd31f07b0efbea6ef65c1eb872b44aa", + "diff_sha256": "8ab6f85d14ef1d6954f4ec6fd5cb506c4f06a3191c2587c401fc7437ce006438", + "width": 3, + "executor": "agy", + "prompt_mode": "inline", + "prompt_bytes": 31205, + "author": { + "model": "claude-opus-5", + "family": "claude", + "source": "flag" + }, + "agreed": true, + "lanes": [ + { + "lane": 1, + "status": "SUCCESS", + "verdict": "PASS", + "summary": "The diff comprehensively and accurately implements PMAT-3733. By parsing all metadata keys and placing non-allowlisted ones into `display_only_metadata`, the diff successfully ensures `apr inspect --json` yields every KV from the GGUF header without perturbing `metadata` mappings that downstream config accessors expect. The related refactoring for CLI testing (`rosetta_json`) and the essential array-allocation hardening correctly resolve the ticket's architectural implications. All test expectations mentioned in the ticket notes are met in `api_tests_all_keys_3733.rs` and `inspect_tests.rs`.", + "findings": [], + "raw_bytes": 2471, + "err_bytes": 0, + "envelope_status": "SUCCESS", + "verdict_source": "structured_output", + "grounding_check": "parity", + "model": "gemini-3.1-pro-high", + "model_measured": "gemini-3.1-pro-high", + "model_source": "measured", + "family": "gemini", + "role": "independent", + "fallback": { + "judged_by": "gemini-3.1-pro-high", + "exhausted": false, + "attempts": [ + { + "model": "gemini-3.1-pro-high", + "family": "gemini", + "outcome": "answered" + } + ], + "skipped": [] + } + }, + { + "lane": 2, + "status": "SUCCESS", + "verdict": "PASS", + "summary": "The diff successfully implements PMAT-3733. It ensures that every GGUF metadata key is parsed and retained without an arch-prefixed allowlist. Keys outside the configuration allowlist are safely directed to `display_only_metadata` so they appear in `apr inspect --json` without unexpectedly widening the model configuration path. The RosettaStone inspection path is correctly traced and tested, and array parsing is hardened against out-of-bounds/out-of-memory errors on hostile inputs. Tests verifying the KV counts and the unknown architecture prefix (`zzz_never_heard_of`) are present and complete.", + "findings": [], + "raw_bytes": 2465, + "err_bytes": 0, + "envelope_status": "SUCCESS", + "verdict_source": "structured_output", + "grounding_check": "parity", + "model": "gemini-3.1-pro-low", + "model_measured": "gemini-3.1-pro-low", + "model_source": "measured", + "family": "gemini", + "role": "independent", + "fallback": { + "judged_by": "gemini-3.1-pro-low", + "exhausted": false, + "attempts": [ + { + "model": "gemini-3.1-pro-low", + "family": "gemini", + "outcome": "answered" + } + ], + "skipped": [] + } + }, + { + "lane": 3, + "status": "SUCCESS", + "verdict": "PASS", + "summary": "The diff fully and accurately implements PMAT-3733. By parsing all GGUF metadata keys and storing non-allowlisted keys in `display_only_metadata`, all header keys (including `qwen35.*`, `quantize.*`, and arbitrary architecture keys such as `zzz_never_heard_of.*`) reach `apr inspect --json` via RosettaStone inspection without unexpectedly exposing them to configuration accessors (e.g. `context_length()`). Array parsing in `read_metadata_array` was properly hardened against hostile counts to prevent OOM or panics. All tests in `aprender-core` (645 passed) and `apr-cli` (140 passed) were measured and confirmed passing as claimed.", + "findings": [], + "raw_bytes": 5187, + "err_bytes": 0, + "envelope_status": "SUCCESS", + "verdict_source": "structured_output", + "grounding_check": "parity", + "model": "gemini-3.8-flash-high", + "model_measured": "gemini-3.8-flash-high", + "model_source": "measured", + "family": "gemini", + "role": "independent", + "fallback": { + "judged_by": "gemini-3.8-flash-high", + "exhausted": false, + "attempts": [ + { + "model": "gemini-3.8-flash-high", + "family": "gemini", + "outcome": "answered" + } + ], + "skipped": [] + } + } + ], + "dissent": [], + "dedup": [], + "uncovered": [], + "coverage_source": "lanes", + "partial": false, + "partial_reasons": [], + "fallback": { + "same_family_width": 1, + "chain": [ + { + "model": "gemini-3.1-pro-high", + "family": "gemini", + "disposition": "configured" + }, + { + "model": "gemini-3.1-pro-low", + "family": "gemini", + "disposition": "configured" + }, + { + "model": "gemini-3.8-flash-high", + "family": "gemini", + "disposition": "configured" + }, + { + "model": "gpt-oss-120b-medium", + "family": "openai", + "disposition": "fallback" + }, + { + "model": "qwen3.5", + "family": "qwen", + "disposition": "not-run", + "why": "no quorum.local_lane in the config — the aprender lane has no model to load" + }, + { + "model": "claude-opus-4-6-thinking", + "family": "claude", + "disposition": "width", + "why": "same family as the author: at most 1 lane, recorded role width, counted toward no floor (R-15a)" + }, + { + "model": "claude-sonnet-4-6", + "family": "claude", + "disposition": "width", + "why": "same family as the author: at most 1 lane, recorded role width, counted toward no floor (R-15a)" + } + ], + "precheck": [ + { + "family": "gemini", + "model": "gemini-3.1-pro-high", + "probe": 1, + "outcome": "live" + } + ], + "prah": { + "source": "install-receipt", + "path": "/home/noah/.claude/skills/paiml-implement/bin/prah" + } + }, + "auto_merge": { + "checked": false, + "was_armed": false, + "disarmed": false, + "note": "no --pr given: nothing to disarm" + }, + "lint": { + "ok": true, + "output": "receipt complete: kind=artifact lanes=3 author=claude-opus-5/claude" + } +} diff --git a/docs/roadmaps/entries/PMAT-3733.yaml b/docs/roadmaps/entries/PMAT-3733.yaml new file mode 100644 index 0000000000..6e6b1ec353 --- /dev/null +++ b/docs/roadmaps/entries/PMAT-3733.yaml @@ -0,0 +1,19 @@ +- id: PMAT-3733 + github_issue: 3733 + item_type: task + title: 'apr inspect --json drops every qwen35.* metadata key: the GGUF reader parsed only an arch-prefix allowlist; every header key now reaches inspect (#3733)' + status: in_progress + priority: critical + assigned_to: aprender-a8 + created: 2026-09-21T17:05:00Z + updated: 2026-09-21T17:05:00Z + spec: null + acceptance_criteria: + - 'Issue #3733 done_when, verbatim: "`apr inspect --json ` emits every KV the header carries (or at least every `.*` key) for any arch, including one inspect has never heard of. A test runs a fixture GGUF with an unknown arch prefix and fails if any header key is missing from `.metadata`."' + - 'Cop comment on #3733 (2026-09-21T16:25:54Z), verbatim: "done_when: `apr inspect --json` emits EVERY GGUF metadata key the file carries, with no arch-prefixed allowlist (the 46 KVs on the 4B). A case row checks the count against the header''s KV count, plus a mutant (drop one prefix and it goes RED). Also trace the RosettaStone path, since 62 didn''t."' + phases: [] + subtasks: [] + estimated_effort: null + labels: + - kind:code + notes: 'ROOT CAUSE (traced, the RosettaStone path included): `apr inspect` sends every GGUF to `run_rosetta_inspect` (inspect.rs:162), then `RosettaStone::inspect_gguf` (rosetta/validate_inspect.rs:419) copies `load_gguf_raw(..).raw_metadata` unfiltered, and `load_gguf_raw` (gguf/api.rs) built that map from `GgufReader::metadata`. `GgufReader::from_bytes` parsed a key only if it started with `tokenizer.`, `general.`, `llama.`, `qwen2.`, `qwen3.`, `phi.`, `mistral.` or `gpt2.` (reader_parsing.rs) and length-skipped everything else, so `qwen35.*` (which does not start with `qwen3.`) and `quantize.*` never existed after the parse. No filter was in inspect or rosetta. FIX: nothing is skipped any more. Keys outside the allowlist are decoded into a new `GgufReader::display_only_metadata`, and `raw_metadata` is `metadata` chained with it. DELIBERATELY NOT `keep_all` on this path: the config accessors (`context_length()`, `num_layers()`, `hidden_size()`...) read `metadata`, and `load_gguf_raw` feeds them into the `apr import` model config, so widening `metadata` would give every unlisted architecture a config the import path has never been given. That is a behaviour change outside this row. A test pins it: `reader.context_length()` stays None on the qwen35 fixture while `display_only_metadata` holds the key. `from_file_full` (the shard merge) is unchanged and keeps everything in `metadata`. `skip_metadata_value` lost its only caller and is deleted; its ten builder tests now also assert that the key lands in `display_only_metadata`. HARDENING, which parsing instead of skipping makes necessary: `read_metadata_array` took its element count from the file, called `Vec::with_capacity(count)` and indexed `data[..]` directly for i32/f32 arrays. The span is now checked against the bytes left before any allocation, and the checked `read_i32_le`/`read_f32_le` are used. A hostile count returns FormatError "overruns the file" instead of panicking. The case row covers element types 4, 5, 6 and 8. CLI: `output_rosetta_json` is split into `rosetta_json(path, report) -> Value` plus the print, so the --json document is testable. It emits `report.metadata` unfiltered. MEASURED on lambda. Pre-fix instrument: `apr 0.68.2 (97093d2a5)`. It is a valid baseline because `git log v0.68.2..52f43da71` touches none of reader_parsing.rs, api.rs, validate_inspect.rs or inspect.rs. Qwen3.5-4B-Q4_K_M: .metadata|length = 24, qwen35.* keys = 0, qwen35.context_length = null, which reproduces #3733. Post-fix instrument: `apr 0.69.0 (336b7230b)`, resolved and HEAD-checked by `scripts/apr_bin.sh` under CARGO_TARGET_DIR=/mnt/nvme-raid0/targets/a8-3718. The header KV count comes independently from python `gguf` (GGUFReader.fields minus GGUF.*): 0.8B 46/46, 2B 46/46, 4B 46/46, 9B 46/46, 27B 49/49 (shown/header), each with all 18 qwen35.* keys, context_length 262144, full_attention_interval 4, block_count 24/24/32/32/64, head_count_kv 2/2/4/4/4, key_length 256. Control on an arch that was already on the allowlist: Qwen3-1.7B 32/32, context_length 40960, unchanged. TESTS: api_tests_all_keys_3733.rs covers a fixture whose header carries qwen35.*, quantize.*, an unheard-of prefix `zzz_never_heard_of.*`, general.* and tokenizer.*. It asserts load_gguf_raw and RosettaStone::inspect both show every header key with count == header length, the accessor firewall, and the hostile-count rows. inspect_tests adds rosetta_json_emits_every_metadata_key. Suites: `cargo test -p aprender-core --lib -- format::gguf format::rosetta` 645 passed; `cargo test -p apr-cli --lib -- inspect` 140 passed. MUTANTS, each run and each RED: (1) skip restored (display-only insert removed): 13 red, including both header-count rows and all 10 builder rows; (2) the `.chain(display_only_metadata)` union dropped: both header-count rows red; (3) the cop''s mutant, one prefix dropped (`quantize.*` not kept): both header-count rows red; (4) bounds check removed: the hostile-count row red; (5) a `qwen35.` filter in rosetta_json: rosetta_json_emits_every_metadata_key red. KNOWN, NOT CHANGED: arrays whose element type is 8/16/64-bit (types 0,1,2,3,7,10,11,12) still decode to an empty ArrayUint32 and so display `[len=0]`. This is pre-existing, and the shard merge also depends on that variant, so it is out of scope. Census of all 21 GGUFs in /home/noah/models: array element types are STRING (59) and INT32 (30) only, so no fleet file reaches it.'