alktype/docs/reviews/007-coverage-audit.md

status, last_updated, reviewed_artifacts, tool, reviewer

status	last_updated	reviewed_artifacts	tool	reviewer
resolved (F1, F2, C1, C2, C3, L1, L2 resolved 2026-09-02; N1/N2a/N3a/N4a are classified-no-action / deferred-by-design)	2026-09-02	src/materialize.rs src/sequential_reader.rs src/read_plan.rs src/offset_map.rs src/layout_builder.rs src/engine.rs src/data_access.rs src/bast.rs src/builder.rs src/tunion.rs src/validation_plan.rs src/walk_guard.rs src/bast_meta.rs tests/poc_roundtrip.rs tests/tunion_dispatch.rs tests/error_paths.rs tests/engine_integration.rs docs/reviews/006-implementation-review-030.md (post-fix coverage re-check)	cargo-llvm-cov 0.8.4 (--release, per-line text) + manual classification of every uncovered production line + disposable probe tests (run in-session, then deleted)	post-review-#006 coverage audit (session request — check test coverage for weak spots, meaningful tests, non-happy-path posture)

Review #007 — Post-#006 Coverage Audit

Purpose

Review #006 closed every finding and its M4 coverage map, but the session-level posture (M4's item: "fold a coverage check into each fix session") had never been run as a whole-tree pass after all those fixes landed. This audit re-measures coverage after the eleven #006 commits, reads every uncovered production line, and classifies it — the same "untested-but-fine / load-bearing / unreachable" discipline M4's map used. Two probes were run in disposable tests (deleted after the session, per #006's no-reproducer rule; neither was a crash hazard — both reproduce cleanly inside the default harness).

Methodology

• cargo llvm-cov --release (0.8.4, same tool as #006): summary + per-line text. TOTAL 90.67% lines / 86.32% functions — stable with #006's post-M4 numbers (90.60%), the expected drift after the N3 fix sessions added parse gates + tests.
• Per-file (worst first): materialize.rs 85.72, data_access.rs 80.32, sequential_reader.rs 86.19, bast.rs 87.41, builder.rs 91.28, layout_builder.rs 91.42, tunion.rs 92.02, offset_map.rs 92.93, read_plan.rs 90.69, engine.rs 96.44, validation_plan.rs 93.97, walk_guard.rs 98.04, bast_meta.rs 98.92, bast_validation.rs/error.rs/schema.rs/ macros.rs/validation.rs 100.
• Every uncovered line outside #[cfg(test)] modules (806 raw lines) was read and classified. Lines inside test modules (the panic!("expected X, got {other:?}") helpers) were excluded — they distort per-file numbers (e.g. bast.rs's 87.41% is really ~96% production once its 60 helper lines are excluded).
• Two suspicions were probe-verified with disposable tests: the F1 cross-consumer divergence and the F2 unbounded-maxLength compile. Probe transcripts quoted verbatim in the findings.
• Happy-path posture audit: cross-checked which error arms adjacent to covered code are 0-execution, and which public surfaces have only success-path tests.

Baseline

Audited at main HEAD bb28ba3 ("Resolve N3"), 0.3.0, working tree clean. Full suite green (548 tests static + 2 ignored doctests, per #006's bookkeeping).

Summary Statistics

Severity	Count	Status
High	2 (F1, F2)	both resolved 2026-09-02
Medium	3 (C1, C2, C3)	all resolved 2026-09-02
Low	2 (L1, L2)	all resolved 2026-09-02
Info	4 (N1, N2a, N3a, N4a)	classified: N1 artifact, N2a/N4a no-action, N3a deferred to pre-release review

Resolution log:

• F1 + F2 (2026-09-02): resolved in one commit — see the resolution blocks on each finding. 477 lib tests green (511 static + 2 ignored doctests across all targets), clippy -D warnings clean, wasm build green.
• C1 + C2 + C3 (2026-09-02): resolved in one commit — see the resolution blocks. 481 lib tests green, clippy -D warnings clean, wasm build green; compile_variant's cycle arm confirmed executed in the post-fix coverage run.
• L1 + L2 (2026-09-02): resolved in one commit — see the resolution blocks. 488 lib tests green, clippy -D warnings clean, wasm build green.

---

Findings

F1. Zero-progress guard missing in the plan materializer — validate_bytes accepts what SequentialReader rejects (cross-consumer divergence)

Files: src/materialize.rs:227-253 (materialize_plan_array — no guard), contrast src/sequential_reader.rs:772-795 (plan_walk_variable_array_size — has the guard) and src/materialize.rs:636-663 (materialize_array_packed — has the guard)

Problem: The H1 fix session added the zero-progress runtime guard ("array element consumed 0 bytes") to two of the three array walkers: the compiled reader's variable-array size walk and the legacy BAST walker's packed array arm. The plan-based packed materializer — materialize_plan_array, the walker validate_bytes actually uses in packed mode (engine.rs:310-312) — got no guard.

A stride-0 array whose elements consume 0 bytes (empty-struct elements are legal: the meta-schema's StructDef has no minItems on fields) compiles with element_stride: 0 and loops count times materializing empty objects without reading a single buffer byte:

PROBE validate_bytes([]): OK — zero-progress guard MISSING in plan materializer
PROBE reader.read_next([]): Err(access error at items[0]: array element 0 consumed 0 bytes;
      a zero-size element makes the declared count unbounded on the wire)

Schema: { "items": { "kind": "array", "element": { "kind": "struct", "fields": [] }, "count": 8 } }, packed mode, empty buffer. Same schema, same buffer, opposite verdicts — the exact cross-consumer-disagreement shape review #006 existed for (H3, M6). Severity High by #006's own keying (AGENTS.md §3): validate_bytes is the flagship untrusted-input path, and it silently accepts a buffer the same engine's reader rejects. The H1 resolution text ("plan_walk_variable_array_size (reader) and materialize_array_packed (materializer) now error") lists only two of the three walkers — the plan materializer was missed because it is not the legacy walker that finding named.

Not a #006 regression: the H1 fix text itself specified only the reader and legacy-walker sites; the plan materializer predates the guard and was outside that fix's blast radius. But the divergence is new information — the guard's invariant ("a zero-progress element makes the declared count unbounded") belongs to the array-walk concept, not to two specific functions.

Fix: hoist the same guard into materialize_plan_array's loop (compare *offset before/after materialize_plan_composite; error with the same wording the other two walkers use so downstream matching sees one shape). Add a locking test driving the same schema through BOTH paths asserting the verdicts agree (both reject an empty buffer; both accept a buffer where the elements make progress — empty-struct elements never do, so the acceptance half needs a non-empty variant struct alongside).

Resolution (2026-09-02): the guard, hoisted verbatim from the two existing sites (*offset == before after the element walk, same "array element {i} consumed 0 bytes…" wording so downstream matching sees one shape). Tests (3, in materialize.rs): f1_zero_progress_array_rejected_by_all_three_walkers (plan materializer + the record-value fallback path, both asserting the Access error with the guard's wording), f1_validate_bytes_and_reader_agree_on_zero_progress_array (the cross-consumer agreement the probe showed was missing — validate_bytes and SequentialReader::read_next both reject the same schema+buffer with the same error class), f1_nonempty_variant_struct_array_still_materializes (the false-positive check: elements that consume bytes still walk). Verified: 477 lib tests green, clippy -D warnings clean, wasm build green.

F2. maxLength is unbounded — the N2 analog

Files: src/bast_meta.rs:81 ("maxLength": { "type": "integer", "minimum": 0 } — no maximum), src/bast.rs:1038-1043 (parse_max_length — no cap, and silently drops non-usize values), contrast src/schema.rs MAX_ALIGN/parse_align (the N2 pattern)

Problem: N2 bounded align at 4096 with a clean parse error plus a meta-schema "maximum". maxLength has the identical shape and was not covered by that fix:

PROBE aligned maxLength 1e12 compiles; total_size = 1099511627776

A one-field schema declares a 1 TiB reservation; total_size in that range is meaningless output the consumer may act on (N2's argument (a)). Unlike align, no Access error follows at read time (an empty buffer still fails buffer bounds first), so this is layout-meaningless output, not a crash — the exact severity N2 recorded. Additionally, parse_max_length returns Option and silently drops values that overflow usize (.and_then(|n| usize::try_from(n).ok())) — on a 32-bit target a 5 GiB maxLength becomes "no maxLength", changing layout semantics without telling the consumer.

Fix: the N2 playbook verbatim. A MAX_LENGTH cap in schema.rs (value TBD — align's 4096 is page granularity; a reservation cap in the tens-of-megabytes range fits honest layouts; suggest 2^26 = 67_108_864, matching MAX_ARRAY_BYTES's rationale), enforced in parse_max_length (converted to Result<Option<usize>>, clean Schema error naming the path/value/maximum — no silent drop), plus "maximum": 67108864 in the meta-schema's maxLength property so the published contract matches the parser (the N2 dual-layer pattern).

Resolution (2026-09-02): the N2 playbook, cap = MAX_LENGTH (2^26 = 67_108_864, matching MAX_ARRAY_BYTES's rationale: a single fixed reservation no larger than the largest legal array):

1. MAX_LENGTH added to schema.rs, documented with the F2 probe arithmetic.
2. parse_max_length converted to Result<Option<usize>>: non-integer → clean Schema error; usize overflow → clean Schema error (the silent .and_then(try_from().ok()) drop is gone); over-cap → clean Schema error naming the path, value, and maximum.
3. Meta-schema maxLength property gains "maximum": 67108864 — the published contract matches the parser.
4. Tests (5, in offset_map.rs, mirroring the n2_ family): above-cap rejection naming value+maximum (bytes and string), at-cap acceptance (total_size == 67108864), u64::MAX-scale value rejected-not-silently-dropped (cap arm on 64-bit, overflow arm on 32-bit — one test covers whichever fires), and the meta-schema dual-layer check (above-cap rejected, at-cap accepted). Verified with F1's commit: 477 lib tests green, clippy clean, wasm green.

C1. Packed validate_bytes has never decoded a wide primitive

Files: src/materialize.rs:121-169 (materialize_plan_primitive's Int16/Int32/Int64/Uint64/Float64/Boolean arms — all 0-execution), src/sequential_reader.rs:345-383 (the reader's same arms are covered via read_next tests, but the materializer's are not)

Problem: every packed validate_bytes test feeds u8/uint32/ string-shaped data. The i16/i32/i64/u64/f64/bool arms of the plan materializer — the code every untrusted packed wire buffer flows through — have never executed through any test. Probe (in-session) confirmed the BE i16/bool path works; the arms are correct, just unexercised. This is the flagship decode path for alkcall's packed frames; one battery test closes it (mirror the aligned read_field battery, tests/engine_integration.rs:130-190, which already covers all twelve primitive kinds on the aligned side).

Resolution (2026-09-02): two tests in engine.rs: c1_validate_bytes_packed_decodes_all_twelve_primitives_le (the full eleven-field battery — i8..bool — plus a corrupted-bool rejection arm) and c1_validate_bytes_packed_decodes_big_endian_subset (BE i16/u64/ f64 through the same public path). Both green.

C2. Aligned validate_bytes never exercises the default inline encoding for string/bytes

Files: src/materialize.rs:1037-1039 (materialize_variable_aligned's LengthPrefixed-else branch — 0-exec through the public path)

Problem: the aligned validate_bytes tests use records, unions, maxLength reservations, and offset-indirect encodings. The default encoding — an inline length-prefixed string or bytes field, the most common real shape — reaches read_field (engine_integration.rs:192) but never validate_bytes. The aligned validate_bytes surface has thus never decoded the single most likely field kind through its public path.

Fix: one aligned validate_bytes test with a trailing inline string (and a bytes variant or arm), asserting acceptance plus a short-buffer rejection.

Resolution (2026-09-02): c2_validate_bytes_aligned_inline_string_and_bytes_default_encoding in engine.rs. One wrinkle the test wrote itself into: ADR-006 allows an inline length-prefixed variable field only in the final position, so the string and bytes shapes get separate one-field schemas (string after a fixed id; bytes as a lone field). Asserts acceptance for both plus a short-buffer rejection for the string.

C3. ReadPlan::compile's union-variant cycle arm is untested standalone

Files: src/read_plan.rs:509-514 (compile_variant's cycle_err arm — 0-exec)

Problem: the H2 test family exercises check_ref_graph (walk guard) via OffsetMap::compute/LayoutBuilder::new/ materialize_aligned, and ValidationPlan::compile's cycle arm is covered (validation_plan.rs:297 shows executions, via the shared_refs_compile_without_false_cycle/cycle tests). But ReadPlan::compile's own cycle rejection — the defense the packed read plan relies on when driven standalone (its doc explicitly promises untrusted-input safety) — has no test driving a two-def cycle through it. The depth cap is tested (deep_nesting_beyond_depth_cap_is_schema_error); the cycle arm is shadowed in every engine-path test by the ValidationPlan gate running first (engine.rs:153).

Fix: a read_plan_compile_two_def_cycle_rejected test calling ReadPlan::compile directly on a two-def cycle, mirroring validation_plan.rs's existing standalone cycle test.

Resolution (2026-09-02): c3_cycle_through_union_mapping_variant_is_schema_error in read_plan.rs. Writing the test sharpened the finding: the field-level cycle arm (compile_typeref, :362) was already covered (2 execs) by cyclic_ref_through_two_defs_is_schema_error; the 0-exec arm was compile_variant's own check (:513), reachable only when the cycle closes through a union mapping entry. The new test's shape (A → B → U(mapping: "1" → $ref B)) trips exactly that arm — verified post-fix at the line level (1 execution).

L1. builder.rs's JSON-Schema conveniences are entirely untested

Files: src/builder.rs:268-290 (array(), number(), boolean_(), null()), :465-479 (items(), additional_properties()), :508-565 (maximum(), min_length(), min_items(), max_items(), format(), title(), description()), :440 (the field()-on-standard-repr path)

Problem: only the BAST-side builders have tests. The standard JSON-Schema side feeds jsonschema::build_validator (the json_schema parameter of AlkTypeEngine::compile), so a typo'd or misplaced keyword would ship silently — the builder emits the JSON, jsonschema interprets it, and nothing checks the translation. One table-style test asserting each convenience produces the expected JSON key/value closes the surface cheaply.

Resolution (2026-09-02): five tests in builder.rs: l1_standard_type_constructors_produce_type_keyword (all eight standard constructors, exact-JSON assertions), l1_field_on_standard_object_builds_properties (field() on the standard repr + required()), l1_items_and_additional_properties_on_standard_types, l1_constraint_keywords_emit_expected_json_keys (minimum/maximum/ minLength/minItems/maxItems/format/title/description, each asserted on its exact keyword), and l1_standard_built_schema_compiles_as_json_validator (the end of the translation chain: the emitted JSON builds a jsonschema validator and the constraints actually bite — valid passes, over-maximum/ missing-required/below-minimum fail).

L2. tunion::read_field_discriminator's enum arm is 0-exec

Files: src/tunion.rs:166-169

Problem: N1's resolution extended tunion to match the reader's kind set and added uint16/uint32 tests both endians — but skipped the enum arm, which is in the documented kind set (tunion.rs:106-112 names "string / uint8 / uint16 / uint32 / enum"). The reader's enum arm is tested (m4_field_disc_enum_dispatches_on_index); tunion's is not. One test locks parity on the last arm.

Resolution (2026-09-02): l2_read_field_discriminator_enum_ dispatches_on_index and l2_read_field_discriminator_enum_big_endian in tunion.rs — enum index 0 (LE) and 1 (BE) dispatch with variant_offset == 4 / discriminator_size == 4.

N1. OffsetEntry::start()/end() 0-execution in the combined run is a merge artifact, not a hole

Files: src/offset_map.rs:79-86

The combined cargo llvm-cov --release run reports these 0-exec; tests/poc_roundtrip.rs:187-189 calls start() (and the big_endian_round_trip_via_offset_map test calls end()). Per-test coverage confirms both execute (32/2 calls respectively in a poc_roundtrip-only run). llvm-cov's profile merge does not attribute integration-test-binary executions to the library in every run configuration. Recorded so nobody "fixes" this by deleting the accessors or writing a redundant in-module test. (Caveat for future audits: when a combined run shows 0-exec on something an integration test visibly calls, re-run per-test-target before classifying.)

N2a. data_access.rs's remaining uncovered lines are the documented >4 GiB guards — fine to leave

Files: src/data_access.rs:54-95, 223-291, 336-414

All are checked_add overflow arms and u32-truncation guards needing multi-GiB slices or near-usize::MAX offsets — already documented as defensively-unreachable on 64-bit test hardware in #006 M4 item 3's resolution. (The read_array/write_array arms at :54-95 are additionally unreachable-after-check_bounds belt-and-suspenders.) No action.

N3a. bast.rs dead-or-orphaned surface — flag for the pre-release review

Files: src/bast.rs:212-214, 305-307, 404-406, 506-508, 741-743, 924-926, 973-975 (source() accessors — zero callers anywhere in src or tests), :353-363 (BastField::synthetic, #[allow(dead_code)], zero callers), :149-167 (resolve_typeref_as_def's inline struct/union/enum arms — both call sites pass $ref-only variants since H3's parse rules forbid re-declaration; plausibly dead now)

Three small deletions-or-justifications. Not fixed this session (the source() accessors are public API — removal is a semver decision for the pre-release review, and AGENTS.md's semver exception list says renames/removals need an explicit ask). Recorded so the pre-release review session has the list.

N4a. Internal-shape error arms are structurally unreachable — fine to leave

Files: src/materialize.rs:241,309,422,858, src/sequential_reader.rs:295,473,533,832, src/offset_map.rs:352, src/layout_builder.rs:194,282, src/read_plan.rs:402

The "internal: …" arms that dispatch on a match the caller already narrowed (e.g. "union body at X is not CompositePlan::Union" inside a function only reachable from a Union match arm). They are honest defensive code — deleting them would force unwrap() — and forcing them in tests would require constructing mid-walk corruption. Leave uncovered; the pattern is consistent across the codebase.

---

What's Good

• The #006 fix sessions left the tree in genuinely good shape: 90.67% lines with every high-traffic wire path (reader dispatch, plan compiler, offset map, walk guard) in the mid-90s or better.
• The untrusted-input discipline is visible in the coverage: every parse-level gate added in #006 (H1 caps, N2 align cap, N3 string/bytes-only maxLength, H2 cycle rejections at all three standalone walkers) has both rejection and boundary tests.
• The #[cfg(test)] helper noise is the only thing making bast.rs/data_access.rs look worse than they are — the production coverage of both is materially higher than the raw per-file number.

Recommended Order

1. F1 — guard hoist + cross-consumer agreement test resolved 2026-09-02 (with F2).
2. F2 — MAX_LENGTH cap, N2's dual-layer playbook verbatim resolved 2026-09-02 (with F1).
3. C1 + C2 + C3 — one locking test each resolved 2026-09-02.
4. L1 + L2 — posture tests resolved 2026-09-02.
5. N3a — defer to the pre-release review (semver decision).

Notes

• Probe tests were run as tests/zzz_probe.rs in-tree during the session and deleted before any commit (the #006 pattern). Neither probe was a crash hazard; both reproduce safely in the default harness.
• Per-file numbers are from a single cargo llvm-cov --release run; the N1 merge artifact means integration-test-only calls (e.g. OffsetEntry::start()) can show 0-exec in the combined report — the classification above already accounts for that.
• The coverage holes fixed this session (C1-C3, L1, L2) were chosen because each is load-bearing and one-test-cheap; the remaining uncovered mass is dominated by N2a/N3a/N4a, which are documented rather than forced.
• Static test counts at the review-#007 commits: 477 (F1/F2), 481 (C1-C3), 488 (L1/L2) — +14 net from the pre-audit 474.
• Post-fix coverage (same tool, full run): TOTAL 91.66% lines / 87.64% functions (from 90.67/86.32). Per-file movement: builder.rs 91.28→99.33, engine.rs 96.44→96.76, materialize.rs 85.72→87.74, read_plan.rs 90.69→90.94, tunion.rs 92.02→92.86. The remaining mass is the documented N2a/N3a/N4a classes.