From ec73440c19535876ef65328f1f51152971b9f9af Mon Sep 17 00:00:00 2001 From: deepseek-v4-pro Date: Sat, 15 Aug 2026 14:43:09 +0000 Subject: [PATCH] Add post-BAST-pivot code review (#003) Covers the whole crate after the BAST pivot: correctness, code smell, panic safety, and coverage (cargo-llvm-cov). 3 Medium findings (field- level endian override ignored, aligned-mode validate_bytes broken for arrays/maxLength/offset-indirect, meta-schema never applied at compile time), 2 Low (offset-indirect dead code, dead endian param), 4 Nits. Timestamp removal recorded as a publisher decision, tracked separately. Verification: cargo test --release (389 pass), cargo clippy --all-targets -- -D warnings (clean), cargo llvm-cov --release (90.14% lines / 86.68% functions). --- docs/reviews/003-code-review.md | 419 ++++++++++++++++++++++++++++++++ 1 file changed, 419 insertions(+) create mode 100644 docs/reviews/003-code-review.md diff --git a/docs/reviews/003-code-review.md b/docs/reviews/003-code-review.md new file mode 100644 index 0000000..3b0376c --- /dev/null +++ b/docs/reviews/003-code-review.md @@ -0,0 +1,419 @@ +--- +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.