--- status: open 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 - docs/architecture/decisions/012-plan-fingerprinting-and-m1-closure.md - docs/reviews/004-performance-review.md - src/lib.rs - src/bast.rs - src/engine.rs - src/sequential_reader.rs - src/materialize.rs - src/offset_map.rs - src/layout_builder.rs - src/schema.rs - poc/readplan/{src/lib.rs, FINDINGS.md} (branch readplan-poc) tool: manual source read + plan-vs-codebase cross-check + POC branch inspection reviewer: 0.3.0 implementation plan review (triggered before phase 1) --- # Review #005 — 0.3.0 Plan Review: Compiled Forms ## Purpose The 0.3.0 implementation plan ([`docs/plans/030-compiled-forms.md`](../plans/030-compiled-forms.md)) is the entry point an implementing agent reads first. It rolls up [ADR-011](../architecture/decisions/011-compiled-read-plan-for-packed-mode.md) (the `ReadPlan` packed read-side compiled form), [ADR-012](../architecture/decisions/012-plan-fingerprinting-and-m1-closure.md) (fingerprinting + owned `BastDoc` + `OffsetMap` `LeafMeta`), and the fingerprinting work into one breaking bump. The plan is deliberately structured as seven phases so each can be picked up by a fresh session without prior context. This review's purpose is to find planning-spec mistakes — factual errors, contradictions, undocumented behavioral changes, hedges into an unplanned future — *before* a phase-by-phase implementation starts, because fresh-session implementations are reliable precisely when the spec is accurate. A spec that contradicts the code or an ADR forces the agent to either reverse-engineer the actual intent or guess, and the failure rate goes up. The review explicitly scans for the "deferral black hole" pattern: a plan or ADR puts work off into a "future version/phase/downstream" with no concrete reactivation condition, the next agent inherits the gap, and the gap festers until something forces an untangle. This is a known LLM-planning quirk distinct from classic planning mistakes, and a default scan for it is part of this review's methodology. ## Methodology - Full read of the plan and its two companion ADRs (011, 012), the performance review (#004) the plan closes, and the POC findings on branch `readplan-poc`. - Cross-check every line-number reference and `src/` claim in the plan against the actual codebase at `main` (commit `2310f6c`, v0.2.0). Verified: `engine.rs:112-115,284,334,467`; `sequential_reader.rs:567`; `layout_builder.rs:190`; `bast.rs:51-55`; `lib.rs` re-export list; `Cargo.toml` version; presence of `poc/` (absent on main, present on `readplan-poc` as expected); presence of `alktty`/`alkcall` downstream path dev-deps. - Cross-check the POC's `ReadPlan`/`CompositePlan` shape against both ADR-011's shape section and the plan's phase-1 description. - Cross-check the plan's phase 2 rewrite claims (`dummy_field_for`/ `ty_source` "are removed") against `materialize.rs`'s actual call sites across both packed and aligned paths. - Verify the derives the plan relies on (`Hash` on `LeafMeta`, `ReadPlan`, `OffsetMap`) are reachable from the derives on their constituent types in `src/schema.rs`. - Scan for the deferral pattern by flagging every "future/deferred/ later/downstream/if needed" occurrence and asking: (a) is there a concrete reactivation trigger? (b) is the decision owned or silent? (c) does inaction have a cost that the deferral framing hides? ## Verification Baseline The plan and both ADRs were read at the tree state at commit `2310f6c` ("Propose ADR-012 + 0.3.0 implementation plan"), which is `main` HEAD. The codebase is v0.2.0 (`Cargo.toml`); the POC lives on branch `readplan-poc` and is not merged, as the plan states. All line-number references in the plan were verified correct against this tree. ## Summary Statistics | Severity | Count | |----------|------:| | High | 2 (H1, H2) | | Medium | 3 (M1, M2, M3) | | Low | 3 (L1, L2, L3) | | Nit | 3 (N1, N2, N3) | The two High findings are correctness/contradiction issues that would block or mislead an implementing agent. The Mediums are either undocumented behavioral drops, missing implementation prerequisites, or a deferral worth re-evaluating. Lows and Nits are wording/typo-level. --- ## Findings ### H1. Phase 1's field-name-discriminator union shape exists in neither ADR-011 nor the POC **File**: `docs/plans/030-compiled-forms.md:144-152` **Problem**: The plan describes the field-name-discriminator union read shape as: > `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. But ADR-011 §"The `ReadPlan` shape" (`011-compiled-read-plan-for-packed-mode.md:144-158`) defines: ```rust pub enum CompositePlan { Struct(ReadPlan), Union { disc: DiscriminatorPlan, variants: Vec<(String, VariantPlan)>, }, ... } ``` There is no field for shared/declared fields on the `Union` variant. The POC (`readplan-poc:poc/readplan/src/lib.rs`) matches the ADR's shape — `CompositePlan::Union { disc, variants }` only — and its `compile_union` does not carry shared fields. The POC's `FINDINGS.md` Finding 1 (the same one the plan cites at lines 143-152) explicitly says: > `plan_read_union`'s `Field` arm is a stub that returns an error. > ... The plan needs a sub-struct for the union's declared fields, > separate from the variant plans. So the plan describes a shape that exists in **neither** the accepted ADR **nor** the reference POC, and presents it as "the production version must implement it" within the existing `CompositePlan::Union` shape. An implementing agent reading ADR-011 + plan + POC gets three different `CompositePlan::Union` shapes and no guidance on where the shared-fields sub-`ReadPlan` goes (a new `shared: Option>` field? a wrapper enum? two-variant split?). This is a shape extension to an accepted ADR's public type. The plan either needs to flag it as an ADR-011 refinement (with the ADR updated first) or specify the concrete shape the agent should build. **Lift**: unblocks phase 1. Without resolution, the agent will either guess the shape and likely diverge from intent, or stop and ask. --- ### H2. `schema()` returning `&Value` from a `&Value` "stored on the plan" is a self-referential struct **File**: `docs/plans/030-compiled-forms.md:188-191` **Problem**: Phase 2 says: > `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). The "the plan stores the `&Value` it was compiled from" is the self-referential struct pattern ADR-011 §"Root cause" (`011-...md:50-58`) explicitly identifies as impossible in safe Rust and rejects. The engine owns `bast_doc: Value` and `Arc`. If `ReadPlan` stores `&Value` borrowing from the engine's `bast_doc`, the engine is self-referential — exactly the construction ADR-007 worked around with "re-parse on demand" and ADR-011's `Arc` was meant to retire. ADR-011 line 204 specifies the plan is "immutable **owned** data"; it does not say the plan stores a `&Value`. `schema()`'s current contract (`src/sequential_reader.rs:247`) is to return the raw BAST `Value` the reader was built from. To preserve that contract on `Arc` without a self-referential borrow, `ReadPlan` must store an `Arc` (engine builds `Arc` at compile time, hands a clone to the plan) or an owned `Value`. Then `schema()` returns `&self.plan.value`. The plan should specify which. **Lift**: prevents an agent from getting stuck in phase 2 trying to make `&Value` in `Arc` work, which the borrow checker will reject. --- ### M1. Nested unions: POC rejects a schema 0.2.0 accepts — undocumented behavioral drop **Files**: `poc/readplan/FINDINGS.md` ("What this POC does not cover"), `docs/plans/030-compiled-forms.md` (silent), `src/sequential_reader.rs:789-815` **Problem**: The POC's `compile_union` rejects a union variant that is itself a union with `AlkTypeError::Schema`. The existing reader supports this: `resolve_and_walk_variant` at `src/sequential_reader.rs:800` has a live `BastDefKind::Union` arm that recurses via `read_union_value`. So 0.2.0 accepts and reads nested-union schemas; phase 1's `ReadPlan::compile` (per the POC the plan cites as the reference scaffold) would reject the same schema. The plan's phase 1 calls out two POC findings explicitly (field-disc union shape → H1 above, struct-array stride → deferred decision 4) and says "the production version must implement/decide these." It does **not** call out the nested-union rejection. An agent following the plan would inherit the POC's reject-nested-unions behavior by default, silently dropping a 0.2.0 capability — a behavioral regression that rides the 0.3.0 bump without being listed in the Semver Contract table. This is also the cleanest example of the deferral-black-hole pattern in the plan: the POC says "if a real schema needs it, the implementation step adds a `VariantKind::Union` read path. Not blocking — no current schema exercises it." The "if needed" framing has no trigger, no OQ, no tracking — it's a black hole. The next agent inherits the gap. **Lift**: either (a) add `VariantKind::Union` read path in phase 1 (small — mirrors the existing `resolve_and_walk_variant` Union arm, ~20 lines), or (b) list it in the Semver Contract table as a behavioral drop with a one-line OQ tracking the deferral. Given the plan says there are zero real consumers, (b) is defensible, but it must be *stated*, not silent. (a) is cheap and avoids the regression. --- ### M2. `Endian` and `VariableEncoding` don't derive `Hash` — phases 5/6 will not compile **Files**: `src/schema.rs:205,212`, `docs/plans/030-compiled-forms.md:62,357,411-415` **Problem**: `src/schema.rs:205` (`Endian`) and `:212` (`VariableEncoding`) both derive only `Debug, Clone, Copy, PartialEq, Eq` — no `Hash`. The plan requires: - Phase 5 (line 62, 357): `LeafMeta { kind, encoding, endian }` as `Copy + PartialEq + Eq + Hash`. - Phase 6 (lines 411-415): `#[derive(Hash, Eq)]` on `ReadPlan`/ `OffsetMap`, and `FieldPlan` carries `endian: Endian` + `encoding: VariableEncoding`. Both derives will fail to compile: `#[derive(Hash)]` on a struct requires all fields to be `Hash`. The plan never mentions adding `Hash` to these two enums. The fix is trivial (both are fieldless enums, already `Eq + PartialEq`, so adding `Hash` is semver-safe — additive, no behavioral change), but it's a prerequisite the plan omits. An agent working phase 5 will hit a compile error and have to diagnose why. **Lift**: trivial. Add a sub-step to phase 5 (or 6): "Add `Hash` to `Endian` and `VariableEncoding` derives in `src/schema.rs`." This is additive and safe to do earlier if convenient. --- ### M3. `ValidationPlan` deferral worth re-evaluating — the read+validate common case **Files**: `docs/architecture/decisions/012-...md:64-73` ("Deferring `ValidationPlan`"), `docs/plans/030-compiled-forms.md:526-528`, `docs/reviews/004-performance-review.md` (the read-path perf review) **Problem**: ADR-012 defers a `ValidationPlan` as "different shape (value-domain, not byte-position), not a hot loop, separate ADR if a bench motivates it." The plan inherits this deferral ("Not a `ValidationPlan`" at lines 526-528). The deferral framing is "if a bench motivates it" — a concrete trigger exists, so this is not a black-hole hedge in the M1 sense. Flagged for re-evaluation, not because the shape argument is wrong (it's correct — value-domain checks are structurally different from byte-position walks), but because the *hot-loop* dismissal may under- account a common case: **read + validate together on untrusted input.** Review #004 found the packed read path was 400x slow per chunk due to per-field `BastDoc` re-parse. ADR-011 closes that. But `validate_bytes`'s packed path (ADR-010) is `materialize_packed` → `bast_validation::validate_value` over the materialized `Value`. After ADR-011, `materialize_packed` walks the `ReadPlan` (fast). `bast_validation::validate_value` still walks `BastDoc` to check value-domain constraints — once per `validate_bytes` call, over the full tree, on every buffer. For a stream of N untrusted buffers (the `alkcall` hub/spoke topology accepts schemas from arbitrary internet peers — AGENTS.md §3 — and the common case is "read incoming frame, validate it before acting"), `validate_bytes` is called N times. Each call does one `BastDoc` walk for validation. After ADR-011, the *read* half of `validate_bytes` is plan-fast; the *validation* half is still a `BastDoc` walk per call. If validation is the common companion to read on untrusted input, then skipping validation is risky (accepting untrusted bytes unchecked) and running it re-walks `BastDoc` per buffer — the same class of cost review #004 measured for the read path, just on a different code path. The argument is not "ValidationPlan has the same shape as ReadPlan" (it doesn't). The argument is: ADR-012's "not a hot loop" dismissal may be incomplete, because read+validate on untrusted streams makes validation hot in the same sense read was hot. The deferral's trigger ("if a bench motivates it") should be sharpened: either (a) add a `validate_bytes`-on-untrusted-stream bench to alktty alongside `wire_vs_bast` and let the bench decide, or (b) reason from the existing review #004 numbers that the validation walk is non-trivial and should be planned, not deferred. This is not a request to implement `ValidationPlan` in 0.3.0. It's a request to *own the decision*: either the trigger fires (and a follow-on ADR/phase is scoped, possibly 0.4.0) or it doesn't (and the deferral stands with a sharper justification than "not a hot loop"). As written, the deferral leaves the cost in the superposition where it can neither be confirmed nor dismissed. **Lift**: removes a latent perf cliff for the read+validate-on- untrusted-input case that 0.3.0 is supposed to make viable. --- ### L1. `dummy_field_for`/`ty_source` are used in aligned `materialize`, not just packed **Files**: `docs/plans/030-compiled-forms.md:209-210`, `src/materialize.rs:249,316,351,391,631,650-663` **Problem**: Phase 2 says: > The `dummy_field_for`/`ty_source` helpers in `materialize.rs` are > removed (the plan carries everything). This is factually wrong. `dummy_field_for` is called at `src/materialize.rs:631` inside `materialize_leaf_at`, which is called by the **aligned** path: `materialize_struct_aligned` (line 475), `materialize_array_aligned` (line 544), `materialize_variable_aligned` (line 613). Aligned `materialize` keeps walking `BastDoc` through 0.3.0 (plan lines 379-385 confirm), so `dummy_field_for`/`ty_source` must stay. Only the packed-side call sites (lines 249, 316, 351, 391) go away when packed-materialize moves to the plan. **Lift**: doc accuracy. An agent following the plan literally would remove the helpers and break aligned `materialize`. --- ### L2. `materialize_packed` rewrite scope underspecified — packed-vs-aligned split of `materialize_typeref_packed` **Files**: `docs/plans/030-compiled-forms.md:207-210`, `src/materialize.rs:122-200, 498-506, 619-637` **Problem**: `materialize_typeref_packed` is shared by both packed and aligned paths — aligned's `materialize_leaf_at` (line 619-637) calls `materialize_typeref_packed` to read leaves, and aligned's record path (line 498-506) calls it directly. Phase 2 says `materialize_packed(&ReadPlan, &[u8])` walks the plan instead of `BastDoc` but does not state what happens to `materialize_typeref_packed`. The honest resolution: packed-materialize gets a new plan-walking function; aligned keeps `materialize_typeref_packed` via `materialize_leaf_at`; the function stays (renamed or not) for aligned. This is two mode-specific paths — the existing design — not a "parallel walker" in the maintenance-tax sense ADR-011 §"Negative" (cautioning against) discusses. ADR-011's "one walker" claim (lines 234-237) is specifically about packed read-side (`SequentialReader` + `materialize_packed` sharing the plan), not packed-vs-aligned, so there's no ADR contradiction — just an underspecification in the plan. **Lift**: prevents the agent from having to discover the split mid-rewrite. Add one line to 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." --- ### L3. `materialize_aligned`'s `BastDoc` structure walk is silent in the plan **Files**: `docs/plans/030-compiled-forms.md` (silent on this), `src/materialize.rs:451-521`, `docs/architecture/decisions/011-...md:264` **Problem**: `materialize_struct_aligned` walks `BastDoc` to traverse struct/array/record structure, using `OffsetMap` only for leaf byte positions. ADR-011 §"Out of scope" says "aligned mode is unchanged; `materialize_aligned` already takes `&OffsetMap`" — which is half true: it takes `&OffsetMap` for positions but also `&BastDoc` for structure. The plan inherits the half-truth silently: there's no statement anywhere that aligned materialize keeps walking `BastDoc` for structure. After phase 3 (owned `BastDoc`) + phase 5 (`LeafMeta`), the walk is over owned data, no re-parse, not O(N²), and aligned `validate_bytes` is one walk per call (not per-field). There's no perf driver analogous to review #004's packed per-chunk gap. But the absence of a driver is not the same as a decision: leaving it silent is a deferral-by-omission. An implementing agent or future reader can't tell whether the silence is "this is the permanent design" or "we'll fix this later." The decision should be owned. Either (a) add a "Scope Boundary" note that aligned materialize keeps walking owned `BastDoc` for structure as the permanent design (with an OQ if a future bench motivates an `AlignedPlan`), or (b) if a bench motivation is plausible, scope an OQ to track it. (a) is recommended — no perf driver, and after phase 3 the walk is over owned data, so it's not the re-parse pattern. **Lift**: removes a silent gap that future agents would otherwise have to reverse-engineer. --- ### N1. Typo: "back-comat" → "back-compat" **File**: `docs/plans/030-compiled-forms.md:104-105` **Problem**: "back-comat" in deferred decision 4. **Lift**: trivial. --- ### N2. Phase 1 verification omits the `Send + Sync` assertion test ADR-011 requires **Files**: `docs/plans/030-compiled-forms.md:158-163`, `docs/architecture/decisions/011-...md:204-206` **Problem**: ADR-011 §"Engine integration" says "the implementation should add a `static` bound assertion test to lock it in" for `ReadPlan: Send + Sync`. Phase 1's verification block lists `cargo test`, `clippy`, `doc`, `wasm` but no mention of adding the assertion test. An agent following the plan literally won't add it; the property is currently true by construction but not asserted, so a future change could break it silently. **Lift**: add "add a `fn read_plan_is_send_sync()` assertion test" to phase 1's verification, mirroring the POC's `readplan_is_send_sync` test. --- ### N3. `SequentialReader::new` return-type change (`Result` drop) undocumented **Files**: `docs/plans/030-compiled-forms.md:64`, `src/sequential_reader.rs:129`, `src/engine.rs:205` **Problem**: Currently `new(&Value, &str) -> Result` — fallible (BastDoc parse). After phase 2, `new(Arc)` is infallible (just stores the Arc) → returns `Self`, not `Result`. The Semver Contract table (line 64) lists only the argument-type change, not the `Result` drop. `engine.rs:205`'s `.ok()` call correspondingly goes away. Minor, but it's a signature change beyond what's listed. **Lift**: add a row to the Semver Contract table noting the `Result` drop. --- ## Deferral-pattern scan (LLM-planning quirk) As part of the methodology, every "future/deferred/later/downstream/if needed" occurrence in the plan and its ADRs was flagged and tested for: (a) concrete reactivation trigger, (b) decision owned or silent, (c) hidden cost of inaction. | Item | Trigger? | Owned? | Cost of inaction | Finding | |---|---|---|---|---| | `ValidationPlan` (ADR-012) | "if a bench motivates it" | Yes (ADR + plan "What this is not") | Possible perf cliff on read+validate untrusted streams | M3 above — sharpen the trigger | | Nested-union `ReadPlan` support | "if a real schema needs it" (POC) | No (POC only, plan silent) | Silent 0.2.0 capability drop | M1 above — state it | | `materialize_aligned` structure walk | None — silent | No (silent) | Future agent ambiguity | L3 above — own the decision | | `Arc` vs `String` (decision 1) | "if phase 4 shows it's measurable" | Yes (deferred decision 1) | None | OK — has trigger, decided in phase 3 | | `OffsetMap::get` shape (decision 2) | "decided in phase 5" | Yes (deferred decision 2) | None | OK | | Fingerprint hasher (decision 3) | "decided in phase 6" | Yes (deferred decision 3) | None | OK | | Struct-array stride (decision 4) | "decided in phase 2" | Yes (deferred decision 4) | None | OK | | `BastDoc` `Arc` vs `Value` | None — silent | No (plan doesn't address) | Agent gets stuck (H2) | H2 above | | Field-disc union shape (POC Finding 1) | "production version must implement" | Yes (plan phase 1) | None, but shape is undefined | H1 above — shape not in ADR | The four explicit "deferred decisions" in the plan (items 4-7) all have concrete triggers and decision points — these are the *good* pattern. The black-hole pattern appears where deferrals lack triggers (items 1-3, 8-9): three of those became findings (M1, L3, H2), and M3 is a deferral worth sharpening even though it has a trigger. The general signal: a deferral is healthy when it has a concrete reactivation condition and is tracked (OQ, ADR, or in-plan deferred decision). A deferral is a black hole when it has no trigger, no tracking, and the next agent inherits the gap by default. --- ## What's Good - **Line-number accuracy is perfect.** Every `src/` reference in the plan (`engine.rs:112-115,284,334,467`; `sequential_reader.rs:567`; `layout_builder.rs:190`; `bast.rs:51-55`; `lib.rs` re-exports) checks out against the v0.2.0 tree. This is unusual for a plan of this length and worth noting. - **The Semver Contract table is a strong scope-creep guardrail.** Walking every public `lib.rs` re-export against the table, the classifications (Breaking / Unchanged / New) are correct for every item, with the exceptions noted in N3 (the `Result` drop on `new`) and M1 (the nested-union behavioral drop not listed). - **The four explicit "deferred decisions" are the right pattern.** Each has a trigger and a decision point in a named phase. This is what deferrals should look like. - **Phases are coherent session boundaries.** Phases 1 (pure addition), 6 (pure addition), 7 (docs/bump) are small and clean. Phases 3 (broad but mechanical), 4 (single file), 5 (single file + engine) are well-scoped. Phase 2 is the largest and the plan sanctions sub-session splits at the step level (lines 40-42), which is the right escape valve. - **Cross-phase invariants are stated and checkable.** "Tree builds and tests pass at every phase boundary" is the right invariant; the POC-on-`readplan-poc`-only convention is clearly separated from production code; AGENTS.md §5-§11 constraints (no `unsafe`, no `async`, no new deps, `preserve_order` load-bearing) are reaffirmed. - **The plan honestly scopes what it is not.** "Not a `ValidationPlan`", "Not cross-version fingerprint stability", "Not a perf bench" — these boundaries are stated rather than left implicit, which helps an implementing agent resist scope creep. (M3 above is about sharpening one of these, not removing the boundary.) - **The POC reference is disciplined.** The plan is explicit that the POC is "not production code," lives only on the branch, and is the reference scaffold for phases 1-2 only. This matches how `bast-validator-poc` was handled and avoids the POC leaking into `main`. --- ## Recommended Order 1. **H1 (field-disc union shape)** — update ADR-011's `CompositePlan::Union` to include the shared-fields sub-`ReadPlan` (or document the wrapper shape), then update the plan's phase 1 to reference the corrected ADR shape. Do this before phase 1 starts; otherwise the implementing agent has to guess. 2. **H2 (`schema()` `&Value` on `Arc`)** — edit the plan's phase 2 to specify `ReadPlan` stores `Arc` (or owned `Value`), and `schema()` borrows from that. One-line edit to the plan; avoid a phase-2 stuck point. 3. **M1 (nested unions)** — decide (a) implement `VariantKind::Union` in phase 1, or (b) list as behavioral drop + OQ. Edit the plan and (if b) the Semver Contract table accordingly. Decide before phase 1. 4. **M2 (`Hash` on `Endian`/`VariableEncoding`)** — add a sub-step to phase 5 or 6. Trivial. 5. **M3 (`ValidationPlan` re-evaluation)** — either add a `validate_bytes`-on-untrusted-stream bench to alktty (alongside `wire_vs_bast`) and let the bench decide, or sharpen ADR-012's "not a hot loop" justification. Does not block 0.3.0; can be resolved in parallel with phase 1-7 work. **Flagged for re-evaluation, not for implementation in 0.3.0.** 6. **L1, L2, L3** — edit the plan's phase 2 to fix the `dummy_field_for` wording (L1), state the packed-vs-aligned materialize split (L2), and add a Scope Boundary note for aligned-materialize's `BastDoc` structure walk (L3). All three are phase-2 doc edits. 7. **N1, N2, N3** — typo, `Send + Sync` assertion test, `Result`-drop Semver row. Minor plan edits. Items 1-3 must be resolved before phase 1 starts (they affect the `ReadPlan` shape or 0.2.0 behavioral surface). Items 4-7 can be resolved any time before their phase begins. Item 5 (M3) is non-blocking and can run in parallel. --- ## Notes - All line numbers refer to the tree at commit `2310f6c` (the plan's commit) for `src/` files, and to the plan/ADR markdown as committed at the same tree. - The POC on `readplan-poc` was inspected via `git show readplan-poc:poc/readplan/{src/lib.rs,FINDINGS.md}`; it is not merged to `main` and the plan correctly states this. - `alktty` and `alkcall` downstream repos exist as path dev-deps (`/workspace/@alkdev/alktty`, `/workspace/@alkdev/alkcall`); the plan's claim that they're in-house and updated with the bump is verifiable, though this review did not inspect their call sites in detail. - This review does not re-litigate ADR-011 or ADR-012's accepted decisions. H1 and H2 are about the plan *contradicting* the ADRs or being unsound, not about the ADR decisions themselves; M3 is about sharpening a deferral, not about re-deciding it. - The deferral-pattern scan is a methodology experiment: a pre-declared scan for LLM-specific planning quirks (deferral black 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. --- ## 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.