feat(types): deep public-type audit gate (SD-2977)#3185
Draft
caio-pizzol wants to merge 7 commits intomainfrom
Draft
feat(types): deep public-type audit gate (SD-2977)#3185caio-pizzol wants to merge 7 commits intomainfrom
caio-pizzol wants to merge 7 commits intomainfrom
Conversation
The existing public-type checks lock in exported type names and top-level `any` assertions, but they do not catch `any` on public members, callback params, return types, or nested type arguments. Consumers can still lose IntelliSense after touching a public API surface that resolves to `any`. Add a deep public-type audit that builds a TypeScript Program from the packed-and-installed `superdoc` tarball, recursively walks every owned type reachable from public export entries, and fails CI if a finding is not in `deep-type-audit.allowlist.json`. Also fails on stale allowlist entries (so fixes must remove their entry), on compiler diagnostics in the published surface, and on private `@superdoc/*` specifiers that survived rewriting in the installed package. Allowlist seeded with the current 293 owned findings, tagged by tier so follow-up PRs can drain by surface (Pinia stores, toolbar config, helpers, curated public contract, other). The gate runs in PR CI and release CI, after the consumer matrix prepares the installed tarball. Closes the gap where release publish did not run packed consumer checks. The allowlist is a starting line, not a waiver. Per the README: do not drain by replacing `any` with `unknown` unless the value is genuinely opaque - prefer precise upstream or local types so IntelliSense actually restores. Verified locally: - node tests/consumer-typecheck/typecheck-matrix.mjs: 53 passed - node tests/consumer-typecheck/deep-type-audit.mjs: 293 findings, 0 new, 0 stale - removing an allowlist entry produces NEW finding output and exit 1
![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: 24a87851bd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…SD-2977) Codex review on the initial PR pointed out two walker blind spots: 1. The walker only visited `getCallSignatures()`, so a public `constructor(...args: any[])` (SuperConverter, DocxZipper, and others) shipped without producing a finding. 2. The walker only enumerated `getProperties()`, so `[key: string]: any` index signatures on public classes were never traversed. Both gaps confirmed in the installed dist before the fix. Walker now visits construct signatures alongside call signatures and queries `getStringIndexType()` / `getNumberIndexType()` for index members. New findings are tagged `tier-4-public-contract` for SuperConverter and DocxZipper (per SD-2966's done-when criteria) and `tier-5-other` for the remaining constructor-surface findings. Allowlist regenerated: 293 → 337 entries. Verified: - node tests/consumer-typecheck/deep-type-audit.mjs: 337/0/0 PASS - SuperConverter[string] and DocxZipper[string] now in tier-4
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code review surfaced three coverage gaps in the seeded allowlist: 1. Sibling members sharing an instantiated container type (`a: any[]; b: any[]`) were path-order-dependent. TypeScript caches `Array<any>` and `Promise<any>` per-shape so siblings share a type id; the persistent-visited gate short-circuited the second sibling before its inner `any` could be recorded. Fix: pre-record `any` inside array elements and type arguments BEFORE the visited gate. Cycle protection still applies to structural members. 2. Class exports walked only the instance side (getDeclaredTypeOfSymbol), so `constructor(...args: any[])` and `static foo(): any` on public classes (SuperConverter, DocxZipper) never produced a finding. Fix: walkExport now also walks the value type (getTypeOfSymbolAtLocation) when it differs from the declared type, prefixed with `.<value>` so constructor / static findings are clearly distinguished. 3. `--pack` bootstrap failed on fresh checkouts because typescript was resolved at module-load (line 49) before the doPack block (line 52) that runs the fixture install. Fix: move the typescript require after the optional pack-and-install block so the documented bootstrap command actually works on a clean tree. A stack-scoped visited variant was tried first but caused combinatorial blow-up on highly interconnected types (16+ minutes with no progress on the public surface). The pre-record + persistent-visited combination keeps cycle protection bounded while still catching sibling regressions. Allowlist regenerated: 337 -> 678 entries, with new tier-4 entries covering SuperConverter / DocxZipper class-side `any`s and tier-5 absorbing the long tail of sibling-shared findings. Verified: - node tests/consumer-typecheck/deep-type-audit.mjs: 678/0/0 PASS - SuperConverter.<value>.new(args)<0>, .new(args)[], static methods all in tier-4 - DocxZipper.<value>.new(args)<0>, .new(args)[] all in tier-4 - Run completes well under 60 seconds
A third review round flagged three potential walker issues. Two were stale (P1: SuperConverter/DocxZipper class-side findings — already caught in fcf84a2, 16 entries in allowlist; P2: visited-singleton-any skip — already prevented by isAnyType returning before the visited gate in walkType lines 220-223). The third is real: MAX_DEPTH=8 silently truncated 302,642 paths in a single audit run, leaving deep public types invisible to the gate and contradicting the README's claim of walking every reachable type. Persistent visited handles cycles, but TypeScript materializes generic instantiations on demand with fresh type ids that visited cannot dedupe, so the cap is load-bearing memory protection (cap=256 OOM'd at ~4GB). Empirical sweet spot at MAX_DEPTH=16: deep enough to reach 451 more legitimate findings (allowlist 678 -> 1129), shallow enough to bound memory. depthCapHits counter now surfaces in the run report so any remaining truncation is visible rather than silent. The new findings are concentrated in pinia store internals, vue reactive chains, and prosemirror type expansions — surface areas the team already knows are noisy. The cap warning lets future maintainers decide whether further bumps are worth the memory cost. Verified: - node tests/consumer-typecheck/deep-type-audit.mjs: 1129/0/0 PASS - WARN line surfaces depth-cap hits in the report - No OOM at cap=16
Three small cleanups bundled:
1. Strip em dashes from README and code comments. User CLAUDE.md
prohibits em dashes in all output including code comments and docs;
replaced with ":", "(", or "." per surrounding grammar.
2. Drop "flagged by Codex on the initial PR, fixed here" trailing
clauses on two comments. The surrounding rationale (call sigs vs
construct sigs; index sigs vs properties) is load-bearing and stays;
only the review-process narration goes. CLAUDE.md says comments
should not reference the current task or fix.
3. Fix walkExport's stale "stack-scoped" comment + add visited
snapshot/restore around the value-side walk. The previous comment
claimed visited pops on exit (the failed try/finally variant), but
the actual code uses persistent per-root visited. That meant
structural types reachable from both an export's instance and value
sides were silently skipped on the value side. Snapshot/restore
scopes the value walk's visited freshly without polluting subsequent
exports' declared walks.
The visited fix surfaced 670 previously-hidden findings (1129 -> 1799),
990 of which live on .<value> paths (class value side). Composition
reinforces the umbrella framing: most additional findings are Pinia
store internals and other surfaces that SD-2966's facade should hide,
not type.
Verified:
- node tests/consumer-typecheck/deep-type-audit.mjs: 1799/0/0 PASS
- 0 em dashes remaining in either file
- 0 "flagged by Codex" comments remaining
Two related walker gaps (sig.thisParameter and `<T = any>` defaults)
were investigated and confirmed real but have zero current matches in
the published superdoc dist; deferred as future-hardening rather than
landed pre-emptively.
…post-SD-2966 (SD-2977) Three independent code-review opinions converged on the same conclusion: landing a 17K-line allowlist of accidental public surface is the wrong move. The current 1799 findings are largely from Pinia stores, EventEmitter generics, Vue SFC component types, and other code that was never deliberately committed as public API. Committing them as an allowlist would risk legitimizing internals as the type contract and forcing the team to type things that should be hidden. Pivot: 1. Delete the 17K-line `deep-type-audit.allowlist.json`. The walker stays; the artifact does not. 2. Make the audit report-only by default. Always exits 0 unless the script itself errors. Prints inventory: total findings, by-tier counts, top files, depth-cap warnings. 3. Add `--strict` flag for the eventual gate behavior. Not used in CI yet because it's only meaningful once SD-2966 defines the public facade and the allowlist is re-seeded against that smaller surface. 4. CI workflows updated: both PR CI and release CI run the audit in report-only mode. Comments explain that `--strict` is added later. 5. README rewritten with prominent "Status: report-only inventory" at the top, explaining what defers the gate and what re-emerges after SD-2966. What this delivers today: - The walker logic ships and runs in CI from day one - The 10K-line public artifact goes away - Inventory data appears in every CI run (visible signal of how much accidental surface is leaking) - No commitment to typing 1799 entries on accidental surface What this defers: - Hard gate against new regressions on the current accidental surface. Net cost: a few weeks where new `any` could land on already-`any`-heavy code. Acceptable because the work to drain that surface shouldn't start before SD-2966 anyway. Verified: - node tests/consumer-typecheck/deep-type-audit.mjs: PASS, exit 0 - node tests/consumer-typecheck/deep-type-audit.mjs --strict: FAIL, exit 1 - node tests/consumer-typecheck/deep-type-audit.mjs --write: still works (creates allowlist; intended for use post-SD-2966)
…6 gate (SD-2977) Companion to the allowlist removal: rewires the walker so default mode prints inventory and always exits 0, while a new --strict flag preserves the original gate behavior for use once SD-2966 defines the public facade. - Compiler diagnostics, private specifier leaks, and allowlist comparison all switch from process.exit(1) to print-and-continue unless --strict is set - New report section prints by-tier and top-files breakdowns regardless of mode (visible CI signal) - When no allowlist file exists, the run prints an explanatory note pointing at SD-2966 and the --write workflow - CI step comments updated in both ci-superdoc.yml and release-superdoc.yml to reflect the report-only intent - README rewritten with prominent "Status: report-only inventory" section at the top; commands section documents --strict and clarifies --write is intended for post-SD-2966 baselining Verified: default exits 0, --strict exits 1, --write still seeds an allowlist correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The existing public-type checks lock in exported type names and top-level
anyassertions, but they do not catchanyon public members, callback params, return types, or nested type arguments. That means consumers can still lose IntelliSense after touching a public API surface that resolves toany.This adds a deep public-type audit that builds a TypeScript Program from the packed-and-installed
superdoctarball, recursively walks every owned type reachable from public export entries, and fails CI if a finding is not present indeep-type-audit.allowlist.json. The audit runs withskipLibCheck: falseand explicitly scans installed declarations for private@superdoc/*specifier leaks, so monorepo-local resolution cannot hide a broken published package.The allowlist is seeded with the current 293 owned findings and tagged by tier so follow-up PRs can drain it by surface:
tier-1-pinia(160): Pinia/store exposuretier-2-toolbar(46): toolbar / custom button configtier-3-helpers(61): track changes and field annotation helperstier-4-public-contract(2): curated public contract leakstier-5-other(24): remaining owned declarationsThe gate runs in PR CI and release CI, after the consumer matrix prepares the installed tarball. This also closes the release path gap by running the packed consumer matrix and deep audit before publish.
The allowlist is a starting line, not a waiver. New findings fail CI; stale findings fail CI so fixes must remove entries. Open follow-up: Pinia stores account for most entries and may be better hidden from the public surface than typed (SD-2966).
Verified:
node tests/consumer-typecheck/typecheck-matrix.mjs: 53 passed, 0 failednode tests/consumer-typecheck/deep-type-audit.mjs: 293 findings, 0 new, 0 staleLinear: SD-2977 (umbrella SD-2976)