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).
This commit is contained in:
deepseek-v4-pro committed
1 parent
562284faf4
commit
ec73440c19
1 file changed
+419
@@ -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<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
|
||||
|
||||
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.
|
||||
Reference in new issue
Block a user