alktype/docs/reviews/008-pre-publish-review.md

status, last_updated, reviewed_artifacts, tool, reviewer

status	last_updated	reviewed_artifacts	tool	reviewer
resolved (F1, F2 fixed 2026-09-07; N1, N2, N3 classified; N4 fixed 2026-09-07)	2026-09-07	src/read_plan.rs src/sequential_reader.rs src/materialize.rs src/bast.rs benches/wire_vs_bast.rs docs/reviews/007-coverage-audit.md (N3a disposition)	manual diff review of post-#007 commits (dea96f0, d4635d2) + disposable probe tests (run in-session, then deleted) + cargo bench + counting-allocator peak-RSS probe	pre-publish review

Review #008 — Pre-Publish Review: Post-#007 Perf Commits

Purpose

0.3.0's release commit (9949f91) predates review #006 entirely; the fix sessions for #006 and #007 landed eleven more commits, and after #007 closed, two more commits landed unreviewed: dea96f0 (bench port from alktty) and d4635d2 (the perf commit — fixed-size struct fast path, integer union dispatch, read_next_borrowed). The perf commit touches the flagship packed read path, which every untrusted wire buffer flows through. This review audits those two commits before the first crates.io publish of the 0.3.x line (0.1.0 and 0.2.0 are published; 0.3.0 never was — every post-release fix can legally ride inside the first published 0.3.0, no semver conflict).

It also disposes of review #007's N3a — the one finding explicitly deferred to "the pre-release review", which this session is.

Methodology

• Full diff read of d4635d2 (perf) and dea96f0 (bench port), cross-checked against the invariants the earlier reviews established: cross-consumer dispatch agreement (#006 H3, #007 F1), the H1 no-count-sized-prealloc rule, and the N2/F2 dual-layer cap pattern.
• Disposable probe tests (tests/zzz_probe*.rs, deleted after the session; none was a crash hazard) to confirm/deny the three behaviors code reading flagged: the int_keys non-canonical-key divergence, the engine's nested-array acceptance envelope, and the with_capacity amplification.
• A counting-GlobalAlloc probe (peak-bytes metric) to measure the worst-case simultaneous allocation of the amplification shape precisely — RSS timing proved too noisy to separate the two test cases.
• cargo bench --quick before/after the fixes to confirm the perf commit's wins survive.
• N3a dispositions probed where cheap (inline-struct union variants).

Baseline

Audited at main HEAD d4635d2, 0.3.0, working tree clean. 566 tests green (488 lib + 17 + 34 + 15 + 12, + 2 ignored doctests), clippy -D warnings clean, wasm build green — per the perf commit's verification block.

Summary Statistics

Severity	Count	Status
High	0	—
Medium	2 (F1, F2)	both fixed 2026-09-07
Info	3 (N1, N2, N3)	classified
Fix	1 (N4)	fixed 2026-09-07

No Highs: both Mediums are probe-verified cross-consumer divergences and resource-bound violations, but neither aborts the process (with_capacity is now bounded per array by MAX_ARRAY_ELEMENTS, so H1's 1 TB SIGABRT class does not return). Both were fixed in-session because they violate AGENTS.md §3 (divergent verdicts on untrusted input; unbounded-count-shaped allocation) — the publish gate.

Resolution log:

• F1 + F2 (2026-09-07): fixed in one commit — see the resolution blocks. 569 tests green (491 lib + 17 + 34 + 15 + 12, + 2 ignored), clippy -D warnings clean, doc 0 warnings, wasm green.
• N4 (2026-09-07): fixed with F1/F2 — see the block.

---

Findings

F1. int_keys integer dispatch breaks cross-consumer agreement on non-canonical mapping keys

Files: src/read_plan.rs (compile_int_keys, introduced by d4635d2), contrast src/materialize.rs (materialize_plan_union's byte-disc arm — stringifies), src/validation_plan.rs (validate_union_numeric — stringifies), src/tunion.rs (read_byte_discriminator — stringifies)

Problem: d4635d2 added a pre-parsed (u64, variant_index) dispatch table for byte-discriminator unions: when every mapping key parses as u64, the reader matches the raw discriminator integer instead of stringifying per read. But the meta-schema does not constrain mapping-key shape beyond "object property name", and key.parse::<u64>() accepts non-canonical decimal strings:

PROBE1 reader: field=msg disc="01" (DISPATCHED)
PROBE1 validate_bytes: Err(access error at msg: union discriminator value 1 not in mapping)

With mapping key "01" (and discriminator 1 on the wire): the reader's numeric dispatch matches ("01".parse::<u64>() == 1) and dispatches — returning discriminator == "01" — while the materializer (1.to_string() == "1" ≠ "01"), the validation plan, and tunion all reject the identical buffer. Pre-d4635d2, all four consumers stringified and all four rejected — agreement held (both verdicts "reject", same error class). The perf commit flipped the reader to accept-while-everyone-else-rejects: the exact cross-consumer-divergence shape #006 H3 and #007 F1 exist for, on the flagship path. "+1" parses as u64 too (Rust's from_str_radix accepts a leading +) — same class. The returned key string also became schema-quirk-dependent: the reader reports "01" where the materializer's __discriminator for a matched key would report the stringified form.

Not a #007 regression: int_keys did not exist before d4635d2. But d4635d2 postdates #007's close and was unreviewed — this is the audit catching it.

Fix: build the integer table only from canonical keys — a key qualifies iff key.parse::<u64>() succeeds and parsed.to_string() == key (i.e. the key is exactly what stringification would produce). Any non-canonical or non-numeric key falls back to the string path (int_keys: None), which every consumer already agrees on. No accepted schema's reachable behavior changed: for fully-canonical mappings the numeric dispatch behaves identically to stringified matching (the numeric value's to_string() equals the key), and for non-canonical keys all consumers now reject exactly as before d4635d2. The perf win (no per-read stringify) is preserved for every mapping that was unambiguous to begin with.

Resolution (2026-09-07): exactly that — compile_int_keys now requires v.to_string() == *key for the table to carry the entry; any miss returns Ok(None) (string fallback). Doc comment states the canonicality rule and why. Tests in read_plan.rs: r8_non_canonical_mapping_key_disables_int_dispatch (key "01" → int_keys is None) and r8_canonical_mapping_keys_keep_int_dispatch (keys "1","2" → table [(1,0),(2,1)]). Probe output after the fix: both validate_bytes and the reader reject disc 1 under key "01" with the same error class — agreement restored.

F2. Vec::with_capacity(count) reintroduced on both array materializers — ~477 MB simultaneous allocation from a ~1 KB schema

Files: src/materialize.rs:247 (materialize_plan_array — the validate_bytes packed path), src/materialize.rs:650 (materialize_array_packed — the legacy walker, reachable via the aligned record arm)

Problem: d4635d2's "materialize: with_capacity for bytes arrays, arrays, and struct objects" item reintroduced Vec::with_capacity(count) at two of the three sites H1's layer-1 fix had converted to Vec::new() + push. count is now compile-capped at MAX_ARRAY_ELEMENTS (2^16), so H1's 1 TB SIGABRT does not return — but the per-array cap does not bound nesting:

PROBE validate peak bytes allocated simultaneously: 476780249

A schema of one 100-level nested array chain (each count: 65535, innermost elements empty structs — all legal: the depth cap is 128 and stride-0 chains evade MAX_ARRAY_BYTES, which only checks stride products) peaks at ~477 MB of simultaneous allocation on validate_bytes(&[]) from a ~1 KB schema and an empty buffer. Each level's with_capacity(65535 × sizeof(Value)) stays live across its element walk, so the sizes multiply across ~127 legal depth levels (the innermost zero-progress rejection fires only after the whole chain has descended). On wasm32 — which this crate explicitly targets — the same shape aborts the wasm heap well below 477 MB. H1's layer-1 rule ("no count-sized prealloc on untrusted input; the per-element walk dominates") is exactly the invariant this violates; the perf commit's own bench evidence doesn't need the prealloc either (see below).

The other with_capacity additions in the commit are fine: byte-array capacity from b.len() (a read slice), struct-object capacity from plan.fields().len(), and the plan-compiler's from fields.len() — all bounded by data/plan already in hand, not by declared counts.

Fix: restore H1's layer-1 shape at both sites — Vec::new() + push loop (the loops already push count elements; the zero-progress guard bounds honest progress per element). Optionally cap preallocation at a small constant, but plain Vec::new() matches H1's shipped behavior.

Resolution (2026-09-07): both sites restored to Vec::new() + push. Bench before/after (criterion --quick, this session): packet read 246 → 220 µs, chunk read 76/68 µs — the revert costs nothing measurable on the bench shapes (small arrays; the materializer's per-element work dominates), and the union/struct preallocs stay. Locking test in materialize.rs: r8_deeply_nested_stride0_array_rejects_before_bulk_prealloc (the 100-level chain still rejects cleanly with the zero-progress error at the innermost level; the allocation shape itself is documented here — in-tree cannot cheaply assert peak allocation, and the #008 probe was deleted per the no-reproducer rule).

N1. d4635d2's fixed-size fast paths are sound (classified, no action)

Files: src/read_plan.rs (fixed_size, fixed_plan_size), src/sequential_reader.rs

The fixed-size struct fast path replaces the cursor size walk with one bounds check; fixed_plan_size already returned Result<Option> with clean overflow errors (L1's shape), and every new error arm formats paths lazily on the error path only. The union-variant fast path (plan_variant_fixed_size) applies only to struct variants and checks bounds before use. No issue found.

N2. Bench port (dea96f0) is methodology-honest (classified, no action)

Files: benches/wire_vs_bast.rs

The port drops alktty's async I/O group (correctly — it measured a different stack) and adds a parity check before measurement so the stream loop can't drift. The historical read_chunk_stream numbers stay comparable by construction. No issue found.

N3. N3a dispositions (review #007's deferred items)

Files: src/bast.rs (source() accessors, BastField::synthetic, resolve_typeref_as_def's inline arms)

• source() accessors (7 sites): public API on BastStruct/ BastUnion/BastField/etc. Removal is a semver decision and AGENTS.md's semver exception requires an explicit ask — kept. They are one-line accessors over parsed source nodes, harmless, and plausibly useful to downstream codegen (the announced consumer).
• BastField::synthetic (#[allow(dead_code)], zero callers): pub(crate), not public API — deleted (2026-09-07). No semver impact; the #[allow(dead_code)] suppression is gone with it.
• resolve_typeref_as_def's inline struct/union/enum arms: the review-#007 suspicion ("plausibly dead after H3") was wrong — probe-verified reachable: the meta-schema's mapping.additionalProperties: TypeRef accepts inline struct variants, and LayoutBuilder's byte-disc and field-disc arms call resolve_typeref_as_def on every union variant. The H3 parse rules forbid variant re-declaration of shared fields, not inline variant bodies. Kept, reachable.

N4. Stale test-count references in review #006's resolution log

Files: docs/reviews/006-implementation-review-030.md

The bookkeeping note ("static count at 2eb086f is 542 + 2 ignored") and per-commit counts are accurate as written; no fix needed. Recorded here so the review trail stays honest about what was re-checked during this session's doc sweep. Resolution (2026-09-07): no code change; superseded the "Fix" entry — this is the classification record.

---

What's Good

• The perf commit's core ideas are sound and survived review: the compile-time fixed_size cache is computed through the existing Result-returning sizer (no unwrap_or_default regression), and the int-dispatch table's design was right — it just needed the canonicality gate.
• The counting-allocator probe took 15 minutes and converted a "probably too big" into a precise number (476,780,249 bytes) — the same probe pattern the earlier reviews used, applied to allocation instead of verdicts.
• cargo bench --quick before/after the fixes is the right tool for guarding perf-fix reverts: packet read 220 µs post-fix vs 246 µs baseline confirms the Vec::new() restore is free.

Recommended Order

1. F1 — canonical-key gate on compile_int_keys fixed 2026-09-07.
2. F2 — restore H1's no-prealloc rule at both array sites fixed 2026-09-07.
3. N4 — BastField::synthetic deletion fixed 2026-09-07.
4. N3 source() accessors — revisit only if/when the codegen consumer confirms it does not want them (removal needs an explicit ask per AGENTS.md).

Notes

• Probe tests were run as tests/zzz_probe*.rs in-tree during the session and deleted before any commit (the #006 pattern). None was a crash hazard; the amplification probe allocates ~477 MB transiently and completes in ~40 ms.
• Benches are not run in CI and are excluded from the publish (the [bench] target ships — that is fine; benches don't affect the library's API or its wasm compatibility).
• The 0.3.0 publish proceeds after these fixes: 0.1.0 and 0.2.0 are on crates.io; this is the first 0.3.0 publish, so F1/F2's behavior changes (both "previously-divergent, now-agreed" shapes) land inside the version's first release — no semver bump implied.