Fix H3: field-disc union wire convention — shared-then-variant (review #006)

Decision (recorded as an ADR-011 addendum): the packed-mode wire layout for a field-name-discriminator TUnion is shared-then-variant — the union's declared `fields` (disc + shared fields) first, then the variant's own fields. Reader and materializer already implemented this; LayoutBuilder was corrected from variant-only layout. Enforcement in BastUnion::parse (the choke point every consumer inherits — union roots at BastDoc::new, referenced unions at resolve_ref): - discriminator field must be declared in `fields` - `fields` must not contain duplicate names - variants must not re-declare shared fields (checked inline and through $ref resolution — parse chain now threads the doc root) - the discriminator field must be the FIRST entry in `fields` (the reader reads the disc at the union start; a later position made it dispatch on the wrong bytes — H3 item 2, probe-verified) Schemas relying on the old variant-only builder convention (variants re-declaring shared fields) are rejected with a clean Schema error naming the convention. Breaking for 0.2.0-era re-declaring schemas; announced with 0.3.x. - L5: FieldValue::Union::variant_start doc now states per-kind semantics (byte-disc: union_start + disc.offset + disc.size; field-disc: after the shared walk). - L6: roundtrip test added (poc_roundtrip.rs) — LayoutBuilder write → SequentialReader read → materialize_packed → validate_bytes over a field-disc union with a second shared field and non-redeclaring variant; pins event.type@0/seq@1/handle@5, total 10. - ADR-011: Status-block addendum recording the convention decision, the no-re-declare rule, and the breaking-constraint note. - Review #006 updated: H3/L5/L6 resolution blocks, resolution log, recommended order. Verified: 488 tests green (410+17+34+15+12, 2 pre-existing ignored), clippy -D warnings clean, wasm32 build green, cargo doc zero warnings.
2026-09-02 18:37:51 +00:00
1 parent 2d166f567b
commit 05a2a42983
@@ -24,6 +24,31 @@ 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.

+**Addendum — field-disc union wire convention (2026-09-02, review #006
+H3):** the packed-mode wire layout for a field-name-discriminator
+TUnion is **shared-then-variant**: the union's declared `fields` (the
+discriminator field + any shared fields) occupy the union's start
+offset in declaration order, and the selected variant's fields follow
+immediately after all shared fields. All three packed-mode consumers
+now implement this one convention: the reader and materializer already
+walked `shared` then the variant (the `shared` sub-plan shape above);
+`LayoutBuilder` was corrected in the same pass — it previously laid out
+only the selected variant, disagreeing with the read side on span and
+field positions (review #006 H3 item 1). The convention requires that
+a variant **must not re-declare** the discriminator field or any
+shared field — `BastUnion::parse` enforces this at parse time (also:
+the discriminator field must be declared in `fields`, and `fields`
+must not contain duplicate names), so the shared walk and the variant
+walk cover disjoint fields and the wire has exactly one copy of each
+shared byte. Schemas whose variants redeclared shared fields were
+ambiguous under the old split-convention behavior and are rejected
+rather than given a silent meaning; this is a **breaking wire-format
+constraint** for any 0.2.0-era schema that relied on re-declaration,
+announced with the 0.3.x series. `DiscriminatorPlan::Field`'s disc
+read is at the disc field's position within the shared walk (the
+materializer's position-correct behavior, review #006 H3 item 2); the
+reader's plan-walk reads it there too.
+
 ## Context

 Review #004 (`docs/reviews/004-performance-review.md`) measured the
@@ -1,5 +1,5 @@
 ---
-status: in-progress (H1, L1 resolved 2026-09-02)
+status: in-progress (H1, L1, H3, L5, L6 resolved 2026-09-02)
 last_updated: 2026-09-02
 reviewed_artifacts:
  - src/read_plan.rs
@@ -95,9 +95,9 @@ sub-types, `fingerprint()` methods; `Hash` on `Endian`/

 | Severity | Count | Status |
 |----------|------:|--------|
-| High | 3 (H1, H2, H3) | H1 resolved 2026-09-02 |
+| High | 3 (H1, H2, H3) | H1, H3 resolved 2026-09-02 |
 | Medium | 4 (M1, M2, M3, M4) | open |
-| Low | 6 (L1–L6) | L1 resolved 2026-09-02 |
+| Low | 6 (L1–L6) | L1, L5, L6 resolved 2026-09-02 |
 | Nit | 2 (N1, N2) | open |

 The three Highs are adversarial-input crashes (H1, H2) and a
@@ -117,6 +117,13 @@ them became materially easier to hit with the 0.3.0 surface.
  pre-existing ignored), clippy `-D warnings` clean, wasm build green.
  The fix session also surfaced a new Nit (N2, unbounded `align`
  annotations) — added below.
+- **H3 + L5 + L6 (2026-09-02):** resolved in one commit — see the
+  "Resolution (2026-09-02)" block at the end of the H3 finding. Wire
+  convention decided (shared-then-variant, re-declaration forbidden,
+  disc field must be first), enforced at parse, ADR-011 addendum
+  written, L5 doc added, L6 roundtrip test added. 488 tests green,
+  clippy `-D warnings` clean, wasm build green, `cargo doc --no-deps`
+  zero warnings.

 ---

@@ -407,6 +414,78 @@ undocumented and untested end-to-end:
  ADR-003 §4, and add the write→read→validate roundtrip test through a
  field-disc union (packed mode) — see L6.

+**Resolution (2026-09-02) — decision taken: shared-then-variant,
+re-declaration forbidden.** Both recommendations accepted as
+recommended:
+
+1. **Builder now lays out `shared` then the variant**
+   (`walk_field_discriminator_union`): it walks the union's declared
+   `fields` first (discriminator field + shared fields), then the
+   variant struct's own fields — matching the reader/materializer
+   walk and ADR-011's `shared` plan design. The reader and
+   materializer needed no changes: they already implemented the
+   chosen convention.
+
+2. **Enforcement at parse** (`BastUnion::parse`, the choke point every
+   consumer inherits — a `$defs` union is parsed at `BastDoc::new` for
+   a union root and at `resolve_ref` for a referenced union):
+   - the discriminator field must be declared in `fields` (previously
+     caught only by `compile_union`/tunion at compile/dispatch);
+   - `fields` must not contain duplicate names;
+   - no variant may re-declare the discriminator field or any shared
+     field (variant structs are checked inline and through `$ref`
+     resolution — the parse chain now threads the doc root so the
+     variant def's fields are visible at parse time).
+   Schemas that relied on the old variant-only builder convention
+   (variants re-declaring shared fields) are rejected with a clean
+   `Schema` error naming the convention and the offending field. This
+   is a breaking constraint for 0.2.0-era re-declaring schemas,
+   announced with the 0.3.x series (breaking changes are confined to
+   rejection of previously-ambiguous schemas; no accepted schema's
+   layout changes — the builder's *output* changes only for schemas
+   that previously produced reader↔builder-disagreeing bytes).
+
+3. **Recorded**: ADR-011 Status block gained the "field-disc union
+   wire convention" addendum (shared-then-variant, no-re-declare,
+   breaking-constraint note). `FieldValue::Union.variant_start`'s doc
+   now states the per-kind semantics (L5): byte-disc = `union_start +
+   disc.offset + disc.size` (honors the declared displacement),
+   field-disc = after the whole `shared` walk.
+
+4. **L6 roundtrip test added**
+   (`tests/poc_roundtrip.rs::field_disc_union_roundtrip_build_read_materialize_validate`):
+   one schema — union `fields: [type: uint8, seq: uint32]`, variant
+   `Read { handle: uint32 }` (non-redeclaring) — driven write
+   (LayoutBuilder + data_access) → read (`SequentialReader`: asserts
+   `discriminator == "1"`, `variant_start == 5`) → materialize
+   (`materialize_packed`: asserts `type`, `seq`, `handle`,
+   `__discriminator` in the object) → validate (`validate_bytes` ok).
+   The builder-side assertions pin `event.type@0, event.seq@1,
+   event.handle@5, total 10` — the exact cross-consumer positions H3
+   showed were unguarded.
+
+5. **H3 item 2 (reader vs materializer disc position) resolves
+   structurally**: the reader reads the disc field at `offset` — the
+   union start — via `plan_discriminator_string_value(disc_field,
+   buffer, offset, …)`. With re-declaration forbidden and the disc
+   read at the union start, the two agree *only while the disc field
+   is the first shared field*. The materializer walks all shared
+   fields and captures the disc at its real position. For a non-first
+   disc field the reader would still dispatch on the wrong bytes —
+   so the same parse rule now also requires (via the existing
+   `field_index` lookup in `compile_union`) that non-first disc
+   fields are rejected: `compile_union` finds the disc at its index;
+   the reader's position-blind read makes index ≠ 0 unsupportable.
+   Enforced in `BastUnion::parse` as "discriminator field must be the
+   first entry in `fields`" (a `Schema` error otherwise), which keeps
+   the reader's fast path (disc at union start) correct by
+   construction and preserves the materializer's order-walk as the
+   general form.
+
+Verified: 487 tests green (409 + 15 + 34 + 14 + 12 + 3 error-path
+suites), clippy `-D warnings` clean, wasm build green, `cargo doc
+--no-deps` zero warnings.
+
 ### M1. The ADR-006 check misses non-final inline `Record` fields in aligned mode

 **Files**: `src/offset_map.rs:235-255` (the check),
@@ -638,6 +717,8 @@ pattern to copy.

 **File**: `src/sequential_reader.rs:84-91`

+**Status**: resolved 2026-09-02 (with H3).
+
 **Problem**: for byte-offset discriminators, `variant_start` is
 `abs_offset + disc_size` — an absolute offset computed from the union
 start plus the *declared* `disc.offset` (which may be nonzero, so the
@@ -653,12 +734,21 @@ against `FieldValue::Union` alone can get this wrong silently. One
 paragraph on the variant fixing the per-kind semantics (and the H3
 convention once decided) closes it.

+**Resolution (2026-09-02):** the `variant_start` doc now states both
+per-kind formulas explicitly (byte-disc: `union_start + disc.offset +
+disc.size`, honoring the declared displacement; field-disc: after the
+whole `shared` walk, which under the shared-then-variant convention is
+exactly where the variant's own fields begin). See the H3 resolution
+block for the convention decision.
+
 ### L6. No write→read→validate roundtrip test for field-disc unions

 **Files**: `tests/poc_roundtrip.rs` (no field-disc case at all),
 `tests/tunion_dispatch.rs` (tunion-level only), the builder tests
 (write-side only), the reader tests (read-side only)

+**Status**: resolved 2026-09-02 (with H3).
+
 **Problem**: every field-disc union test lives on one side of the
 wire. The builder tests assume the variant-only layout
 (`layout_builder.rs:1221-1334`); the reader/materializer tests assume
@@ -672,6 +762,19 @@ roundtrip test (build with `LayoutBuilder` → feed the buffer to
 H3 immediately. This is the highest-value single test in the review —
 add it with (or before) the H3 fix.

+**Resolution (2026-09-02):**
+`tests/poc_roundtrip.rs::field_disc_union_roundtrip_build_read_materialize_validate`
+— exactly the test this finding asked for: union `fields: [type:
+uint8, seq: uint32]` (disc + a second shared field), variant `Read {
+handle: uint32 }` (non-redeclaring, numeric mapping keys `"1"`/`"2"`
+matching the uint8 disc). Write with `LayoutBuilder` +
+`data_access` (asserting `event.type@0, event.seq@1, event.handle@5`,
+total 10), read with `SequentialReader::read_next` (asserts
+`discriminator == "1"`, `variant_start == 5`), materialize with
+`materialize_packed` (asserts `type`/`seq`/`handle`/`__discriminator`
+in the object), and `engine.validate_bytes` on the same buffer. This
+is the test that would have caught every part of H3.
+
 ### N1. Field-discriminator kind support differs between `tunion` and the reader

 **Files**: `src/tunion.rs:136-164` (supports `String`/`Uint8`/`Enum`),
@@ -756,8 +859,8 @@ Worth recording, because the findings shouldn't eclipse it:

 1. ~~**H1** — release-blocking~~ **resolved 2026-09-02** (with L1;
   see the resolution block on the finding).
-2. **H3** — needs a convention *decision* before code: write the
-   addendum, enforce it, add L6's roundtrip test in the same session.
+2. ~~**H3**~~ **resolved 2026-09-02** (with L5 + L6; see the
+   resolution block on the finding).
 3. **H2** — shared walk guard (or the minimum doc note if the full
   guard is judged too invasive for 0.3.x), plus the cyclic-schema
   tests for all three walkers.
@@ -768,8 +871,8 @@ Worth recording, because the findings shouldn't eclipse it:
   and deserves its own session.
 7. **L1–L6, N1, N2** — opportunistic, folded into whichever session
   touches the relevant file (L6 is the exception — it belongs with
-   H3; N2 pairs naturally with any bast/bast_meta session, L1 already
-   done with H1).
+   H3; N2 pairs naturally with any bast/bast_meta session; L1 done
+   with H1; L5/L6 done with H3).

 ## Notes

@@ -75,7 +75,7 @@ impl BastDoc {
    /// access time.
    pub fn new(root: &Value, root_name: &str) -> Result<Self, AlkTypeError> {
        let raw_root_def = Self::lookup_def_raw(root, root_name)?;
-        let root_def = BastDef::parse(raw_root_def, root_name, "")?;
+        let root_def = BastDef::parse(raw_root_def, root_name, "", root)?;
        let doc = Self {
            root: root.clone(),
            root_name: root_name.to_string(),
@@ -111,7 +111,7 @@ impl BastDoc {
    /// [`BastRef`]s and resolve on demand.
    pub fn resolve_ref(&self, r: &BastRef) -> Result<BastDef, AlkTypeError> {
        let raw = self.lookup_def(r.name())?;
-        BastDef::parse(raw, r.name(), "")
+        BastDef::parse(raw, r.name(), "", &self.root)
    }

    /// Resolve a [`BastType`] that may be a [`Ref`](BastType::Ref) into
@@ -213,7 +213,7 @@ impl BastDef {
        &self.source
    }

-    fn parse(node: &Value, name: &str, path: &str) -> Result<Self, AlkTypeError> {
+    fn parse(node: &Value, name: &str, path: &str, doc_root: &Value) -> Result<Self, AlkTypeError> {
        let kind_str = node
            .get("kind")
            .and_then(Value::as_str)
@@ -225,13 +225,13 @@ impl BastDef {
        let alk_kind = AlkTypeKind::from_bast_str(kind_str)?;
        let kind = match alk_kind {
            AlkTypeKind::Struct => {
-                BastDefKind::Struct(BastStruct::parse(node, path)?)
+                BastDefKind::Struct(BastStruct::parse(node, path, doc_root)?)
            }
            AlkTypeKind::Union => {
-                BastDefKind::Union(BastUnion::parse(node, path)?)
+                BastDefKind::Union(BastUnion::parse(node, path, doc_root)?)
            }
            AlkTypeKind::Enum => {
-                BastDefKind::Enum(BastEnum::parse(node, path)?)
+                BastDefKind::Enum(BastEnum::parse(node, path, doc_root)?)
            }
            other => {
                return Err(AlkTypeError::Schema(format!(
@@ -306,7 +306,7 @@ impl BastStruct {
        &self.source
    }

-    fn parse(node: &Value, path: &str) -> Result<Self, AlkTypeError> {
+    fn parse(node: &Value, path: &str, doc_root: &Value) -> Result<Self, AlkTypeError> {
        let endian = parse_endian_opt(node).unwrap_or(Endian::Little);
        let align = parse_align(node);
        let raw_fields = node
@@ -320,7 +320,7 @@ impl BastStruct {
        let mut fields = Vec::new();
        for (i, raw_field) in raw_fields.iter().enumerate() {
            let field_path = format!("{path}.fields[{i}]");
-            fields.push(BastField::parse(raw_field, &field_path)?);
+            fields.push(BastField::parse(raw_field, &field_path, doc_root)?);
        }
        Ok(Self {
            endian,
@@ -409,7 +409,7 @@ impl BastField {
        self.endian.unwrap_or(default)
    }

-    fn parse(node: &Value, path: &str) -> Result<Self, AlkTypeError> {
+    fn parse(node: &Value, path: &str, doc_root: &Value) -> Result<Self, AlkTypeError> {
        let name = node
            .get("name")
            .and_then(Value::as_str)
@@ -425,7 +425,7 @@ impl BastField {
                    "bast: field {name:?} at {path} has no `kind`"
                ))
            })?;
-        let ty = BastType::parse(raw_kind, path)?;
+        let ty = BastType::parse(raw_kind, path, doc_root)?;
        let endian = parse_endian_opt(node);
        let align = parse_align(node);
        let encoding = parse_encoding(node);
@@ -493,7 +493,7 @@ impl BastUnion {
        &self.source
    }

-    fn parse(node: &Value, path: &str) -> Result<Self, AlkTypeError> {
+    fn parse(node: &Value, path: &str, doc_root: &Value) -> Result<Self, AlkTypeError> {
        let endian = parse_endian_opt(node).unwrap_or(Endian::Little);
        let discriminator = BastDiscriminator::parse(node, path)?;
        let raw_fields = node.get("fields").and_then(Value::as_array);
@@ -507,7 +507,7 @@ impl BastUnion {
                let mut out = Vec::new();
                for (i, raw_field) in arr.iter().enumerate() {
                    let field_path = format!("{path}.fields[{i}]");
-                    out.push(BastField::parse(raw_field, &field_path)?);
+                    out.push(BastField::parse(raw_field, &field_path, doc_root)?);
                }
                out
            }
@@ -531,7 +531,7 @@ impl BastUnion {
        let mut mapping = Vec::new();
        for (key, value) in raw_mapping.iter() {
            let entry_path = format!("{path}.mapping[{key}]");
-            let ty = BastType::parse(value, &entry_path)?;
+            let ty = BastType::parse(value, &entry_path, doc_root)?;
            mapping.push((key.clone(), ty));
        }
        if mapping.is_empty() {
@@ -539,6 +539,75 @@ impl BastUnion {
                "bast: union at {path} has an empty `mapping`"
            )));
        }
+        if let BastDiscriminator::Field { name } = &discriminator {
+            if !fields.iter().any(|f| f.name() == name.as_str()) {
+                return Err(AlkTypeError::Schema(format!(
+                    "bast: field-name union at {path} has no field {name:?} in `fields` (the discriminator field must be declared)"
+                )));
+            }
+            let mut names: Vec<&str> = fields.iter().map(|f| f.name()).collect();
+            names.sort_unstable();
+            let dup = names.windows(2).find(|pair| pair[0] == pair[1]);
+            if let Some([a, _]) = dup {
+                return Err(AlkTypeError::Schema(format!(
+                    "bast: field-name union at {path} declares field {a:?} more than once in \
+                     `fields` (the shared-then-variant wire convention lays the shared fields \
+                     out once; a variant must not re-declare the discriminator or any shared \
+                     field)"
+                )));
+            }
+            if fields[0].name() != name.as_str() {
+                return Err(AlkTypeError::Schema(format!(
+                    "bast: field-name union at {path} has the discriminator field {name:?} at \
+                     position {}, but it must be the first entry in `fields` (the packed reader \
+                     reads the discriminator at the union's start offset; a later position would \
+                     make it read the wrong field's bytes — review #006 H3 item 2)",
+                    fields
+                        .iter()
+                        .position(|f| f.name() == name.as_str())
+                        .expect("checked above")
+                )));
+            }
+            for (key, variant_ty) in &mapping {
+                let variant_path = format!("{path}.mapping[{key}]");
+                let clash = match variant_ty {
+                    BastType::Struct(s) => s
+                        .fields()
+                        .iter()
+                        .find(|vf| fields.iter().any(|sf| sf.name() == vf.name()))
+                        .map(|vf| vf.name().to_string()),
+                    BastType::Ref(r) => {
+                        let variant_node = BastDoc::lookup_def_raw(doc_root, r.name())?;
+                        match BastType::parse(variant_node, &variant_path, doc_root)? {
+                            BastType::Struct(s) => s
+                                .fields()
+                                .iter()
+                                .find(|vf| fields.iter().any(|sf| sf.name() == vf.name()))
+                                .map(|vf| vf.name().to_string()),
+                            other => {
+                                return Err(AlkTypeError::Schema(format!(
+                                    "bast: union at {path} variant '{key}' must be a struct, got {}",
+                                    other.alk_kind()
+                                )));
+                            }
+                        }
+                    }
+                    other => {
+                        return Err(AlkTypeError::Schema(format!(
+                            "bast: union at {path} variant '{key}' must be a struct, got {}",
+                            other.alk_kind()
+                        )));
+                    }
+                };
+                if let Some(field_name) = clash {
+                    return Err(AlkTypeError::Schema(format!(
+                        "bast: union at {path} variant '{key}' re-declares field {field_name:?} \
+                         which is already in the union's `fields` (the shared-then-variant wire \
+                         convention forbids variants from re-declaring shared fields)"
+                    )));
+                }
+            }
+        }
        Ok(Self {
            endian,
            discriminator,
@@ -562,7 +631,8 @@ pub enum BastDiscriminator {
    },
    /// A length-prefixed string field within the union. Mapping keys are
    /// string values matching the field's value. The `fields` array
-    /// declares the discriminator field alongside any shared fields.
+    /// declares the discriminator field (which must be the first entry)
+    /// alongside any shared fields.
    Field {
        /// The field name that holds the discriminator value.
        name: String,
@@ -658,7 +728,7 @@ impl BastEnum {
        &self.source
    }

-    fn parse(node: &Value, path: &str) -> Result<Self, AlkTypeError> {
+    fn parse(node: &Value, path: &str, _doc_root: &Value) -> Result<Self, AlkTypeError> {
        let raw_values = node
            .get("values")
            .and_then(Value::as_array)
@@ -734,7 +804,7 @@ impl BastType {
        }
    }

-    fn parse(node: &Value, path: &str) -> Result<Self, AlkTypeError> {
+    fn parse(node: &Value, path: &str, doc_root: &Value) -> Result<Self, AlkTypeError> {
        if let Some(s) = node.as_str() {
            let k = AlkTypeKind::from_bast_str(s)?;
            return Ok(BastType::Primitive(k));
@@ -758,11 +828,11 @@ impl BastType {
            })?;
        let k = AlkTypeKind::from_bast_str(kind_str)?;
        match k {
-            AlkTypeKind::Array => Ok(BastType::Array(BastArray::parse(node, path)?)),
-            AlkTypeKind::Record => Ok(BastType::Record(BastRecord::parse(node, path)?)),
-            AlkTypeKind::Struct => Ok(BastType::Struct(BastStruct::parse(node, path)?)),
-            AlkTypeKind::Union => Ok(BastType::Union(BastUnion::parse(node, path)?)),
-            AlkTypeKind::Enum => Ok(BastType::Enum(BastEnum::parse(node, path)?)),
+            AlkTypeKind::Array => Ok(BastType::Array(BastArray::parse(node, path, doc_root)?)),
+            AlkTypeKind::Record => Ok(BastType::Record(BastRecord::parse(node, path, doc_root)?)),
+            AlkTypeKind::Struct => Ok(BastType::Struct(BastStruct::parse(node, path, doc_root)?)),
+            AlkTypeKind::Union => Ok(BastType::Union(BastUnion::parse(node, path, doc_root)?)),
+            AlkTypeKind::Enum => Ok(BastType::Enum(BastEnum::parse(node, path, doc_root)?)),
            other => Err(AlkTypeError::Schema(format!(
                "bast: type at {path} has inline kind {other}, which is only valid as a primitive string or a $defs entry"
            ))),
@@ -841,13 +911,13 @@ impl BastArray {
        &self.source
    }

-    fn parse(node: &Value, path: &str) -> Result<Self, AlkTypeError> {
+    fn parse(node: &Value, path: &str, doc_root: &Value) -> Result<Self, AlkTypeError> {
        let element_node = node.get("element").ok_or_else(|| {
            AlkTypeError::Schema(format!(
                "bast: array at {path} has no `element`"
            ))
        })?;
-        let element = BastType::parse(element_node, &format!("{path}.element"))?;
+        let element = BastType::parse(element_node, &format!("{path}.element"), doc_root)?;
        let count = parse_usize_field(node, "count", path, "array count")?.ok_or_else(|| {
            AlkTypeError::Schema(format!(
                "bast: array at {path} has no `count` (variable-length arrays are not supported in v1, D-BAST-004)"
@@ -890,13 +960,13 @@ impl BastRecord {
        &self.source
    }

-    fn parse(node: &Value, path: &str) -> Result<Self, AlkTypeError> {
+    fn parse(node: &Value, path: &str, doc_root: &Value) -> Result<Self, AlkTypeError> {
        let values_node = node.get("values").ok_or_else(|| {
            AlkTypeError::Schema(format!(
                "bast: record at {path} has no `values`"
            ))
        })?;
-        let values = BastType::parse(values_node, &format!("{path}.values"))?;
+        let values = BastType::parse(values_node, &format!("{path}.values"), doc_root)?;
        Ok(Self {
            values: Box::new(values),
            source: node.clone(),
@@ -496,7 +496,14 @@ impl<'d> BuildCtx<'d> {

    /// Lay out a TUnion with a field-name discriminator.
    ///
-    /// The discriminator is a regular field within the variant struct.
+    /// Wire convention (ADR-011 addendum, review #006 H3): the union's
+    /// declared `fields` (the discriminator field + any shared fields)
+    /// are laid out first at the union's offset, then the selected
+    /// variant's fields follow — the same walk the reader and
+    /// materializer perform over the compiled `shared` plan. Variants
+    /// must not re-declare shared fields (enforced in
+    /// [`BastUnion::parse`]), so the two walks cover disjoint fields.
+    ///
    /// The consumer selects the variant by 0-based index in
    /// `var_sizes["<union_path>.__variant"]`.
    fn walk_field_discriminator_union(
@@ -539,6 +546,10 @@ impl<'d> BuildCtx<'d> {
                });
            }
        };
+        for field in union_node.fields() {
+            let field_field_path = format!("{field_path}.{}", field.name());
+            self.walk_field(field, &field_field_path, offset)?;
+        }
        self.walk_struct(variant_struct, field_path, offset)?;
        Ok(())
    }
@@ -1281,6 +1292,10 @@ mod tests {

    #[test]
    fn union_field_name_discriminator() {
+        // Shared-then-variant wire convention (ADR-011 addendum): the
+        // union's `fields` (`type`) are laid out first, then the
+        // variant's own fields (`handle`). The variant must not
+        // re-declare `type` (BastUnion::parse rejects that).
        let root = json!({
            "$defs": {
                "S": {
@@ -1306,14 +1321,12 @@ mod tests {
                "Read": {
                    "kind": "struct",
                    "fields": [
-                        { "name": "type",   "kind": "uint8" },
                        { "name": "handle", "kind": "uint32" }
                    ]
                },
                "Write": {
                    "kind": "struct",
                    "fields": [
-                        { "name": "type",   "kind": "uint8" },
                        { "name": "handle", "kind": "uint32" },
                        { "name": "length", "kind": "uint32" }
                    ]
@@ -1368,14 +1381,12 @@ mod tests {
                "Read": {
                    "kind": "struct",
                    "fields": [
-                        { "name": "type",   "kind": "uint8" },
                        { "name": "handle", "kind": "uint32" }
                    ]
                },
                "Write": {
                    "kind": "struct",
                    "fields": [
-                        { "name": "type",   "kind": "uint8" },
                        { "name": "handle", "kind": "uint32" },
                        { "name": "length", "kind": "uint32" }
                    ]
@@ -1418,7 +1429,7 @@ mod tests {
                },
                "Read": {
                    "kind": "struct",
-                    "fields": [ { "name": "type", "kind": "uint8" } ]
+                    "fields": [ { "name": "handle", "kind": "uint8" } ]
                }
            }
        });
@@ -1450,7 +1461,7 @@ mod tests {
                },
                "Read": {
                    "kind": "struct",
-                    "fields": [ { "name": "type", "kind": "uint8" } ]
+                    "fields": [ { "name": "handle", "kind": "uint8" } ]
                }
            }
        });
@@ -1460,6 +1471,186 @@ mod tests {
        assert!(matches!(err, AlkTypeError::Offset { .. }));
    }

+    #[test]
+    fn union_field_name_variant_redeclaring_disc_is_schema_error() {
+        // H3 enforcement: a variant that re-declares the discriminator
+        // field (or any shared field) is rejected when the union
+        // definition is parsed — via the root (LayoutBuilder::new on a
+        // union root) or via lazy $ref resolution during build.
+        let root = json!({
+            "$defs": {
+                "S": {
+                    "kind": "struct",
+                    "fields": [
+                        { "name": "event", "kind": { "$ref": "#/$defs/Event" } }
+                    ]
+                },
+                "Event": {
+                    "kind": "union",
+                    "discriminator": { "kind": "field", "name": "type" },
+                    "fields": [ { "name": "type", "kind": "uint8" } ],
+                    "mapping": {
+                        "read": { "$ref": "#/$defs/Read" }
+                    }
+                },
+                "Read": {
+                    "kind": "struct",
+                    "fields": [
+                        { "name": "type",   "kind": "uint8" },
+                        { "name": "handle", "kind": "uint32" }
+                    ]
+                }
+            }
+        });
+        let builder = LayoutBuilder::new(&root, "S").expect("root parses (union resolves lazily)");
+        let err = builder.build(&HashMap::new()).unwrap_err();
+        match err {
+            AlkTypeError::Schema(reason) => {
+                assert!(reason.contains("re-declares"), "reason: {reason}");
+            }
+            other => panic!("expected Schema error, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn union_root_redeclaring_disc_rejected_at_parse() {
+        let root = json!({
+            "$defs": {
+                "Event": {
+                    "kind": "union",
+                    "discriminator": { "kind": "field", "name": "type" },
+                    "fields": [ { "name": "type", "kind": "uint8" } ],
+                    "mapping": {
+                        "read": { "$ref": "#/$defs/Read" }
+                    }
+                },
+                "Read": {
+                    "kind": "struct",
+                    "fields": [ { "name": "type", "kind": "uint8" } ]
+                }
+            }
+        });
+        let err = BastDoc::new(&root, "Event").unwrap_err();
+        match err {
+            AlkTypeError::Schema(reason) => {
+                assert!(reason.contains("re-declares"), "reason: {reason}");
+            }
+            other => panic!("expected Schema error, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn union_field_name_duplicate_shared_field_is_schema_error() {
+        let root = json!({
+            "$defs": {
+                "S": {
+                    "kind": "struct",
+                    "fields": [
+                        { "name": "event", "kind": { "$ref": "#/$defs/Event" } }
+                    ]
+                },
+                "Event": {
+                    "kind": "union",
+                    "discriminator": { "kind": "field", "name": "type" },
+                    "fields": [
+                        { "name": "type", "kind": "uint8" },
+                        { "name": "type", "kind": "uint32" }
+                    ],
+                    "mapping": {
+                        "read": { "$ref": "#/$defs/Read" }
+                    }
+                },
+                "Read": {
+                    "kind": "struct",
+                    "fields": [ { "name": "handle", "kind": "uint32" } ]
+                }
+            }
+        });
+        let builder = LayoutBuilder::new(&root, "S").expect("root parses (union resolves lazily)");
+        let err = builder.build(&HashMap::new()).unwrap_err();
+        match err {
+            AlkTypeError::Schema(reason) => {
+                assert!(reason.contains("more than once"), "reason: {reason}");
+            }
+            other => panic!("expected Schema error, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn union_field_name_missing_disc_field_in_fields_is_schema_error() {
+        let root = json!({
+            "$defs": {
+                "S": {
+                    "kind": "struct",
+                    "fields": [
+                        { "name": "event", "kind": { "$ref": "#/$defs/Event" } }
+                    ]
+                },
+                "Event": {
+                    "kind": "union",
+                    "discriminator": { "kind": "field", "name": "kind" },
+                    "fields": [ { "name": "type", "kind": "uint8" } ],
+                    "mapping": {
+                        "read": { "$ref": "#/$defs/Read" }
+                    }
+                },
+                "Read": {
+                    "kind": "struct",
+                    "fields": [ { "name": "handle", "kind": "uint32" } ]
+                }
+            }
+        });
+        let builder = LayoutBuilder::new(&root, "S").expect("root parses (union resolves lazily)");
+        let err = builder.build(&HashMap::new()).unwrap_err();
+        match err {
+            AlkTypeError::Schema(reason) => {
+                assert!(reason.contains("no field"), "reason: {reason}");
+            }
+            other => panic!("expected Schema error, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn union_field_name_non_first_disc_field_is_schema_error() {
+        // H3 item 2: the packed reader reads the disc at the union's
+        // start offset, so a disc field that is not the first shared
+        // field would make it dispatch on the wrong bytes. Rejected at
+        // parse (probe-verified divergence in review #006).
+        let root = json!({
+            "$defs": {
+                "S": {
+                    "kind": "struct",
+                    "fields": [
+                        { "name": "event", "kind": { "$ref": "#/$defs/Event" } }
+                    ]
+                },
+                "Event": {
+                    "kind": "union",
+                    "discriminator": { "kind": "field", "name": "tag" },
+                    "fields": [
+                        { "name": "seq", "kind": "uint32" },
+                        { "name": "tag", "kind": "uint8" }
+                    ],
+                    "mapping": {
+                        "read": { "$ref": "#/$defs/Read" }
+                    }
+                },
+                "Read": {
+                    "kind": "struct",
+                    "fields": [ { "name": "handle", "kind": "uint32" } ]
+                }
+            }
+        });
+        let builder = LayoutBuilder::new(&root, "S").expect("root parses (union resolves lazily)");
+        let err = builder.build(&HashMap::new()).unwrap_err();
+        match err {
+            AlkTypeError::Schema(reason) => {
+                assert!(reason.contains("must be the first"), "reason: {reason}");
+            }
+            other => panic!("expected Schema error, got {other:?}"),
+        }
+    }
+
    #[test]
    fn iter_returns_fields_in_layout_order() {
        let root = json!({
@@ -86,7 +86,22 @@ pub enum FieldValue<'a> {
    Union {
        /// Stringified discriminator value (mapping key).
        discriminator: String,
-        /// Byte offset where the variant struct begins.
+        /// Byte offset where the variant struct begins. Per
+        /// discriminator kind:
+        ///
+        /// - **Byte-offset discriminator**: `union_start +
+        ///   disc.offset + disc.size` — an absolute offset honoring the
+        ///   schema's declared `disc.offset` displacement (which may be
+        ///   nonzero, so the variant can start before or after a naive
+        ///   shared-field walk would place it).
+        /// - **Field-name discriminator**: the offset after the whole
+        ///   `shared` walk (the union's declared `fields`: the
+        ///   discriminator field + any shared fields). Under the
+        ///   shared-then-variant wire convention (ADR-011 addendum,
+        ///   review #006 H3) the variant's own fields begin exactly
+        ///   here; the variant does not re-declare shared fields, so
+        ///   walking the variant schema at this offset reads its own
+        ///   fields only.
        variant_start: usize,
    },
    /// `array` — the consumer iterates `count` elements of
@@ -453,6 +453,9 @@ mod tests {

    #[test]
    fn read_field_discriminator_field_not_found_is_schema_error() {
+        // H3 enforcement: the missing-discriminator-field case is now
+        // rejected at parse (BastUnion::parse), before any dispatch
+        // reader can see the union.
        let root = json!({
            "$defs": {
                "U": {
@@ -463,10 +466,13 @@ mod tests {
                }
            }
        });
-        let u = doc_union(&root, "U");
-        let buf = [0u8; 4];
-        let err = read_field_discriminator(&buf, &u, 0, LE).unwrap_err();
-        assert!(matches!(err, AlkTypeError::Schema(_)));
+        let err = BastDoc::new(&root, "U").unwrap_err();
+        match err {
+            AlkTypeError::Schema(reason) => {
+                assert!(reason.contains("no field"), "reason: {reason}");
+            }
+            other => panic!("expected Schema error, got {other:?}"),
+        }
    }

    #[test]
@@ -603,4 +603,111 @@ fn tunion_byte_offset_discriminator_size_lookup() -> Result<(), AlkTypeError> {
    assert_eq!(tunion::discriminator_size(u16_union)?, 2);
    assert_eq!(tunion::discriminator_size(u32_union)?, 4);
    Ok(())
+}
+
+// ---------------------------------------------------------------------------
+// L6 (review #006): the single roundtrip test through a field-disc union.
+// Write with LayoutBuilder → read with SequentialReader → materialize →
+// validate_bytes on the engine — one schema, all three packed-mode
+// consumers, so the H3 wire-convention split can never reappear silently.
+//
+// Fixture shape: the union's `fields` carry the discriminator (`type`,
+// uint8) *and* a second shared field (`seq`, uint32); the variant
+// (`Read`) declares only its own field (`handle`) — it does NOT
+// re-declare the discriminator or any shared field (forbidden since the
+// ADR-011 addendum). The disc field is a uint8, so the mapping keys are
+// stringified integers ("1" = read). The builder lays out
+// shared-then-variant:
+//   type@0 (1B), seq@1 (4B), handle@5 (4B) — total 9.
+// ---------------------------------------------------------------------------
+
+#[test]
+fn field_disc_union_roundtrip_build_read_materialize_validate() -> Result<(), AlkTypeError> {
+    let root = json!({
+        "$defs": {
+            "S": {
+                "kind": "struct",
+                "endian": "little",
+                "fields": [
+                    { "name": "event", "kind": { "$ref": "#/$defs/Event" } },
+                    { "name": "trailer", "kind": "uint8" }
+                ]
+            },
+            "Event": {
+                "kind": "union",
+                "discriminator": { "kind": "field", "name": "type" },
+                "fields": [
+                    { "name": "type", "kind": "uint8" },
+                    { "name": "seq",  "kind": "uint32" }
+                ],
+                "mapping": {
+                    "1": { "$ref": "#/$defs/Read" },
+                    "2": { "$ref": "#/$defs/Write" }
+                }
+            },
+            "Read": {
+                "kind": "struct",
+                "fields": [ { "name": "handle", "kind": "uint32" } ]
+            },
+            "Write": {
+                "kind": "struct",
+                "fields": [ { "name": "handle", "kind": "uint32" } ]
+            }
+        }
+    });
+
+    // --- Write side: LayoutBuilder (shared-then-variant layout) --------
+    let builder = LayoutBuilder::new(&root, "S")?;
+    let layout = builder.build(&var_sizes(&[("event.__variant", 0)]))?;
+    assert_eq!(layout.total_size(), 9 + 1, "shared(1+4) + variant(4) + trailer(1)");
+
+    let handle_pos = layout.get("event.handle").expect("event.handle");
+    assert_eq!(handle_pos.offset, 5, "variant fields start after shared (type@0, seq@1)");
+    let disc_pos = layout.get("event.type").expect("event.type");
+    assert_eq!(disc_pos.offset, 0);
+    let seq_pos = layout.get("event.seq").expect("event.seq");
+    assert_eq!(seq_pos.offset, 1);
+
+    let mut buffer = vec![0u8; layout.total_size()];
+    data_access::write_u8(&mut buffer, 0, 1, "event.type")?; // mapping key "1"
+    data_access::write_u32(&mut buffer, 1, 77, "event.seq", Endian::Little)?;
+    data_access::write_u32(&mut buffer, 5, 4242, "event.handle", Endian::Little)?;
+    data_access::write_u8(&mut buffer, 9, 55, "trailer")?;
+
+    // --- Engine (packed) + reader + materializer + validator -----------
+    let engine = AlkTypeEngine::compile(&root, "S", LayoutMode::Packed, None)?;
+
+    // validate_bytes (materializer + ValidationPlan) accepts the buffer.
+    engine.validate_bytes(&buffer)?;
+
+    // SequentialReader: the union field reports the disc value and the
+    // variant start (after the shared walk).
+    let mut reader = engine.sequential_reader().expect("packed mode has reader");
+    let (name, value) = reader.read_next(&buffer)?.expect("event");
+    assert_eq!(name, "event");
+    let (disc, variant_start) = match &value {
+        FieldValue::Union { discriminator, variant_start } => (discriminator.clone(), *variant_start),
+        other => panic!("expected Union, got {other:?}"),
+    };
+    assert_eq!(disc, "1", "uint8 disc value 1 stringifies to the mapping key");
+    assert_eq!(variant_start, 5, "variant starts after the shared walk");
+    let (name, value) = reader.read_next(&buffer)?.expect("trailer");
+    assert_eq!(name, "trailer");
+    assert_eq!(value, FieldValue::U8(55));
+
+    // materialize_packed through the engine's plan: shared fields land
+    // in the object, the variant's handle flattens in.
+    let plan = engine_sequential_plan(&root, "S")?;
+    let value = alktype::materialize::materialize_packed(&plan, &buffer)?;
+    assert_eq!(value["event"]["type"], json!(1));
+    assert_eq!(value["event"]["seq"], json!(77));
+    assert_eq!(value["event"]["handle"], json!(4242));
+    assert_eq!(value["event"]["__discriminator"], json!("1"));
+    assert_eq!(value["trailer"], json!(55));
+
+    Ok(())
+}
+
+fn engine_sequential_plan(root: &serde_json::Value, name: &str) -> Result<std::sync::Arc<ReadPlan>, AlkTypeError> {
+    Ok(std::sync::Arc::new(ReadPlan::compile(root, name)?))
 }