Commit Graph
7 Commits
Author SHA1 Message Date
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
glm-5.2 fb3a27f974 Pre-publish docs sweep: README, AGENTS.md, licenses, inline doc fixes
- Add README.md reflecting the v0.1.0 state: 19 AlkType kinds, two
  layout modes, builder + AlkTypeEngine usage example (verified to
  compile and run), validation entry points, crate independence,
  untrusted-schemas guarantee, docs pointers. Mirrors the alkvault
  README structure.
- Add AGENTS.md with alktype-specific git workflow, project
  conventions (no comments, AlkTypeError, untrusted schemas, overflow
  safety, no async, no feature flags, wasm-clean, preserve_order
  load-bearing, no unsafe), verification commands, and ADR/OQ index.
  Blocks auto-commit on semver-relevant public API changes per the
  crates.io 0.1.0 contract.
- Add LICENSE-MIT and LICENSE-APACHE (dual MIT/Apache-2.0, matching
  alkvault and the Cargo.toml license field).
- Cargo.toml: add readme, keywords, categories, rust-version = "1.85".
- Fix broken intra-doc link in builder.rs: DiscriminatorKind ->
  crate::schema::DiscriminatorKind (cargo doc now warning-free).
- N1 (review #002): document is_rfc3339_timestamp as non-strict in the
  function doc comment. Lists the specific gaps (day-of-month per
  month, seconds range, leap seconds) and points consumers needing
  strict validation to chrono/time.
- N2 (review #002): document the FieldValue::Bytes-for-Record API
  asymmetry in the FieldValue enum doc and on read_record_value.
- .opencode/agents/implementation-specialist.md: point to AGENTS.md
  for full convention details (matches the alkvault pattern).
- review #002: mark N1/N2 resolved; all 7 findings now closed.

Verification:
- cargo test --release: 396 tests pass (310 crate + 86 integration)
- cargo clippy --all-targets -- -D warnings: clean
- cargo doc --no-deps: clean (no broken intra-doc link warnings)
- cargo build --target wasm32-unknown-unknown --release: clean
- cargo publish --dry-run --allow-dirty: clean
2026-08-11 09:33:33 +00:00
glm-5.2 5f88bca0d8 Fix L2: replace unreachable! with Err for untrusted-schema safety
The immediate downstream consumer (alkcall) accepts schemas from
arbitrary internet peers in its hub/spoke topology. A panic is the
wrong failure mode for a malicious or unsupported schema - an Err
the caller can handle is correct.

Three sites converted:
- offset_map.rs: _ => Err(Offset { unsupported AlkType kind for
  aligned offset computation })
- layout_builder.rs: _ => Err(Offset { unsupported AlkType kind
  for packed layout computation })
- materialize.rs: _ => Err(Schema { internal: union discriminator
  type N is not a supported byte discriminator })

The materialize.rs site uses Schema (not Access) because a wrong
disc_type is a schema-authoring bug (parse_discriminator should have
caught it), not a buffer-access error. The sibling sites in
sequential_reader.rs and tunion.rs already returned Err(Schema) -
only materialize.rs was the holdout.

Note: the k if k.is_fixed_size() guard in offset_map and
layout_builder means the compiler cannot enforce exhaustiveness at
compile time. Converting _ from unreachable! to Err is the runtime
mitigation. A future refactor could list all fixed-size kinds
explicitly to restore compile-time checking. Deferred to a separate
cleanup pass.

Verified: no unreachable! remains in production code (the one
remaining hit at offset_map.rs:688 is inside a #[test] fn).

cargo test --release: 396 tests pass, 0 failures
cargo clippy --all-targets -- -D warnings: clean
cargo build --target wasm32-unknown-unknown --release: clean (prior)
2026-08-11 09:02:56 +00:00
glm-5.2 a975befdd1 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.
2026-08-11 08:41:15 +00:00
glm-5.2 ee6e773123 Add pre-publish code review #002 (2 medium, 3 low, 2 nit)
Full source read of all 13 src/*.rs files for correctness, panic
safety, and API ergonomics ahead of v0.1.0 crates.io publish.

Findings:
- M1: Definitions::merge_into silently drops data via double-
  unwrap_or_default chain; untested public API
- M2: write_bytes truncates u32 length prefix on >4GiB data
  (data_len as u32 without bounds check)
- L1: materialize.rs union path uses unchecked offset arithmetic
  (sequential_reader.rs sibling uses checked_add)
- L2: three unreachable!() in production code (offset_map, layout_
  builder, materialize)
- L3: AlkTypeError::source() returns None for Validation variant
  (ValidationError implements std::error::Error)
- N1: is_rfc3339_timestamp is non-strict (Feb 31 passes, seconds
  unchecked)
- N2: SequentialReader returns FieldValue::Bytes for Record (API
  asymmetry vs other composites)

Verification baseline (commit c0217d9):
- cargo test --release: 438 tests pass (391 crate + 47 integration)
- cargo clippy --all-targets -- -D warnings: clean
- cargo build --target wasm32-unknown-unknown --release: clean
- no unsafe, no TODO/FIXME, all unwrap/expect/panic in test modules
2026-08-11 08:38:39 +00:00
glm-5.2 57d8ed25ba Add tests for coverage gaps S1, S2, S3, S5, S6, S7 (285→346 tests, 88.9%→91.9% lines)
- S1 (error.rs): 5 tests for Display impl on all 4 AlkTypeError variants
  + Error::source(). error.rs 0%→100%.
- S2 (validation.rs, macros.rs): 28 tests for validate() (Result-returning)
  method on every validator + factory rejection arms for all 12 keywords.
  validation.rs 85%→97.5% lines / 100% fns; macros.rs 75.7%→93.1% / 100% fns.
- S3 (engine.rs): 3 tests for read_field on nested struct leaf fields, Bytes,
  and Timestamp. engine.rs fns 95%→95.3%.
- S5 (layout_builder.rs): 2 tests for union 'variant must be Struct' error
  (byte + field discriminator).
- S6 (schema.rs): 6 tests for as_str() round-trip (all 19 kinds), Display,
  needs_endian(), is_composite(), get_alktype_kind_enum(). schema.rs
  91.2%→97.5% lines / 94.3% fns.
- S7 (offset_map.rs): 1 test for nested-struct-without-properties schema error.

Updated docs/reviews/001-coverage-analysis.md with resolution section and
partial-resolved status. Remaining: S4 (sequential_reader error paths, medium
effort) and S8 (overflow guards, recommended to skip for v1).
2026-08-02 08:04:44 +00:00
glm-5.2 9fb85417b5 Add coverage analysis review #001 (88.9% lines, 82.3% fns, 8 suggestions) 2026-08-02 07:59:59 +00:00