0 Fork 0

Files

T

deepseek-v4-pro ec73440c19 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).

2026-08-15 14:43:09 +00:00

17 KiB

Raw Permalink Blame History

status, last_updated, reviewed_artifacts, tool, reviewer

status

last_updated

reviewed_artifacts

tool

reviewer

open

2026-08-15

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

manual source read + cargo test/clippy + cargo-llvm-cov

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:

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.
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:

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.
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<Value>), 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<AlkTypeKind> / 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

M1 (field-level endian override) — ~10 lines + tests, closes a silent-corruption path on a documented feature. Smallest and highest-value.
M2 (aligned-mode materialization) — decide implement-vs-reject with the publisher; the reject option is small and matches the ADR-006/ADR-008 pattern.
M3 (meta-schema at compile time) — ~5 lines + tests, closes a silent-misinterpretation path on untrusted input.
L1 (offset-indirect dead code) — decide together with M2.
L2 (dead endian parameter) — ~4 lines, clarity only.
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.

17 KiB Raw Permalink Blame History Unescape Escape