package: stop shipping generated private declarations - #1771
Conversation
Stage 2 of the private-types design, and the last of it. A `!**/private.d.ts` negation in `package.json`'s `files` excludes the generated declarations; `prepack` is unchanged and the working tree is left alone, so a contributor who runs `npm pack` does not silently lose declarations a following `npx tsc` expects. The invariant the design asked for, measured rather than assumed: 679 packed files become 663, the 16 that disappear are all `private.d.ts`, and nothing else moves in either direction. The packed-declaration type-check merged in #1767 cannot guard this on its own. With `private.d.ts` shipped, every reference to it resolves and that job is green — so a dropped negation is invisible to it. The Node job therefore also asserts what was packed, from `npm pack --json`, npm's own account of the tarball. The two fail on opposite inputs: - negation dropped, private dependency present → contents assertion red, type-check green; - negation in place, private dependency present → type-check red (TS2307), contents assertion green. Both measured end to end before this landed. The second used `fjs/emergent_testing`, a module with no `private.ts` and not the package fixture, because a violation anywhere a hand-written import list would already look proves the check can fail but says nothing about whether the file set was enumerated. `node` for the assertion rather than a text search: the paths arrive as JSON and are compared as whole filenames, so nothing can mistake a path containing the name for one ending in it. AGENTS.md §6 asks for a tool that parses what it checks; here that is the runtime this repository is written in, already running in every job. `fjs/fsc/README.md` no longer tolerates a shipped `private.d.ts`, since none ships. The permanent half of the contract stays: `_` names still reach the declarations that do ship and are still not API. `fjs/todo/separate-private-types.md` is deleted, as it specified. Its thirteen inbound references across seven documents are retargeted to where the rules now live — root `AGENTS.md`, `fjs/AGENTS.md` §3.2, and `fjs/fsc/README.md` — rather than left dangling, and the measurements two CI todos cited from it are written into those todos. 3480/3480, coverage 100%, tsc clean, ci-update round-trips. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | 7616cae | Commit Preview URL Branch Preview URL |
Aug 29 2026, 04:18 AM |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aa7513f16
ℹ️ 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".
One conflict, resolved by taking main's side: #1769 deleted `fjs/todo/module-tag-restore.md`, which this branch had edited only to retarget a link at the design document Stage 2 removes. With the file gone there is no link left to dangle, so the edit had nothing left to do. That PR also settled the paragraph in `fjs/AGENTS.md` this branch touches. The two agree: `private.ts` keeps `@module` because its prose is for contributors reading the sources, and its generated declarations are not packaged — which this branch turns from a plan into a fact. 3480/3480, coverage 100%, tsc clean, ci-update round-trips. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
The negation was written, measured, and then lost before it was committed. The negative control set `files` back to its unnegated value to show the assertion fails, and restored it with `git checkout -- package.json` — which restores from HEAD, not from the working tree, so it discarded the change instead of the control's. Every measurement in the previous commit was real when taken; none of them was re-taken against the tree that was committed. So the branch enforced the assertion without making it satisfiable: `prepack` emits the 16 `private.d.ts`, the positive `**/*.d.ts` allowlist packs them, and the new step exits 1 before the upload. CI had not yet dispatched a run, so the red never appeared; Codex read the diff and found it. Verified against the committed tree this time, not the working tree it was measured in: the generated node26 steps run clean, 663 files packed, 0 `private.d.ts`. Co-Authored-By: Claude Opus 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: 470b32992c
ℹ️ 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".
It guarded a dropped `files` negation, which the packed-declaration type-check cannot see: with `private.d.ts` shipped, every reference to it resolves and that job is green. But the consequence it prevented is declaration noise in the tarball — the state shipped until this branch, and one the `_` contract already tolerates — not a broken package. The check that catches breakage is the type-check, and it stays. Against that, the cost was real and kept arriving. It produced three defects in three commits: it enforced an exclusion the branch had lost, so nothing could pass; its predicate was a suffix test over the whole path, which also flags `notprivate.d.ts` — a name the negation deliberately keeps, so the job would have failed on a legal file and banned a filename nothing documents; and its own doc comment could not contain `**/` without ending the block. Root AGENTS.md §6: "Leaving the check undone is the better trade against that complexity. A rule no available tool can express stays written down and unenforced. That is honest, and cheaper than machinery whose failures are silent." The negation is one line in `package.json` and losing it is a visible diff in review. `fjs/ci/**` and `.gitignore` are byte-identical to main again; what remains on this branch is the negation, the documentation, and the retired design. `fjs/ci/todo/f-mjs-package-support.md` records that a dropped negation is now unguarded, and why that was accepted. 3479/3479, coverage 100%, tsc clean, ci-update round-trips. Package still packs 663 files with no `private.d.ts`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
A generated `.d.mts` beside a `.mjs` shadows it: TypeScript reads the declaration and never checks the source. Declarations are gitignored, so CI clones without them and checks the sources, while a working tree that has run `prepack` once keeps them — and `npx tsc` then reports success for code it did not open. Found the hard way on this branch. An earlier commit inserted a const between a `/** @type {readonly MetaStep[]} */` annotation and the declaration it belonged to, so the annotation landed on a string. Local `npx tsc`: exit 0. CI: TS2322 on that line. Same tree and same compiler; deleting the generated declarations is the only difference, and it turns the local run red too. Filed rather than fixed, because the fix is a real choice — clear declarations in the gate and pay a cold emit every time, emit to a `declarationDir` and change the package layout, or document the trap and leave the tool alone. Co-Authored-By: Claude Opus 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: 46f2082b4a
ℹ️ 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/fsc/README.md` claimed CI asserts both halves of the exclusion — that the tarball carries no `private.d.ts` and that what it does carry type-checks. The first half stopped being true when the packed-listing assertion was removed one commit earlier, and the sentence was left behind. It now says what holds: the type-check covers the consequence, and losing the negation is not caught, because the private declarations come back and every reference to them resolves. The reasoning for accepting that gap is one link away rather than restated. Co-Authored-By: Claude Opus 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: 2e68cc944c
ℹ️ 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".
`changelog/README.md` asks for about three wrapped lines, ~250 characters. The entry was five lines and 370, and the excess was rationale — why the files were private, and what CI checks about the package — which the same guidance sends to the pull request description or the relevant README. Both are already there. What is left is the user-visible change: the files are gone, and depending on them was never supported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approved.
The negation is exactly scoped: 677 files to 661, the 16 removed are precisely the 16 private.d.ts, nothing else appears or disappears, and there is no private.d.mts variant for it to miss.
The question I most wanted answered is whether the package still type-checks for a consumer once those declarations stop shipping, since that is what makes package-check able to bite for the first time. Replicating the job's exact recipe — real npm pack, fresh consumer outside the repo, typescript@=7.0.2, skipLibCheck: false — gives exit 0 with 378 declarations actually processed, so it is a real pass rather than a no-op. And it passes for a reason: no shipped declaration imports ./private.ts as a live type-checked import; the only occurrences are inside @module JSDoc prose, which is inert. So this is not the check quietly tolerating unfinished work.
The −621 lines are entirely documentation — no .ts or .mjs file is touched, which is also why the proof set is provably identical and npm test reads 3541/3541 on both sides. tsc 0.
On the mutant: removing the negation is caught by nothing — npm test stays at 3541/3541 and package-check stays green, because the private declarations come back and every reference resolves again. Worth saying that you documented this yourself in fsc/README.md rather than leaving it to be discovered, and the reasoning holds: losing one line in files is a visible diff in review, which is a different risk from a check that silently stops meaning anything.
No breaking marker, consistent with the pre-existing policy that _-prefixed exports are private by contract regardless of what TypeScript emits. That is a call made in an earlier stage; I verified it is the standing policy rather than re-arguing it here.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b02b188bd
ℹ️ 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".
`changelog/README.md` asks for the `Topic: short description` form, and every other entry in `unreleased/` carries one — `rtti`, `effects`, `web`, `emergent_testing`, `fjs/fsc`. This one started with the change itself, so a reader scanning a release's notes had nothing to sort it by. `package`, matching what the change is about: what the published npm package contains. 214 characters over three lines, still inside the documented limit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approved. The topic prefix matches the sibling entries — emergent_testing:, website/…:, fjs/types/rtti/ all lead the same way — and the rewrite keeps the substance while shedding the redundant "from the package". Everything else in the delta is merge traffic; the net change lines are 759 on both sides.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06e48d3d5e
ℹ️ 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".
`todo/README.md`: "Issue headings should be short and direct — describe the action or the thing, not the full context", and its template is a plain `## Title`. This heading carried a repeat of the filename slug followed by the whole symptom. `Generated declarations shadow their sources` is the thing. The symptom it replaced — a local `npx tsc` passing on an error CI reports — is the first paragraph of `### Problem`, where the context belongs. Sibling files under `fjs/*/todo/` do use a `slug. description` heading, which is what this one was modelled on; the documented rule is the plain title, so the habit is not evidence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approved. The heading now names the issue rather than the symptom — a generated declaration shadowing its source is the cause; npx tsc passing locally was the effect.
Stage 2's last step, and the one the previous four existed for. Steps 1 (#1762), 2 (#1763) and 3 (#1767) built the guard;
package-checkbecame a required check; this is the change it guards.prepackis unchanged and the working tree is left alone, so a contributor who runsnpm packdoes not silently lose declarations a followingnpx tscexpects.No CI change.
fjs/ci/**,.github/workflows/ci.ymland.gitignoreare byte-identical to main. The rest is documentation.The invariant, measured
677 packed files become 661. The 16 that disappear are all
private.d.ts, and nothing is added — set difference overnpm pack --json, not a subtraction of two totals. Re-measured on the current head; the totals drift as main adds modules, so the invariant is the claim, not the absolute figures.What guards it, and what does not
package-checktype-checks every packed declaration from the installed artifact with no checkout. That is what catches the failure that matters: a shipped declaration depending on a private module the tarball no longer carries. Measured withfjs/emergent_testing— a module with noprivate.ts, and deliberately not the package fixture, because a violation anywhere a hand-written import list would already look proves the check can fail but says nothing about whether the file set was enumerated. Given a private type and an exported binding whose signature names it, the job exits 2 withTS2307from the packed artifact.A dropped
filesnegation is not guarded, and that is a deliberate choice. The type-check goes green in that case — withprivate.d.tsshipped, every reference resolves — so nothing detects it. An assertion over the packed listing was written for exactly that and then removed:_contract already tolerates, not a broken package;notprivate.d.ts, and its own doc comment could not contain**/without ending the block;AGENTS.md§6: "Leaving the check undone is the better trade against that complexity."The negation is one line and losing it is a visible diff in review.
fjs/ci/todo/f-mjs-package-support.mdrecords the gap and the reasoning next to the measurement, so the next reader finds it rather than assuming coverage.Documentation
fjs/fsc/README.mdno longer tolerates a shippedprivate.d.ts, since none ships. The permanent half of the contract stays:_names still reach the declarations that do ship and are still not API.fjs/todo/separate-private-types.mdis deleted, as it specified. It had 13 inbound references across 7 documents; rather than leave them dangling, each is retargeted to where the rule now lives — rootAGENTS.md,fjs/AGENTS.md§3.2,fjs/fsc/README.md— and the measurements the CI todos cited from it are written into those todos.Not a breaking change
By the contract Stage 1 established and
fjs/fsc/README.mddocuments: consumers must not depend on a shippedprivate.d.ts.types.tsmay not depend onprivate.ts, so no public declaration semantically needed one.A trap found on the way, filed separately
todo/local-tsc-skips-mjs-with-a-generated-declaration.md. A generated.d.mtsbeside a.mjsshadows it, so localnpx tscreports success for source it never opened; CI clones without declarations and checks them. This branch pushed a realTS2322past a clean local run because of it. Same tree, same compiler, declarations deleted — the local run goes red too. Filed rather than fixed: the options are clearing declarations in the gate (a cold emit on every check), adeclarationDir(changes the package layout), or documenting it.Checks
On the current head, with main merged in:
npx tscclean with generated declarations cleared, the condition CI runs in; 3482/3482; coverage 100%;npm run ci-updateround-trips; package packs 661 files with noprivate.d.ts. All 20 CI checks green.🤖 Generated with Claude Code
https://claude.ai/code/session_01LqeS5t2ZKkSu3chRPMXR7n