diff --git a/docs/reviews/007-coverage-audit.md b/docs/reviews/007-coverage-audit.md new file mode 100644 index 0000000..208955a --- /dev/null +++ b/docs/reviews/007-coverage-audit.md @@ -0,0 +1,400 @@ +--- +status: resolved (F1, F2, C1, C2, C3, L1, L2 resolved 2026-09-02; N1/N2a/N3a/N4a are classified-no-action / deferred-by-design) +last_updated: 2026-09-02 +reviewed_artifacts: + - src/{materialize,sequential_reader,read_plan,offset_map,layout_builder,engine,data_access,bast,builder,tunion,validation_plan,walk_guard,bast_meta}.rs + - tests/{poc_roundtrip,tunion_dispatch,error_paths,engine_integration}.rs + - docs/reviews/006-implementation-review-030.md (post-fix coverage re-check) +tool: cargo-llvm-cov 0.8.4 (--release, per-line text) + manual classification of every uncovered production line + disposable probe tests (run in-session, then deleted) +reviewer: post-review-#006 coverage audit (session request: "check test coverage for weak spots, meaningful tests, non-happy-path posture") +--- + +# Review #007 — Post-#006 Coverage Audit + +## Purpose + +Review #006 closed every finding and its M4 coverage map, but the +session-level posture (M4's item: "fold a coverage check into each fix +session") had never been run as a *whole-tree* pass after all those +fixes landed. This audit re-measures coverage after the eleven #006 +commits, reads every uncovered production line, and classifies it — +the same "untested-but-fine / load-bearing / unreachable" discipline +M4's map used. Two probes were run in disposable tests (deleted after +the session, per #006's no-reproducer rule; neither was a crash +hazard — both reproduce cleanly inside the default harness). + +## Methodology + +- `cargo llvm-cov --release` (0.8.4, same tool as #006): summary + + per-line text. TOTAL **90.67% lines / 86.32% functions** — stable + with #006's post-M4 numbers (90.60%), the expected drift after the + N3 fix sessions added parse gates + tests. +- Per-file (worst first): `materialize.rs` 85.72, `data_access.rs` + 80.32, `sequential_reader.rs` 86.19, `bast.rs` 87.41, + `builder.rs` 91.28, `layout_builder.rs` 91.42, `tunion.rs` 92.02, + `offset_map.rs` 92.93, `read_plan.rs` 90.69, `engine.rs` 96.44, + `validation_plan.rs` 93.97, `walk_guard.rs` 98.04, + `bast_meta.rs` 98.92, `bast_validation.rs`/`error.rs`/`schema.rs`/ + `macros.rs`/`validation.rs` 100. +- Every uncovered line *outside* `#[cfg(test)]` modules (806 raw + lines) was read and classified. Lines inside test modules (the + `panic!("expected X, got {other:?}")` helpers) were excluded — they + distort per-file numbers (e.g. `bast.rs`'s 87.41% is really ~96% + production once its 60 helper lines are excluded). +- Two suspicions were probe-verified with disposable tests: + the F1 cross-consumer divergence and the F2 unbounded-`maxLength` + compile. Probe transcripts quoted verbatim in the findings. +- Happy-path posture audit: cross-checked which *error arms* adjacent + to covered code are 0-execution, and which public surfaces have only + success-path tests. + +## Baseline + +Audited at `main` HEAD `bb28ba3` ("Resolve N3"), 0.3.0, working tree +clean. Full suite green (548 tests static + 2 ignored doctests, per +#006's bookkeeping). + +## Summary Statistics + +| Severity | Count | Status | +|----------|------:|--------| +| High | 2 (F1, F2) | both resolved 2026-09-02 | +| Medium | 3 (C1, C2, C3) | all resolved 2026-09-02 | +| Low | 2 (L1, L2) | all resolved 2026-09-02 | +| Info | 4 (N1, N2a, N3a, N4a) | classified: N1 artifact, N2a/N4a no-action, N3a deferred to pre-release review | + +**Resolution log:** + +- **F1 + F2 (2026-09-02):** resolved in one commit — see the + resolution blocks on each finding. 477 lib tests green (511 + static + 2 ignored doctests across all targets), clippy + `-D warnings` clean, wasm build green. + +--- + +## Findings + +### F1. Zero-progress guard missing in the plan materializer — `validate_bytes` accepts what `SequentialReader` rejects (cross-consumer divergence) + +**Files**: `src/materialize.rs:227-253` (`materialize_plan_array` — no +guard), contrast `src/sequential_reader.rs:772-795` +(`plan_walk_variable_array_size` — has the guard) and +`src/materialize.rs:636-663` (`materialize_array_packed` — has the +guard) + +**Problem**: The H1 fix session added the zero-progress runtime guard +("array element consumed 0 bytes") to two of the three array walkers: +the compiled reader's variable-array size walk and the legacy BAST +walker's packed array arm. The *plan-based* packed materializer — +`materialize_plan_array`, the walker `validate_bytes` actually uses in +packed mode (engine.rs:310-312) — got no guard. + +A stride-0 array whose elements consume 0 bytes (empty-struct elements +are legal: the meta-schema's `StructDef` has no `minItems` on +`fields`) compiles with `element_stride: 0` and loops `count` times +materializing empty objects without reading a single buffer byte: + +``` +PROBE validate_bytes([]): OK — zero-progress guard MISSING in plan materializer +PROBE reader.read_next([]): Err(access error at items[0]: array element 0 consumed 0 bytes; + a zero-size element makes the declared count unbounded on the wire) +``` + +Schema: `{ "items": { "kind": "array", "element": { "kind": +"struct", "fields": [] }, "count": 8 } }`, packed mode, empty buffer. +Same schema, same buffer, opposite verdicts — the exact +cross-consumer-disagreement shape review #006 existed for (H3, M6). +Severity High by #006's own keying (AGENTS.md §3): `validate_bytes` +is the flagship untrusted-input path, and it silently accepts a +buffer the same engine's reader rejects. The H1 resolution text +("plan_walk_variable_array_size (reader) and materialize_array_packed +(materializer) now error") lists only two of the three walkers — the +plan materializer was missed because it is *not* the legacy walker +that finding named. + +**Not a #006 regression**: the H1 fix text itself specified only the +reader and legacy-walker sites; the plan materializer predates the +guard and was outside that fix's blast radius. But the divergence is +new information — the guard's *invariant* ("a zero-progress element +makes the declared count unbounded") belongs to the array-walk +concept, not to two specific functions. + +**Fix**: hoist the same guard into `materialize_plan_array`'s loop +(compare `*offset` before/after `materialize_plan_composite`; error +with the same wording the other two walkers use so downstream +matching sees one shape). Add a locking test driving the same schema +through BOTH paths asserting the verdicts agree (both reject an empty +buffer; both accept a buffer where the elements make progress — +empty-struct elements never do, so the acceptance half needs a +non-empty variant struct alongside). + +**Resolution (2026-09-02):** the guard, hoisted verbatim from the two +existing sites (`*offset == before` after the element walk, same +"array element {i} consumed 0 bytes…" wording so downstream matching +sees one shape). Tests (3, in `materialize.rs`): +`f1_zero_progress_array_rejected_by_all_three_walkers` (plan +materializer + the record-value fallback path, both asserting the +`Access` error with the guard's wording), +`f1_validate_bytes_and_reader_agree_on_zero_progress_array` (the +cross-consumer agreement the probe showed was missing — +`validate_bytes` and `SequentialReader::read_next` both reject the +same schema+buffer with the same error class), +`f1_nonempty_variant_struct_array_still_materializes` (the +false-positive check: elements that consume bytes still walk). +Verified: 477 lib tests green, clippy `-D warnings` clean, wasm build +green. + +### F2. `maxLength` is unbounded — the N2 analog + +**Files**: `src/bast_meta.rs:81` (`"maxLength": { "type": "integer", +"minimum": 0 }` — no maximum), `src/bast.rs:1038-1043` +(`parse_max_length` — no cap, and silently drops non-`usize` values), +contrast `src/schema.rs` `MAX_ALIGN`/`parse_align` (the N2 pattern) + +**Problem**: N2 bounded `align` at 4096 with a clean parse error plus +a meta-schema `"maximum"`. `maxLength` has the identical shape and +was not covered by that fix: + +``` +PROBE aligned maxLength 1e12 compiles; total_size = 1099511627776 +``` + +A one-field schema declares a 1 TiB reservation; `total_size` in that +range is meaningless output the consumer may act on (N2's argument +(a)). Unlike align, no `Access` error follows at read time (an empty +buffer still fails buffer bounds first), so this is layout-meaningless +output, not a crash — the exact severity N2 recorded. Additionally, +`parse_max_length` returns `Option` and silently *drops* values that +overflow `usize` (`.and_then(|n| usize::try_from(n).ok())`) — on a +32-bit target a 5 GiB `maxLength` becomes "no maxLength", changing +layout semantics without telling the consumer. + +**Fix**: the N2 playbook verbatim. A `MAX_LENGTH` cap in +`schema.rs` (value TBD — `align`'s 4096 is page granularity; a +reservation cap in the tens-of-megabytes range fits honest layouts; +suggest `2^26 = 67_108_864`, matching `MAX_ARRAY_BYTES`'s rationale), +enforced in `parse_max_length` (converted to `Result>`, +clean `Schema` error naming the path/value/maximum — no silent drop), +plus `"maximum": 67108864` in the meta-schema's `maxLength` property +so the published contract matches the parser (the N2 dual-layer +pattern). + +**Resolution (2026-09-02):** the N2 playbook, cap = `MAX_LENGTH` +(2^26 = 67_108_864, matching `MAX_ARRAY_BYTES`'s rationale: a single +fixed reservation no larger than the largest legal array): + +1. `MAX_LENGTH` added to `schema.rs`, documented with the F2 probe + arithmetic. +2. `parse_max_length` converted to `Result>`: non-integer + → clean `Schema` error; `usize` overflow → clean `Schema` error (the + silent `.and_then(try_from().ok())` drop is gone); over-cap → clean + `Schema` error naming the path, value, and maximum. +3. Meta-schema `maxLength` property gains `"maximum": 67108864` — the + published contract matches the parser. +4. Tests (5, in `offset_map.rs`, mirroring the `n2_` family): + above-cap rejection naming value+maximum (bytes and string), + at-cap acceptance (`total_size == 67108864`), u64::MAX-scale value + rejected-not-silently-dropped (cap arm on 64-bit, overflow arm on + 32-bit — one test covers whichever fires), and the meta-schema + dual-layer check (above-cap rejected, at-cap accepted). +Verified with F1's commit: 477 lib tests green, clippy clean, wasm +green. + +### C1. Packed `validate_bytes` has never decoded a wide primitive + +**Files**: `src/materialize.rs:121-169` (`materialize_plan_primitive`'s +Int16/Int32/Int64/Uint64/Float64/Boolean arms — all 0-execution), +`src/sequential_reader.rs:345-383` (the reader's same arms are covered +via `read_next` tests, but the materializer's are not) + +**Problem**: every packed `validate_bytes` test feeds u8/uint32/ +string-shaped data. The i16/i32/i64/u64/f64/bool arms of the plan +materializer — the code every untrusted packed wire buffer flows +through — have never executed through any test. Probe (in-session) +confirmed the BE i16/bool path works; the arms are correct, just +unexercised. This is the flagship decode path for `alkcall`'s packed +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. + +### C2. Aligned `validate_bytes` never exercises the default inline encoding for string/bytes + +**Files**: `src/materialize.rs:1037-1039` +(`materialize_variable_aligned`'s `LengthPrefixed`-else branch — +0-exec through the public path) + +**Problem**: the aligned `validate_bytes` tests use records, unions, +maxLength reservations, and offset-indirect encodings. The *default* +encoding — an inline length-prefixed string or bytes field, the most +common real shape — reaches `read_field` (engine_integration.rs:192) +but never `validate_bytes`. The aligned `validate_bytes` surface has +thus never decoded the single most likely field kind through its +public path. + +**Fix**: one aligned `validate_bytes` test with a trailing inline +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. + +### C3. `ReadPlan::compile`'s union-variant cycle arm is untested standalone + +**Files**: `src/read_plan.rs:509-514` (`compile_variant`'s +`cycle_err` arm — 0-exec) + +**Problem**: the H2 test family exercises `check_ref_graph` (walk +guard) via `OffsetMap::compute`/`LayoutBuilder::new`/ +`materialize_aligned`, and `ValidationPlan::compile`'s cycle arm is +covered (`validation_plan.rs:297` shows executions, via the +`shared_refs_compile_without_false_cycle`/cycle tests). But +`ReadPlan::compile`'s own cycle rejection — the defense the *packed +read plan* relies on when driven standalone (its doc explicitly +promises untrusted-input safety) — has no test driving a two-def +cycle through it. The depth cap is tested +(`deep_nesting_beyond_depth_cap_is_schema_error`); the cycle arm is +shadowed in every engine-path test by the ValidationPlan gate running +first (engine.rs:153). + +**Fix**: a `read_plan_compile_two_def_cycle_rejected` test calling +`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. + +### L1. `builder.rs`'s JSON-Schema conveniences are entirely untested + +**Files**: `src/builder.rs:268-290` (`array()`, `number()`, +`boolean_()`, `null()`), `:465-479` (`items()`, +`additional_properties()`), `:508-565` (`maximum()`, `min_length()`, +`min_items()`, `max_items()`, `format()`, `title()`, +`description()`), `:440` (the `field()`-on-standard-repr path) + +**Problem**: only the BAST-side builders have tests. The standard +JSON-Schema side feeds `jsonschema::build_validator` (the +`json_schema` parameter of `AlkTypeEngine::compile`), so a typo'd or +misplaced keyword would ship silently — the builder emits the JSON, +`jsonschema` interprets it, and nothing checks the translation. One +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. + +### L2. `tunion::read_field_discriminator`'s enum arm is 0-exec + +**Files**: `src/tunion.rs:166-169` + +**Problem**: N1's resolution extended tunion to match the reader's +kind set and added uint16/uint32 tests both endians — but skipped the +enum arm, which is in the documented kind set +(tunion.rs:106-112 names "string / uint8 / uint16 / uint32 / enum"). +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. + +### N1. `OffsetEntry::start()`/`end()` 0-execution in the combined run is a merge artifact, not a hole + +**Files**: `src/offset_map.rs:79-86` + +The combined `cargo llvm-cov --release` run reports these 0-exec; +`tests/poc_roundtrip.rs:187-189` calls `start()` (and the +`big_endian_round_trip_via_offset_map` test calls `end()`). Per-test +coverage confirms both execute (32/2 calls respectively in a +poc_roundtrip-only run). llvm-cov's profile merge does not attribute +integration-test-binary executions to the library in every run +configuration. Recorded so nobody "fixes" this by deleting the +accessors or writing a redundant in-module test. (Caveat for future +audits: when a combined run shows 0-exec on something an integration +test visibly calls, re-run per-test-target before classifying.) + +### N2a. `data_access.rs`'s remaining uncovered lines are the documented >4 GiB guards — fine to leave + +**Files**: `src/data_access.rs:54-95, 223-291, 336-414` + +All are `checked_add` overflow arms and u32-truncation guards needing +multi-GiB slices or near-`usize::MAX` offsets — already documented as +defensively-unreachable on 64-bit test hardware in #006 M4 item 3's +resolution. (The `read_array`/`write_array` arms at :54-95 are +additionally unreachable-after-`check_bounds` belt-and-suspenders.) +No action. + +### N3a. `bast.rs` dead-or-orphaned surface — flag for the pre-release review + +**Files**: `src/bast.rs:212-214, 305-307, 404-406, 506-508, 741-743, +924-926, 973-975` (`source()` accessors — zero callers anywhere in +src or tests), `:353-363` (`BastField::synthetic`, +`#[allow(dead_code)]`, zero callers), `:149-167` +(`resolve_typeref_as_def`'s inline struct/union/enum arms — both call +sites pass `$ref`-only variants since H3's parse rules forbid +re-declaration; plausibly dead now) + +Three small deletions-or-justifications. Not fixed this session (the +`source()` accessors are public API — removal is a semver decision +for the pre-release review, and AGENTS.md's semver exception list +says renames/removals need an explicit ask). Recorded so the +pre-release review session has the list. + +### N4a. Internal-shape error arms are structurally unreachable — fine to leave + +**Files**: `src/materialize.rs:241,309,422,858`, +`src/sequential_reader.rs:295,473,533,832`, `src/offset_map.rs:352`, +`src/layout_builder.rs:194,282`, `src/read_plan.rs:402` + +The `"internal: …"` arms that dispatch on a `match` the caller +already narrowed (e.g. "union body at X is not CompositePlan::Union" +inside a function only reachable from a `Union` match arm). They are +honest defensive code — deleting them would force `unwrap()` — and +forcing them in tests would require constructing mid-walk corruption. +Leave uncovered; the pattern is consistent across the codebase. + +--- + +## What's Good + +- The #006 fix sessions left the tree in genuinely good shape: 90.67% + lines with every high-traffic wire path (reader dispatch, plan + compiler, offset map, walk guard) in the mid-90s or better. +- The untrusted-input discipline is visible in the coverage: every + parse-level gate added in #006 (H1 caps, N2 align cap, N3 + string/bytes-only maxLength, H2 cycle rejections at all three + standalone walkers) has both rejection and boundary tests. +- The `#[cfg(test)]` helper noise is the only thing making + `bast.rs`/`data_access.rs` look worse than they are — the + production coverage of both is materially higher than the raw + per-file number. + +## 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. +5. **N3a** — defer to the pre-release review (semver decision). + +## Notes + +- Probe tests were run as `tests/zzz_probe.rs` in-tree during the + session and deleted before any commit (the #006 pattern). Neither + probe was a crash hazard; both reproduce safely in the default + harness. +- Per-file numbers are from a single `cargo llvm-cov --release` + run; the N1 merge artifact means integration-test-only calls + (e.g. `OffsetEntry::start()`) can show 0-exec in the combined + report — the classification above already accounts for that. +- 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. \ No newline at end of file diff --git a/src/bast.rs b/src/bast.rs index 9360084..37dfa88 100644 --- a/src/bast.rs +++ b/src/bast.rs @@ -36,7 +36,7 @@ use crate::error::AlkTypeError; use crate::schema::{ - AlkTypeKind, Endian, VariableEncoding, MAX_ALIGN, MAX_ARRAY_ELEMENTS, + AlkTypeKind, Endian, VariableEncoding, MAX_ALIGN, MAX_ARRAY_ELEMENTS, MAX_LENGTH, }; use serde_json::Value; @@ -428,7 +428,7 @@ impl BastField { )) })?; let ty = BastType::parse(raw_kind, path, doc_root)?; - let max_length = parse_max_length(node); + let max_length = parse_max_length(node, path)?; if max_length.is_some() && !matches!( &ty, BastType::Primitive(AlkTypeKind::String) | BastType::Primitive(AlkTypeKind::Bytes) @@ -1035,11 +1035,36 @@ fn parse_align(node: &Value, path: &str) -> Result, AlkTypeError> Ok(Some(n)) } -fn parse_max_length(node: &Value) -> Option { - node.as_object() - .and_then(|o| o.get("maxLength")) - .and_then(Value::as_u64) - .and_then(|n| usize::try_from(n).ok()) +/// Parse a field's `maxLength` annotation. Over-sized values are a +/// clean `Schema` error (review #007 F2 — the N2 pattern: a +/// reservation contributes its full `maxLength` to `total_size`, so an +/// unbounded value lets a one-field schema declare terabyte-scale +/// layouts). Values that overflow `usize` are rejected, not silently +/// dropped — a silent drop would change layout semantics without +/// telling the consumer. +fn parse_max_length(node: &Value, path: &str) -> Result, AlkTypeError> { + let raw = match node.as_object().and_then(|o| o.get("maxLength")) { + None | Some(Value::Null) => return Ok(None), + Some(v) => v, + }; + let n = raw.as_u64().ok_or_else(|| { + AlkTypeError::Schema(format!( + "bast: maxLength at {path} is not a non-negative integer" + )) + })?; + let n = usize::try_from(n).map_err(|_| { + AlkTypeError::Schema(format!( + "bast: maxLength at {path} (= {n}) overflows usize" + )) + })?; + if n > MAX_LENGTH { + return Err(AlkTypeError::Schema(format!( + "bast: maxLength at {path} (= {n}) exceeds the maximum of {MAX_LENGTH} \ + (review #007 F2: a reservation contributes its full value to total_size; \ + honest layouts never need more than the largest legal array)" + ))); + } + Ok(Some(n)) } fn parse_encoding(node: &Value) -> VariableEncoding { diff --git a/src/bast_meta.rs b/src/bast_meta.rs index 9b8ad3d..4cddd29 100644 --- a/src/bast_meta.rs +++ b/src/bast_meta.rs @@ -78,7 +78,7 @@ pub static BAST_META_SCHEMA: LazyLock = LazyLock::new(|| { "endian": { "enum": ["little", "big"] }, "align": { "type": "integer", "minimum": 1, "maximum": 4096 }, "encoding": { "enum": ["length-prefixed", "offset-indirect"] }, - "maxLength": { "type": "integer", "minimum": 0 } + "maxLength": { "type": "integer", "minimum": 0, "maximum": 67108864 } }, "if": { "properties": { diff --git a/src/materialize.rs b/src/materialize.rs index b757c0f..a6aca7c 100644 --- a/src/materialize.rs +++ b/src/materialize.rs @@ -246,7 +246,17 @@ fn materialize_plan_array( let mut arr = Vec::new(); for i in 0..count { let path = format!("{field_path}[{i}]"); + let before = *offset; let value = materialize_plan_composite(element, &path, endian, buffer, offset)?; + if *offset == before { + return Err(AlkTypeError::Access { + field_path: path, + reason: format!( + "array element {i} consumed 0 bytes; a zero-size element makes the \ + declared count unbounded on the wire" + ), + }); + } arr.push(value); } Ok(Value::Array(arr)) @@ -2095,4 +2105,105 @@ mod tests { let err = materialize_packed_strict(&root, &doc, &buf).unwrap_err(); assert!(matches!(err, AlkTypeError::Access { .. }), "got {err:?}"); } + + // ----- F1 (review #007): zero-progress guard parity across walkers -- + + #[test] + fn f1_zero_progress_array_rejected_by_all_three_walkers() { + // Plan materializer (validate_bytes' packed path): an empty + // struct element consumes 0 bytes, so the walk makes no + // progress and the declared count is unbounded on the wire. + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "endian": "little", + "fields": [ + { "name": "items", "kind": { "kind": "array", "element": { "kind": "struct", "fields": [] }, "count": 8 } } + ] + } + } + }); + let doc = doc_from(&root, "S"); + let err = materialize_packed_strict(&root, &doc, &[]).unwrap_err(); + assert!(matches!(err, AlkTypeError::Access { .. }), "got {err:?}"); + assert!(err.to_string().contains("consumed 0 bytes"), "got {err:?}"); + + // Legacy BAST walker (aligned-record value fallback): the same + // zero-progress shape as a record's array-valued entries — the + // record walk reads the count prefix + key, then the array + // value walks with the same no-progress rejection. + let rec_root = json!({ + "$defs": { + "S": { + "kind": "struct", + "endian": "little", + "fields": [ + { "name": "counts", "kind": { "kind": "record", "values": { "kind": "array", "element": { "kind": "struct", "fields": [] }, "count": 8 } } } + ] + } + } + }); + let rec_doc = doc_from(&rec_root, "S"); + let mut buf = vec![0u8; 12]; + buf[0..4].copy_from_slice(&1u32.to_le_bytes()); + buf[4..8].copy_from_slice(&1u32.to_le_bytes()); + buf[8] = b'k'; + let err = materialize_packed_strict(&rec_root, &rec_doc, &buf).unwrap_err(); + assert!(matches!(err, AlkTypeError::Access { .. }), "got {err:?}"); + assert!(err.to_string().contains("consumed 0 bytes"), "got {err:?}"); + } + + #[test] + fn f1_validate_bytes_and_reader_agree_on_zero_progress_array() { + // The cross-consumer agreement the F1 probe showed was missing: + // both public consumers must reject the same schema+buffer with + // the same error class. + use crate::engine::AlkTypeEngine; + use crate::engine::LayoutMode; + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "endian": "little", + "fields": [ + { "name": "items", "kind": { "kind": "array", "element": { "kind": "struct", "fields": [] }, "count": 8 } } + ] + } + } + }); + let engine = AlkTypeEngine::compile(&root, "S", LayoutMode::Packed, None) + .expect("empty-struct element array compiles"); + + // validate_bytes (plan materializer): clean Access error. + let err = engine.validate_bytes(&[]).unwrap_err(); + assert!(matches!(err, AlkTypeError::Access { .. }), "got {err:?}"); + assert!(err.to_string().contains("consumed 0 bytes"), "got {err:?}"); + + // SequentialReader: same verdict on the same buffer. + let mut reader = engine.sequential_reader().expect("packed reader"); + let err = reader.read_next(&[]).unwrap_err(); + assert!(matches!(err, AlkTypeError::Access { .. }), "got {err:?}"); + assert!(err.to_string().contains("consumed 0 bytes"), "got {err:?}"); + } + + #[test] + fn f1_nonempty_variant_struct_array_still_materializes() { + // Guard must not false-positive: elements that consume bytes + // still walk. + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "endian": "little", + "fields": [ + { "name": "items", "kind": { "kind": "array", "element": { "kind": "struct", "fields": [ { "name": "v", "kind": "uint8" } ] }, "count": 3 } } + ] + } + } + }); + let doc = doc_from(&root, "S"); + let value = materialize_packed_strict(&root, &doc, &[1, 2, 3]).expect("materialize"); + assert_eq!(value, json!({ "items": [ { "v": 1 }, { "v": 2 }, { "v": 3 } ] })); + } } \ No newline at end of file diff --git a/src/offset_map.rs b/src/offset_map.rs index 8ef0b1c..08ed153 100644 --- a/src/offset_map.rs +++ b/src/offset_map.rs @@ -811,6 +811,122 @@ mod tests { } } + // ----- F2 (review #007): unbounded maxLength — the N2 pattern ------- + + #[test] + fn f2_max_length_above_cap_rejected_at_parse() { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "blob", "kind": "bytes", "maxLength": 1099511627776u64 } + ] + } + } + }); + let err = BastDoc::new(&root, "S").unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("maxLength"), "reason: {reason}"); + assert!(reason.contains("maximum"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + + #[test] + fn f2_max_length_string_above_cap_rejected_at_parse() { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "name", "kind": "string", "maxLength": 67108865u64 } + ] + } + } + }); + let err = BastDoc::new(&root, "S").unwrap_err(); + assert!(matches!(err, AlkTypeError::Schema(_)), "got {err:?}"); + } + + #[test] + fn f2_max_length_at_cap_accepted() { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "endian": "little", + "fields": [ + { "name": "blob", "kind": "bytes", "maxLength": 67108864u64 } + ] + } + } + }); + let m = map(&root, "S"); + assert_eq!(m.total_size(), 67108864); + } + + #[test] + fn f2_max_length_u64_max_rejected_not_silently_dropped() { + // The old parse silently returned None for values it could not + // handle; a u64::MAX-scale maxLength must be a clean Schema + // error (the cap arm on 64-bit, the usize-overflow arm on + // 32-bit), never a silent layout change. + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "blob", "kind": "bytes", "maxLength": 18446744073709551615u64 } + ] + } + } + }); + let err = BastDoc::new(&root, "S").unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("maxLength"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + + #[test] + fn f2_meta_schema_rejects_max_length_above_cap() { + // The published contract matches the parser (N2 dual-layer + // pattern). + let doc = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "blob", "kind": "bytes", "maxLength": 67108865u64 } + ] + } + } + }); + assert!( + crate::bast_meta::validate_bast_doc(&doc).is_err(), + "meta-schema must reject maxLength above the cap" + ); + let ok = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "blob", "kind": "bytes", "maxLength": 67108864u64 } + ] + } + } + }); + assert!( + crate::bast_meta::validate_bast_doc(&ok).is_ok(), + "meta-schema must accept maxLength at the cap" + ); + } + #[test] fn h1_array_bytes_above_cap_rejected_at_compute() { // 65536 elements × stride 4096 (a u8 array in a struct with the diff --git a/src/schema.rs b/src/schema.rs index 52d58d2..57e29b6 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -38,6 +38,15 @@ pub(crate) const MAX_ARRAY_BYTES: usize = 1 << 26; /// need more than page granularity (4096). pub(crate) const MAX_ALIGN: usize = 4096; +/// Maximum declared `maxLength` annotation. Schemas are untrusted input +/// (AGENTS.md §3): a string/bytes reservation contributes its full +/// `maxLength` to `total_size`, so an unbounded value lets a one-field +/// schema declare a terabyte-scale layout (`total_size = 2^40` from +/// `maxLength: 1e12` — review #007 F2 probe). The cap matches +/// [`MAX_ARRAY_BYTES`]'s rationale (a single fixed reservation no +/// larger than the largest legal array). +pub(crate) const MAX_LENGTH: usize = 1 << 26; + /// The 18 BAST kinds recognized by the engine. /// /// Each variant corresponds to a lowercase BAST kind string