From 8eb0970b3f2defb8290a4f7d6c2339aafc1c88c2 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 16 Jul 2026 11:00:06 -0400 Subject: [PATCH 01/14] Add `typescript-typing` skill for `any`-handling and type derivation --- CHANGELOG.md | 4 ++ .../coding/skills/typescript-typing/skill.md | 58 +++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 domains/coding/skills/typescript-typing/skill.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 8557ff8f..3fe0994a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `typescript-typing` skill (coding domain): the reasoning layer beneath `coding-guidelines`' TypeScript section — `any` as a type-checking-off directive with substitute-by-position guidance (assignee → `unknown`, assigned → `never`) and the generic-constraint exception, plus derive-types-from-authoritative-sources over ad-hoc declarations. Experimental. + ## [0.2.0] ### Added diff --git a/domains/coding/skills/typescript-typing/skill.md b/domains/coding/skills/typescript-typing/skill.md new file mode 100644 index 00000000..b3e9cc64 --- /dev/null +++ b/domains/coding/skills/typescript-typing/skill.md @@ -0,0 +1,58 @@ +--- +name: typescript-typing +description: TypeScript typing discipline — treat `any` as a directive that disables type checking (substitute by position: assignee → `unknown`, assigned → `never`), and derive types from authoritative sources rather than hand-writing ad-hoc ones that duplicate and drift. +maturity: experimental +--- + +# TypeScript Typing Discipline + +Deepens the TypeScript section of `mms-coding-guidelines` — which already says "avoid `any`, prefer `unknown`" and links `MetaMask/contributor-docs` `docs/typescript.md`. This skill is the reasoning layer underneath those bullets: why `any` is dangerous and how to replace it by position, and how to avoid hand-writing a type that an authoritative source already defines. Grounded in `docs/typescript.md` (§ Avoid `any`, § Prefer type inference). + +## `any` is not a type — it is a directive that disables type checking + +The mental model matters more than the ESLint rule, because `@typescript-eslint/no-explicit-any` (already `error` in extension CI) does not stop the reasoning that reaches for `any`: + +- **`any` is not "the widest type" — that is `unknown`.** `any` is a compiler directive that _disables_ type checking for the value it annotates. +- **It suppresses every error about its assignee** — the equivalent of `@ts-ignore` on every use of that variable. The errors still affect the code; `any` only makes them invisible. +- **It subsumes what it touches.** Any type in a union, intersection, or property relationship with `any` becomes `any` — an unmitigated loss of type information. +- **It infects downstream code.** One `any` at a source (e.g. a library type that resolves to `any`) propagates silently through every consumer, converting compile-time errors into **silent runtime failures** — defeating the point of a statically-typed language. + +## Substitute `any` by position — assignee vs assigned + +Identify which side of an assignment the `any` sits on: + +- **Assignee** (a variable, parameter, or return that _receives_ a value — "it could be anything"): **try `unknown` first**, then narrow. `unknown` is the true universal supertype: everything is assignable to it, but it forces a type guard before use. `any` ↔ `unknown` are interchangeable in this position, so it is almost always a safe swap. + - 🚫 `type Fn = () => any; const xs: any[]` + - ✅ `type Fn = () => unknown; const xs: unknown[]` +- **Assigned** (a value that _flows into_ a slot): **try `never` first**, then widen to a subtype of the assignee's type. `unknown` cannot substitute here (it is only assignable to `unknown`); `never` is the bottom type, assignable to everything. + +## The one acceptable exception — generic *constraints* + +`any` is acceptable in a generic **constraint**, and only there. It bounds a type parameter without being assigned to a value, so it neither pollutes nor infects. + +```typescript +class BaseController< + // eslint-disable-next-line @typescript-eslint/no-explicit-any + Messenger extends RestrictedMessenger, +> // ... +``` + +Three conditions attach: + +- **Declare it explicitly.** `no-explicit-any` is `error`, so a constraint `any` needs an inline `// eslint-disable-next-line @typescript-eslint/no-explicit-any` at the site — a deliberate, visible exception. +- **Constraints only — never a generic _argument_.** Passing `any` as an argument (`ControllerMessenger`) is 🚫 — that assigns `any` and infects. Constraint-vs-argument is the whole distinction. +- **Prefer a narrower constraint anyway.** Reach for `any` here only when the narrower bound is genuinely unavailable. + +When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a TODO. Never reach for `any` to unblock feature work "to fix later." + +## Derive types from authoritative sources — don't hand-write ad-hoc ones + +When a type already exists at an authoritative source — a controller's state type, a function's return, a library's exported type, a schema/struct — **derive from it** rather than restating it. Derive with indexed access (`State['field']`), `typeof`, `ReturnType` / `Parameters`, utility types (`Pick` / `Omit` / `Partial`), and `Infer`; let inference carry the rest. This is the structural form of the contributor-docs rule *Prefer type inference over annotations and assertions*: inferred and derived types stay "responsive to changes in code," while hand-written declarations "rely on hard-coding, making them brittle against code drift." + +An ad-hoc type — one hand-defined to describe a value an authoritative type already describes — carries three dangers: + +- **Duplication.** The same shape is stated twice; every reader reconciles them and every change touches both. +- **Incorrect, usually too wide.** A hand-written type is a _guess_ at the source's shape, and the guess is almost always looser than the real type — it admits values the authoritative type would reject, so invalid data still type-checks. +- **Drift.** The source evolves; the copy does not. Because it is hand-written rather than derived, the compiler cannot flag the divergence — the bug surfaces at runtime, not at build. + +**Rule:** before writing a type, ask where the value comes from and whether that source already types it. If it does, derive. Define a fresh type only when no authoritative source exists — a genuinely new shape at a boundary you own. Before defining, exhaust deriving: search the internal `@metamask/*` packages and the consuming repo for the authoritative source first. From 9e20763e8775838730e59883de4f96be684317e7 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 16 Jul 2026 12:52:34 -0400 Subject: [PATCH 02/14] Ground the derive rule in a real `metamask-extension` #42583 counterexample --- .../coding/skills/typescript-typing/skill.md | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/domains/coding/skills/typescript-typing/skill.md b/domains/coding/skills/typescript-typing/skill.md index b3e9cc64..1b4d23d9 100644 --- a/domains/coding/skills/typescript-typing/skill.md +++ b/domains/coding/skills/typescript-typing/skill.md @@ -55,4 +55,30 @@ An ad-hoc type — one hand-defined to describe a value an authoritative type al - **Incorrect, usually too wide.** A hand-written type is a _guess_ at the source's shape, and the guess is almost always looser than the real type — it admits values the authoritative type would reject, so invalid data still type-checks. - **Drift.** The source evolves; the copy does not. Because it is hand-written rather than derived, the compiler cannot flag the divergence — the bug surfaces at runtime, not at build. +**A grounded example (`metamask-extension` #42583).** A `wallet-services` module hand-wrote a slice of `NetworkController` state instead of deriving it. + +🚫 Re-declared — every field optional (wider than the real, _required_ field), keyed by `string` not `Hex`, and unlinked from the source, so it drifts silently when the controller changes: + +```typescript +type NetworkControllerState = { + networkConfigurationsByChainId?: Record< + string, + { + defaultRpcEndpointIndex?: number; + rpcEndpoints?: { networkClientId?: string }[]; + } + >; +}; +``` + +✅ Derived — tracks the authoritative shape (`Record`), narrowed to the one field in use: + +```typescript +import type { NetworkState } from '@metamask/network-controller'; + +type NetworkConfigurations = NetworkState['networkConfigurationsByChainId']; +``` + +A too-wide copy does not save work; it moves the work downstream. The same PR typed a dependency as `getMetaMaskState: () => Record`, so every consumer then had to re-cast the shape back by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. Deriving that dependency from the authoritative state type deletes the casts. The same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand; the discipline is extending it to every referenced type. + **Rule:** before writing a type, ask where the value comes from and whether that source already types it. If it does, derive. Define a fresh type only when no authoritative source exists — a genuinely new shape at a boundary you own. Before defining, exhaust deriving: search the internal `@metamask/*` packages and the consuming repo for the authoritative source first. From be1dc98c5c1dfafaddde569ec0fb07181ff1a31e Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 16 Jul 2026 13:09:53 -0400 Subject: [PATCH 03/14] Split typescript-typing into a `typescript` domain: avoid-any, derive-types, decompose-large-files --- CHANGELOG.md | 2 +- .../coding/skills/typescript-typing/skill.md | 84 ------------------- domains/typescript/skills/avoid-any/skill.md | 46 ++++++++++ .../skills/decompose-large-files/skill.md | 46 ++++++++++ .../typescript/skills/derive-types/skill.md | 51 +++++++++++ 5 files changed, 144 insertions(+), 85 deletions(-) delete mode 100644 domains/coding/skills/typescript-typing/skill.md create mode 100644 domains/typescript/skills/avoid-any/skill.md create mode 100644 domains/typescript/skills/decompose-large-files/skill.md create mode 100644 domains/typescript/skills/derive-types/skill.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 3fe0994a..65fe0f20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `typescript-typing` skill (coding domain): the reasoning layer beneath `coding-guidelines`' TypeScript section — `any` as a type-checking-off directive with substitute-by-position guidance (assignee → `unknown`, assigned → `never`) and the generic-constraint exception, plus derive-types-from-authoritative-sources over ad-hoc declarations. Experimental. +- Add `typescript` domain (experimental) with three skills: `avoid-any` (`any` is a type-checking-off directive, not a type — substitute by position: assignee → `unknown`, assigned → `never`; the one exception is a generic constraint), `derive-types` (derive from authoritative sources over ad-hoc declarations that duplicate, run too wide, and drift), and `decompose-large-files` (decompose a large file by coherent, independently-mergeable units for modularity, maintainability, and reviewability — and to unblock incremental TS migration). ## [0.2.0] diff --git a/domains/coding/skills/typescript-typing/skill.md b/domains/coding/skills/typescript-typing/skill.md deleted file mode 100644 index 1b4d23d9..00000000 --- a/domains/coding/skills/typescript-typing/skill.md +++ /dev/null @@ -1,84 +0,0 @@ ---- -name: typescript-typing -description: TypeScript typing discipline — treat `any` as a directive that disables type checking (substitute by position: assignee → `unknown`, assigned → `never`), and derive types from authoritative sources rather than hand-writing ad-hoc ones that duplicate and drift. -maturity: experimental ---- - -# TypeScript Typing Discipline - -Deepens the TypeScript section of `mms-coding-guidelines` — which already says "avoid `any`, prefer `unknown`" and links `MetaMask/contributor-docs` `docs/typescript.md`. This skill is the reasoning layer underneath those bullets: why `any` is dangerous and how to replace it by position, and how to avoid hand-writing a type that an authoritative source already defines. Grounded in `docs/typescript.md` (§ Avoid `any`, § Prefer type inference). - -## `any` is not a type — it is a directive that disables type checking - -The mental model matters more than the ESLint rule, because `@typescript-eslint/no-explicit-any` (already `error` in extension CI) does not stop the reasoning that reaches for `any`: - -- **`any` is not "the widest type" — that is `unknown`.** `any` is a compiler directive that _disables_ type checking for the value it annotates. -- **It suppresses every error about its assignee** — the equivalent of `@ts-ignore` on every use of that variable. The errors still affect the code; `any` only makes them invisible. -- **It subsumes what it touches.** Any type in a union, intersection, or property relationship with `any` becomes `any` — an unmitigated loss of type information. -- **It infects downstream code.** One `any` at a source (e.g. a library type that resolves to `any`) propagates silently through every consumer, converting compile-time errors into **silent runtime failures** — defeating the point of a statically-typed language. - -## Substitute `any` by position — assignee vs assigned - -Identify which side of an assignment the `any` sits on: - -- **Assignee** (a variable, parameter, or return that _receives_ a value — "it could be anything"): **try `unknown` first**, then narrow. `unknown` is the true universal supertype: everything is assignable to it, but it forces a type guard before use. `any` ↔ `unknown` are interchangeable in this position, so it is almost always a safe swap. - - 🚫 `type Fn = () => any; const xs: any[]` - - ✅ `type Fn = () => unknown; const xs: unknown[]` -- **Assigned** (a value that _flows into_ a slot): **try `never` first**, then widen to a subtype of the assignee's type. `unknown` cannot substitute here (it is only assignable to `unknown`); `never` is the bottom type, assignable to everything. - -## The one acceptable exception — generic *constraints* - -`any` is acceptable in a generic **constraint**, and only there. It bounds a type parameter without being assigned to a value, so it neither pollutes nor infects. - -```typescript -class BaseController< - // eslint-disable-next-line @typescript-eslint/no-explicit-any - Messenger extends RestrictedMessenger, -> // ... -``` - -Three conditions attach: - -- **Declare it explicitly.** `no-explicit-any` is `error`, so a constraint `any` needs an inline `// eslint-disable-next-line @typescript-eslint/no-explicit-any` at the site — a deliberate, visible exception. -- **Constraints only — never a generic _argument_.** Passing `any` as an argument (`ControllerMessenger`) is 🚫 — that assigns `any` and infects. Constraint-vs-argument is the whole distinction. -- **Prefer a narrower constraint anyway.** Reach for `any` here only when the narrower bound is genuinely unavailable. - -When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a TODO. Never reach for `any` to unblock feature work "to fix later." - -## Derive types from authoritative sources — don't hand-write ad-hoc ones - -When a type already exists at an authoritative source — a controller's state type, a function's return, a library's exported type, a schema/struct — **derive from it** rather than restating it. Derive with indexed access (`State['field']`), `typeof`, `ReturnType` / `Parameters`, utility types (`Pick` / `Omit` / `Partial`), and `Infer`; let inference carry the rest. This is the structural form of the contributor-docs rule *Prefer type inference over annotations and assertions*: inferred and derived types stay "responsive to changes in code," while hand-written declarations "rely on hard-coding, making them brittle against code drift." - -An ad-hoc type — one hand-defined to describe a value an authoritative type already describes — carries three dangers: - -- **Duplication.** The same shape is stated twice; every reader reconciles them and every change touches both. -- **Incorrect, usually too wide.** A hand-written type is a _guess_ at the source's shape, and the guess is almost always looser than the real type — it admits values the authoritative type would reject, so invalid data still type-checks. -- **Drift.** The source evolves; the copy does not. Because it is hand-written rather than derived, the compiler cannot flag the divergence — the bug surfaces at runtime, not at build. - -**A grounded example (`metamask-extension` #42583).** A `wallet-services` module hand-wrote a slice of `NetworkController` state instead of deriving it. - -🚫 Re-declared — every field optional (wider than the real, _required_ field), keyed by `string` not `Hex`, and unlinked from the source, so it drifts silently when the controller changes: - -```typescript -type NetworkControllerState = { - networkConfigurationsByChainId?: Record< - string, - { - defaultRpcEndpointIndex?: number; - rpcEndpoints?: { networkClientId?: string }[]; - } - >; -}; -``` - -✅ Derived — tracks the authoritative shape (`Record`), narrowed to the one field in use: - -```typescript -import type { NetworkState } from '@metamask/network-controller'; - -type NetworkConfigurations = NetworkState['networkConfigurationsByChainId']; -``` - -A too-wide copy does not save work; it moves the work downstream. The same PR typed a dependency as `getMetaMaskState: () => Record`, so every consumer then had to re-cast the shape back by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. Deriving that dependency from the authoritative state type deletes the casts. The same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand; the discipline is extending it to every referenced type. - -**Rule:** before writing a type, ask where the value comes from and whether that source already types it. If it does, derive. Define a fresh type only when no authoritative source exists — a genuinely new shape at a boundary you own. Before defining, exhaust deriving: search the internal `@metamask/*` packages and the consuming repo for the authoritative source first. diff --git a/domains/typescript/skills/avoid-any/skill.md b/domains/typescript/skills/avoid-any/skill.md new file mode 100644 index 00000000..b7a207d9 --- /dev/null +++ b/domains/typescript/skills/avoid-any/skill.md @@ -0,0 +1,46 @@ +--- +name: avoid-any +description: Handle `any` correctly — it is not a type but a directive that disables type checking. Substitute by position (assignee → `unknown`, assigned → `never`); the one exception is a generic constraint, declared with an inline eslint-disable. +maturity: experimental +--- + +# Avoid `any` + +Deepens the TypeScript guidance in `mms-coding-guidelines` — which already says "avoid `any`, prefer `unknown`" and links `MetaMask/contributor-docs` `docs/typescript.md`. This is the reasoning layer beneath that bullet: why `any` is dangerous, and how to replace it by position. Grounded in `docs/typescript.md` (§ Avoid `any`). + +## `any` is not a type — it is a directive that disables type checking + +The mental model matters more than the ESLint rule, because `@typescript-eslint/no-explicit-any` (already `error` in extension CI) does not stop the reasoning that reaches for `any`: + +- **`any` is not "the widest type" — that is `unknown`.** `any` is a compiler directive that _disables_ type checking for the value it annotates. +- **It suppresses every error about its assignee** — the equivalent of `@ts-ignore` on every use of that variable. The errors still affect the code; `any` only makes them invisible. +- **It subsumes what it touches.** Any type in a union, intersection, or property relationship with `any` becomes `any` — an unmitigated loss of type information. +- **It infects downstream code.** One `any` at a source (e.g. a library type that resolves to `any`) propagates silently through every consumer, converting compile-time errors into **silent runtime failures** — defeating the point of a statically-typed language. + +## Substitute `any` by position — assignee vs assigned + +Identify which side of an assignment the `any` sits on: + +- **Assignee** (a variable, parameter, or return that _receives_ a value — "it could be anything"): **try `unknown` first**, then narrow. `unknown` is the true universal supertype: everything is assignable to it, but it forces a type guard before use. `any` ↔ `unknown` are interchangeable in this position, so it is almost always a safe swap. + - 🚫 `type Fn = () => any; const xs: any[]` + - ✅ `type Fn = () => unknown; const xs: unknown[]` +- **Assigned** (a value that _flows into_ a slot): **try `never` first**, then widen to a subtype of the assignee's type. `unknown` cannot substitute here (it is only assignable to `unknown`); `never` is the bottom type, assignable to everything. + +## The one acceptable exception — generic *constraints* + +`any` is acceptable in a generic **constraint**, and only there. It bounds a type parameter without being assigned to a value, so it neither pollutes nor infects. + +```typescript +class BaseController< + // eslint-disable-next-line @typescript-eslint/no-explicit-any + Messenger extends RestrictedMessenger, +> // ... +``` + +Three conditions attach: + +- **Declare it explicitly.** `no-explicit-any` is `error`, so a constraint `any` needs an inline `// eslint-disable-next-line @typescript-eslint/no-explicit-any` at the site — a deliberate, visible exception. +- **Constraints only — never a generic _argument_.** Passing `any` as an argument (`ControllerMessenger`) is 🚫 — that assigns `any` and infects. Constraint-vs-argument is the whole distinction. +- **Prefer a narrower constraint anyway.** Reach for `any` here only when the narrower bound is genuinely unavailable. + +When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a TODO. Never reach for `any` to unblock feature work "to fix later." diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md new file mode 100644 index 00000000..99a749d5 --- /dev/null +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -0,0 +1,46 @@ +--- +name: decompose-large-files +description: Decompose a large file into coherent, independently-mergeable modules to improve modularity, maintainability, reviewability, and code organization — and to unblock incremental TS migration. Extract by coherent subject/domain cluster; never fragment for its own sake. +maturity: experimental +--- + +# Decompose Large Files By Coherent Units + +A file that has grown to thousands of lines is hard to review, hard to maintain, and — if it is still `.js` — hard to migrate to TypeScript in one pass. Decompose it by moving coherent subject/domain clusters into their own modules. The goal is **modularity, maintainability, reviewability, and code organization**; unblocking an incremental JS→TS migration is a direct benefit, because each extracted module becomes a small, independently-typable unit. + +Reference application: `metamask-extension` #41735 (`MetamaskController` decomposition, 9,260 → ~3,500 lines). + +## The decision that matters: what is a coherent unit? + +Extraction is worth it only when the extracted piece is a **coherent unit that can move independently**. Apply this judgment _before_ proposing any module: + +- **Coherent subject.** The cluster is about one thing — one domain, one lifecycle, one concern (phishing detection, metrics emission, badge rendering). A reader can state its responsibility in one sentence. +- **Independently mergeable.** It can be lifted behind a defined seam — injected dependencies, or a messenger action — without dragging half the file with it. Its coupling to shared state and to the composition root is small and nameable. +- **Not fragmentation.** Extraction for its own sake — splitting a cohesive routine across files, or pulling out a 20-line helper that only one caller uses and has no independent identity — makes the code _harder_ to follow, not easier. If pulling the piece out means the two halves must still change together, leave it inline. **Refactoring is a means to modularity, not an end; a change that raises the file count without raising coherence is a regression.** + +Some code should **stay** in the original file: the thin **composition / bootstrap root** that wires the modules together. Extracting the wiring itself fragments rather than clarifies — it is the one place the whole is assembled. The tell is code that references _everything_ (the central object plus the shared mutable state every cluster reads); relocating it behind a wide "params bag" just moves the tangle. + +## How to extract one unit (self-contained, per #41735) + +Each extraction is one self-contained change — no separate "final deletion" or "integration" ticket: + +1. **Scaffold** the module (its own file/dir, TypeScript from the start). +2. **Port** the bodies in, unchanged in behavior. +3. **Define the seam** — inject the dependencies the module needs (or register its public methods as messenger actions) instead of reaching back into the file's globals. This is where the human judgment is. +4. **Rewire** the call sites to go through the seam. +5. **Delete the original** in the same change; leave no forwarding stub. +6. **Add a structural unit test** against a stub/mock of the seam — especially valuable when the original file had no tests. + +Steps 1, 2, 5, 6 are largely mechanical (codemod territory — `jscodeshift` on the source, `ts-morph` on the module). Step 3/4 is the part that needs a person. + +## Sizing and sequencing + +- **Size each unit S / M / L / XL** by body size × coupling to rewire. Ship one module (or one subject area) per PR — reviewer context window is usually the binding constraint, so a focused S/M PR merges where an XL one stalls. +- **Sequence lowest-coupling-first.** Extract the clusters with the smallest, cleanest seam first: they establish the pattern and shrink the file so later, more-entangled extractions are easier to see. Save the most coupled cluster (often the core lifecycle) for last, or leave it as the composition root. +- **Enforce the new boundary** so the file cannot silently re-absorb the module — an ESLint `import/no-restricted-paths` rule on the module directory (per #41735). + +## When NOT to decompose + +- The file is large but **already cohesive** — one subject, read top to bottom. Size alone is not a reason. +- The only available splits are **arbitrary** (by line count, or a catch-all `utils`) rather than by subject. That produces fragments, not modules. +- The extraction **cannot get a clean seam** — everything it touches is shared mutable state with the rest of the file. Fix the coupling first, or leave it inline and say so. diff --git a/domains/typescript/skills/derive-types/skill.md b/domains/typescript/skills/derive-types/skill.md new file mode 100644 index 00000000..84bdbe7c --- /dev/null +++ b/domains/typescript/skills/derive-types/skill.md @@ -0,0 +1,51 @@ +--- +name: derive-types +description: Derive types from authoritative sources (indexed access, `typeof`, `ReturnType`/`Parameters`, `Pick`/`Omit`, `Infer`) instead of hand-writing ad-hoc types that duplicate, run too wide, and drift. +maturity: experimental +--- + +# Derive Types From Authoritative Sources + +Deepens the TypeScript guidance in `mms-coding-guidelines`, and is the structural counterpart to the contributor-docs rule *Prefer type inference over annotations and assertions* (`MetaMask/contributor-docs` `docs/typescript.md`). Where inference handles values, derivation handles types that reference other types. + +## Derive, don't re-declare + +When a type already exists at an authoritative source — a controller's state type, a function's return, a library's exported type, a schema/struct — **derive from it** rather than restating it. Derive with indexed access (`State['field']`), `typeof`, `ReturnType` / `Parameters`, utility types (`Pick` / `Omit` / `Partial`), and `Infer`; let inference carry the rest. Inferred and derived types stay "responsive to changes in code," while hand-written declarations "rely on hard-coding, making them brittle against code drift." + +An ad-hoc type — one hand-defined to describe a value an authoritative type already describes — carries three dangers: + +- **Duplication.** The same shape is stated twice; every reader reconciles them and every change touches both. +- **Incorrect, usually too wide.** A hand-written type is a _guess_ at the source's shape, and the guess is almost always looser than the real type — it admits values the authoritative type would reject, so invalid data still type-checks. +- **Drift.** The source evolves; the copy does not. Because it is hand-written rather than derived, the compiler cannot flag the divergence — the bug surfaces at runtime, not at build. + +## A grounded example (`metamask-extension` #42583) + +A `wallet-services` module hand-wrote a slice of `NetworkController` state instead of deriving it. + +🚫 Re-declared — every field optional (wider than the real, _required_ field), keyed by `string` not `Hex`, and unlinked from the source, so it drifts silently when the controller changes: + +```typescript +type NetworkControllerState = { + networkConfigurationsByChainId?: Record< + string, + { + defaultRpcEndpointIndex?: number; + rpcEndpoints?: { networkClientId?: string }[]; + } + >; +}; +``` + +✅ Derived — tracks the authoritative shape (`Record`), narrowed to the one field in use: + +```typescript +import type { NetworkState } from '@metamask/network-controller'; + +type NetworkConfigurations = NetworkState['networkConfigurationsByChainId']; +``` + +A too-wide copy does not save work; it moves the work downstream. The same PR typed a dependency as `getMetaMaskState: () => Record`, so every consumer then had to re-cast the shape back by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. Deriving that dependency from the authoritative state type deletes the casts. The same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand; the discipline is extending it to every referenced type. + +## Rule + +Before writing a type, ask where the value comes from and whether that source already types it. If it does, derive. Define a fresh type only when no authoritative source exists — a genuinely new shape at a boundary you own. Before defining, exhaust deriving: search the internal `@metamask/*` packages and the consuming repo for the authoritative source first. From a89095d6301960892cce4e1b7a619d6c1c9a0da0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 17 Jul 2026 05:39:05 -0400 Subject: [PATCH 04/14] Add "why decompose first" rationale to `decompose-large-files`: boundary-identification is step one of any migration --- domains/typescript/skills/decompose-large-files/skill.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md index 99a749d5..081192b5 100644 --- a/domains/typescript/skills/decompose-large-files/skill.md +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -10,6 +10,12 @@ A file that has grown to thousands of lines is hard to review, hard to maintain, Reference application: `metamask-extension` #41735 (`MetamaskController` decomposition, 9,260 → ~3,500 lines). +## Why decompose first — even if the file could convert in one pass + +Documenting a large file's modularizable boundaries — one coherent unit per ticket — is worth doing **even if the file could somehow be converted and reviewed in a single PR**, because identifying those boundaries is the first logical step of _any_ migration process, human or AI. The boundary map is not throwaway scaffolding; it is the migration's own plan. + +And a single-pass conversion is impractical even for a capable AI. Converting a multi-thousand-line file in one PR means holding the entire file in context **and** progressively loading every upstream file whose source types the code should derive from (see `derive-types`) **and** every downstream file that imports it and must be updated. That context fan-in (source types) and fan-out (consumers) is the real cost — not the line count of the file itself. Decomposing shrinks each unit's context to one subject plus its seam, which is what makes the conversion tractable and reviewable at all. + ## The decision that matters: what is a coherent unit? Extraction is worth it only when the extracted piece is a **coherent unit that can move independently**. Apply this judgment _before_ proposing any module: From 388dd98f3debcb003d1ac78ae27a393d8980419b Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 17 Jul 2026 05:51:52 -0400 Subject: [PATCH 05/14] Add `migration-context-cost` skill (fan-in/fan-out) + avoid-any second exception (bivariant callback `any`) --- CHANGELOG.md | 2 +- domains/typescript/skills/avoid-any/skill.md | 32 +++++++++++++++++-- .../skills/decompose-large-files/skill.md | 2 +- .../skills/migration-context-cost/skill.md | 27 ++++++++++++++++ 4 files changed, 59 insertions(+), 4 deletions(-) create mode 100644 domains/typescript/skills/migration-context-cost/skill.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 65fe0f20..8c839195 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `typescript` domain (experimental) with three skills: `avoid-any` (`any` is a type-checking-off directive, not a type — substitute by position: assignee → `unknown`, assigned → `never`; the one exception is a generic constraint), `derive-types` (derive from authoritative sources over ad-hoc declarations that duplicate, run too wide, and drift), and `decompose-large-files` (decompose a large file by coherent, independently-mergeable units for modularity, maintainability, and reviewability — and to unblock incremental TS migration). +- Add `typescript` domain (experimental) with four skills: `avoid-any` (`any` is a type-checking-off directive, not a type — substitute by position: assignee → `unknown`, assigned → `never`; two narrow exceptions: generic constraints and bivariant callback parameters), `derive-types` (derive from authoritative sources over ad-hoc declarations that duplicate, run too wide, and drift), `decompose-large-files` (decompose a large file by coherent, independently-mergeable units for modularity, maintainability, and reviewability — and to unblock incremental TS migration), and `migration-context-cost` (a file's JS→TS migration cost is dominated by context fan-in + fan-out, not line count — scope and sequence tickets by it). ## [0.2.0] diff --git a/domains/typescript/skills/avoid-any/skill.md b/domains/typescript/skills/avoid-any/skill.md index b7a207d9..b9256e87 100644 --- a/domains/typescript/skills/avoid-any/skill.md +++ b/domains/typescript/skills/avoid-any/skill.md @@ -1,6 +1,6 @@ --- name: avoid-any -description: Handle `any` correctly — it is not a type but a directive that disables type checking. Substitute by position (assignee → `unknown`, assigned → `never`); the one exception is a generic constraint, declared with an inline eslint-disable. +description: Handle `any` correctly — it is not a type but a directive that disables type checking. Substitute by position (assignee → `unknown`, assigned → `never`). Two narrow exceptions: a generic constraint, and a callback parameter caught in a bivariant position between two fixed, irresolvable function-type constraints — both declared with an inline eslint-disable. maturity: experimental --- @@ -26,7 +26,7 @@ Identify which side of an assignment the `any` sits on: - ✅ `type Fn = () => unknown; const xs: unknown[]` - **Assigned** (a value that _flows into_ a slot): **try `never` first**, then widen to a subtype of the assignee's type. `unknown` cannot substitute here (it is only assignable to `unknown`); `never` is the bottom type, assignable to everything. -## The one acceptable exception — generic *constraints* +## Acceptable exception 1 — generic *constraints* `any` is acceptable in a generic **constraint**, and only there. It bounds a type parameter without being assigned to a value, so it neither pollutes nor infects. @@ -43,4 +43,32 @@ Three conditions attach: - **Constraints only — never a generic _argument_.** Passing `any` as an argument (`ControllerMessenger`) is 🚫 — that assigns `any` and infects. Constraint-vs-argument is the whole distinction. - **Prefer a narrower constraint anyway.** Reach for `any` here only when the narrower bound is genuinely unavailable. +## Acceptable exception 2 — a callback parameter between two irresolvable function-type constraints + +A callback's parameter may be `any` when all three hold: + +1. **Bivariant position** — the callback is _assignable to_ a wider function type **and** an _assignee of_ a narrower one. +2. **Irresolvable** — no concrete type satisfies both directions, because the wider function type is not a supertype of the narrower (`WideParam extends T extends NarrowParam` has no solution). +3. **Fixed** — neither constraint can be redesigned without breaking callers or losing accuracy. + +Under `--strictFunctionTypes` parameters are contravariant, so the callback's parameter must be a _supertype_ of the outer slot's (outward: `unknown` ✓, `never` ✗) **and** a _subtype_ of the incoming value's (inward: `never` ✓, `unknown` ✗). `any` is the only inhabitant of both the top and bottom of the assignability lattice, so it is the only escape. (Return types stay covariant — keep `unknown`.) + +🚫 If the constraints are **redesignable**, the contravariance error is a real design smell — fix it (usually by parametrizing with a generic), don't suppress: + +```typescript +declare const acceptGeneric: (handler: (event: E) => void) => void; +acceptGeneric(onA); // no `any` needed +``` + +✅ Only when the constraints are **fixed and irresolvable**: + +```typescript +// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Bivariant position with irresolvable, fixed constraints +let bridge: (x: any) => void; +``` + +Like the generic-constraint case, this `any` is **not infectious** — it is scoped to one parameter position, and both constraint types re-impose their signatures at each use site. Annotate the `eslint-disable` with the criteria so a reviewer can check them. Caveat: the safety claim holds only if the constraint types are accurate — a fixed constraint that is itself imprecise (a library type that is `any` internally) still forces the bridge `any` but no longer preserves safety at the use sites. + +Canonical instance: a messenger `registerActionHandler` slot typed `(...args: any[]) => any` — strongly-typed handlers flow inward at registration, strongly-typed argument tuples outward at dispatch; `unknown[]` fails registration, `never[]` fails dispatch. It encodes rank-N polymorphism (`∀α. (α) => R`) that TypeScript cannot express directly. + When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a TODO. Never reach for `any` to unblock feature work "to fix later." diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md index 081192b5..5d91e44f 100644 --- a/domains/typescript/skills/decompose-large-files/skill.md +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -14,7 +14,7 @@ Reference application: `metamask-extension` #41735 (`MetamaskController` decompo Documenting a large file's modularizable boundaries — one coherent unit per ticket — is worth doing **even if the file could somehow be converted and reviewed in a single PR**, because identifying those boundaries is the first logical step of _any_ migration process, human or AI. The boundary map is not throwaway scaffolding; it is the migration's own plan. -And a single-pass conversion is impractical even for a capable AI. Converting a multi-thousand-line file in one PR means holding the entire file in context **and** progressively loading every upstream file whose source types the code should derive from (see `derive-types`) **and** every downstream file that imports it and must be updated. That context fan-in (source types) and fan-out (consumers) is the real cost — not the line count of the file itself. Decomposing shrinks each unit's context to one subject plus its seam, which is what makes the conversion tractable and reviewable at all. +And a single-pass conversion is impractical even for a capable AI — the binding constraint is the file's **context fan-in and fan-out** (upstream source types + downstream consumers), not its line count (see `migration-context-cost`). Decomposing shrinks each unit's context to one subject plus its seam, which is what makes the conversion tractable and reviewable at all. ## The decision that matters: what is a coherent unit? diff --git a/domains/typescript/skills/migration-context-cost/skill.md b/domains/typescript/skills/migration-context-cost/skill.md new file mode 100644 index 00000000..9ff9b6d6 --- /dev/null +++ b/domains/typescript/skills/migration-context-cost/skill.md @@ -0,0 +1,27 @@ +--- +name: migration-context-cost +description: A file's JS→TS migration cost is dominated by context fan-in (upstream files whose source types to derive from) + fan-out (downstream files that import it and must update), not its line count. Scope and sequence migration tickets by this cost. +maturity: experimental +--- + +# TypeScript Migration Context Cost — Fan-In and Fan-Out + +The cost of converting a file to TypeScript is not its line count. It is the **context the conversion pulls in** — everything the converter, human or AI, must load to do it correctly. + +## The two axes + +- **Fan-in (upstream source types).** To type the file's values correctly you must load every upstream module whose types the code should _derive_ from — controller state, function returns, library exports, schemas (see `derive-types`). A file that touches many upstream types has high fan-in even if it is short. +- **Fan-out (downstream consumers).** Changing the file's types ripples to every module that imports it; each may need its own update or re-type. A widely-imported file has high fan-out even if it is small. + +The real work — and the real review surface — is the sum of these, not the target file's length. A 200-line hub imported by 100 files can be a larger migration than a self-contained 2,000-line leaf. + +## Why it matters even for AI + +Single-pass conversion of a high-fan-in/fan-out file is impractical even for a capable AI: it must hold the target file in context **and** progressively load every upstream source-type file **and** every downstream consumer. That context fan-in/fan-out is the binding constraint, not the model's ability to read the file itself. + +## How to use it + +- **Scope tickets by context cost, not LOC.** Estimate fan-in (upstream types the file derives from) + fan-out (`grep -rl` importer count) before sizing a migration ticket. Size by the import-rewrite / re-type surface, not the line count. +- **Sequence low-cost first.** Convert leaf / low-fan-out files early (few downstream updates), and files whose upstream types are already TypeScript (low fan-in), so later conversions have more typed ground to derive from. +- **When the cost won't fit one PR, reduce it before converting.** High fan-in → the upstream types may need to land first. High fan-out through a hub → decompose the hub into coherent units so each unit's fan-out is bounded (see `decompose-large-files`). Decomposition is one response to high context cost — not the only place the cost applies. +- **Keep the fan-out map honest.** A barrel / re-export file inflates apparent fan-out and hides real dependencies; importing from the actual source file (not a barrel) keeps the dependency graph — and the cost estimate — accurate. From 8483d91e835f30a2f8297e7b96f319992afc1ac1 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 17 Jul 2026 06:09:45 -0400 Subject: [PATCH 06/14] Fix migration-context-cost inaccuracies, reframe decompose how-to around unit conversion, swap in the stronger derive-types example, slim CHANGELOG - migration-context-cost: line count is a factor not a non-factor; fan-in is a reading cost (not a change/review surface), fan-out is the change surface; drop the wrong "upstream types land first" and off-topic barrel bullet - decompose-large-files: the point is converting to TS in small self-contained units, not extraction - derive-types: replace the NetworkState restatement with the reinvented-messenger + hand-copied-return example (derive via `ReturnType`) - CHANGELOG: list the domain, not each skill --- CHANGELOG.md | 2 +- .../skills/decompose-large-files/skill.md | 4 +- .../typescript/skills/derive-types/skill.md | 39 ++++++++++++------- .../skills/migration-context-cost/skill.md | 21 +++++----- 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c839195..7f0f46d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `typescript` domain (experimental) with four skills: `avoid-any` (`any` is a type-checking-off directive, not a type — substitute by position: assignee → `unknown`, assigned → `never`; two narrow exceptions: generic constraints and bivariant callback parameters), `derive-types` (derive from authoritative sources over ad-hoc declarations that duplicate, run too wide, and drift), `decompose-large-files` (decompose a large file by coherent, independently-mergeable units for modularity, maintainability, and reviewability — and to unblock incremental TS migration), and `migration-context-cost` (a file's JS→TS migration cost is dominated by context fan-in + fan-out, not line count — scope and sequence tickets by it). +- Add `typescript` domain (experimental) — TypeScript authoring and JS→TS migration guidance. ## [0.2.0] diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md index 5d91e44f..f8a13708 100644 --- a/domains/typescript/skills/decompose-large-files/skill.md +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -26,9 +26,9 @@ Extraction is worth it only when the extracted piece is a **coherent unit that c Some code should **stay** in the original file: the thin **composition / bootstrap root** that wires the modules together. Extracting the wiring itself fragments rather than clarifies — it is the one place the whole is assembled. The tell is code that references _everything_ (the central object plus the shared mutable state every cluster reads); relocating it behind a wide "params bag" just moves the tangle. -## How to extract one unit (self-contained, per #41735) +## How to convert one unit (self-contained, per #41735) -Each extraction is one self-contained change — no separate "final deletion" or "integration" ticket: +The extraction is not the point — **converting the file to TypeScript in small, self-contained units is.** Each unit is a piece you can type and review on its own instead of holding the whole file at once; pulling it into its own module is just what makes that unit self-contained and independently convertible. Each unit is one self-contained change — no separate "final deletion" or "integration" ticket: 1. **Scaffold** the module (its own file/dir, TypeScript from the start). 2. **Port** the bodies in, unchanged in behavior. diff --git a/domains/typescript/skills/derive-types/skill.md b/domains/typescript/skills/derive-types/skill.md index 84bdbe7c..242238d3 100644 --- a/domains/typescript/skills/derive-types/skill.md +++ b/domains/typescript/skills/derive-types/skill.md @@ -20,31 +20,44 @@ An ad-hoc type — one hand-defined to describe a value an authoritative type al ## A grounded example (`metamask-extension` #42583) -A `wallet-services` module hand-wrote a slice of `NetworkController` state instead of deriving it. +A `wallet-services` module hand-rolled a messenger type, re-declaring each controller action's signature and **hand-copying its return shape** inline. -🚫 Re-declared — every field optional (wider than the real, _required_ field), keyed by `string` not `Hex`, and unlinked from the source, so it drifts silently when the controller changes: +🚫 Reinvents the controller's messenger and re-states its action returns: ```typescript -type NetworkControllerState = { - networkConfigurationsByChainId?: Record< - string, - { - defaultRpcEndpointIndex?: number; - rpcEndpoints?: { networkClientId?: string }[]; - } +type TokenResolutionMessenger = { + call( + action: 'AssetsContractController:getTokenStandardAndDetails', + address: string, + // … + ): Promise< + | { + balance?: string | number | bigint | { toString(radix?: number): string }; + decimals?: string | number | bigint | { toString(radix?: number): string }; + standard?: string; + symbol?: string; + } + | undefined >; + // the other action's return is discarded entirely: + call(action: 'AssetsContractController:getBalancesInSingleCall' /* … */): Promise; }; ``` -✅ Derived — tracks the authoritative shape (`Record`), narrowed to the one field in use: +✅ Derive each return from the controller's exported action type; don't hand-copy it: ```typescript -import type { NetworkState } from '@metamask/network-controller'; +import type { AssetsContractControllerGetTokenStandardAndDetailsAction } from '@metamask/assets-controllers'; -type NetworkConfigurations = NetworkState['networkConfigurationsByChainId']; +// the action already types its own return — derive it +type TokenDetails = ReturnType< + AssetsContractControllerGetTokenStandardAndDetailsAction['handler'] +>; ``` -A too-wide copy does not save work; it moves the work downstream. The same PR typed a dependency as `getMetaMaskState: () => Record`, so every consumer then had to re-cast the shape back by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. Deriving that dependency from the authoritative state type deletes the casts. The same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand; the discipline is extending it to every referenced type. +The messenger itself should extend the controller's `RestrictedMessenger` parameterized with those exported action types, so every `call` signature comes from the controller rather than a hand-rolled overload. The hand-rolled version is worse than a plain duplicate: one return is hand-copied (already looser than the controller's real type), the other (`Promise`) discards the type entirely. + +The same PR also typed a dependency `getMetaMaskState: () => Record`, which forced every consumer to re-cast the shape by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. That downstream cast tax is what a too-wide type always imposes; deriving the dependency from the authoritative state type deletes it. Notably the same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand — the discipline is extending it to every referenced type. ## Rule diff --git a/domains/typescript/skills/migration-context-cost/skill.md b/domains/typescript/skills/migration-context-cost/skill.md index 9ff9b6d6..ea1ca3b4 100644 --- a/domains/typescript/skills/migration-context-cost/skill.md +++ b/domains/typescript/skills/migration-context-cost/skill.md @@ -1,27 +1,26 @@ --- name: migration-context-cost -description: A file's JS→TS migration cost is dominated by context fan-in (upstream files whose source types to derive from) + fan-out (downstream files that import it and must update), not its line count. Scope and sequence migration tickets by this cost. +description: A file's JS→TS migration cost is driven mostly by context fan-in (upstream types it must read to derive from) and fan-out (downstream files that import it and must be updated), not by its line count. Scope and sequence migration tickets by it. maturity: experimental --- # TypeScript Migration Context Cost — Fan-In and Fan-Out -The cost of converting a file to TypeScript is not its line count. It is the **context the conversion pulls in** — everything the converter, human or AI, must load to do it correctly. +A file's line count is a factor in how hard it is to convert to TypeScript, but usually not the dominant one. The dominant cost is the **context the conversion pulls in** — everything the converter, human or AI, must read or touch to do it correctly. -## The two axes +## The two axes are different kinds of cost -- **Fan-in (upstream source types).** To type the file's values correctly you must load every upstream module whose types the code should _derive_ from — controller state, function returns, library exports, schemas (see `derive-types`). A file that touches many upstream types has high fan-in even if it is short. -- **Fan-out (downstream consumers).** Changing the file's types ripples to every module that imports it; each may need its own update or re-type. A widely-imported file has high fan-out even if it is small. +- **Fan-in (upstream source types) — a _reading_ cost.** To type the file's values correctly you must load every upstream module whose types the code should _derive_ from: controller state, function returns, library exports, schemas (see `derive-types`). You read these; you do not change them, and they are usually already TypeScript. A file that derives from many sources is expensive to hold in context even if it is short. +- **Fan-out (downstream consumers) — a _change_ cost.** Re-typing the file ripples to every module that imports it; each may need its own update. These are files you actually edit, so fan-out — together with the file itself — is the **change and review surface**. A widely-imported file has a large change surface even if it is small. -The real work — and the real review surface — is the sum of these, not the target file's length. A 200-line hub imported by 100 files can be a larger migration than a self-contained 2,000-line leaf. +The two are not the same kind of cost: fan-in is what you must _read_ to get the types right; fan-out is what you must _modify_. A 200-line hub imported by 100 files can be a larger migration than a self-contained 2,000-line leaf. ## Why it matters even for AI -Single-pass conversion of a high-fan-in/fan-out file is impractical even for a capable AI: it must hold the target file in context **and** progressively load every upstream source-type file **and** every downstream consumer. That context fan-in/fan-out is the binding constraint, not the model's ability to read the file itself. +Single-pass conversion of a high-fan-in/fan-out file is impractical even for a capable AI: it must hold the target file in context **and** read every upstream source-type file **and** edit every downstream consumer. That context load plus change surface — not the model's ability to read the file itself — is the binding constraint. ## How to use it -- **Scope tickets by context cost, not LOC.** Estimate fan-in (upstream types the file derives from) + fan-out (`grep -rl` importer count) before sizing a migration ticket. Size by the import-rewrite / re-type surface, not the line count. -- **Sequence low-cost first.** Convert leaf / low-fan-out files early (few downstream updates), and files whose upstream types are already TypeScript (low fan-in), so later conversions have more typed ground to derive from. -- **When the cost won't fit one PR, reduce it before converting.** High fan-in → the upstream types may need to land first. High fan-out through a hub → decompose the hub into coherent units so each unit's fan-out is bounded (see `decompose-large-files`). Decomposition is one response to high context cost — not the only place the cost applies. -- **Keep the fan-out map honest.** A barrel / re-export file inflates apparent fan-out and hides real dependencies; importing from the actual source file (not a barrel) keeps the dependency graph — and the cost estimate — accurate. +- **Scope tickets by context cost, not LOC.** Before sizing a migration ticket, estimate fan-in (how many upstream types it must derive from) and fan-out (`grep -rl` importer count). Size by the read-context plus the change surface, not the line count. +- **Sequence low-fan-out first.** Convert leaf / low-fan-out files early — few downstream edits per PR. Files whose upstream types are already TypeScript are cheaper on the fan-in side and give later conversions more typed ground to derive from. +- **When the change surface won't fit one PR, reduce it first.** A hub with high fan-out is the case to decompose: split it into coherent units so each unit's fan-out — and thus each PR — is bounded (see `decompose-large-files`). Decomposition is one response to high context cost, not the only place the cost applies. From 801758fa0f682296ac676f1e1b90a1dbd69622f0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 17 Jul 2026 06:50:52 -0400 Subject: [PATCH 07/14] Reframe decompose how-to: identifying the boundaries is the key; extraction is optional --- domains/typescript/skills/decompose-large-files/skill.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md index f8a13708..dc1ccbf5 100644 --- a/domains/typescript/skills/decompose-large-files/skill.md +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -26,9 +26,13 @@ Extraction is worth it only when the extracted piece is a **coherent unit that c Some code should **stay** in the original file: the thin **composition / bootstrap root** that wires the modules together. Extracting the wiring itself fragments rather than clarifies — it is the one place the whole is assembled. The tell is code that references _everything_ (the central object plus the shared mutable state every cluster reads); relocating it behind a wide "params bag" just moves the tangle. -## How to convert one unit (self-contained, per #41735) +## Identifying the boundaries is the key — extraction is optional -The extraction is not the point — **converting the file to TypeScript in small, self-contained units is.** Each unit is a piece you can type and review on its own instead of holding the whole file at once; pulling it into its own module is just what makes that unit self-contained and independently convertible. Each unit is one self-contained change — no separate "final deletion" or "integration" ticket: +**The key step is identifying the coherent boundaries.** Once you know them, you can convert the file to TypeScript in small, self-contained units — type and review one cluster at a time — instead of holding the whole file at once. That is true whether or not you physically move anything: the boundary map is what makes incremental conversion possible, and documenting it (one unit per ticket) is the deliverable. + +**Extraction — moving a unit into its own module — is optional.** It buys modularity, reviewability, and a bounded per-unit change surface, so it is often worth doing, but you convert in units because you identified the boundaries, not because you moved the code. Extract where the move adds value; leave a cluster in place where it doesn't. + +When you *do* extract a unit, it is one self-contained change — no separate "final deletion" or "integration" ticket: 1. **Scaffold** the module (its own file/dir, TypeScript from the start). 2. **Port** the bodies in, unchanged in behavior. From 712d17a406665e6357221f3cb0796ab75ef1ca88 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 08:09:53 -0400 Subject: [PATCH 08/14] =?UTF-8?q?Drop=20the=20CHANGELOG=20entry=20?= =?UTF-8?q?=E2=80=94=20it=20is=20for=20the=20CLI=20package,=20not=20skills?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHANGELOG.md tracks consumer-facing changes to the `@metamask/skills` package, per CONTRIBUTING's "CLI / tooling changes" section. No merged skill-only PR adds an entry (#80, #78, #70, #62, #61 all touch zero changelog lines). It was also the sole source of this branch's conflict with `main`, since every skill PR edits the same `[Unreleased]` block. --- CHANGELOG.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f0f46d6..8557ff8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Added - -- Add `typescript` domain (experimental) — TypeScript authoring and JS→TS migration guidance. - ## [0.2.0] ### Added From 8dc5b86b0da7720f0d6270f8a969a2570679eb74 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 11:22:59 -0400 Subject: [PATCH 09/14] Fold in `typescript-compiler-blindspots`, moved from the `testing` domain Was a separate PR against `domains/testing`. It belongs here: its subject is whether a hand-written type agrees with its authoritative source, which is the question `derive-types` answers from the authoring side, and it shares this domain's premise that a green `tsc` is not evidence the types are correct. Directory name and frontmatter `name` already agree; only the domain moved. --- .../references/false-negatives.md | 259 +++++++++++++++++ .../references/metamask-extension.md | 54 ++++ .../references/worked-example.md | 92 ++++++ .../scripts/substitution-ab.sh | 51 ++++ .../typescript-compiler-blindspots/skill.md | 272 ++++++++++++++++++ 5 files changed, 728 insertions(+) create mode 100644 domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md create mode 100644 domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md create mode 100644 domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md create mode 100755 domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh create mode 100644 domains/typescript/skills/typescript-compiler-blindspots/skill.md diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md b/domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md new file mode 100644 index 00000000..5bcf97b8 --- /dev/null +++ b/domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md @@ -0,0 +1,259 @@ +# The standing blind spots — where `tsc` returns a false negative + +Defects the compiler cannot report, independent of any particular PR. Unlike the +restated-type class, these need no substitution to find: they are properties of +the language and the config, and they are present in every file. + +**Verified, not asserted.** The demonstration file below was typechecked against +`metamask-extension` at `7fafda0` with the repo's own `tsconfig.json`: + +``` +$ NODE_OPTIONS='--max-old-space-size=9216' npx tsc -p tsconfig.json --noEmit +$ echo $? +0 +``` + +Every block in it is wrong. `tsc` reports **zero errors** on all ten. + +--- + +## A. Unsoundness in the type system + +### 1. Index access is not `| undefined` + +```ts +const parts = host.split('.'); +return parts[9]; // typed `string`; `undefined` at runtime +``` + +`arr[i]` and `record[key]` are typed as if the element always exists. Easy to underestimate how much surface this covers: every `.split()[n]`, every +lookup table, every `find`-then-index is an instance. + +- **Flag:** `noUncheckedIndexedAccess` (not enabled in `metamask-extension`). +- **In review:** any index or dynamic key access on a path that can be empty or + short. `parts[parts.length - 1]` is only safe if the array is provably non-empty. + +### 2. Optional property vs. explicit `undefined` + +```ts +type PopupState = { currentPopupId?: number }; +const cleared: PopupState = { currentPopupId: undefined }; // accepted +``` + +`?:` means "may be absent"; without the strict flag it *also* accepts +present-and-undefined. Code that distinguishes the two (`'k' in obj`, +`Object.keys().length`, serialization that drops vs. writes `null`) breaks on a +distinction the type cannot express. + +- **Flag:** `exactOptionalPropertyTypes` (not enabled). +- **In review:** persisted state and message payloads, where absent and + `undefined` serialize differently. + +### 3. Method parameters are bivariant + +```ts +type MessageHandler = { handle(msg: { kind: 'booted' | 'connectivity' }): void }; +const narrow: MessageHandler = { handle(msg: { kind: 'booted' }) { … } }; // accepted +``` + +`strictFunctionTypes` makes function *properties* contravariant but exempts +**method shorthand** — deliberately, for DOM/array compatibility. So a handler +that only accepts a narrow subtype satisfies a wide handler type and receives +values it declared it would not. + +- **Fix:** declare callbacks as properties (`handle: (msg: …) => void`), which + *is* checked. +- **In review:** any interface with method-shorthand callbacks, especially + message/event handlers. + +### 4. Arrays are covariant + +```ts +const bases: Base[] = specials; // Special[] → Base[], accepted +bases.push(new Base()); // `specials` now holds a non-Special +``` + +- **In review:** a narrower array widened and then mutated. `readonly T[]` blocks it. + +### 5. Excess-property checking only fires on fresh literals + +```ts +const draft = { url: 'a', justification: 'b', reasosn: ['typo'] }; +const params: CreateParams = draft; // no error — not a fresh literal +``` + +Assigning the literal directly would catch the typo. Through a variable, the +extra property is silently ignored — and the intended one is missing. + +- **In review:** config/params objects built up in a variable before being passed. + This is how a misspelled option key survives to runtime. + +### 6. Structural typing erases domain distinctions + +```ts +type AccountAddress = string; +type TransactionHash = string; +fetchBalance(txHash); // accepted — both are `string` +``` + +Aliases are not nominal. Two semantically incompatible values are interchangeable +whenever their structure matches. + +- **Fix:** branded types, or a template-literal type where the format differs + (`Hex` = `` `0x${string}` `` genuinely does discriminate). +- **In review:** same-primitive parameters, especially adjacent ones in a + signature, where swapping the arguments would still compile. + +## B. Boundaries the compiler does not cross + +### 7. `any` absorbs any annotation + +```ts +declare function readPersisted(key: string): any; +const meta: VaultMeta = readPersisted('meta'); // asserted, never validated +meta.version.toFixed(2); // may be a string at runtime +``` + +An `any` satisfies every annotation silently. Sources: untyped dependencies, +`JSON.parse`, generics that default to `any`, and `as any`. + +- **In review:** trace where a confidently-typed value *entered* the program. If + it entered as `any`, its type is a wish. + +### 8. Ambient `declare module` is an unverified assertion + +```ts +declare module '@ensdomains/content-hash' { + const contentHash: { decode: (h: string) => string /* … */ }; + export default contentHash; +} +``` + +Hand-written module declarations are believed unconditionally — nothing compares +them to the package. Getting a return type wrong here is invisible forever, and +the declaration is **global**, so it also shadows any real types the package +later ships. + +- **In review:** read the package's actual source at the installed version when a + `declare module` is added or changed. Prefer `@types/*` or a PR upstream. + +### 9. External data is asserted, not validated + +```ts +const chainId = (rpcResult as { chainId: string }).chainId.slice(2); +``` + +Every `as` on data crossing a boundary — RPC responses, `fetch().json()`, +`chrome.storage` reads, persisted state written by an *older version of the app* — +is a claim the compiler cannot evaluate. + +- **In review:** highest stakes for persisted state and migrations, where the real + input was produced by code that no longer exists. A runtime validator + (`@metamask/superstruct`, zod) is the only thing that actually checks. + +### 10. A JS caller is not checked at all + +With `checkJs` off (the default, and the case in `metamask-extension`), a type +written for a function whose callers are still `.js` is compared against no call +site, ever. See the restated-type class in the main skill — this is why that class +exists. + +## C. Config-level blind spots + +Check these before trusting a green build. Values shown are `metamask-extension` +at the time of writing. + +| Setting | Effect | Here | +|---|---|---| +| `skipLibCheck` | Errors *inside* `.d.ts` files are suppressed, including conflicts between library types | **`true`** (from `@tsconfig/node22`) | +| `exclude` | Excluded files are never typechecked | `**/*.stories.tsx`, `**/*.stories.ts` | +| `include` | Anything outside it is invisible to `tsc` | `app`, `development`, `shared`, `test`, `types`, `ui`, `*.ts` | +| `checkJs` | Off ⇒ `.js` callers unchecked | unset | +| `noEmit` + bundler | `tsc` never produces the shipped artifact; webpack/swc transpiles **without** typechecking, so a type error cannot break the build — only the separate `lint:tsc` job reports it | `noEmit: true` | +| `@ts-expect-error` / `@ts-ignore` | Point suppressions | grep before trusting a clean file | + +The last row is worth stating plainly: **type errors do not break the build.** They +break a CI job. If that job is skipped, filtered, or its output is not read, the +types were never checked at all. + +--- + +## The demonstration file + +Drop this anywhere inside the `include` paths and typecheck. Zero errors is the +expected — and alarming — result. + +```ts +/* eslint-disable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unused-vars */ + +// 1. Index access is not `| undefined` +function lastSegment(host: string): string { + const parts = host.split('.'); + return parts[9]; // typed `string`; `undefined` at runtime +} +export const seg = lastSegment('foo.eth').toUpperCase(); + +// 2. Record lookup claims the value always exists +declare const gateways: Record; +export const gw = gateways.definitelyNotAKey.url; + +// 3. Optional property accepts an explicit undefined +type PopupState = { currentPopupId?: number }; +const cleared: PopupState = { currentPopupId: undefined }; +export const idPlusOne = (cleared.currentPopupId ?? 0) + 1; + +// 4. Method-shorthand parameters are bivariant +type MessageHandler = { handle(msg: { kind: 'booted' | 'connectivity' }): void }; +const narrow: MessageHandler = { handle(msg: { kind: 'booted' }) {} }; +export { narrow }; + +// 5. Arrays are covariant +class Base {} +class Special extends Base { + special() { + return 1; + } +} +const specials: Special[] = [new Special()]; +const bases: Base[] = specials; +bases.push(new Base()); +export const boom = () => specials.map((s) => s.special()); + +// 6. Excess-property checking only fires on fresh literals +type CreateParams = { url: string; justification: string }; +const draft = { url: 'a', justification: 'b', reasosn: ['typo'] }; +export const params: CreateParams = draft; + +// 7. `any` absorbs any annotation +declare function readPersisted(key: string): any; +type VaultMeta = { version: number; storageKind: 'data' | 'split' }; +export const meta: VaultMeta = readPersisted('meta'); +export const ver = meta.version.toFixed(2); + +// 8. An ambient `declare module` is an unverified assertion +import contentHash from '@ensdomains/content-hash'; + +export const decoded: string = contentHash.decode('0x'); + +// 9. Structural typing erases domain distinctions +type AccountAddress = string; +type TransactionHash = string; +declare function fetchBalance(addr: AccountAddress): Promise; +declare const txHash: TransactionHash; +export const wrong = fetchBalance(txHash); + +// 10. A type assertion on external data is unchecked by construction +declare const rpcResult: unknown; +export const chainId = (rpcResult as { chainId: string }).chainId.slice(2); +``` + +## How to use the catalog in a review + +Don't run all ten as a checklist. Pick by what the diff touches: + +- **New indexing / destructuring** → 1, 2 +- **New message, event, or callback types** → 3, 5, 6 +- **New `declare module`, new dependency, `@types` change** → 8 +- **Anything reading persisted state, storage, or an RPC response** → 7, 9 +- **A JS→TS conversion** → 10, plus the restated-type class in the main skill +- **Any PR whose safety argument is "CI is green"** → section C, first diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md b/domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md new file mode 100644 index 00000000..36a67496 --- /dev/null +++ b/domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md @@ -0,0 +1,54 @@ +# Repo notes — metamask-extension + +Specifics for running the two-arm proof in `MetaMask/metamask-extension`. (Kept +here rather than in `repos/` on purpose: a `repos/` subdir containing only an +extension overlay would make this skill *skip* installs for mobile and core.) + +## Typecheck invocation + +```bash +NODE_OPTIONS='--max-old-space-size=9216' npx tsc -p tsconfig.json --noEmit +``` + +`package.json`'s `lint:tsc` uses `--max-old-space-size=6144`, which **OOMs** on a +full run on a 16 GB machine — and the OOM exits non-zero with no type diagnostics, +so a naive exit-code check reads it as "errors found." Raise the heap and read the +output. A full run takes roughly 3–5 minutes. + +## Where to put probes + +Anywhere under the `include` list — `app`, `development`, `shared`, `test`, +`types`, `ui`. `app/scripts/derive-probe/` works. Delete it afterwards; it is +inside the build's include paths. + +## What the compiler is *not* checking + +- **`checkJs` is unset** and there is no `// @ts-check` in `app/scripts/background.js`. + `background.js` is the sole caller of much of `app/scripts/lib/**`, so a type + written for those functions is validated against **nothing**. This is where + migration PRs accumulate silent divergence, and where this skill pays. +- `tsconfig.json` sets `lib: ["DOM", "es2023"]`, overriding the base. **`webworker` + is absent**, so service-worker globals (`clients`, `Clients`, `Client`) have no + authoritative type in scope — hand-declaring them is legitimate here. +- Strictness comes from `@tsconfig/node22` (`strict: true`), so `strictNullChecks` + is on and nullability divergences do surface in a probe. + +## Authoritative sources worth knowing + +| Looking for | Derive from | +|---|---| +| current chain id | `ReturnType` (`shared/lib/selectors/networks.ts`) → `Hex`, not `string` | +| the EIP-1193 provider | `ReturnType['provider']` — note the `\| undefined` | +| a controller method's params | `SomeController['methodName']` | +| persisted state root | `MetaMaskStorageStructure` (`shared/lib/stores/base-store.ts`) | +| a `browser.*` listener payload | `browser.WebRequest.OnErrorOccurredDetailsType` and siblings, from `webextension-polyfill` | +| offscreen message targets/events | the enums in `shared/constants/offscreen-communication.ts` | + +## Before "fixing" a `chrome.*` type error + +Read the declaration in `node_modules/@types/chrome/index.d.ts` first. Several +parameters are template-literal **string** types, not the enums they mirror — e.g. +`ContextFilter.contextTypes?: ` `` `${ContextType}`[] `` and +`CreateParameters.reasons: ` `` `${Reason}`[] ``. A plain string literal already +satisfies them, so swapping in `chrome.offscreen.Reason.X` is an unnecessary +runtime change, not a type fix. diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md b/domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md new file mode 100644 index 00000000..a7f1f978 --- /dev/null +++ b/domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md @@ -0,0 +1,92 @@ +# Worked example — a JS→TS migration PR + +[metamask-extension#44397](https://github.com/MetaMask/metamask-extension/pull/44397), +head `7fafda0`. 11 files converted, +153/−72, described as *"mostly mechanical +JS→TS with equivalent runtime logic"*, four files *"rename only"*. All CI green, +including `lint:tsc`. + +Twelve hand-written types. Nine had an authoritative source. Five disagreed with it. + +## Arm A + +``` +$ NODE_OPTIONS='--max-old-space-size=9216' npx tsc -p tsconfig.json --noEmit +$ echo $? +0 +``` + +Silent — so every diagnostic below is attributable to the substitution. + +## Arm B + +Six probe files, each substituting the derived type and calling it as the real +code does: + +``` +probe-1-get-obj-structure.ts(19,47): error TS2345: Argument of type + 'MetaMaskStorageStructure | undefined' is not assignable to parameter of type + 'Record'. +probe-2-set-current-popup-id.ts(22,21): error TS2345: Argument of type 'undefined' + is not assignable to parameter of type 'number'. +probe-2-set-current-popup-id.ts(29,21): error TS2345: Argument of type + 'number | undefined' is not assignable to parameter of type 'number'. +probe-3-ens-provider.ts(27,32): error TS18048: 'provider' is possibly 'undefined'. +probe-3-ens-provider.ts(44,14): error TS2322: Type + 'SwappableProxy> | undefined' is not + assignable to type 'HandWrittenEthProvider'. +probe-4-offscreen-message.ts(43,14): error TS2322: Types of property 'target' are + incompatible. Type 'string' is not assignable to type 'OffscreenCommunicationTarget'. +probe-6-chain-id-widening.ts(31,50): error TS2322: Type '"1"' is not assignable to + type '`0x${string}`'. +``` + +A seventh probe compiled the PR's *original* string literals with **zero** errors — +which is how the two unnecessary runtime changes below were established. + +## Findings + +| Hand-written | Authoritative source | Shape | +|---|---|---| +| `_setCurrentPopupId: ((id: number \| undefined) => void)` | `AppStateController['setCurrentPopupId']` → `(id: number) => void` | widening | +| `getCurrentChainId: () => string` | `ReturnType` → `` `0x${string}` `` | widening | +| `target: string` on a received message | the sender, TypeScript in-repo → `OffscreenCommunicationTarget` | widening | +| `EthProvider` (written twice, unshared) | `ReturnType['provider']` | duplication + dropped nullability | +| `obj: Record` | already in a JSDoc `@type` on the argument at the call site: `MetaMaskStorageStructure \| undefined` | placeholder + dropped nullability | + +Two of these had a consequence beyond tidiness: + +- The widened setter is what let `setter?.(undefined)` compile. Deriving it fails — + usefully, because it surfaces a genuine mismatch between a controller method's + declared parameter and how its callers actually use it. +- The dropped nullability sat in the same edit that deleted a `= {}` default + parameter, i.e. the guard that existed *for* the nullable case. (Traced: inert + today, because a fallback upstream guarantees an object by the time the path runs.) + +**Separately**, reading `@types/chrome` before trusting a type error found two +runtime changes the types never required: `contextTypes` and `reasons` are declared +as template-literal **string** types (`` `${ContextType}`[] ``, `` `${Reason}`[] ``), +so the original `['OFFSCREEN_DOCUMENT']` / `['IFRAME_SCRIPTING']` already compiled. +The PR replaced both with runtime enum lookups, and added a redundant cast. + +## Clearances — and one that mattered + +Five claims the probes **cleared**: + +- `Promise` as a provider `request` return looked unsound. It isn't: the + real `request` is generic in its result, so a call + site may legitimately fix `Result = string`. +- Two `declare module` blocks: neither package ships types, and no `@types/*` is + installed → no authoritative source exists, so hand-writing is correct. +- A hand-declared `clients?: { matchAll }`: the authoritative `Clients` lives in + `lib.webworker.d.ts`, which is not in this repo's `tsconfig.lib` → out of scope. +- Two dependency callbacks (`() => string`, `() => boolean`) matched their sources + exactly. +- A dropped argument (`_getPopup(id)` → `_getPopup()`) was a genuine no-op: the + base function declared no parameters, so the argument was already discarded. + +**The provider clearance is the reason Step 5 exists.** The first Arm B run +reported `TS18048: 'provider' is possibly 'undefined'` on that probe — an error on +the line the return-type claim lived on, which reads as confirmation if you count +exit codes. The nullability error fired one property *ahead* of the claim. Setting +it aside with `NonNullable<…>` and re-running showed the return type compiles +clean. Reported as a finding, it would have been wrong. diff --git a/domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh b/domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh new file mode 100755 index 00000000..32d76bd1 --- /dev/null +++ b/domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# +# Two-arm type proof: does a hand-written type agree with the authoritative one? +# +# substitution-ab.sh [probe-dest] +# +# repo checked out at the PR head, deps installed +# directory of probe-*.ts files (see skill.md Step 3) +# [probe-dest] where to stage them, relative to ; must sit inside +# the tsconfig `include` paths. Default: src/__type-probe__ +# +# Arm A must be silent. If it is not, stop — nothing in Arm B is attributable. +set -uo pipefail + +REPO=${1:?usage: substitution-ab.sh [probe-dest]} +PROBES=${2:?usage: substitution-ab.sh [probe-dest]} +DEST=${3:-src/__type-probe__} + +: "${NODE_OPTIONS:=--max-old-space-size=9216}" +export NODE_OPTIONS + +cd "$REPO" || exit 1 +[ -d "$PROBES" ] || { echo "no such probe dir: $PROBES" >&2; exit 1; } + +cleanup() { rm -rf "$REPO/$DEST"; } +trap cleanup EXIT INT TERM + +echo "=== Arm A — PR head as written (must be silent) ===" +A=$(npx tsc -p tsconfig.json --noEmit 2>&1) +A_STATUS=$? +if [ -n "$A" ]; then + echo "$A" + echo + echo "!! Arm A is NOT silent (exit $A_STATUS). The comparison is INCONCLUSIVE:" + echo "!! Arm B's diagnostics cannot be attributed to the substitution." + echo "!! Fix the baseline (toolchain, lockfile, heap, project scope) before reading Arm B." + exit 2 +fi +echo "0 diagnostics — baseline clean, Arm B is attributable." +echo + +echo "=== Arm B — same commit, derived types substituted ===" +mkdir -p "$DEST" +cp "$PROBES"/probe-*.ts "$DEST"/ 2>/dev/null || { + echo "no probe-*.ts found in $PROBES" >&2; exit 1; } + +npx tsc -p tsconfig.json --noEmit 2>&1 +echo +echo "=== Each diagnostic above is a disagreement the hand-written type concealed. ===" +echo "=== Before believing any of them: confirm the diagnostic is the one the ===" +echo "=== claim needs, not an earlier cause short-circuiting it (skill.md Step 5).===" diff --git a/domains/typescript/skills/typescript-compiler-blindspots/skill.md b/domains/typescript/skills/typescript-compiler-blindspots/skill.md new file mode 100644 index 00000000..90c1bc4f --- /dev/null +++ b/domains/typescript/skills/typescript-compiler-blindspots/skill.md @@ -0,0 +1,272 @@ +--- +name: typescript-compiler-blindspots +description: >- + Find the type defects `tsc` is structurally unable to report — a green build is + not evidence the types are correct. Covers the two classes: (1) hand-written + types that restate an authoritative source and disagree with it, caught by + substituting the derived type at a fixed commit and diffing `tsc` output; and + (2) the standing blind spots in the language and config — unchecked array/record + indexing, bivariant method parameters, covariant arrays, `any` absorption at + untyped boundaries, ambient `declare module` assertions, excess-property checks + that only fire on fresh literals, and external data asserted rather than + validated. Also audits typing edits that quietly change runtime behavior: + stripped `| undefined`, deleted default parameters, literals swapped for runtime + enum lookups, calls made optional so a throw becomes a silent no-op. Use when + reviewing a JS→TS migration, a PR that hand-writes types for values that already + have them, a "rename-only" refactor, or any PR claiming a change is mechanical. + Trigger phrases include "validate this TypeScript migration", "is this type + right", "does this type match the real shape", "why didn't CI catch this type", + "derive vs define", and "what can tsc not check". +maturity: experimental +--- + +# TypeScript compiler blind spots + +A hand-written type is a **claim about a value's shape**, and it compiles whether +or not the claim is true. `tsc` checks declarations for internal **consistency** — +never for **correspondence** to the source they restate. Across a JavaScript +boundary (`checkJs` off) it checks nothing at all. + +Those are the blind spots. This skill finds what is hiding in them. + +Two classes, two methods: + +| Class | What it is | Method | +|---|---|---| +| **Restated types** | A type hand-written to describe a value that already has an authoritative type | **Substitution A/B** — swap in the derived type, diff `tsc` output | +| **Standing blind spots** | Defects the language and config cannot report at all, in any codebase | **Targeted audit** — [references/false-negatives.md](references/false-negatives.md) | + +The second class is the one that surprises people: a file of genuinely broken +code can typecheck clean. The reference includes exactly that — a demonstration +file where every block is wrong and `tsc` reports zero errors. + +Companion to the authoring rule it enforces — *derive types from authoritative +sources instead of re-declaring them* in +[contributor-docs `docs/typescript.md`](https://github.com/MetaMask/contributor-docs/blob/main/docs/typescript.md). +This skill is the review-side proof; that document is the write-side guidance. + +## When to use + +- A **JS→TS migration** PR, or one whose body says *mechanical*, *rename-only*, or *no behavior change*. +- A PR that **hand-writes a type for a value that already has one** — a controller method's parameters, a selector's return, a message payload, a package's exported shape. +- A reviewer asks "is this type actually right?" and the answer so far is "it compiles." + +Out of scope: code correctness (use a normal review), runtime behavior (use e2e / +visual proof), and lint/format (CI owns those). + +## Prerequisites + +- The repo checked out at the PR head, dependencies installed, `tsc` runnable. +- Enough heap for a full typecheck on large repos (see Troubleshooting). +- A scratch directory inside the `tsconfig` `include` paths for probe files. + +## The core idea: two arms, one commit + +Both arms sit at the **same commit**. They differ by a *substitution*, not by a +ref — so there is no build, no rebase, and no merge boundary to confound. + +| | What it is | What it must show | +|---|---|---| +| **Arm A** | The PR exactly as written | **Silent.** Zero diagnostics | +| **Arm B** | Same tree + probes that use the *derived* type, exercised as the real code exercises it | Each new diagnostic = a disagreement the hand-written type concealed | + +**Arm A must be silent or the run is inconclusive.** If the untouched tree +already emits diagnostics, nothing in Arm B is attributable to the substitution — +"N errors in Arm B" is then a count, not a finding. Publish Arm A's result +verbatim as the delivery check. + +## Instructions + +### Step 1: Inventory every type the PR hand-wrote + +```bash +gh pr diff | grep -nE '^\+.*(type [A-Z]|interface [A-Z]|: (Record<|string|number|boolean|unknown|any)\b)' +``` + +List them. Each one is a claim you are about to test. + +### Step 2: Find the authoritative source for each + +Work down this list — the first hit wins. In the worked example 9 of the 12 +hand-written types had a source, each found in under a minute: + +1. **The call site.** What is actually passed? In a JS caller, check for a JSDoc + `@type {import('…').Foo}` annotation on the variable — the answer is sometimes + literally already written down there. +2. **The class or method being wrapped** → `MyController['someMethod']`. +3. **A package already imported in the same file** — e.g. `webextension-polyfill` + defines every listener payload; if the file calls the API, the type is in reach. +4. **The sender**, for a message or event payload. If the sender is TypeScript, the + shape is derivable, not guessable. +5. **A selector's return** → `ReturnType`. +6. **`@types/*` for a platform API.** Read the actual declaration before "fixing" + a type error — many are template-literal *string* types (`` `${SomeEnum}`[] ``), + which already accept a plain string literal. +7. **No source exists** — an untyped dependency, a lib absent from `tsconfig.lib`, + a genuinely new boundary the repo owns. Hand-writing is then **correct**. Record + it as a cleared falsifier with the reason; do not report it as a finding. + +### Step 3: Write one probe per claim + +One file per claim, in a scratch dir inside the `include` paths. Each probe names +its authoritative source in a header comment and calls the derived type **the way +the real call site calls it**: + +```ts +// PROBE — src/thing.ts hand-wrote `setFoo: (id: number | undefined) => void`. +// Authoritative: FooController['setFoo'] (foo-controller.ts:120) — param is `number`. +// The real code calls it as below. +import type { FooController } from '../controllers/foo-controller'; + +declare const setFoo: FooController['setFoo']; + +export function asCalledByTheRealCode() { + setFoo(undefined); // thing.ts:105 +} +``` + +### Step 4: Run both arms + +```bash +./scripts/substitution-ab.sh +``` + +Or by hand — Arm A first, and stop if it is not silent. + +### Step 5: Isolate diagnostics that fire for the wrong reason + +A checker reports the *first* failure it reaches, so an unrelated earlier cause +can short-circuit the claim under test — and an exit-code read scores that as a +confirmation. **Assert on the specific diagnostic** (code + message + line), and +where an earlier cause intervenes, neutralise it and re-probe: + +```ts +const defined = value as NonNullable; // set nullability aside +// …now the return-type claim is the only thing left to fail +``` + +This is not hypothetical — see the worked example, where a claim that a return +type was unsound turned out **sound** once the nullability error ahead of it was +isolated. + +### Step 6: Report findings *and* clearances + +Give each claim a verdict, and say which ones the probes **cleared**. A +substitution sweep that only ever confirms is indistinguishable from one that +never isolated anything. + +## The four divergence shapes + +Four shapes to check for. All five divergences in the worked example were one of +these, which is a small sample — treat the list as a starting checklist, not a +partition: + +1. **Widening** — `string` for a `Hex`/template-literal type, `string` for an enum, + `number | undefined` for `number`. Admits values the real type rejects; worst + when a guard downstream depends on the narrower form. +2. **Dropped nullability** — the source says `| undefined`, the hand-written type + doesn't. Erases the compiler's record of why a runtime guard exists. +3. **Duplication** — the same shape written out in two files, unshared. Both copies + now need every future change. +4. **Placeholder** — `Record`, `any`, or `unknown` standing in for + a shape that is known. Pushes a cast to every use site. + +## Escape hatches are the tell + +When a diff adds a hand-written type *and* an `as`, a `!`, a new `?.`, or an +`eslint-disable` in the same region, check whether the escape hatch exists to service +the type rather than the runtime. Count them — a cluster marks where to probe first. + +## A typing change should not change runtime behavior + +The second axis, and the one whose defects reach runtime rather than staying in +the type layer. A migration PR is +allowed to add annotations; it is not allowed to change what the program *does*. +Four patterns to grep the diff for, all of which look like typing work: + +1. **A literal replaced by a runtime lookup.** `['IFRAME_SCRIPTING']` becoming + `[SomeApi.Reason.IFRAME_SCRIPTING]` adds a dependency on that object existing at + runtime. **Read the declaration first** — if the parameter is a template-literal + string type (`` `${SomeEnum}`[] ``), the literal already type-checked and the + swap bought nothing. +2. **A default parameter or fallback deleted.** `function f(x = {})` → `function f(x: T)` + removes a guard. Ask what the guard was *for*: a `| undefined` the new type just + dropped is the first candidate. Then check reachability rather than assuming either way. +3. **A call made optional.** `obj.method()` → `obj.method?.()`, added to satisfy a + hand-written `| undefined`, converts a **throw into a silent no-op**. The loud + failure was load-bearing; now the same state produces no signal at all. +4. **A widened local to keep a check alive.** `let name: string | undefined` on a + value the authoritative type calls `string`, so that an `=== undefined` branch + still compiles. If the runtime check is genuinely needed, the *input* type is + wrong — fix that instead of widening downstream. + +For each hit: state whether it is reachable, and say so plainly either way. "I +traced it and it is inert today" is a useful review finding. "This might be a bug" +is not. + +### Silent failure modes deserve their own pass + +Ask where a newly-introduced failure would *surface*. A change inside a +`try { … } catch { captureException(e); return; }` degrades a feature without +crashing — nothing goes red, no test fails, and the only signal is an error-tracker +entry nobody is watching. The same edit in a hot path would be caught in minutes. +Weight findings by observability, not just by likelihood: **an unlikely failure in a +swallowed path can outrank a likely one in a loud path.** + +## Why the build stays green regardless + +- The hand-written type **compiles by construction** — that is why it was written. +- With `checkJs` off, a type written for a function whose callers are still `.js` + is checked against **nothing** and can drift indefinitely. +- A value that arrives as `any` silently satisfies any annotation. + +So cite the green build as the *premise* of the finding, never as counter-evidence. + +## Examples + +**Worked example** — 12 hand-written types across a JS→TS migration PR, 5 confirmed +divergences and 5 cleared falsifiers, with the verbatim two-arm output: +[references/worked-example.md](references/worked-example.md). + +**Repo notes** for MetaMask Extension (heap, probe location, `checkJs` status): +[references/metamask-extension.md](references/metamask-extension.md). + +``` +User: "Validate this TS migration PR — is it really mechanical?" +Agent: inventories the 12 new types → finds the authoritative source for 9 → + writes 6 probes → Arm A silent, Arm B reports 6 diagnostics → isolates + one that fired for the wrong reason → reports 5 findings, 5 clearances. +``` + +## Troubleshooting + +### Arm A is not silent + +**Problem:** the untouched tree already emits diagnostics, so Arm B is unattributable. +**Fix:** pin the toolchain, install against the PR's own lockfile, raise the heap, or +narrow the project. If it cannot be made silent, the lane is **inconclusive** — say +so; do not report Arm B's count as findings. + +### `tsc` runs out of memory + +**Problem:** `FATAL ERROR: Ineffective mark-compacts near heap limit`. +**Fix:** raise the heap — `NODE_OPTIONS='--max-old-space-size=9216'`. Note the OOM +exits non-zero *without* type diagnostics, so a naive exit-code check reads it as +"errors found." Always look at the output, not just the status. + +### A probe errors, but not for the claimed reason + +**Problem:** the diagnostic is about an earlier property, not the claim. +**Fix:** neutralise the earlier cause (`NonNullable<…>`, a narrow assertion) and +re-run. If the claim then compiles clean, the claim was **wrong** — report it as cleared. + +### There is no authoritative source + +Not a failure. Hand-writing is correct where nothing defines the shape; record the +reason (package ships no types, lib not in `tsconfig.lib`, new boundary) so the next +reviewer doesn't re-litigate it. + +## Related + +- [contributor-docs `docs/typescript.md`](https://github.com/MetaMask/contributor-docs/blob/main/docs/typescript.md) — the write-side rule this proves. +- `unit-testing`, `integration-test` — for behavior claims; this skill proves *types*. From ab926b82bfbe0ca4abe2cb29ea1d981169ef32f7 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 08:32:19 -0400 Subject: [PATCH 10/14] Rename `typescript-compiler-blindspots` to `compiler-blindspots` The skill lives in the `typescript` domain, so the prefix repeated information already carried by the path and by every discovery surface that shows it. Installed as `mms-compiler-blindspots`. --- .../references/false-negatives.md | 0 .../references/metamask-extension.md | 0 .../references/worked-example.md | 0 .../scripts/substitution-ab.sh | 0 .../skill.md | 2 +- 5 files changed, 1 insertion(+), 1 deletion(-) rename domains/typescript/skills/{typescript-compiler-blindspots => compiler-blindspots}/references/false-negatives.md (100%) rename domains/typescript/skills/{typescript-compiler-blindspots => compiler-blindspots}/references/metamask-extension.md (100%) rename domains/typescript/skills/{typescript-compiler-blindspots => compiler-blindspots}/references/worked-example.md (100%) rename domains/typescript/skills/{typescript-compiler-blindspots => compiler-blindspots}/scripts/substitution-ab.sh (100%) rename domains/typescript/skills/{typescript-compiler-blindspots => compiler-blindspots}/skill.md (99%) diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md b/domains/typescript/skills/compiler-blindspots/references/false-negatives.md similarity index 100% rename from domains/typescript/skills/typescript-compiler-blindspots/references/false-negatives.md rename to domains/typescript/skills/compiler-blindspots/references/false-negatives.md diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md b/domains/typescript/skills/compiler-blindspots/references/metamask-extension.md similarity index 100% rename from domains/typescript/skills/typescript-compiler-blindspots/references/metamask-extension.md rename to domains/typescript/skills/compiler-blindspots/references/metamask-extension.md diff --git a/domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md b/domains/typescript/skills/compiler-blindspots/references/worked-example.md similarity index 100% rename from domains/typescript/skills/typescript-compiler-blindspots/references/worked-example.md rename to domains/typescript/skills/compiler-blindspots/references/worked-example.md diff --git a/domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh b/domains/typescript/skills/compiler-blindspots/scripts/substitution-ab.sh similarity index 100% rename from domains/typescript/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh rename to domains/typescript/skills/compiler-blindspots/scripts/substitution-ab.sh diff --git a/domains/typescript/skills/typescript-compiler-blindspots/skill.md b/domains/typescript/skills/compiler-blindspots/skill.md similarity index 99% rename from domains/typescript/skills/typescript-compiler-blindspots/skill.md rename to domains/typescript/skills/compiler-blindspots/skill.md index 90c1bc4f..0cfaa6c3 100644 --- a/domains/typescript/skills/typescript-compiler-blindspots/skill.md +++ b/domains/typescript/skills/compiler-blindspots/skill.md @@ -1,5 +1,5 @@ --- -name: typescript-compiler-blindspots +name: compiler-blindspots description: >- Find the type defects `tsc` is structurally unable to report — a green build is not evidence the types are correct. Covers the two classes: (1) hand-written From e170b394c277906fb5ba3b17e62d60eca7542fee Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 10:44:06 -0400 Subject: [PATCH 11/14] Rename `compiler-blindspots` to `tsc-blindspots` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skills install flat as `mms-`, so `domains/typescript/` does not disambiguate the name at the callsite — and "compiler" reads as React Compiler in a repo where that is a live subject. The skill is about `tsc` specifically: its own first clause is "the type defects `tsc` is structurally unable to report". Also adds the slash trigger to the description, which listed only prose phrases. --- .../references/false-negatives.md | 0 .../references/metamask-extension.md | 0 .../references/worked-example.md | 0 .../scripts/substitution-ab.sh | 0 .../{compiler-blindspots => tsc-blindspots}/skill.md | 9 +++++---- 5 files changed, 5 insertions(+), 4 deletions(-) rename domains/typescript/skills/{compiler-blindspots => tsc-blindspots}/references/false-negatives.md (100%) rename domains/typescript/skills/{compiler-blindspots => tsc-blindspots}/references/metamask-extension.md (100%) rename domains/typescript/skills/{compiler-blindspots => tsc-blindspots}/references/worked-example.md (100%) rename domains/typescript/skills/{compiler-blindspots => tsc-blindspots}/scripts/substitution-ab.sh (100%) rename domains/typescript/skills/{compiler-blindspots => tsc-blindspots}/skill.md (98%) diff --git a/domains/typescript/skills/compiler-blindspots/references/false-negatives.md b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md similarity index 100% rename from domains/typescript/skills/compiler-blindspots/references/false-negatives.md rename to domains/typescript/skills/tsc-blindspots/references/false-negatives.md diff --git a/domains/typescript/skills/compiler-blindspots/references/metamask-extension.md b/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md similarity index 100% rename from domains/typescript/skills/compiler-blindspots/references/metamask-extension.md rename to domains/typescript/skills/tsc-blindspots/references/metamask-extension.md diff --git a/domains/typescript/skills/compiler-blindspots/references/worked-example.md b/domains/typescript/skills/tsc-blindspots/references/worked-example.md similarity index 100% rename from domains/typescript/skills/compiler-blindspots/references/worked-example.md rename to domains/typescript/skills/tsc-blindspots/references/worked-example.md diff --git a/domains/typescript/skills/compiler-blindspots/scripts/substitution-ab.sh b/domains/typescript/skills/tsc-blindspots/scripts/substitution-ab.sh similarity index 100% rename from domains/typescript/skills/compiler-blindspots/scripts/substitution-ab.sh rename to domains/typescript/skills/tsc-blindspots/scripts/substitution-ab.sh diff --git a/domains/typescript/skills/compiler-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md similarity index 98% rename from domains/typescript/skills/compiler-blindspots/skill.md rename to domains/typescript/skills/tsc-blindspots/skill.md index 0cfaa6c3..495142b4 100644 --- a/domains/typescript/skills/compiler-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -1,5 +1,5 @@ --- -name: compiler-blindspots +name: tsc-blindspots description: >- Find the type defects `tsc` is structurally unable to report — a green build is not evidence the types are correct. Covers the two classes: (1) hand-written @@ -14,9 +14,10 @@ description: >- enum lookups, calls made optional so a throw becomes a silent no-op. Use when reviewing a JS→TS migration, a PR that hand-writes types for values that already have them, a "rename-only" refactor, or any PR claiming a change is mechanical. - Trigger phrases include "validate this TypeScript migration", "is this type - right", "does this type match the real shape", "why didn't CI catch this type", - "derive vs define", and "what can tsc not check". + Triggers on /tsc-blindspots, or on phrases like "validate this TypeScript + migration", "is this type right", "does this type match the real shape", + "why didn't CI catch this type", "derive vs define", and "what can tsc not + check". maturity: experimental --- From d50d94fb71013440e22937836ce63c63eee9e7b0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 3 Aug 2026 12:18:50 -0400 Subject: [PATCH 12/14] Add exit-tracing, a runtime declaration check, and a dead-module check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four additions, each of which found something the existing sections did not point at, while reviewing a JS->TS conversion. `any` absorption gains its complement: the existing bullet traces where a confidently-typed value entered, which finds nothing when the value is a module's own return. Tracing where an `any` *exits* — assigning the return to two impossible types with a known-typed sibling as control — found `any` escaping a resolver into its caller. The control line is load-bearing: without it a silent probe is indistinguishable from one that cannot fail, and running outside the project tsconfig produces errors that are the harness rather than the finding. Ethers `Contract` dynamic methods are named as a source, since the ABI is runtime data and the call reads as an ordinary typed await. It is already tracked at #31973, where one consumer declares `Promise` with a disable comment and another lets it infer; the second is the dangerous form. Ambient `declare module` verification moves from reading package source to requiring the package and checking exports, return `typeof`, and whether a default export is legitimate under esModuleInterop. Inventory now begins by checking the module is referenced at all — a dead module's types are unfalsifiable, and its conversion is a deletion candidate rather than a typing exercise. --- .../references/false-negatives.md | 46 +++++++++++++++++-- .../typescript/skills/tsc-blindspots/skill.md | 12 +++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/domains/typescript/skills/tsc-blindspots/references/false-negatives.md b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md index 5bcf97b8..fc494022 100644 --- a/domains/typescript/skills/tsc-blindspots/references/false-negatives.md +++ b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md @@ -115,10 +115,38 @@ meta.version.toFixed(2); // may be a string at runtime ``` An `any` satisfies every annotation silently. Sources: untyped dependencies, -`JSON.parse`, generics that default to `any`, and `as any`. +`JSON.parse`, generics that default to `any`, `as any` — and the one that hides +best in a typed-looking file: **dynamic methods on an ethers `Contract`**, where +the ABI is runtime data so every call returns `any` while reading as an ordinary +typed `await`. This is a known source in `metamask-extension` +([#31973](https://github.com/MetaMask/metamask-extension/issues/31973)): +`shared/lib/token-util.ts` declares it as `Promise` with a disable comment, +which is the honest form. The dangerous form is letting it infer — nothing +annotates it, so it propagates into the module's return type unflagged. - **In review:** trace where a confidently-typed value *entered* the program. If it entered as `any`, its type is a wish. +- **Also trace where an `any` *exits*.** The bullet above looks backwards from a + suspicious value; this looks forward from a module's public surface. Assign its + return to two impossible types, with a known-typed sibling as a control: + + ```ts + const { type, hash } = await resolveEnsToIpfsContentId(args); + const a: number = hash; // compiles ⇒ `hash` is any + const c: symbol = hash.whateverIWant.deeply; // compiles ⇒ ditto + const d: number = type; // MUST error ⇒ probe can discriminate + ``` + + If the nonsense compiles and the control errors, `any` is escaping into every + caller. **The control line is not optional** — without it, a probe that reports + nothing is indistinguishable from a probe that cannot fail. Run it under the + project's `tsconfig`, not a standalone `tsc` invocation, or missing ambient + declarations and `resolveJsonModule` will produce errors that are the harness + rather than the finding. +- **Precise signatures around an `any` source make it worse, not better.** A file + with a derived provider type and `hexValueIsEmpty(value: string | null | undefined)` + reads as a checked boundary while every value crossing it is unchecked. A + migration that adds those signatures is what creates the appearance. ### 8. Ambient `declare module` is an unverified assertion @@ -134,8 +162,20 @@ them to the package. Getting a return type wrong here is invisible forever, and the declaration is **global**, so it also shadows any real types the package later ships. -- **In review:** read the package's actual source at the installed version when a - `declare module` is added or changed. Prefer `@types/*` or a PR upstream. +- **In review:** verify the declaration against the installed package at runtime — + faster and more decisive than reading source: + + ```bash + node -e 'const m = require("@ensdomains/content-hash"); + console.log(Object.keys(m), "default:", typeof m.default); + console.log(typeof m.decode(m.encode("ipfs-ns", "Qm…")));' + ``` + + Check three things: **the exports exist**, **each declared signature's return + `typeof` matches**, and **whether `export default` is legitimate** — a CJS module + with `typeof m.default === 'undefined'` still warrants a default declaration *if* + `esModuleInterop` is on, and is a defect if it is not. Prefer `@types/*` or an + upstream PR over hand-writing. ### 9. External data is asserted, not validated diff --git a/domains/typescript/skills/tsc-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md index 495142b4..8cdab2cc 100644 --- a/domains/typescript/skills/tsc-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -86,6 +86,18 @@ gh pr diff | grep -nE '^\+.*(type [A-Z]|interface [A-Z]|: (Record<|string|n List them. Each one is a claim you are about to test. +**First, check each converted module is still referenced:** + +```bash +grep -rn "moduleName" --include=*.ts --include=*.js . | grep -v node_modules | grep -v '\.test\.' +``` + +If the only hits are the module's own definition and its test, the file is dead — +every type on it is unfalsifiable, because nothing constrains it and no divergence +can ever surface. This is the highest value-per-second check in a migration, and +it reorders the work: a dead module's conversion is a deletion candidate, not a +typing exercise. + ### Step 2: Find the authoritative source for each Work down this list — the first hit wins. In the worked example 9 of the 12 From a1ea24a681952793ec683115e5417a1f94fda3fa Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 05:34:03 -0400 Subject: [PATCH 13/14] Add false precision as a divergence shape and the `IsAny` probe that finds it A precise annotation fed `any` at every call site is reportable by neither `tsc` nor `no-explicit-any`, and an ambient `declare module` in the path re-mints the `any` as a confident `string`. Both arms of the probe verified against `metamask-extension`. Also fixes `avoid-any`'s frontmatter, which did not parse as YAML. --- domains/typescript/skills/avoid-any/skill.md | 22 +++++++- .../references/false-negatives.md | 52 ++++++++++++++++++- .../typescript/skills/tsc-blindspots/skill.md | 47 ++++++++++++++--- 3 files changed, 112 insertions(+), 9 deletions(-) diff --git a/domains/typescript/skills/avoid-any/skill.md b/domains/typescript/skills/avoid-any/skill.md index b9256e87..0fb55dcf 100644 --- a/domains/typescript/skills/avoid-any/skill.md +++ b/domains/typescript/skills/avoid-any/skill.md @@ -1,6 +1,13 @@ --- name: avoid-any -description: Handle `any` correctly — it is not a type but a directive that disables type checking. Substitute by position (assignee → `unknown`, assigned → `never`). Two narrow exceptions: a generic constraint, and a callback parameter caught in a bivariant position between two fixed, irresolvable function-type constraints — both declared with an inline eslint-disable. +description: >- + Handle `any` correctly — it is not a type but a directive that disables type + checking. Substitute by position (assignee → `unknown`, assigned → `never`). + Two narrow exceptions: a generic constraint, and a callback parameter caught + in a bivariant position between two fixed, irresolvable function-type + constraints — both declared with an inline eslint-disable. Also covers the + `any` that is never written down: a precise signature fed `any` at every call + site, which no lint rule and no compiler check can report. maturity: experimental --- @@ -72,3 +79,16 @@ Like the generic-constraint case, this `any` is **not infectious** — it is sco Canonical instance: a messenger `registerActionHandler` slot typed `(...args: any[]) => any` — strongly-typed handlers flow inward at registration, strongly-typed argument tuples outward at dispatch; `unknown[]` fails registration, `never[]` fails dispatch. It encodes rank-N polymorphism (`∀α. (α) => R`) that TypeScript cannot express directly. When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a TODO. Never reach for `any` to unblock feature work "to fix later." + +## Declared `any` beats absorbed `any` — and only one of them is countable + +The two exceptions above are both *declared*: the `any` is written down, an inline disable sits next to it, and a reviewer can find it with `grep`. The dangerous case is the one where **no `any` appears anywhere** and the value is `any` regardless: + +- 🚫 **Absorbed.** A precise annotation on a parameter or return that receives `any` at every call site — `hexValueIsEmpty(value: string | null | undefined)` fed ethers dynamic-method results. `no-explicit-any` never fires, because no `any` was written. `tsc` never fires, because `any` satisfies every annotation. The file reads as checked and none of it is. +- ✅ **Declared.** `): Promise` with an inline disable and a linked issue, as `shared/lib/token-util.ts` does for the same ethers API. Nothing is safer at runtime, but the claim is now honest, greppable, and countable by CI. + +The absorbed form is strictly worse than the declared one, and it is what a JS→TS conversion produces by default: the writer annotates what the value *ought* to be, and `any` accepts the annotation without comment. + +**A conversion is where the boundary's type is chosen, so an absorbed `any` is a decision, not an inheritance.** With `checkJs` off the predecessor asserted nothing; the precise signature is new. "The `any` is pre-existing" is true of the library and false of the annotation next to it. + +Detect it with `IsAny` at the call sites rather than by reading the signature — the probe, its controls, and the `declare module` composition that turns an `any` into a confident `string` are in the `tsc-blindspots` skill. The fix is to type or validate the value where it enters, so the precise annotations downstream are earned. diff --git a/domains/typescript/skills/tsc-blindspots/references/false-negatives.md b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md index fc494022..ea093572 100644 --- a/domains/typescript/skills/tsc-blindspots/references/false-negatives.md +++ b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md @@ -147,6 +147,28 @@ annotates it, so it propagates into the module's return type unflagged. with a derived provider type and `hexValueIsEmpty(value: string | null | undefined)` reads as a checked boundary while every value crossing it is unchecked. A migration that adds those signatures is what creates the appearance. +- **To ask "is this specific value `any`", use `IsAny` rather than a nonsense + assignment.** The exit probe above traces a module's public surface; this answers + the question at any single site, and its verdict is a compile error rather than an + absence of one: + + ```ts + type IsAny = 0 extends 1 & T ? true : false; + + const resolverAddress = await registryContract.resolver(hash); + const a1: IsAny = true; // silent ⇒ it IS any + const known = 'x' as string; + const a3: IsAny = true; // MUST error ⇒ probe discriminates + ``` + + `0 extends 1 & T` holds only for `any`, because `1 & any` is `any` and every type + extends `any`. **Read the polarity carefully: silence is the finding here**, which + is the reverse of every other probe in this file — so the known-`string` control + is what separates "this value is `any`" from "the probe never ran." + + Verified against `metamask-extension` with the repo's own `tsconfig.json`: four + arms — an ethers dynamic method and an explicit `as any` both silent, a known + `string` and a laundered `string` (below) both `TS2322`. ### 8. Ambient `declare module` is an unverified assertion @@ -177,6 +199,32 @@ later ships. `esModuleInterop` is on, and is a defect if it is not. Prefer `@types/*` or an upstream PR over hand-writing. +#### 7 + 8 compose: a declaration launders `any` into a confident type + +The two blind spots above are usually audited apart, and the defect lives in their +composition. §7 says an `any` propagates; §8 says a hand-written declaration is +believed. Put them in sequence and the propagation **stops** — replaced by a type +nobody checked, which every reader downstream then trusts: + +| hop | site | resulting type | who asserted it | +|---|---|---|---| +| 1 | `await resolverContract.contenthash(hash)` | `any` | ethers `readonly [key: string]: ContractFunction \| any` | +| 2 | `contentHash.getCodec(rawContentHash)` | **`string`** | a hand-written `declare module` | +| 3 | `return { type, hash: decoded }` | `{ type: string; hash: any }` | inferred from hop 2 | +| 4 | `` `https://${hash}.${type.slice(0, 4)}.${gateway}` `` | `.slice` on a `string` | inferred from hop 3 | + +Hop 2 is declared `(contentHash: string) => string` and **called with an `any`** — +so it neither rejects its input nor earns its output. By hop 4 the value is a URL +segment, and the only claim it was ever a string is a line a human wrote. + +- **Audit rule:** for every `declare module`, list its call sites and run `IsAny` on + each **argument**. A parameter declared `string` and passed `any` is the laundering + point, and it is where the annotation or the validator belongs — not at hop 4, + where the value already looks trustworthy. +- Verified in the demonstration run above: hop 1 silent under `IsAny` (it is `any`), + hop 2 `TS2322` (it is `string`), with the declaration block supplied locally so the + only variable between the two arms is the `declare module`. + ### 9. External data is asserted, not validated ```ts @@ -293,7 +341,9 @@ Don't run all ten as a checklist. Pick by what the diff touches: - **New indexing / destructuring** → 1, 2 - **New message, event, or callback types** → 3, 5, 6 -- **New `declare module`, new dependency, `@types` change** → 8 +- **New `declare module`, new dependency, `@types` change** → 8, then **7 + 8** on + its call sites — a declaration is audited against the package by default and + against its *arguments* almost never - **Anything reading persisted state, storage, or an RPC response** → 7, 9 - **A JS→TS conversion** → 10, plus the restated-type class in the main skill - **Any PR whose safety argument is "CI is green"** → section C, first diff --git a/domains/typescript/skills/tsc-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md index 8cdab2cc..e8006f05 100644 --- a/domains/typescript/skills/tsc-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -7,9 +7,11 @@ description: >- substituting the derived type at a fixed commit and diffing `tsc` output; and (2) the standing blind spots in the language and config — unchecked array/record indexing, bivariant method parameters, covariant arrays, `any` absorption at - untyped boundaries, ambient `declare module` assertions, excess-property checks - that only fire on fresh literals, and external data asserted rather than - validated. Also audits typing edits that quietly change runtime behavior: + untyped boundaries, precise signatures fed `any` at every call site, ambient + `declare module` assertions that launder an `any` into a confident type, + excess-property checks that only fire on fresh literals, and external data + asserted rather than validated. Also audits typing edits that quietly change + runtime behavior: stripped `| undefined`, deleted default parameters, literals swapped for runtime enum lookups, calls made optional so a throw becomes a silent no-op. Use when reviewing a JS→TS migration, a PR that hand-writes types for values that already @@ -168,11 +170,34 @@ Give each claim a verdict, and say which ones the probes **cleared**. A substitution sweep that only ever confirms is indistinguishable from one that never isolated anything. -## The four divergence shapes +### Severity: "pre-existing" is about the upstream `any`, not about the annotation -Four shapes to check for. All five divergences in the worked example were one of -these, which is a small sample — treat the list as a starting checklist, not a -partition: +A conversion PR invites two reflexes that both understate a false-precision +finding, and the underlying `any` is what makes each of them sound reasonable: + +- **"It's pre-existing."** Split the claim. The library's `any` is genuinely + inherited — ethers has returned `any` from dynamic contract methods since long + before this diff. The *annotation beside it* was written here: check `git log + --diff-filter=A -- ` and, for a `declare module`, whether the block itself + is added by this PR. If the file is new, nothing in it is pre-existing, because + with `checkJs` off the predecessor asserted nothing at all. A conversion is the + moment the boundary's type is **chosen**. +- **"It's a nit."** A nit is a finding that is minor *in itself*. A value crossing + a boundary unchecked, under a signature that says it was checked, is a type-safety + defect — the class this whole skill exists to surface. Scope and severity are + separate axes: a substantive finding a PR need not fix is still substantive, and + saying so costs one sentence. + +The remedy follows from which one it is. Annotating the hole and filing a TODO is +right for the inherited `any`; it is not a fix for false precision, because the +misleading signature stays exactly as it was. Type or validate the value where it +enters, so the precise annotations downstream are earned. + +## The five divergence shapes + +Five shapes to check for. Treat the list as a starting checklist, not a partition — +the first four each account for at least one divergence in the worked example, and +the fifth was found on a later pass over the same PR: 1. **Widening** — `string` for a `Hex`/template-literal type, `string` for an enum, `number | undefined` for `number`. Admits values the real type rejects; worst @@ -183,6 +208,14 @@ partition: now need every future change. 4. **Placeholder** — `Record`, `any`, or `unknown` standing in for a shape that is known. Pushes a cast to every use site. +5. **False precision** — the inverse of a placeholder: the annotation is *narrower* + than what actually arrives. `hexValueIsEmpty(value: string | null | undefined)` + on a parameter fed `any` at every call site. `tsc` cannot report it, because + `any` satisfies every annotation, and `no-explicit-any` cannot either, because + no `any` was written. The narrower the type, the more confident the file reads + and the less any of it is checked. Find it with `IsAny` at the call sites, not + by reading the signature — see + [references/false-negatives.md](references/false-negatives.md) §7. ## Escape hatches are the tell From 995f3c83f3ab5008169c31a3e18db97725e0f3e0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 08:01:36 -0400 Subject: [PATCH 14/14] Name the installed command in `tsc-blindspots`'s description The installer emits `mms-tsc-blindspots`; the description advertised `/tsc-blindspots`. --- domains/typescript/skills/tsc-blindspots/skill.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domains/typescript/skills/tsc-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md index e8006f05..a62730d9 100644 --- a/domains/typescript/skills/tsc-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -16,7 +16,7 @@ description: >- enum lookups, calls made optional so a throw becomes a silent no-op. Use when reviewing a JS→TS migration, a PR that hand-writes types for values that already have them, a "rename-only" refactor, or any PR claiming a change is mechanical. - Triggers on /tsc-blindspots, or on phrases like "validate this TypeScript + Triggers on /mms-tsc-blindspots, or on phrases like "validate this TypeScript migration", "is this type right", "does this type match the real shape", "why didn't CI catch this type", "derive vs define", and "what can tsc not check".