Add review #006: 0.3.0 post-implementation review
Audits the shipped 0.3.0 code (commits ff85258..9949f91) for correctness, untrusted-input discipline, 0.2.0 parity, code smells, and coverage. Gates re-run green in-session (474 tests, clippy -D warnings, doc, wasm32, publish --dry-run); coverage measured with cargo-llvm-cov (89.59% lines / 84.80% fn). Findings: - H1: huge declared array count OOM-aborts validate_bytes (three Vec::with_capacity(count) sites; release-blocking) - H2: cyclic $ref stack-overflows OffsetMap::compute / LayoutBuilder::new / materialize_aligned when driven standalone (engine gated, public walkers not; parity-preserved) - H3: field-disc unions — builder (variant-only layout), reader (disc at union start), and materializer (position-correct shared walk) disagree on layout and discriminator position; needs a convention decision - M1: ADR-006 check misses non-final inline Record fields (probe- confirmed) - M2: aligned-mode record fields untested end-to-end - M3: read_field's aligned Struct arm is unreachable dead code - M4: coverage weak spots (materialize.rs 64.5% lines; aligned nested-struct recursion 0 executions through any test) - L1-L6, N1: stride unwrap_or_default conflation, Null-schema placeholder, dead locals, linear-scan get, variant_start doc gap, missing field-disc roundtrip test, tunion/reader disc-kind split Includes an operational warning: the H1/H2 reproducers OOM/stack- abort the test harness — reproduce in an isolated process only. Verification: file-only change, no code touched; suite green before commit.
This commit is contained in:
glm-5.3-flash committed
1 parent
9949f914df
commit
27be01af93
1 file changed
+683
@@ -0,0 +1,683 @@
|
||||
---
|
||||
status: open
|
||||
last_updated: 2026-09-02
|
||||
reviewed_artifacts:
|
||||
- src/read_plan.rs
|
||||
- src/sequential_reader.rs
|
||||
- src/materialize.rs
|
||||
- src/offset_map.rs
|
||||
- src/layout_builder.rs
|
||||
- src/engine.rs
|
||||
- src/validation_plan.rs
|
||||
- src/bast.rs
|
||||
- src/bast_meta.rs
|
||||
- src/bast_validation.rs
|
||||
- src/tunion.rs
|
||||
- src/data_access.rs
|
||||
- src/lib.rs
|
||||
- tests/{poc_roundtrip,tunion_dispatch,error_paths,engine_integration}.rs
|
||||
- docs/plans/030-compiled-forms.md
|
||||
- docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md
|
||||
- docs/architecture/decisions/012-plan-fingerprinting-and-m1-closure.md
|
||||
tool: manual source read + 0.2.0-vs-0.3.0 parity diff (git show cab4932) + cargo-llvm-cov 0.8.4 (release profile) + disposable probe tests (run in-session, then deleted)
|
||||
reviewer: 0.3.0 post-implementation review (triggered after release commit 9949f91)
|
||||
---
|
||||
|
||||
# Review #006 — 0.3.0 Implementation Review: Compiled Forms
|
||||
|
||||
## Purpose
|
||||
|
||||
The 0.3.0 release (eight phases, commits `ff85258`..`9949f91`) landed
|
||||
the compiled-form rollup: `ReadPlan` (ADR-011), owned `BastDoc`,
|
||||
`OffsetMap` `LeafMeta`, `ValidationPlan` (ADR-012 §3), and plan
|
||||
fingerprinting (ADR-012 §1/§4). This review is the post-implementation
|
||||
check the other reviews were not: it audits the *shipped code* —
|
||||
correctness, untrusted-input discipline (AGENTS.md §3/§4), 0.2.0
|
||||
behavioral parity, code smells, and coverage — rather than a plan or a
|
||||
design. It runs after the release commit, before `cargo publish`.
|
||||
|
||||
The intent is not to relitigate the ADRs or the plan; both held up
|
||||
well. The intent is to find the classic implementation-phase problems
|
||||
the phase gates could not see: cross-consumer convention disagreements,
|
||||
adversarial input that survives every gate, dead-but-live code paths,
|
||||
and the test-coverage holes that hide them.
|
||||
|
||||
## Methodology
|
||||
|
||||
- Full read of every module in `src/` touched by 0.3.0, plus
|
||||
`docs/plans/030-compiled-forms.md` and ADR-011/012.
|
||||
- Parity diff against 0.2.0 (`git show cab4932:src/...` for
|
||||
`sequential_reader.rs`, `engine.rs`, `offset_map.rs`, `materialize.rs`,
|
||||
`bast_validation.rs`) to separate pre-existing flaws from 0.3.0
|
||||
regressions. Where a finding is parity-preserved, it says so.
|
||||
- `cargo llvm-cov --release` (cargo-llvm-cov 0.8.4, summary + per-line
|
||||
text) to find weak spots; every uncovered region cited in the
|
||||
findings was read to decide "untested but fine", "untested and
|
||||
load-bearing", or "unreachable".
|
||||
- Disposable probe tests (`tests/zzz_review_probe.rs`, deleted after
|
||||
the session) to confirm/deny the four behaviors code reading flagged:
|
||||
the ADR-006 record gap, the field-disc union builder/reader split,
|
||||
the non-first discriminator field, and standalone cyclic-`$ref`
|
||||
handling. Probe results are quoted verbatim in the findings.
|
||||
|
||||
**Operational warning for future sessions:** two probes were *not*
|
||||
safe to run in the default harness. The huge-array-count repro
|
||||
(H1) OOM-aborted the machine it ran on (a `Vec::with_capacity(1e12)`
|
||||
allocation failure takes the process down with SIGABRT — and in this
|
||||
session it took down the agent server running it). The cyclic-`$ref`
|
||||
repro (H2) reliably SIGABRTs the test runner via stack overflow. Any
|
||||
session reproducing H1/H2 must do so in an isolated process (separate
|
||||
`cargo test --test <probe>` invocation on a throwaway machine, `ulimit -v`
|
||||
wrapper, or a CI job with a memory cap) — never inside the main suite.
|
||||
This is recorded here so the reproduction hazard does not have to be
|
||||
rediscovered.
|
||||
|
||||
## Verification Baseline
|
||||
|
||||
Reviewed at `main` HEAD `9949f91` ("Release v0.3.0", `Cargo.toml`
|
||||
0.3.0). All gates re-run in this session and green: 474 tests pass
|
||||
(397 + 17 + 34 + 14 + 12), `cargo clippy --all-targets -- -D warnings`
|
||||
clean, `cargo doc --no-deps` zero warnings, `cargo build --target
|
||||
wasm32-unknown-unknown --release` green, `cargo publish --dry-run
|
||||
--allow-dirty` clean. The release summary's claims were cross-checked:
|
||||
the four commit hashes exist with the described content; the bench
|
||||
numbers are recorded in the plan and ADR-007; ADR-011/012 status
|
||||
blocks, review #004/#005 status lines, and the ADR table in the
|
||||
architecture README are flipped as claimed; the `lib.rs` re-export list
|
||||
matches the plan's Semver Contract table row for row (net breaking =
|
||||
`Bast*` lifetime removal, `OffsetMap::get` return type,
|
||||
`SequentialReader::new`, `materialize_*` signatures; net additive =
|
||||
`ReadPlan` + sub-types, `LeafMeta`/`OffsetEntry`, `ValidationPlan` +
|
||||
sub-types, `fingerprint()` methods; `Hash` on `Endian`/
|
||||
`VariableEncoding` present per phase 5's prerequisite step).
|
||||
|
||||
## Summary Statistics
|
||||
|
||||
| Severity | Count |
|
||||
|----------|------:|
|
||||
| High | 3 (H1, H2, H3) |
|
||||
| Medium | 4 (M1, M2, M3, M4) |
|
||||
| Low | 6 (L1–L6) |
|
||||
| Nit | 1 (N1) |
|
||||
|
||||
The three Highs are adversarial-input crashes (H1, H2) and a
|
||||
cross-consumer wire-layout convention gap (H3) — all three are the
|
||||
exact class of problem AGENTS.md §3 exists for, given that `alkcall`
|
||||
feeds schemas from arbitrary internet peers into this engine. H1 should
|
||||
block publishing 0.3.0 to crates.io until fixed. None of the three is a
|
||||
0.2.0 regression in the strict sense (details per finding), but two of
|
||||
them became materially easier to hit with the 0.3.0 surface.
|
||||
|
||||
---
|
||||
|
||||
## Findings
|
||||
|
||||
### H1. A huge declared array `count` OOM-aborts the process (DoS on untrusted schemas)
|
||||
|
||||
**Files**: `src/read_plan.rs:533-553`, `src/materialize.rs:248`,
|
||||
`src/materialize.rs:637`, `src/materialize.rs:924`,
|
||||
`src/bast_meta.rs:113`
|
||||
|
||||
**Problem**: The BAST meta-schema allows any array `count >= 0`
|
||||
(`bast_meta.rs:113`, `"minimum": 0`, no upper bound), and nothing
|
||||
downstream bounds the product `count × element size`:
|
||||
|
||||
- `ReadPlan::compile_array` (`read_plan.rs:533`) accepts any `count`
|
||||
verbatim. `compile` validates depth and cycles but never total size.
|
||||
- `materialize_plan_array` (`materialize.rs:248`) — the packed
|
||||
`validate_bytes` path — does `Vec::with_capacity(count)` before
|
||||
reading a single byte. With `count: 1000000000000` (1e12) and u8
|
||||
elements, that is a 1 TB capacity request: the allocator fails and
|
||||
the process aborts (SIGABRT), not an `Err` return.
|
||||
- The same `Vec::with_capacity(count)` pattern is on the aligned path
|
||||
twice: `materialize_array_packed` (`materialize.rs:637`, reached via
|
||||
aligned record values) and `materialize_array_aligned`
|
||||
(`materialize.rs:924`).
|
||||
|
||||
The sequential reader gets away with it only incidentally:
|
||||
`plan_read_array` (`sequential_reader.rs:726-733`) computes
|
||||
`count.checked_mul(element_stride)` and errors cleanly when the stride
|
||||
is nonzero — but a fixed-stride array's *materialize* (the
|
||||
`validate_bytes` half) still hits the `with_capacity`, so
|
||||
`validate_bytes` on an accepted schema aborts regardless of mode. For
|
||||
stride-0 (variable-element) arrays, `plan_walk_variable_array_size`
|
||||
(:749-771) errors per element on a short buffer — safe, but after
|
||||
walking up to `count` iterations of per-element error returns.
|
||||
|
||||
Probe (run in-session; OOM-aborted before printing): an engine compiled
|
||||
from `{"kind": "array", "element": "uint8", "count": 1000000000000}`
|
||||
accepted `AlkTypeEngine::compile` in both modes; the
|
||||
`validate_bytes(&[0,0,0,0])` call did not return — the process died in
|
||||
the allocator. Note the meta-schema itself is *not* at fault (count is
|
||||
a legitimate schema knob); the engine's failure to bound its own
|
||||
allocation is.
|
||||
|
||||
**This violates AGENTS.md §3** ("a malicious or unsupported schema
|
||||
definition must produce a handleable error, not a crash") on the
|
||||
engine's flagship untrusted-input path (`validate_bytes` on an incoming
|
||||
stream). **Fix before publishing 0.3.0.**
|
||||
|
||||
**Fix**: two layers, both cheap:
|
||||
|
||||
1. Replace all three `Vec::with_capacity(count)` with a plain
|
||||
`Vec::new()` + push loop (the loops already push `count` elements;
|
||||
the pre-allocation is pure downside for adversarial input and no
|
||||
measurable win for honest input at the sizes that matter — the
|
||||
materializer's per-element work dominates).
|
||||
2. Optionally, bound total computed array bytes at compile time
|
||||
(`count.checked_mul(elem_size)` against a documented cap, returning
|
||||
`AlkTypeError::Schema("array size exceeds compile-time limit")`), so
|
||||
adversarial schemas die at compile with a clean error instead of
|
||||
per-buffer. The reader's `checked_mul` in `plan_read_array`
|
||||
(`sequential_reader.rs:729`) already refuses oversized fixed-stride
|
||||
arrays at read time; the materializer should not be weaker than the
|
||||
reader.
|
||||
|
||||
Add tests: compile+validate with `count: usize::MAX` (expect clean
|
||||
`Err`), and a moderate count against a short buffer (expect clean
|
||||
`Err`).
|
||||
|
||||
### H2. Cyclic `$ref` stack-overflows `OffsetMap::compute` / `LayoutBuilder::new` / `materialize_aligned` when driven standalone
|
||||
|
||||
**Files**: `src/offset_map.rs:123` (`compute`),
|
||||
`src/layout_builder.rs:150` (`new`), `src/materialize.rs:462`
|
||||
(`materialize_aligned`), contrast `src/engine.rs:146-152`,
|
||||
`src/read_plan.rs:54,283`, `src/validation_plan.rs:81`
|
||||
|
||||
**Problem**: The engine is safe: `AlkTypeEngine::compile` runs
|
||||
`ValidationPlan::compile` *before* the layout builders, and the plan's
|
||||
compile walk rejects cyclic `$ref` graphs with a clean `Schema` error
|
||||
(engine.rs:146-152 documents this gate explicitly — good defensive
|
||||
design). `ReadPlan::compile` and `ValidationPlan::compile` each carry
|
||||
their own depth cap (128) + definition-level cycle set
|
||||
(`read_plan.rs:54,283`, `validation_plan.rs:81`), so both are
|
||||
untrusted-input-safe standalone.
|
||||
|
||||
But the *other three public schema walkers* have neither guard, and
|
||||
`BastDoc::new` (`bast.rs:74`) parses the root eagerly while resolving
|
||||
`$ref`s lazily (`bast.rs:110-130`) — so a cyclic document parses
|
||||
successfully and the walkers recurse unboundedly:
|
||||
|
||||
- `OffsetMap::compute` → `ComputeCtx::compute_struct` →
|
||||
`resolve_typeref` → nested `compute_struct` → …
|
||||
- `LayoutBuilder::new` (parse succeeds) → `build` → `walk_struct` →
|
||||
same shape.
|
||||
- `materialize_aligned` → `materialize_struct_aligned` → same shape.
|
||||
|
||||
Probe result (verbatim):
|
||||
|
||||
```
|
||||
PROBE6 … process didn't exit successfully: … (signal: 6, SIGABRT:
|
||||
process abort signal)
|
||||
```
|
||||
|
||||
i.e. standalone `OffsetMap::compute` on
|
||||
`S = { next: { "$ref": "#/$defs/S" } }` stack-overflows. All three
|
||||
types are public re-exports (`lib.rs:66,72,73`), so a consumer using
|
||||
them without the engine — exactly the "consumer holds a `BastDoc`"
|
||||
use case `bast_validation::validate_value` was retained for — hits an
|
||||
abort, violating AGENTS.md §3 for the public API surface, not just the
|
||||
engine.
|
||||
|
||||
This is **parity-preserved** (0.2.0 had the same gap; it predates
|
||||
0.3.0), but 0.3.0 is the release that made the *pattern* explicit:
|
||||
`ReadPlan`/`ValidationPlan` got guards and the plan text says the
|
||||
engine's ValidationPlan gate "serves as the engine's cyclic-`$ref`
|
||||
gate (the layout walkers have no cycle guard)". The residual exposure
|
||||
— public walkers without guards — was left silent. AGENTS.md §3 makes
|
||||
no engine-vs-standalone distinction.
|
||||
|
||||
**Fix**: extract the depth-cap + cycle-set guard into a shared helper
|
||||
(the two existing implementations in `read_plan.rs` and
|
||||
`validation_plan.rs` are already near-identical — `depth_err`/
|
||||
`cycle_err`/`seen` threading) and thread it through `ComputeCtx`,
|
||||
`BuildCtx`, and the aligned materializer's struct/array/record walk.
|
||||
Alternatively (smaller): document on `OffsetMap::compute`,
|
||||
`LayoutBuilder::new`, and `materialize_aligned` that cyclic input is
|
||||
rejected *only* via `AlkTypeEngine::compile`'s gate and that standalone
|
||||
calls require pre-validated documents — but that re-introduces the
|
||||
"trust boundary depends on call path" trap the plan's phase-1 notes
|
||||
explicitly retired for `ReadPlan`. The shared guard is the right fix;
|
||||
the doc note is the minimum acceptable one.
|
||||
|
||||
### H3. Field-name-discriminator unions: builder, reader, and materializer disagree on layout and on discriminator position
|
||||
|
||||
**Files**: `src/layout_builder.rs:485-527` (write side),
|
||||
`src/sequential_reader.rs:551-598,607-646` (read side),
|
||||
`src/materialize.rs:356-404` (materialize side), builder doc
|
||||
`src/layout_builder.rs:35-40`
|
||||
|
||||
**Problem**: Three packed-mode consumers each implement the field-disc
|
||||
union slightly differently, and their agreements/disagreements are
|
||||
undocumented and untested end-to-end:
|
||||
|
||||
1. **Layout convention (builder vs reader/materializer).** The builder
|
||||
walks *only the selected variant struct* starting at the union's
|
||||
offset (`walk_field_discriminator_union`, layout_builder.rs:485-527
|
||||
— `self.walk_struct(variant_struct, field_path, offset)`). The
|
||||
reader and materializer walk the union's declared `fields` (the
|
||||
`shared` sub-plan: discriminator field + any shared fields) *first*,
|
||||
then the variant (`sequential_reader.rs:573-588`,
|
||||
`materialize.rs:371-381`). The two conventions never agree on the
|
||||
wire: reader span = `shared_size + variant_size`, builder span =
|
||||
`variant_size`. On the builder tests' own fixture (union
|
||||
`fields: [type: uint8]`, variant `Read { type, handle }` — the
|
||||
variant *re-declares* the discriminator), the builder produces
|
||||
`type@0, handle@1, total 5` (probe-verified:
|
||||
`PROBE1 builder: total=5 event.type=Some((0, 1)) event.handle=Some((1, 4))`)
|
||||
while the reader's walk places the variant's `type` at 1 and
|
||||
`handle` at 2 (span 6) — the discriminator field would appear twice
|
||||
on the wire under the reader's convention. Conversely, if the
|
||||
variant does *not* re-declare the disc field, the reader's
|
||||
convention is coherent (disc from `shared`, variant fields follow)
|
||||
and the *builder's* is the one that drops the disc's wire position.
|
||||
`LayoutBuilder`'s tests (:1221-1334) and the reader/materializer
|
||||
tests (`sequential_reader.rs:1260-1305`,
|
||||
`materialize.rs:1183-1258`) each encode their own convention with
|
||||
different fixture shapes, so nothing cross-checks them. No doc
|
||||
comment on either side states the constraint; no roundtrip test
|
||||
drives a field-disc union from `LayoutBuilder` output through
|
||||
`SequentialReader`/`materialize_packed`.
|
||||
|
||||
2. **Discriminator position (reader vs materializer).** The reader's
|
||||
`plan_read_union` Field arm reads the discriminator value at the
|
||||
union's *start* offset
|
||||
(`plan_discriminator_string_value(disc_field, buffer, offset, …)`,
|
||||
:563-564 — `offset` is the union start, not the disc field's
|
||||
position). The materializer's Field arm walks all shared fields in
|
||||
order and captures the disc value at its *real* position
|
||||
(`materialize.rs:371-381`). For a non-first discriminator field the
|
||||
two consumers read different bytes. Probe-verified (verbatim):
|
||||
|
||||
```
|
||||
PROBE4 reader error: access error at event: unknown union
|
||||
discriminator value: 1 ← read seq's first byte at union start
|
||||
PROBE4 validate_bytes ok ← materializer dispatched correctly on
|
||||
the same buffer
|
||||
```
|
||||
|
||||
the reader stringified `1` (seq's first byte at the union start,
|
||||
offset 0) and failed the mapping lookup; `validate_bytes` on the
|
||||
identical buffer passed. Note this probe ran after probe 1's reader
|
||||
section had already panicked on an assertion; the error itself is
|
||||
the evidence — the reader dispatched on the wrong field's bytes
|
||||
while the materializer dispatched correctly on the same buffer.
|
||||
|
||||
3. **0.2.0 parity, precisely scoped.** The reader-side flaw (disc read
|
||||
at union start) is parity-preserved: 0.2.0's `read_union_value`
|
||||
Field arm also read only the disc field at the union start
|
||||
(`cab4932:src/sequential_reader.rs:460-472`), and 0.2.0's
|
||||
materializer did the same — so in 0.2.0 reader and materializer
|
||||
*agreed* (both position-blind). 0.3.0's materializer became
|
||||
position-correct (it must, to walk shared fields in order), which
|
||||
silently made the two packed read-side consumers disagree for
|
||||
non-first disc fields. The builder's variant-only layout is also
|
||||
parity-preserved. So H3 is not a regression — it is a
|
||||
long-standing convention gap that 0.3.0's `shared`-plan shape made
|
||||
visible and partially divergent.
|
||||
|
||||
**Fix**: make the decision explicit and enforce it:
|
||||
|
||||
- Decide the wire convention. The reader/materializer shape (shared
|
||||
fields, then variant) is the one the materializer already implements
|
||||
position-correctly, and it matches ADR-011's `shared` plan design —
|
||||
recommend: builder lays out `shared` then the variant, and the
|
||||
"variant re-declares the discriminator field" pattern becomes either
|
||||
required (validated at compile) or forbidden (rejected at compile).
|
||||
The other consistent option — variant-only layout, no shared walk —
|
||||
would mean reverting the reader/materializer to 0.2.0's
|
||||
position-blind disc read, which the `shared` plan shape was
|
||||
specifically built to move past.
|
||||
- Enforce at compile/parse time whatever constraint the convention
|
||||
needs (e.g. discriminator field must be first in `fields`; variant
|
||||
must not re-declare shared fields — or must, if that's the
|
||||
convention). `BastUnion::parse` (`bast.rs:494-547`) or
|
||||
`compile_union` is the natural enforcement point; today nothing
|
||||
constrains field order in a field-disc union.
|
||||
- Record the decision in ADR-011's union section (a short addendum) or
|
||||
ADR-003 §4, and add the write→read→validate roundtrip test through a
|
||||
field-disc union (packed mode) — see L6.
|
||||
|
||||
### M1. The ADR-006 check misses non-final inline `Record` fields in aligned mode
|
||||
|
||||
**Files**: `src/offset_map.rs:235-255` (the check),
|
||||
`src/offset_map.rs:515-520` (`field_variable_kind`)
|
||||
|
||||
**Problem**: `compute_struct`'s non-final inline length-prefixed
|
||||
variable-field rejection (ADR-006) dispatches on
|
||||
`field_variable_kind(field)`, which matches only *primitive*
|
||||
variable-length kinds (`BastType::Primitive(k) if
|
||||
k.is_variable_length()`). A `Record` field — whose inline
|
||||
length-prefixed form has the same 4-byte-prefix-then-variable-data
|
||||
shape and the same clobbering hazard the ADR-006 error text describes —
|
||||
slips past the check. Probe-verified (verbatim):
|
||||
|
||||
```
|
||||
PROBE2 ADR-006 MISSED for record: counts=Some((0, 4)) id=Some((4, 8)) total=8
|
||||
```
|
||||
|
||||
`{ counts: record<uint32>, id: uint32 }` computes `counts` as a 4-byte
|
||||
prefix at 0 and `id` at 4 — the record's variable data lands exactly
|
||||
where `id` reads from. `validate_bytes` then succeeds with silently
|
||||
corrupt data; `read_field("counts")` fails only incidentally (the
|
||||
composite rejection, M2). **Parity-preserved** — the identical code
|
||||
shipped in 0.2.0 (`cab4932:src/offset_map.rs:177,433-438`) — but it is
|
||||
a hole in the exact check ADR-006 created, and it is the kind of thing
|
||||
the check exists to catch.
|
||||
|
||||
**Fix**: include `BastType::Record(_) => Some(AlkTypeKind::Record)` in
|
||||
`field_variable_kind` (offset_map.rs:515-520). The ADR-006 error
|
||||
message's fix list ("use `maxLength` … `offset-indirect` … move to last
|
||||
position") already covers records correctly. Add the
|
||||
`non_final_record_rejected_in_aligned_mode` test mirroring
|
||||
`non_final_inline_string_rejected_in_aligned_mode`.
|
||||
|
||||
### M2. Aligned-mode record fields are untested end-to-end and sit right next to the M1 hole
|
||||
|
||||
**Files**: `src/materialize.rs:880-893` (aligned record dispatch),
|
||||
`src/engine.rs:453-460` (`read_field` composite rejection)
|
||||
|
||||
**Problem**: `materialize_struct_aligned` dispatches `Record` through
|
||||
`materialize_typeref_packed` at the entry's prefix offset — the
|
||||
count-prefixed walk, which works. But nothing else in the aligned stack
|
||||
coheres around records: `OffsetMap` records the entry as
|
||||
`encoding: LengthPrefixed` with the 4-byte prefix range;
|
||||
`engine::read_field` refuses `Record` as a composite ("use the
|
||||
layout-specific APIs" — there is no aligned record API); and only the
|
||||
M1 hole makes the "record as last field" position the safe one, with no
|
||||
error distinguishing it from the clobbering position. Probe 5 showed
|
||||
the happy path validates (`PROBE5 map: counts=Some((0, 4)) total=4`,
|
||||
`PROBE5 validate ok`), so the *mechanism* works — but zero tests
|
||||
exercise an aligned record field through any public path (coverage:
|
||||
`materialize_struct_aligned`'s Record arm and `materialize_leaf_at`'s
|
||||
record dispatch are hit only by in-module unit tests, never through
|
||||
`validate_bytes`/`read_field`). Whatever M1's fix decides about
|
||||
records, this path needs a locking test: record-as-last-field
|
||||
round-trips, record-mid-struct is rejected (post-M1), and
|
||||
`total_size` accounting for the prefix is asserted.
|
||||
|
||||
### M3. `read_field`'s aligned `Struct` arm is unreachable dead code
|
||||
|
||||
**File**: `src/engine.rs:449-452`
|
||||
|
||||
**Problem**: `read_field`'s kind dispatch has a
|
||||
`AlkTypeKind::Struct => Ok(FieldValue::Struct { start, end })` arm,
|
||||
but no struct entry ever exists in an `OffsetMap`: `compute_struct_field`
|
||||
(offset_map.rs:346-387) pushes only the nested struct's *inner leaf*
|
||||
entries, never an entry for the struct path itself, and the root is not
|
||||
an entry either. `read_field("header")` on a nested-struct schema
|
||||
returns the `Offset` "field not found in offset map" error — the
|
||||
`Struct` arm cannot be reached through any schema the engine accepts.
|
||||
Coverage confirms it: the arm shows 0 executions, and the composite
|
||||
rejection arm right below it (:453-460) is likewise 0-execution
|
||||
(because composites also never get entries — the reachable failure for
|
||||
composite paths is the `Offset` miss, which the tests accept in
|
||||
either variant). Two options: delete the `Struct` arm (and tighten the
|
||||
doc comment, which currently implies composites error at the dispatch
|
||||
stage rather than at lookup), or keep it and make struct entries exist
|
||||
(deliberate feature — then document and test it). Deleting is the
|
||||
smaller, honest change for 0.3.0's design.
|
||||
|
||||
### M4. Coverage weak spots map onto the findings above — the aligned materializer half is the least-tested code in the engine
|
||||
|
||||
**Tool**: `cargo llvm-cov --release` (0.8.4). Overall: **89.59% lines,
|
||||
84.80% functions**. Per-file (worst first):
|
||||
|
||||
| file | line% | fn% |
|
||||
|---|---:|---:|
|
||||
| `materialize.rs` | 64.48 | 59.30 |
|
||||
| `data_access.rs` | 79.74 | 74.65 |
|
||||
| `sequential_reader.rs` | 81.42 | 70.77 |
|
||||
| `bast.rs` | 87.00 | 88.06 |
|
||||
| `offset_map.rs` | 91.13 | 82.46 |
|
||||
| `layout_builder.rs` | 91.18 | 81.54 |
|
||||
| `builder.rs` | 91.28 | 87.50 |
|
||||
| `tunion.rs` | 92.73 | 91.67 |
|
||||
| `validation_plan.rs` | 93.97 | 83.87 |
|
||||
| `read_plan.rs` | 94.53 | 95.89 |
|
||||
| `engine.rs` | 96.57 | 100.00 |
|
||||
|
||||
The specific holes worth closing (each was read; all are
|
||||
load-bearing-or-error-path, none is trivially dead except where noted):
|
||||
|
||||
1. **`materialize.rs` aligned half** — `materialize_struct_aligned`,
|
||||
`materialize_array_aligned`, `materialize_variable_aligned`, and
|
||||
`materialize_typeref_packed` carry ~47 uncovered lines between
|
||||
them. Most striking: the nested-struct recursion
|
||||
(`materialize_struct_aligned(doc, s, &path, …)`, :866) has **0
|
||||
executions** — the function is only ever entered at the root
|
||||
(10 calls, all `path_prefix: ""`). Aligned nested-struct
|
||||
materialization — the heart of aligned `validate_bytes` — has never
|
||||
been exercised through any test. The maxLength-trim arm
|
||||
(:971-998), the offset-indirect arms (:956-969), the UTF-8 error
|
||||
(:984-987), and both "resolved kind X but type was Y" internal
|
||||
errors (:868-878) are likewise 0-execution. The engine-facing
|
||||
aligned `validate_bytes` tests cover exactly one flat 2-field
|
||||
struct. This is where H1's aligned variant, M1, and M2 all live.
|
||||
2. **`sequential_reader.rs`** — the public `schema()` (:235-237) and
|
||||
`plan()` (:240-242) accessors are never called by any test (public
|
||||
API, phase-2 deliverables). The field-disc discriminator-kind arms
|
||||
for uint16/uint32/enum (:621-641) and `plan_read_byte_discriminator`'s
|
||||
uint16/uint32 arms (:663-668) are 0-execution — wire-visible
|
||||
dispatch paths. `plan_walk_variant_size`'s union arm (:688-691) —
|
||||
the nested-union variant size walk, the capability phase 1
|
||||
specifically restored — is 0-execution (the compile side is tested;
|
||||
the read-side size walk is not).
|
||||
3. **`data_access.rs`** — nearly all uncovered lines are error paths
|
||||
(offset overflow, mutable-slice-missing, and notably the
|
||||
`write_bytes` u32-truncation guard :266-270 and
|
||||
`write_bytes_indirect`'s two u32 guards :383-391 — review #002 M2's
|
||||
"canonical overflow-guard pattern" is itself untested). One test
|
||||
each would lock the canonical pattern.
|
||||
4. **`offset_map.rs`** — `field_endian_for_element`'s
|
||||
`BastType::Struct(s) => s.endian()` / `Union(u) => u.endian()`
|
||||
arms (:529-530) are 0-execution. The union arm is unreachable
|
||||
(unions rejected in aligned mode); the struct arm says an array of
|
||||
inline struct elements inherits the *element struct's own* endian
|
||||
annotation — which contradicts the phase-5 parity rule everywhere
|
||||
else in 0.3.0 ("the referring field's effective endian propagates;
|
||||
the nested container's own annotation is not consulted"). Either
|
||||
this is a genuine divergence in array-element endian propagation
|
||||
(then it needs a parity test and probably a fix), or it's
|
||||
unreachable for inline elements too (then it's dead). Worth a
|
||||
focused look — it touches the exact parity story phase 5 closed.
|
||||
|
||||
Recommended posture (matches how this review was run): fold a coverage
|
||||
check into each fix session — after fixing a finding, extend
|
||||
`cargo llvm-cov` over the touched file and close the immediately
|
||||
adjacent uncovered branches while the context is fresh, rather than
|
||||
scheduling a standalone coverage sweep.
|
||||
|
||||
### L1. `unwrap_or_default()` conflates "variable-length" with "checked-mul overflow" for array strides
|
||||
|
||||
**File**: `src/read_plan.rs:547`
|
||||
|
||||
**Problem**: `let element_stride = fixed_composite_size(&element)
|
||||
.unwrap_or_default();` — `fixed_composite_size` returns `None` both
|
||||
for genuine variable-length elements (correct → stride 0) and for
|
||||
`checked_mul` overflow of an absurd `count × stride` (→ silently
|
||||
stride 0, i.e. "walk every element sequentially" — the reader then
|
||||
does up to `count` per-element error returns instead of one clean
|
||||
arithmetic error). With H1 fixed at compile time this becomes
|
||||
unreachable, but as written the two cases are indistinguishable at the
|
||||
call site. Prefer making `compile_array` return `Err` on the overflow
|
||||
arm (a small refactor of `fixed_composite_size` to return a
|
||||
`Result<Option<usize>, …>` or to take the cap from H1's fix).
|
||||
|
||||
### L2. `ReadPlan` construction carries a temporary `Value::Null` schema placeholder
|
||||
|
||||
**Files**: `src/read_plan.rs:295,460-461,601` (three
|
||||
`schema: Arc::new(Value::Null)` sites), `src/read_plan.rs:176-179`
|
||||
(the caller's overwrite)
|
||||
|
||||
**Problem**: every internal `compile_struct`/`compile_union`/
|
||||
`wrap_leaf` builds a `ReadPlan` with a placeholder `Arc<Value::Null>`
|
||||
schema that the top-level `compile` then overwrites via a `map` on the
|
||||
result. Harmless today (the placeholder never escapes; the public
|
||||
`schema()` accessor always returns the real doc), but it is a lie in
|
||||
the type for the duration of the walk and a trap if a future refactor
|
||||
adds a consumer mid-walk. Passing `schema: &Arc<Value>` (or the
|
||||
already-`Arc`'d clone) down through `compile_struct` removes the
|
||||
placeholder, the `map`, and the smell in one small change.
|
||||
|
||||
### L3. Dead locals left from the phase-2 rewrite
|
||||
|
||||
**Files**: `src/materialize.rs:68` (`let _ = plan;` in
|
||||
`materialize_plan_field`), `src/materialize.rs:360-365`
|
||||
(`disc_field` bound then discarded via `let _ = disc_field;`)
|
||||
|
||||
**Problem**: `materialize_plan_field` takes `plan: &ReadPlan` solely
|
||||
to discard it (`let _ = plan;`), and the field-disc union arm binds
|
||||
`disc_field` from `shared_plan.fields().get(*field_index)` then
|
||||
discards it — the materializer walks all shared fields and captures
|
||||
the disc by name instead (the correct design, per the parity note at
|
||||
:366-371). Both are clippy-invisible (`let _ =` suppresses the unused
|
||||
warnings). Drop the `plan` parameter (and fix the recursive call
|
||||
sites) and delete the `disc_field`/`let _` pair — or keep `disc_field`
|
||||
and use it to assert the walk actually encountered the disc field
|
||||
(the current `disc_value.ok_or_else(...)` at :382-384 already does
|
||||
that job, so deletion is cleaner).
|
||||
|
||||
### L4. `OffsetMap::get` / `PackedLayout::get` are linear scans positioned as random access
|
||||
|
||||
**Files**: `src/offset_map.rs:154-159`, `src/layout_builder.rs:83-88`
|
||||
|
||||
**Problem**: both `get` implementations are
|
||||
`self.fields.iter().find(…)` over a `Vec` — O(n) per lookup, on the
|
||||
types whose docs pitch random access ("the consumer can read field N
|
||||
without reading fields 0..N-1 first", offset_map.rs:92-94). Fine for
|
||||
small schemas; the claim gets less honest with schema size, and
|
||||
`read_field`/`write_field` (the aligned hot path for per-field
|
||||
access) pays it per call. `ReadPlan` already moved to `BTreeMap` for
|
||||
exactly this class of reason (phase 1, and to enable `Hash`). An
|
||||
internal `BTreeMap<String, usize>` path→index alongside the
|
||||
insertion-ordered `Vec` (keeping `iter()` order and the `Hash` derive
|
||||
over the `Vec`) makes the claim true with no public-surface change.
|
||||
Not a 0.3.0 blocker; a good 0.3.x cleanup with an existing in-repo
|
||||
pattern to copy.
|
||||
|
||||
### L5. `FieldValue::Union`'s `variant_start` semantics are undocumented and inconsistent between discriminator kinds
|
||||
|
||||
**File**: `src/sequential_reader.rs:84-91`
|
||||
|
||||
**Problem**: for byte-offset discriminators, `variant_start` is
|
||||
`abs_offset + disc_size` — an absolute offset computed from the union
|
||||
start plus the *declared* `disc.offset` (which may be nonzero, so the
|
||||
variant can start before or after the shared-field walk would place
|
||||
it). For field-name discriminators it is the position after the whole
|
||||
`shared` walk. The `FieldValue` enum doc (the "consumer recurses"
|
||||
contract) says only "Byte offset where the variant struct begins" —
|
||||
nothing about which base the offset is relative to, that byte-disc
|
||||
`offset` is honored as an arbitrary displacement, or (per H3) that the
|
||||
field-disc variant starts after shared fields while the builder lays
|
||||
out only the variant. A consumer writing generic union-handling code
|
||||
against `FieldValue::Union` alone can get this wrong silently. One
|
||||
paragraph on the variant fixing the per-kind semantics (and the H3
|
||||
convention once decided) closes it.
|
||||
|
||||
### L6. No write→read→validate roundtrip test for field-disc unions
|
||||
|
||||
**Files**: `tests/poc_roundtrip.rs` (no field-disc case at all),
|
||||
`tests/tunion_dispatch.rs` (tunion-level only), the builder tests
|
||||
(write-side only), the reader tests (read-side only)
|
||||
|
||||
**Problem**: every field-disc union test lives on one side of the
|
||||
wire. The builder tests assume the variant-only layout
|
||||
(`layout_builder.rs:1221-1334`); the reader/materializer tests assume
|
||||
the shared-then-variant walk (`sequential_reader.rs:1260-1305`,
|
||||
`materialize.rs:1183-1258`) — and the two fixtures don't even use the
|
||||
same schema shape, so nothing would catch H3's divergence. A single
|
||||
roundtrip test (build with `LayoutBuilder` → feed the buffer to
|
||||
`SequentialReader::read_next` and `materialize_packed` →
|
||||
`validate_bytes` on the engine) over a field-disc union with a
|
||||
*non-redeclaring* variant and a second shared field would have caught
|
||||
H3 immediately. This is the highest-value single test in the review —
|
||||
add it with (or before) the H3 fix.
|
||||
|
||||
### N1. Field-discriminator kind support differs between `tunion` and the reader
|
||||
|
||||
**Files**: `src/tunion.rs:136-164` (supports `String`/`Uint8`/`Enum`),
|
||||
`src/sequential_reader.rs:613-645` (supports `String`/
|
||||
`Uint8`/`Uint16`/`Uint32`/`Enum`)
|
||||
|
||||
**Problem**: `tunion::read_field_discriminator` rejects uint16/uint32
|
||||
discriminator fields with a `Schema` error, while the plan reader
|
||||
handles them. Parity-preserved (tunion is unchanged from 0.2.0), but
|
||||
the public dispatch surface now has two answers to "which field kinds
|
||||
can discriminate a union". Either extend `tunion` to match the reader
|
||||
or document the divergence; the meta-schema does not constrain the
|
||||
discriminator field's kind, so both code paths are reachable from the
|
||||
same schema.
|
||||
|
||||
---
|
||||
|
||||
## What's Good
|
||||
|
||||
Worth recording, because the findings shouldn't eclipse it:
|
||||
|
||||
- **The compiled forms are well-built.** Both `ReadPlan::compile` and
|
||||
`ValidationPlan::compile` carry their own depth cap + definition-level
|
||||
cycle set (the correct untrusted-input shape), and the
|
||||
cycle-set-is-path-scoped semantics are tested for diamond refs on
|
||||
both (`repeated_ref_to_shared_def_is_allowed`,
|
||||
`shared_refs_compile_without_false_cycle`).
|
||||
- **The engine's ValidationPlan-as-cycle-gate layering**
|
||||
(`engine.rs:146-152`) is a genuinely clever piece of defensive design,
|
||||
and it is documented at the place a future maintainer will read it.
|
||||
- **Parity discipline is real, not ritual.** The effective-endian
|
||||
propagation divergence (phase 5) was found, fixed, and locked with
|
||||
tests on both the plan and the offset map; the stride fix (deferred
|
||||
decision 4) is documented in the `FieldValue::Array` doc comment with
|
||||
the behavioral-change note; the union paths were compared arm-by-arm
|
||||
against the 0.2.0 `read_union_value` in this review and match on
|
||||
every dispatch shape.
|
||||
- **The fingerprint derives are sound.** Verified against the pinned
|
||||
`serde_json` 1.0.150 source: under `preserve_order`,
|
||||
`Map::hash` sorts keys (`map.rs:418-429`), so `Value: Hash` is
|
||||
consistent with `Value: Eq` and the phase-6 plan's analysis holds.
|
||||
Contract tests on all three plans (ReadPlan/OffsetMap/ValidationPlan)
|
||||
plus the root-name sensitivity test.
|
||||
- **`Send + Sync` static assertions** on the plan types (phase 1 +
|
||||
phase 7), as ADR-011 required.
|
||||
- **Docs/status hygiene is accurate.** Every status flip, bench number,
|
||||
and "closed" claim in the release commit checked out against the
|
||||
actual files.
|
||||
- **Semver contract honored**: `lib.rs` re-exports match the plan's
|
||||
contract table exactly; the breaking surface is exactly the declared
|
||||
one.
|
||||
|
||||
## Recommended Order
|
||||
|
||||
1. **H1** — release-blocking. Smallest correct fix: drop the three
|
||||
`Vec::with_capacity(count)`; add the compile-time cap + tests.
|
||||
Isolate the reproducer (see the Methodology warning).
|
||||
2. **H3** — needs a convention *decision* before code: write the
|
||||
addendum, enforce it, add L6's roundtrip test in the same session.
|
||||
3. **H2** — shared walk guard (or the minimum doc note if the full
|
||||
guard is judged too invasive for 0.3.x), plus the cyclic-schema
|
||||
tests for all three walkers.
|
||||
4. **M1 + M2** — same file, same test family; do together.
|
||||
5. **M3** — trivial deletion (or the feature decision, if kept).
|
||||
6. **M4** — ongoing: per-fix coverage extension as recommended above;
|
||||
the aligned-materializer test gap (1) is the single biggest chunk
|
||||
and deserves its own session.
|
||||
7. **L1–L6, N1** — opportunistic, folded into whichever session touches
|
||||
the relevant file (L6 is the exception — it belongs with H3).
|
||||
|
||||
## Notes
|
||||
|
||||
- The probe file used in this review was deleted before commit; no
|
||||
test changes ship with this review.
|
||||
- The H1 OOM incident (test run aborted the session's host) is the
|
||||
reason the two crash reproducers are described rather than committed.
|
||||
If a reproducer test is wanted in-tree for H1/H2, it should be
|
||||
`#[ignore]`-gated with a comment pointing at the isolation
|
||||
requirements, or assert only the compile-time rejection (the safe
|
||||
half of the fix) in the default suite.
|
||||
- Coverage was measured with the default harness (`cargo llvm-cov
|
||||
--release`, summary + text). Numbers quoted are stable across two
|
||||
runs this session.
|
||||
- Severity here keys off AGENTS.md §3 (untrusted schema ⇒ handleable
|
||||
error) and the semver contract, not off effort: two of the three
|
||||
Highs are single-file, small-diff fixes; H3 is the only one that
|
||||
needs a decision first.
|
||||
Reference in new issue
Block a user