diff --git a/docs/architecture/README.md b/docs/architecture/README.md index e15e251..d5bbe7f 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -46,7 +46,7 @@ format definition; the engine is generic. | [009](decisions/009-builder-api.md) | Builder API for Schema Construction | Fluent Rust API producing `serde_json::Value`; covers BAST kinds + standard JSON Schema; resolves OQ-003. *Output format amended to BAST / standard JSON Schema by ADR-BAST.* | | [010](decisions/010-generalized-validation-validate-bytes.md) | Generalized Validation — `validate_bytes` on `AlkTypeEngine` | Single-call binary-buffer validation; materialize `Value` from bytes, then validate. *Validation step amended to the BAST-native validator by ADR-VAL-SPLIT.* | | [011](decisions/011-compiled-read-plan-for-packed-mode.md) | Compiled Read Plan for Packed Mode | `ReadPlan` — the packed read-side compiled form, symmetric to `OffsetMap` (aligned) and `PackedLayout` (packed write). Closes review #004's 400x read-path gap; retires ADR-007's "re-parse on demand" framing. *Accepted.* | -| [012](decisions/012-plan-fingerprinting-and-m1-closure.md) | Plan Fingerprinting and Closing the Deferred M1 Sites in 0.3.0 | `ReadPlan`/`OffsetMap` `Hash + Eq` + `fingerprint()`; owned `BastDoc` (lifetime removal); `OffsetMap` carries `LeafMeta` to close the aligned-side M1 sites. Bundles with ADR-011 into one 0.3.0 breaking release. *Proposed.* | +| [012](decisions/012-plan-fingerprinting-and-m1-closure.md) | Plan Fingerprinting, ValidationPlan, and Closing the Deferred M1 Sites in 0.3.0 | `ReadPlan`/`OffsetMap`/`ValidationPlan` `Hash + Eq` + `fingerprint()`; owned `BastDoc` (lifetime removal); `OffsetMap` carries `LeafMeta` to close the aligned-side M1 sites; `ValidationPlan` retires the interpretive `bast_validation` walk (review #005 M3 reversed the original deferral). Bundles with ADR-011 into one 0.3.0 breaking release. *Proposed.* | ## Relevant Open Questions diff --git a/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md b/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md index 64e74c1..c2f1a8b 100644 --- a/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md +++ b/docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md @@ -8,6 +8,21 @@ POC on branch `readplan-poc` confirmed the `ReadPlan` shape covers every `BastType` arm in the current read loop before implementation began (see "POC coverage" at the end). +**Refinements on ADR-012 acceptance (2026-08-20, review #005):** the +`CompositePlan::Union` shape was refined to carry `shared: +Option>` (field-disc union shared fields, resolving POC +Finding 1 / review #005 H1) and to drop `VariantPlan`/`VariantKind` +in favor of `variants: Vec<(String, CompositePlan)>` (resolving review +#005 M1 — nested unions now work by `CompositePlan` recursion, +restoring the 0.2.0 capability the POC rejected). The "BastDoc +unchanged" scope statement stands as the ADR-011-only view; ADR-012 +§2a subsequently makes `BastDoc` owned. The `ValidationPlan` this +ADR's "Out of scope" originally deferred indefinitely is now in +0.3.0 via ADR-012 §3 (review #005 M3 reversed the deferral). These +are pre-implementation refinements to types that do not yet exist on +`main`; the ADR-011 decision (a compiled `ReadPlan` for packed reads) +is unchanged. + ## Context Review #004 (`docs/reviews/004-performance-review.md`) measured the @@ -145,7 +160,8 @@ pub enum CompositePlan { Struct(ReadPlan), Union { disc: DiscriminatorPlan, - variants: Vec<(String, VariantPlan)>, + shared: Option>, + variants: Vec<(String, CompositePlan)>, }, Array { element: Box, @@ -165,12 +181,30 @@ pub enum DiscriminatorPlan { Nested structs share the `ReadPlan` shape (a struct field's `body` is `CompositePlan::Struct(ReadPlan)`). Union variants are pre-resolved: -each `(key, VariantPlan)` entry carries the variant's `ReadPlan`, so -dispatch is a flat lookup + recurse — no `resolve_typeref_as_def` at -read time. Array element strides are precomputed (`element_stride = 0` +each `(key, CompositePlan)` entry carries the variant's compiled body, +so dispatch is a flat lookup + recurse — no `resolve_typeref_as_def` at +read time. A variant may itself be `CompositePlan::Union { ... }`, so +**nested unions** (a union variant that is itself a union, which the +0.2.0 reader supports via `resolve_and_walk_variant`'s `Union` arm) are +covered by ordinary recursion; no separate `VariantKind` enum is +needed. Array element strides are precomputed (`element_stride = 0` signals variable-length elements, same convention as today's `FieldValue::Array`). +`Union.shared` carries the union's declared `fields` (the discriminator +field + any shared fields) for the field-name-discriminator case — +`DiscriminatorPlan::Field.field_index` indexes into `shared`, and the +read loop walks `shared` first, then looks up and walks the selected +variant's `CompositePlan` starting after the shared fields. The +byte-offset-discriminator case has no shared fields (`shared: None`): +the discriminator byte is read at `disc.offset` and the variant starts +immediately after the discriminator size. The POC's `plan_read_union` +`Field` arm stub (Finding 1) is replaced by this `shared` sub-plan; +there is no separate `VariantPlan`/`VariantKind` type in the production +shape — the POC's `VariantPlan { kind, plan }` wrapper is dropped in +favor of recursing on `CompositePlan` directly, which is what makes +nested-union support fall out for free. + ### Construction ```rust @@ -215,10 +249,16 @@ plan being immutable owned data, but the implementation should add a - `materialize_aligned` is unchanged (already takes `&OffsetMap`, a compiled form). - `ReadPlan` is a new public type, re-exported from `lib.rs`. -- `BastDoc` and the `Bast*` types are **unchanged** — they remain the - validation-side typed tree, borrowed, as today. This is a smaller - breakage than review #004's Option A (which changed `BastDoc<'a>` → - `BastDoc` and every `Bast*` signature). +- `BastDoc` and the `Bast*` types are **unchanged by this ADR** — they + remain the validation-side typed tree, borrowed, as today. This is a + smaller breakage than review #004's Option A (which changed + `BastDoc<'a>` → `BastDoc` and every `Bast*` signature). + **Note (added on ADR-012 acceptance):** ADR-012 §2a subsequently + makes `BastDoc` owned, riding the same 0.3.0 bump. That is an + ADR-012 change, not an ADR-011 change; ADR-011's scope statement + stands as the ADR-011-only view. With ADR-012 §3 (ValidationPlan, + now in 0.3.0), `bast_validation` will also stop being the permanent + home of the `BastDoc` walk — see ADR-012. The crate is pre-1.0 with two in-house downstream consumers (`alktty`, `alkcall`), both of which will be updated with the bump. @@ -248,12 +288,15 @@ The crate is pre-1.0 with two in-house downstream consumers - **`bast_validation`** — the BAST-native value-domain validator walks `BastDoc` to check constraints (`maxLength`, enum string values, union variant keys, integer ranges). These are value-domain checks, - not byte-position walks; they don't benefit from a read plan and + not byte-position walks; they don't benefit from a *read* plan and would require a separate "validation plan" with a different shape. - Validation is not a per-chunk hot loop (AGENTS.md: validation is - opt-in per operation). A future `ValidationPlan` is a two-way door - if a bench motivates it; for now `bast_validation` keeps walking - `BastDoc`, which remains the validation-side typed tree. + **Not in scope for ADR-011** — but no longer deferred indefinitely: + ADR-012 §3 brings a `ValidationPlan` into 0.3.0. The "not a hot + loop" framing this paragraph originally relied on was re-evaluated + and rejected (see ADR-012 §3): read+validate on untrusted streams + makes validation hot in the same sense review #004 measured for + the read path. For 0.3.0 as accepted by ADR-011 alone, + `bast_validation` keeps walking `BastDoc`; ADR-012 §3 closes that. - **`LayoutBuilder` / `PackedLayout`** — the packed write-side already has a compiled form (`PackedLayout`). `LayoutBuilder::build` (`src/layout_builder.rs:190`) re-parses `BastDoc::new` per `build()` @@ -363,19 +406,25 @@ The crate is pre-1.0 with two in-house downstream consumers ## Scope Boundaries (What This Is Not) - **Not a `BastDoc` replacement.** `BastDoc` stays as the - validation-side typed tree, borrowed from `&Value`, unchanged. The - `Bast*` types and their signatures are not touched. Validation - (`bast_validation`), aligned one-shot reads/writes - (`engine.rs:334,467`), and `LayoutBuilder::build` continue to walk - `BastDoc` until they get their own compiled forms (additive, later). + validation-side typed tree within ADR-011's scope (borrowed from + `&Value`, unchanged). The `Bast*` types and their signatures are not + touched by ADR-011. Validation (`bast_validation`), aligned one-shot + reads/writes (`engine.rs:334,467`), and `LayoutBuilder::build` + continue to walk `BastDoc` within ADR-011's scope. **ADR-012 + subsequently revises two of these:** `BastDoc` becomes owned (§2a) + and `bast_validation` adopts a `ValidationPlan` (§3), both riding + the same 0.3.0 bump. ADR-011's scope statement is the ADR-011-only + view and is not re-litigated here. - **Not a flat lookup table.** Packed positions are data-dependent; the plan is a read program (instructions to walk), not a `(path, offset)` table. This is inherent to packed sequential layout (ADR-002), not a limitation of this design. - **Not a validation plan.** `bast_validation`'s value-domain checks (maxLength, enum values, union variant keys, integer ranges) are a - different concern and a different shape. They stay on `BastDoc`. A - future `ValidationPlan` is a two-way door. + different concern and a different shape from `ReadPlan`. They are + out of ADR-011's scope; ADR-012 §3 adds a `ValidationPlan` in 0.3.0 + rather than leaving validation on an interpretive `BastDoc` walk + indefinitely. - **Not the review's Option A or Option B.** It is the "compiled form" path the review pointed at but did not name: Option A (make `BastDoc` own its data) kills the re-parse but leaves the read loop as a @@ -448,13 +497,13 @@ closes L2. L1 falls out at step 2. The aligned-side M1 paths `validate_bytes` (packed) consumes the `ReadPlan` via `materialize_packed` -## Future capabilities (partly in 0.3.0 via ADR-012) +## Future capabilities (in 0.3.0 via ADR-012) The deterministic-compile property of `ReadPlan` is a prerequisite for -several capabilities. ADR-012 ("Plan Fingerprinting and Closing the -Deferred M1 Sites in 0.3.0") picks up the first two items below into -the 0.3.0 release so they ship with this ADR's breaking changes in one -round of downstream churn, not two: +several capabilities. ADR-012 ("Plan Fingerprinting, ValidationPlan, +and Closing the Deferred M1 Sites in 0.3.0") picks up all three items +below into the 0.3.0 release so they ship with this ADR's breaking +changes in one round of downstream churn, not two or three: - **Fingerprinting the plan** (`#[derive(Hash)]` + a `fingerprint()` method) for cross-run caching of compiled plans, disk-cached plans, @@ -466,9 +515,12 @@ round of downstream churn, not two: candidate listed here as "not a new type — extend the existing compiled forms (`PackedLayout`/`OffsetMap`) and cache the parse." - A `ValidationPlan` that follows the same compile-once-walk-many - pattern for `bast_validation`. → **Deferred (ADR-012).** Different - shape (value-domain, not byte-position); not a hot loop; separate - ADR if a bench motivates it. + pattern for `bast_validation`. → **In 0.3.0 (ADR-012 §3).** Different + shape (value-domain, not byte-position) but the same class of + per-buffer re-walk cost on the read+validate-on-untrusted-input + common case. ADR-012 owns the shape decision and the implementation + plan scopes it. (Originally deferred by ADR-012 as "not a hot loop"; + review #005 M3 reversed the deferral — see ADR-012 §3.) None of the in-0.3.0 items justify this ADR; the 400x read-path gap does. They are listed here as forward references and to record that @@ -487,7 +539,15 @@ two spots where a plan arm could subtly miss a case: `DiscriminatorPlan::Byte { offset, disc_type }` and `DiscriminatorPlan::Field { name, field_index }`. The field-name case pre-resolves the discriminator field's `ReadKind` so dispatch reads - it from the plan, not from a re-parsed `BastField`. + it from the plan, not from a re-parsed `BastField`. The + field-disc union's declared `fields` (discriminator + any shared + fields) are carried as a sub-`ReadPlan` on `CompositePlan::Union`'s + `shared` field (a refinement of the POC shape, which stubbed the + `Field` arm — Finding 1); the production read loop walks `shared` + first, then the selected variant's `CompositePlan`. Nested-union + variants (a variant that is itself a union) are covered by ordinary + `CompositePlan` recursion; the POC rejected them, the 0.2.0 reader + accepts them, and the production shape restores parity. - **Array variable-element-stride (`element_stride = 0`).** Covered: `CompositePlan::Array { element, count, element_stride }` preserves the `0`-signals-variable convention, and the read loop walks diff --git a/docs/architecture/decisions/012-plan-fingerprinting-and-m1-closure.md b/docs/architecture/decisions/012-plan-fingerprinting-and-m1-closure.md index 548942f..2031a06 100644 --- a/docs/architecture/decisions/012-plan-fingerprinting-and-m1-closure.md +++ b/docs/architecture/decisions/012-plan-fingerprinting-and-m1-closure.md @@ -1,11 +1,16 @@ -# ADR-012: Plan Fingerprinting and Closing the Deferred M1 Sites in 0.3.0 +# ADR-012: Plan Fingerprinting, ValidationPlan, and Closing the Deferred M1 Sites in 0.3.0 ## Status -Proposed. Bundles two pieces of work into the 0.3.0 release so the -crate ships one round of breaking changes, not two. Companion to -[ADR-011](011-compiled-read-plan-for-packed-mode.md) (the `ReadPlan`) -and the [0.3.0 implementation plan](../../plans/030-compiled-forms.md). +Proposed. Bundles three pieces of work into the 0.3.0 release so the +crate ships one round of breaking changes, not two (or three). The +three pieces: (a) fingerprinting `ReadPlan`/`OffsetMap`, (b) closing +the deferred M1 sites via an owned `BastDoc` + `OffsetMap` `LeafMeta`, +and (c) a `ValidationPlan` that retires the interpretive +`bast_validation` walk (added by reversing the original "defer +`ValidationPlan`" decision — see "ValidationPlan — in scope for +0.3.0" below). Companion to [ADR-011](011-compiled-read-plan-for-packed-mode.md) +(the `ReadPlan`) and the [0.3.0 implementation plan](../../plans/030-compiled-forms.md). ## Context @@ -15,21 +20,26 @@ and deferred two things to "future capabilities": 1. **Fingerprinting the plan** for cross-run caching, disk-cached compiled plans, and `alkcall` hub/spoke schema-version handshakes. 2. **A `WritePlan` and/or `ValidationPlan`** following the same - compile-once-walk-many pattern for the deferred M1 sites. + compile-once-walk-many pattern for the deferred M1 sites and the + validation walk. ADR-011 also explicitly deferred the aligned-side M1 sites (`engine.rs:334,467` `read_field`/`write_field`; `layout_builder.rs:190` `LayoutBuilder::build`) as "a deliberate reversible bet that an aligned-mode hot loop won't emerge." -This ADR retires both deferrals in one release. The reasoning is +This ADR retires the deferrals in one release. The reasoning is timing: 0.3.0 is already a breaking bump (ADR-011 changes `SequentialReader::new` and `materialize_packed` signatures), and the crate has no real downstream consumers yet (only `alktty`/`alkcall`, -both in-house). Doing both pieces now costs one round of downstream -churn instead of two, and the fingerprinting work cuts across both -`ReadPlan` and `OffsetMap` — splitting would create a cross-release -dependency that's cleaner in one release. +both in-house). Doing all three pieces now costs one round of +downstream churn instead of two or three, and the fingerprinting work +cuts across both `ReadPlan` and `OffsetMap` — splitting would create +a cross-release dependency that's cleaner in one release. The +`ValidationPlan` inclusion follows the same logic applied to the +validation walk: deferring it would create a *second* breaking change +to `validate_bytes`/`bast_validation` after 0.3.0, which is exactly +the round of downstream churn this release exists to retire. ### Reframing "WritePlan" @@ -61,16 +71,49 @@ fixes are "cache the parse" and "extend the compiled form with leaf metadata," not "add a third compiled form." This serves the minimal-public-API-changes goal better than a literal `WritePlan`. -### Deferring `ValidationPlan` +### `ValidationPlan` — in scope for 0.3.0 (no longer deferred) The BAST-native validator (`bast_validation`) walks `BastDoc` to check value-domain constraints (enum value sets, integer ranges, -`maxLength` caps, union variant keys). This is a different shape from -`ReadPlan`/`OffsetMap` (value-domain, not byte-position) and is not a -hot loop — validation is opt-in per operation per AGENTS.md. A future -`ValidationPlan` is a two-way door with its own ADR if a bench -motivates it; for 0.3.0 it is scope creep that would delay the -read-path fix. Explicitly deferred. +`maxLength` caps, union variant keys). This is a different shape +from `ReadPlan`/`OffsetMap` (value-domain, not byte-position), and +the earlier framing deferred it as "not a hot loop — validation is +opt-in per operation per AGENTS.md." + +**That deferral is reversed.** The "not a hot loop" dismissal +under-counted the common case: **read + validate together on +untrusted input.** The downstream `alkcall` consumer accepts schemas +from arbitrary internet peers in a hub/spoke topology (AGENTS.md §3); +the common operation on an incoming frame is "read it, then validate +it before acting." `validate_bytes` (ADR-010) is therefore called +once per incoming buffer, and each call re-walks `BastDoc` for +validation even after ADR-011 makes the *read* half plan-fast. That +is the same class of per-buffer interpretive cost review #004 measured +for the read path (400x per chunk), on a different code path, on the +operation the untrusted-input discipline actually requires. + +The cost-of-inaction framing that the original deferral relied on was +also wrong: a `ValidationPlan` introduced *after* 0.3.0 would be a +breaking change to `validate_bytes`'s contract and to the +`bast_validation` public surface, forcing rework of `alktty`/`alkcall` +— the exact downstream-churn this release is supposed to retire, not +create a second round of. Shipping it in 0.3.0 pays the cost once, +alongside the other breaking changes, while there are zero real +consumers. The cost of action now is a static, known quantity; the +cost of action later is the same work plus a second round of +downstream churn plus the risk of the interpretive path being the +one that gets used in the meantime on untrusted bytes. + +**Decision: a `ValidationPlan` ships in 0.3.0 as §3 below.** The +shape is a compile-once-walk-many compiled form over the BAST +document's value-domain constraints, symmetric to `ReadPlan` (packed +read-side) and `OffsetMap` (aligned R/W). The concrete shape, +construction, and `validate_bytes` integration are scoped in the +0.3.0 implementation plan (a dedicated phase) and detailed in a +follow-on design session before implementation; this ADR commits the +*decision* (in 0.3.0, not deferred) and the *scope* (a compiled +validation form that retires the interpretive `BastDoc` walk in +`bast_validation`), so the deferral black hole is closed. ## Decision @@ -79,7 +122,11 @@ read-path fix. Explicitly deferred. Add `#[derive(Hash, Eq)]` (alongside the existing `Debug, Clone, PartialEq`) to `ReadPlan` and `OffsetMap`, plus their public sub-types (`FieldPlan`, `CompositePlan`, `ReadKind`, `DiscriminatorPlan`, -`VariantPlan`, `ByteRange`, and the new `LeafMeta` — see §2). +`ByteRange`, and the new `LeafMeta` — see §2). `VariantPlan`/ +`VariantKind` are not in the production `ReadPlan` shape (ADR-011 +was refined on acceptance to drop them — see ADR-011 status), so +they are not derived. `ValidationPlan` (§3) gets `Hash + Eq` + its +own `fingerprint()` as part of its public surface. **`by_name` representation change.** `ReadPlan.by_name` is currently `HashMap`. `HashMap` iteration order is non-deterministic @@ -224,7 +271,76 @@ This is additive on the *capability* (the map now answers questions it previously couldn't) but breaking on the *signature* (`get`'s return type changes). Rides the 0.3.0 bump. -### 3. Fingerprinting `OffsetMap` (bundled with §2b) +### 3. `ValidationPlan` — compile-once validation form + +`bast_validation` currently walks `BastDoc` interpretively on every +`validate_bytes` call to check value-domain constraints (enum value +sets, integer ranges, `maxLength` caps, union variant keys). After +ADR-011, the *read* half of `validate_bytes` (packed) is plan-fast; +the *validation* half is still an interpretive `BastDoc` walk per +buffer. On the `alkcall` hub/spoke topology, `validate_bytes` is the +gate between "bytes arrived from an untrusted peer" and "act on the +decoded frame," so it runs once per incoming buffer and validation is +hot in the same sense review #004 measured for the read path. + +**Decision: a `ValidationPlan` is a compiled form over the BAST +document's value-domain constraints, built once at `compile` time +(symmetric to `ReadPlan`/`OffsetMap`) and walked by +`bast_validation`/`validate_bytes` without re-touching `BastDoc`.** + +The shape, construction, and `validate_bytes` integration are scoped +in the 0.3.0 implementation plan as a dedicated phase and detailed in +a follow-on design session before implementation begins. The +properties this ADR commits to (so the plan and any implementing agent +have a fixed contract): + +- **Compile-once-walk-many.** `ValidationPlan::compile` walks `BastDoc` + once; `validate_bytes` (both modes) walks the `ValidationPlan` per + buffer, never `BastDoc`. This is the same pattern as `ReadPlan` and + `OffsetMap`; it is the structural reason the per-buffer + interpretive cost goes away. +- **Value-domain, not byte-position.** The plan carries constraint + descriptors (enum allowed-sets, integer range bounds, `maxLength` + caps, union variant keys, and any other value-domain checks + `bast_validation` performs today), keyed for dispatch against the + materialized `Value` tree, not byte offsets. The shape is therefore + different from `ReadPlan`/`OffsetMap`; the *pattern* (compiled form, + immutable, shared via `Arc`) is the same. +- **No new `BastDoc` walk in the hot path.** After this ADR, the only + consumers that walk `BastDoc` interpretively are the one-shot + `compile` paths (`ReadPlan::compile`, `OffsetMap::compute`, + `ValidationPlan::compile`, `LayoutBuilder::new`). The per-buffer + paths (`sequential_reader`, `materialize_packed`, + `materialize_aligned`, `validate_bytes`) all walk compiled forms. + This is the end state ADR-011 pointed at; this ADR closes it. +- **Semver.** `ValidationPlan` is a new public type, re-exported from + `lib.rs`. `validate_bytes`'s *signature* is unchanged (still + `(buffer) -> Result<(), AlkTypeError>`); the change is internal + (walks the plan instead of `BastDoc`). If the `ValidationPlan` + design surfaces a need to change `validate_bytes`'s signature, that + rides the 0.3.0 bump and is recorded in the plan's Semver Contract + table when the shape is scoped. `bast_validation`'s public surface + (`build_validator`, `validate_value`) is reviewed at shape-scope + time; additive changes ride the bump, removals/renames are avoided + unless the shape work shows they're necessary. +- **Fingerprinting.** `ValidationPlan` is `Hash + Eq` with a + `fingerprint()` method, same as `ReadPlan`/`OffsetMap` (§1/§4), so + the downstream uses (cross-run cache, `alkcall` handshake, + schema-version diagnostics) extend to the validation form without + new API. The fingerprint contract generalizes: two validation plans + with equal hashes accept/reject identical `(bytes)` identically. + +**What this ADR does *not* decide** (left to the follow-on shape +session + plan phase): the concrete `ValidationPlan` struct/enum +shape, how `maxLength`/range/enum/union-key constraints are +represented, whether `bast_validation`'s `validate_value` is retired +or kept as a convenience wrapper over the plan, and whether the +`AlkTypeKind`-driven dispatch in `bast_validation` collapses into the +plan or stays a thin match over plan-carried descriptors. These are +shape questions, not decision questions; the decision (in 0.3.0, +compiled form, no per-buffer `BastDoc` walk) is fixed here. + +### 4. Fingerprinting `OffsetMap` (bundled with §2b) Since `OffsetMap` is getting new fields (`LeafMeta`) in §2b, its `#[derive(Hash, Eq)]` (from §1) covers the new fields automatically. @@ -237,7 +353,7 @@ aligned reads/writes over identical bytes. ### In scope - `ReadPlan: Hash + Eq` + `fingerprint()` method (§1). -- `OffsetMap: Hash + Eq` + `fingerprint()` method (§1, §3). +- `OffsetMap: Hash + Eq` + `fingerprint()` method (§1, §4). - `BastDoc<'a>` → `BastDoc` (owned) refactor, scoped to the consumers that currently hold `doc_value: Value` and re-parse: `LayoutBuilder`, `bast_validation`, `materialize_aligned`, `OffsetMap::compute` @@ -249,17 +365,26 @@ aligned reads/writes over identical bytes. `BastDoc::new` + `lookup_leaf_field` (§2b). - `LeafMeta` new public type (§2b). - `BTreeMap` for `ReadPlan.by_name` (§1). +- `ValidationPlan` new public type + `compile` + `Hash + Eq` + + `fingerprint()` (§3). `validate_bytes` (both modes) walks the + `ValidationPlan` instead of re-walking `BastDoc`. `bast_validation` + adopts the plan; the public `validate_value`/`build_validator` + surface is reviewed at shape-scope time and rides the bump only if + the shape work shows a signature change is necessary. +- `ValidationPlan: Hash + Eq` + `fingerprint()` method (§3, §1) — the + fingerprint contract extends to the validation form. ### Out of scope -- `ValidationPlan` — different shape, not a hot loop, deferred (see - "Deferring `ValidationPlan`" above). - Disk-cache or handshake *implementations* — the fingerprint - *contract* and method are in scope; the downstream uses (cache - format, wire protocol) are the consumers' problem, not this ADR's. -- Aligned-mode `validate_bytes` — already uses `OffsetMap` + the - BAST-native validator; the `BastDoc` ownership change touches it - but no new compiled form is needed. + *contract* and method are in scope (§1, §3); the downstream uses + (cache format, wire protocol) are the consumers' problem, not this + ADR's. +- The concrete `ValidationPlan` struct/enum shape and constraint + representation — scoped in the 0.3.0 implementation plan's + dedicated phase and a follow-on design session (see §3 "What this + ADR does *not* decide"). The decision (in 0.3.0, compiled form, no + per-buffer `BastDoc` walk) is fixed; the shape is not. - Cross-version fingerprint stability — the fingerprint is stable within a crate version but may change across versions (a new `AlkTypeKind` variant, for example, changes the hash). Cross-version @@ -271,21 +396,32 @@ aligned reads/writes over identical bytes. ### Positive -- **One breaking release, not two.** ADR-011's `ReadPlan` + this - ADR's `BastDoc`-owned + `OffsetMap` extension ship together. The - two in-house downstream consumers (`alktty`, `alkcall`) update once. +- **One breaking release, not two (or three).** ADR-011's `ReadPlan` + + this ADR's `BastDoc`-owned + `OffsetMap` extension + + `ValidationPlan` all ship together. The two in-house downstream + consumers (`alktty`, `alkcall`) update once. - **Closes all deferred M1 sites.** `LayoutBuilder::build` (`layout_builder.rs:190`), `read_field` (`engine.rs:334`), `write_field` (`engine.rs:467`) all stop re-parsing. The packed-side `validate_bytes` (`engine.rs:284`) was already closed by ADR-011; this ADR closes the aligned-side equivalent. +- **Retires the interpretive validation walk.** `validate_bytes` on + untrusted streams (the `alkcall` common case) stops re-walking + `BastDoc` per buffer. This is the latent perf cliff review #005 M3 + flagged: the read half was plan-fast after ADR-011, the validation + half was not. Closing it here — while there are zero real consumers + and one breaking bump already paying the downstream-churn cost — + avoids a second breaking change to `validate_bytes`/`bast_validation` + after 0.3.0. - **Fingerprinting enables downstream uses.** Cross-run plan caching, `alkcall` schema handshake, and schema-version diagnostics all - become possible without further API work. + become possible without further API work — across `ReadPlan`, + `OffsetMap`, and `ValidationPlan`. - **`BastDoc` owned is a net simplification.** One typed-tree type, - owned, used by all non-`ReadPlan` consumers. No more + owned, used by all `*::compile` paths. No more lifetime-entanglement workarounds. The "re-parse on demand" framing - from ADR-007 is fully retired across both read and write paths. + from ADR-007 is fully retired across read, write, and validation + paths. - **`OffsetMap` extension is additive capability.** The map now answers `kind`/`encoding`/`endian` questions it previously couldn't, enabling future aligned-side tools without re-walking `BastDoc`. @@ -295,7 +431,8 @@ aligned reads/writes over identical bytes. - **Breaking public-API changes (0.2.0 → 0.3.0).** `BastDoc<'a>` → `BastDoc` (owned) changes every `Bast*` signature that took `&'a`. `OffsetMap::get` return type changes. `LeafMeta` is new public. - `ReadPlan` is new public (from ADR-011). All ride the bump. + `ReadPlan` is new public (from ADR-011). `ValidationPlan` is new + public (§3). All ride the bump. - **`BastDoc` ownership refactor is broad.** Touches `bast.rs` (every typed node: `&'a str` → `String`/`Arc`, `&'a Value` → `Value`/`Arc`) and every consumer (`layout_builder`, @@ -305,6 +442,13 @@ aligned reads/writes over identical bytes. anyway. The refactor is mechanical (lifetime removal, not logic rewrites); the POC on `readplan-poc` confirmed the read path is unaffected. +- **`ValidationPlan` shape work is not yet scoped.** §3 fixes the + decision (in 0.3.0, compiled form) but defers the concrete shape to + a follow-on design session + a dedicated plan phase. This is a + tracked, owned deferral with a concrete reactivation trigger (the + shape session before phase 7), not a black-hole hedge: the + implementation phase is committed in the plan, so the work cannot + slip past 0.3.0 without reopening this ADR. - **Fingerprint cross-version stability is not guaranteed.** A future `AlkTypeKind` variant changes the hash. Documented as a within- version contract. Consumers that need cross-version stability @@ -318,7 +462,9 @@ aligned reads/writes over identical bytes. `PackedLayout`; the aligned R/W compiled form is `OffsetMap`. The M1 fixes are "cache the parse" (§2a) and "extend the compiled form with leaf metadata" (§2b), not "add a third compiled form." -- **Not a `ValidationPlan`.** Deferred — different shape, not hot. +- **Not a `ValidationPlan` deferral.** `ValidationPlan` is in scope + (§3). The shape is to be scoped in a follow-on session; the + decision to ship in 0.3.0 is fixed. - **Not cross-version fingerprint stability.** Within-version only. - **Not a disk-cache or wire-protocol spec.** The fingerprint contract and method are in scope; the downstream uses are the consumers' @@ -332,27 +478,34 @@ for the step-by-step execution order. The high-level grouping: 1. **`ReadPlan` (ADR-011 steps 1–5)** — the packed read-path fix. Closes H1 + packed-side M1 + L1 + L2. 2. **`BastDoc` owned (§2a)** — the typed-tree ownership refactor. Prerequisite - for the `LayoutBuilder` M1 fix. + for the `LayoutBuilder` M1 fix and for `ValidationPlan::compile`. 3. **`LayoutBuilder` M1 fix (§2a)** — cache the owned `BastDoc` at `new()`. 4. **`OffsetMap` extension (§2b)** — carry `LeafMeta`; close the aligned-side `read_field`/`write_field` M1. -5. **Fingerprinting (§1, §3)** — `Hash + Eq` + `fingerprint()` on - `ReadPlan` and `OffsetMap`. Rides on top of the above. -6. **Public API bump (0.2.0 → 0.3.0)** — `lib.rs` re-exports, version +5. **`ValidationPlan` (§3)** — compiled validation form; `validate_bytes` + walks the plan instead of `BastDoc`. Requires the owned `BastDoc` + from step 2 for `ValidationPlan::compile`. +6. **Fingerprinting (§1, §4)** — `Hash + Eq` + `fingerprint()` on + `ReadPlan`, `OffsetMap`, and `ValidationPlan`. Rides on top of the + above. +7. **Public API bump (0.2.0 → 0.3.0)** — `lib.rs` re-exports, version bump, update `alktty`/`alkcall`. -7. **Verification block** — full suite + wasm + bench. +8. **Verification block** — full suite + wasm + bench. ## References - [ADR-011](011-compiled-read-plan-for-packed-mode.md) — the `ReadPlan` (packed read-side compiled form). This ADR extends the - 0.3.0 release with fingerprinting and the deferred M1 fixes. + 0.3.0 release with fingerprinting, the deferred M1 fixes, and the + `ValidationPlan`. - [Review #004](../../reviews/004-performance-review.md) — the performance finding (H1, M1, L1, L2). ADR-011 closed H1 + packed M1 + L1 + L2; this ADR closes the aligned-side M1. +- [Review #005](../../reviews/005-plan-review-030.md) — the 0.3.0 plan + review whose M3 finding reversed the `ValidationPlan` deferral. - [ADR-007](007-packed-mode-read-factory.md) — the "re-parse on - demand" framing, retired across both read and write paths by - ADR-011 + this ADR. + demand" framing, retired across read, write, and validation paths + by ADR-011 + this ADR. - [ADR-002](002-two-layout-modes-packed-vs-aligned.md) — the two layout modes; `OffsetMap` is the aligned R/W compiled form extended here with `LeafMeta`. diff --git a/docs/plans/030-compiled-forms.md b/docs/plans/030-compiled-forms.md index 04d6600..5df9a2d 100644 --- a/docs/plans/030-compiled-forms.md +++ b/docs/plans/030-compiled-forms.md @@ -1,16 +1,18 @@ --- status: in-progress created: 2026-08-19 -last_updated: 2026-08-19 +last_updated: 2026-08-20 adr: ADR-011, ADR-012 --- -# 0.3.0 — Compiled Forms: ReadPlan, Owned BastDoc, OffsetMap LeafMeta, Fingerprinting +# 0.3.0 — Compiled Forms: ReadPlan, Owned BastDoc, OffsetMap LeafMeta, ValidationPlan, Fingerprinting This is the execution plan for the 0.3.0 release: the compiled-form rollup that closes review #004's 400x read-path gap (ADR-011) *and* -the deferred M1 sites (ADR-012) *and* adds plan fingerprinting -(ADR-012) in one breaking bump. It is the **entry point** an +the deferred M1 sites (ADR-012) *and* retires the interpretive +validation walk via a `ValidationPlan` (ADR-012 §3, reversing the +original deferral per review #005 M3) *and* adds plan fingerprinting +(ADR-012 §1/§4) in one breaking bump. It is the **entry point** an implementing agent reads first. Companion documents: @@ -18,8 +20,8 @@ Companion documents: - [ADR-011](../architecture/decisions/011-compiled-read-plan-for-packed-mode.md) — the `ReadPlan` decision (packed read-side compiled form). - [ADR-012](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md) - — fingerprinting + owned `BastDoc` + `OffsetMap` `LeafMeta` (this - release's two other pieces). + — fingerprinting + owned `BastDoc` + `OffsetMap` `LeafMeta` + + `ValidationPlan` (this release's other three pieces). - [Review #004](../reviews/004-performance-review.md) — the performance finding being closed. - [POC findings](../../poc/readplan/FINDINGS.md) (branch `readplan-poc`) @@ -33,7 +35,7 @@ phases in order; within a phase, steps are ordered by dependency. ## Phases vs sessions -This plan is deliberately larger than one session's work. The seven +This plan is deliberately larger than one session's work. The eight phases are the session boundaries — each phase is a coherent unit that leaves the tree building and tests green, so any one session can pick up a phase without needing context from the previous one. @@ -61,10 +63,13 @@ re-exports. | `ByteRange` | **Unchanged** | Still `Copy + PartialEq + Eq + Hash`. | | `LeafMeta` | **New public type** | `{ kind: AlkTypeKind, encoding: VariableEncoding, endian: Endian }`, re-exported from `lib.rs`. `Copy + PartialEq + Eq + Hash`. | | `ReadPlan` | **New public type** | From ADR-011. Re-exported from `lib.rs`. `Debug + Clone + PartialEq + Eq + Hash`. | -| `SequentialReader` | **Breaking (constructor)** | `SequentialReader::new(&Value, &str)` → `SequentialReader::new(Arc)`. Public methods (`read_next`/`read_field`/`reset`/`position`/`endian`/`schema`) unchanged. `schema()` returns `&Value` retained on the plan. | +| `SequentialReader` | **Breaking (constructor + return type)** | `SequentialReader::new(&Value, &str) -> Result` → `SequentialReader::new(Arc) -> Self` (infallible — just stores the `Arc`; the `BastDoc` parse moved to `ReadPlan::compile`). Public methods (`read_next`/`read_field`/`reset`/`position`/`endian`/`schema`) unchanged. `schema()` returns `&Value` retained on the plan (see phase 2 — the plan stores `Arc`, not `&Value`, to avoid the self-referential struct ADR-011 rejects). `engine.rs`'s `.ok()` on the old `Result` correspondingly goes away. | | `FieldValue` | **Unchanged** | — | | `materialize_packed` | **Breaking (signature)** | `materialize_packed(&BastDoc<'_>, &[u8])` → `materialize_packed(&ReadPlan, &[u8])`. | | `materialize_aligned` | **Breaking (signature)** | `materialize_aligned(&BastDoc<'_>, &[u8], &OffsetMap)` → `materialize_aligned(&BastDoc, &[u8], &OffsetMap)` (owned `BastDoc`, no lifetime). | +| `ValidationPlan` | **New public type** | From ADR-012 §3. Re-exported from `lib.rs`. `Debug + Clone + PartialEq + Eq + Hash`. `compile(&BastDoc, &str) -> Result`, `fingerprint() -> u64`. Shape scoped in phase 7 (and a preceding design session); the contract is fixed in ADR-012 §3. | +| `AlkTypeEngine::validate_bytes` | **Unchanged (signature)** | Still `(buffer) -> Result<(), AlkTypeError>`. Internally walks `&ValidationPlan` (both modes) instead of re-walking `BastDoc` for value-domain checks. The `materialize` half is unchanged from the ADR-011/§2b work (packed: `materialize_packed(&self.plan, ...)`; aligned: `materialize_aligned(&self.doc, ..., &self.offset_map)`). | +| `bast_validation` (`build_validator`, `validate_value`) | **Additive (reviewed at phase 7)** | `validate_value` is expected to become a thin wrapper over `ValidationPlan` (or be retired if the shape work shows it's redundant). Additive changes ride the bump; renames/removals are avoided unless phase 7's shape work shows they're necessary. The BAST meta-schema validator (`build_validator`/`BAST_META_SCHEMA`) used at `compile` time is unaffected. | | `LayoutBuilder`, `PackedLayout`, `FieldPosition` | **Unchanged (signature)** | `LayoutBuilder::new`/`build` signatures unchanged. Internally caches the owned `BastDoc` instead of re-parsing. | | `AlkTypeKind`, `Endian`, `VariableEncoding` | **Unchanged** | — | | `AlkTypeError` | **Unchanged** | — | @@ -74,12 +79,13 @@ re-exports. **Net breaking surface:** `BastDoc` and all `Bast*` types (lifetime removal), `OffsetMap::get` (return type), `SequentialReader::new` -(constructor), `materialize_packed`/`materialize_aligned` (signatures). -**Net additive:** `ReadPlan`, `LeafMeta`, `fingerprint()` methods, -`Hash + Eq` derives on `ReadPlan`/`OffsetMap`. **Net unchanged:** the -builder, `AlkTypeEngine` accessors (signatures), `FieldValue`, -`AlkTypeKind`, `AlkTypeError`, `data_access`, the `Schema`/`Definitions` -builders. +(constructor + `Result` drop), `materialize_packed`/`materialize_aligned` +(signatures). **Net additive:** `ReadPlan`, `LeafMeta`, `ValidationPlan`, +`fingerprint()` methods, `Hash + Eq` derives on `ReadPlan`/`OffsetMap`/ +`ValidationPlan`. **Net unchanged:** the builder, `AlkTypeEngine` +accessors (signatures), `FieldValue`, `AlkTypeKind`, `AlkTypeError`, +`data_access`, the `Schema`/`Definitions` builders, `validate_bytes` +(signature). ### Decisions deferred to their implementation phases @@ -102,7 +108,7 @@ builders. returns `element_stride: 0` for fixed-size struct arrays (`sequential_reader.rs:567`). The `ReadPlan` correctly computes the stride. Decision: preserve the existing `0` behavior in `ReadPlan` for - back-comat with `SequentialReader`'s consumer contract, *or* fix it + back-compat with `SequentialReader`'s consumer contract, *or* fix it and document the behavioral change. Lean: fix it — the `0` is a latent bug, the stride is behaviorally observable, and we're bumping. The plan step calls this out explicitly. Decided in phase 2. @@ -110,23 +116,31 @@ builders. ## Phase 1 — `ReadPlan` type + `compile` (ADR-011 step 1) **Goal:** Add the `ReadPlan`/`FieldPlan`/`CompositePlan`/`ReadKind`/ -`DiscriminatorPlan`/`VariantPlan` types and `ReadPlan::compile(&Value, -&str) -> Result`. Pure addition; no existing code -touched. This is the foundation — later phases wire it into the engine -and reader. +`DiscriminatorPlan` types and `ReadPlan::compile(&Value, &str) -> +Result`. Pure addition; no existing code touched. +This is the foundation — later phases wire it into the engine and +reader. (The POC's `VariantPlan`/`VariantKind` types are **dropped** +in the production shape — see the union-shape note below.) **ADR reference:** [ADR-011 §The `ReadPlan` shape](../architecture/decisions/011-compiled-read-plan-for-packed-mode.md#the-readplan-shape), [ADR-011 §Construction](../architecture/decisions/011-compiled-read-plan-for-packed-mode.md#construction). +The `CompositePlan::Union` shape in ADR-011 was refined (vs the +accepted-at-POC shape) to carry the field-disc union's shared fields +and to drop `VariantPlan`/`VariantKind`; this phase implements the +refined shape. -**POC reference:** `poc/readplan/src/lib.rs` (branch `readplan-poc`) is -the reference scaffold. The production version lives in `src/` and -adds doc comments, clippy cleanliness, and the field-name-discriminator -union read shape the POC stubbed (see POC Finding 1). +**POC reference:** `poc/readplan/src/lib.rs` (branch `readplan-poc`) +is the reference scaffold. The production version lives in `src/` and +adds doc comments, clippy cleanliness, the field-name-discriminator +union read shape the POC stubbed (POC Finding 1), and nested-union +variant support the POC rejected but 0.2.0 accepts (POC "What this +POC does not cover" → nested unions). Both are resolved by the +refined `CompositePlan::Union` shape — see below. **Files:** New `src/read_plan.rs`. Update `src/lib.rs` to add `pub mod read_plan;` and re-export `ReadPlan` (and the plan sub-types -that are part of the public surface — `ReadKind`, `CompositePlan`, etc. -if the ADR's public-API section calls for them; the ADR lists +that are part of the public surface — `ReadKind`, `CompositePlan`, +etc. if the ADR's public-API section calls for them; the ADR lists `ReadPlan` as the public type, sub-types can stay `pub` in-module if consumers don't need to name them). @@ -140,16 +154,32 @@ consumers don't need to name them). - `by_name: BTreeMap` (not `HashMap` — ADR-012 §1 requires `Hash` on `ReadPlan`, and `HashMap` blocks derive). The POC used `HashMap`; swap to `BTreeMap`. -- **Field-name-discriminator union read shape (POC Finding 1):** the - POC stubbed `plan_read_union`'s `Field` arm. The production - `compile_union` must produce a plan that the read loop can walk for - the field-disc case. The shape: `CompositePlan::Union` carries the - union's declared `fields` as a sub-`ReadPlan` (the discriminator field - + any shared fields), and the variant plans are laid out *after* the - shared fields. The read loop reads the discriminator field from the - sub-plan, looks up the variant, and walks the variant plan. This is - the one piece the POC deliberately left as a TODO — the production - version must implement it. See `poc/readplan/FINDINGS.md` Finding 1. +- **Union shape (ADR-011 refined — resolves POC Finding 1 and the + nested-union gap):** `CompositePlan::Union { disc, shared, + variants: Vec<(String, CompositePlan)> }`. + - `shared: Option>` — the union's declared `fields` + (the discriminator field + any shared fields) for the + field-name-discriminator case. `DiscriminatorPlan::Field.field_index` + indexes into `shared`. The read loop walks `shared` first, then + looks up the selected variant and walks its `CompositePlan` + starting after the shared fields. The byte-offset-discriminator + case sets `shared: None` (no shared fields; the variant starts + immediately after the discriminator size). This replaces the + POC's stubbed `plan_read_union` `Field` arm. + - `variants: Vec<(String, CompositePlan)>` — **not** the POC's + `Vec<(String, VariantPlan)>`. Dropping `VariantPlan`/`VariantKind` + means a variant's body is just a `CompositePlan`, so **nested + unions** (a variant that is itself a union, which the 0.2.0 + reader supports via `resolve_and_walk_variant`'s `Union` arm at + `sequential_reader.rs:800`) work by ordinary `CompositePlan` + recursion — a variant can be `CompositePlan::Union { ... }`. No + separate `VariantKind::Union` arm, no behavioral drop vs 0.2.0, + no Semver Contract entry for a capability regression. The POC's + `VariantPlan { kind, plan }` wrapper is not carried forward. + - `compile_union` must reject a variant that is neither a struct + nor a union with `AlkTypeError::Schema` (mirroring + `resolve_and_walk_variant`'s `other => Err(...)` arm), so the + eager-resolution path keeps the untrusted-input discipline. - Do *not* wire `ReadPlan` into `SequentialReader` or `materialize` yet — that's phase 2. Phase 1 is the type + `compile` only, unit-tested against the same BAST fixtures `bast.rs` uses (the existing `bast.rs` @@ -157,10 +187,18 @@ consumers don't need to name them). **Verification:** `cargo test --release` (new unit tests for `compile` covering every `BastType` arm — port the `cov_*` tests from -`poc/readplan/tests/coverage.rs`). `cargo clippy --all-targets -- -D +`poc/readplan/tests/coverage.rs`, **plus** a nested-union-variant +test asserting `compile_union` produces `CompositePlan::Union` whose +variant body is itself `CompositePlan::Union`, restoring the 0.2.0 +capability the POC rejected). `cargo clippy --all-targets -- -D warnings`. `cargo doc --no-deps` (new public type). `cargo build --target wasm32-unknown-unknown --release` (`read_plan.rs` is -wasm-relevant). +wasm-relevant). **Add a `fn read_plan_is_send_sync()` assertion +test** (a `const _: fn() = || { fn assert_send_sync() +{}; assert_send_sync::(); };` static-bound assertion, as +ADR-011 §"Engine integration" requires) to lock in `ReadPlan: Send + +Sync` so a future change can't break it silently — mirror the POC's +`readplan_is_send_sync` test. --- @@ -182,20 +220,46 @@ packed mode, `sequential_reader()` hands out `Arc::clone`, packed `validate_bytes` calls `materialize_packed(&self.plan, ...)`). **Implementation notes:** -- `SequentialReader::new(&Value, &str)` → - `SequentialReader::new(Arc)`. The reader stores - `plan: Arc`, `field_index: usize`, `position: usize`. - `endian()` reads `self.plan.endian()`. `schema()` returns a `&Value` - retained on the plan (the plan stores the `&Value` it was compiled - from — see ADR-011 §Engine integration; the `&Value` outlives the - plan because the engine owns both). +- `SequentialReader::new(&Value, &str) -> Result` → + `SequentialReader::new(Arc) -> Self` (infallible — the + fallible `BastDoc` parse moved to `ReadPlan::compile` in phase 1; + `new` just stores the `Arc`). The reader stores `plan: Arc`, + `field_index: usize`, `position: usize`. `endian()` reads + `self.plan.endian()`. +- **`schema()` ownership (resolves review #005 H2):** `schema()` + returns `&Value` retained on the plan, but the plan stores an + **`Arc`**, not a `&Value`. ADR-011 §"Root cause" rejects the + self-referential struct pattern (a `ReadPlan` storing `&Value` + borrowing from the engine's `bast_doc: Value` would make the engine + self-referential — exactly the construction ADR-007 worked around + and ADR-011's `Arc` was meant to retire). The fix: + `ReadPlan` carries `schema: Arc`; `ReadPlan::compile` clones + the input `&Value` into `Arc` once; `schema()` returns + `&self.schema`. The engine stores `bast_doc: Arc` internally + (one allocation, shared via refcount between the engine and all + plans it builds) — this is an internal change, not a public + signature change (`compile` still takes `&Value`). `Arc` + implements `Hash + Eq` (`serde_json::Value: Hash + Eq` as of the + pinned `serde_json` with `preserve_order`; `Map::hash` sorts keys + for determinism), so phase 6's `#[derive(Hash)]` on `ReadPlan` is + not blocked by carrying the schema. **Note:** if a future + `serde_json` version regresses `Value: Hash`, phase 6 would need + `ReadPlan`'s hash to exclude the `schema` field; that is a phase-6 + concern, not a phase-2 blocker. - `read_field_at`/`read_field_value`/`read_typeref_value`/ `walk_struct_size`/`read_union_value`/`read_array_value`/ `read_record_value` are rewritten to take plan nodes (`&FieldPlan`/`&CompositePlan`/`&ReadKind`) instead of `&BastField`/`&BastType`/`&BastDoc`. Port `plan_read_field_at`/ `plan_walk_struct_size`/etc. from `poc/readplan/src/lib.rs` — they're - the reference implementations. + the reference implementations. The `read_union_value` rewrite + handles both discriminator kinds via the refined `CompositePlan::Union` + shape from phase 1: byte-disc reads the discriminator at + `disc.offset` then walks the variant (no `shared`); field-disc walks + `shared` first, reads the discriminator field at + `disc.field_index` within `shared`, looks up the variant, and walks + it starting after the shared fields. Nested unions (variant body is + itself `CompositePlan::Union`) recurse naturally — no special arm. - **Struct-array stride (deferred decision 4):** the POC computes the true fixed-struct stride; the existing reader returns `0`. The production `ReadPlan::compile_array` should compute the true stride @@ -204,21 +268,47 @@ packed mode, `sequential_reader()` hands out `Arc::clone`, packed now the true stride for fixed-size struct elements, not `0`. This is a breaking behavioral change; rides the bump. Update the `eq_array_ref_element`-style test to assert the new stride. -- `materialize_packed(&BastDoc<'_>, &[u8])` → - `materialize_packed(&ReadPlan, &[u8])`. The materializer walks the - plan instead of `BastDoc`. The `dummy_field_for`/`ty_source` helpers - in `materialize.rs` are removed (the plan carries everything). +- **`materialize_packed` rewrite + packed/aligned split (resolves + review #005 L1 and L2):** `materialize_packed(&BastDoc<'_>, &[u8])` + → `materialize_packed(&ReadPlan, &[u8])`. The materializer walks the + plan instead of `BastDoc`. **Scoped removal of helpers:** only the + *packed-side* call sites of `dummy_field_for`/`ty_source` + (`materialize.rs:249, 316, 351, 391` — the packed + `materialize_*_packed` paths) go away when packed-materialize moves + to the plan. The helpers themselves **stay**, because + `materialize_leaf_at` (`materialize.rs:631`, which calls + `dummy_field_for`) is on the **aligned** path — it's called by + `materialize_struct_aligned` (`:475`), `materialize_array_aligned` + (`:544`), `materialize_variable_aligned` (`:613`). Aligned + `materialize` keeps walking `BastDoc` through 0.3.0 (see the Scope + Boundary note in phase 5), so `dummy_field_for`/`ty_source` must + stay. **`materialize_typeref_packed` split:** this function is + currently shared by both packed and aligned paths (aligned's + `materialize_leaf_at` calls it to read leaves, and aligned's record + path at `:498-506` calls it directly). After phase 2, + packed-materialize gets a new plan-walking function; + `materialize_typeref_packed` stays for aligned's + `materialize_leaf_at` and the aligned record path (renamed or not — + implementer's choice; the function is private). This is two + mode-specific paths — the existing design — not a "parallel walker" + in the maintenance-tax sense ADR-011 §"Negative" cautions against + (that caution is about packed read-side `SequentialReader` + + `materialize_packed` sharing one plan, which this preserves). - `AlkTypeEngine::compile` (packed branch): build `Arc` via `ReadPlan::compile(bast_doc, root_name)`, store it in `Layout::Packed`. `sequential_reader()` returns `Some(SequentialReader::new(Arc::clone(&self.plan)))`. `validate_bytes` (packed) calls - `materialize_packed(&self.plan, buffer)` then `bast_validation` on - the result (validation still uses `BastDoc` until/unless a future - `ValidationPlan` ADR; for 0.3.0 `validate_bytes` reconstructs a - `BastDoc` for the validator only — the *read* path uses the plan, - the *validation* path uses `BastDoc`. This is acceptable: validation - is not the hot loop H1 traces). + `materialize_packed(&self.plan, buffer)` then runs validation on the + materialized `Value`. **Validation path through phase 2:** until + phase 7 (ValidationPlan), `validate_bytes` reconstructs a `BastDoc` + for the validator only — the *read* path uses the plan, the + *validation* path uses `BastDoc`. This is a temporary bridge: phase 7 + replaces it with a `ValidationPlan` walk (ADR-012 §3), retiring the + per-call `BastDoc` reconstruction. The bridge is acceptable for + phases 2–6 because the ValidationPlan work is committed in this + release (not deferred), so the bridge has a known removal point in + phase 7. **Verification:** `cargo test --release` — the existing `sequential_reader.rs` and `materialize.rs` tests drive `read_next`/ @@ -354,9 +444,22 @@ Closes the last two M1 sites (`engine.rs:334,467`). (re-export `LeafMeta`). **Implementation notes:** +- **`Hash` on `Endian`/`VariableEncoding` (resolves review #005 M2 — + do this first, it's a prerequisite):** `src/schema.rs:205` (`Endian`) + and `:212` (`VariableEncoding`) currently derive only `Debug, Clone, + Copy, PartialEq, Eq` — no `Hash`. `LeafMeta` (below) requires all + its fields to be `Hash` for `#[derive(Hash)]`, and phase 6's + `#[derive(Hash)]` on `ReadPlan`/`OffsetMap` requires `FieldPlan`'s + `endian: Endian` + `encoding: VariableEncoding` to be `Hash`. Add + `Hash` to both derives in `src/schema.rs`. Both are fieldless enums + already at `Eq + PartialEq`, so this is additive and semver-safe — + no behavioral change. Trivial, but it's an unstated prerequisite + the original plan omitted. - New public type `LeafMeta { kind: AlkTypeKind, encoding: VariableEncoding, endian: Endian }`. `Copy + PartialEq + Eq + Hash` - (all fields are `Copy + Hash`). + (all fields are `Copy + Hash` once the sub-step above is done — + `AlkTypeKind` already derives `Hash`; `Endian`/`VariableEncoding` + get it from the sub-step above). - `OffsetMap` storage: `fields: Vec<(String, ByteRange, LeafMeta)>` (was `Vec<(String, ByteRange)>`). The `compute` walk already resolves each leaf's type; add the `LeafMeta` extraction at the point where @@ -383,6 +486,23 @@ Closes the last two M1 sites (`engine.rs:334,467`). leaves; they're walked recursively). The `BastDoc` argument to `materialize_aligned` is now owned (phase 3) — no signature change beyond the lifetime drop. +- **Scope Boundary — aligned `materialize`'s `BastDoc` structure walk + (resolves review #005 L3):** `materialize_struct_aligned` + (`materialize.rs:451-521`) walks `BastDoc` to traverse + struct/array/record *structure*, using `OffsetMap` only for leaf + byte positions. This is the **permanent design for 0.3.0**, not a + deferral: after phase 3 the walk is over owned data (no re-parse, + not O(N²)), and aligned `validate_bytes` is one structure walk per + call (not per-field), so there is no perf driver analogous to review + #004's packed per-chunk gap. ADR-011 §"Out of scope" is half-true + here (aligned materialize takes `&OffsetMap` *and* `&BastDoc`) — + this note owns the decision: aligned materialize keeps walking owned + `BastDoc` for structure through 0.3.0. An `AlignedPlan` that + compiles the structure walk is **not** in scope; if a future bench + shows an aligned-mode hot loop, it gets its own ADR (tracked as an + open question, not a silent gap). Phase 7's `ValidationPlan` does + not change this — validation is value-domain, orthogonal to the + aligned structure walk. **Verification:** `cargo test --release` — existing `offset_map.rs` and `engine.rs` `read_field`/`write_field` tests pass (they go through @@ -392,14 +512,16 @@ wasm32-unknown-unknown --release`. --- -## Phase 6 — Fingerprinting (ADR-012 §1, §3) +## Phase 6 — Fingerprinting `ReadPlan`/`OffsetMap` (ADR-012 §1, §4) **Goal:** Add `Hash + Eq` derives + `fingerprint() -> u64` to `ReadPlan` and `OffsetMap`. Enables cross-run caching, `alkcall` schema handshake, -schema-version diagnostics. +schema-version diagnostics. (`ValidationPlan` gets the same treatment +in phase 7, where it's built — it carries its own `Hash + Eq` + +`fingerprint()` as part of its public surface.) **ADR reference:** [ADR-012 §1](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md#1-fingerprinting--readplan-hash--eq-offsetmap-hash--eq), -[ADR-012 §3](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md#3-fingerprinting-offsetmap-bundled-with-2b). +[ADR-012 §4](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md#4-fingerprinting-offsetmap-bundled-with-2b). **Files:** `src/read_plan.rs` (derives + `fingerprint`), `src/offset_map.rs` (derives + `fingerprint`), `src/lib.rs` (no new re-exports — `Hash`/`Eq` @@ -409,9 +531,18 @@ are trait derives, `fingerprint` is an inherent method). - `ReadPlan` already uses `BTreeMap` for `by_name` (phase 1), so `#[derive(Hash, Eq)]` works. Add it alongside the existing `Debug, Clone, PartialEq`. Same for `FieldPlan`, `CompositePlan`, - `ReadKind`, `DiscriminatorPlan`, `VariantPlan`, `VariantKind`. + `ReadKind`, `DiscriminatorPlan`. (The POC's `VariantPlan`/`VariantKind` + are not in the production shape — phase 1 dropped them — so they are + not derived here.) `ReadPlan` also carries `schema: Arc` from + phase 2; `Arc: Hash + Eq` because `serde_json::Value: Hash + + Eq` (with `preserve_order`, `Map::hash` sorts keys deterministically), + so the `schema` field does not block the derive. If a future + `serde_json` version regresses `Value: Hash`, exclude `schema` from + the derived `Hash` via a manual `impl Hash for ReadPlan` that hashes + every field except `schema` — phase-6 concern, not a blocker. - `OffsetMap` already carries `LeafMeta` (phase 5), and `LeafMeta` is - `Copy + Hash + Eq`. Add `#[derive(Hash, Eq)]` to `OffsetMap`, + `Copy + Hash + Eq` (phase 5 added `Hash` to `Endian`/`VariableEncoding`). + Add `#[derive(Hash, Eq)]` to `OffsetMap`, `OffsetEntry`, `ByteRange` (already `Eq + Hash`), `LeafMeta`. - **Fingerprint hasher (deferred decision 3):** `DefaultHasher` (std, no new dep). The fingerprint isn't hot; cross-version stability is a @@ -435,7 +566,99 @@ are trait derives, `fingerprint` is an inherent method). --- -## Phase 7 — Public API bump, docs, verification (ADR-011 step 6, ADR-012) +## Phase 7 — `ValidationPlan` (ADR-012 §3) + +**Goal:** Retire the interpretive `bast_validation` walk. Introduce a +`ValidationPlan` — a compile-once-walk-many compiled form over the +BAST document's value-domain constraints — built once at `compile` +time and walked by `validate_bytes` (both modes) per buffer instead +of re-walking `BastDoc`. Closes the latent perf cliff review #005 M3 +flagged: after ADR-011 the *read* half of `validate_bytes` is +plan-fast, but the *validation* half still re-walks `BastDoc` per +buffer, which is hot on the `alkcall` read+validate-on-untrusted-stream +common case. + +**ADR reference:** [ADR-012 §3](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md#3-validationplan--compile-once-validation-form). + +**Predecessor for this phase:** a **design session** to scope the +concrete `ValidationPlan` shape (constraint representation, +`compile`/walk structure, `bast_validation` public-surface review) +**before** implementation begins. ADR-012 §3 fixes the decision (in +0.3.0, compiled form, no per-buffer `BastDoc` walk, `Hash + Eq` + +`fingerprint`) and lists what is *not* decided (the struct/enum +shape, the constraint descriptors, whether `validate_value` is +retired or kept as a wrapper). This phase implements whatever the +design session scopes; the contract below holds regardless of shape. + +**Files:** New `src/validation_plan.rs` (the `ValidationPlan` type, +`compile`, walk entry points). `src/bast_validation.rs` (adopt the +plan; `validate_value` either becomes a thin wrapper over the plan +or is retired per the design session's call). `src/engine.rs` +(`compile` builds `Arc` in both modes, stores it; +`validate_bytes` walks `&self.validation_plan` instead of +reconstructing a `BastDoc` for the validator — this removes the +temporary bridge from phase 2). `src/lib.rs` (re-export +`ValidationPlan`). + +**Implementation contract (fixed by ADR-012 §3, independent of +shape):** +- **Compile-once-walk-many.** `ValidationPlan::compile` walks the + owned `BastDoc` once (phase 3 made it owned); `validate_bytes` + walks the `ValidationPlan` per buffer, never `BastDoc`. The only + consumers that walk `BastDoc` interpretively after this phase are + the one-shot `*::compile` paths (`ReadPlan::compile`, + `OffsetMap::compute`, `ValidationPlan::compile`, + `LayoutBuilder::new`). +- **Value-domain, not byte-position.** The plan carries constraint + descriptors (enum allowed-sets, integer range bounds, `maxLength` + caps, union variant keys, and any other value-domain checks + `bast_validation` performs today), keyed for dispatch against the + materialized `Value` tree. The shape is different from + `ReadPlan`/`OffsetMap`; the pattern (compiled form, immutable, + shared via `Arc`) is the same. +- **`Send + Sync`.** `ValidationPlan: Send + Sync` (immutable owned + data, no interior mutability) so `Arc` shares from + the `Send + Sync` engine. Add a `static` bound assertion test + mirroring phase 1's `read_plan_is_send_sync`. +- **`Hash + Eq` + `fingerprint()`.** `ValidationPlan` derives + `Debug, Clone, PartialEq, Eq, Hash` and has + `fingerprint() -> u64` (same `DefaultHasher` implementation as + phase 6). The fingerprint contract generalizes: two validation + plans with equal hashes accept/reject identical `(bytes)` + identically. Add a fingerprint contract test (compile the same + schema twice, assert `plan1 == plan2` and `plan1.fingerprint() == + plan2.fingerprint()`; change one constraint, assert fingerprints + differ). +- **Untrusted-input discipline.** `compile` surfaces malformed + schemas as `AlkTypeError::Schema` (AGENTS.md §3); overflow-safe + arithmetic (AGENTS.md §4). No `unsafe`, no `async`, no new deps, + wasm-clean (AGENTS.md §5–§11). + +**What this phase does *not* include (shape-dependent, scoped by the +design session):** the concrete `ValidationPlan` struct/enum, the +constraint-descriptor representation, the `bast_validation` +public-surface decision (`validate_value` retire-vs-wrapper), and any +`AlkTypeError::Validation` variant changes. These are shape questions +the design session resolves; they are *not* a re-opening of the +"ship in 0.3.0" decision, which is fixed in ADR-012 §3. + +**Verification:** `cargo test --release` — the existing +`bast_validation.rs` and `engine.rs` `validate_bytes` tests are the +primary validation (they drive validation through the public API and +must pass unchanged, confirming behavioral parity with the +interpretive walk). New unit tests for `ValidationPlan::compile` +covering every constraint kind. New `Send + Sync` assertion test. +New fingerprint contract tests. `cargo clippy --all-targets -- -D +warnings`. `cargo doc --no-deps` (new public type). `cargo build +--target wasm32-unknown-unknown --release` (`validation_plan.rs` is +wasm-relevant). **Re-run the alktty `wire_vs_bast` bench** and, if +the design session scopes one, a `validate_bytes`-on-untrusted-stream +bench alongside `wire_vs_bast` to confirm the validation half of +`validate_bytes` no longer dominates per-buffer. + +--- + +## Phase 8 — Public API bump, docs, verification (ADR-011 step 6, ADR-012) **Goal:** Flip the version to 0.3.0, update `lib.rs` re-exports, update the architecture docs (ADR-007 "Cost" rewrite, ADR-011/012 status flip @@ -446,12 +669,14 @@ consumers, run the full verification block. [ADR-012](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md). **Files:** `Cargo.toml` (version 0.2.0 → 0.3.0), `src/lib.rs` -(re-export `ReadPlan`, `LeafMeta`, `OffsetEntry`), `docs/architecture/` -(README ADR table, ADR-007 "Cost" section, ADR-011/012 status), -`docs/architecture/validation.md` / `layout-engine.md` (mention -`ReadPlan`/`LeafMeta` where relevant), in-house downstream repos -(`alktty`, `alkcall` — update call sites for `OffsetMap::get`, -`SequentialReader::new`, `materialize_packed`, `BastDoc` owned). +(re-export `ReadPlan`, `LeafMeta`, `OffsetEntry`, `ValidationPlan`), +`docs/architecture/` (README ADR table, ADR-007 "Cost" section, +ADR-011/012 status), `docs/architecture/validation.md` / +`layout-engine.md` (mention `ReadPlan`/`LeafMeta`/`ValidationPlan` +where relevant), in-house downstream repos (`alktty`, `alkcall` — +update call sites for `OffsetMap::get`, `SequentialReader::new`, +`materialize_packed`, `BastDoc` owned, `validate_bytes` internal +change if any signature change surfaced in phase 7's shape work). **Implementation notes:** - **ADR-007 "Cost" section (L2 from review #004):** rewrite the @@ -460,13 +685,14 @@ consumers, run the full verification block. "Accepted." This is the last loose end from review #004. - **`src/engine.rs:112-115` doc comment (L2):** rewrite the "re-parse the typed tree on demand" comment to describe the compiled-form - architecture (ReadPlan for packed reads, OffsetMap+LeafMeta for - aligned, owned BastDoc for the builder/validator). -- **`lib.rs` re-exports:** add `ReadPlan`, `LeafMeta`, `OffsetEntry`. - `BastDoc` and `Bast*` stay re-exported (signatures changed in phase 3, - names unchanged). `materialize_packed`/`materialize_aligned` stay - re-exported (signatures changed). `SequentialReader` stays - re-exported (`new` signature changed). + architecture (`ReadPlan` for packed reads, `OffsetMap`+`LeafMeta` + for aligned, `ValidationPlan` for validation, owned `BastDoc` for + the builder and the `*::compile` paths). +- **`lib.rs` re-exports:** add `ReadPlan`, `LeafMeta`, `OffsetEntry`, + `ValidationPlan`. `BastDoc` and `Bast*` stay re-exported (signatures + changed in phase 3, names unchanged). `materialize_packed`/ + `materialize_aligned` stay re-exported (signatures changed). + `SequentialReader` stays re-exported (`new` signature changed). - **Downstream updates:** `alktty`'s bench (`benches/wire_vs_bast.rs`) updates `SequentialReader::new` call + any `OffsetMap::get` usage. `alkcall` updates similarly. Both are in-house path dev-deps; the @@ -502,36 +728,52 @@ variant dispatch cost. ## Cross-phase invariants - **The tree builds and tests pass at every phase boundary.** No phase - leaves the crate in a non-compiling state. Phase 1 (add `ReadPlan`) - and phase 6 (add derives) are pure additions. Phases 2–5 are rewrites - that must leave tests green. + leaves the crate in a non-compiling state. Phases 1 (add `ReadPlan`), + 6 (add `Hash`/`Eq` derives to `ReadPlan`/`OffsetMap`), and 7 (add + `ValidationPlan`) are pure additions; phases 2–5 are rewrites that + must leave tests green; phase 8 is the bump/docs. - **The POC on `readplan-poc` is the reference scaffold for phases 1–2.** It is *not* merged to main; it stays on the branch as the derisking record, like `bast-validator-poc`. If a phase 1–2 implementation - question arises about the plan shape, consult the POC. + question arises about the plan shape, consult the POC. (The POC's + `VariantPlan`/`VariantKind` and its nested-union rejection are + **not** carried forward — phase 1's refined `CompositePlan::Union` + shape supersedes both; see phase 1.) - **Review #004 is the closure target.** H1 → phase 2; packed M1 → phase 2; aligned M1 → phases 4–5; L1 → phase 2 (falls out); L2 → - phase 7 (doc rewrite). The review's status flips to "closed" in the - phase 7 commit. + phase 8 (doc rewrite). The review's status flips to "closed" in the + phase 8 commit. +- **Review #005 is the closure target for the plan-spec issues.** H1 + → phase 1 (refined union shape in ADR-011 + plan); H2 → phase 2 + (`Arc` on the plan); M1 → phase 1 (nested unions via + `CompositePlan` recursion, no behavioral drop); M2 → phase 5 (`Hash` + on `Endian`/`VariableEncoding`); M3 → ADR-012 §3 + phase 7 + (`ValidationPlan` shipped in 0.3.0, deferral reversed); L1/L2/L3 → + phase 2 / phase 5 Scope Boundary; N1 → typo; N2 → phase 1 + `Send + Sync` assertion test; N3 → Semver Contract table row. - **No `unsafe`, no `async`, no new deps, no feature flags** (AGENTS.md §5–§11). The owned-`BastDoc` refactor uses `String`/`Value`, not `unsafe` self-referential tricks. `DefaultHasher` is std. Wasm-clean - throughout. + throughout. `ValidationPlan` follows the same constraints. - **`preserve_order` stays load-bearing** (AGENTS.md §8). The owned- `BastDoc` refactor must not sort schema object keys anywhere; field order in the `Value` still determines byte order in packed mode and - iteration order in both modes. + iteration order in both modes. `ValidationPlan::compile` inherits + this — value-domain checks that depend on field ordering (e.g. union + discriminator field lookup) respect `preserve_order`. ## What this plan is *not* -- **Not a `ValidationPlan`.** Deferred per ADR-012 — different shape, - not a hot loop. - **Not a disk-cache or wire-protocol spec.** The fingerprint contract - and method are in scope (phase 6); downstream uses are the consumers' - concern. + and method are in scope (phase 6 for `ReadPlan`/`OffsetMap`, phase 7 + for `ValidationPlan`); downstream uses are the consumers' concern. - **Not cross-version fingerprint stability.** Within-version only (ADR-012). The fingerprint may change across versions if a new `AlkTypeKind` variant is added; consumers cache within a version. - **Not a perf bench.** The bench lives in alktty; this plan re-runs it - at phase 2 and phase 7 to confirm the gap closes. The plan itself - only asserts correctness/coverage. \ No newline at end of file + at phase 2 and phase 8 to confirm the gap closes. The plan itself + only asserts correctness/coverage. +- **Not an `AlignedPlan`.** Aligned `materialize`'s `BastDoc` structure + walk is the permanent 0.3.0 design (phase 5 Scope Boundary). An + `AlignedPlan` is out of scope; if a future bench motivates one, it + gets its own ADR. \ No newline at end of file diff --git a/docs/reviews/005-plan-review-030.md b/docs/reviews/005-plan-review-030.md index a490a35..208f820 100644 --- a/docs/reviews/005-plan-review-030.md +++ b/docs/reviews/005-plan-review-030.md @@ -1,6 +1,7 @@ --- status: open -last_updated: 2026-08-19 +last_updated: 2026-08-20 +resolved_findings: 2026-08-20 (all 11 — see "Resolution" at the end) reviewed_artifacts: - docs/plans/030-compiled-forms.md - docs/architecture/decisions/011-compiled-read-plan-for-packed-mode.md @@ -580,4 +581,79 @@ non-blocking and can run in parallel. holes) alongside classic planning mistakes. It surfaced M1 and L3 that a conventional severity-only review would have missed or under-weighted. Worth retaining as a default scan for future plan - reviews. \ No newline at end of file + reviews. + +--- + +## Resolution (2026-08-20) + +All 11 findings resolved in one docs-only edit pass to ADR-011, +ADR-012, and the 0.3.0 plan. No source changed; the crate still +builds/tests at v0.2.0. The M3 deferral reversal is the one +substantive decision change (per user direction: ship ValidationPlan +in 0.3.0, no more hedging); the rest are spec corrections or +pre-implementation refinements to types that do not yet exist on +`main`. + +- **H1 (union shape):** ADR-011 §"The `ReadPlan` shape" refined — + `CompositePlan::Union` now carries `shared: Option>` + (field-disc shared fields) and `variants: Vec<(String, + CompositePlan)>` (dropping `VariantPlan`/`VariantKind`). Plan + phase 1 rewritten to implement the refined shape. The shape + refinement is pre-implementation (the types don't exist on `main`). +- **H2 (`schema()` `&Value`):** plan phase 2 rewritten — `ReadPlan` + stores `schema: Arc` (not `&Value`); `schema()` returns + `&self.schema`. Verified `serde_json::Value: Hash + Eq` holds with + `preserve_order` (`Map::hash` sorts keys deterministically), so + phase 6's `#[derive(Hash)]` on `ReadPlan` is not blocked. +- **M1 (nested unions):** resolved as the review's option (a) — + nested-union support falls out of the H1 shape refinement (a + variant can be `CompositePlan::Union`), so no behavioral drop vs + 0.2.0 and no Semver Contract entry for a capability regression. + Plan phase 1 adds a nested-union-variant test. +- **M2 (`Hash` on `Endian`/`VariableEncoding`):** plan phase 5 + rewritten with an explicit first sub-step to add `Hash` to both + derives in `src/schema.rs` (additive, semver-safe). The inaccurate + "all fields are `Copy + Hash`" parenthetical on `LeafMeta` is + corrected. +- **M3 (`ValidationPlan`):** deferral **reversed** per user + direction. ADR-012 §"Deferring `ValidationPlan`" rewritten as + "ValidationPlan — in scope for 0.3.0"; new ADR-012 §3 commits the + decision (compiled form, no per-buffer `BastDoc` walk, `Hash + Eq` + + `fingerprint()`) and lists the shape questions deferred to a + follow-on design session + the plan's new phase 7. Plan gains a + new phase 7 (ValidationPlan); old phase 7 (bump) renumbered to + phase 8. ADR-011's "Out of scope" `bast_validation` bullet and + "Scope Boundaries" `Not a validation plan` bullet updated to point + at ADR-012 §3. Plan's "What this plan is *not*" first bullet + removed. The deferral-black-hole pattern this review's methodology + flagged is closed: the work is committed in the plan with a + concrete reactivation trigger (the shape session before phase 7), + not hedged into an unplanned future. +- **L1 (`dummy_field_for`/`ty_source`):** plan phase 2 rewritten — + only the packed-side call sites go away; the helpers stay for the + aligned `materialize_leaf_at` path. +- **L2 (`materialize_typeref_packed` split):** plan phase 2 + rewritten — packed-materialize gets a new plan-walking function; + `materialize_typeref_packed` stays for aligned's + `materialize_leaf_at` and the aligned record path. +- **L3 (aligned-materialize `BastDoc` structure walk):** plan phase 5 + gains a Scope Boundary note — the walk is the permanent 0.3.0 + design; an `AlignedPlan` is out of scope, tracked as an open + question if a future bench motivates it. +- **N1 (typo):** "back-comat" → "back-compat" in deferred decision 4. +- **N2 (`Send + Sync` assertion test):** plan phase 1 verification + rewritten to add the `read_plan_is_send_sync` static-bound + assertion test ADR-011 §"Engine integration" requires. +- **N3 (`Result` drop on `SequentialReader::new`):** Semver Contract + table row updated to note the constructor return-type change + (`Result` → `Self`) alongside the argument-type + change. + +The deferral-pattern scan's general signal (healthy deferrals have a +concrete reactivation condition + tracking; black holes have neither) +is reaffirmed by the M3 reversal: the original "if a bench motivates +it" trigger was a black hole because no bench was ever going to be +run against a path that didn't exist yet, and the cost of inaction +(a second breaking change to `validate_bytes`/`bast_validation` after +0.3.0) was hidden by the "not a hot loop" framing. \ No newline at end of file