fix(typecheck): make a crashed typecheck report as crashed, not as clean (#3619)

* 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=<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`.
This commit is contained in:
Zachary Lowden
2026-08-04 13:48:02 -05:00
committed by GitHub
parent aaf209dc21
commit c318ef1fb8
6 changed files with 378 additions and 2 deletions
+5
View File
@@ -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
+12
View File
@@ -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"
+1 -1
View File
@@ -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",
+115
View File
@@ -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);
});
});
+240
View File
@@ -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=<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=<larger> 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);
});
+5 -1
View File
@@ -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