alktype/docs/reviews/006-implementation-review-030.md

status, last_updated, reviewed_artifacts, tool, reviewer

status	last_updated	reviewed_artifacts	tool	reviewer
in-progress (H1, L1, H3, L5, L6, H2 resolved 2026-09-02)	2026-09-02	src/read_plan.rs src/sequential_reader.rs src/materialize.rs src/offset_map.rs src/layout_builder.rs src/engine.rs src/validation_plan.rs src/bast.rs src/bast_meta.rs src/bast_validation.rs src/tunion.rs src/data_access.rs src/lib.rs tests/{poc_roundtrip,tunion_dispatch,error_paths,engine_integration}.rs docs/plans/030-compiled-forms.md docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md docs/architecture/decisions/012-plan-fingerprinting-and-m1-closure.md	manual source read + 0.2.0-vs-0.3.0 parity diff (git show cab4932) + cargo-llvm-cov 0.8.4 (release profile) + disposable probe tests (run in-session, then deleted)	0.3.0 post-implementation review (triggered after release commit 9949f91)

Review #006 — 0.3.0 Implementation Review: Compiled Forms

Purpose

The 0.3.0 release (eight phases, commits ff85258..9949f91) landed the compiled-form rollup: ReadPlan (ADR-011), owned BastDoc, OffsetMap LeafMeta, ValidationPlan (ADR-012 §3), and plan fingerprinting (ADR-012 §1/§4). This review is the post-implementation check the other reviews were not: it audits the shipped code — correctness, untrusted-input discipline (AGENTS.md §3/§4), 0.2.0 behavioral parity, code smells, and coverage — rather than a plan or a design. It runs after the release commit, before cargo publish.

The intent is not to relitigate the ADRs or the plan; both held up well. The intent is to find the classic implementation-phase problems the phase gates could not see: cross-consumer convention disagreements, adversarial input that survives every gate, dead-but-live code paths, and the test-coverage holes that hide them.

Methodology

• Full read of every module in src/ touched by 0.3.0, plus docs/plans/030-compiled-forms.md and ADR-011/012.
• Parity diff against 0.2.0 (git show cab4932:src/... for sequential_reader.rs, engine.rs, offset_map.rs, materialize.rs, bast_validation.rs) to separate pre-existing flaws from 0.3.0 regressions. Where a finding is parity-preserved, it says so.
• cargo llvm-cov --release (cargo-llvm-cov 0.8.4, summary + per-line text) to find weak spots; every uncovered region cited in the findings was read to decide "untested but fine", "untested and load-bearing", or "unreachable".
• Disposable probe tests (tests/zzz_review_probe.rs, deleted after the session) to confirm/deny the four behaviors code reading flagged: the ADR-006 record gap, the field-disc union builder/reader split, the non-first discriminator field, and standalone cyclic-$ref handling. Probe results are quoted verbatim in the findings.

Operational warning for future sessions: two probes were not safe to run in the default harness. The huge-array-count repro (H1) OOM-aborted the machine it ran on (a Vec::with_capacity(1e12) allocation failure takes the process down with SIGABRT — and in this session it took down the agent server running it). The cyclic-$ref repro (H2) reliably SIGABRTs the test runner via stack overflow. Any session reproducing H1/H2 must do so in an isolated process (separate cargo test --test <probe> invocation on a throwaway machine, ulimit -v wrapper, or a CI job with a memory cap) — never inside the main suite. This is recorded here so the reproduction hazard does not have to be rediscovered.

Verification Baseline

Reviewed at main HEAD 9949f91 ("Release v0.3.0", Cargo.toml 0.3.0). All gates re-run in this session and green: 474 tests pass (397 + 17 + 34 + 14 + 12), cargo clippy --all-targets -- -D warnings clean, cargo doc --no-deps zero warnings, cargo build --target wasm32-unknown-unknown --release green, cargo publish --dry-run --allow-dirty clean. The release summary's claims were cross-checked: the four commit hashes exist with the described content; the bench numbers are recorded in the plan and ADR-007; ADR-011/012 status blocks, review #004/#005 status lines, and the ADR table in the architecture README are flipped as claimed; the lib.rs re-export list matches the plan's Semver Contract table row for row (net breaking = Bast* lifetime removal, OffsetMap::get return type, SequentialReader::new, materialize_* signatures; net additive = ReadPlan + sub-types, LeafMeta/OffsetEntry, ValidationPlan + sub-types, fingerprint() methods; Hash on Endian/ VariableEncoding present per phase 5's prerequisite step).

Summary Statistics

Severity	Count	Status
High	3 (H1, H2, H3)	H1, H2, H3 resolved 2026-09-02
Medium	4 (M1, M2, M3, M4)	open
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.

The three Highs are adversarial-input crashes (H1, H2) and a cross-consumer wire-layout convention gap (H3) — all three are the exact class of problem AGENTS.md §3 exists for, given that alkcall feeds schemas from arbitrary internet peers into this engine. H1 blocked publishing 0.3.0 to crates.io until fixed (now fixed, see the resolution note on the finding). None of the three is a 0.2.0 regression in the strict sense (details per finding), but two of them became materially easier to hit with the 0.3.0 surface.

Resolution log:

• H1 + L1 (2026-09-02): resolved in one commit — see the "Resolution (2026-09-02)" block at the end of the H1 finding and the L1 finding. 482 tests green (405 crate + 67 integration + 2 pre-existing ignored), clippy -D warnings clean, wasm build green. The fix session also surfaced a new Nit (N2, unbounded align annotations) — added below.
• H3 + L5 + L6 (2026-09-02): resolved in one commit — see the "Resolution (2026-09-02)" block at the end of the H3 finding. Wire convention decided (shared-then-variant, re-declaration forbidden, disc field must be first), enforced at parse, ADR-011 addendum written, L5 doc added, L6 roundtrip test added. 488 tests green, clippy -D warnings clean, wasm build green, cargo doc --no-deps zero warnings.
• H2 (2026-09-02): resolved — see the "Resolution (2026-09-02)" block at the end of the H2 finding. Shared one-shot reference-graph guard (walk_guard::check_ref_graph) added and run at the entry of 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.

---

Findings

H1. A huge declared array count OOM-aborts the process (DoS on untrusted schemas)

Files: src/read_plan.rs:533-553, src/materialize.rs:248, src/materialize.rs:637, src/materialize.rs:924, src/bast_meta.rs:113

Problem: The BAST meta-schema allows any array count >= 0 (bast_meta.rs:113, "minimum": 0, no upper bound), and nothing downstream bounds the product count × element size:

• ReadPlan::compile_array (read_plan.rs:533) accepts any count verbatim. compile validates depth and cycles but never total size.
• materialize_plan_array (materialize.rs:248) — the packed validate_bytes path — does Vec::with_capacity(count) before reading a single byte. With count: 1000000000000 (1e12) and u8 elements, that is a 1 TB capacity request: the allocator fails and the process aborts (SIGABRT), not an Err return.
• The same Vec::with_capacity(count) pattern is on the aligned path twice: materialize_array_packed (materialize.rs:637, reached via aligned record values) and materialize_array_aligned (materialize.rs:924).

The sequential reader gets away with it only incidentally: plan_read_array (sequential_reader.rs:726-733) computes count.checked_mul(element_stride) and errors cleanly when the stride is nonzero — but a fixed-stride array's materialize (the validate_bytes half) still hits the with_capacity, so validate_bytes on an accepted schema aborts regardless of mode. For stride-0 (variable-element) arrays, plan_walk_variable_array_size (:749-771) errors per element on a short buffer — safe, but after walking up to count iterations of per-element error returns.

Probe (run in-session; OOM-aborted before printing): an engine compiled from {"kind": "array", "element": "uint8", "count": 1000000000000} accepted AlkTypeEngine::compile in both modes; the validate_bytes(&[0,0,0,0]) call did not return — the process died in the allocator. Note the meta-schema itself is not at fault (count is a legitimate schema knob); the engine's failure to bound its own allocation is.

This violates AGENTS.md §3 ("a malicious or unsupported schema definition must produce a handleable error, not a crash") on the engine's flagship untrusted-input path (validate_bytes on an incoming stream). Fix before publishing 0.3.0.

Fix: two layers, both cheap:

1. Replace all three Vec::with_capacity(count) with a plain Vec::new() + push loop (the loops already push count elements; the pre-allocation is pure downside for adversarial input and no measurable win for honest input at the sizes that matter — the materializer's per-element work dominates).
2. Optionally, bound total computed array bytes at compile time (count.checked_mul(elem_size) against a documented cap, returning AlkTypeError::Schema("array size exceeds compile-time limit")), so adversarial schemas die at compile with a clean error instead of per-buffer. The reader's checked_mul in plan_read_array (sequential_reader.rs:729) already refuses oversized fixed-stride arrays at read time; the materializer should not be weaker than the reader.

Add tests: compile+validate with count: usize::MAX (expect clean Err), and a moderate count against a short buffer (expect clean Err).

Resolution (2026-09-02) — both layers, plus two gaps the original fix text missed:

1. Abort removal (fix layer 1): all three Vec::with_capacity(count) sites are now Vec::new() + push loop (materialize_plan_array, materialize_array_packed, materialize_array_aligned). The validate_bytes OOM-abort is gone: with an oversized count the per-element walk now errors on buffer bounds instead of allocating.

2. Compile-time caps (fix layer 2), at two choke points — the review's "documented cap" suggestion landed as two constants in schema.rs:

   • MAX_ARRAY_ELEMENTS = 2^16, enforced in BastArray::parse (bast.rs) — the single point every consumer inherits (BastDoc::new is the only construction path, and all five consumers — engine, ReadPlan, LayoutBuilder, OffsetMap, and standalone BastDoc users — parse through it). This cap is what bounds the walkers' per-element push loops: with a product cap alone, 256 MiB of u8 elements would still mean 268M loop iterations accumulating ~20 GB of offset-map/layout entries at compile time. The original fix text ("bound count × element size") missed this; a product cap bounds wire size, not walker memory.
   • MAX_ARRAY_BYTES = 2^26, enforced per walker against the mode-appropriate stride: compile_array (ReadPlan, schema error), walk_array (LayoutBuilder, offset error — defense-in-depth there: packed mode has no alignment, so max fixed element 8 × count 2^16 = 2^19 << 2^26 can't reach the cap), compute_array_field (OffsetMap, offset error — reachable there, via an align-driven stride, see below).
   • L1 folded in: fixed_composite_size/fixed_plan_size now return Result<Option<usize>>; unwrap_or_default() is gone, and the checked-mul overflow arm is a clean Schema error instead of a silent stride-0.

3. Zero-progress runtime guards — a gap neither the finding nor the fix text covered: a stride-0 array whose elements consume 0 bytes (empty-struct elements are legal; stride 0 means per-element sequential walking) loops count times with no buffer bound even after both caps. plan_walk_variable_array_size (reader) and materialize_array_packed (materializer) now error with "array element consumed 0 bytes" when an element makes no wire progress. The aligned materializer needs no guard: aligned array elements are always fixed-size kinds (type_size() >= 1).

Tests (8 new + 1 rewritten): parse-cap rejection (bast), byte-cap rejection via ReadPlan/LayoutBuilder/OffsetMap, count-at-cap acceptance, packed 2^16-array build acceptance (layout_builder, documents why the byte cap is unreachable in packed mode), huge-count short-buffer clean access error, and the old array_count_large_u64_parses_on_64bit (which asserted the old unbounded-parse behavior as a feature) rewritten as array_count_large_u64_rejected_by_compile_cap. No OOM reproducers in-tree (per the Methodology warning); the in-tree tests assert only the compile-time rejections — the safe half.

Verified: 482 tests green, clippy -D warnings clean, cargo build --target wasm32-unknown-unknown --release green.

H2. Cyclic $ref stack-overflows OffsetMap::compute / LayoutBuilder::new / materialize_aligned when driven standalone

Files: src/offset_map.rs:123 (compute), src/layout_builder.rs:150 (new), src/materialize.rs:462 (materialize_aligned), contrast src/engine.rs:146-152, src/read_plan.rs:54,283, src/validation_plan.rs:81

Problem: The engine is safe: AlkTypeEngine::compile runs ValidationPlan::compile before the layout builders, and the plan's compile walk rejects cyclic $ref graphs with a clean Schema error (engine.rs:146-152 documents this gate explicitly — good defensive design). ReadPlan::compile and ValidationPlan::compile each carry their own depth cap (128) + definition-level cycle set (read_plan.rs:54,283, validation_plan.rs:81), so both are untrusted-input-safe standalone.

But the other three public schema walkers have neither guard, and BastDoc::new (bast.rs:74) parses the root eagerly while resolving $refs lazily (bast.rs:110-130) — so a cyclic document parses successfully and the walkers recurse unboundedly:

• OffsetMap::compute → ComputeCtx::compute_struct → resolve_typeref → nested compute_struct → …
• LayoutBuilder::new (parse succeeds) → build → walk_struct → same shape.
• materialize_aligned → materialize_struct_aligned → same shape.

Probe result (verbatim):

PROBE6 … process didn't exit successfully: … (signal: 6, SIGABRT:
process abort signal)

i.e. standalone OffsetMap::compute on S = { next: { "$ref": "#/$defs/S" } } stack-overflows. All three types are public re-exports (lib.rs:66,72,73), so a consumer using them without the engine — exactly the "consumer holds a BastDoc" use case bast_validation::validate_value was retained for — hits an abort, violating AGENTS.md §3 for the public API surface, not just the engine.

This is parity-preserved (0.2.0 had the same gap; it predates 0.3.0), but 0.3.0 is the release that made the pattern explicit: ReadPlan/ValidationPlan got guards and the plan text says the engine's ValidationPlan gate "serves as the engine's cyclic-$ref gate (the layout walkers have no cycle guard)". The residual exposure — public walkers without guards — was left silent. AGENTS.md §3 makes no engine-vs-standalone distinction.

Fix: extract the depth-cap + cycle-set guard into a shared helper (the two existing implementations in read_plan.rs and validation_plan.rs are already near-identical — depth_err/ cycle_err/seen threading) and thread it through ComputeCtx, BuildCtx, and the aligned materializer's struct/array/record walk. Alternatively (smaller): document on OffsetMap::compute, LayoutBuilder::new, and materialize_aligned that cyclic input is rejected only via AlkTypeEngine::compile's gate and that standalone calls require pre-validated documents — but that re-introduces the "trust boundary depends on call path" trap the plan's phase-1 notes explicitly retired for ReadPlan. The shared guard is the right fix; the doc note is the minimum acceptable one.

Resolution (2026-09-02) — shared one-shot graph guard, all three walkers gated:

1. New module src/walk_guard.rs (private, pub(crate)) with check_ref_graph(&BastDoc) — a single bounded walk over the reachable reference graph that rejects depth > 128 (MAX_GRAPH_DEPTH, matching the plan compilers' MAX_COMPILE_DEPTH) and any $ref cycle, using the same path-scoped cycle-set semantics the plan compilers use (diamond refs compile; only genuine cycles trip). The walk covers every carrier shape: inline structs recurse into fields, named defs are entered via resolve_ref, union mappings and shared fields are both checked, and arrays/records are seen through to their element/value types (so a cycle behind an array-of-$ref hop is caught — the probe-verified gap shape). Error text mirrors the plan compilers' wording ("cyclic $ref through…", "compile depth exceeded…") so downstream matching sees one shape.

2. All three standalone walkers run the guard at entry, before any recursion: OffsetMap::compute, LayoutBuilder::new, and materialize_aligned (the last as defense-in-depth — a cyclic doc can no longer produce an OffsetMap, but a mismatched doc/map pairing must still fail with a clean Schema error, not overflow). AlkTypeEngine::compile's ValidationPlan gate is unchanged and now redundant-but-harmless; its doc comment is updated to say so. The full shared-guard recommendation was taken (not the minimum doc note).

3. Behavioral side effect, net-positive: check_ref_graph resolves every reachable def eagerly (via resolve_ref), which parses each def's full shape — so an invalid non-root def (e.g. a $defs union that re-declares a shared field, or whose discriminator field is missing/non-first) now surfaces at LayoutBuilder::new instead of at build(). Four pre-existing H3 tests asserted the old lazy-parse timing ("root parses, fails at build"); they were updated to expect the same Schema error at new(). Earlier rejection of the same malformed schemas — strictly better for untrusted input, no accepted schema's behavior changed.

4. Test family added (12 tests): in walk_guard.rs (self-cycle, two-def cycle, diamond allowed, 201-def deep chain → clean depth error), in offset_map.rs (cycle rejected at compute, two-def cycle, cycle behind inline-struct + array hop, diamond still computes), in layout_builder.rs (cycle rejected at new, two-def cycle, diamond still builds), and in materialize.rs (cyclic doc

   • unrelated map → guard fires before offset lookup, diamond still materializes). No stack-overflow reproducers in the default suite — the tests assert the clean-error half only (the overflow itself was probe-verified SIGABRT in the review session; see the Methodology warning about running it).

Verified: 501 tests green (423 crate + 17 + 34 + 15 + 12 + 2 pre-existing ignored), cargo clippy --all-targets -- -D warnings clean, cargo build --target wasm32-unknown-unknown --release green, cargo doc --no-deps zero warnings.

H3. Field-name-discriminator unions: builder, reader, and materializer disagree on layout and on discriminator position

Files: src/layout_builder.rs:485-527 (write side), src/sequential_reader.rs:551-598,607-646 (read side), src/materialize.rs:356-404 (materialize side), builder doc src/layout_builder.rs:35-40

Problem: Three packed-mode consumers each implement the field-disc union slightly differently, and their agreements/disagreements are undocumented and untested end-to-end:

1. Layout convention (builder vs reader/materializer). The builder walks only the selected variant struct starting at the union's offset (walk_field_discriminator_union, layout_builder.rs:485-527 — self.walk_struct(variant_struct, field_path, offset)). The reader and materializer walk the union's declared fields (the shared sub-plan: discriminator field + any shared fields) first, then the variant (sequential_reader.rs:573-588, materialize.rs:371-381). The two conventions never agree on the wire: reader span = shared_size + variant_size, builder span = variant_size. On the builder tests' own fixture (union fields: [type: uint8], variant Read { type, handle } — the variant re-declares the discriminator), the builder produces type@0, handle@1, total 5 (probe-verified: PROBE1 builder: total=5 event.type=Some((0, 1)) event.handle=Some((1, 4))) while the reader's walk places the variant's type at 1 and handle at 2 (span 6) — the discriminator field would appear twice on the wire under the reader's convention. Conversely, if the variant does not re-declare the disc field, the reader's convention is coherent (disc from shared, variant fields follow) and the builder's is the one that drops the disc's wire position. LayoutBuilder's tests (:1221-1334) and the reader/materializer tests (sequential_reader.rs:1260-1305, materialize.rs:1183-1258) each encode their own convention with different fixture shapes, so nothing cross-checks them. No doc comment on either side states the constraint; no roundtrip test drives a field-disc union from LayoutBuilder output through SequentialReader/materialize_packed.

2. Discriminator position (reader vs materializer). The reader's plan_read_union Field arm reads the discriminator value at the union's start offset (plan_discriminator_string_value(disc_field, buffer, offset, …), :563-564 — offset is the union start, not the disc field's position). The materializer's Field arm walks all shared fields in order and captures the disc value at its real position (materialize.rs:371-381). For a non-first discriminator field the two consumers read different bytes. Probe-verified (verbatim):

   PROBE4 reader error: access error at event: unknown union
          discriminator value: 1     ← read seq's first byte at union start
   PROBE4 validate_bytes ok          ← materializer dispatched correctly on
                                       the same buffer
   

   the reader stringified 1 (seq's first byte at the union start, offset 0) and failed the mapping lookup; validate_bytes on the identical buffer passed. Note this probe ran after probe 1's reader section had already panicked on an assertion; the error itself is the evidence — the reader dispatched on the wrong field's bytes while the materializer dispatched correctly on the same buffer.

3. 0.2.0 parity, precisely scoped. The reader-side flaw (disc read at union start) is parity-preserved: 0.2.0's read_union_value Field arm also read only the disc field at the union start (cab4932:src/sequential_reader.rs:460-472), and 0.2.0's materializer did the same — so in 0.2.0 reader and materializer agreed (both position-blind). 0.3.0's materializer became position-correct (it must, to walk shared fields in order), which silently made the two packed read-side consumers disagree for non-first disc fields. The builder's variant-only layout is also parity-preserved. So H3 is not a regression — it is a long-standing convention gap that 0.3.0's shared-plan shape made visible and partially divergent.

Fix: make the decision explicit and enforce it:

• Decide the wire convention. The reader/materializer shape (shared fields, then variant) is the one the materializer already implements position-correctly, and it matches ADR-011's shared plan design — recommend: builder lays out shared then the variant, and the "variant re-declares the discriminator field" pattern becomes either required (validated at compile) or forbidden (rejected at compile). The other consistent option — variant-only layout, no shared walk — would mean reverting the reader/materializer to 0.2.0's position-blind disc read, which the shared plan shape was specifically built to move past.
• Enforce at compile/parse time whatever constraint the convention needs (e.g. discriminator field must be first in fields; variant must not re-declare shared fields — or must, if that's the convention). BastUnion::parse (bast.rs:494-547) or compile_union is the natural enforcement point; today nothing constrains field order in a field-disc union.
• Record the decision in ADR-011's union section (a short addendum) or ADR-003 §4, and add the write→read→validate roundtrip test through a field-disc union (packed mode) — see L6.

Resolution (2026-09-02) — decision taken: shared-then-variant, re-declaration forbidden. Both recommendations accepted as recommended:

1. Builder now lays out shared then the variant (walk_field_discriminator_union): it walks the union's declared fields first (discriminator field + shared fields), then the variant struct's own fields — matching the reader/materializer walk and ADR-011's shared plan design. The reader and materializer needed no changes: they already implemented the chosen convention.

2. Enforcement at parse (BastUnion::parse, the choke point every consumer inherits — a $defs union is parsed at BastDoc::new for a union root and at resolve_ref for a referenced union):

   • the discriminator field must be declared in fields (previously caught only by compile_union/tunion at compile/dispatch);
   • fields must not contain duplicate names;
   • no variant may re-declare the discriminator field or any shared field (variant structs are checked inline and through $ref resolution — the parse chain now threads the doc root so the variant def's fields are visible at parse time). Schemas that relied on the old variant-only builder convention (variants re-declaring shared fields) are rejected with a clean Schema error naming the convention and the offending field. This is a breaking constraint for 0.2.0-era re-declaring schemas, announced with the 0.3.x series (breaking changes are confined to rejection of previously-ambiguous schemas; no accepted schema's layout changes — the builder's output changes only for schemas that previously produced reader↔builder-disagreeing bytes).

3. Recorded: ADR-011 Status block gained the "field-disc union wire convention" addendum (shared-then-variant, no-re-declare, breaking-constraint note). FieldValue::Union.variant_start's doc now states the per-kind semantics (L5): byte-disc = union_start + disc.offset + disc.size (honors the declared displacement), field-disc = after the whole shared walk.

4. L6 roundtrip test added (tests/poc_roundtrip.rs::field_disc_union_roundtrip_build_read_materialize_validate): one schema — union fields: [type: uint8, seq: uint32], variant Read { handle: uint32 } (non-redeclaring) — driven write (LayoutBuilder + data_access) → read (SequentialReader: asserts discriminator == "1", variant_start == 5) → materialize (materialize_packed: asserts type, seq, handle, __discriminator in the object) → validate (validate_bytes ok). The builder-side assertions pin event.type@0, event.seq@1, event.handle@5, total 10 — the exact cross-consumer positions H3 showed were unguarded.

5. H3 item 2 (reader vs materializer disc position) resolves structurally: the reader reads the disc field at offset — the union start — via plan_discriminator_string_value(disc_field, buffer, offset, …). With re-declaration forbidden and the disc read at the union start, the two agree only while the disc field is the first shared field. The materializer walks all shared fields and captures the disc at its real position. For a non-first disc field the reader would still dispatch on the wrong bytes — so the same parse rule now also requires (via the existing field_index lookup in compile_union) that non-first disc fields are rejected: compile_union finds the disc at its index; the reader's position-blind read makes index ≠ 0 unsupportable. Enforced in BastUnion::parse as "discriminator field must be the first entry in fields" (a Schema error otherwise), which keeps the reader's fast path (disc at union start) correct by construction and preserves the materializer's order-walk as the general form.

Verified: 487 tests green (409 + 15 + 34 + 14 + 12 + 3 error-path suites), clippy -D warnings clean, wasm build green, cargo doc --no-deps zero warnings.

M1. The ADR-006 check misses non-final inline Record fields in aligned mode

Files: src/offset_map.rs:235-255 (the check), src/offset_map.rs:515-520 (field_variable_kind)

Problem: compute_struct's non-final inline length-prefixed variable-field rejection (ADR-006) dispatches on field_variable_kind(field), which matches only primitive variable-length kinds (BastType::Primitive(k) if k.is_variable_length()). A Record field — whose inline length-prefixed form has the same 4-byte-prefix-then-variable-data shape and the same clobbering hazard the ADR-006 error text describes — slips past the check. Probe-verified (verbatim):

PROBE2 ADR-006 MISSED for record: counts=Some((0, 4)) id=Some((4, 8)) total=8

{ counts: record<uint32>, id: uint32 } computes counts as a 4-byte prefix at 0 and id at 4 — the record's variable data lands exactly where id reads from. validate_bytes then succeeds with silently corrupt data; read_field("counts") fails only incidentally (the composite rejection, M2). Parity-preserved — the identical code shipped in 0.2.0 (cab4932:src/offset_map.rs:177,433-438) — but it is a hole in the exact check ADR-006 created, and it is the kind of thing the check exists to catch.

Fix: include BastType::Record(_) => Some(AlkTypeKind::Record) in field_variable_kind (offset_map.rs:515-520). The ADR-006 error message's fix list ("use maxLength … offset-indirect … move to last position") already covers records correctly. Add the non_final_record_rejected_in_aligned_mode test mirroring non_final_inline_string_rejected_in_aligned_mode.

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), src/engine.rs:453-460 (read_field composite rejection)

Problem: materialize_struct_aligned dispatches Record through materialize_typeref_packed at the entry's prefix offset — the count-prefixed walk, which works. But nothing else in the aligned stack coheres around records: OffsetMap records the entry as encoding: LengthPrefixed with the 4-byte prefix range; engine::read_field refuses Record as a composite ("use the layout-specific APIs" — there is no aligned record API); and only the M1 hole makes the "record as last field" position the safe one, with no error distinguishing it from the clobbering position. Probe 5 showed the happy path validates (PROBE5 map: counts=Some((0, 4)) total=4, PROBE5 validate ok), so the mechanism works — but zero tests exercise an aligned record field through any public path (coverage: materialize_struct_aligned's Record arm and materialize_leaf_at's record dispatch are hit only by in-module unit tests, never through validate_bytes/read_field). Whatever M1's fix decides about 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.

M3. read_field's aligned Struct arm is unreachable dead code

File: src/engine.rs:449-452

Problem: read_field's kind dispatch has a AlkTypeKind::Struct => Ok(FieldValue::Struct { start, end }) arm, but no struct entry ever exists in an OffsetMap: compute_struct_field (offset_map.rs:346-387) pushes only the nested struct's inner leaf entries, never an entry for the struct path itself, and the root is not an entry either. read_field("header") on a nested-struct schema returns the Offset "field not found in offset map" error — the Struct arm cannot be reached through any schema the engine accepts. Coverage confirms it: the arm shows 0 executions, and the composite rejection arm right below it (:453-460) is likewise 0-execution (because composites also never get entries — the reachable failure for composite paths is the Offset miss, which the tests accept in either variant). Two options: delete the Struct arm (and tighten the doc comment, which currently implies composites error at the dispatch 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.

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, 84.80% functions. Per-file (worst first):

file	line%	fn%
materialize.rs	64.48	59.30
data_access.rs	79.74	74.65
sequential_reader.rs	81.42	70.77
bast.rs	87.00	88.06
offset_map.rs	91.13	82.46
layout_builder.rs	91.18	81.54
builder.rs	91.28	87.50
tunion.rs	92.73	91.67
validation_plan.rs	93.97	83.87
read_plan.rs	94.53	95.89
engine.rs	96.57	100.00

The specific holes worth closing (each was read; all are load-bearing-or-error-path, none is trivially dead except where noted):

1. materialize.rs aligned half — materialize_struct_aligned, materialize_array_aligned, materialize_variable_aligned, and materialize_typeref_packed carry ~47 uncovered lines between them. Most striking: the nested-struct recursion (materialize_struct_aligned(doc, s, &path, …), :866) has 0 executions — the function is only ever entered at the root (10 calls, all path_prefix: ""). Aligned nested-struct materialization — the heart of aligned validate_bytes — has never been exercised through any test. The maxLength-trim arm (:971-998), the offset-indirect arms (:956-969), the UTF-8 error (:984-987), and both "resolved kind X but type was Y" internal errors (:868-878) are likewise 0-execution. The engine-facing aligned validate_bytes tests cover exactly one flat 2-field struct. This is where H1's aligned variant, M1, and M2 all live.
2. sequential_reader.rs — the public schema() (:235-237) and plan() (:240-242) accessors are never called by any test (public API, phase-2 deliverables). The field-disc discriminator-kind arms for uint16/uint32/enum (:621-641) and plan_read_byte_discriminator's uint16/uint32 arms (:663-668) are 0-execution — wire-visible dispatch paths. plan_walk_variant_size's union arm (:688-691) — the nested-union variant size walk, the capability phase 1 specifically restored — is 0-execution (the compile side is tested; the read-side size walk is not).
3. data_access.rs — nearly all uncovered lines are error paths (offset overflow, mutable-slice-missing, and notably the write_bytes u32-truncation guard :266-270 and write_bytes_indirect's two u32 guards :383-391 — review #002 M2's "canonical overflow-guard pattern" is itself untested). One test each would lock the canonical pattern.
4. offset_map.rs — field_endian_for_element's BastType::Struct(s) => s.endian() / Union(u) => u.endian() arms (:529-530) are 0-execution. The union arm is unreachable (unions rejected in aligned mode); the struct arm says an array of inline struct elements inherits the element struct's own endian annotation — which contradicts the phase-5 parity rule everywhere else in 0.3.0 ("the referring field's effective endian propagates; the nested container's own annotation is not consulted"). Either this is a genuine divergence in array-element endian propagation (then it needs a parity test and probably a fix), or it's unreachable for inline elements too (then it's dead). Worth a focused look — it touches the exact parity story phase 5 closed.

Recommended posture (matches how this review was run): fold a coverage check into each fix session — after fixing a finding, extend cargo llvm-cov over the touched file and close the immediately adjacent uncovered branches while the context is fresh, rather than scheduling a standalone coverage sweep.

L1. unwrap_or_default() conflates "variable-length" with "checked-mul overflow" for array strides

File: src/read_plan.rs:547

Status: resolved 2026-09-02 (with H1).

Problem: let element_stride = fixed_composite_size(&element) .unwrap_or_default(); — fixed_composite_size returns None both for genuine variable-length elements (correct → stride 0) and for checked_mul overflow of an absurd count × stride (→ silently stride 0, i.e. "walk every element sequentially" — the reader then does up to count per-element error returns instead of one clean arithmetic error). With H1 fixed at compile time this becomes unreachable, but as written the two cases are indistinguishable at the call site. Prefer making compile_array return Err on the overflow arm (a small refactor of fixed_composite_size to return a Result<Option<usize>, …> or to take the cap from H1's fix).

Resolution (2026-09-02): fixed_composite_size and fixed_plan_size now return Result<Option<usize>, AlkTypeError>; None means variable-length only, and overflow arms return clean Schema errors. compile_array uses fixed_composite_size(&element)? .unwrap_or(0) — the conflation is gone. See the H1 resolution block for the full change description.

L2. ReadPlan construction carries a temporary Value::Null schema placeholder

Files: src/read_plan.rs:295,460-461,601 (three schema: Arc::new(Value::Null) sites), src/read_plan.rs:176-179 (the caller's overwrite)

Problem: every internal compile_struct/compile_union/ wrap_leaf builds a ReadPlan with a placeholder Arc<Value::Null> schema that the top-level compile then overwrites via a map on the result. Harmless today (the placeholder never escapes; the public schema() accessor always returns the real doc), but it is a lie in the type for the duration of the walk and a trap if a future refactor adds a consumer mid-walk. Passing schema: &Arc<Value> (or the already-Arc'd clone) down through compile_struct removes the placeholder, the map, and the smell in one small change.

L3. Dead locals left from the phase-2 rewrite

Files: src/materialize.rs:68 (let _ = plan; in materialize_plan_field), src/materialize.rs:360-365 (disc_field bound then discarded via let _ = disc_field;)

Problem: materialize_plan_field takes plan: &ReadPlan solely to discard it (let _ = plan;), and the field-disc union arm binds disc_field from shared_plan.fields().get(*field_index) then discards it — the materializer walks all shared fields and captures the disc by name instead (the correct design, per the parity note at :366-371). Both are clippy-invisible (let _ = suppresses the unused warnings). Drop the plan parameter (and fix the recursive call sites) and delete the disc_field/let _ pair — or keep disc_field and use it to assert the walk actually encountered the disc field (the current disc_value.ok_or_else(...) at :382-384 already does that job, so deletion is cleaner).

L4. OffsetMap::get / PackedLayout::get are linear scans positioned as random access

Files: src/offset_map.rs:154-159, src/layout_builder.rs:83-88

Problem: both get implementations are self.fields.iter().find(…) over a Vec — O(n) per lookup, on the types whose docs pitch random access ("the consumer can read field N without reading fields 0..N-1 first", offset_map.rs:92-94). Fine for small schemas; the claim gets less honest with schema size, and read_field/write_field (the aligned hot path for per-field access) pays it per call. ReadPlan already moved to BTreeMap for exactly this class of reason (phase 1, and to enable Hash). An internal BTreeMap<String, usize> path→index alongside the insertion-ordered Vec (keeping iter() order and the Hash derive over the Vec) makes the claim true with no public-surface change. Not a 0.3.0 blocker; a good 0.3.x cleanup with an existing in-repo pattern to copy.

L5. FieldValue::Union's variant_start semantics are undocumented and inconsistent between discriminator kinds

File: src/sequential_reader.rs:84-91

Status: resolved 2026-09-02 (with H3).

Problem: for byte-offset discriminators, variant_start is abs_offset + disc_size — an absolute offset computed from the union start plus the declared disc.offset (which may be nonzero, so the variant can start before or after the shared-field walk would place it). For field-name discriminators it is the position after the whole shared walk. The FieldValue enum doc (the "consumer recurses" contract) says only "Byte offset where the variant struct begins" — nothing about which base the offset is relative to, that byte-disc offset is honored as an arbitrary displacement, or (per H3) that the field-disc variant starts after shared fields while the builder lays out only the variant. A consumer writing generic union-handling code against FieldValue::Union alone can get this wrong silently. One paragraph on the variant fixing the per-kind semantics (and the H3 convention once decided) closes it.

Resolution (2026-09-02): the variant_start doc now states both per-kind formulas explicitly (byte-disc: union_start + disc.offset + disc.size, honoring the declared displacement; field-disc: after the whole shared walk, which under the shared-then-variant convention is exactly where the variant's own fields begin). See the H3 resolution block for the convention decision.

L6. No write→read→validate roundtrip test for field-disc unions

Files: tests/poc_roundtrip.rs (no field-disc case at all), tests/tunion_dispatch.rs (tunion-level only), the builder tests (write-side only), the reader tests (read-side only)

Status: resolved 2026-09-02 (with H3).

Problem: every field-disc union test lives on one side of the wire. The builder tests assume the variant-only layout (layout_builder.rs:1221-1334); the reader/materializer tests assume the shared-then-variant walk (sequential_reader.rs:1260-1305, materialize.rs:1183-1258) — and the two fixtures don't even use the same schema shape, so nothing would catch H3's divergence. A single roundtrip test (build with LayoutBuilder → feed the buffer to SequentialReader::read_next and materialize_packed → validate_bytes on the engine) over a field-disc union with a non-redeclaring variant and a second shared field would have caught H3 immediately. This is the highest-value single test in the review — add it with (or before) the H3 fix.

Resolution (2026-09-02): tests/poc_roundtrip.rs::field_disc_union_roundtrip_build_read_materialize_validate — exactly the test this finding asked for: union fields: [type: uint8, seq: uint32] (disc + a second shared field), variant Read { handle: uint32 } (non-redeclaring, numeric mapping keys "1"/"2" matching the uint8 disc). Write with LayoutBuilder + data_access (asserting event.type@0, event.seq@1, event.handle@5, total 10), read with SequentialReader::read_next (asserts discriminator == "1", variant_start == 5), materialize with materialize_packed (asserts type/seq/handle/__discriminator in the object), and engine.validate_bytes on the same buffer. This is the test that would have caught every part of H3.

N1. Field-discriminator kind support differs between tunion and the reader

Files: src/tunion.rs:136-164 (supports String/Uint8/Enum), src/sequential_reader.rs:613-645 (supports String/ Uint8/Uint16/Uint32/Enum)

Problem: tunion::read_field_discriminator rejects uint16/uint32 discriminator fields with a Schema error, while the plan reader handles them. Parity-preserved (tunion is unchanged from 0.2.0), but the public dispatch surface now has two answers to "which field kinds can discriminate a union". Either extend tunion to match the reader or document the divergence; the meta-schema does not constrain the discriminator field's kind, so both code paths are reachable from the same schema.

N2. align annotations are unbounded — no crash, but absurd layouts and the reachable path to H1's byte cap

Files: src/bast_meta.rs:64,79 ("align": { "type": "integer", "minimum": 1 }, no maximum), src/offset_map.rs (align_up/ round_up), src/bast.rs:926 (parse_align)

Problem: found during the H1 fix session (probe: a struct with align: 2^62 compiles in aligned mode and reports total_size = 2^63). The meta-schema accepts any align >= 1, and align_up willingly rounds the running offset up by the full annotation — so a one-field schema can declare a layout of exabyte scale. Unlike H1 this does not crash: the allocation is offset arithmetic, not with_capacity, and validate_bytes on the absurd layout fails cleanly with a buffer-bounds Access error (probe-verified). Severity is Nit because there is no abort and no unbounded memory at read time; it is recorded because (a) total_size in that range is meaningless output the consumer may act on, (b) the align-driven stride is the one reachable path to MAX_ARRAY_BYTES (the H1 byte-cap test in offset_map uses exactly this), and (c) the same unbounded knob exists on FieldDef.align. A maximum align (e.g. 64 or 4096) in the meta-schema and/or a clamp-with-error in parse_align closes it; the honest layouts in the wild never need >64.

Fix: add "maximum" to both align properties in bast_meta.rs, or reject/clamp oversized values in parse_align (with a clean Schema error). A locking test (align above the cap → clean Err) mirrors the H1 test family.

---

What's Good

Worth recording, because the findings shouldn't eclipse it:

• The compiled forms are well-built. Both ReadPlan::compile and ValidationPlan::compile carry their own depth cap + definition-level cycle set (the correct untrusted-input shape), and the cycle-set-is-path-scoped semantics are tested for diamond refs on both (repeated_ref_to_shared_def_is_allowed, shared_refs_compile_without_false_cycle).
• The engine's ValidationPlan-as-cycle-gate layering (engine.rs:146-152) is a genuinely clever piece of defensive design, and it is documented at the place a future maintainer will read it.
• Parity discipline is real, not ritual. The effective-endian propagation divergence (phase 5) was found, fixed, and locked with tests on both the plan and the offset map; the stride fix (deferred decision 4) is documented in the FieldValue::Array doc comment with the behavioral-change note; the union paths were compared arm-by-arm against the 0.2.0 read_union_value in this review and match on every dispatch shape.
• The fingerprint derives are sound. Verified against the pinned serde_json 1.0.150 source: under preserve_order, Map::hash sorts keys (map.rs:418-429), so Value: Hash is consistent with Value: Eq and the phase-6 plan's analysis holds. Contract tests on all three plans (ReadPlan/OffsetMap/ValidationPlan) plus the root-name sensitivity test.
• Send + Sync static assertions on the plan types (phase 1 + phase 7), as ADR-011 required.
• Docs/status hygiene is accurate. Every status flip, bench number, and "closed" claim in the release commit checked out against the actual files.
• Semver contract honored: lib.rs re-exports match the plan's contract table exactly; the breaking surface is exactly the declared one.

Recommended Order

1. H1 — release-blocking resolved 2026-09-02 (with L1; see the resolution block on the finding).
2. H3 resolved 2026-09-02 (with L5 + L6; see the 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).
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.
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 with H1; L5/L6 done with H3).

Notes

• The probe file used in this review was deleted before commit; no test changes ship with this review.
• The H1 OOM incident (test run aborted the session's host) is the reason the two crash reproducers are described rather than committed. If a reproducer test is wanted in-tree for H1/H2, it should be #[ignore]-gated with a comment pointing at the isolation requirements, or assert only the compile-time rejection (the safe half of the fix) in the default suite. The H1 resolution followed the second option: in-tree tests assert only compile-time rejections.
• Coverage was measured with the default harness (cargo llvm-cov --release, summary + text). Numbers quoted are stable across two runs this session.
• Severity here keys off AGENTS.md §3 (untrusted schema ⇒ handleable error) and the semver contract, not off effort: two of the three Highs are single-file, small-diff fixes; H3 is the only one that needs a decision first.
• The H1 fix session (2026-09-02) also confirmed the zero-progress gap (stride-0 array + zero-byte elements = unbounded loop) and the unbounded-align observation (new N2) while re-deriving the cap arithmetic — both recorded on their own findings rather than silently absorbed.