Resolve N3: maxLength is string/bytes-only (review #006)
- Parse gate in BastField::parse: maxLength on any kind other than
string/bytes is a clean Schema error (records, arrays, inline
structs, refs, union shared fields all covered; the choke point
needs no ref-following since $defs entries are struct/union/enum)
- Meta-schema FieldDef: if kind in {string, bytes} else maxLength
forbidden — the published alk.dev/bast/v1 contract matches the
parser (N2 dual-layer pattern)
- M5's compute-side record maxLength arm became unreachable and was
deleted (offset-indirect arm stays); the two superseded M5
maxLength tests rewritten as the n3_* parse-rejection family
- ADR-006 remedy message tailored per kind: for records both
annotated remedies are dead ends, so the error text points at the
last-position fix only
- Docs aligned: bast-format.md (FieldDef meta-schema + FieldDef/
Variable-Length Encoding prose), layout-engine.md (Strategy 2 +
ADR-006 paragraph), schema-layer.md, ADR-003 §2/§3a amended,
builder .max_length() doc
- Review #006: N3 resolved (all findings now closed), M5 update
note, test-count bookkeeping note (in-session probes vs static
counts), status lines flipped to fully resolved
Verified: 547 tests green + 2 ignored doctests in BOTH release and
default profiles (a stale debug artifact from an earlier session
masked one H3 roundtrip test in debug; clean rebuild passes both),
clippy -D warnings clean, cargo doc --no-deps zero warnings, wasm
build green.
This commit is contained in:
1 parent
0857ea1c23
commit
bb28ba3006
9 files changed
+352
-95
No files matched your search
@@ -128,6 +128,12 @@ These are different validators for different inputs.
|
||||
"encoding": { "enum": ["length-prefixed", "offset-indirect"] },
|
||||
"maxLength": { "type": "integer", "minimum": 0 }
|
||||
},
|
||||
"if": {
|
||||
"properties": {
|
||||
"kind": { "enum": ["string", "bytes"] }
|
||||
}
|
||||
},
|
||||
"else": { "properties": { "maxLength": false } },
|
||||
"required": ["name", "kind"],
|
||||
"additionalProperties": false
|
||||
},
|
||||
@@ -269,8 +275,11 @@ These are different validators for different inputs.
|
||||
- `align` (optional): field-level alignment (aligned mode only).
|
||||
- `encoding` (optional): `"length-prefixed"` (default) or
|
||||
`"offset-indirect"`. See [Variable-length encoding](#variable-length-encoding).
|
||||
- `maxLength` (optional): byte-length cap. See
|
||||
[Variable-length encoding](#variable-length-encoding).
|
||||
- `maxLength` (optional, `string`/`bytes` fields only): byte-length
|
||||
cap. See [Variable-length encoding](#variable-length-encoding).
|
||||
Rejected at parse on any other kind (review #006 N3: the annotation
|
||||
was silently unenforced there — the validation plan bakes `maxLength`
|
||||
into string/bytes leaves only).
|
||||
|
||||
### TypeRef
|
||||
|
||||
@@ -456,8 +465,11 @@ override). In little-endian mode, `u32::from_le_bytes`; in big-endian
|
||||
mode, `u32::from_be_bytes`. Ensures SFTP consumers (big-endian) have
|
||||
consistent byte order for field values and length prefixes.
|
||||
|
||||
Applies to all variable-length types: `string`, `bytes`,
|
||||
`record`, and arrays of variable-length elements.
|
||||
Applies to variable-length primitive types only: `string` and
|
||||
`bytes`. The parser rejects `maxLength` (and the meta-schema forbids
|
||||
it) on every other kind — including `record` (review #006 N3/M5: no
|
||||
consumer honored it there, so the annotation was either silently
|
||||
unenforced or, in aligned mode, silently corrupt).
|
||||
|
||||
## Endianness
|
||||
|
||||
|
||||
@@ -136,9 +136,9 @@ reserving worst-case space.
|
||||
|
||||
- `true` is a shorthand for the default (length-prefixed). This keeps
|
||||
the common case concise and the override explicit.
|
||||
- The `encoding` annotation and `maxLength` apply to all variable-length
|
||||
types: `AlkType:String`, `AlkType:Bytes`, `AlkType:Array`,
|
||||
`AlkType:Record`, `AlkType:Timestamp`.
|
||||
- The `encoding` annotation and `maxLength` apply to the variable-length
|
||||
primitive types `AlkType:String` and `AlkType:Bytes`. (`maxLength` on
|
||||
records was amended out by review #006 N3/M5 — see §3a.)
|
||||
|
||||
### 3a. TRecord value type
|
||||
|
||||
@@ -164,8 +164,13 @@ the `"values"` property in the schema:
|
||||
the value's size is determined by its kind (fixed-size kinds have a
|
||||
known size; variable-length kinds carry their own length prefix).
|
||||
- The count and key-length prefixes respect the schema's endianness.
|
||||
- In aligned static mode with `maxLength`, the entire record is reserved
|
||||
at `maxLength` bytes (zero-padded).
|
||||
- ~~In aligned static mode with `maxLength`, the entire record is
|
||||
reserved at `maxLength` bytes (zero-padded).~~ **Amended (review #006
|
||||
N3/M5, 2026-09-02):** `maxLength` is rejected at parse on record
|
||||
fields. The aligned materializer walks the record's inline
|
||||
count-prefixed form and never honors the reservation (M5: silent
|
||||
cross-field corruption), and no packed consumer enforced it either
|
||||
(N3: silently unenforced). `maxLength` is `string`/`bytes`-only.
|
||||
|
||||
### 4. TUnion discriminators
|
||||
|
||||
|
||||
@@ -113,7 +113,11 @@ with a `AlkTypeError::Offset` — the `OffsetMap` reserves only 4 bytes
|
||||
(the length prefix), but `data_access::write_string` writes prefix +
|
||||
data inline, which would clobber subsequent fields. Non-final variable
|
||||
fields must use `maxLength` (fixed-size reservation) or
|
||||
`"encoding": "offset-indirect"`. See
|
||||
`"encoding": "offset-indirect"` — except `record` fields, for which
|
||||
neither remedy is available (`maxLength` is rejected at parse — review
|
||||
#006 N3 — and `offset-indirect` is rejected for records in aligned
|
||||
mode — review #006 M5), so a non-final record field cannot be repaired
|
||||
and must move to the last position. See
|
||||
[ADR-006](decisions/006-reject-non-final-inline-length-prefixed-in-aligned-mode.md).
|
||||
|
||||
## Offset Computation Algorithm
|
||||
@@ -194,6 +198,12 @@ annotation shapes).
|
||||
only. The engine uses strategy 1 (inline length-prefixing) because
|
||||
protocols don't benefit from fixed-size reservation.
|
||||
|
||||
`maxLength` applies to `string` and `bytes` fields only. The parser
|
||||
rejects it on any other kind (review #006 N3): the validation plan
|
||||
bakes it into string/bytes leaves only, so on a record (or any other
|
||||
kind) the annotation did nothing — and in aligned mode a record
|
||||
reservation was silently corrupt (review #006 M5).
|
||||
|
||||
**Strategy 3: Offset indirection (`"encoding": "offset-indirect"`).**
|
||||
1. The field is a struct `{offset: u32, length: u32}`.
|
||||
2. The `OffsetMap` records the position of this struct.
|
||||
|
||||
@@ -230,7 +230,10 @@ type-level properties. The concrete BAST shapes are in
|
||||
The `maxLength` keyword is *not* a BAST invention — it is the standard
|
||||
JSON Schema `maxLength`, repurposed as a byte-length cap. In aligned
|
||||
mode it reserves a fixed-size slot; in packed mode it is a validation
|
||||
constraint only. See [bast-format.md §Variable-Length
|
||||
constraint only. It applies to `string`/`bytes` fields only: the parser
|
||||
rejects it on any other kind (review #006 N3 — elsewhere it was
|
||||
silently unenforced), and in aligned mode a record reservation was
|
||||
silently corrupt (review #006 M5). See [bast-format.md §Variable-Length
|
||||
Encoding](bast-format.md#variable-length-encoding) and
|
||||
[ADR-003](decisions/003-schema-annotations.md).
|
||||
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
---
|
||||
status: in-progress (H1, L1, H3, L5, L6, H2, M1, M2, M3, L2, L3, N2, L4, N1, M5, M6 resolved 2026-09-02)
|
||||
status: resolved (H1, L1, H3, L5, L6, H2, M1, M2, M3, L2, L3, N2, L4, N1, M5, M6, N3 resolved 2026-09-02; M4 items 1–4 closed, per-fix coverage posture ongoing by design)
|
||||
last_updated: 2026-09-02
|
||||
reviewed_artifacts:
|
||||
- src/read_plan.rs
|
||||
@@ -98,14 +98,14 @@ sub-types, `fingerprint()` methods; `Hash` on `Endian`/
|
||||
| High | 3 (H1, H2, H3) | all resolved 2026-09-02 |
|
||||
| Medium | 6 (M1–M4, M5, M6) | M1, M2, M3, M5, M6 resolved 2026-09-02; M4 items 1–4 closed 2026-09-02 (posture ongoing) |
|
||||
| Low | 6 (L1–L6) | all resolved 2026-09-02 |
|
||||
| Nit | 3 (N1, N2, N3) | N1, N2 resolved 2026-09-02; N3 open |
|
||||
| Nit | 3 (N1, N2, N3) | all resolved 2026-09-02 |
|
||||
|
||||
All Highs, five of six original Mediums (M4's four concrete items are
|
||||
closed; the per-fix coverage posture itself is ongoing by design), all
|
||||
six Lows, and two of three Nits are resolved. Two new findings (M5, M6)
|
||||
and one new Nit (N3) surfaced during the follow-up session while
|
||||
closing M4's coverage map — all in the same untrusted-input family the
|
||||
review exists for. Remaining: M4 (ongoing posture), N3.
|
||||
Every finding is resolved. The only intentionally-ongoing item is M4's
|
||||
*posture* (fold a coverage check into each future fix session), which
|
||||
is process, not a defect. Two new findings (M5, M6) and one new Nit
|
||||
(N3) surfaced during the follow-up session while closing M4's coverage
|
||||
map — all in the same untrusted-input family the review exists for;
|
||||
N3 closed last (string/bytes-only `maxLength` gate).
|
||||
|
||||
The three Highs are adversarial-input crashes (H1, H2) and a
|
||||
cross-consumer wire-layout convention gap (H3) — all three are the
|
||||
@@ -161,9 +161,20 @@ them became materially easier to hit with the 0.3.0 surface.
|
||||
resolution blocks. M6 was probe-verified while writing the M4
|
||||
record-with-union-values coverage test; items 1–3 closed the aligned
|
||||
materializer, reader, and data-access holes from M4's map. Coverage
|
||||
after: materialize.rs 64.48→85.72% lines, TOTAL 89.59→90.60%. 567
|
||||
tests green, clippy `-D warnings` clean, wasm build green, `cargo
|
||||
doc --no-deps` zero warnings.
|
||||
after: materialize.rs 64.48→85.72% lines, TOTAL 89.59→90.60%. (The
|
||||
"567 tests green" quoted at the time counts in-session disposable
|
||||
probes that were deleted before commit; static count at `2eb086f` is
|
||||
542 + 2 ignored — see the N3 resolution block's bookkeeping note.)
|
||||
- **N3 (2026-09-02):** resolved — see the "Resolution (2026-09-02)"
|
||||
block on the finding. Posture decision: option (b) generalized —
|
||||
`maxLength` is string/bytes-only, rejected at parse on every other
|
||||
kind, enforced in the meta-schema; M5's compute-side record
|
||||
`maxLength` arm became dead and was deleted; ADR-006's record
|
||||
remedy text tailored; five normative docs aligned. 548 tests green
|
||||
(static count 547 + 2 ignored doctests, +6 net: six `n3_` parse
|
||||
tests and two meta-schema tests added, the two superseded M5
|
||||
`maxLength` tests removed), clippy `-D warnings` clean, wasm build
|
||||
green, `cargo doc --no-deps` zero warnings.
|
||||
|
||||
---
|
||||
|
||||
@@ -1181,6 +1192,15 @@ silently corrupt, now rejected with a clean error naming the
|
||||
annotation). Verified with M5's commit: 546 tests green, clippy clean,
|
||||
wasm green.
|
||||
|
||||
**Update (2026-09-02, N3):** the `maxLength` half of this fix was
|
||||
superseded — `maxLength` is now rejected *earlier*, at parse
|
||||
(`BastField::parse`), for every non-string/bytes kind, so the
|
||||
compute-side record `maxLength` arm became unreachable and was
|
||||
deleted. The offset-indirect arm remains reachable and stays. The two
|
||||
M5 `maxLength` tests were rewritten as the `n3_*` parse-rejection
|
||||
family; `m5_record_offset_indirect_rejected_in_aligned_mode` is
|
||||
unchanged. See the N3 resolution block.
|
||||
|
||||
### M6. The legacy BAST-walker's field-disc union arm skips non-discriminator shared fields — variant materializes from shared fields' bytes
|
||||
|
||||
**Files**: `src/materialize.rs:728-765` (the `Field` arm of
|
||||
@@ -1278,6 +1298,67 @@ but rejects schemas that are otherwise fine); (c) document that
|
||||
Option (c) is the cheapest honest closure; option (a) is the most
|
||||
useful. Needs a posture decision like N1's — either is small.
|
||||
|
||||
**Resolution (2026-09-02) — posture decision taken: option (b),
|
||||
generalized to string/bytes-only.** `maxLength` is rejected at parse on
|
||||
every non-`string`/`bytes` field kind, not just records — the
|
||||
"silently unenforced" argument (the validation plan bakes `maxLength`
|
||||
into string/bytes leaves only, validation_plan.rs:338-339) applies
|
||||
identically to every other kind, so one gate closes the whole family:
|
||||
|
||||
1. **Parse gate** (`BastField::parse`, the choke point every consumer
|
||||
inherits — struct fields and union `fields` both parse through it;
|
||||
records can only appear as inline field kinds since `$defs` entries
|
||||
are struct/union/enum only, so no ref-following is needed): a
|
||||
`maxLength` on any kind other than the `string`/`bytes` primitives
|
||||
is a clean `Schema` error naming the field, the path, the kind, and
|
||||
the review reference. `BastType::Ref` fields are rejected too — a
|
||||
ref resolves to a struct/union/enum, never to a string/bytes
|
||||
primitive, so the annotation is equally dead there and the
|
||||
resolved shape does not need to be known to reject it.
|
||||
2. **Meta-schema** (`bast_meta.rs` FieldDef): `if kind ∈ {string,
|
||||
bytes}` / `else: maxLength: false` — the published
|
||||
`https://alk.dev/bast/v1/schema` contract now matches the parser
|
||||
(the N2 dual-layer pattern).
|
||||
3. **Dead compute arm deleted** (repo precedent M3/M4-item-4): M5's
|
||||
aligned-mode `maxLength` rejection in `compute_field`'s Record arm
|
||||
is unreachable after the parse gate and was removed; the
|
||||
offset-indirect arm stays (still reachable — the parser accepts
|
||||
offset-indirect on any field). The two M5 `maxLength` tests
|
||||
(`m5_record_max_length_rejected_in_aligned_mode`,
|
||||
`m5_record_max_length_rejected_even_as_last_field`) were rewritten
|
||||
to the parse-rejection posture as the `n3_*` family in `bast.rs`;
|
||||
`m5_record_offset_indirect_rejected_in_aligned_mode` is unchanged.
|
||||
4. **ADR-006 remedy message tailored per kind**: for a non-final
|
||||
*record* field, both annotated remedies are now dead ends
|
||||
(`maxLength` → N3 parse rejection, `offset-indirect` → M5 compute
|
||||
rejection), so the error text now tells the consumer to move the
|
||||
field to the last position and says why; string/bytes fields keep
|
||||
the original remedy list.
|
||||
5. **Docs aligned** (the "schema is the format" principle): ADR-003
|
||||
§2's kind list and §3a's record-reservation sentence amended with
|
||||
strikethrough + review reference; bast-format.md FieldDef +
|
||||
meta-schema listing + Variable-Length Encoding section updated;
|
||||
layout-engine.md Strategy 2 and the ADR-006 summary paragraph
|
||||
updated; schema-layer.md annotation semantics paragraph updated;
|
||||
builder `.max_length()` doc states the string/bytes-only posture.
|
||||
|
||||
Breaking constraint for 0.2.0-era schemas that put `maxLength` on any
|
||||
non-string/bytes field — same class as M5's aligned-record rejection
|
||||
(previously silently unenforced, now rejected with a clean error).
|
||||
Tests: six `n3_*` parse tests in `bast.rs` (record, fixed primitive,
|
||||
array, inline struct, union shared field rejected; string/bytes still
|
||||
accepted) and two meta-schema tests (`maxLength` on record rejected by
|
||||
the published contract; string/bytes accepted).
|
||||
|
||||
Session note (test-count bookkeeping): the resolution log below quotes
|
||||
567 tests green for the M6/M4 commit (`2eb086f`); the static
|
||||
`#[test]` count at that commit is 542 (+2 ignored doctests). The
|
||||
difference matches the disposable probe tests that session ran in-tree
|
||||
during verification and deleted before commit (per the Methodology
|
||||
no-reproducer rule). Static counts at the other 0.3.0 commits: 475
|
||||
(release `9949f91`), 489 (H3 `05a2a42`), 507 (L2/L3), 510 (N2), 523
|
||||
(L4/N1/M5), 542 (M6/M4).
|
||||
|
||||
---
|
||||
|
||||
## What's Good
|
||||
@@ -1336,13 +1417,15 @@ Worth recording, because the findings shouldn't eclipse it:
|
||||
`field_endian_for_element` verdict: dead arms deleted); the
|
||||
per-fix coverage posture stays ongoing by design. The M4 session
|
||||
surfaced **M5** and **M6** (both fixed same-day, see their
|
||||
findings) and **N3** (open).
|
||||
findings) and **N3** (resolved, see below).
|
||||
9. ~~**M5**~~ **resolved 2026-09-02** (aligned record
|
||||
`maxLength`/`offset-indirect` rejected at compute).
|
||||
10. ~~**M6**~~ **resolved 2026-09-02** (legacy walker's field-disc
|
||||
union arm walks shared fields first).
|
||||
11. **N3** — open: pick the posture (enforce record `maxLength` in
|
||||
`validate_bytes`, or document string/bytes-only) and close.
|
||||
11. ~~**N3**~~ **resolved 2026-09-02** (posture decision: `maxLength`
|
||||
is string/bytes-only — rejected at parse on every other kind,
|
||||
enforced in the meta-schema; supersedes M5's compute-side record
|
||||
`maxLength` arm, which became dead and was deleted).
|
||||
|
||||
## Notes
|
||||
|
||||
|
||||
Reference in new issue
Block a user