todo: separate private types into private.ts - #1740

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dc1b5f3ad2

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fbe4f4811b

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1154ec6a2

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: beef545757

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9bddb33754

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 18ab3d91a2

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3ddac53c1e

One thing to address before this lands.

The document proposes eliminating a leak that this repo already decided to tolerate, without saying so. fjs/fsc/README.md:129-150 records the underscore prefix as a deliberate interim convention — "The underscore is an API contract, not declaration-level visibility… renaming or removing a _-prefixed alias is not a breaking change" — and ties the wait to microsoft/TypeScript#46407 for @internal/stripInternal. This proposal argues for physical separation instead, but never cites that rule, the upstream issue, or why physical separation is now preferred over waiting. I checked at the current head 32eec933d: separate-private-types.md contains no mention of fsc/README, 46407, stripInternal or @internal. A reader arriving from fsc/README.md will find two live answers and no way to tell which governs.

The premises themselves hold — I verified them at 3ddac53c1: a file-scope JSDoc @typedef does surface as export type _Name in generated declarations (fjs/crypto/sha2/module.f.d.mts:17-19 yields _V3, _V4, _BaseInit), private types in types.ts do ship (fjs/asn.1/types.d.ts:11 exports _Tag), and the quoted prepack script matches package.json:13 exactly. All citations and relative links resolve, the shape matches its fjs/todo/*.md siblings, Status: open with everything unchecked, and omitting the changelog is correct for a todo/-only PR. Gates at 3ddac53c1: npm test 3470/3470 exit 0, tsc --noEmit exit 0, identical to main.

Note those verifications predate the rewrite in c64c52e19/de513977f/32eec933d, which reworked 232 lines — I have not re-checked the premises against the new text, only the missing reconciliation above.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 89e33c10ec

Approved. The reconciliation closes my finding: the document now cites fjs/fsc/README.md:129-150, says plainly which policy governs today — "That policy remains authoritative until this migration is implemented" — and carries the fix as a real unchecked task (- [ ] Reconcile the implemented convention with the current private-JSDoc policy: update fjs/fsc/README.md …, line 315) echoed in the acceptance criteria, rather than as prose. Verified at 89e33c10e: npm test 3472/3472 exit 0 and tsc --noEmit exit 0, identical to main; the branch's own diff against main is still just fjs/todo/separate-private-types.md; all six links resolve, and microsoft/TypeScript#46407 is indeed open ("Support stripInternal for types in JSDoc").

The "moving a meta constant is a breaking change" rule is correct, and if anything understated: package.json has no exports map and no main, only files globs, so consumers import by literal deep path and there is no subpath indirection that could preserve the old specifier.

One small asymmetry, not worth holding the PR for: the pointer is one-directional. fsc/README.md is untouched and still states the old policy unqualified, so a reader who lands there first gets no hint it is under active reconsideration. Fine while the todo owns the change — a one-line forward reference would close the loop.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0401908595

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8622f5a1cf

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 59c146826b

Approval still stands. Verified at 8622f5a1c: npm test 3472/3472 exit 0, tsc --noEmit exit 0, branch diff against main still only fjs/todo/separate-private-types.md, no changelog owed, Status: open with 21 unchecked tasks.

Both commitments survive the third rewrite: "That policy remains authoritative until this migration is implemented" at line 251, and the fjs/fsc/README.md update plus deleting-or-narrowing todo/blocked/jsdoc-typedef-strip-internal.md still an unchecked task at lines 276-278.

The widening from .f.mjs to all .mjs is true, and I checked it by emitting rather than reasoning: after purging declarations and running tsc --noEmit false --emitDeclarationOnly, fjs/website/browser-prepare.mjs:28 — a plain .mjs — has a file-scope @typedef … _Module and fjs/website/browser-prepare.d.mts:2 emits export type _Module. Same leak in effects/node/module.mjs (_Socket, _Readable) and effects/node/memory/module.mjs.

One thing worth naming for the record: the meta.f.mjs clarification is forward-looking — no meta.f.mjs exists in the repo yet, so it is a self-consistency fix to the document's earlier "coverage must include it" wording rather than a claim about current behaviour. It contradicts nothing, but it is not empirically checkable today.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0a948de0bc

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 01e4309176

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0d839d99bf

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 114fd58b7b

Approval still stands. Verified at 114fd58b7: npm test 3472/3472 exit 0, tsc --noEmit exit 0, branch diff still only fjs/todo/separate-private-types.md, no changelog owed, links resolve and shape matches its siblings.

Both commitments survive the fourth rewrite: "That policy remains authoritative until this migration is implemented" at line 325, and the fjs/fsc/README.md update plus deleting-or-narrowing todo/blocked/jsdoc-typedef-strip-internal.md still an unchecked task at lines 356-358.

The repo-wide widening is true and was tested on the category it newly covers, not assumed: emitting declarations for todo/proof.f.mjs — a file outside fjs/ — leaks export type _TemplateType into todo/proof.f.d.mts.

The underscore rule for meta constants does not fork the prefix's meaning: fsc/README.md:143 says "The underscore is an API contract, not declaration-level visibility", and the new meta.f.mjs section uses exactly that sense — private by contract, renaming not breaking.

The @import-as-comment rule holds in the compliant case, and the check that convinced me is the failing one: when the imported private type appears only in a non-public position, TypeScript emits it as a comment and a consumer typechecks clean after deleting private.ts; when it leaks into a public signature, TypeScript synthesizes a real import type that breaks resolution — which is precisely the case the document's own public-declaration-closure rule forbids and its validation task exists to catch. The dependency-order claim checks out too: fjs/effects/types.ts:11-13 imports six step functions from ./module.f.mjs purely for ReturnType<typeof …>, a real reverse edge.

Withdrawing my approval at this head — the two things it rested on are no longer there. Measured at 21c444e0e; gates are fine (npm test 3477/3477 exit 0, tsc --noEmit exit 0, same as main; branch diff still only this file; no changelog owed).

1. The "which policy governs today" sentence is gone. "That policy remains authoritative until this migration is implemented" was at line 325 at 114fd58b7; grep -n authoritative across both this document and fjs/fsc/README.md now returns nothing. That sentence was the whole answer to my original finding — without it a reader arriving at fsc/README.md is back to two live policies and no tiebreak.

2. The reconciliation task is no longer falsifiable. It used to name the file and the action: "Update fjs/fsc/README.md and delete or narrow todo/blocked/jsdoc-typedef-strip-internal.md so they no longer prescribe a conflicting private-JSDoc strategy." It is now a generic checkbox at line 241, "Update root/fjs policy documentation and reconcile the old _ leak policy", with the specifics demoted to prose. A reviewer cannot tell whether that box has been earned.

Restoring both — the authoritative-until sentence, and the task with its file names and the delete-or-narrow action — puts this back where I approved it.

Separately, worth deciding deliberately rather than by drift: 1e69c5ac5 and e785b3b61 turn private.ts and meta/ from convention into "optional tool… use only when useful". The two hard rules survive unweakened — no file-scope @typedef in authored .mjs, and types.ts must not depend on private.ts — so the document is still actionable. But placement is now discretionary, which means a future PR cannot be failed for not using private.ts where it would have helped. If that is the intent, fine; if the point was a convention, the softening undoes it.

The rest checks out: the intra-module chain in 21c444e0e is accurate, the fjs/effects/types.ts:11-13 reverse edge is still present and still true with its task unchecked, all eight links resolve, and Status: open with 15 unchecked tasks.