Fix M1/M2/M3: ADR-006 record gap + aligned record coverage + dead Struct arm (review #006)
M1: field_variable_kind (offset_map) now matches Record — a non-final inline length-prefixed record field in aligned mode hits the ADR-006 rejection instead of computing silently corrupt offsets (probe-verified clobber in the review: counts prefix at 0, id at 4). M2: aligned record path locked with public-path tests — materialize_aligned roundtrip (record<uint16>, wire arithmetic asserted) and engine validate_bytes roundtrip + corrupted-buffer rejection (record<uint32>). Record-as-last-field is the only safe inline position post-M1. M3: read_field's unreachable Struct arm (no struct-path entry ever exists in an OffsetMap) replaced with a documented defensive Offset error; doc comment states struct paths have no entry and the Offset miss is the reachable composite failure. FieldValue::Struct (public API, constructed by the packed reader) untouched. Tests: 7 new (2 offset_map, 2 materialize, 1 engine M3 lock, plus the roundtrip pair). 508 tests green, clippy -D warnings clean, wasm32 build green, cargo doc zero warnings.
This commit is contained in:
1 parent
5e74b991ac
commit
dcfe9d16ff
4 files changed
+247
-17
No files matched your search
@@ -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<uint16>` 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<uint32>`, 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
|
||||
|
||||
+94
-7
@@ -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]
|
||||
|
||||
@@ -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!({
|
||||
|
||||
+55
-1
@@ -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<AlkTypeKind> {
|
||||
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!({
|
||||
|
||||
Reference in new issue
Block a user