From b138a8d7d8d22292480651316aedaf073b10bc8d Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 09:20:53 -0400 Subject: [PATCH 1/4] =?UTF-8?q?feat(testing):=20add=20typescript-type-proo?= =?UTF-8?q?f=20skill=20A=20hand-written=20TypeScript=20type=20is=20a=20cla?= =?UTF-8?q?im=20about=20a=20value's=20shape,=20and=20it=20compiles=20wheth?= =?UTF-8?q?er=20or=20not=20the=20claim=20is=20true=20=E2=80=94=20so=20a=20?= =?UTF-8?q?green=20build=20is=20not=20evidence=20that=20the=20types=20are?= =?UTF-8?q?=20correct.=20In=20a=20partially-migrated=20repo=20the=20compil?= =?UTF-8?q?er=20often=20cannot=20tell=20you=20at=20all:=20with=20`checkJs`?= =?UTF-8?q?=20off,=20a=20type=20written=20for=20a=20function=20whose=20cal?= =?UTF-8?q?lers=20are=20still=20`.js`=20is=20checked=20against=20nothing.?= =?UTF-8?q?=20This=20skill=20makes=20the=20claim=20falsifiable=20with=20a?= =?UTF-8?q?=20two-arm=20substitution=20proof=20at=20a=20fixed=20commit:=20?= =?UTF-8?q?Arm=20A=20is=20the=20PR=20as=20written=20and=20must=20be=20sile?= =?UTF-8?q?nt,=20Arm=20B=20swaps=20the=20hand-written=20type=20for=20the?= =?UTF-8?q?=20derived=20one=20and=20exercises=20it=20as=20the=20real=20cal?= =?UTF-8?q?l=20site=20does.=20Each=20new=20diagnostic=20is=20a=20disagreem?= =?UTF-8?q?ent=20the=20hand-written=20type=20concealed.=20It=20also=20audi?= =?UTF-8?q?ts=20the=20second=20axis=20a=20migration=20can=20fail=20on=20?= =?UTF-8?q?=E2=80=94=20typing=20edits=20that=20quietly=20change=20runtime?= =?UTF-8?q?=20behavior:=20stripped=20`|=20undefined`,=20deleted=20default?= =?UTF-8?q?=20parameters,=20literals=20swapped=20for=20runtime=20enum=20lo?= =?UTF-8?q?okups,=20and=20calls=20made=20optional=20so=20a=20throw=20becom?= =?UTF-8?q?es=20a=20silent=20no-op.=20Includes=20a=20runner=20script,=20a?= =?UTF-8?q?=20worked=20example=20(12=20hand-written=20types,=205=20confirm?= =?UTF-8?q?ed=20divergences,=205=20cleared=20falsifiers),=20and=20extensio?= =?UTF-8?q?n-specific=20notes.=20Review-side=20companion=20to=20the=20"der?= =?UTF-8?q?ive=20types=20from=20authoritative=20sources"=20guideline=20in?= =?UTF-8?q?=20contributor-docs.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../references/metamask-extension.md | 54 ++++ .../references/worked-example.md | 92 +++++++ .../scripts/type-proof.sh | 51 ++++ .../skills/typescript-type-proof/skill.md | 256 ++++++++++++++++++ 4 files changed, 453 insertions(+) create mode 100644 domains/testing/skills/typescript-type-proof/references/metamask-extension.md create mode 100644 domains/testing/skills/typescript-type-proof/references/worked-example.md create mode 100755 domains/testing/skills/typescript-type-proof/scripts/type-proof.sh create mode 100644 domains/testing/skills/typescript-type-proof/skill.md diff --git a/domains/testing/skills/typescript-type-proof/references/metamask-extension.md b/domains/testing/skills/typescript-type-proof/references/metamask-extension.md new file mode 100644 index 00000000..36a67496 --- /dev/null +++ b/domains/testing/skills/typescript-type-proof/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/testing/skills/typescript-type-proof/references/worked-example.md b/domains/testing/skills/typescript-type-proof/references/worked-example.md new file mode 100644 index 00000000..a7f1f978 --- /dev/null +++ b/domains/testing/skills/typescript-type-proof/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/testing/skills/typescript-type-proof/scripts/type-proof.sh b/domains/testing/skills/typescript-type-proof/scripts/type-proof.sh new file mode 100755 index 00000000..7f4903e4 --- /dev/null +++ b/domains/testing/skills/typescript-type-proof/scripts/type-proof.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# +# Two-arm type proof: does a hand-written type agree with the authoritative one? +# +# type-proof.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: type-proof.sh [probe-dest]} +PROBES=${2:?usage: type-proof.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/testing/skills/typescript-type-proof/skill.md b/domains/testing/skills/typescript-type-proof/skill.md new file mode 100644 index 00000000..23ca35b2 --- /dev/null +++ b/domains/testing/skills/typescript-type-proof/skill.md @@ -0,0 +1,256 @@ +--- +name: typescript-type-proof +description: >- + Prove whether a hand-written TypeScript type actually agrees with the + authoritative type it restates, by substituting the derived type at a fixed + commit and diffing `tsc` output. A hand-written type compiles whether or not + it is true, so a green build is not evidence — this turns the type into a + falsifiable claim. Also audits the second axis a migration can fail on — 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 or validating a + JS→TS migration, a PR that hand-writes types/interfaces 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", "prove the conversion is + mechanical", "derive vs define", and "why didn't CI catch this type". +maturity: experimental +--- + +# TypeScript type proof + +A hand-written type is a **claim about a value's shape**, and it compiles whether +or not the claim is true. So `tsc` passing tells you the code is well-formed, not +that the types are correct — and in a partially-migrated repo it often cannot +tell you, because the caller is untyped JS and is never checked at all. + +This skill makes the claim falsifiable: replace the hand-written type with the +**derived** one and let the compiler report the disagreement. + +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 practice most types have a source +and it takes under a minute to find: + +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/type-proof.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 + +Nearly every real finding is one of these: + +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. Often deletes the reason a runtime guard existed. +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, the escape hatch usually 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 most likely to ship a real defect. 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*: usually the `| undefined` that the + new type just dropped. 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 CI cannot catch any of this + +- 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 f7f6e4941c57eeb011abb1d007d76f7af6e4eb60 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 09:38:03 -0400 Subject: [PATCH 2/4] Rename `typescript-type-proof` to `typescript-compiler-blindspots` and add the false-negative catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old name described what `tsc` already does. The skill's subject is the complement: defects the compiler is structurally unable to report, because it checks declarations for internal consistency and never for correspondence to the source they restate. Adds `references/false-negatives.md` covering the class that needs no PR to find — unchecked index access, optional-vs-undefined, bivariant method parameters, covariant arrays, excess-property checks that only fire on fresh literals, structural erasure of domain types, `any` absorption, unverified `declare module`, asserted external data, and the config-level gaps (`skipLibCheck`, `exclude`, `checkJs`, and `noEmit` meaning a type error breaks a CI job rather than the build). Verified against `metamask-extension` at `7fafda0` with a demonstration file whose ten blocks are all wrong and on which `tsc` reports zero errors. --- .../references/false-negatives.md | 260 ++++++++++++++++++ .../references/metamask-extension.md | 0 .../references/worked-example.md | 0 .../scripts/substitution-ab.sh} | 6 +- .../skill.md | 51 ++-- 5 files changed, 295 insertions(+), 22 deletions(-) create mode 100644 domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md rename domains/testing/skills/{typescript-type-proof => typescript-compiler-blindspots}/references/metamask-extension.md (100%) rename domains/testing/skills/{typescript-type-proof => typescript-compiler-blindspots}/references/worked-example.md (100%) rename domains/testing/skills/{typescript-type-proof/scripts/type-proof.sh => typescript-compiler-blindspots/scripts/substitution-ab.sh} (89%) rename domains/testing/skills/{typescript-type-proof => typescript-compiler-blindspots}/skill.md (83%) diff --git a/domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md b/domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md new file mode 100644 index 00000000..0a55cfd2 --- /dev/null +++ b/domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md @@ -0,0 +1,260 @@ +# 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. This is the +single highest-frequency false negative in most codebases — every `.split()[n]`, +every lookup table, every `find`-then-index. + +- **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 this 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/testing/skills/typescript-type-proof/references/metamask-extension.md b/domains/testing/skills/typescript-compiler-blindspots/references/metamask-extension.md similarity index 100% rename from domains/testing/skills/typescript-type-proof/references/metamask-extension.md rename to domains/testing/skills/typescript-compiler-blindspots/references/metamask-extension.md diff --git a/domains/testing/skills/typescript-type-proof/references/worked-example.md b/domains/testing/skills/typescript-compiler-blindspots/references/worked-example.md similarity index 100% rename from domains/testing/skills/typescript-type-proof/references/worked-example.md rename to domains/testing/skills/typescript-compiler-blindspots/references/worked-example.md diff --git a/domains/testing/skills/typescript-type-proof/scripts/type-proof.sh b/domains/testing/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh similarity index 89% rename from domains/testing/skills/typescript-type-proof/scripts/type-proof.sh rename to domains/testing/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh index 7f4903e4..32d76bd1 100755 --- a/domains/testing/skills/typescript-type-proof/scripts/type-proof.sh +++ b/domains/testing/skills/typescript-compiler-blindspots/scripts/substitution-ab.sh @@ -2,7 +2,7 @@ # # Two-arm type proof: does a hand-written type agree with the authoritative one? # -# type-proof.sh [probe-dest] +# substitution-ab.sh [probe-dest] # # repo checked out at the PR head, deps installed # directory of probe-*.ts files (see skill.md Step 3) @@ -12,8 +12,8 @@ # Arm A must be silent. If it is not, stop — nothing in Arm B is attributable. set -uo pipefail -REPO=${1:?usage: type-proof.sh [probe-dest]} -PROBES=${2:?usage: type-proof.sh [probe-dest]} +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}" diff --git a/domains/testing/skills/typescript-type-proof/skill.md b/domains/testing/skills/typescript-compiler-blindspots/skill.md similarity index 83% rename from domains/testing/skills/typescript-type-proof/skill.md rename to domains/testing/skills/typescript-compiler-blindspots/skill.md index 23ca35b2..17a9f107 100644 --- a/domains/testing/skills/typescript-type-proof/skill.md +++ b/domains/testing/skills/typescript-compiler-blindspots/skill.md @@ -1,31 +1,44 @@ --- -name: typescript-type-proof +name: typescript-compiler-blindspots description: >- - Prove whether a hand-written TypeScript type actually agrees with the - authoritative type it restates, by substituting the derived type at a fixed - commit and diffing `tsc` output. A hand-written type compiles whether or not - it is true, so a green build is not evidence — this turns the type into a - falsifiable claim. Also audits the second axis a migration can fail on — 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 or validating a - JS→TS migration, a PR that hand-writes types/interfaces for values that already + 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", "prove the conversion is - mechanical", "derive vs define", and "why didn't CI catch 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 type proof +# 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. So `tsc` passing tells you the code is well-formed, not -that the types are correct — and in a partially-migrated repo it often cannot -tell you, because the caller is untyped JS and is never checked at all. +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. -This skill makes the claim falsifiable: replace the hand-written type with the -**derived** one and let the compiler report the disagreement. +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 @@ -115,7 +128,7 @@ export function asCalledByTheRealCode() { ### Step 4: Run both arms ```bash -./scripts/type-proof.sh +./scripts/substitution-ab.sh ``` Or by hand — Arm A first, and stop if it is not silent. From bc4292ec207d22059c185b53444cb914584124da Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 09:54:41 -0400 Subject: [PATCH 3/4] Replace unsupported frequency claims with counts, checks, and mechanisms Six distributional claims across the skill and its references were asserted from a single observed migration: "nearly every real finding", "most types have a source", "the escape hatch usually exists to service the type", "often deletes the reason a guard existed", "the single highest-frequency false negative in most codebases", and "the one most likely to ship a real defect". Where a count exists it is now cited (9 of 12 hand-written types had a source; all five divergences fit the four shapes, stated as a small sample rather than a partition). Where none exists the sentence is rewritten as an instruction to check or as the mechanism itself, which is what each was doing anyway. --- .../references/false-negatives.md | 5 ++--- .../typescript-compiler-blindspots/skill.md | 19 +++++++++++-------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md b/domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md index 0a55cfd2..06df3a1d 100644 --- a/domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md +++ b/domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md @@ -26,9 +26,8 @@ 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. This is the -single highest-frequency false negative in most codebases — every `.split()[n]`, -every lookup table, every `find`-then-index. +`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 diff --git a/domains/testing/skills/typescript-compiler-blindspots/skill.md b/domains/testing/skills/typescript-compiler-blindspots/skill.md index 17a9f107..afebd9e3 100644 --- a/domains/testing/skills/typescript-compiler-blindspots/skill.md +++ b/domains/testing/skills/typescript-compiler-blindspots/skill.md @@ -87,8 +87,8 @@ 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 practice most types have a source -and it takes under a minute to find: +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 @@ -157,13 +157,15 @@ never isolated anything. ## The four divergence shapes -Nearly every real finding is one of these: +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. Often deletes the reason a runtime guard existed. + 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 @@ -172,12 +174,13 @@ Nearly every real finding is one of these: ## 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, the escape hatch usually exists to service +`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 most likely to ship a real defect. A migration PR is +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: @@ -187,8 +190,8 @@ Four patterns to grep the diff for, all of which look like typing work: 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*: usually the `| undefined` that the - new type just dropped. Then check reachability rather than assuming either way. + 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. From 406c9b907edfa02c60314ac7ac39e44817f4a08b Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 10:05:14 -0400 Subject: [PATCH 4/4] Name the referent in headings instead of using a bare demonstrative A heading is read out of order, linked to directly, and shown in outlines and search results, so it has no antecedent available: "any of this" and "use this in a review" resolve only for a reader who arrived from the line above. --- .../references/false-negatives.md | 2 +- domains/testing/skills/typescript-compiler-blindspots/skill.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md b/domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md index 06df3a1d..5bcf97b8 100644 --- a/domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md +++ b/domains/testing/skills/typescript-compiler-blindspots/references/false-negatives.md @@ -247,7 +247,7 @@ declare const rpcResult: unknown; export const chainId = (rpcResult as { chainId: string }).chainId.slice(2); ``` -## How to use this in a review +## How to use the catalog in a review Don't run all ten as a checklist. Pick by what the diff touches: diff --git a/domains/testing/skills/typescript-compiler-blindspots/skill.md b/domains/testing/skills/typescript-compiler-blindspots/skill.md index afebd9e3..90c1bc4f 100644 --- a/domains/testing/skills/typescript-compiler-blindspots/skill.md +++ b/domains/testing/skills/typescript-compiler-blindspots/skill.md @@ -213,7 +213,7 @@ 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 CI cannot catch any of this +## 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`