From c318ef1fb8e50bb83dd18b8f530db08e953ac825 Mon Sep 17 00:00:00 2001 From: Zachary Lowden Date: Tue, 4 Aug 2026 13:48:02 -0500 Subject: [PATCH] fix(typecheck): make a crashed typecheck report as crashed, not as clean (#3619) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(typecheck): make a crashed typecheck report as crashed, not as clean `pnpm run typecheck` was `cross-env NODE_OPTIONS="--max_old_space_size=8192" tsc --noEmit`. When the heap cap is too small for the program graph, V8 aborts part way through checking, so tsc emits ZERO diagnostics and dies. cross-env normalises the SIGABRT to exit 1, and V8's explanation goes to stderr — so a caller that captures stdout gets an empty log, a bare non-zero exit, and no type errors anywhere in it. That is indistinguishable from a clean pass to anything that judges the run by its output, which is what people and scripts actually do (a clean run also prints nothing). Reproduced with a deliberate `const x: number = 'nope'` in `src/`: at a 4096 MB cap the run reported 0 errors and hid it completely; the same tree at 8192 MB reported it. Measured cold on a clean checkout, with that error in place as a visibility control: node 24.18.1 4096 -> OOM/0 diags 4608 -> OOM/0 diags 5120 -> pass/found 8192 -> pass/found node 22.22.2 6144 -> pass/found 8192 -> pass/found So the current 8192 is NOT at the cliff — the cliff is between 4608 and 5120, and 8192 carries ~1.6x headroom. The number is left alone deliberately: the CI runner has 16 GB, and a cap near that trades a self-describing V8 abort for a kernel OOM-kill, which says less. Raising it would only move the cliff anyway. What changes is that crossing the cliff becomes loud. `scripts/typecheck.mjs` runs tsc and classifies the outcome: - clean -> prints an explicit "typecheck: OK" line, so silence is no longer what a pass looks like - type errors -> passed through untouched, exit code preserved - crashed -> a CRASHED banner naming the cause, on stdout AND stderr (the original blind spot was a stdout-only capture), plus a ::error:: annotation under Actions - exit 0 w/ diags -> treated as a crash rather than trusted Heap exhaustion, an outside kill (out of system RAM / a container limit) and an unexplained abort are named separately, because the fix differs — an outside kill wants a LOWER cap, not a higher one. The cap is passed as an argv flag rather than via NODE_OPTIONS so an inherited NODE_OPTIONS cannot override it. Override per-run with TYPECHECK_HEAP_MB=. Covered by scripts/__tests__/typecheck.test.ts, which drives the classifier with stub typecheckers (sub-second, vs minutes for a real run). Each of the five cases was mutation-checked against the wrapper: 6/6 mutations killed, each by its own test. One mutation initially SURVIVED and exposed a real gap in the test — the crash banner is written to stderr, so asserting only on stdout let a grep-poisoning regression through; both streams are asserted now. CI already invoked this via `pnpm run typecheck` and so inherits the wrapper; the step carries a comment against being "simplified" back to a bare tsc. * fix(husky): stop the pre-push hook echoing success over a failed typecheck The hook was: npm run typecheck echo "Typecheck successful" `sh` without `set -e` runs the next line regardless of what the previous one returned, and a script's exit status is its last command's — so the `echo` became the hook's verdict. A failing typecheck on `main` printed "Typecheck successful" and the push went through. Measured against the real hook in a throwaway repo on `main`, with a stub `npm` whose exit code is controlled: npm exit hook exit (before) hook exit (after) 0 0 0 1 0 1 134 0 134 Before, all three printed "Typecheck successful". The 134 row is the case this matters most for: that is V8 aborting on heap exhaustion, which emits no diagnostics at all, so the hook was echoing success over a typecheck that had not merely failed but never finished. The failure message points at scripts/typecheck.mjs, which distinguishes the two. The branch/username guard above is unchanged, and still makes the hook a no-op off `main`. --- .github/workflows/lint.yml | 5 + .husky/pre-push | 12 ++ package.json | 2 +- scripts/__tests__/typecheck.test.ts | 115 +++++++++++++ scripts/typecheck.mjs | 240 ++++++++++++++++++++++++++++ vitest.config.mts | 6 +- 6 files changed, 378 insertions(+), 2 deletions(-) create mode 100644 scripts/__tests__/typecheck.test.ts create mode 100644 scripts/typecheck.mjs diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 97c71a4dcf..52c5867032 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -174,6 +174,11 @@ jobs: - name: Install run: pnpm install --frozen-lockfile + # Must stay `pnpm run typecheck`, not a bare `tsc --noEmit`. The script is + # a wrapper (scripts/typecheck.mjs) that sets the V8 heap cap AND + # classifies the outcome: a `tsc` that dies of heap exhaustion emits zero + # diagnostics, so a raw invocation produces an empty log that reads as a + # clean pass to anyone scanning it. The wrapper says CRASHED instead. - name: Typecheck run: pnpm run typecheck diff --git a/.husky/pre-push b/.husky/pre-push index abe05a7371..bc9adb10ef 100644 --- a/.husky/pre-push +++ b/.husky/pre-push @@ -12,6 +12,18 @@ fi echo "Running typecheck for all files" +# The exit code has to be read and re-raised. `sh` without `set -e` runs the next +# line regardless, so an unconditional "Typecheck successful" echo became the +# hook's own exit status — a failing typecheck printed success and the push went +# through. Verified: `sh -c 'false; echo ok'` exits 0. npm run typecheck +rc=$? + +if [ "$rc" -ne 0 ]; then + echo "Typecheck FAILED (exit $rc) — push blocked." + echo "If the output above has no type errors in it, the typechecker CRASHED;" + echo "scripts/typecheck.mjs says which. A crash is not a passing typecheck." + exit "$rc" +fi echo "Typecheck successful" diff --git a/package.json b/package.json index f18f4624f5..28cc6227e4 100644 --- a/package.json +++ b/package.json @@ -65,7 +65,7 @@ "size": "node scripts/bundle-budget.mjs", "deploy": "pnpm run build && pnpm run db:deploy", "postinstall": "pnpm run db:generate", - "typecheck": "cross-env NODE_OPTIONS=\"--max_old_space_size=8192\" tsc --noEmit", + "typecheck": "node scripts/typecheck.mjs", "lint": "eslint src/ --cache --cache-strategy metadata", "lint:packages": "eslint packages --ext .ts", "eslint": "cross-env TIMING=1 eslint src/ --quiet --cache --cache-strategy metadata", diff --git a/scripts/__tests__/typecheck.test.ts b/scripts/__tests__/typecheck.test.ts new file mode 100644 index 0000000000..f7b3a6bbad --- /dev/null +++ b/scripts/__tests__/typecheck.test.ts @@ -0,0 +1,115 @@ +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +/** + * `scripts/typecheck.mjs` exists to stop a CRASHED typecheck from reading as a + * clean one. The crash it was built for — V8 aborting on heap exhaustion — emits + * zero diagnostics and writes its explanation to stderr, so a caller that greps + * for type errors, or that captures only stdout, sees an empty log. + * + * These cases drive the wrapper with stub "typecheckers" that exit the way each + * real outcome does. That keeps the classifier under test at sub-second cost; a + * real repo-wide `tsc` run is minutes, so nobody would run it per-case. + */ + +const WRAPPER = path.resolve(__dirname, '../typecheck.mjs'); + +let dir: string; +const stub = (name: string, body: string) => { + const file = path.join(dir, `${name}.mjs`); + writeFileSync(file, body); + return file; +}; + +// The wrapper always appends --noEmit; stubs ignore argv. +const run = (tscPath: string) => + spawnSync(process.execPath, [WRAPPER], { + encoding: 'utf8', + env: { ...process.env, TYPECHECK_TSC_PATH: tscPath, GITHUB_ACTIONS: '' }, + }); + +const CRASH_MARKER = 'TYPECHECK CRASHED'; +const OK_MARKER = 'typecheck: OK'; +// Built by concatenation so this file's own source cannot be mistaken for a +// diagnostic by the same greps the wrapper is defending against. +const DIAGNOSTIC_MARKER = `error${' '}TS`; +const DIAGNOSTIC_LINE = `src/foo.ts(1,1): ${DIAGNOSTIC_MARKER}2322: Type 'string' is not assignable to type 'number'.`; + +beforeAll(() => { + dir = mkdtempSync(path.join(tmpdir(), 'typecheck-guard-')); +}); +afterAll(() => { + rmSync(dir, { recursive: true, force: true }); +}); + +describe('typecheck wrapper outcome classification', () => { + it('reports a clean run explicitly rather than by silence', () => { + const res = run(stub('clean', 'process.exit(0);\n')); + + expect(res.status).toBe(0); + // The success line is the point: a clean `tsc` prints nothing, which is + // byte-identical to a crash whose stderr was dropped. + expect(res.stdout).toContain(OK_MARKER); + expect(res.stdout).not.toContain(CRASH_MARKER); + }); + + it('passes ordinary type errors through untouched', () => { + const res = run( + stub('errors', `console.log(${JSON.stringify(DIAGNOSTIC_LINE)});\nprocess.exit(2);\n`) + ); + + expect(res.status).toBe(2); + expect(res.stdout).toContain(DIAGNOSTIC_MARKER); + // A real type error is not a crash, and must not be dressed up as one. + expect(res.stdout).not.toContain(CRASH_MARKER); + expect(res.stdout).not.toContain(OK_MARKER); + }); + + it('names heap exhaustion when V8 aborts with no diagnostics', () => { + const res = run( + stub( + 'oom', + [ + "console.error('<--- Last few GCs --->');", + "console.error('FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory');", + 'process.abort();', + ].join('\n') + ) + ); + + expect(res.status).not.toBe(0); + // Loud on stdout, because the log that hid this originally was stdout-only. + expect(res.stdout).toContain(CRASH_MARKER); + expect(res.stdout).not.toContain(OK_MARKER); + expect(res.stderr).toContain('ran out of old-space heap'); + // And the wrapper's own crash report must not contain the marker callers + // grep for, or the report itself reads as a type error. Checked on BOTH + // streams: the report is written to stderr, so a stdout-only assertion here + // passes with the marker reintroduced (it did, until this line was added). + expect(res.stdout).not.toContain(DIAGNOSTIC_MARKER); + expect(res.stderr).not.toContain(DIAGNOSTIC_MARKER); + }); + + it('distinguishes an outside kill from V8 running out of heap', () => { + const res = run(stub('sigkill', "process.kill(process.pid, 'SIGKILL');\n")); + + expect(res.status).not.toBe(0); + expect(res.stdout).toContain(CRASH_MARKER); + // Different cause, different fix: more RAM / a LOWER cap, not a higher one. + expect(res.stderr).toContain('killed from outside'); + expect(res.stderr).not.toContain('ran out of old-space heap'); + }); + + it('refuses to call it a pass when tsc exits 0 while printing diagnostics', () => { + const res = run( + stub('contradiction', `console.log(${JSON.stringify(DIAGNOSTIC_LINE)});\nprocess.exit(0);\n`) + ); + + expect(res.status).not.toBe(0); + expect(res.stdout).toContain(CRASH_MARKER); + expect(res.stdout).not.toContain(OK_MARKER); + }); +}); diff --git a/scripts/typecheck.mjs b/scripts/typecheck.mjs new file mode 100644 index 0000000000..6add5bebfe --- /dev/null +++ b/scripts/typecheck.mjs @@ -0,0 +1,240 @@ +#!/usr/bin/env node +/** + * Runs the full-repo `tsc --noEmit` and classifies the OUTCOME, rather than + * leaving that to whoever reads the log. + * + * Why this exists + * --------------- + * `tsc` on this repo needs a raised V8 old-space (the default cap is far below + * what the program graph costs). When the cap is too low, V8 aborts: + * + * FATAL ERROR: Ineffective mark-compacts near heap limit + * Allocation failed - JavaScript heap out of memory + * + * The abort happens PART WAY THROUGH checking, so the process emits **zero** + * `error TS` lines and dies. Through `pnpm run`, that surfaces as + * `ELIFECYCLE Command failed with exit code 1` — a non-zero exit with a log + * that contains no type errors at all. + * + * That is byte-for-byte indistinguishable from a clean run to anything that + * judges the typecheck by its OUTPUT ("no `error TS` lines => clean"), which is + * what humans and scripted checks usually do. Measured on this repo: with a + * deliberate `const x: number = 'nope'` in `src/`, a run capped at 4096 MB + * reported 0 errors and hid it completely, while the same tree at 8192 MB + * reported it. Raising the cap only moves that cliff — the codebase grows into + * it again — so the number alone is not the fix. This wrapper makes the crash + * LOUD and unmistakably distinct from both "clean" and "you have type errors". + * + * Classification + * -------------- + * exit 0, no `error TS` -> clean. + * exit != 0, has `error TS` -> ordinary type errors; exit code passed through. + * exit != 0, no `error TS` -> CRASH. Never report this as a typecheck result. + * Names heap exhaustion / kernel OOM-kill / + * unknown-crash separately, since the fixes differ. + * exit 0, has `error TS` -> defensive: tsc contradicting itself, treated as + * a crash rather than trusted. + * + * Heap size + * --------- + * Measured cold (no .tsbuildinfo) on a clean checkout, with a deliberate type + * error in `src/` as a visibility control: + * + * node 24.18.1 4096 MB -> OOM, 0 diagnostics 4608 MB -> OOM, 0 diagnostics + * 5120 MB -> pass, error found 8192 MB -> pass, error found + * node 22.22.2 6144 MB -> pass, error found 8192 MB -> pass, error found + * + * So the cliff sits between 4608 and 5120 MB, and the 8192 default carries ~1.6x + * headroom over it. It is deliberately NOT raised further: the CI runner has + * 16 GB, and a cap approaching that trades a self-describing V8 abort for a + * kernel OOM-kill, which says less. Override per-run with TYPECHECK_HEAP_MB. + * When the codebase does grow into 8192, the crash report below is what says so + * — out loud — instead of a silently empty log. + */ + +import { spawn } from 'node:child_process'; +import { createRequire } from 'node:module'; + +const require = createRequire(import.meta.url); + +const DEFAULT_HEAP_MB = 8192; +const heapMb = Number(process.env.TYPECHECK_HEAP_MB || DEFAULT_HEAP_MB); +if (!Number.isFinite(heapMb) || heapMb <= 0) { + console.error( + `typecheck: TYPECHECK_HEAP_MB must be a positive number, got "${process.env.TYPECHECK_HEAP_MB}"` + ); + process.exit(2); +} + +// TYPECHECK_TSC_PATH is a test seam: it lets the suite drive this classifier +// with a stub that exits the way a crashed / clean / erroring tsc does, without +// paying a multi-minute real typecheck per case. Not meant for normal use. +let tscPath = process.env.TYPECHECK_TSC_PATH; +if (!tscPath) { + try { + tscPath = require.resolve('typescript/lib/tsc.js'); + } catch { + console.error('typecheck: cannot resolve typescript/lib/tsc.js — run `pnpm install` first.'); + process.exit(2); + } +} + +const tscArgs = process.argv.slice(2); +const startedAt = Date.now(); +const child = spawn( + process.execPath, + [`--max-old-space-size=${heapMb}`, tscPath, '--noEmit', ...tscArgs], + { + // NODE_OPTIONS is dropped on purpose: the heap cap is passed as an argv flag + // so an inherited NODE_OPTIONS cannot silently override it, and so the value + // reported below is the value actually in force. + env: { ...process.env, NODE_OPTIONS: '' }, + stdio: ['inherit', 'pipe', 'pipe'], + } +); + +// Counted while streaming rather than buffered: a badly broken tree can emit +// tens of thousands of diagnostics, and this wrapper must not become the thing +// that runs out of memory. +let errorTsLines = 0; +let sawHeapOom = false; +const tail = []; +const TAIL_LINES = 40; + +const OOM_SIGNATURES = [ + 'JavaScript heap out of memory', + 'Ineffective mark-compacts near heap limit', + 'FATAL ERROR', +]; + +function onLine(line) { + // `tsc` diagnostics are `path(line,col): error TS####: message`. `--pretty` + // is not used by this script, so the plain form is what arrives. + if (line.includes('error TS')) errorTsLines++; + if (OOM_SIGNATURES.some((sig) => line.includes(sig))) sawHeapOom = true; + tail.push(line); + if (tail.length > TAIL_LINES) tail.shift(); +} + +function wire(stream, sink) { + let residual = ''; + stream.on('data', (chunk) => { + sink.write(chunk); + // Split on the residual + chunk so a signature straddling a chunk boundary + // is still seen whole. + const text = residual + chunk.toString('utf8'); + const lines = text.split('\n'); + residual = lines.pop() ?? ''; + for (const line of lines) onLine(line); + }); + stream.on('end', () => { + if (residual) onLine(residual); + }); +} + +wire(child.stdout, process.stdout); +wire(child.stderr, process.stderr); + +function annotate(message) { + // Surfaces in the checks UI when running under Actions; harmless elsewhere. + if (process.env.GITHUB_ACTIONS === 'true') console.log(`::error::${message}`); +} + +function reportCrash({ code, signal, reason, remedy }) { + const how = signal ? `killed by signal ${signal}` : `exit code ${code}`; + // The detail goes to stderr, but a one-line verdict also goes to STDOUT. + // The failure this whole wrapper exists to catch was found by a caller that + // redirected stdout and dropped stderr: V8's abort is written to stderr, so + // that caller saw an empty log and a bare non-zero exit. A crash has to be + // visible in whichever stream the reader happens to be capturing. + console.log(`TYPECHECK CRASHED (${how}): ${reason}. NOT a clean typecheck — see stderr.`); + console.error(''); + console.error('================================================================'); + console.error(' TYPECHECK CRASHED — THIS IS NOT A TYPECHECK RESULT'); + console.error('================================================================'); + // Nothing this wrapper prints may contain the literal diagnostic marker that + // callers grep for — otherwise a crash report reads as a type error to them. + console.error(` tsc ${how} after emitting 0 diagnostics.`); + console.error(` Cause: ${reason}`); + console.error(''); + console.error(' The check did not finish, so it says NOTHING about whether the'); + console.error(' code typechecks. Do not read the absence of errors as a pass.'); + console.error(''); + console.error(` Fix: ${remedy}`); + console.error(` Heap cap in force: --max-old-space-size=${heapMb} (MB).`); + console.error(' Override with TYPECHECK_HEAP_MB= pnpm run typecheck'); + if (tail.length) { + console.error(''); + console.error(` Last ${tail.length} line(s) of tsc output:`); + for (const line of tail) console.error(` ${line}`); + } + console.error('================================================================'); + annotate(`Typecheck crashed (${how}, 0 diagnostics emitted): ${reason}`); +} + +child.on('error', (err) => { + console.error(`typecheck: failed to start tsc: ${err.message}`); + process.exit(2); +}); + +child.on('close', (code, signal) => { + const crashed = signal !== null || code !== 0; + + if (!crashed) { + if (errorTsLines > 0) { + // tsc printed diagnostics and still claimed success. Whatever that is, it + // is not a verdict worth trusting. + reportCrash({ + code, + signal, + reason: `tsc exited 0 but printed ${errorTsLines} diagnostic line(s) — contradictory result`, + remedy: 'investigate the tsc invocation; do not treat this run as a pass.', + }); + process.exit(1); + } + // A clean `tsc --noEmit` prints NOTHING, so an empty log is exactly what a + // crash-with-stderr-dropped looks like too. Stating success explicitly means + // "the log has no errors in it" is no longer a thing anyone has to infer. + console.log( + `typecheck: OK — 0 type errors in ${((Date.now() - startedAt) / 1000).toFixed(0)}s ` + + `(heap cap ${heapMb} MB).` + ); + process.exit(0); + } + + if (errorTsLines > 0) { + // The ordinary, useful failure: real type errors. Pass the verdict through + // untouched so the existing developer experience is unchanged. + console.error(`\ntypecheck: ${errorTsLines} type error(s).`); + process.exit(code === 0 || code === null ? 1 : code); + } + + if (sawHeapOom) { + reportCrash({ + code, + signal, + reason: `V8 ran out of old-space heap at ${heapMb} MB`, + remedy: + 'raise the cap — TYPECHECK_HEAP_MB= to confirm, then raise ' + + 'DEFAULT_HEAP_MB in this script (and check the CI runner has the RAM for it).', + }); + } else if (signal === 'SIGKILL' || code === 137) { + reportCrash({ + code, + signal, + reason: + 'the process was killed from outside (out of system memory, or a job/container limit)', + remedy: + 'give the machine or runner more RAM, or LOWER the heap cap so V8 stays ' + + 'inside it — a cap above available RAM turns into an unhelpful kill.', + }); + } else { + reportCrash({ + code, + signal, + reason: 'tsc terminated abnormally without producing any diagnostics', + remedy: 'read the tsc output above; this is a tsc/tooling failure, not a code failure.', + }); + } + process.exit(code === 0 || code === null ? 1 : code); +}); diff --git a/vitest.config.mts b/vitest.config.mts index 3307e6836d..9ebc1bf312 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -68,7 +68,11 @@ export default defineConfig({ name: 'unit', globals: true, environment: 'node', - include: ['src/**/*.test.ts'], + // `scripts/` is included so the typecheck wrapper's outcome + // classifier (scripts/typecheck.mjs) is covered — it is the thing that + // decides whether a run counts as a pass, so it needs a suite of its + // own rather than a one-off manual check. + include: ['src/**/*.test.ts', 'scripts/**/*.test.ts'], exclude: ['node_modules', 'tests/**/*'], // Exclude Playwright tests setupFiles: ['src/__tests__/setup.ts'], // Several unit tests cold-`await import(...)` a large Next API-page / service