--- status: open last_updated: 2026-08-15 reviewed_artifacts: - src/lib.rs - src/bast.rs - src/bast_meta.rs - src/bast_validation.rs - src/builder.rs - src/data_access.rs - src/engine.rs - src/error.rs - src/layout_builder.rs - src/materialize.rs - src/offset_map.rs - src/schema.rs - src/sequential_reader.rs - src/tunion.rs - src/validation.rs - src/macros.rs - tests/{engine_integration,error_paths,poc_roundtrip,tunion_dispatch}.rs - Cargo.toml tool: manual source read + cargo test/clippy + cargo-llvm-cov reviewer: post-BAST-pivot code review --- # Code Review #003 — Post-BAST-Pivot Review ## Purpose First logic/correctness review after the BAST pivot (the v0.1.0 `AlkType:*` custom-keyword JSON Schema format was replaced with the BAST format; see `docs/plans/bast-implementation.md`). The pivot touched every schema-walking path, so this pass re-reads the whole crate for correctness, code smell, panic safety, and coverage — the same scope as review #002, but against the new BAST surface. Two things motivated this review beyond the routine sweep: 1. The pivot was a large, multi-step change (10 steps, 8 commits). A couple of pre-existing bugs were fixed *during* the pivot (the enum index-bounds dead constraint, the `write_bytes` u32 truncation), so the same class of bug could be lurking in the newly-rewritten paths. 2. The publisher asked specifically for a coverage pass (`cargo-llvm-cov`) with an eye toward *important* things being covered rather than raw numbers. ## Methodology - Full read of all 16 `src/*.rs` files (production + test modules) and all 4 integration test files. - `cargo test --release`, `cargo clippy --all-targets -- -D warnings`. - `cargo llvm-cov --release` (summary + per-file + uncovered-lines) to attribute coverage gaps to specific code paths. - Targeted reproduction of the suspicious paths (field-level endian override, aligned-mode variable-length/array materialization) via throwaway integration tests. - Cross-reference every error path against its caller to confirm errors propagate (not swallowed) and carry useful attribution. - Read `docs/reviews/002-code-review.md` for prior context and resolved/unresolved items. ## Verification Baseline All verification run on the reviewed tree (commit `562284f`): - `cargo test --release`: **389 tests pass** (312 crate unit tests + 77 integration tests across 4 files). Zero failures. - `cargo clippy --all-targets -- -D warnings`: **clean**. - `cargo llvm-cov --release`: **90.14% line coverage** (7903/8682), **86.68% function coverage** (743/842). Per-file breakdown below. - No `unsafe` anywhere in the crate. - No `TODO`/`FIXME`/`HACK`/`XXX` markers in source. - All `unwrap`/`expect`/`panic!`/`unreachable!` are confined to `#[cfg(test)]` modules, verified by line-context cross-reference. ### Coverage breakdown | Module | Lines | Functions | |---|---:|---:| | bast.rs | 88.3% | 90.3% | | bast_validation.rs | 93.0% | 90.5% | | builder.rs | 91.3% | 87.6% | | data_access.rs | 83.5% | 76.2% | | engine.rs | 96.9% | 98.3% | | layout_builder.rs | 91.6% | 81.8% | | materialize.rs | **81.8%** | **76.1%** | | offset_map.rs | 89.5% | 79.2% | | sequential_reader.rs | **84.6%** | **75.0%** | | tunion.rs | 92.7% | 91.7% | | **TOTAL** | **90.1%** | **86.7%** | The low-function-count modules are not test-helper noise — they are exactly where the correctness bugs below live. The uncovered lines in `materialize.rs` and `sequential_reader.rs` are the aligned-mode variable-length/array paths and the field-level-endian paths, which are **untested and broken** (see M1, M2). The `data_access.rs` 76% function coverage is mostly the `read_*_indirect` family, which has no production caller (see L1). ## Summary Statistics | Severity | Count | |----------|------:| | Critical | 0 | | Medium | 3 (M1, M2, M3) | | Low | 2 (L1, L2) | | Nit | 4 (N1, N2, N3, N4) | No critical findings. The crate is in good shape, but the three Medium findings are **silent data-corruption / silent-misinterpretation bugs** in the newly-rewritten paths — they must be fixed before the next release. The Low findings are dead code and a robustness gap; the Nits are hygiene. --- ## Findings ### M1. Field-level `endian` override is ignored by the reader and aligned materializer **Files**: `src/sequential_reader.rs:290`, `src/engine.rs:338,455`, `src/materialize.rs:461` **Problem**: The spec documents per-field endian override (`docs/architecture/bast-format.md` §Endianness — "Field-level `endian` overrides the struct/union default"), and the packed materializer honors it (`materialize.rs:120` uses `field.effective_endian(endian)`). But three paths use only the struct-level endian: - `sequential_reader.rs:290` `read_field_value` — uses `self.endian`, never `field.effective_endian`. - `engine.rs:338` `read_field` and `engine.rs:455` `write_field` — `let endian = self.endian;`. - `materialize.rs:461` `materialize_struct_aligned` — passes the struct `endian` to `materialize_leaf_at`, never `field.effective_endian`. **Reproduction** (throwaway integration test, confirmed): a big-endian struct with a `"crc": { "kind": "uint32", "endian": "little" }` field reads `0x01020304` as `67305985` (big-endian interpretation) in both `sequential_reader` and `read_field`. The bytes are correct; the interpretation is wrong — silent data corruption. **Fix**: thread `field.effective_endian(endian)` through all three paths. `read_field_value` already receives the `BastField`; `engine::read_field` / `write_field` need to look up the field's effective endian (they already walk the BAST tree via `lookup_field_kind`); `materialize_struct_aligned` needs to pass `field.effective_endian(endian)` to `materialize_leaf_at` instead of the struct default. **Lift**: closes a silent-corruption path on a documented feature. Small effort (~10 lines + regression tests). --- ### M2. Aligned-mode `validate_bytes` is broken for arrays, `maxLength` fields, and `offset-indirect` fields **File**: `src/materialize.rs:461-512` (`materialize_struct_aligned`) **Problem**: `materialize_struct_aligned` routes fixed-size and variable-length leaves through `materialize_leaf_at`, which calls `materialize_typeref_packed` — i.e. it always reads a **length-prefixed** value. But the aligned `OffsetMap` stores three different shapes: - `maxLength` fields are a raw reservation (no length prefix) — the materializer reads the first 4 bytes of the *data* as a length prefix. Reproduced: `"hello"` in an 8-byte reservation read a length of `1819043180` and failed with a bounds error. - `offset-indirect` fields are an 8-byte `{offset, length}` pair — read as a length prefix, garbage. - arrays are recorded as `vals[0]`/`vals[1]` entries only, so `offset_map.get("vals")` returns `None` → `Offset` error. Reproduced. Only the default inline length-prefixed variable field (and only as the final field, per ADR-006) works in aligned mode. There are **no tests** covering aligned `validate_bytes` with arrays or non-default variable encodings — that is why this slipped through the pivot. **Fix**: two options, decide with the publisher: 1. **Implement** aligned materialization for the three shapes: read `maxLength` fields as a fixed-size slice, `offset-indirect` fields via `read_*_indirect` (with a data region), and arrays by iterating the `vals[i]` offset-map entries. 2. **Reject** these combinations at compile time (return `AlkTypeError::Schema`/`Offset` from `OffsetMap::compute` or `compile`) if they are out of scope for v1, so the failure is loud and at load time rather than a silent misread at access time. Option 2 is the smaller, safer fix and matches the existing ADR-006/ ADR-008 pattern of rejecting unsupported aligned-mode combinations. The `offset-indirect` encoding is already dead code on the read path (see L1), which argues for rejecting it in aligned mode until it is actually implemented. **Lift**: closes a silent-misread path. Medium effort either way. --- ### M3. The BAST meta-schema is never applied at compile time; annotation parsers silently tolerate malformed values **Files**: `src/engine.rs:123` (`compile`), `src/bast.rs:899-926` (`parse_endian_opt`, `parse_align`, `parse_encoding`, `parse_max_length`) **Problem**: `BAST_META_SCHEMA` is exported and self-tested, but `AlkTypeEngine::compile` never validates the document against it. The parser (`bast.rs`) is the only gate, and it silently tolerates malformed annotations: - `parse_endian_opt` (`bast.rs:899`) — `"endian": "middle"` → `None` → silently defaults to little. - `parse_encoding` (`bast.rs:921`) — unknown encoding → silently `LengthPrefixed`. - `parse_align` / `parse_max_length` — non-integer / negative → silently `None`. These are exactly the cases the meta-schema's `enum` / `minimum` constraints exist to reject. Per AGENTS.md §3, schemas are untrusted input (the `alkcall` consumer accepts them from arbitrary internet peers). A malicious peer can send `"endian": "bogus"` and get a silently-misinterpreted layout instead of a `Schema` error. **Fix**: validate the document against `BAST_META_SCHEMA` in `compile` (one-time, cheap — the meta-schema is a `LazyLock`), *or* make the annotation parsers return `Err(AlkTypeError::Schema)` on unknown values. The meta-schema route is preferred: it is the single source of truth and catches the whole class of malformed-annotation bugs at once. **Lift**: closes a silent-misinterpretation path on untrusted input. Small effort (~5 lines + tests). --- ### L1. `offset-indirect` is dead code on the read path **Files**: `src/data_access.rs:307-355`, `src/engine.rs:388-399` **Problem**: `data_access::read_string_indirect` / `read_bytes_indirect` have no production caller (only their own unit tests). `engine.read_field` always calls `read_string` / `read_bytes` (length-prefixed) regardless of the field's `encoding`. So a schema declaring `"encoding": "offset-indirect"` compiles and lays out correctly in the offset map, but can never be read back. This is the same root cause as M2's `offset-indirect` arm. Decide together with M2: either wire `read_*_indirect` into the read path (and the aligned materializer), or drop the `offset-indirect` encoding entirely until a consumer needs it. Leaving it half-wired is the worst state — it looks supported but silently misreads. **Lift**: removes dead code or completes a feature. Small effort. --- ### L2. `materialize_packed` / `materialize_aligned` take a dead `endian` parameter **File**: `src/materialize.rs:44-88` **Problem**: both functions take `endian: Endian` and immediately `let _ = endian;`, using `struct_node.endian()` instead. The caller's `self.endian` (from `engine.rs:285,287`) is ignored. The signature is misleading — a reader assumes the passed endian is honored. **Fix**: drop the parameter and read the endian from the root struct inside the function (it already does). ~4 lines. Purely a clarity fix; no behavior change. --- ### N1. `number_from_f64` maps NaN/Inf to `Value::Null` **File**: `src/materialize.rs:451-455` **Problem**: `serde_json::Number::from_f64` returns `None` for NaN/Inf, so `number_from_f64` substitutes `Value::Null`. A NaN float in the buffer then surfaces as "expected a number" from `validate_float` (`bast_validation.rs:176`) rather than "expected a finite number". The error is misleading, though the outcome (rejection) is correct. **Fix** (optional): have the materializer propagate a non-finite float as an `AlkTypeError::Access` at read time, or leave as-is and accept the slightly-off error message. Not a correctness bug. --- ### N2. `BastType::alk_kind()` returns `Struct` for any `$ref` **File**: `src/bast.rs:715-725` **Problem**: `BastType::Ref(_) => AlkTypeKind::Struct` is documented but a footgun — a `$ref` to a union/enum misreports its kind unless the caller resolves first. Most callers do resolve first, but the invariant is fragile and easy to break in a future edit. **Fix** (optional): leave as-is (documented) or make `alk_kind` return `Option` / require resolution. Defer unless it bites. --- ### N3. `check_bytes` accepts both `String` and `Array` forms **File**: `src/bast_validation.rs:213-256` **Problem**: `check_bytes` handles `Value::String` and `Value::Array`, but the materializer only ever emits `Array` for bytes (`materialize.rs:212`). The `String` arm is dead/legacy. Harmless, but it widens the accepted surface for no reason. **Fix** (optional): drop the `String` arm, or keep it if a future materializer emits bytes as a string. Defer. --- ### N4. Stale ADR references in doc comments **Files**: `src/error.rs:3` ("ADR-098"), `src/tunion.rs:1` ("ADR-097"), `src/engine.rs:51,196` ("ADR-101"), `src/offset_map.rs:1`, `src/layout_builder.rs:1`, `src/sequential_reader.rs:1` ("ADR-096") **Problem**: none of these ADR numbers exist in `docs/architecture/decisions/` (which has 001–010 + `bast-bast-format` + `val-split-two-validator-model`). The pivot renumbered/renamed ADRs but the code comments were not synced. A reader following the reference hits a dead end. **Fix**: map each stale reference to the correct ADR (e.g. "ADR-096" → ADR-002 for the two layout modes, "ADR-101" → ADR-007 for the packed read factory, "ADR-098" → ADR-004 for error handling, "ADR-097" → ADR-003 for annotations) and update the comments. ~6 lines. --- ## The `Timestamp` kind `AlkTypeKind::Timestamp` is a first-class kind that is byte-identical to `String` everywhere (length-prefixed UTF-8), and its only distinguishing behavior is `is_rfc3339_timestamp` (`bast_validation.rs:406`) — a hand-rolled, non-strict check that the doc itself admits "Feb 31 passes; seconds range isn't checked; leap seconds aren't handled." The parsing is fragile (the `rfind('-')` timezone-offset heuristic, no fractional-second handling). It is documented as matching v0.1.0, so it is not a regression, but it is the weakest part of the validator and adds a 19th kind plus a `needs_endian` / `is_variable_length` / `natural_alignment` arm, all to validate a string that a consumer could validate with a standard JSON Schema `format: "date-time"` on the `validate_json` path. **Decision (publisher)**: remove it. It is a residual from an early research reference that included a timestamp; it is largely irrelevant at the BAST level, and JSON-level timestamp validation is `jsonschema`'s job, not alktype's. Tracked as a follow-up task, not part of this review's findings. --- ## What's Good The crate is in notably good shape after the pivot. Highlights: - **The BAST parser is clean and defensive.** `bast.rs` returns `AlkTypeError::Schema` on every malformed-document path, never panics, and uses `checked_add` / `usize::try_from` for all count/offset casts. The typed tree (`BastDoc`/`BastDef`/`BastType`) is a real improvement over the v0.1.0 raw-JSON accessors. - **The enum index-bounds fix is correct.** `validate_enum` (`bast_validation.rs:274`) checks the materialized index against `values.len()`, closing the v0.1.0 dead constraint. Well-tested. - **Overflow safety is thorough.** `checked_add` everywhere in the hot paths; the `write_bytes` u32 truncation from review #002 (M2) is fixed and the guard pattern is now the norm. - **Error attribution is excellent.** Every `Access`/`Offset` error carries a `field_path`; the BAST parser errors carry a dotted path into the document (`"bast: struct at .fields[2] ..."`). - **The two-validator split is clean.** `bast_validation` (bytes) and `validation` (JSON) are clearly separated, and the `AlkTypeError::Validation` payload stays uniform across both (D-BAST-009). - **Tests are strong where they exist.** 389 tests, good coverage of error paths, both endiannesses, short buffers, unknown discriminators, invalid UTF-8. The gaps are precisely the paths M1/M2 identify. - **No `unsafe`, no `TODO`/`FIXME`** — clean codebase hygiene. --- ## Recommended Order 1. **M1** (field-level endian override) — ~10 lines + tests, closes a silent-corruption path on a documented feature. Smallest and highest-value. 2. **M2** (aligned-mode materialization) — decide implement-vs-reject with the publisher; the reject option is small and matches the ADR-006/ADR-008 pattern. 3. **M3** (meta-schema at compile time) — ~5 lines + tests, closes a silent-misinterpretation path on untrusted input. 4. **L1** (offset-indirect dead code) — decide together with M2. 5. **L2** (dead `endian` parameter) — ~4 lines, clarity only. 6. **N1–N4** — hygiene; N4 (stale ADR refs) is worth doing in the same pass as the `Timestamp` removal since both touch doc comments. The `Timestamp` removal is a separate, self-contained task the publisher has already decided on; it can be done independently of the above. --- ## Notes - All line numbers refer to the tree at commit `562284f` (the last commit on `main` at review time). - The coverage numbers are from `cargo llvm-cov --release` on the same tree. The `--summary-only` and `--show-missing-lines` outputs were used to attribute gaps; the full HTML report is at `target/llvm-cov/html`. - This review does not cover documentation quality (README, inline docs, docs.rs rendering) beyond the stale-ADR-reference nit (N4). Per the publisher's workflow, that is a separate sweep. - Findings M1 and M2 were confirmed by throwaway integration tests that were removed after reproduction; the regression tests for the fixes should be added to the permanent suite.