types/rtti/ts: render trailing omittable positions optional - #1708

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e82031f574

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 29ed8851a9

trailingOptionalPositions closes the real gap, and it does so better than the fix I was about to suggest — running all five cases through all three readers catches a shared regression in triplicate rather than only in the validator.

The gap was worth naming: before that commit, the five new rows fed only the two differential proofs, which assert that the readers agree, never what the verdict is. So I deleted the PR's entire subject — made option(t) stop admitting undefined in all three readers — and all 54 rows stayed in agreement with both loops passing. Nothing in the repo asserted that [2,4n] is accepted for a four-position schema. At this head that mutation fails trailingOptionalPositions by name, while the differential loops still pass; the commit message states the defect exactly.

Each of the five assertions now bites as a unique catcher, including the two I couldn't demonstrate before: only-the-last-may-be-absent → accepted([2,4n]); all-or-nothing → accepted([2,4n,'x']); open tuple capped at declared-1 → accepted([2,4n,'x',null]); absence-past-the-end-never-fails → rejected([2]); present-but-unchecked → rejected([2,4n,5]).

The behaviour itself is correct and fully independent — all eight presence subsets on a four-position schema agree across the three readers — and it composes cleanly with the open-tuple caveat rather than contradicting it: accepted lengths are {2,3,4,5,…}, both properties falling out of constContainerValidate iterating the schema's entries. The 167-line issue doc I verified end to end: the reproduction is exact including {path:['1'],message:'no match'}, both TypeScript claims compile as described (TS2344 and TS2322 verbatim), every citation resolves, and the scope is accurate in both directions — exactly two rebuild functions at six sites, with nothing on the data side to include.

npm test 3379/0, tsc clean.

Three things left:

The body has no Changelog: section at all — Changelog: none is the right content for a proof-and-todo-only change. It also still describes only the first of three commits: the 167-line issue doc goes unmentioned, and so does trailingOptionalPositions, which is the most valuable thing in the diff.

One line would pin the hole case. A later optional present while an earlier one is absent is the sharpest witness of per-position independence — it's exactly what all-or-nothing forbids and what truncation can't express — and no assertion states it, new or old. accepted([2, 4n, , null]) closes it and would make present-but-unchecked mutations fail on a second front.

Two carried nits: the filename parse-omits-undefined-members names the proposed behaviour where the defect is that parse materializes them (the heading inside the file gets it right), and neither the comment nor the doc says how "stop short" composes with "run long".

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8a4b6c7dde

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 424e56435e

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f7cb65292

The TupleTs change is right and better derived than I expected. It keys off the set, not the syntax of option(...) — a Const undefined, a hand-spelled three-way or(string, number, undefined), and the unknown tag all optionalize — so the changelog wording is accurate rather than overstated. Verified with 11 invariant Equal pins plus value-level accept/@ts-expect-error cases, tsc exit 0.

Type and runtime agree at the bottom end on every schema I probed, all three readers, all presence subsets including holes and explicit undefined. The one place the claim could quietly have been false — [number, unknown], where the type now says position 1 is omittable — holds: [1] is ok/ok/ok. The top-end asymmetry (runtime open, Ts<T> exact) is unchanged and the README's new wording states it correctly.

It also closes a hole that was there before: validate([number, option(string)])([42]) returns the length-1 array, which at 424e5643 did not inhabit its own Ts<T>. It does now.

_tupleOption and _tupleInteriorOption pin it. Reverting TupleTs to the required-length rendering fails _tupleOption by name (TS2344, sole error). Worth recording why the third mutation is not a gap: optionalizing every admits-undefined position rather than only the trailing run survives, and that is correct — TypeScript itself normalizes an optional element before a required one back to required, so "only the trailing run" is enforced by the compiler and both spellings are the same type. I checked that against the full battery rather than assuming it.

Blast radius is clean: all 74 Check</Check3< pins are Equal-based, so a hand-written pin against a tuple schema with trailing optionals would have failed to compile. Every option(...) in a tuple position lives inside rtti's own proofs; the external uses in mcp/cas and media/json/schema are struct keys.

One blocking item and two minor:

The body still has no Changelog: section. I raised this at 29ed8851 and it is still absent — CONTRIBUTING.md:201 makes it mandatory, and the body becomes the squash commit. changelog/unreleased/1708.md is now correct and conforming; it just needs its list item copied into the body. The title and body are also stale in a way that now matters more than before: both still describe only the old proof rows, and neither mentions the types.ts rendering change or the BREAKING flag. This started as a test-cases PR and is now a public type-API break.

parse-omits-undefined-members.md:120 overstates what is pinned. It says an interior position admitting undefined may still be absent at runtime "and ../validate/proof.f.mjs's optionalPositions pins exactly that." It does not — that proof's hole row is accepted([2, 4n, , null]), where position 2 is followed by another omittable position, not a required one. No runtime proof anywhere covers a [option(X), required] shape. The claim is true (I confirmed [<hole>, 1] and [undefined, 1] are ok/ok/ok), just unpinned — _tupleInteriorOption has no runtime counterpart. One row closes it, or soften the sentence.

7f7cb6529 is dead weight. The @type {const} cast on extra changes no compile or test outcome — dropping it leaves 3379/0 and zero tsc diagnostics. Harmless and consistent with the line above it, so keep it if you like the symmetry; just noting it pins nothing. The row it sits in is live: flipping either side of the open/closed split fails optionalPositions by name.

Gates: npm test 3379/0 at head, identical to 424e5643 — the delta adds no runtime test, which is expected for a type-level change. tsc --noEmit exit 0.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a6942c1669

The production change is correct, minimal, and genuinely pinned. Head moved twice while I was checking — findings are against a6942c166, which includes your own merge of main.

RequiredPart's fallback going from readonly [] to M fixes three shapes, not the one the commit message names: readonly (N|B)[] and (N|B)[] both rendered readonly [], and so did readonly [N, ...S[]], while readonly [...S[], OS] rendered readonly [(string|undefined)?] with the rest dropped. The rest-tuple repair is real but unpinned — worth a line if you want it held.

No fixed-length rendering moved: all 11 rows I measured at 7f7cb6529 are byte-identical at head, interior-stays-required included. a351949d8's respelling of the pins as schema types is faithful to the real constructor, not just its JSDoc — Equal<typeof option(string), Or<readonly [typeof string, undefined]>> is true.

Mutations: _NonFixedLength and _OptionalTail each die on their own; restoring the readonly [] fallback dies with _NonFixedLength as the sole error, so that pin is the only guard on the regression; _tupleOption and _tupleInteriorOption both still bite. One row is enforced by TypeScript rather than by the assert — making the interior optional is inexpressible (TS1257) — but dropping its | undefined does die, so it isn't dead weight.

The type disagrees with the runtime in both directions on the non-fixed-length shape (it admits [], [1], [1,1] that runtime rejects, and rejects [1,2n,'extra'] that runtime accepts). Both are inherent to an erased length, and strictly better than readonly [], which admitted exactly one value the runtime rejects and rejected every value it accepts. Top-end asymmetry unchanged.

Gates: npm test 3379/0 at the branch tip and 3392/0 at the merge — the +13 is main's side (#1712's rows), not this delta. tsc --noEmit exit 0 both. No new casts; the delta actually removes two. changelog/unreleased/1708.md correctly needs no new item — the commit that introduced the readonly [] bug never reached main, so the broken rendering never shipped.

Still the one blocking item: the body has no Changelog: section. I raised this at 29ed8851 and at 7f7cb6529; four commits have landed since and none touched it. CONTRIBUTING.md:201 makes it mandatory and the body becomes the squash commit. The file is correct and conforming — copy its list item in. The title and description are also still stale: both describe only the five original proof rows, and neither mentions the types.ts rendering change or that it is BREAKING. This PR is a public type-API break now, and nothing in what merges to main will say so.

Also still open: parse-omits-undefined-members.md:120 claims optionalPositions pins that an interior admits-undefined position may be absent at runtime. It doesn't — the hole row is still accepted([2, 4n, , null]), position 2 followed by another omittable one. I checked whether #1712 supplied a new candidate on main; validate/proof.f.mjs:130's [option(number), null] is a [schema, value] pair with a scalar schema, not a [option(X), required] tuple. _tupleInteriorOption still has no runtime counterpart.

The @type {const} I mentioned last time: this delta removed its own, and the one at validate/proof.f.mjs:261 is untouched and harmless.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f8804bccb3

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b4adaeeb50

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 12f3df73f5

Head moved twice while I was checking; this is against 777542724, and the body was rewritten mid-review too.

The net types.ts change versus my last review is just the number extends M['length'] guard plus the SplitTs extraction — and what those commits do is narrow the PR back to main's behaviour, not extend it. Three-way probes (a6942c166 / head / main) make that concrete:

• f8804bccb fixed a real regression of this PR: a variadic schema tuple was flattened, so Ts<[...number[], option(string)]> rendered readonly (string | number | undefined)[] and admitted a string in the number prefix. Main never had that.
• 990912e23 fixed a second one: TupleTs<A|B> cross-multiplied, so readonly [1, true] — A's prefix with B's suffix — was assignable. Main rejects it. Only reachable through the exported TupleTs, as your message says.
• cd2f99c6b is intra-PR churn — its shape renders readonly [number, string?] identically at a6942c166, head and main. It repairs the fallback that f8804bccb itself had changed three commits earlier. Fine, just noting the commit message reads as if it were a pre-existing bug.

All 11 fixed-length renderings are byte-identical to my reference table, so nothing moved for consumers. I checked the Ts<> consumers directly — ci/common, protocol/{mcp,json_rpc}, media/{json,note,revision}, mcp/{cas,evo} — and found no tuple schema with a trailing option() at all; they're struct schemas going through the untouched StructTs. No call site should gain a ? from these commits.

All 11 new assertion rows die individually, and every kill is a TS2344 at the pin's own line — a named proof, never a TS1257 inexpressibility rejection. Each source mutation dies at exactly one pin.

Three things:

The body's Changelog: item is HTML-escaped. The marker, placement and **BREAKING CHANGES:** prefix are all correct now, and the title and description are fixed — but the body contains Ts<T> where the file has `Ts<T>`, 7 entity occurrences body-wide. My first fetch had plain <, so the most recent body edit introduced it. Since the body becomes the squash commit verbatim, Ts<T> lands in git history, and CONTRIBUTING wants the item identical to the entry file. Unescaping fixes it. New shape for the list: correct in every structural respect, wrong at the character level.

Two pins survive every single-point mutation. _NonFixedLength and _RestTuple are killed by none of {guard neutered, fallback → readonly [], distribution undone, full TupleTs revert} — only by the guard-and-fallback double mutation. Guard and fallback both return M for those shapes, so they're redundant. Worth knowing because b4adaeeb5's message claims _RestTuple "closes that; all three now die when the guard is disabled" — true when written, falsified by cd2f99c6b's fallback restore. The squash discards that message, so this is informational; _RestTuple still documents intent, it just doesn't discriminate any mechanism.

parse-omits-undefined-members.md:120 still cites the wrong proof. interiorOptionBeforeRequired is a genuine [option(X), required] runtime row now, run through open and closed readers — that's the substance fixed. But the doc still says optionalPositions "pins exactly that", which is the sentence b4adaeeb5's own message calls wrong, and interiorOptionBeforeRequired appears nowhere in that file. The file is already in your diff.

One non-blocking observation, pre-existing rather than introduced: a schema whose own last member is optional takes the fallback and keeps the whole mapping, so a preceding omittable position isn't optionalized even where TypeScript could spell it — Ts<[N, option(B), (S)?]> gives readonly [number, boolean | undefined, string?] where (boolean|undefined)? is expressible. Renders identically at main and at a6942c166, so the fallback preserves it rather than causing it. A sentence in RequiredPart's comment would cover it.

No new changelog item needed — these commits move the renderings back to main's behaviour, so the public delta is unchanged and the shortened item is still accurate.

Gates: npm test 3398/0, tsc --noEmit exit 0, no casts introduced, branch up to date with main.

Approving. Both must-fix items are closed.

The entity count is 0 (was 7) and the Changelog: section is md5-identical to changelog/unreleased/1708.md, last section, single trailer. The citation now points at interiorOptionBeforeRequired, and I verified that proof genuinely covers [option(X), required] — [, 5], [undefined, 5] and ['x', 5] accepted, [5] rejected, all three readers, both open and closed, with closing flipping only the 'extra' row.

Both types.ts and proof.f.mjs +7 blocks are pure // comments — stripping comment lines leaves both files byte-identical and the 22 @typedef pins unchanged in count and content — so no rendering could have moved and there was nothing new to mutation-test. The Y2 note is accurate: I re-ran all four single-point mutations and each of the three mechanisms the comment names is killed by exactly the pin it names, while _NonFixedLength and _RestTuple still die under none. Documenting rather than fixing is a fine disposition for two rows that state intent. Blast radius nil — walking every consumer schema graph found 9 tuple-kind nodes and none with a trailing position admitting undefined, and merging main is a no-op.

Two things worth knowing, neither blocking:

The rationale in cb0b1ea24's message is false. It says GitHub rewrites <T> and ' in a PR body. It doesn't — #1716's body carries a raw unescaped Ts<T>, and #1714 and #1707 carry raw < and several raw apostrophes, all with zero entities. The escaping came from whatever wrote 1708's body, not from GitHub. Nothing ships wrong, since the squash takes the description and the branch commit message evaporates. But the fix landed on the entry file rather than on the body that was actually malformed, so the Ts<T> → Ts precision loss was unnecessary (defensible on its own under the types/rtti/ts topic prefix), and the failure can recur.

One loose clause at parse-omits-undefined-members.md:118: "so truncation explains that row equally well" is literally false — truncation predicts rejection of optionalPositions's [2, 4n, , null], and all three readers accept it. It also contradicts that proof's own comment at validate/proof.f.mjs:247, "Omission is independent, not just truncation." The defensible point is that the hole sits inside the trailing omittable run, so it can't witness absence at a position the renderer marks required.

Gates: npm test 3398/0 at head and at 777542724, tsc --noEmit exit 0, purge matched nothing, no new changelog item warranted.

Still approving — the delta is documentation only, one file, +5/-2, no .f.mjs or .ts moved, so the approval still covers the code.

The corrected clause is true. optionalPositions's schema is [number, bigint, option(string), option(null)], so the trailing omittable run is positions 2-3 and the hole sits at 2 — inside it, with only an optional position after. The hole row is accepted by all three readers in both open and closed form, which is what makes the old "truncation explains that row equally well" false: truncation needs a dense prefix and would predict rejection. The new parenthetical now agrees with validate/proof.f.mjs:247's own comment instead of denying it, and matches what interiorOptionBeforeRequired's comment says. The interiorOptionBeforeRequired citation is untouched and still correct.

Gates: npm test 3398/0 — the +2 over the sibling branches are this PR's own proofs, so both optionalPositions and interiorOptionBeforeRequired do execute under npm test. tsc --noEmit exit 0.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9f6e131df9