diff --git a/docs/reviews/006-implementation-review-030.md b/docs/reviews/006-implementation-review-030.md index 83c598d..982baaf 100644 --- a/docs/reviews/006-implementation-review-030.md +++ b/docs/reviews/006-implementation-review-030.md @@ -1,5 +1,5 @@ --- -status: in-progress (H1, L1, H3, L5, L6, H2 resolved 2026-09-02) +status: in-progress (H1, L1, H3, L5, L6, H2, M1, M2, M3 resolved 2026-09-02) last_updated: 2026-09-02 reviewed_artifacts: - src/read_plan.rs @@ -95,14 +95,13 @@ sub-types, `fingerprint()` methods; `Hash` on `Endian`/ | Severity | Count | Status | |----------|------:|--------| -| High | 3 (H1, H2, H3) | H1, H2, H3 resolved 2026-09-02 | -| Medium | 4 (M1, M2, M3, M4) | open | +| High | 3 (H1, H2, H3) | all resolved 2026-09-02 | +| Medium | 4 (M1, M2, M3, M4) | M1, M2, M3 resolved 2026-09-02; M4 ongoing | | Low | 6 (L1–L6) | L1, L5, L6 resolved 2026-09-02 | | Nit | 2 (N1, N2) | open | -All three Highs are now resolved. The remaining work is Medium/Low/Nit: -M1+M2 (same file, one session), M3 (trivial), M4 (per-fix posture), and -L2/L3/L4/N1/N2 opportunistic. +All three Highs and three of four Mediums are resolved. Remaining: +M4 (ongoing per-fix coverage posture), L2/L3/L4/N1/N2 opportunistic. The three Highs are adversarial-input crashes (H1, H2) and a cross-consumer wire-layout convention gap (H3) — all three are the @@ -134,6 +133,10 @@ them became materially easier to hit with the 0.3.0 surface. all three standalone walkers; full cyclic/deep-nesting/diamond test family added. 501 tests green, clippy `-D warnings` clean, wasm build green, `cargo doc --no-deps` zero warnings. +- **M1 + M2 + M3 (2026-09-02):** resolved in one commit (same file / + adjacent surfaces, per the recommended order) — see the resolution + blocks on each finding. 508 tests green, clippy `-D warnings` clean, + wasm build green, `cargo doc --no-deps` zero warnings. --- @@ -586,6 +589,16 @@ position") already covers records correctly. Add the `non_final_record_rejected_in_aligned_mode` test mirroring `non_final_inline_string_rejected_in_aligned_mode`. +**Resolution (2026-09-02):** exactly the recommended fix — +`field_variable_kind` now matches `BastType::Record(_) => +Some(AlkTypeKind::Record)` (with a doc comment noting the +prefix-then-variable shape and the M1 hole it closes). Tests added +mirroring the string family: `non_final_record_rejected_in_aligned_mode` +(Offset error, path `counts`, message contains ADR-006 + "record") and +`final_inline_record_allowed_in_aligned_mode` (record-as-last-field +computes `counts` prefix @ 4, total 8 — the M2 position). Done with M2 +in the same commit. + ### M2. Aligned-mode record fields are untested end-to-end and sit right next to the M1 hole **Files**: `src/materialize.rs:880-893` (aligned record dispatch), @@ -610,6 +623,23 @@ records, this path needs a locking test: record-as-last-field round-trips, record-mid-struct is rejected (post-M1), and `total_size` accounting for the prefix is asserted. +**Resolution (2026-09-02):** the requested locking tests, post-M1: + +- `materialize.rs::materialize_aligned_record_last_field_roundtrips` — + `record` as last field driven end-to-end through + `OffsetMap::compute` + `materialize_aligned`: both entries + materialize (`{ "b": 10, "a": 20 }`), `id` intact at its fixed + offset, wire arithmetic asserted in the test (22 bytes: 4 id + 4 + record count + 2 × (4 key-len + 1 key + 2 value)). +- `engine.rs::validate_bytes_aligned_record_last_field_round_trips` — + the same shape through the full public `validate_bytes` path + (`record`, 26 bytes), plus a corrupted-buffer arm asserting + garbage in entry bytes is rejected (not silently accepted). +- Post-M1 rejection covered by `non_final_record_rejected_in_aligned_mode` + (see M1's resolution); `total_size` prefix accounting asserted by + `final_inline_record_allowed_in_aligned_mode`. +Done with M1 + M3 in the same commit. + ### M3. `read_field`'s aligned `Struct` arm is unreachable dead code **File**: `src/engine.rs:449-452` @@ -632,6 +662,20 @@ stage rather than at lookup), or keep it and make struct entries exist (deliberate feature — then document and test it). Deleting is the smaller, honest change for 0.3.0's design. +**Resolution (2026-09-02):** deletion taken. The `Struct` arm now +returns a documented defensive `Offset` error ("read_field does not +support composite types (no offset-map entry exists for a struct path — +only its leaf fields are recorded)") instead of a fabricated +`FieldValue::Struct` — the reachable failure for a struct path remains +the offset-map lookup miss, as the finding established. The doc comment +now states that struct paths have no `OffsetMap` entry (read the leaf +fields by dotted paths) and that the Offset miss is the reachable +failure for any composite path. Locking test +`read_field_on_nested_struct_path_is_offset_error_not_struct_value`: +`read_field("header")` is an `Offset` error while +`read_field("header.magic")` succeeds. `FieldValue::Struct` itself is +untouched (public API; still constructed by the packed reader). + ### M4. Coverage weak spots map onto the findings above — the aligned materializer half is the least-tested code in the engine **Tool**: `cargo llvm-cov --release` (0.8.4). Overall: **89.59% lines, @@ -929,11 +973,15 @@ Worth recording, because the findings shouldn't eclipse it: resolution block on the finding). 3. ~~**H2** — shared walk guard~~ **resolved 2026-09-02** (see the resolution block on the finding). -4. **M1 + M2** — same file, same test family; do together. -5. **M3** — trivial deletion (or the feature decision, if kept). +4. ~~**M1 + M2**~~ **resolved 2026-09-02** (with M3; see the + resolution blocks on the findings). +5. ~~**M3**~~ **resolved 2026-09-02** (see the resolution block on the + finding). 6. **M4** — ongoing: per-fix coverage extension as recommended above; the aligned-materializer test gap (1) is the single biggest chunk - and deserves its own session. + and deserves its own session. (M1/M2/M3's fixes each extended + coverage over their touched paths — the aligned record dispatch and + the aligned record `validate_bytes` path are now publicly exercised.) 7. **L1–L6, N1, N2** — opportunistic, folded into whichever session touches the relevant file (L6 is the exception — it belongs with H3; N2 pairs naturally with any bast/bast_meta session; L1 done diff --git a/src/engine.rs b/src/engine.rs index ec7d106..8995398 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -342,14 +342,18 @@ impl AlkTypeEngine { /// fields. /// /// Returns an error if compiled in packed mode — use - /// [`AlkTypeEngine::sequential_reader`] for packed mode. Also - /// returns an error for composite kinds (`Union`, `Array`, - /// `Record`) — those are better handled via the layout-specific APIs. + /// [`AlkTypeEngine::sequential_reader`] for packed mode. Composite + /// kinds error as well: a struct path never has an `OffsetMap` entry + /// (only its leaf fields are recorded — read those by their dotted + /// paths), and `Union`/`Array`/`Record` are better handled via the + /// layout-specific APIs. /// /// # Errors /// /// - [`AlkTypeError::Access`] if compiled in packed mode. - /// - [`AlkTypeError::Offset`] if `field_path` is not in the offset map. + /// - [`AlkTypeError::Offset`] if `field_path` is not in the offset + /// map (the reachable failure for any composite path, including + /// struct paths — no entry exists for them). /// - [`AlkTypeError::Access`] for buffer-too-short or invalid data, /// propagated from [`crate::data_access`]. pub fn read_field<'a>( @@ -447,9 +451,12 @@ impl AlkTypeEngine { }; Ok(FieldValue::Bytes(v)) } - AlkTypeKind::Struct => Ok(FieldValue::Struct { - start: range.start, - end: range.end, + AlkTypeKind::Struct => Err(AlkTypeError::Offset { + field_path: field_path.to_string(), + reason: "read_field does not support composite types (no offset-map entry \ + exists for a struct path — only its leaf fields are recorded); \ + read the leaf fields by their dotted paths instead" + .to_string(), }), AlkTypeKind::Union | AlkTypeKind::Array | AlkTypeKind::Record => { Err(AlkTypeError::Access { @@ -738,6 +745,37 @@ mod tests { assert!(matches!(err, AlkTypeError::Offset { .. }), "got {err:?}"); } + #[test] + fn read_field_on_nested_struct_path_is_offset_error_not_struct_value() { + // M3: the read_field kind dispatch previously had a Struct arm + // returning FieldValue::Struct — unreachable, because + // OffsetMap::compute records only a nested struct's inner leaf + // entries, never an entry for the struct path itself. Lock the + // honest behavior: the struct path is an Offset miss. + let doc = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { + "name": "header", + "kind": { + "kind": "struct", + "fields": [ { "name": "magic", "kind": "uint32" } ] + } + } + ] + } + } + }); + let engine = AlkTypeEngine::compile(&doc, "S", LayoutMode::Aligned, None).expect("compile"); + let buf = [0u8; 4]; + let err = engine.read_field(&buf, "header").unwrap_err(); + assert!(matches!(err, AlkTypeError::Offset { .. }), "got {err:?}"); + // The leaf inside the nested struct is reachable by dotted path. + assert!(engine.read_field(&buf, "header.magic").is_ok()); + } + #[test] fn read_field_returns_error_for_composite_types() { let doc = json!({ @@ -1275,6 +1313,55 @@ mod tests { assert!(engine.validate_bytes(&buf).is_ok()); } + #[test] + fn validate_bytes_aligned_record_last_field_round_trips() { + // M2: the aligned record path (OffsetMap LengthPrefixed entry at + // the prefix offset + materialize_typeref_packed dispatch) had + // zero public-path coverage. Record-as-last-field is the only + // safe inline position (ADR-006, M1). + let doc = json!({ + "$defs": { + "S": { + "kind": "struct", + "endian": "little", + "fields": [ + { "name": "id", "kind": "uint32" }, + { "name": "counts", "kind": { "kind": "record", "values": "uint32" } } + ] + } + } + }); + let engine = AlkTypeEngine::compile(&doc, "S", LayoutMode::Aligned, None).expect("compile"); + // Offsets: id @ 0 (4 bytes), counts prefix @ 4 (4 bytes). + // Wire: 4 (id) + 4 (record count) + [4 (key len) + 1 (key) + 4 + // (value)] × 2 = 26 bytes. + let mut buf = vec![0u8; 26]; + buf[0..4].copy_from_slice(&7u32.to_le_bytes()); + let mut off = 4; + buf[off..off + 4].copy_from_slice(&2u32.to_le_bytes()); + off += 4; + buf[off..off + 4].copy_from_slice(&1u32.to_le_bytes()); + off += 4; + buf[off] = b'b'; + off += 1; + buf[off..off + 4].copy_from_slice(&10u32.to_le_bytes()); + off += 4; + buf[off..off + 4].copy_from_slice(&1u32.to_le_bytes()); + off += 4; + buf[off] = b'a'; + off += 1; + buf[off..off + 4].copy_from_slice(&20u32.to_le_bytes()); + assert!(engine.validate_bytes(&buf).is_ok()); + // Corrupting bytes inside the first entry (key byte + value + // bytes) produces garbage the value-domain check must reject. + let mut corrupt = buf.clone(); + corrupt[12] = 0xFF; + corrupt[13] = 0xFF; + corrupt[14] = 0xFF; + corrupt[15] = 0xFF; + assert!(engine.validate_bytes(&corrupt).is_err()); + } + // ----- ValidationPlan / validate_bytes (ADR-012 §3, phase 7) ---------- #[test] diff --git a/src/materialize.rs b/src/materialize.rs index 6bb5c89..03422b1 100644 --- a/src/materialize.rs +++ b/src/materialize.rs @@ -1554,6 +1554,47 @@ mod tests { assert_eq!(v["blob"], json!([0xAA, 0xBB, 0xCC])); } + #[test] + fn materialize_aligned_record_last_field_roundtrips() { + // M2: the aligned record dispatch (count-prefixed walk at the + // entry's prefix offset) had zero public-path coverage. Record + // must be the last field (ADR-006, M1) — this pins the happy + // path end-to-end through OffsetMap + materialize_aligned. + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "id", "kind": "uint32" }, + { "name": "counts", "kind": { "kind": "record", "values": "uint16" } } + ] + } + } + }); + let doc = doc_from(&root, "S"); + // Wire: 4 (id) + 4 (record count) + [4 (key len) + 1 (key) + 2 + // (u16 value)] × 2 = 22 bytes. + let mut buf = vec![0u8; 22]; + buf[0..4].copy_from_slice(&7u32.to_le_bytes()); + let mut off = 4; + buf[off..off + 4].copy_from_slice(&2u32.to_le_bytes()); + off += 4; + buf[off..off + 4].copy_from_slice(&1u32.to_le_bytes()); + off += 4; + buf[off] = b'b'; + off += 1; + buf[off..off + 2].copy_from_slice(&10u16.to_le_bytes()); + off += 2; + buf[off..off + 4].copy_from_slice(&1u32.to_le_bytes()); + off += 4; + buf[off] = b'a'; + off += 1; + buf[off..off + 2].copy_from_slice(&20u16.to_le_bytes()); + let v = materialize_aligned_strict(&doc, &buf).expect("materialize"); + assert_eq!(v["id"], json!(7)); + assert_eq!(v["counts"], json!({ "b": 10, "a": 20 })); + } + #[test] fn materialize_aligned_offset_indirect_bytes() { let root = json!({ diff --git a/src/offset_map.rs b/src/offset_map.rs index c7bd8ec..340f8c4 100644 --- a/src/offset_map.rs +++ b/src/offset_map.rs @@ -531,10 +531,16 @@ impl<'d> ComputeCtx<'d> { } } -/// If the field's type is a variable-length primitive, return its kind. +/// If the field's type is a variable-length kind, return its kind. +/// Matches primitive variable-length kinds (String/Bytes) *and* +/// `Record` — whose inline length-prefixed form has the same +/// 4-byte-prefix-then-variable-data shape and the same clobbering +/// hazard the ADR-006 check exists for (review #006 M1: records +/// previously slipped past the check). fn field_variable_kind(field: &BastField) -> Option { match field.ty() { BastType::Primitive(k) if k.is_variable_length() => Some(*k), + BastType::Record(_) => Some(AlkTypeKind::Record), _ => None, } } @@ -938,6 +944,54 @@ mod tests { assert_eq!(m.get("b.x").map(|e| e.range), Some(ByteRange { start: 2, end: 4 })); } + #[test] + fn non_final_record_rejected_in_aligned_mode() { + // M1: a Record field's inline length-prefixed form has the same + // 4-byte-prefix-then-variable-data shape as a string's, so a + // non-final record field must hit the ADR-006 rejection too + // (previously it computed silently corrupt offsets). + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "counts", "kind": { "kind": "record", "values": "uint32" } }, + { "name": "id", "kind": "uint32" } + ] + } + } + }); + let doc = BastDoc::new(&root, "S").expect("doc"); + let err = OffsetMap::compute(&doc).unwrap_err(); + match err { + AlkTypeError::Offset { field_path, reason } => { + assert_eq!(field_path, "counts"); + assert!(reason.contains("ADR-006"), "reason: {reason}"); + assert!(reason.contains("record"), "reason: {reason}"); + } + other => panic!("expected Offset, got {other:?}"), + } + } + + #[test] + fn final_inline_record_allowed_in_aligned_mode() { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "id", "kind": "uint32" }, + { "name": "counts", "kind": { "kind": "record", "values": "uint32" } } + ] + } + } + }); + let m = map(&root, "S"); + assert_eq!(m.get("id"), Some(&OffsetEntry { range: ByteRange { start: 0, end: 4 }, meta: LeafMeta { kind: AlkTypeKind::Uint32, encoding: VariableEncoding::LengthPrefixed, endian: Endian::Little } })); + assert_eq!(m.get("counts"), Some(&OffsetEntry { range: ByteRange { start: 4, end: 8 }, meta: LeafMeta { kind: AlkTypeKind::Record, encoding: VariableEncoding::LengthPrefixed, endian: Endian::Little } })); + assert_eq!(m.total_size(), 8); + } + #[test] fn non_final_inline_string_rejected_in_aligned_mode() { let root = json!({