fjs: keep private types out of authored .mjs (Stage 1) - #1750
Conversation
Implement Stage 1 of fjs/todo/separate-private-types.md: no authored .mjs anywhere in the repository carries a file-scope JSDoc typedef any more; function-local typedefs remain allowed. Each former file-scope typedef moved to its sibling types.ts when it is part of the public declaration closure, to a new optional private.ts when implementation-private and worth naming (asn.1, bnf, common/monoid, djs, effects/node, rtti, types/bigfloat, types/btree/remove), inline into its annotations when trivial, or function-local into a proof when it is a compile-time assert (edag, effects, js/keywords, rtti, djs/parser, media/json/schema, media/revision). fjs/effects/types.ts no longer imports implementation functions: its ReturnType signature asserts now live in fjs/effects/proof.f.mjs. Recursive RTTI constants stay in module.f.mjs with their consistency asserts moved downstream. Breaking type import-path moves, with importers updated and no compatibility re-exports: Grammar (fjs/fsm) and MemoryOperationMap, MemoryRun, Uuid (fjs/effects/node/memory) moved to sibling types.ts files; the Unknown alias of fjs/media/json/schema is now spelled Ts<typeof unknown>. Policy documentation updated: root AGENTS.md and fjs/AGENTS.md state the prohibition and placement rules; fjs/fsc/README.md's typedef prescription is rewritten as "Private types"; the blocked wait-for-@internal/stripInternal TODO is deleted as superseded; the typedef prescriptions in todo/migrate-typescript-to-mjs.md, fjs/ci/todo/f-mjs-package-support.md, and fjs/effects/memory/todo/sync-interpreter-owner.md are retargeted to the Stage 1 forms. Stage 1 checkboxes in the design TODO are checked; the file stays for Stage 2 (packaging cleanup). npx tsc clean; full suite 3487/3487. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | 7695ae3 | Commit Preview URL Branch Preview URL |
Aug 28 2026, 03:32 PM |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Values referenced only through typeof in consistency-proof typedefs do not need a runtime import; the JSDoc @import serves the type position and the code-quality unused-import finding goes away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e934de8ed1
ℹ️ 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".
fjs/AGENTS.md section 2 reserves @module for module.f.mjs / module.mjs entry points; strip it from the type files this branch added. The pre-existing types.ts files that carry the tag are recorded as debt in fjs/todo/module-tag-on-types-ts.md rather than widened into this change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eebc0078a0
ℹ️ 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".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Measured at 141b6204f on node v23.11.0. The leak is genuinely closed, and I measured it rather than trusting the sweep: after purging declarations and running the repo's own emit, main produces 226 export type _* occurrences across 63 .d.mts files, and this head produces 0. That is the claim this PR exists to make, and it holds.
No coverage lost either, which matters at 1733 deleted lines: comparing fully-qualified leaf names (module + path, since bare names collide across files), zero disappear; the +9 are genuinely new proofs. Gates 3487/3487 exit 0, tsc --noEmit exit 0 (main 3478/3478).
Spot-checked fjs/djs/parser and fjs/rtti/data: both private.ts files use import type only and depend on their sibling types.ts, never the reverse — the direction the design requires, no cycle. The parser's tokenKindNames/framingKeywords becoming _-prefixed runtime exports so the proof can re-verify them is deliberate and documented as linkage rather than API; consistent with the private-constant convention.
One substantive point: Stage 1 is a sweep, not a check. I added /** @typedef {number} _Probe */ to fjs/path/module.f.mjs — a file this PR does not touch — and tsc --noEmit, npm test (3487/3487) and npm run prepack all exit 0 while _Probe leaks straight into the emitted fjs/path/module.f.d.mts. So the 226→0 result is real but unguarded: nothing prevents the next file-scope typedef from putting it back. The design document does frame verification as a repo-wide search, so this is consistent rather than contradictory — but a one-line grep in CI would turn a state that happens to be clean into one that stays clean, and it is much cheaper to add now than after the next regression.
Minor: the entry is 438 characters against the ~250 guideline in changelog/README.md — soft wording, so a nit. The breaking marker is right: moved public types (Grammar, MemoryOperationMap, MemoryRun, Uuid, BrowserTestReport) change import path and the Unknown alias is removed.
The node26 invariants job now fails when any authored .mjs contains a file-scope JSDoc typedef, so the 226-to-0 declaration-leak result stays guarded instead of merely achieved. Verified against a probe typedef in an untouched module: clean tree passes, probe fails. The changelog entry is trimmed toward the ~250-character guideline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
|
Both review points are addressed in f0ca31b:
Thanks for measuring 226→0 and the coverage-by-qualified-leaf comparison — that's exactly the claim this PR needed checked independently. Generated by Claude Code |
ci.yml is generated by npm run ci-update, so the guard added directly to it was erased by regeneration and failed the diff check. The step now comes from node26Steps in fjs/ci/node/module.f.mjs; the regenerated ci.yml is byte-identical to the previous hand edit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approved. The guard works, and I tested it rather than taking its presence as proof: re-running my probe at ec77b6293 — /** @typedef {number} _Probe */ in untouched fjs/path/module.f.mjs — npm test still exits 0 (3487/3487) and tsc --noEmit exits 0, but the CI grep exits 1 and catches it. No false positive on the clean tree, and ci.yml genuinely round-trips: node ./fjs/module.mjs ci regenerates it byte-identical.
One narrow gap, non-blocking: the pattern requires a literal space before the continuation * ( \* *@typedef), so a JSDoc block indented with a tab slips past — /**\n\t* @typedef … \n */ is not caught, while the no-space form /**@typedef …*/ and an ordinary multi-line block both are. Changing that space to \s closes it.
The guard required a literal space before the continuation asterisk, so a JSDoc block indented with a tab slipped past; \s closes that gap without matching function-local blocks, whose continuations are indented by four or more characters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
|
The tab gap is closed in 4b8c1d9: the continuation pattern is now Generated by Claude Code |
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approved.
Verified the tab-indentation fix: the guard now catches a tab-indented @typedef continuation, does not fire on prose that merely mentions @typedef, and ci.yml still round-trips byte-identical through node ./fjs/module.mjs ci.
Resolves the conflicts the merge queue rejected. main's option-as-omission work (`option` as a nullary schema, `or(option, t)`), its hardened parse rebuilds, and its new reporter/RunTotals types all land here; the Stage 1 transformation is re-applied on top of them: - fjs/rtti/parse: main's new recursive `_Entries`/`_Rebuild`/`_Declared` move to a new private.ts; the dead `_ItemResult` goes. - fjs/rtti/data/proof: the shared recursive schemas are declared inside a `recursiveSchemas()` factory and destructured back out, so their typedefs are function-local and every use site is unchanged. - fjs/rtti/ts/proof, fjs/rtti/validate/proof, fjs/media/json/schema: main's rewritten content with the typedefs moved into proof entries; validate's rewritten `rows` table is spliced into the existing thunk. - fjs/emergent_testing/types.ts keeps both sides' additions. - docs: main's `todo/rtti-type-system.md` prose and the `fjs/AGENTS.md` assert example retargeted to the Stage 1 forms. npx tsc clean; npm run ci-update regenerates nothing; full suite 3517/3517; the file-scope typedef guard passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
main's ENOTDIR re-check added a use of the `_TooLarge`/`_NotRegular` typedefs this branch inlined away; spell them inline there too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
The mutually-recursive partners (orB, t2, w, topOr, b2, closedChildren, absCycleB, optNumB) are reached through their counterparts inside the factory, so destructuring them at file scope left them unused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
…d claim Three review findings, all against text I wrote in the previous commit. §1.2 still showed a `proof.f.mjs` leading block carrying `@module`, while the §2 I had just rewritten said a proof's documentation is never published so the tag has nothing to attach to. Same file, opposite instructions for the same shape. The example loses the tag and says why. The restore issue omitted `private.ts`. All 16 have prose and none carry the tag — stripped by #1750 rather than #1756, so they were not in the 102 I counted, but it is the same reading and the same fix, and §2 names `private.ts` explicitly. As written the issue could have been completed in full while leaving 16 files violating the rule it exists to satisfy, and its last task says to delete §2's non-compliance note. And the publication rationale was overstated. The website issue plans `deno doc --html **/module.f.mjs`, a glob excluding every `types.ts` and `private.ts`, so restoring the tag alone puts nothing on the website. The tag is necessary, not sufficient: it decides whether `deno doc` *can* see a file's module documentation, and what the build is pointed at is a separate question. Both §2 and the issue say that now, and widening the glob is named as the other half. Also added: a task to correct `todo/migrate-typescript-to-mjs.md`, which restates the old rule verbatim under "Module header and import ordering" rather than linking §2, so it did not move when §2 did. Documentation only. `npx tsc` clean, suite 3533/3533. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
3 participants
Implements Stage 1 of
fjs/todo/separate-private-types.md: no authored.mjsanywhere in the repository carries a file-scope JSDoc@typedefany more (232 removed across 63 files); function-local typedefs remain allowed and are the home for compile-time proofs. Thenode26CI job now guards the prohibition, so the clean state stays clean.What moved where
types.ts:_Byte(byte_set),_Base(bit_vec),_Signature(crypto/sign),_CompileOp(djs),_EmptyTagMap(bnf/data),_RefCounter/_Refs(djs/serializer),BrowserTestReport(emergent_testing); newtypes.tsfiles forfjs/fsm,fjs/fsc,fjs/djs/serializer, andfjs/effects/node/memory.private.tswhere separating implementation-private types reads cleaner than inlining:asn.1,bnf(4),common/monoid,djs(3),effects/node,rtti/data,rtti/ts,types/bigfloat,types/btree/remove.Assert<Check<...>>checks became aconsistencyproof entry,fjs/effects/types.tsno longer imports implementation functions (itsReturnTypesignature asserts moved to asignaturesentry inproof.f.mjs), andfjs/media/revision's pins moved to its proof — per the TODO's dependency-order rule.module.f.mjs(fjs/edag,fjs/media/json/schema) with inline/hand-written annotations; values needed only for proof linkage are exported with a leading_(linkage, not API) or referenced through type-only@import. Nometa/module.f.mjssplit was warranted; the option is documented.Policy documentation
Root
AGENTS.mdandfjs/AGENTS.md§3.2 state the prohibition and placement rules with live examples;fjs/fsc/README.md's "Private JSDoc typedefs" is rewritten as "Private types"; the blocked wait-for-@internal/stripInternalTODO is deleted as superseded; the prescriptions intodo/migrate-typescript-to-mjs.md,fjs/ci/todo/f-mjs-package-support.md, andfjs/effects/memory/todo/sync-interpreter-owner.mdare retargeted to the Stage 1 forms. The design TODO's Stage 1 checkboxes are checked; the file stays for Stage 2 (packaging cleanup: unshipping generatedprivate.d.ts). New.tshelper files carry no@moduletag; the pre-existingtypes.tsfiles that do are recorded as debt infjs/todo/module-tag-on-types-ts.md.Verification
npx tscclean;npm run ci-updateregenerates nothing; full suite 3487/3487 (node), with deno/bun covered by CI. Independently measured in review:export type _*occurrences in emitted declarations drop from 226 onmainto 0 on this head, with zero proof coverage lost. The newnode26guard step fails CI on any future file-scope typedef. Importers of every moved public type were updated in this PR; no compatibility re-exports.Changelog
Grammar→fjs/fsm/types.ts;MemoryOperationMap,MemoryRun,Uuid→fjs/effects/node/memory/types.ts;BrowserTestReport→fjs/emergent_testing/types.ts; the JSON-SchemaUnknownalias is gone — spell itTs<typeof unknown>.🤖 Generated with Claude Code
https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n