types/rtti/todo: closed containers by default, then option as omission - #1725
Conversation
…ssion
Files one issue with two ordered stages: a bare `Struct`/`Tuple` becomes
closed with `open(c)`/`rest(c, r)` stating otherwise, and then `option(t)`
becomes omission rather than `or(t, undefined)`.
Stage 1 makes `validate`'s success cast sound — `Ts<readonly[42]>` is the
exact tuple, so returning `[42, 'extra']` under that type is a lie about
`.length` — and matches what `edag` already writes 21 times. Stage 2 closes
the gap where no schema separates `{}` from `{ a: undefined }`, and forces
`parse`'s construction answer, which dissolves parse-omits-undefined-members.
The order is deliberate: stage 2 reads the acceptance tables stage 1
rewrites, so the reverse order rewrites every table, proof and consumer
schema twice.
Changelog: none
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
`option` becomes a nullary schema — `() => ['option']` — denoting absence, so
an omittable member is `or(option, t)`. This answers the two spellings a
wrapper design had to forbid: `[or(option, number), 3]` is a hole at position
0, and `or(option, number, string)` is an ordinary flattened union.
The representation collapses to one more bit in the unit kind's bitset, where
`or(true, false)` already lives. Union, `subset`, `cmp`, `equal` and the
coverage collapse stay bitwise and unchanged; the whole absence rule is the
two `& unitBit(undefined)` tests in `trimPrefix` and `objectMayOmit`. A
wrapper would instead have needed a second syntactic category and an
`{ optional, node }` pair on every prop and prefix position.
Records the four rules the bit needs — `unknown` excludes absence, absence is
observable only at a container position, a `rest` never sees it, and absence
at a tuple position is "no such own index" — plus the trade it accepts:
degenerate spellings normalize away instead of being rejected.
Changelog: none
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | 4ae8bad | Commit Preview URL Branch Preview URL |
Aug 27 2026, 01:02 AM |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d6415726a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`Ts<[1, or(option, number)]>` is `readonly[1,number?]`, and that rendering is exact rather than an approximation: checked against the repo's tsc, the type accepts `[1]` and `[1, 2]`, rejects `[1, undefined]` with TS2322, and rejects `[1, 2, 3]`, which is what the closed schema does. Reading position 1 still gives `number | undefined`, matching JavaScript. Records that the exactness needs both stages — stage 1 for the length, stage 2 for the element type — that it depends on `exactOptionalPropertyTypes`, which this repo sets, and that the runtime printer and `Ts<>` converge on the same spelling afterwards. Adds the proof task that pins it. Changelog: none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
…o claude/rtti-proposal-qqkct0
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e95bb1579d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
o2alexanderfedin
left a comment
There was a problem hiding this comment.
The "today" picture is the strongest part of this and I could not find a single wrong statement in it. Every claim reproduces: validate([42])([42,'extra']) is ok, array(number) accepts [1, , 3], the data-README asymmetry holds in both readers, and the JSON round-trip defect is exact — parse([number,option(string)])([42]) gives [42, undefined], which stringifies to "[42,null]" and then fails validate. All 20 cited paths resolve, every quoted sentence is verbatim, and both pinned rendered strings match ts/proof.f.mjs. The consumer counts check out too — 21 close( in edag, 10 and 3 option( in the two protocol modules, and "the other 13 modules" is right.
The proposal is coherent on its own terms: Object.keys([1,,]) is ["0"] while Object.keys([1,undefined]) is ["0","1"], so the own-index rule does separate the two cells it needs to.
Two substantive things, both about coherence rather than facts.
The stage-2 changelog line prescribes a migration your own line 168 says is the wrong set. L346 says "option(t) becomes or(option, t)", but L168 writes { a: or(option, number, undefined) } // today's option(number) and the L226 table says or(option, number, string) at a key rejects {a: undefined}. Today validate({a:number,b:option(string)})({a:1,b:undefined}) is ok, so the mechanical rewrite silently narrows all 13 existing option( call sites by dropping the undefined member. Either the line should read or(option, t, undefined), or the narrowing is a deliberate second break and should say so. This is the exact contentious cell, so two sentences disagreeing about it is the thing to fix first.
This is the third open proposal on this subsystem with no cross-reference to either of the others. grep for issue numbers finds only #1622 and #1712 (both accurate; #1712 is merged).
- #1716 collides directly. Its whole lever is the
close(c, rest?)overload — it proposes normalizingclose(c, never)toclose(c)and arguesclose([number], () => ['const', undefined])must stay canonically distinct. #1725 deletescloseoutright. Both PRs even edit the same neighbouring file,schema-walk-own-indices.md, which #1725 cites in Related. And your stage 1 explicitly declines to answer the[1, undefined]-against-closed-[number]question ("No algebra change") while stage 2 answers it a third way — so someone landing this first has no idea #1716's defect persists in the interval. - #1719 enumerates the eDSL surface as "
primitives, array, record, or, option, never, close, and consts", with worked examplesconst pair = t => close([t, t])andconst maybeKey = option(key). Renamingclose→restand makingoptionnullary invalidates both spellings, and its stages 8 and 11 reason from them.
Related is the right place to state the ordering constraint and who owns the [1, undefined] question in the interval.
I measured the blast radius rather than reasoning about it — patched validateVisitor so bare tuple/struct route through the closed validators, ran the suite: 3395 pass / 7 fail, and all 7 are in fjs/types/rtti/validate/proof.f.mjs. Zero failures in media, protocol, mcp, edag, ci. That cuts both ways and the doc says neither half: the migration breaks nothing tested, and equally no test would catch a regression when the two protocol modules get wrapped in open(...) — which is the riskiest part, since JSON-RPC and MCP envelopes routinely carry undeclared fields in real traffic. "Audit the other 13 modules" is right; worth adding that the audit has no test backstop.
One claim I want to confirm because #1716 argues the opposite about the same escape hatch: L96-99's point that a required second parameter removes the () => ['const', undefined] hatch is correct — a bare undefined is already a legal Type (validate(undefined)(undefined) ok, validate(undefined)(1) error), so rest(c, undefined) is unambiguous.
Three smaller items. L313 says data/proof.f.mjs has 39 close(; it has 50 — and not from base drift, it was 50 at the commit that introduced the doc. validate/proof.f.mjs (37) is right; parse/proof.f.mjs has 24 and goes uncounted. The stage-2 proof task doesn't name the pin it contradicts: validate/proof.f.mjs:290 asserts [undefined, 5] is ok against [option(string), number] with the comment "the same value, spelled densely" — that comment is precisely the premise stage 2 abolishes, so it belongs on the deletion list. And the body has no Changelog: section — mandatory under the current rule for a docs-only PR as Changelog: none. (#1724 proposes making exactly this optional; judged under the rule on main today, it's the visible omission the rule exists to produce.) Title topic Design: isn't a module path or listed area — types/rtti/todo: is what #1716 uses — and the body's Related links are repo-relative, so they don't resolve from a PR description.
Gates: npm test 3402/0, tsc --noEmit exit 0, purge matched nothing. Conventions match the two closest siblings.
Review found three claims in stage 2 that do not hold; all three verified against the code and fixed. The set algebra is bitwise and genuinely unchanged, but the normalizations are not "two bit tests swapped": three sites read absence and only `objectMayOmit` is a substitution. `trimPrefix` must move its test to the trailing declared position, since a rest carries no absent bit and a bit test on it is dead — measured, `close([option(number)], option(number))` and `array(option(number))` are one Node today, and the stage-2 counterpart `rest([or(option, number)], number)` would no longer collapse to `array(number)`. The declared-key drop keeps its "rest is gone" guard, which `objectSet` already applies; only `isTop` moves, to `or(option, unknown)` for a declared member. Also settles the case the bit-strip creates: `array(option)` is the empty array, since length bounds a closed array, which is what `close` already does. That surfaced a live divergence — `toData(array(or()))` and `toData(close([]))` are the same Node, yet the thunk readers accept `new Array(1)` for one and reject it for the other, because the array handler walks `Object.entries` and never bounds length. Changelog: none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1d2eb14a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review found the stage 2 rule undefined when a rest resolves to a rule. `trimPrefix` already declines to see through a reference, and a rule cannot be stripped globally: the same rule may sit at a declared position, where the bit is meaningful. For `X = or(option, array(X))` the stripped form is not even inline — it is the fixpoint `X' = array(X')`. Stage 2 therefore strips an inline rest and leaves a referenced one alone, joining the structural incompleteness the data form already accepts for rule names: semantically equal, structurally distinct, mutual subsets rather than equal. `subset` masks the absent bit at rest positions so that mutual-subset property actually holds. Derived rules would restore canonicality at the price of the bisimulation-grade machinery the module deliberately avoids; recorded as the alternative with its cost. Changelog: none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d21e9a0b23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review found a third renderer over the data form that the task lists never
named. `media/json/schema/module.f.mjs` derives `required` and `minItems` from
`admitsUndefined`, so under stage 2 `{ a: or(option, number) }` would render
`required: ["a"]` while RTTI accepts `{}`.
Its two uses of the undefined bit split rather than both moving: the one
driving `required`/`minItems` asks about absence and takes the absent bit,
while `stripUndefined` asks what JSON can carry and stays on `undefined` — so
a key of `or(number, undefined)` renders as a required `number`, an
under-approximation JSON forces, alongside the `NaN` and `-0` corners the
module already documents.
Stage 1 reaches the same file more quietly: it follows the data form, so a
bare struct starts rendering `additionalProperties: false` on its own, and its
proof pins the old output.
Changelog: none
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14d8680a7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ions
Review found that omitting an absent member from `parse`'s entry list corrupts
the array kind: `arrayRebuild` is `entries => entries.map(([, v]) => v)`, so
`[, 3]` would rebuild as `[3]`, shifting the required value to index 0 and
returning something that fails its own schema. Acceptance rows would not catch
it, so the proof now asserts on the built value.
The predecessor issue avoided this by only dropping a trailing run and leaving
an interior `undefined` explicit. Stage 2 cannot: absence and `undefined` are
different values now, so an interior absent position has to stay a hole.
Records the constraint that decides the implementation. `Array.prototype.map`
preserves holes, `Array.from({ length }, ...)` yields a dense array, and
FunctionalScript has no index assignment or mutation — so the array kind's
rebuild maps over the value while the check keeps walking the schema.
Changelog: none
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dccf4ec513
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two review findings, both real. `CloseTs<C> = ConstTs<C>` ignores its rest, which was harmless while closed was the wrapper. After stage 1 the open form is the wrapper, so a straight rename would make `Ts<typeof open([42])>` the exact `readonly[42]` while `validate(open([42]))([42, 'extra'])` returns two elements — the unsound cast relocated rather than removed. The struct kind needs nothing, being width-open already; the tuple tail turns out to be expressible, measured against the repo's tsc: a mapped schema tuple takes a rest element, `...unknown[]` covers `open(c)`, and a rest may follow an optional element. `TupleTs`'s documented failure is that tail composed with the trailing-optional transform, which stage 1 must spike. Stage 2 never tasked the `option(t)` call sites: 52 across 10 modules outside rtti. `checkJs` makes a missed one `TS2554` rather than a silently absence-only schema, verified, but the schemas are wrong until migrated. Changelog: none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Both new commits are correct, and every mechanism claim reproduces. trimPrefix's doc comment does say "reading its unit bits would need the rule set, and the form already declines to see through a reference" — quoted exactly. objectSet (data/module.f.mjs:356-357) really gates the isTop prop filter on r === undefined, so "the rest guard stays" is right and the old wording was wrong. toData(close([option(number)], option(number))) and toData(array(option(number))) are identical as claimed. The three-reader divergence table verifies row for row, including array(or()) against new Array(1) — thunk ok, data error, parse ok — and the cause is right: containerValidate walks Object.entries and never reads .length. grep confirms no array(or()) row exists in validate/proof.f.mjs today. The data/README.md characterization is verbatim-accurate.
Four things still open.
The stage-2 changelog line is unchanged, and I under-counted its blast radius. L428 still says "option(t) becomes or(option, t)", while L168 writes { a: or(option, number, undefined) } // today's option(number) and L294 says or(option, number, string) at a key rejects {a: undefined}. module.f.mjs:105 is option = t => or(t, undefined), and validate({a:number,b:option(string)})({a:1,b:undefined}) is ok today. Correction to my own figure: I said 13 call sites; it is 90 rtti-option( sites, of which 39 are production rather than proof — protocol/mcp 10, media/json/schema 11, ci/common 5, mcp/evo 5, json_rpc 3, media/note 2, media/revision 2, mcp/cas 1. That makes the silent narrowing considerably more consequential than I first said.
Neither sibling is referenced. grep for issue numbers finds only #1712. #1719 still enumerates the eDSL with close at rtti-type-system.md:61 and works examples close([t, t]) at :75, :1087, :1167 and option(key) at :121 — all invalidated by this proposal. A correction I owe you: I said #1725 edits schema-walk-own-indices.md. It doesn't — its diff is one file, and it only links that file in Related. #1716 is the one that edits it. The collision with #1716 stands regardless, since #1716's lever is the close(c, rest?) overload this proposal deletes.
The close( count is still 39 where the file has 50, and parse/proof.f.mjs (24) is still uncounted at L380. validate/proof.f.mjs (37) is right.
The stage-2 proof task still doesn't name the pin it abolishes. It names _tupleOption, _tupleInteriorOption, optionalTuplePosition and allOptionalTuple, but not validate/proof.f.mjs:290 — every(rtti)(assertOk)([undefined, 5]) //< the same value, spelled densely against [option(string), number], run through all three readers and through close(t). That comment is precisely what L293's new pin abolishes, so it belongs on the deletion list.
Fixed since last time: the body now ends Changelog: none, and Related uses backticked paths instead of repo-relative links that don't resolve. Minor leftover: the title topic Design: isn't a module path or listed area — main's convention is types/rtti/ts:, protocol/json_rpc:, basen:, and your own branch commits use rtti/todo:.
Gates: npm test 3402/0 at d21e9a0b2, docs-only. One further commit landed while I was checking — rtti/todo: add the JSON Schema renderer to both stages' migration — which I have not reviewed.
…iblings Human review, four items, all verified. The migration is not a rename. `option(t)` is `or(t, undefined)`, so it accepts a present `undefined` — measured — and rewriting it to `or(option, t)` narrows every migrated schema; the faithful form is `or(option, t, undefined)`. This issue takes the narrowing deliberately, per site rather than swept, and the changelog now says the schemas got stricter rather than that a spelling changed. The `close` counts were per-line, not per-occurrence: `data/proof.f.mjs` is 50 rather than 39, `parse/proof.f.mjs` (24) was missing, and the two `ts` proofs were uncounted — 124 in all. Stage 2's proof task now names the pin it abolishes, `validate/proof.f.mjs:290`, whose `[undefined, 5]` is commented as the same value as the hole and stops being so. Related now cites both siblings: #1716, whose lever is the `close(c, rest?)` overload stage 1 deletes and which already documents the empty-rest length-check defect this file's `array(or())` row belongs to, and #1719, whose worked examples are written in the eDSL this proposal respells. Changelog: none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
option as omission
|
Thanks — all four verified and fixed in 27c7721, plus the title. Taking them in order. The migration is not a rename, and that was the important one. Confirmed by running it: On the figure: your 39 production sites and my 52 are the same count at different scopes — 39 production plus 13 in consumer proofs ( Counts were per-line, not per-occurrence — my error, and it makes the figures useless for sizing. Corrected to occurrences: The abolished pin is now named: Both siblings are cited now, and reading #1716 properly changed what I say about one of my own findings. Its "empty Thank you for the two self-corrections — the 13→39 figure and the One item of yours I did not act on: the Codex finding about Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27c772108f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review observed that `or(option, number)` and `number` accept the same inputs at the entry while staying structurally distinct, and asked for the bit to be stripped there as it is in a rest. Recorded the opposite decision with its reason. A rest node has no life outside its position — every value that position sees is a present member, so the bit is vacuous by construction. An entry node is the schema itself, and a `Data` is serializable, so a consumer may embed it at a member position where the bit is live; stripping at the root would lose information that reappears as a silent meaning change on reuse. The two sets really are different, and this form compares sets — the entry position just cannot witness it. Joins the rule-name limit in data/README.md as a stated incompleteness rather than a latent one. Changelog: none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 199ba34001
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…readers Two review findings, both verified, one of them a defect in this file's own previous fix. Mapping over the value preserves length, so the trailing absent run would not shorten and `[1, ,]` under `[1, or(option, number)]` would serialize back to `[1,null]` — the defect stage 2 exists to remove. The rebuild is slice-then-map: truncate to the last present declared position, then map the parsed element results over it, so a trailing run shortens while an interior hole survives. Measured: slice keeps an interior hole and drops a trailing one, and a map after either keeps it. `orVisit` tries union members in order, so a present value under `or(option, t)` reaches the `option` branch first and needs an ordinary rejection there. Extending the `Visitor` type does not force it: both `parseVisitor` and `validateVisitor` are cast to `any`, so a missing handler is a `v.option is not a function` throw rather than a type error, with no `try`/`catch` in the language to contain it. Changelog: none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0344f05eb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sition case Two review findings, both verified against the code. "Strip the absent bit" is data-form vocabulary with no meaning in `ts/types.ts`, where `OptionalFields` keys on `undefined extends Ts<T[K]>` and members arrive already reduced through `Ts`. The task now names a branded uninhabited marker: not `never`, which vanishes in a union and takes the information with it, and not `undefined`, which would make `or(undefined, number)` optional too and conflate the pair this stage separates. `Exclude` at the array/record element keeps `Ts<array(or(option, number))>` as `readonly number[]`, and at the reader result types it is "observable only at a container position" in type-level vocabulary. The redesigned `trimPrefix` reaches a referenced trailing position independently of a referenced rest, and neither it nor `arraySet` takes a rule set to resolve one with. The referenced exemption now covers both, so `rest([X], number)` with a recursive `X` stays structurally distinct from `array(number)` by a stated rule rather than by accident. Changelog: none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf
…sites - `RestTs`'s tail renders `...(Ts<R> | undefined)[]`: both readers check an undeclared member as an entry, so a hole past the prefix is accepted (verified: `validate(close([42], string))([42, , ])` is `ok`, `hasOwn(v, 1)` false) while `...string[]` types that index `string`, and `noUncheckedIndexedAccess` is off. `open(c)` is unaffected. - `arraySetSubset` compares each prefix position straight through `nodeSubset`, so the absent bit makes `[or(option, number)]` no subset of `array(number)` though both its values are admitted. Split the position test the way the object kind already splits present-set from may-omit, and restate the doc comment's `undefined`-keyed shortest-array argument. - Migrate three more sites stage 1 inverts: `../parse/module.f.mjs`'s module doc (the open-default statement, the "do not add a length check" instruction, and the `close(...)` examples), `media/json/todo/rtti-parse.md`, and `excluded-string-values.md`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f28f7130d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The `...(Ts<R> | undefined)[]` rule needs a guard. `rest([42], option)` strips its inline rest to nothing after stage 2, so both readers and the runtime printer see the exact `[42]`, while the formula computes `Ts<option>` as `never`, adds `undefined`, and renders `readonly [42, ...undefined[]]` — admitting `[42, undefined]`, which the schema rejects. `rest([42], or())` is the same in stage 1, without any absence involved. Strip absence from the rest; if what remains is empty, render the exact tuple. Noted that the exact rendering is only fully right once the length bound lands: measured, `validate(close([42], or()))([42, , ])` is `ok` while `validate(close([42]))([42, , ])` is `error` — the `array(or())` divergence the acceptance table already records.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe85e169ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ion) - The bound belongs with the rendering it makes sound. Stage 1 is what makes `RestTs` render an empty rest as the exact tuple, so shipping that without the bound hands `rest([42], or())` a `readonly [42]` over a value `validate` accepts at length 2 — the unsound cast this issue opens with, on a narrower spelling. Moved out of stage 2 and given `rest([42], or())` against `[42, , ]` as a second pin. - `ArrayTs` emits `readonly []` when the marker exclusion leaves `never`. `readonly never[]` is not the empty array: `readonly never[] = new Array<never>(1)` type-checks and its `.length` is `number`, while `readonly []` rejects it and its `.length` is `0` — both measured. So `Ts<array(option)>` would otherwise be wider than the runtime printer on the schema "Length still bounds a closed array" exists to settle. `RecordTs` needs no counterpart — an object type has no length.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93afd3434e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sites - Stage 1's validator item covered only the "Do not add a length check" paragraph. The module doc also carries a `## Structs and tuples are open` section (:29-38) stating the rule as this reader's own contract, a `## Closed containers` section (:47-52) defining close(c)/close(c, rest), and an `@example` that demonstrates the open reading and calls `close` twice. Deleting the paragraph alone would leave two section headings asserting the reverse of the code. - The stage 2 documentation sweep was a markdown inventory, and a comment is no more compiled than a .md file, so eleven JSDoc sites in six files fell between the two sweeps. Two are statements of the semantics stage 2 replaces rather than spellings: ts/proof.f.mjs:27 defines `option(t)` as `or(t, undefined)`, and data/module.f.mjs:453 argues a design decision from the very subset claim that flips in data/proof.f.mjs:620. The markdown count stands; the scope did not.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdd601a29a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- "The rest admits nothing" needs the semantic definition, not a paraphrase. `[or()]` is closed after stage 1 and has no inhabitant, and measured, `validate(close([42], [or()]))([42, , ])` is `ok` while `([42, 1])` is `error` — so a reader keyed on the exported `never` bounds nothing there, and the empty-rest `RestTs` rule would type an accepted length-2 array as the exact `readonly [42]`. close-counts-trailing-undefined works the criterion out in full (a `toData` equality compared up to rule renaming with `subset` both ways, fixed by five cases) and this file deletes that file, so the criterion is now its own carry-over task with `rest([42], [or()])` pinned. - checked-const-pin.md:14,36 quotes the `casAddArgs` schema twice as its motivating example. Not a call site, so neither the `option(` sweep nor `checkJs` reaches it, and it outlives stage 2 — it would be left arguing from a schema whose `type` key had become required.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2b75e1949
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… stage 1 - The empty-tail exception said "the absence-stripped rest is empty", which is a different question from the empty-rest criterion the length bound uses. A retained reference separates them: for the absence-only cycle the reference exemption keeps unstripped, `toData(rest([42], X))` still carries `rest: "X"`, so the readers accept hole-only arrays of any length, while stripping absence from `X` leaves nothing — the wrong test renders the exact `readonly [42]` over a length-2 value. Keyed on the criterion it keeps its tail: `...undefined[]` is wider than the schema but wide in the direction the success cast needs. - The acceptance-table paragraph still assigned the `array(or())` fix to stage 2, contradicting the task moved to stage 1 two commits ago. Stage 1 is what cannot ship without it; stage 2 would only have reached the same defect again through `array(option)`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ca86d0da2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- `| undefined` on the tail is necessary but not sufficient. A hole past the prefix can be readable through the prototype, and then it is neither absent nor checked: `undeclaredEntries` filters `Object.entries`, so measured, `validate(close([42], string))` is `ok` on a `[42, , ]` whose prototype carries `1: 99`, and the tail promises `string | undefined` over a number. Decide the rest region by what the index reads — walk the index range and hold every HasProperty index to the rest. `parse` escapes it by rebuilding. Same hazard as stage 2's declared-member prototype task, one region to the left. - Swept the tree for JSDoc stating the open/closed contract instead of naming sites as review finds them: types.ts:35-40,:168, module.f.mjs:120-128, ts/types.ts:112-124,:194,:201, ts/module.f.mjs:292,:308, data/types.ts:33-36, data/module.f.mjs:774-780 — all declaration files the tag and constructor edits touch without a checker reading the sentence. Plus three consumers that justify a design decision by the open default.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e20400d3d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a conservative type-level empty-rest test
- `Absent extends _TsRaw<T[K]>` cannot decide optionality. Measured:
`_TsRaw<unknown>` is `unknown`, `Absent extends unknown` is true, and
`unknown | Absent` is `unknown` — so the closed `{ a: unknown }`, which
stage 2 rejects `{}` for, would render `a?:` and become
indistinguishable from `{ a: or(option, unknown) }`, which the runtime
printer tells apart. The marker is absorbed by the top; only the schema
still carries the fact. `OptionalFields` keys on a structural
`_AdmitsAbsence<T>` recursing through `or`, which does no flattening.
- The inherited-index walk is bounded by `length`, and an inherited index
can lie above it. Measured, `close([42])` and `array(number)` accept
such values too, so this is the whole reader family and cannot be fixed
by extending a walk. Bound the readable set at the `isArray` boundary,
or state the incompleteness.
- `RestTs` cannot evaluate the empty-rest criterion — it is `toData` plus
`subset`, and `Ts<[or()]>` is `readonly [never]`, not `never`. Specify a
conservative syntactic approximation that keeps the tail when it cannot
tell: wide but sound, versus an unsound dropped tail.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 144a269445
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…h hole Measured with `Array.prototype[10] = 99`: `[42]` has exactly `Array.prototype` and length 1, and `validate(close([42], string))` still accepts it while `v[10]` reads `99`. So the boundary check closes only the per-value half, and the previous claim that it makes every index walk complete was wrong. The two halves differ in kind: a per-value prototype is a property of the argument, which a reader can inspect; pollution of the intrinsic is a property of the realm, invisible to any per-value test and free to change between a check and a read. Both need host code outside the subset, which forbids mutation — so the answer is to state the assumption in the README beside the others the readers rest on, and treat a prototype check as a narrowing rather than a fix.
Replaces the prescribed remedy with a scoped note. Measured with `Array.prototype[10] = 99`, `validate(rest([42], string))([42])` is `ok` while `v[10]` reads `99`, and so are `close([42])` and `array(number)` — whole reader family, predates both stages. Neither obvious remedy settles it, so stage 1 owes the README caveat and nothing more; the rest is a separate question about what the subset assumes of its host.
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approved. Verified at c2b75e194 (head has since moved); npm test 3402/3402, tsc --noEmit exit 0, all 15 file:line citations resolve, and every runtime claim in the delta reproduces — including the exact readonly [] rejection message and the "eleven JSDoc sites in six files" count, which an independent grep confirms.
One nit, non-blocking: the two carry-over items point at each other with reversed directions. :619 says the criterion is "the one the carry-over item below does not cover", but that item is at :568 — above it; and :572 calls the criterion "the task above those two" while it sits at :617, below.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc457da8be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The delete item (:669) called the length bound (:737) and the criterion (:718) "above" when both follow it, and the criterion item called the delete item "below" when it precedes. Reworded to the correct directions.
|
Thanks for the approval, and for re-running the claims rather than taking them. The nit is real and there was a third instance in the same sentence. Fixed in 2c76ffe. On the current head:
Both items moved during this review (the length bound from stage 2 to stage 1, and the criterion out of the delete item into its own task), and the direction words stayed put. Reworded to the correct directions. Note the head has moved several times since Generated by Claude Code |
- The consumer-rationale item listed three sites; a sweep that excluded proof files missed five. Two media READMEs base a whole extension strategy on the blanket claim, and three proof comments state it as the reason their row expects `ok` — those rows keep passing under `open(...)` while their stated reason stops being true. - The inherited-index walk named only `undeclaredEntries`. `arraySetValidate` filters `Object.entries` on its own path, and measured, the data reader answers `ok` on the same value, so fixing one would leave the two disagreeing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acce79b160
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The stage-2 task citing "the prototype asymmetry close-counts-trailing-undefined records" and the `## Related` entry both sit after stage 1 in a file that survives to stage 2, so deleting the issue without them leaves dangling links. The stage-1 tasks naming it are fine — that PR is the one doing the deleting.
Summary
This PR adds a design document outlining a two-stage refactoring of the RTTI schema system to improve type soundness and expressiveness:
Const(Struct/Tuple) schemas closed by default, withopen(c)andrest(c, r)primitives for open containersoptionas a nullary schema denoting absence rather thanor(t, undefined)Motivation
The current design has two soundness issues:
validatereturns tuples typed as closed but actually open at runtime, causing the static type to lie about.lengthundefined, so schemas cannot express the set{}vs{ a: undefined }The document shows that consumers already vote with their code: the tuple ADT uses
close(...)on every node (21 times), while protocol modules never use it, indicating closed is the natural default.Key Changes
The document specifies:
{ prefix }/{ props, rest: never }(closed), whileopen(c)normalizes to the current open formclose(c, rest?)becomesrest(c, r)with required second parameter, eliminating theundefinedsentinel collision'close'→'rest'throughout the ADT with corresponding type renames (InfoRest,Rest,_MakeRest,RestTs)null, undefined, false, true), observable only at container positionsStructTsrenders optional keys asa?: T; for a closed tupleTs<[1, or(option, number)]>is exactlyreadonly [1, number?], verified against the repo'stscin both directionsImplementation Notes
objectMayOmitis a straight bit substitution:trimPrefixmoves its test to the trailing declared position, and the declared-key drop keeps its "rest is gone" guard whileisTopbecomes position-aware. (An earlier revision of this description claimed only two sites change; review found that wrong and the document is corrected.)array(or(option, number))normalize rather than error, matching set-theoretic principles{ members, rest? }structure; onlytoDatamapping changesRelated Issues
parse-omits-undefined-membersdefect (fjs/types/rtti/todo/parse-omits-undefined-members.md), deleted by Stage 2schema-walk-own-indicesfor consistent tuple handlingfjs/types/rtti/data/README.mdtoData(array(or()))andtoData(close([]))are the sameNode, yet the thunk readers acceptnew Array(1)for one and reject it for the otherChangelog: none
https://claude.ai/code/session_01MQwnUe7SYoUFXchmAfx8Yf