Fix M1, M2, L1, L3 from code review #002
Four of seven review findings resolved. 5 new tests (391 -> 396 crate tests; 438 -> 443 total). cargo test, clippy, wasm32 all green. M2 (data_access.rs): write_bytes now validates data_len fits in u32 before the length-prefix cast. A >4GiB blob returns Access error instead of silently writing a truncated length prefix (silent data corruption on read-back). M1 (builder.rs): Definitions::merge_into rewritten to access self.defs directly instead of round-tripping through self.build() with a double- cloned/unwrap_or_default chain that could silently drop definitions on a shape mismatch. 4 new tests: insert-when-absent, merge-into- existing, overwrite-duplicate-keys, no-op-on-non-object-top. L1 (materialize.rs): byte-offset discriminator arm of materialize_union_packed now uses checked_add for offset+disc_offset and disc_abs_offset+disc_size, returning Access error on overflow. Mirrors the existing sequential_reader.rs::read_union_value pattern. L3 (error.rs): AlkTypeError::source() now returns Some(inner) for the Validation variant (jsonschema::ValidationError implements std::error::Error). Existing source_returns_none_for_all_variants test split into source_returns_none_for_schema_offset_access and source_returns_some_for_validation_variant. Deferred: L2 (unreachable! -> Err, defense-in-depth), N1 (non-strict RFC 3339 validator, docs-only), N2 (FieldValue::Bytes for Record, API asymmetry). Review doc updated with resolution section.
This commit is contained in:
@@ -1,5 +1,5 @@
|
||||
---
|
||||
status: open
|
||||
status: resolved (M1, M2, L1, L3); open (L2, N1, N2)
|
||||
last_updated: 2026-08-11
|
||||
reviewed_artifacts:
|
||||
- src/lib.rs
|
||||
@@ -465,4 +465,70 @@ sanity check (README, inline docs, docs.rs render) and publish.
|
||||
separate sweep after the code is settled.
|
||||
- Coverage gaps from review #001 (S4 sequential reader error paths,
|
||||
S8 overflow guards) are not re-litigated here. They remain
|
||||
coverage gaps, not correctness issues.
|
||||
coverage gaps, not correctness issues.
|
||||
|
||||
---
|
||||
|
||||
## Resolution (2026-08-11)
|
||||
|
||||
Four of the seven findings were resolved in the same session as the
|
||||
review. 5 new tests added (391 → 396 crate tests; 438 → 443 total
|
||||
with integration tests). `cargo test`, `cargo clippy --all-targets --
|
||||
-D warnings`, and `cargo build --target wasm32-unknown-unknown
|
||||
--release` all green.
|
||||
|
||||
### M2 (u32 truncation in `write_bytes`) — resolved
|
||||
|
||||
`src/data_access.rs`: added a `u32::try_from(data_len)` guard before
|
||||
the `u32_to` call. A >4GiB blob now returns
|
||||
`AlkTypeError::Access { reason: "data length N exceeds u32::MAX
|
||||
(length prefix width)" }` instead of silently writing a truncated
|
||||
length prefix. ~3 lines.
|
||||
|
||||
### M1 (`Definitions::merge_into` silent data loss) — resolved
|
||||
|
||||
`src/builder.rs`: rewrote `merge_into` to access `self.defs` directly
|
||||
instead of round-tripping through `self.build()`. The double-
|
||||
`cloned().unwrap_or_default().as_object().cloned().unwrap_or_default()`
|
||||
chain is gone — the definitions are moved directly into the target's
|
||||
`$defs` object. 4 new tests cover: insert-when-absent, merge-into-
|
||||
existing, overwrite-duplicate-keys, and no-op-on-non-object-top.
|
||||
~15 lines + ~50 lines of tests.
|
||||
|
||||
### L1 (materialize.rs unchecked offset arithmetic) — resolved
|
||||
|
||||
`src/materialize.rs`: the byte-offset discriminator arm of
|
||||
`materialize_union_packed` now uses `checked_add` for both
|
||||
`offset + disc_offset` and `disc_abs_offset + disc_size`, returning
|
||||
`AlkTypeError::Access` on overflow. Mirrors the existing pattern in
|
||||
`sequential_reader.rs::read_union_value`. The `variant_offset`
|
||||
computation is now overflow-safe. ~15 lines.
|
||||
|
||||
### L3 (`AlkTypeError::source()` for `Validation`) — resolved
|
||||
|
||||
`src/error.rs`: replaced the blanket `impl std::error::Error for
|
||||
AlkTypeError {}` with an explicit impl that returns `Some(inner)` for
|
||||
the `Validation` variant and `None` for the others. The existing
|
||||
`source_returns_none_for_all_variants` test was split into two:
|
||||
`source_returns_none_for_schema_offset_access` (unchanged behavior)
|
||||
and `source_returns_some_for_validation_variant` (new). ~6 lines +
|
||||
~10 lines of tests.
|
||||
|
||||
### Deferred
|
||||
|
||||
- **L2** (`unreachable!` → `Err`): the three `unreachable!` sites are
|
||||
genuinely unreachable today. Converting them to `Err` is defense-in-
|
||||
depth against a future `AlkTypeKind` variant addition or a
|
||||
`parse_discriminator` logic bug. Deferred until the "load untrusted
|
||||
schemas" use case is on the roadmap — until then, the exhaustiveness
|
||||
check is the safety net.
|
||||
- **N1** (non-strict `is_rfc3339_timestamp`): documented as "simple"
|
||||
in the existing doc comment. A strict implementation would add a
|
||||
`chrono` or `time` dependency, not worth it for 0.1.0. Will add an
|
||||
explicit "non-strict" note in the docs sweep.
|
||||
- **N2** (`FieldValue::Bytes` for `Record`): API asymmetry, not a
|
||||
bug. Revisit if the alkcall consumer finds it awkward.
|
||||
|
||||
After M1, M2, L1, and L3, the remaining open findings (L2, N1, N2)
|
||||
are all deferrable. The crate is ready for the pre-publish docs sweep
|
||||
(README, inline doc cleanup for docs.rs) and the final sanity check.
|
||||
Reference in New Issue
Block a user