Close review #007 coverage holes C1-C3, L1, L2 (locking tests)
- C1: packed validate_bytes now exercised over all eleven primitive kinds (LE battery + BE subset + corrupted-bool rejection) — the plan materializer's i16..bool arms had zero public-path executions. - C2: aligned validate_bytes over the default inline length-prefixed encoding (string + bytes; ADR-006 last-position rule honored). - C3: ReadPlan::compile cycle rejection through a union mapping entry (compile_variant's own cycle arm — field-level cycles were already covered; this shape reaches the variant path). Arm confirmed executed in the post-fix coverage run. - L1: builder.rs standard JSON-Schema conveniences locked with exact- JSON table tests, plus an end-to-end build_validator compile test. - L2: tunion::read_field_discriminator's enum arm (both endians) — the last untested arm of the documented kind set (N1 parity). docs/reviews/007-coverage-audit.md updated with per-finding resolution blocks. Verification: 488 lib + 78 integration tests green, clippy -D warnings clean, wasm32-unknown-unknown build green.
This commit is contained in:
1 parent
844c199fb8
commit
7e5e58aa1b
5 files changed
+335
-21
No files matched your search
@@ -69,6 +69,13 @@ clean. Full suite green (548 tests static + 2 ignored doctests, per
|
||||
resolution blocks on each finding. 477 lib tests green (511
|
||||
static + 2 ignored doctests across all targets), clippy
|
||||
`-D warnings` clean, wasm build green.
|
||||
- **C1 + C2 + C3 (2026-09-02):** resolved in one commit — see the
|
||||
resolution blocks. 481 lib tests green, clippy `-D warnings` clean,
|
||||
wasm build green; `compile_variant`'s cycle arm confirmed executed
|
||||
in the post-fix coverage run.
|
||||
- **L1 + L2 (2026-09-02):** resolved in one commit — see the
|
||||
resolution blocks. 488 lib tests green, clippy `-D warnings` clean,
|
||||
wasm build green.
|
||||
|
||||
---
|
||||
|
||||
@@ -217,12 +224,11 @@ frames; one battery test closes it (mirror the aligned
|
||||
`read_field` battery, tests/engine_integration.rs:130-190, which
|
||||
already covers all twelve primitive kinds on the aligned side).
|
||||
|
||||
**Fix**: one `validate_bytes` test in packed mode with all twelve
|
||||
primitive kinds (LE), plus a BE variant of a subset. Assert both
|
||||
acceptance and a corrupted-value rejection.
|
||||
|
||||
**Resolution (2026-09-02)**: fixed this session — see the resolution
|
||||
block.
|
||||
**Resolution (2026-09-02):** two tests in `engine.rs`:
|
||||
`c1_validate_bytes_packed_decodes_all_twelve_primitives_le` (the full
|
||||
eleven-field battery — i8..bool — plus a corrupted-bool rejection arm)
|
||||
and `c1_validate_bytes_packed_decodes_big_endian_subset` (BE i16/u64/
|
||||
f64 through the same public path). Both green.
|
||||
|
||||
### C2. Aligned `validate_bytes` never exercises the default inline encoding for string/bytes
|
||||
|
||||
@@ -242,8 +248,13 @@ public path.
|
||||
string (and a bytes variant or arm), asserting acceptance plus a
|
||||
short-buffer rejection.
|
||||
|
||||
**Resolution (2026-09-02)**: fixed this session — see the resolution
|
||||
block.
|
||||
**Resolution (2026-09-02):**
|
||||
`c2_validate_bytes_aligned_inline_string_and_bytes_default_encoding`
|
||||
in `engine.rs`. One wrinkle the test wrote itself into: ADR-006
|
||||
allows an inline length-prefixed variable field only in the final
|
||||
position, so the string and bytes shapes get separate one-field
|
||||
schemas (string after a fixed `id`; bytes as a lone field). Asserts
|
||||
acceptance for both plus a short-buffer rejection for the string.
|
||||
|
||||
### C3. `ReadPlan::compile`'s union-variant cycle arm is untested standalone
|
||||
|
||||
@@ -267,8 +278,15 @@ first (engine.rs:153).
|
||||
`ReadPlan::compile` directly on a two-def cycle, mirroring
|
||||
`validation_plan.rs`'s existing standalone cycle test.
|
||||
|
||||
**Resolution (2026-09-02)**: fixed this session — see the resolution
|
||||
block.
|
||||
**Resolution (2026-09-02):**
|
||||
`c3_cycle_through_union_mapping_variant_is_schema_error` in
|
||||
`read_plan.rs`. Writing the test sharpened the finding: the
|
||||
*field-level* cycle arm (`compile_typeref`, :362) was already covered
|
||||
(2 execs) by `cyclic_ref_through_two_defs_is_schema_error`; the
|
||||
0-exec arm was `compile_variant`'s own check (:513), reachable only
|
||||
when the cycle closes through a **union mapping entry**. The new
|
||||
test's shape (`A → B → U(mapping: "1" → $ref B)`) trips exactly that
|
||||
arm — verified post-fix at the line level (1 execution).
|
||||
|
||||
### L1. `builder.rs`'s JSON-Schema conveniences are entirely untested
|
||||
|
||||
@@ -286,8 +304,19 @@ misplaced keyword would ship silently — the builder emits the JSON,
|
||||
table-style test asserting each convenience produces the expected
|
||||
JSON key/value closes the surface cheaply.
|
||||
|
||||
**Resolution (2026-09-02)**: fixed this session — see the resolution
|
||||
block.
|
||||
**Resolution (2026-09-02):** five tests in `builder.rs`:
|
||||
`l1_standard_type_constructors_produce_type_keyword` (all eight
|
||||
standard constructors, exact-JSON assertions),
|
||||
`l1_field_on_standard_object_builds_properties` (`field()` on the
|
||||
standard repr + `required()`),
|
||||
`l1_items_and_additional_properties_on_standard_types`,
|
||||
`l1_constraint_keywords_emit_expected_json_keys` (minimum/maximum/
|
||||
minLength/minItems/maxItems/format/title/description, each asserted
|
||||
on its exact keyword), and
|
||||
`l1_standard_built_schema_compiles_as_json_validator` (the end of the
|
||||
translation chain: the emitted JSON builds a `jsonschema` validator
|
||||
and the constraints actually bite — valid passes, over-maximum/
|
||||
missing-required/below-minimum fail).
|
||||
|
||||
### L2. `tunion::read_field_discriminator`'s enum arm is 0-exec
|
||||
|
||||
@@ -300,8 +329,10 @@ enum arm, which is in the documented kind set
|
||||
The reader's enum arm is tested (`m4_field_disc_enum_dispatches_on_index`);
|
||||
tunion's is not. One test locks parity on the last arm.
|
||||
|
||||
**Resolution (2026-09-02)**: fixed this session — see the resolution
|
||||
block.
|
||||
**Resolution (2026-09-02):** `l2_read_field_discriminator_enum_
|
||||
dispatches_on_index` and `l2_read_field_discriminator_enum_big_endian`
|
||||
in `tunion.rs` — enum index 0 (LE) and 1 (BE) dispatch with
|
||||
`variant_offset == 4` / `discriminator_size == 4`.
|
||||
|
||||
### N1. `OffsetEntry::start()`/`end()` 0-execution in the combined run is a merge artifact, not a hole
|
||||
|
||||
@@ -376,12 +407,12 @@ Leave uncovered; the pattern is consistent across the codebase.
|
||||
|
||||
## Recommended Order
|
||||
|
||||
1. **F1** — guard hoist + cross-consumer agreement test (small,
|
||||
real-behavior fix).
|
||||
2. **F2** — `MAX_LENGTH` cap, N2's dual-layer playbook verbatim.
|
||||
3. **C1 + C2 + C3** — one locking test each, same session if
|
||||
convenient.
|
||||
4. **L1 + L2** — posture tests, cheap.
|
||||
1. ~~**F1** — guard hoist + cross-consumer agreement test~~
|
||||
**resolved 2026-09-02** (with F2).
|
||||
2. ~~**F2** — `MAX_LENGTH` cap, N2's dual-layer playbook verbatim~~
|
||||
**resolved 2026-09-02** (with F1).
|
||||
3. ~~**C1 + C2 + C3** — one locking test each~~ **resolved 2026-09-02**.
|
||||
4. ~~**L1 + L2** — posture tests~~ **resolved 2026-09-02**.
|
||||
5. **N3a** — defer to the pre-release review (semver decision).
|
||||
|
||||
## Notes
|
||||
@@ -397,4 +428,6 @@ Leave uncovered; the pattern is consistent across the codebase.
|
||||
- The coverage holes fixed this session (C1-C3, L1, L2) were chosen
|
||||
because each is load-bearing *and* one-test-cheap; the remaining
|
||||
uncovered mass is dominated by N2a/N3a/N4a, which are documented
|
||||
rather than forced.
|
||||
rather than forced.
|
||||
- Static test counts at the review-#007 commits: 477 (F1/F2),
|
||||
481 (C1-C3), 488 (L1/L2) — +14 net from the pre-audit 474.
|
||||
@@ -1386,4 +1386,93 @@ mod tests {
|
||||
);
|
||||
assert!(engine.is_ok(), "record should compile: {engine:?}");
|
||||
}
|
||||
|
||||
// ----- Standard JSON-Schema conveniences (review #007 L1) -----------
|
||||
|
||||
#[test]
|
||||
fn l1_standard_type_constructors_produce_type_keyword() {
|
||||
assert_eq!(Schema::object().build(), json!({ "type": "object" }));
|
||||
assert_eq!(Schema::array().build(), json!({ "type": "array" }));
|
||||
assert_eq!(Schema::string_().build(), json!({ "type": "string" }));
|
||||
assert_eq!(Schema::integer().build(), json!({ "type": "integer" }));
|
||||
assert_eq!(Schema::number().build(), json!({ "type": "number" }));
|
||||
assert_eq!(Schema::boolean_().build(), json!({ "type": "boolean" }));
|
||||
assert_eq!(Schema::null().build(), json!({ "type": "null" }));
|
||||
assert_eq!(Schema::any().build(), json!({}));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn l1_field_on_standard_object_builds_properties() {
|
||||
let s = Schema::object()
|
||||
.field("id", Schema::integer())
|
||||
.field("tag", Schema::string_())
|
||||
.required(&["id"]);
|
||||
assert_eq!(
|
||||
s.build(),
|
||||
json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"id": { "type": "integer" },
|
||||
"tag": { "type": "string" }
|
||||
},
|
||||
"required": ["id"]
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn l1_items_and_additional_properties_on_standard_types() {
|
||||
let arr = Schema::array().items(Schema::integer());
|
||||
assert_eq!(
|
||||
arr.build(),
|
||||
json!({ "type": "array", "items": { "type": "integer" } })
|
||||
);
|
||||
let obj = Schema::object()
|
||||
.additional_properties(Schema::boolean_());
|
||||
assert_eq!(
|
||||
obj.build(),
|
||||
json!({ "type": "object", "additionalProperties": { "type": "boolean" } })
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn l1_constraint_keywords_emit_expected_json_keys() {
|
||||
// Each convenience setter must land on the exact JSON Schema
|
||||
// keyword jsonschema interprets — a typo here would silently
|
||||
// drop the constraint (nothing else checks the translation).
|
||||
let s = Schema::integer()
|
||||
.minimum(1.0)
|
||||
.maximum(100.0);
|
||||
assert_eq!(
|
||||
s.build(),
|
||||
json!({ "type": "integer", "minimum": 1.0, "maximum": 100.0 })
|
||||
);
|
||||
|
||||
let str_schema = Schema::string_().min_length(2).format("uri").title("T").description("D");
|
||||
assert_eq!(
|
||||
str_schema.build(),
|
||||
json!({ "type": "string", "minLength": 2, "format": "uri", "title": "T", "description": "D" })
|
||||
);
|
||||
|
||||
let arr_schema = Schema::array().min_items(1).max_items(10);
|
||||
assert_eq!(
|
||||
arr_schema.build(),
|
||||
json!({ "type": "array", "minItems": 1, "maxItems": 10 })
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn l1_standard_built_schema_compiles_as_json_validator() {
|
||||
// End of the translation chain: the emitted JSON is accepted by
|
||||
// build_validator and the constraints actually bite.
|
||||
let schema = Schema::object()
|
||||
.field("id", Schema::integer().minimum(0.0).maximum(10.0))
|
||||
.required(&["id"]);
|
||||
let validator = crate::validation::build_validator(&schema.build())
|
||||
.expect("standard schema compiles");
|
||||
assert!(validator.validate(&json!({ "id": 5 })).is_ok());
|
||||
assert!(validator.validate(&json!({ "id": 11 })).is_err());
|
||||
assert!(validator.validate(&json!({})).is_err());
|
||||
assert!(validator.validate(&json!({ "id": -1 })).is_err());
|
||||
}
|
||||
}
|
||||
+137
@@ -1292,6 +1292,85 @@ mod tests {
|
||||
assert!(engine.validate_bytes(&buf).is_ok(), "valid mixed struct should pass");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn c1_validate_bytes_packed_decodes_all_twelve_primitives_le() {
|
||||
// The plan materializer's i16/i32/i64/u64/f64/bool arms had
|
||||
// zero public-path executions before this battery (review #007
|
||||
// C1): every packed validate_bytes test fed u8/u32/string
|
||||
// shapes.
|
||||
let doc = json!({
|
||||
"$defs": {
|
||||
"S": {
|
||||
"kind": "struct",
|
||||
"endian": "little",
|
||||
"fields": [
|
||||
{ "name": "i8", "kind": "int8" },
|
||||
{ "name": "i16", "kind": "int16" },
|
||||
{ "name": "i32", "kind": "int32" },
|
||||
{ "name": "i64", "kind": "int64" },
|
||||
{ "name": "u8", "kind": "uint8" },
|
||||
{ "name": "u16", "kind": "uint16" },
|
||||
{ "name": "u32", "kind": "uint32" },
|
||||
{ "name": "u64", "kind": "uint64" },
|
||||
{ "name": "f32", "kind": "float32" },
|
||||
{ "name": "f64", "kind": "float64" },
|
||||
{ "name": "b", "kind": "bool" }
|
||||
]
|
||||
}
|
||||
}
|
||||
});
|
||||
let engine = AlkTypeEngine::compile(&doc, "S", LayoutMode::Packed, None).expect("compile");
|
||||
let mut buf = vec![0u8; 1 + 2 + 4 + 8 + 1 + 2 + 4 + 8 + 4 + 8 + 1];
|
||||
let mut off = 0;
|
||||
buf[off] = 0x81; off += 1; // i8 = -127
|
||||
buf[off..off + 2].copy_from_slice(&(-32000i16).to_le_bytes()); off += 2;
|
||||
buf[off..off + 4].copy_from_slice(&(-2_000_000_007i32).to_le_bytes()); off += 4;
|
||||
buf[off..off + 8].copy_from_slice(&(-9_000_000_000_000_000_000i64).to_le_bytes()); off += 8;
|
||||
buf[off] = 0xAB; off += 1; // u8
|
||||
buf[off..off + 2].copy_from_slice(&0xBEEFu16.to_le_bytes()); off += 2;
|
||||
buf[off..off + 4].copy_from_slice(&0xDEADBEEFu32.to_le_bytes()); off += 4;
|
||||
buf[off..off + 8].copy_from_slice(&0x0102030405060708u64.to_le_bytes()); off += 8;
|
||||
buf[off..off + 4].copy_from_slice(&1.5f32.to_le_bytes()); off += 4;
|
||||
buf[off..off + 8].copy_from_slice(&2.5f64.to_le_bytes()); off += 8;
|
||||
buf[off] = 1; off += 1; // bool
|
||||
assert_eq!(off, buf.len());
|
||||
assert!(
|
||||
engine.validate_bytes(&buf).is_ok(),
|
||||
"all-twelve-primitive battery must validate"
|
||||
);
|
||||
|
||||
// Corrupted-value rejection: bool 0x40 is not 0/1.
|
||||
let mut bad = buf.clone();
|
||||
bad[off - 1] = 0x40;
|
||||
let err = engine.validate_bytes(&bad).unwrap_err();
|
||||
assert!(matches!(err, AlkTypeError::Access { .. }), "got {err:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn c1_validate_bytes_packed_decodes_big_endian_subset() {
|
||||
// The BE leg: packed validate_bytes had only ever decoded BE
|
||||
// u32s (the chunk-header fixture) before this test.
|
||||
let doc = json!({
|
||||
"$defs": {
|
||||
"S": {
|
||||
"kind": "struct",
|
||||
"endian": "big",
|
||||
"fields": [
|
||||
{ "name": "i16", "kind": "int16" },
|
||||
{ "name": "u64", "kind": "uint64" },
|
||||
{ "name": "f64", "kind": "float64" }
|
||||
]
|
||||
}
|
||||
}
|
||||
});
|
||||
let engine = AlkTypeEngine::compile(&doc, "S", LayoutMode::Packed, None).expect("compile");
|
||||
let mut buf = vec![0u8; 18];
|
||||
buf[0..2].copy_from_slice(&(-32000i16).to_be_bytes());
|
||||
buf[2..10].copy_from_slice(&0x0102030405060708u64.to_be_bytes());
|
||||
buf[10..18].copy_from_slice(&2.5f64.to_be_bytes());
|
||||
assert!(engine.validate_bytes(&buf).is_ok(), "BE battery must validate");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn validate_bytes_with_simple_struct_round_trips() {
|
||||
let doc = json!({
|
||||
@@ -1362,6 +1441,64 @@ mod tests {
|
||||
assert!(engine.validate_bytes(&corrupt).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn c2_validate_bytes_aligned_inline_string_and_bytes_default_encoding() {
|
||||
// The aligned validate_bytes tests covered maxLength
|
||||
// reservations, offset-indirect, records, and unions — but the
|
||||
// *default* inline length-prefixed encoding (the most common
|
||||
// real shape) had zero public-path executions
|
||||
// (materialize_variable_aligned's LengthPrefixed-else branch,
|
||||
// review #007 C2). ADR-006 allows inline variable fields only
|
||||
// in the last position, so each shape gets its own schema.
|
||||
let string_doc = json!({
|
||||
"$defs": {
|
||||
"S": {
|
||||
"kind": "struct",
|
||||
"endian": "little",
|
||||
"fields": [
|
||||
{ "name": "id", "kind": "uint32" },
|
||||
{ "name": "name", "kind": "string" }
|
||||
]
|
||||
}
|
||||
}
|
||||
});
|
||||
let engine =
|
||||
AlkTypeEngine::compile(&string_doc, "S", LayoutMode::Aligned, None).expect("compile");
|
||||
// Offsets: id @ 0 (4), name prefix @ 4 (4), name data @ 8 (5).
|
||||
let mut buf = vec![0u8; 13];
|
||||
buf[0..4].copy_from_slice(&42u32.to_le_bytes());
|
||||
buf[4..8].copy_from_slice(&5u32.to_le_bytes());
|
||||
buf[8..13].copy_from_slice(b"hello");
|
||||
assert!(
|
||||
engine.validate_bytes(&buf).is_ok(),
|
||||
"aligned inline string must validate through the default encoding"
|
||||
);
|
||||
// Short buffer: name's declared data runs past the buffer end.
|
||||
let err = engine.validate_bytes(&buf[..10]).unwrap_err();
|
||||
assert!(matches!(err, AlkTypeError::Access { .. }), "got {err:?}");
|
||||
|
||||
let bytes_doc = json!({
|
||||
"$defs": {
|
||||
"S": {
|
||||
"kind": "struct",
|
||||
"endian": "little",
|
||||
"fields": [
|
||||
{ "name": "blob", "kind": "bytes" }
|
||||
]
|
||||
}
|
||||
}
|
||||
});
|
||||
let engine =
|
||||
AlkTypeEngine::compile(&bytes_doc, "S", LayoutMode::Aligned, None).expect("compile");
|
||||
let mut bbuf = vec![0u8; 7];
|
||||
bbuf[0..4].copy_from_slice(&3u32.to_le_bytes());
|
||||
bbuf[4..7].copy_from_slice(&[0xAA, 0xBB, 0xCC]);
|
||||
assert!(
|
||||
engine.validate_bytes(&bbuf).is_ok(),
|
||||
"aligned inline bytes must validate through the default encoding"
|
||||
);
|
||||
}
|
||||
|
||||
// ----- ValidationPlan / validate_bytes (ADR-012 §3, phase 7) ----------
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -1197,6 +1197,35 @@ mod tests {
|
||||
assert!(matches!(err, AlkTypeError::Schema(_)), "got {err:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn c3_cycle_through_union_mapping_variant_is_schema_error() {
|
||||
// The compile_variant cycle arm (review #007 C3): a cycle that
|
||||
// closes through a union *mapping entry* — B appears as a
|
||||
// variant of U while B is still being expanded on the current
|
||||
// path. Field-level cycles (compile_typeref's arm) are covered
|
||||
// by cyclic_ref_through_two_defs_is_schema_error; this shape
|
||||
// reaches compile_variant's own cycle check.
|
||||
let root = json!({ "$defs": {
|
||||
"A": { "kind": "struct", "fields": [
|
||||
{ "name": "b", "kind": { "$ref": "#/$defs/B" } }
|
||||
]},
|
||||
"B": { "kind": "struct", "fields": [
|
||||
{ "name": "u", "kind": { "$ref": "#/$defs/U" } }
|
||||
]},
|
||||
"U": {
|
||||
"kind": "union",
|
||||
"discriminator": { "kind": "byte", "offset": 0, "type": "uint8" },
|
||||
"mapping": { "1": { "$ref": "#/$defs/B" } }
|
||||
}
|
||||
}});
|
||||
let err = ReadPlan::compile(&root, "A").unwrap_err();
|
||||
assert!(matches!(err, AlkTypeError::Schema(_)), "got {err:?}");
|
||||
assert!(
|
||||
err.to_string().contains("cyclic $ref"),
|
||||
"expected a cycle error, got {err:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn deep_nesting_beyond_depth_cap_is_schema_error() {
|
||||
let mut defs = serde_json::Map::new();
|
||||
|
||||
@@ -629,4 +629,30 @@ mod tests {
|
||||
let err = discriminator_size(&u).unwrap_err();
|
||||
assert!(matches!(err, AlkTypeError::Schema(_)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn l2_read_field_discriminator_enum_dispatches_on_index() {
|
||||
// Review #007 L2: the enum arm of read_field_discriminator had
|
||||
// zero executions — N1's parity tests covered uint16/uint32 but
|
||||
// skipped enum, the last arm of the documented kind set.
|
||||
let root = field_union_root("kind", "enum");
|
||||
let u = doc_union(&root, "U");
|
||||
let mut buf = vec![0u8; 8];
|
||||
buf[0..4].copy_from_slice(&0u32.to_le_bytes());
|
||||
let d = read_field_discriminator(&buf, &u, 0, LE).expect("read");
|
||||
assert_eq!(d.key, "0");
|
||||
assert_eq!(d.variant_offset, 4);
|
||||
assert_eq!(d.discriminator_size, 4);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn l2_read_field_discriminator_enum_big_endian() {
|
||||
let root = field_union_root("kind", "enum");
|
||||
let u = doc_union(&root, "U");
|
||||
let mut buf = vec![0u8; 8];
|
||||
buf[0..4].copy_from_slice(&1u32.to_be_bytes());
|
||||
let d = read_field_discriminator(&buf, &u, 0, Endian::Big).expect("read");
|
||||
assert_eq!(d.key, "1");
|
||||
assert_eq!(d.variant_offset, 4);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user