From c09658568e1a19d09593d4e9ce0300fa5d9ee06d Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Mon, 21 Sep 2026 14:46:18 +0200 Subject: [PATCH 01/25] feat: block new type errors from being added to the suppressions file lint:tsc:check keeps unsuppressed errors from landing, but its escape hatch, regenerating the file, can be used to paper over a new error instead of fixing it. lint:tsc:ratchet closes that hatch. Measured against the merge base with the target branch, the suppressions file may only shrink: removing a suppression or lowering its count is always fine, while adding one or raising a count fails. --- .github/workflows/lint-build-test.yml | 30 +++++ package.json | 1 + scripts/lib/lint-tsc-ratchet.test.ts | 173 ++++++++++++++++++++++++++ scripts/lib/lint-tsc-ratchet.ts | 120 ++++++++++++++++++ scripts/lib/tsc-suppressions.test.ts | 102 +++++++++++++++ scripts/lib/tsc-suppressions.ts | 68 ++++++++++ scripts/lint-tsc-ratchet.test.ts | 39 ++++++ scripts/lint-tsc-ratchet.ts | 10 ++ 8 files changed, 543 insertions(+) create mode 100644 scripts/lib/lint-tsc-ratchet.test.ts create mode 100644 scripts/lib/lint-tsc-ratchet.ts create mode 100644 scripts/lint-tsc-ratchet.test.ts create mode 100644 scripts/lint-tsc-ratchet.ts diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index 29af55a6f66..b8220ae733d 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -166,6 +166,36 @@ jobs: exit 1 fi + lint-tsc-ratchet: + name: Lint (lint:tsc:ratchet) + if: github.event_name == 'pull_request' || github.event_name == 'merge_group' + runs-on: ubuntu-latest + strategy: + matrix: + node-version: [24.x] + steps: + - name: Checkout and setup environment + uses: MetaMask/action-checkout-and-setup@v3 + with: + is-high-risk-environment: false + persist-credentials: false + node-version: ${{ matrix.node-version }} + # Full history so the merge base with the base branch can be found. + fetch-depth: 0 + - name: Run yarn lint:tsc:ratchet + shell: bash + run: | + yarn lint:tsc:ratchet --base "origin/${BASE_REF#refs/heads/}" + env: + BASE_REF: ${{ github.event.pull_request.base.ref || github.event.merge_group.base_ref }} + - name: Require clean working directory + shell: bash + run: | + if ! git diff --exit-code; then + echo "Working tree dirty at end of job" + exit 1 + fi + validate-changelog-diffs: name: Validate changelog diffs if: github.event_name == 'pull_request' || github.event_name == 'merge_group' diff --git a/package.json b/package.json index 1f36f5cc216..88f2510fbc2 100644 --- a/package.json +++ b/package.json @@ -44,6 +44,7 @@ "lint:tsc:clean": "yarn lint:tsc:only-clean && yarn lint:tsc", "lint:tsc:only-clean": "rimraf -g 'packages/*/.tsc-lint-cache' '.tsc-lint-cache'", "lint:tsc:prune": "node --experimental-strip-types scripts/lint-tsc.ts --prune-suppressions", + "lint:tsc:ratchet": "node --experimental-strip-types scripts/lint-tsc-ratchet.ts", "lint:tsc:suppress": "node --experimental-strip-types scripts/lint-tsc.ts --suppress-all", "lint:tsconfigs": "node --import ./scripts/resolver/register.ts --experimental-transform-types scripts/lint-tsconfigs/lint-tsconfigs.ts", "lint:tsconfigs:all": "yarn workspaces foreach --all --parallel --interlaced --verbose run lint:tsconfigs", diff --git a/scripts/lib/lint-tsc-ratchet.test.ts b/scripts/lib/lint-tsc-ratchet.test.ts new file mode 100644 index 00000000000..c8011591bd0 --- /dev/null +++ b/scripts/lib/lint-tsc-ratchet.test.ts @@ -0,0 +1,173 @@ +import { jest } from '@jest/globals'; + +// `jest.mock` does not apply to ES modules, so the module registry is stubbed +// with `jest.unstable_mockModule` and the modules under test are imported +// dynamically afterwards. +jest.unstable_mockModule('execa', () => ({ + __esModule: true, + default: jest.fn(), +})); + +jest.unstable_mockModule('./tsc-suppressions.js', () => ({ + findAddedSuppressions: jest.fn(), + printAddedSuppressions: jest.fn(), + readSuppressions: jest.fn(), +})); + +const { default: execa } = await import('execa'); +const tscSuppressions = await import('./tsc-suppressions.js'); +const { lintTscRatchet } = await import('./lint-tsc-ratchet.js'); + +const BASE = { 'a.ts': { TS2322: { count: 2 } } }; +const CURRENT = { 'a.ts': { TS2322: { count: 1 } } }; + +const ADDITION = { + filePath: 'a.ts', + code: 'TS2322', + count: 3, + baseCount: 2, +}; + +/** + * Stubs the Git commands that the script runs. + * + * @param args - The arguments to this function. + * @param args.mergeBase - The commit that `git merge-base` should report, or + * null if it should fail. + * @param args.fileContents - The suppressions file that `git show` should + * produce, or null if it should fail. + * @param args.refExists - Whether `git rev-parse` should resolve the ref. + */ +function mockGit({ + mergeBase = 'abc123', + fileContents = JSON.stringify(BASE), + refExists = true, +}: { + mergeBase?: string | null; + fileContents?: string | null; + refExists?: boolean; +} = {}): void { + jest.mocked(execa).mockImplementation((async ( + _file: string, + args: string[], + ) => { + const succeed = (stdout: string): { stdout: string; exitCode: number } => ({ + stdout, + exitCode: 0, + }); + const fail = { stdout: '', exitCode: 1 }; + + if (args[0] === 'merge-base') { + return mergeBase === null ? fail : succeed(`${mergeBase}\n`); + } + if (args[0] === 'rev-parse') { + return refExists ? succeed('abc123') : fail; + } + return fileContents === null ? fail : succeed(fileContents); + }) as never); +} + +describe('lintTscRatchet', () => { + let originalProcess: typeof globalThis.process; + + beforeEach(() => { + originalProcess = globalThis.process; + // The exit code is reset because it is global state that another test file + // may have set. + globalThis.process = { ...globalThis.process, exitCode: undefined }; + jest.spyOn(console, 'log').mockReturnValue(undefined); + jest.mocked(tscSuppressions.readSuppressions).mockResolvedValue(CURRENT); + jest.mocked(tscSuppressions.findAddedSuppressions).mockReturnValue([]); + }); + + afterEach(() => { + globalThis.process = originalProcess; + }); + + it('compares the current suppressions against the merge base', async () => { + mockGit({ mergeBase: 'abc123' }); + + await lintTscRatchet([]); + + expect(execa).toHaveBeenCalledWith( + 'git', + ['merge-base', 'HEAD', 'origin/main'], + expect.objectContaining({ reject: false }), + ); + expect(execa).toHaveBeenCalledWith( + 'git', + ['show', 'abc123:tsc-suppressions.json'], + expect.objectContaining({ reject: false }), + ); + expect(tscSuppressions.findAddedSuppressions).toHaveBeenCalledWith({ + current: CURRENT, + base: BASE, + }); + }); + + it('compares against the given base branch when one is passed', async () => { + mockGit(); + + await lintTscRatchet(['--base', 'origin/some-branch']); + + expect(execa).toHaveBeenCalledWith( + 'git', + ['merge-base', 'HEAD', 'origin/some-branch'], + expect.objectContaining({ reject: false }), + ); + }); + + it('falls back to the base ref when the merge base cannot be determined', async () => { + mockGit({ mergeBase: null }); + + await lintTscRatchet([]); + + expect(execa).toHaveBeenCalledWith( + 'git', + ['show', 'origin/main:tsc-suppressions.json'], + expect.objectContaining({ reject: false }), + ); + }); + + it('leaves the exit code alone when nothing has been added', async () => { + mockGit(); + jest.mocked(tscSuppressions.findAddedSuppressions).mockReturnValue([]); + + await lintTscRatchet([]); + + expect(tscSuppressions.printAddedSuppressions).toHaveBeenCalledWith([]); + expect(process.exitCode).toBeUndefined(); + }); + + it('exits with a non-zero code when suppressions have been added', async () => { + mockGit(); + jest + .mocked(tscSuppressions.findAddedSuppressions) + .mockReturnValue([ADDITION]); + + await lintTscRatchet([]); + + expect(tscSuppressions.printAddedSuppressions).toHaveBeenCalledWith([ + ADDITION, + ]); + expect(process.exitCode).toBe(1); + }); + + it('throws when the ref to compare against cannot be resolved', async () => { + mockGit({ mergeBase: null, refExists: false }); + + await expect(lintTscRatchet([])).rejects.toThrow( + 'Cannot resolve origin/main. Fetch the base branch and try again.', + ); + expect(tscSuppressions.findAddedSuppressions).not.toHaveBeenCalled(); + }); + + it('skips the check when the base has no suppressions file to compare against', async () => { + mockGit({ fileContents: null }); + + await lintTscRatchet([]); + + expect(tscSuppressions.findAddedSuppressions).not.toHaveBeenCalled(); + expect(process.exitCode).toBeUndefined(); + }); +}); diff --git a/scripts/lib/lint-tsc-ratchet.ts b/scripts/lib/lint-tsc-ratchet.ts new file mode 100644 index 00000000000..64ba6482efd --- /dev/null +++ b/scripts/lib/lint-tsc-ratchet.ts @@ -0,0 +1,120 @@ +import execa from 'execa'; +import path from 'path'; + +import { + findAddedSuppressions, + printAddedSuppressions, + readSuppressions, +} from './tsc-suppressions.js'; +import type { TscSuppressions } from './tsc-suppressions.js'; + +const REPO_ROOT = path.join(import.meta.dirname, '..', '..'); + +const SUPPRESSIONS_FILE_NAME = 'tsc-suppressions.json'; + +const DEFAULT_BASE_REF = 'origin/main'; + +/** + * Reads the value that follows an option in the given arguments. + * + * @param argv - The arguments passed to this script. + * @param option - The option to look for. + * @returns The value following the option, or undefined if it is absent. + */ +function getOptionValue( + argv: readonly string[], + option: string, +): string | undefined { + const index = argv.indexOf(option); + return index === -1 ? undefined : argv[index + 1]; +} + +/** + * Runs a Git command, returning nothing if it fails. + * + * @param args - The arguments to pass to Git. + * @returns The trimmed standard output, or undefined if Git exited non-zero. + */ +async function git(args: string[]): Promise { + const { stdout, exitCode } = await execa('git', args, { + cwd: REPO_ROOT, + reject: false, + }); + return exitCode === 0 ? stdout.trim() : undefined; +} + +/** + * Finds the commit to compare against. + * + * The merge base is preferred, so that suppressions removed on the base branch + * since this branch was cut are not mistaken for additions. Where it cannot be + * determined — a shallow clone, say — the base ref itself is used. + * + * @param baseRef - The branch this work is destined for. + * @returns The ref to read the baseline suppressions from. + */ +async function resolveComparisonRef(baseRef: string): Promise { + return (await git(['merge-base', 'HEAD', baseRef])) ?? baseRef; +} + +/** + * Reads the suppressions file as it stands at the given ref. + * + * @param ref - The ref to read the file from. + * @returns The suppressions it holds, or undefined if the file does not exist + * there. + */ +async function readSuppressionsAtRef( + ref: string, +): Promise { + const contents = await git(['show', `${ref}:${SUPPRESSIONS_FILE_NAME}`]); + return contents === undefined + ? undefined + : (JSON.parse(contents) as TscSuppressions); +} + +/** + * Checks that no type errors have been added to the suppressions file. + * + * `lint:tsc:check` keeps errors that are not suppressed from landing, but its + * escape hatch — regenerating the file — can be used to paper over new errors + * rather than fix them. This closes that hatch: measured against the base + * branch, the file may only shrink. + * + * Pass `--base ` to compare against a branch other than `origin/main`. + * + * @param argv - The arguments passed to this script. + */ +export async function lintTscRatchet(argv: readonly string[]): Promise { + const baseRef = getOptionValue(argv, '--base') ?? DEFAULT_BASE_REF; + const comparisonRef = await resolveComparisonRef(baseRef); + + // A ref that cannot be resolved would leave nothing to compare against, and + // this check must fail rather than wave the change through. + if ( + (await git(['rev-parse', '--verify', `${comparisonRef}^{commit}`])) === + undefined + ) { + throw new Error( + `Cannot resolve ${comparisonRef}. Fetch the base branch and try again.`, + ); + } + + const base = await readSuppressionsAtRef(comparisonRef); + if (base === undefined) { + console.log( + `ℹ️ ${SUPPRESSIONS_FILE_NAME} does not exist at ${comparisonRef}, so there is nothing to compare against.`, + ); + return; + } + + const current = await readSuppressions( + path.join(REPO_ROOT, SUPPRESSIONS_FILE_NAME), + ); + const added = findAddedSuppressions({ current, base }); + printAddedSuppressions(added); + + if (added.length > 0) { + process.exitCode = 1; + } +} diff --git a/scripts/lib/tsc-suppressions.test.ts b/scripts/lib/tsc-suppressions.test.ts index b3f0d06ba29..b7c1e66607f 100644 --- a/scripts/lib/tsc-suppressions.test.ts +++ b/scripts/lib/tsc-suppressions.test.ts @@ -12,8 +12,10 @@ import { buildSuppressions, compareErrorsToSuppressions, compareStrings, + findAddedSuppressions, isTscError, parseTscOutput, + printAddedSuppressions, printReport, pruneSuppressions, readSuppressions, @@ -375,6 +377,106 @@ describe('compareErrorsToSuppressions', () => { }); }); +describe('findAddedSuppressions', () => { + it('flags a file that the baseline does not suppress at all', () => { + const added = findAddedSuppressions({ + current: { 'a.ts': { TS2322: { count: 1 } } }, + base: {}, + }); + + expect(added).toStrictEqual([ + { filePath: 'a.ts', code: 'TS2322', count: 1, baseCount: 0 }, + ]); + }); + + it('flags a code that the baseline does not suppress within a file it does', () => { + const added = findAddedSuppressions({ + current: { 'a.ts': { TS2322: { count: 1 }, TS7005: { count: 1 } } }, + base: { 'a.ts': { TS2322: { count: 1 } } }, + }); + + expect(added).toStrictEqual([ + { filePath: 'a.ts', code: 'TS7005', count: 1, baseCount: 0 }, + ]); + }); + + it('flags a count that has grown', () => { + const added = findAddedSuppressions({ + current: { 'a.ts': { TS2322: { count: 3 } } }, + base: { 'a.ts': { TS2322: { count: 2 } } }, + }); + + expect(added).toStrictEqual([ + { filePath: 'a.ts', code: 'TS2322', count: 3, baseCount: 2 }, + ]); + }); + + it('allows a count that is unchanged', () => { + expect( + findAddedSuppressions({ + current: { 'a.ts': { TS2322: { count: 2 } } }, + base: { 'a.ts': { TS2322: { count: 2 } } }, + }), + ).toStrictEqual([]); + }); + + it('allows a count that has shrunk', () => { + expect( + findAddedSuppressions({ + current: { 'a.ts': { TS2322: { count: 1 } } }, + base: { 'a.ts': { TS2322: { count: 5 } } }, + }), + ).toStrictEqual([]); + }); + + it('allows a code or a file to disappear entirely', () => { + expect( + findAddedSuppressions({ + current: {}, + base: { 'a.ts': { TS2322: { count: 5 }, TS7005: { count: 1 } } }, + }), + ).toStrictEqual([]); + }); + + it('reports every addition, not just the first', () => { + const added = findAddedSuppressions({ + current: { + 'a.ts': { TS2322: { count: 1 } }, + 'b.ts': { TS7005: { count: 2 } }, + }, + base: {}, + }); + + expect(added).toHaveLength(2); + }); +}); + +describe('printAddedSuppressions', () => { + beforeEach(() => { + jest.spyOn(console, 'log').mockReturnValue(undefined); + }); + + it('announces success when nothing was added', () => { + printAddedSuppressions([]); + + expect(console.log).toHaveBeenCalledWith( + '✅ No type errors have been added to the suppressions file. Good job!', + ); + }); + + it('prints each addition and how to resolve it', () => { + printAddedSuppressions([ + { filePath: 'a.ts', code: 'TS2322', count: 3, baseCount: 2 }, + ]); + + const output = jest.mocked(console.log).mock.calls.flat().join('\n'); + expect(output).toContain('a.ts'); + expect(output).toContain('TS2322'); + expect(output).toContain('3'); + expect(output).toContain('2'); + }); +}); + describe('readSuppressions', () => { it('reads the suppressions that the file holds', async () => { expect.assertions(1); diff --git a/scripts/lib/tsc-suppressions.ts b/scripts/lib/tsc-suppressions.ts index fd6d5956c18..1453e18ab98 100644 --- a/scripts/lib/tsc-suppressions.ts +++ b/scripts/lib/tsc-suppressions.ts @@ -58,6 +58,17 @@ export type StaleSuppression = { suppressedCount: number; }; +/** + * A suppression that covers more errors than the baseline it is compared + * against, meaning that type errors have been added rather than fixed. + */ +export type AddedSuppression = { + filePath: string; + code: string; + count: number; + baseCount: number; +}; + /** * The result of checking the type errors in the repo against the suppressions * file. @@ -353,6 +364,63 @@ export function compareErrorsToSuppressions({ }; } +/** + * Finds the suppressions that cover more errors than a baseline does. + * + * Suppressions are meant to be worked off, never added to: an error that is new + * should be fixed rather than recorded. Removing suppressions, or shrinking + * their counts, is always allowed. + * + * @param args - The arguments to this function. + * @param args.current - The suppressions as they now stand. + * @param args.base - The suppressions to measure them against. + * @returns Every suppression that grew or appeared, in file order. + */ +export function findAddedSuppressions({ + current, + base, +}: { + current: TscSuppressions; + base: TscSuppressions; +}): AddedSuppression[] { + const added: AddedSuppression[] = []; + + for (const [filePath, currentByCode] of Object.entries(current)) { + for (const [code, { count }] of Object.entries(currentByCode)) { + const baseCount = base[filePath]?.[code]?.count ?? 0; + if (count > baseCount) { + added.push({ filePath, code, count, baseCount }); + } + } + } + + return added; +} + +/** + * Prints the suppressions that have been added, if any. + * + * @param added - The added suppressions to print. + */ +export function printAddedSuppressions(added: AddedSuppression[]): void { + if (added.length === 0) { + console.log( + '✅ No type errors have been added to the suppressions file. Good job!', + ); + return; + } + + console.log('❌ Detected type errors added to the suppressions file:\n'); + for (const suppression of added) { + console.log( + ` ${suppression.filePath}: ${suppression.code} (${suppression.count} suppressed, was ${suppression.baseCount})`, + ); + } + console.log( + '\nSuppressions may only be removed, never added. Fix these type errors rather than suppressing them.', + ); +} + /** * Reads the suppressions file, treating a missing file as having no * suppressions. diff --git a/scripts/lint-tsc-ratchet.test.ts b/scripts/lint-tsc-ratchet.test.ts new file mode 100644 index 00000000000..520cd54cb2f --- /dev/null +++ b/scripts/lint-tsc-ratchet.test.ts @@ -0,0 +1,39 @@ +import { jest } from '@jest/globals'; + +// `jest.mock` does not apply to ES modules, so the module registry is stubbed +// with `jest.unstable_mockModule` and the modules under test are imported +// dynamically afterwards. +jest.unstable_mockModule('./lib/lint-tsc-ratchet.js', () => ({ + lintTscRatchet: jest.fn(), +})); + +const { lintTscRatchet } = await import('./lib/lint-tsc-ratchet.js'); + +describe('lint-tsc-ratchet', () => { + let originalProcess: typeof globalThis.process; + + beforeEach(() => { + originalProcess = globalThis.process; + // The exit code is reset because it is global state that another test file + // may have set. + globalThis.process = { ...globalThis.process, exitCode: undefined }; + }); + + afterEach(() => { + globalThis.process = originalProcess; + }); + + it('runs the check, reporting any error it throws', async () => { + jest.mocked(lintTscRatchet).mockRejectedValue('foo'); + jest.spyOn(console, 'error').mockReturnValue(undefined); + + // Importing the entry point runs it, which is the behaviour under test. + await import('./lint-tsc-ratchet.js'); + await new Promise((resolve) => setImmediate(resolve)); + + expect(lintTscRatchet).toHaveBeenCalledTimes(1); + expect(lintTscRatchet).toHaveBeenCalledWith(process.argv.slice(2)); + expect(console.error).toHaveBeenCalledWith('foo'); + expect(process.exitCode).toBe(1); + }); +}); diff --git a/scripts/lint-tsc-ratchet.ts b/scripts/lint-tsc-ratchet.ts new file mode 100644 index 00000000000..942d617f10f --- /dev/null +++ b/scripts/lint-tsc-ratchet.ts @@ -0,0 +1,10 @@ +/** + * Entry point file for the `lint:tsc:ratchet` script. + */ + +import { lintTscRatchet } from './lib/lint-tsc-ratchet.js'; + +lintTscRatchet(process.argv.slice(2)).catch((error) => { + console.error(error); + process.exitCode = 1; +}); From 95bbaa914e2384346dd456927f41be21b4eee69a Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Sat, 26 Sep 2026 11:33:49 +0200 Subject: [PATCH 02/25] chore: use execa's named export in the ratchet execa v10, which main now depends on, has no default export. --- scripts/lib/lint-tsc-ratchet.test.ts | 5 ++--- scripts/lib/lint-tsc-ratchet.ts | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/scripts/lib/lint-tsc-ratchet.test.ts b/scripts/lib/lint-tsc-ratchet.test.ts index c8011591bd0..f0845c80b9d 100644 --- a/scripts/lib/lint-tsc-ratchet.test.ts +++ b/scripts/lib/lint-tsc-ratchet.test.ts @@ -4,8 +4,7 @@ import { jest } from '@jest/globals'; // with `jest.unstable_mockModule` and the modules under test are imported // dynamically afterwards. jest.unstable_mockModule('execa', () => ({ - __esModule: true, - default: jest.fn(), + execa: jest.fn(), })); jest.unstable_mockModule('./tsc-suppressions.js', () => ({ @@ -14,7 +13,7 @@ jest.unstable_mockModule('./tsc-suppressions.js', () => ({ readSuppressions: jest.fn(), })); -const { default: execa } = await import('execa'); +const { execa } = await import('execa'); const tscSuppressions = await import('./tsc-suppressions.js'); const { lintTscRatchet } = await import('./lint-tsc-ratchet.js'); diff --git a/scripts/lib/lint-tsc-ratchet.ts b/scripts/lib/lint-tsc-ratchet.ts index 64ba6482efd..44f08ecde4e 100644 --- a/scripts/lib/lint-tsc-ratchet.ts +++ b/scripts/lib/lint-tsc-ratchet.ts @@ -1,4 +1,4 @@ -import execa from 'execa'; +import { execa } from 'execa'; import path from 'path'; import { From 5cd129ccf7fe9dfb0717eeb49ce16aaef48e1497 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Tue, 29 Sep 2026 11:41:30 +0200 Subject: [PATCH 03/25] chore: use .ts import extensions in the ratchet Follows the convention main adopted in #10537. --- oxlint-suppressions.json | 15 +++++++++++++++ scripts/lib/lint-tsc-ratchet.test.ts | 6 +++--- scripts/lib/lint-tsc-ratchet.ts | 4 ++-- scripts/lint-tsc-ratchet.test.ts | 6 +++--- scripts/lint-tsc-ratchet.ts | 2 +- 5 files changed, 24 insertions(+), 9 deletions(-) diff --git a/oxlint-suppressions.json b/oxlint-suppressions.json index 0fdc97ddaf1..ce22a6f5d65 100644 --- a/oxlint-suppressions.json +++ b/oxlint-suppressions.json @@ -7665,6 +7665,16 @@ "count": 2 } }, + "scripts/lib/lint-tsc-ratchet.test.ts": { + "no-shadow": { + "count": 1 + } + }, + "scripts/lib/lint-tsc-ratchet.ts": { + "n/no-unsupported-features/node-builtins": { + "count": 1 + } + }, "scripts/lib/lint-tsc.test.ts": { "no-shadow": { "count": 1 @@ -7700,6 +7710,11 @@ "count": 2 } }, + "scripts/lint-tsc-ratchet.test.ts": { + "no-shadow": { + "count": 1 + } + }, "scripts/lint-tsc.test.ts": { "no-shadow": { "count": 1 diff --git a/scripts/lib/lint-tsc-ratchet.test.ts b/scripts/lib/lint-tsc-ratchet.test.ts index f0845c80b9d..f890ae2e4ca 100644 --- a/scripts/lib/lint-tsc-ratchet.test.ts +++ b/scripts/lib/lint-tsc-ratchet.test.ts @@ -7,15 +7,15 @@ jest.unstable_mockModule('execa', () => ({ execa: jest.fn(), })); -jest.unstable_mockModule('./tsc-suppressions.js', () => ({ +jest.unstable_mockModule('./tsc-suppressions.ts', () => ({ findAddedSuppressions: jest.fn(), printAddedSuppressions: jest.fn(), readSuppressions: jest.fn(), })); const { execa } = await import('execa'); -const tscSuppressions = await import('./tsc-suppressions.js'); -const { lintTscRatchet } = await import('./lint-tsc-ratchet.js'); +const tscSuppressions = await import('./tsc-suppressions.ts'); +const { lintTscRatchet } = await import('./lint-tsc-ratchet.ts'); const BASE = { 'a.ts': { TS2322: { count: 2 } } }; const CURRENT = { 'a.ts': { TS2322: { count: 1 } } }; diff --git a/scripts/lib/lint-tsc-ratchet.ts b/scripts/lib/lint-tsc-ratchet.ts index 44f08ecde4e..bb873287c08 100644 --- a/scripts/lib/lint-tsc-ratchet.ts +++ b/scripts/lib/lint-tsc-ratchet.ts @@ -5,8 +5,8 @@ import { findAddedSuppressions, printAddedSuppressions, readSuppressions, -} from './tsc-suppressions.js'; -import type { TscSuppressions } from './tsc-suppressions.js'; +} from './tsc-suppressions.ts'; +import type { TscSuppressions } from './tsc-suppressions.ts'; const REPO_ROOT = path.join(import.meta.dirname, '..', '..'); diff --git a/scripts/lint-tsc-ratchet.test.ts b/scripts/lint-tsc-ratchet.test.ts index 520cd54cb2f..01f29724161 100644 --- a/scripts/lint-tsc-ratchet.test.ts +++ b/scripts/lint-tsc-ratchet.test.ts @@ -3,11 +3,11 @@ import { jest } from '@jest/globals'; // `jest.mock` does not apply to ES modules, so the module registry is stubbed // with `jest.unstable_mockModule` and the modules under test are imported // dynamically afterwards. -jest.unstable_mockModule('./lib/lint-tsc-ratchet.js', () => ({ +jest.unstable_mockModule('./lib/lint-tsc-ratchet.ts', () => ({ lintTscRatchet: jest.fn(), })); -const { lintTscRatchet } = await import('./lib/lint-tsc-ratchet.js'); +const { lintTscRatchet } = await import('./lib/lint-tsc-ratchet.ts'); describe('lint-tsc-ratchet', () => { let originalProcess: typeof globalThis.process; @@ -28,7 +28,7 @@ describe('lint-tsc-ratchet', () => { jest.spyOn(console, 'error').mockReturnValue(undefined); // Importing the entry point runs it, which is the behaviour under test. - await import('./lint-tsc-ratchet.js'); + await import('./lint-tsc-ratchet.ts'); await new Promise((resolve) => setImmediate(resolve)); expect(lintTscRatchet).toHaveBeenCalledTimes(1); diff --git a/scripts/lint-tsc-ratchet.ts b/scripts/lint-tsc-ratchet.ts index 942d617f10f..6d4ab74f7db 100644 --- a/scripts/lint-tsc-ratchet.ts +++ b/scripts/lint-tsc-ratchet.ts @@ -2,7 +2,7 @@ * Entry point file for the `lint:tsc:ratchet` script. */ -import { lintTscRatchet } from './lib/lint-tsc-ratchet.js'; +import { lintTscRatchet } from './lib/lint-tsc-ratchet.ts'; lintTscRatchet(process.argv.slice(2)).catch((error) => { console.error(error); From fde59603f11b2061619874c8ec2d4fd8620d7a32 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Tue, 29 Sep 2026 14:51:46 +0200 Subject: [PATCH 04/25] chore: apply review feedback to the ratchet Uses top-level await in the entry point and resolves the repo root without import.meta.dirname, matching the base branch. --- oxlint-suppressions.json | 5 ----- scripts/lib/lint-tsc-ratchet.ts | 7 ++++++- scripts/lint-tsc-ratchet.test.ts | 21 ++------------------- scripts/lint-tsc-ratchet.ts | 5 +---- 4 files changed, 9 insertions(+), 29 deletions(-) diff --git a/oxlint-suppressions.json b/oxlint-suppressions.json index ce22a6f5d65..65a31b2ae4c 100644 --- a/oxlint-suppressions.json +++ b/oxlint-suppressions.json @@ -7670,11 +7670,6 @@ "count": 1 } }, - "scripts/lib/lint-tsc-ratchet.ts": { - "n/no-unsupported-features/node-builtins": { - "count": 1 - } - }, "scripts/lib/lint-tsc.test.ts": { "no-shadow": { "count": 1 diff --git a/scripts/lib/lint-tsc-ratchet.ts b/scripts/lib/lint-tsc-ratchet.ts index bb873287c08..665859bca5d 100644 --- a/scripts/lib/lint-tsc-ratchet.ts +++ b/scripts/lib/lint-tsc-ratchet.ts @@ -1,5 +1,6 @@ import { execa } from 'execa'; import path from 'path'; +import { fileURLToPath } from 'url'; import { findAddedSuppressions, @@ -8,7 +9,11 @@ import { } from './tsc-suppressions.ts'; import type { TscSuppressions } from './tsc-suppressions.ts'; -const REPO_ROOT = path.join(import.meta.dirname, '..', '..'); +const REPO_ROOT = path.join( + path.dirname(fileURLToPath(import.meta.url)), + '..', + '..', +); const SUPPRESSIONS_FILE_NAME = 'tsc-suppressions.json'; diff --git a/scripts/lint-tsc-ratchet.test.ts b/scripts/lint-tsc-ratchet.test.ts index 01f29724161..4c806f28cdb 100644 --- a/scripts/lint-tsc-ratchet.test.ts +++ b/scripts/lint-tsc-ratchet.test.ts @@ -10,30 +10,13 @@ jest.unstable_mockModule('./lib/lint-tsc-ratchet.ts', () => ({ const { lintTscRatchet } = await import('./lib/lint-tsc-ratchet.ts'); describe('lint-tsc-ratchet', () => { - let originalProcess: typeof globalThis.process; - - beforeEach(() => { - originalProcess = globalThis.process; - // The exit code is reset because it is global state that another test file - // may have set. - globalThis.process = { ...globalThis.process, exitCode: undefined }; - }); - - afterEach(() => { - globalThis.process = originalProcess; - }); - - it('runs the check, reporting any error it throws', async () => { - jest.mocked(lintTscRatchet).mockRejectedValue('foo'); - jest.spyOn(console, 'error').mockReturnValue(undefined); + it('runs the check with the arguments it was given', async () => { + jest.mocked(lintTscRatchet).mockResolvedValue(undefined); // Importing the entry point runs it, which is the behaviour under test. await import('./lint-tsc-ratchet.ts'); - await new Promise((resolve) => setImmediate(resolve)); expect(lintTscRatchet).toHaveBeenCalledTimes(1); expect(lintTscRatchet).toHaveBeenCalledWith(process.argv.slice(2)); - expect(console.error).toHaveBeenCalledWith('foo'); - expect(process.exitCode).toBe(1); }); }); diff --git a/scripts/lint-tsc-ratchet.ts b/scripts/lint-tsc-ratchet.ts index 6d4ab74f7db..3c8ad9ae5f6 100644 --- a/scripts/lint-tsc-ratchet.ts +++ b/scripts/lint-tsc-ratchet.ts @@ -4,7 +4,4 @@ import { lintTscRatchet } from './lib/lint-tsc-ratchet.ts'; -lintTscRatchet(process.argv.slice(2)).catch((error) => { - console.error(error); - process.exitCode = 1; -}); +await lintTscRatchet(process.argv.slice(2)); From c9c09fc63191f8d88b557dfaf805423ec93be4da Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 30 Sep 2026 19:24:26 +0200 Subject: [PATCH 05/25] chore: let the merge commit supply the baseline CI checks a pull request out as the merge of the branch into its base, so the base is the first parent and the working tree file already has the base's own changes folded in. That removes the merge base lookup, the base ref plumbing and the full history checkout, leaving the check to compare two files. --- .github/workflows/lint-build-test.yml | 11 +-- scripts/lib/lint-tsc-ratchet.test.ts | 102 ++++---------------------- scripts/lib/lint-tsc-ratchet.ts | 98 +++++-------------------- 3 files changed, 37 insertions(+), 174 deletions(-) diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index b8220ae733d..b26b187dc46 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -180,14 +180,11 @@ jobs: is-high-risk-environment: false persist-credentials: false node-version: ${{ matrix.node-version }} - # Full history so the merge base with the base branch can be found. - fetch-depth: 0 + # The pull request is checked out as the merge of the branch into its + # base, and the check reads the base from the first parent. + fetch-depth: 2 - name: Run yarn lint:tsc:ratchet - shell: bash - run: | - yarn lint:tsc:ratchet --base "origin/${BASE_REF#refs/heads/}" - env: - BASE_REF: ${{ github.event.pull_request.base.ref || github.event.merge_group.base_ref }} + run: yarn lint:tsc:ratchet - name: Require clean working directory shell: bash run: | diff --git a/scripts/lib/lint-tsc-ratchet.test.ts b/scripts/lib/lint-tsc-ratchet.test.ts index f890ae2e4ca..487fc6d0f8b 100644 --- a/scripts/lib/lint-tsc-ratchet.test.ts +++ b/scripts/lib/lint-tsc-ratchet.test.ts @@ -27,45 +27,6 @@ const ADDITION = { baseCount: 2, }; -/** - * Stubs the Git commands that the script runs. - * - * @param args - The arguments to this function. - * @param args.mergeBase - The commit that `git merge-base` should report, or - * null if it should fail. - * @param args.fileContents - The suppressions file that `git show` should - * produce, or null if it should fail. - * @param args.refExists - Whether `git rev-parse` should resolve the ref. - */ -function mockGit({ - mergeBase = 'abc123', - fileContents = JSON.stringify(BASE), - refExists = true, -}: { - mergeBase?: string | null; - fileContents?: string | null; - refExists?: boolean; -} = {}): void { - jest.mocked(execa).mockImplementation((async ( - _file: string, - args: string[], - ) => { - const succeed = (stdout: string): { stdout: string; exitCode: number } => ({ - stdout, - exitCode: 0, - }); - const fail = { stdout: '', exitCode: 1 }; - - if (args[0] === 'merge-base') { - return mergeBase === null ? fail : succeed(`${mergeBase}\n`); - } - if (args[0] === 'rev-parse') { - return refExists ? succeed('abc123') : fail; - } - return fileContents === null ? fail : succeed(fileContents); - }) as never); -} - describe('lintTscRatchet', () => { let originalProcess: typeof globalThis.process; @@ -75,6 +36,9 @@ describe('lintTscRatchet', () => { // may have set. globalThis.process = { ...globalThis.process, exitCode: undefined }; jest.spyOn(console, 'log').mockReturnValue(undefined); + jest.mocked(execa).mockResolvedValue({ + stdout: JSON.stringify(BASE), + } as never); jest.mocked(tscSuppressions.readSuppressions).mockResolvedValue(CURRENT); jest.mocked(tscSuppressions.findAddedSuppressions).mockReturnValue([]); }); @@ -83,55 +47,29 @@ describe('lintTscRatchet', () => { globalThis.process = originalProcess; }); - it('compares the current suppressions against the merge base', async () => { - mockGit({ mergeBase: 'abc123' }); - + it('compares against the first parent of the merge commit CI checks out', async () => { await lintTscRatchet([]); - expect(execa).toHaveBeenCalledWith( + expect(jest.mocked(execa).mock.calls[0]?.slice(0, 2)).toStrictEqual([ 'git', - ['merge-base', 'HEAD', 'origin/main'], - expect.objectContaining({ reject: false }), - ); - expect(execa).toHaveBeenCalledWith( - 'git', - ['show', 'abc123:tsc-suppressions.json'], - expect.objectContaining({ reject: false }), - ); + ['show', 'HEAD^1:tsc-suppressions.json'], + ]); expect(tscSuppressions.findAddedSuppressions).toHaveBeenCalledWith({ current: CURRENT, base: BASE, }); }); - it('compares against the given base branch when one is passed', async () => { - mockGit(); - - await lintTscRatchet(['--base', 'origin/some-branch']); - - expect(execa).toHaveBeenCalledWith( - 'git', - ['merge-base', 'HEAD', 'origin/some-branch'], - expect.objectContaining({ reject: false }), - ); - }); - - it('falls back to the base ref when the merge base cannot be determined', async () => { - mockGit({ mergeBase: null }); - - await lintTscRatchet([]); + it('compares against the ref it is given', async () => { + await lintTscRatchet(['origin/main']); - expect(execa).toHaveBeenCalledWith( + expect(jest.mocked(execa).mock.calls[0]?.slice(0, 2)).toStrictEqual([ 'git', ['show', 'origin/main:tsc-suppressions.json'], - expect.objectContaining({ reject: false }), - ); + ]); }); it('leaves the exit code alone when nothing has been added', async () => { - mockGit(); - jest.mocked(tscSuppressions.findAddedSuppressions).mockReturnValue([]); - await lintTscRatchet([]); expect(tscSuppressions.printAddedSuppressions).toHaveBeenCalledWith([]); @@ -139,7 +77,6 @@ describe('lintTscRatchet', () => { }); it('exits with a non-zero code when suppressions have been added', async () => { - mockGit(); jest .mocked(tscSuppressions.findAddedSuppressions) .mockReturnValue([ADDITION]); @@ -152,21 +89,10 @@ describe('lintTscRatchet', () => { expect(process.exitCode).toBe(1); }); - it('throws when the ref to compare against cannot be resolved', async () => { - mockGit({ mergeBase: null, refExists: false }); + it('throws when the baseline cannot be read, rather than passing', async () => { + jest.mocked(execa).mockRejectedValue(new Error('unknown revision')); - await expect(lintTscRatchet([])).rejects.toThrow( - 'Cannot resolve origin/main. Fetch the base branch and try again.', - ); + await expect(lintTscRatchet([])).rejects.toThrow('unknown revision'); expect(tscSuppressions.findAddedSuppressions).not.toHaveBeenCalled(); }); - - it('skips the check when the base has no suppressions file to compare against', async () => { - mockGit({ fileContents: null }); - - await lintTscRatchet([]); - - expect(tscSuppressions.findAddedSuppressions).not.toHaveBeenCalled(); - expect(process.exitCode).toBeUndefined(); - }); }); diff --git a/scripts/lib/lint-tsc-ratchet.ts b/scripts/lib/lint-tsc-ratchet.ts index 665859bca5d..9bdc5e4b842 100644 --- a/scripts/lib/lint-tsc-ratchet.ts +++ b/scripts/lib/lint-tsc-ratchet.ts @@ -17,66 +17,11 @@ const REPO_ROOT = path.join( const SUPPRESSIONS_FILE_NAME = 'tsc-suppressions.json'; -const DEFAULT_BASE_REF = 'origin/main'; - /** - * Reads the value that follows an option in the given arguments. - * - * @param argv - The arguments passed to this script. - * @param option - The option to look for. - * @returns The value following the option, or undefined if it is absent. + * The first parent of the merge commit that CI checks a pull request out as, + * which is the branch the work is destined for. */ -function getOptionValue( - argv: readonly string[], - option: string, -): string | undefined { - const index = argv.indexOf(option); - return index === -1 ? undefined : argv[index + 1]; -} - -/** - * Runs a Git command, returning nothing if it fails. - * - * @param args - The arguments to pass to Git. - * @returns The trimmed standard output, or undefined if Git exited non-zero. - */ -async function git(args: string[]): Promise { - const { stdout, exitCode } = await execa('git', args, { - cwd: REPO_ROOT, - reject: false, - }); - return exitCode === 0 ? stdout.trim() : undefined; -} - -/** - * Finds the commit to compare against. - * - * The merge base is preferred, so that suppressions removed on the base branch - * since this branch was cut are not mistaken for additions. Where it cannot be - * determined — a shallow clone, say — the base ref itself is used. - * - * @param baseRef - The branch this work is destined for. - * @returns The ref to read the baseline suppressions from. - */ -async function resolveComparisonRef(baseRef: string): Promise { - return (await git(['merge-base', 'HEAD', baseRef])) ?? baseRef; -} - -/** - * Reads the suppressions file as it stands at the given ref. - * - * @param ref - The ref to read the file from. - * @returns The suppressions it holds, or undefined if the file does not exist - * there. - */ -async function readSuppressionsAtRef( - ref: string, -): Promise { - const contents = await git(['show', `${ref}:${SUPPRESSIONS_FILE_NAME}`]); - return contents === undefined - ? undefined - : (JSON.parse(contents) as TscSuppressions); -} +const DEFAULT_BASE_REF = 'HEAD^1'; /** * Checks that no type errors have been added to the suppressions file. @@ -86,32 +31,27 @@ async function readSuppressionsAtRef( * rather than fix them. This closes that hatch: measured against the base * branch, the file may only shrink. * - * Pass `--base ` to compare against a branch other than `origin/main`. + * CI checks a pull request out as the merge of the branch into its base, so the + * base is simply the first parent, and the file in the working tree already has + * the base's own changes folded in. That leaves this to compare two files, with + * no merge base to work out and nothing to fetch. + * + * Pass a ref to compare against something else, which is useful when running + * this outside of CI, where there is no merge commit. * * @param argv - The arguments passed to this script. */ export async function lintTscRatchet(argv: readonly string[]): Promise { - const baseRef = getOptionValue(argv, '--base') ?? DEFAULT_BASE_REF; - const comparisonRef = await resolveComparisonRef(baseRef); - - // A ref that cannot be resolved would leave nothing to compare against, and - // this check must fail rather than wave the change through. - if ( - (await git(['rev-parse', '--verify', `${comparisonRef}^{commit}`])) === - undefined - ) { - throw new Error( - `Cannot resolve ${comparisonRef}. Fetch the base branch and try again.`, - ); - } + const baseRef = argv[0] ?? DEFAULT_BASE_REF; - const base = await readSuppressionsAtRef(comparisonRef); - if (base === undefined) { - console.log( - `ℹ️ ${SUPPRESSIONS_FILE_NAME} does not exist at ${comparisonRef}, so there is nothing to compare against.`, - ); - return; - } + // Left to throw if the ref or the file is missing, as a check that cannot + // find its baseline must not wave the change through. + const { stdout } = await execa( + 'git', + ['show', `${baseRef}:${SUPPRESSIONS_FILE_NAME}`], + { cwd: REPO_ROOT }, + ); + const base = JSON.parse(stdout) as TscSuppressions; const current = await readSuppressions( path.join(REPO_ROOT, SUPPRESSIONS_FILE_NAME), From d2b5e8df887ef4b6e1c8a7a3b6b5ce8b9cbd2861 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 30 Sep 2026 20:39:07 +0200 Subject: [PATCH 06/25] feat: guard the oxlint suppressions too, and deduplicate The two suppressions files share a shape, so the check now compares both and the comparison moved out of the tsc specific module. Scripts tests have to import jest from @jest/globals, as Jest does not inject it in ESM, which the no-shadow rule flagged in every one of them. Allowing that one name in the rule removes 41 lines of suppressions and stops the check from blocking every new scripts test. Also stops tsc-suppressions.json being named in two places. --- .github/workflows/lint-build-test.yml | 8 +- oxlint-suppressions.json | 51 ----- oxlint.config.ts | 8 + package.json | 2 +- scripts/lib/lint-suppressions-ratchet.test.ts | 184 ++++++++++++++++++ scripts/lib/lint-suppressions-ratchet.ts | 149 ++++++++++++++ scripts/lib/lint-tsc-ratchet.test.ts | 98 ---------- scripts/lib/lint-tsc-ratchet.ts | 65 ------- scripts/lib/lint-tsc.test.ts | 1 + scripts/lib/lint-tsc.ts | 3 +- scripts/lib/tsc-suppressions.test.ts | 102 ---------- scripts/lib/tsc-suppressions.ts | 73 +------ scripts/lint-suppressions-ratchet.test.ts | 23 +++ scripts/lint-suppressions-ratchet.ts | 7 + scripts/lint-tsc-ratchet.test.ts | 22 --- scripts/lint-tsc-ratchet.ts | 7 - 16 files changed, 383 insertions(+), 420 deletions(-) create mode 100644 scripts/lib/lint-suppressions-ratchet.test.ts create mode 100644 scripts/lib/lint-suppressions-ratchet.ts delete mode 100644 scripts/lib/lint-tsc-ratchet.test.ts delete mode 100644 scripts/lib/lint-tsc-ratchet.ts create mode 100644 scripts/lint-suppressions-ratchet.test.ts create mode 100644 scripts/lint-suppressions-ratchet.ts delete mode 100644 scripts/lint-tsc-ratchet.test.ts delete mode 100644 scripts/lint-tsc-ratchet.ts diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index b26b187dc46..68b3d23f1f0 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -166,8 +166,8 @@ jobs: exit 1 fi - lint-tsc-ratchet: - name: Lint (lint:tsc:ratchet) + lint-suppressions-ratchet: + name: Lint (lint:suppressions:ratchet) if: github.event_name == 'pull_request' || github.event_name == 'merge_group' runs-on: ubuntu-latest strategy: @@ -183,8 +183,8 @@ jobs: # The pull request is checked out as the merge of the branch into its # base, and the check reads the base from the first parent. fetch-depth: 2 - - name: Run yarn lint:tsc:ratchet - run: yarn lint:tsc:ratchet + - name: Run yarn lint:suppressions:ratchet + run: yarn lint:suppressions:ratchet - name: Require clean working directory shell: bash run: | diff --git a/oxlint-suppressions.json b/oxlint-suppressions.json index 65a31b2ae4c..ffa7a7c1aa8 100644 --- a/oxlint-suppressions.json +++ b/oxlint-suppressions.json @@ -7622,27 +7622,11 @@ } }, "scripts/create-package/cli.test.ts": { - "no-shadow": { - "count": 1 - }, "typescript/no-unsafe-argument": { "count": 2 } }, - "scripts/create-package/commands.test.ts": { - "no-shadow": { - "count": 1 - } - }, - "scripts/create-package/index.test.ts": { - "no-shadow": { - "count": 1 - } - }, "scripts/create-package/utils.test.ts": { - "no-shadow": { - "count": 1 - }, "typescript/no-unsafe-assignment": { "count": 2 } @@ -7652,11 +7636,6 @@ "count": 1 } }, - "scripts/lib/changelog-conflicts.test.ts": { - "no-shadow": { - "count": 1 - } - }, "scripts/lib/changelog-conflicts.ts": { "n/no-unsupported-features/node-builtins": { "count": 1 @@ -7665,21 +7644,6 @@ "count": 2 } }, - "scripts/lib/lint-tsc-ratchet.test.ts": { - "no-shadow": { - "count": 1 - } - }, - "scripts/lib/lint-tsc.test.ts": { - "no-shadow": { - "count": 1 - } - }, - "scripts/lib/tsc-suppressions.test.ts": { - "no-shadow": { - "count": 1 - } - }, "scripts/lib/workspaces.ts": { "jsdoc/require-param": { "count": 1 @@ -7705,16 +7669,6 @@ "count": 2 } }, - "scripts/lint-tsc-ratchet.test.ts": { - "no-shadow": { - "count": 1 - } - }, - "scripts/lint-tsc.test.ts": { - "no-shadow": { - "count": 1 - } - }, "scripts/lint-tsconfigs/utils.ts": { "n/no-unsupported-features/node-builtins": { "count": 1 @@ -7728,11 +7682,6 @@ "count": 1 } }, - "scripts/merge-changelog-conflicts.test.ts": { - "no-shadow": { - "count": 1 - } - }, "scripts/update-readme-content.ts": { "n/no-unsupported-features/node-builtins": { "count": 1 diff --git a/oxlint.config.ts b/oxlint.config.ts index 2163303abe1..755a3070e33 100644 --- a/oxlint.config.ts +++ b/oxlint.config.ts @@ -103,6 +103,14 @@ export default createConfig({ }, }, + { + // Jest does not inject its globals in ESM, so the scripts' tests import + // `jest` from `@jest/globals`. Declaring it as a global as well would + // make every one of those imports shadow it. + files: ['scripts/**/*.test.ts'], + rules: { 'no-shadow': ['error', { allow: ['jest'] }] }, + }, + { files: ['scripts/**/*.ts'], rules: { diff --git a/package.json b/package.json index 88f2510fbc2..c7396af30b1 100644 --- a/package.json +++ b/package.json @@ -38,13 +38,13 @@ "lint:misc": "oxfmt --ignore-path .gitignore", "lint:misc:check": "yarn lint:misc --check", "lint:oxlint": "oxlint", + "lint:suppressions:ratchet": "node --experimental-strip-types scripts/lint-suppressions-ratchet.ts", "lint:teams": "node --import ./scripts/resolver/register.ts --experimental-transform-types scripts/lint-teams-json.ts", "lint:tsc": "tsc --build tsconfig.lint.json", "lint:tsc:check": "node --experimental-strip-types scripts/lint-tsc.ts", "lint:tsc:clean": "yarn lint:tsc:only-clean && yarn lint:tsc", "lint:tsc:only-clean": "rimraf -g 'packages/*/.tsc-lint-cache' '.tsc-lint-cache'", "lint:tsc:prune": "node --experimental-strip-types scripts/lint-tsc.ts --prune-suppressions", - "lint:tsc:ratchet": "node --experimental-strip-types scripts/lint-tsc-ratchet.ts", "lint:tsc:suppress": "node --experimental-strip-types scripts/lint-tsc.ts --suppress-all", "lint:tsconfigs": "node --import ./scripts/resolver/register.ts --experimental-transform-types scripts/lint-tsconfigs/lint-tsconfigs.ts", "lint:tsconfigs:all": "yarn workspaces foreach --all --parallel --interlaced --verbose run lint:tsconfigs", diff --git a/scripts/lib/lint-suppressions-ratchet.test.ts b/scripts/lib/lint-suppressions-ratchet.test.ts new file mode 100644 index 00000000000..fcc46c67920 --- /dev/null +++ b/scripts/lib/lint-suppressions-ratchet.test.ts @@ -0,0 +1,184 @@ +import { jest } from '@jest/globals'; + +// `jest.mock` does not apply to ES modules, so the module registry is stubbed +// with `jest.unstable_mockModule` and the modules under test are imported +// dynamically afterwards. +jest.unstable_mockModule('execa', () => ({ + execa: jest.fn(), +})); + +jest.unstable_mockModule('./tsc-suppressions.ts', () => ({ + SUPPRESSIONS_FILE_NAME: 'tsc-suppressions.json', + readSuppressions: jest.fn(), +})); + +const { execa } = await import('execa'); +const tscSuppressions = await import('./tsc-suppressions.ts'); +const { + findAddedSuppressions, + printAddedSuppressions, + lintSuppressionsRatchet, +} = await import('./lint-suppressions-ratchet.ts'); + +describe('findAddedSuppressions', () => { + it('flags a file that the baseline does not suppress at all', () => { + expect( + findAddedSuppressions({ + current: { 'a.ts': { 'no-shadow': { count: 1 } } }, + base: {}, + }), + ).toStrictEqual([ + { filePath: 'a.ts', rule: 'no-shadow', count: 1, baseCount: 0 }, + ]); + }); + + it('flags a rule that the baseline does not suppress within a file it does', () => { + expect( + findAddedSuppressions({ + current: { + 'a.ts': { 'no-shadow': { count: 1 }, 'id-length': { count: 1 } }, + }, + base: { 'a.ts': { 'no-shadow': { count: 1 } } }, + }), + ).toStrictEqual([ + { filePath: 'a.ts', rule: 'id-length', count: 1, baseCount: 0 }, + ]); + }); + + it('flags a count that has grown', () => { + expect( + findAddedSuppressions({ + current: { 'a.ts': { TS2322: { count: 3 } } }, + base: { 'a.ts': { TS2322: { count: 2 } } }, + }), + ).toStrictEqual([ + { filePath: 'a.ts', rule: 'TS2322', count: 3, baseCount: 2 }, + ]); + }); + + it('allows a count that is unchanged', () => { + expect( + findAddedSuppressions({ + current: { 'a.ts': { TS2322: { count: 2 } } }, + base: { 'a.ts': { TS2322: { count: 2 } } }, + }), + ).toStrictEqual([]); + }); + + it('allows a count that has shrunk', () => { + expect( + findAddedSuppressions({ + current: { 'a.ts': { TS2322: { count: 1 } } }, + base: { 'a.ts': { TS2322: { count: 5 } } }, + }), + ).toStrictEqual([]); + }); + + it('allows a rule or a file to disappear entirely', () => { + expect( + findAddedSuppressions({ + current: {}, + base: { 'a.ts': { TS2322: { count: 5 }, TS7005: { count: 1 } } }, + }), + ).toStrictEqual([]); + }); + + it('reports every addition, not just the first', () => { + expect( + findAddedSuppressions({ + current: { + 'a.ts': { TS2322: { count: 1 } }, + 'b.ts': { TS7005: { count: 2 } }, + }, + base: {}, + }), + ).toHaveLength(2); + }); +}); + +describe('printAddedSuppressions', () => { + beforeEach(() => { + jest.spyOn(console, 'log').mockReturnValue(undefined); + }); + + it('announces success when nothing was added, naming the file', () => { + printAddedSuppressions('oxlint-suppressions.json', []); + + expect(console.log).toHaveBeenCalledWith( + '✅ Nothing has been added to oxlint-suppressions.json. Good job!', + ); + }); + + it('prints each addition and how to resolve it', () => { + printAddedSuppressions('oxlint-suppressions.json', [ + { filePath: 'a.ts', rule: 'no-shadow', count: 3, baseCount: 2 }, + ]); + + const output = jest.mocked(console.log).mock.calls.flat().join('\n'); + expect(output).toContain('oxlint-suppressions.json'); + expect(output).toContain('a.ts'); + expect(output).toContain('no-shadow'); + expect(output).toContain('3'); + expect(output).toContain('2'); + }); +}); + +describe('lintSuppressionsRatchet', () => { + let originalProcess: typeof globalThis.process; + + beforeEach(() => { + originalProcess = globalThis.process; + // The exit code is reset because it is global state that another test file + // may have set. + globalThis.process = { ...globalThis.process, exitCode: undefined }; + jest.spyOn(console, 'log').mockReturnValue(undefined); + jest.mocked(execa).mockResolvedValue({ stdout: '{}' } as never); + jest.mocked(tscSuppressions.readSuppressions).mockResolvedValue({}); + }); + + afterEach(() => { + globalThis.process = originalProcess; + }); + + it('checks both suppressions files against the merge commit CI checks out', async () => { + await lintSuppressionsRatchet([]); + + expect(jest.mocked(execa).mock.calls.map((call) => call[1])).toStrictEqual([ + ['show', 'HEAD^1:oxlint-suppressions.json'], + ['show', 'HEAD^1:tsc-suppressions.json'], + ]); + }); + + it('checks them against the ref it is given', async () => { + await lintSuppressionsRatchet(['origin/main']); + + expect(jest.mocked(execa).mock.calls.map((call) => call[1])).toStrictEqual([ + ['show', 'origin/main:oxlint-suppressions.json'], + ['show', 'origin/main:tsc-suppressions.json'], + ]); + }); + + it('leaves the exit code alone when nothing has been added', async () => { + await lintSuppressionsRatchet([]); + + expect(process.exitCode).toBeUndefined(); + }); + + it('exits with a non-zero code when a file has grown', async () => { + jest + .mocked(tscSuppressions.readSuppressions) + .mockResolvedValue({ 'a.ts': { 'no-shadow': { count: 1 } } }); + + await lintSuppressionsRatchet([]); + + expect(process.exitCode).toBe(1); + }); + + it('throws when a baseline cannot be read, rather than passing', async () => { + jest.mocked(execa).mockRejectedValue(new Error('unknown revision')); + + await expect(lintSuppressionsRatchet([])).rejects.toThrow( + 'unknown revision', + ); + }); +}); diff --git a/scripts/lib/lint-suppressions-ratchet.ts b/scripts/lib/lint-suppressions-ratchet.ts new file mode 100644 index 00000000000..b155f990215 --- /dev/null +++ b/scripts/lib/lint-suppressions-ratchet.ts @@ -0,0 +1,149 @@ +import { execa } from 'execa'; +import path from 'path'; +import { fileURLToPath } from 'url'; + +import { + SUPPRESSIONS_FILE_NAME as TSC_SUPPRESSIONS_FILE_NAME, + readSuppressions, +} from './tsc-suppressions.ts'; + +const REPO_ROOT = path.join( + path.dirname(fileURLToPath(import.meta.url)), + '..', + '..', +); + +/** + * The suppressions files this guards, which Oxlint and the type error checker + * write in the same shape. + */ +const SUPPRESSIONS_FILE_NAMES = [ + 'oxlint-suppressions.json', + TSC_SUPPRESSIONS_FILE_NAME, +]; + +/** + * The first parent of the merge commit that CI checks a pull request out as, + * which is the branch the work is destined for. + */ +const DEFAULT_BASE_REF = 'HEAD^1'; + +/** + * Problems that are knowingly ignored, counted by file and then by the rule or + * error code that reports them. Oxlint and the type error checker both write + * their suppressions this way. + */ +export type Suppressions = Record>; + +/** + * A suppression that covers more problems than the baseline it is compared + * against, meaning that problems have been added rather than fixed. + */ +export type AddedSuppression = { + filePath: string; + rule: string; + count: number; + baseCount: number; +}; + +/** + * Finds the suppressions that cover more problems than a baseline does. + * + * Suppressions are meant to be worked off, never added to: a problem that is + * new should be fixed rather than recorded. Removing suppressions, or shrinking + * their counts, is always allowed. + * + * @param args - The arguments to this function. + * @param args.current - The suppressions as they now stand. + * @param args.base - The suppressions to measure them against. + * @returns Every suppression that grew or appeared, in file order. + */ +export function findAddedSuppressions({ + current, + base, +}: { + current: Suppressions; + base: Suppressions; +}): AddedSuppression[] { + const added: AddedSuppression[] = []; + + for (const [filePath, currentByRule] of Object.entries(current)) { + for (const [rule, { count }] of Object.entries(currentByRule)) { + const baseCount = base[filePath]?.[rule]?.count ?? 0; + if (count > baseCount) { + added.push({ filePath, rule, count, baseCount }); + } + } + } + + return added; +} + +/** + * Prints the suppressions that have been added to a file, if any. + * + * @param fileName - The suppressions file the additions were found in. + * @param added - The added suppressions to print. + */ +export function printAddedSuppressions( + fileName: string, + added: AddedSuppression[], +): void { + if (added.length === 0) { + console.log(`✅ Nothing has been added to ${fileName}. Good job!`); + return; + } + + console.log(`❌ Detected problems added to ${fileName}:\n`); + for (const suppression of added) { + console.log( + ` ${suppression.filePath}: ${suppression.rule} (${suppression.count} suppressed, was ${suppression.baseCount})`, + ); + } + console.log( + '\nSuppressions may only be removed, never added. Fix these problems rather than suppressing them.', + ); +} + +/** + * Checks that no problems have been added to any of the suppressions files. + * + * The lint and type error checks keep new problems from landing, but their + * escape hatch — regenerating a suppressions file — can be used to paper over + * one rather than fix it. This closes that hatch: measured against the base + * branch, these files may only shrink. + * + * CI checks a pull request out as the merge of the branch into its base, so the + * base is simply the first parent, and the files in the working tree already + * have the base's own changes folded in. That leaves this to compare two files, + * with no merge base to work out and nothing to fetch. + * + * Pass a ref to compare against something else, which is useful when running + * this outside of CI, where there is no merge commit. + * + * @param argv - The arguments passed to this script. + */ +export async function lintSuppressionsRatchet( + argv: readonly string[], +): Promise { + const baseRef = argv[0] ?? DEFAULT_BASE_REF; + let didPass = true; + + for (const fileName of SUPPRESSIONS_FILE_NAMES) { + // Left to throw if the ref or the file is missing, as a check that cannot + // find its baseline must not wave the change through. + const { stdout } = await execa('git', ['show', `${baseRef}:${fileName}`], { + cwd: REPO_ROOT, + }); + const base = JSON.parse(stdout) as Suppressions; + const current = await readSuppressions(path.join(REPO_ROOT, fileName)); + + const added = findAddedSuppressions({ current, base }); + printAddedSuppressions(fileName, added); + didPass = didPass && added.length === 0; + } + + if (!didPass) { + process.exitCode = 1; + } +} diff --git a/scripts/lib/lint-tsc-ratchet.test.ts b/scripts/lib/lint-tsc-ratchet.test.ts deleted file mode 100644 index 487fc6d0f8b..00000000000 --- a/scripts/lib/lint-tsc-ratchet.test.ts +++ /dev/null @@ -1,98 +0,0 @@ -import { jest } from '@jest/globals'; - -// `jest.mock` does not apply to ES modules, so the module registry is stubbed -// with `jest.unstable_mockModule` and the modules under test are imported -// dynamically afterwards. -jest.unstable_mockModule('execa', () => ({ - execa: jest.fn(), -})); - -jest.unstable_mockModule('./tsc-suppressions.ts', () => ({ - findAddedSuppressions: jest.fn(), - printAddedSuppressions: jest.fn(), - readSuppressions: jest.fn(), -})); - -const { execa } = await import('execa'); -const tscSuppressions = await import('./tsc-suppressions.ts'); -const { lintTscRatchet } = await import('./lint-tsc-ratchet.ts'); - -const BASE = { 'a.ts': { TS2322: { count: 2 } } }; -const CURRENT = { 'a.ts': { TS2322: { count: 1 } } }; - -const ADDITION = { - filePath: 'a.ts', - code: 'TS2322', - count: 3, - baseCount: 2, -}; - -describe('lintTscRatchet', () => { - let originalProcess: typeof globalThis.process; - - beforeEach(() => { - originalProcess = globalThis.process; - // The exit code is reset because it is global state that another test file - // may have set. - globalThis.process = { ...globalThis.process, exitCode: undefined }; - jest.spyOn(console, 'log').mockReturnValue(undefined); - jest.mocked(execa).mockResolvedValue({ - stdout: JSON.stringify(BASE), - } as never); - jest.mocked(tscSuppressions.readSuppressions).mockResolvedValue(CURRENT); - jest.mocked(tscSuppressions.findAddedSuppressions).mockReturnValue([]); - }); - - afterEach(() => { - globalThis.process = originalProcess; - }); - - it('compares against the first parent of the merge commit CI checks out', async () => { - await lintTscRatchet([]); - - expect(jest.mocked(execa).mock.calls[0]?.slice(0, 2)).toStrictEqual([ - 'git', - ['show', 'HEAD^1:tsc-suppressions.json'], - ]); - expect(tscSuppressions.findAddedSuppressions).toHaveBeenCalledWith({ - current: CURRENT, - base: BASE, - }); - }); - - it('compares against the ref it is given', async () => { - await lintTscRatchet(['origin/main']); - - expect(jest.mocked(execa).mock.calls[0]?.slice(0, 2)).toStrictEqual([ - 'git', - ['show', 'origin/main:tsc-suppressions.json'], - ]); - }); - - it('leaves the exit code alone when nothing has been added', async () => { - await lintTscRatchet([]); - - expect(tscSuppressions.printAddedSuppressions).toHaveBeenCalledWith([]); - expect(process.exitCode).toBeUndefined(); - }); - - it('exits with a non-zero code when suppressions have been added', async () => { - jest - .mocked(tscSuppressions.findAddedSuppressions) - .mockReturnValue([ADDITION]); - - await lintTscRatchet([]); - - expect(tscSuppressions.printAddedSuppressions).toHaveBeenCalledWith([ - ADDITION, - ]); - expect(process.exitCode).toBe(1); - }); - - it('throws when the baseline cannot be read, rather than passing', async () => { - jest.mocked(execa).mockRejectedValue(new Error('unknown revision')); - - await expect(lintTscRatchet([])).rejects.toThrow('unknown revision'); - expect(tscSuppressions.findAddedSuppressions).not.toHaveBeenCalled(); - }); -}); diff --git a/scripts/lib/lint-tsc-ratchet.ts b/scripts/lib/lint-tsc-ratchet.ts deleted file mode 100644 index 9bdc5e4b842..00000000000 --- a/scripts/lib/lint-tsc-ratchet.ts +++ /dev/null @@ -1,65 +0,0 @@ -import { execa } from 'execa'; -import path from 'path'; -import { fileURLToPath } from 'url'; - -import { - findAddedSuppressions, - printAddedSuppressions, - readSuppressions, -} from './tsc-suppressions.ts'; -import type { TscSuppressions } from './tsc-suppressions.ts'; - -const REPO_ROOT = path.join( - path.dirname(fileURLToPath(import.meta.url)), - '..', - '..', -); - -const SUPPRESSIONS_FILE_NAME = 'tsc-suppressions.json'; - -/** - * The first parent of the merge commit that CI checks a pull request out as, - * which is the branch the work is destined for. - */ -const DEFAULT_BASE_REF = 'HEAD^1'; - -/** - * Checks that no type errors have been added to the suppressions file. - * - * `lint:tsc:check` keeps errors that are not suppressed from landing, but its - * escape hatch — regenerating the file — can be used to paper over new errors - * rather than fix them. This closes that hatch: measured against the base - * branch, the file may only shrink. - * - * CI checks a pull request out as the merge of the branch into its base, so the - * base is simply the first parent, and the file in the working tree already has - * the base's own changes folded in. That leaves this to compare two files, with - * no merge base to work out and nothing to fetch. - * - * Pass a ref to compare against something else, which is useful when running - * this outside of CI, where there is no merge commit. - * - * @param argv - The arguments passed to this script. - */ -export async function lintTscRatchet(argv: readonly string[]): Promise { - const baseRef = argv[0] ?? DEFAULT_BASE_REF; - - // Left to throw if the ref or the file is missing, as a check that cannot - // find its baseline must not wave the change through. - const { stdout } = await execa( - 'git', - ['show', `${baseRef}:${SUPPRESSIONS_FILE_NAME}`], - { cwd: REPO_ROOT }, - ); - const base = JSON.parse(stdout) as TscSuppressions; - - const current = await readSuppressions( - path.join(REPO_ROOT, SUPPRESSIONS_FILE_NAME), - ); - const added = findAddedSuppressions({ current, base }); - printAddedSuppressions(added); - - if (added.length > 0) { - process.exitCode = 1; - } -} diff --git a/scripts/lib/lint-tsc.test.ts b/scripts/lib/lint-tsc.test.ts index d968c7736dd..09913561bdc 100644 --- a/scripts/lib/lint-tsc.test.ts +++ b/scripts/lib/lint-tsc.test.ts @@ -10,6 +10,7 @@ jest.unstable_mockModule('execa', () => ({ })); jest.unstable_mockModule('./tsc-suppressions.ts', () => ({ + SUPPRESSIONS_FILE_NAME: 'tsc-suppressions.json', parseTscOutput: jest.fn(), isTscError: jest.fn(), addSuppressions: jest.fn(), diff --git a/scripts/lib/lint-tsc.ts b/scripts/lib/lint-tsc.ts index b1dd88aa563..b4ee3db8972 100644 --- a/scripts/lib/lint-tsc.ts +++ b/scripts/lib/lint-tsc.ts @@ -3,6 +3,7 @@ import path from 'path'; import { fileURLToPath } from 'url'; import { + SUPPRESSIONS_FILE_NAME, addSuppressions, compareErrorsToSuppressions, isTscError, @@ -19,8 +20,6 @@ const REPO_ROOT = path.join( '..', ); -const SUPPRESSIONS_FILE_NAME = 'tsc-suppressions.json'; - /** * Typechecks every package in the repo and compares the type errors it finds * against `tsc-suppressions.json`, failing if any error is not suppressed there diff --git a/scripts/lib/tsc-suppressions.test.ts b/scripts/lib/tsc-suppressions.test.ts index b7c1e66607f..b3f0d06ba29 100644 --- a/scripts/lib/tsc-suppressions.test.ts +++ b/scripts/lib/tsc-suppressions.test.ts @@ -12,10 +12,8 @@ import { buildSuppressions, compareErrorsToSuppressions, compareStrings, - findAddedSuppressions, isTscError, parseTscOutput, - printAddedSuppressions, printReport, pruneSuppressions, readSuppressions, @@ -377,106 +375,6 @@ describe('compareErrorsToSuppressions', () => { }); }); -describe('findAddedSuppressions', () => { - it('flags a file that the baseline does not suppress at all', () => { - const added = findAddedSuppressions({ - current: { 'a.ts': { TS2322: { count: 1 } } }, - base: {}, - }); - - expect(added).toStrictEqual([ - { filePath: 'a.ts', code: 'TS2322', count: 1, baseCount: 0 }, - ]); - }); - - it('flags a code that the baseline does not suppress within a file it does', () => { - const added = findAddedSuppressions({ - current: { 'a.ts': { TS2322: { count: 1 }, TS7005: { count: 1 } } }, - base: { 'a.ts': { TS2322: { count: 1 } } }, - }); - - expect(added).toStrictEqual([ - { filePath: 'a.ts', code: 'TS7005', count: 1, baseCount: 0 }, - ]); - }); - - it('flags a count that has grown', () => { - const added = findAddedSuppressions({ - current: { 'a.ts': { TS2322: { count: 3 } } }, - base: { 'a.ts': { TS2322: { count: 2 } } }, - }); - - expect(added).toStrictEqual([ - { filePath: 'a.ts', code: 'TS2322', count: 3, baseCount: 2 }, - ]); - }); - - it('allows a count that is unchanged', () => { - expect( - findAddedSuppressions({ - current: { 'a.ts': { TS2322: { count: 2 } } }, - base: { 'a.ts': { TS2322: { count: 2 } } }, - }), - ).toStrictEqual([]); - }); - - it('allows a count that has shrunk', () => { - expect( - findAddedSuppressions({ - current: { 'a.ts': { TS2322: { count: 1 } } }, - base: { 'a.ts': { TS2322: { count: 5 } } }, - }), - ).toStrictEqual([]); - }); - - it('allows a code or a file to disappear entirely', () => { - expect( - findAddedSuppressions({ - current: {}, - base: { 'a.ts': { TS2322: { count: 5 }, TS7005: { count: 1 } } }, - }), - ).toStrictEqual([]); - }); - - it('reports every addition, not just the first', () => { - const added = findAddedSuppressions({ - current: { - 'a.ts': { TS2322: { count: 1 } }, - 'b.ts': { TS7005: { count: 2 } }, - }, - base: {}, - }); - - expect(added).toHaveLength(2); - }); -}); - -describe('printAddedSuppressions', () => { - beforeEach(() => { - jest.spyOn(console, 'log').mockReturnValue(undefined); - }); - - it('announces success when nothing was added', () => { - printAddedSuppressions([]); - - expect(console.log).toHaveBeenCalledWith( - '✅ No type errors have been added to the suppressions file. Good job!', - ); - }); - - it('prints each addition and how to resolve it', () => { - printAddedSuppressions([ - { filePath: 'a.ts', code: 'TS2322', count: 3, baseCount: 2 }, - ]); - - const output = jest.mocked(console.log).mock.calls.flat().join('\n'); - expect(output).toContain('a.ts'); - expect(output).toContain('TS2322'); - expect(output).toContain('3'); - expect(output).toContain('2'); - }); -}); - describe('readSuppressions', () => { it('reads the suppressions that the file holds', async () => { expect.assertions(1); diff --git a/scripts/lib/tsc-suppressions.ts b/scripts/lib/tsc-suppressions.ts index 1453e18ab98..f35478a0e1b 100644 --- a/scripts/lib/tsc-suppressions.ts +++ b/scripts/lib/tsc-suppressions.ts @@ -1,5 +1,10 @@ import fs from 'fs/promises'; +/** + * The file in which the type errors that are knowingly ignored are recorded. + */ +export const SUPPRESSIONS_FILE_NAME = 'tsc-suppressions.json'; + /** * A diagnostic reported by `tsc`. Most belong to a file; those that report a * broken build rather than a type error do not. @@ -58,17 +63,6 @@ export type StaleSuppression = { suppressedCount: number; }; -/** - * A suppression that covers more errors than the baseline it is compared - * against, meaning that type errors have been added rather than fixed. - */ -export type AddedSuppression = { - filePath: string; - code: string; - count: number; - baseCount: number; -}; - /** * The result of checking the type errors in the repo against the suppressions * file. @@ -364,63 +358,6 @@ export function compareErrorsToSuppressions({ }; } -/** - * Finds the suppressions that cover more errors than a baseline does. - * - * Suppressions are meant to be worked off, never added to: an error that is new - * should be fixed rather than recorded. Removing suppressions, or shrinking - * their counts, is always allowed. - * - * @param args - The arguments to this function. - * @param args.current - The suppressions as they now stand. - * @param args.base - The suppressions to measure them against. - * @returns Every suppression that grew or appeared, in file order. - */ -export function findAddedSuppressions({ - current, - base, -}: { - current: TscSuppressions; - base: TscSuppressions; -}): AddedSuppression[] { - const added: AddedSuppression[] = []; - - for (const [filePath, currentByCode] of Object.entries(current)) { - for (const [code, { count }] of Object.entries(currentByCode)) { - const baseCount = base[filePath]?.[code]?.count ?? 0; - if (count > baseCount) { - added.push({ filePath, code, count, baseCount }); - } - } - } - - return added; -} - -/** - * Prints the suppressions that have been added, if any. - * - * @param added - The added suppressions to print. - */ -export function printAddedSuppressions(added: AddedSuppression[]): void { - if (added.length === 0) { - console.log( - '✅ No type errors have been added to the suppressions file. Good job!', - ); - return; - } - - console.log('❌ Detected type errors added to the suppressions file:\n'); - for (const suppression of added) { - console.log( - ` ${suppression.filePath}: ${suppression.code} (${suppression.count} suppressed, was ${suppression.baseCount})`, - ); - } - console.log( - '\nSuppressions may only be removed, never added. Fix these type errors rather than suppressing them.', - ); -} - /** * Reads the suppressions file, treating a missing file as having no * suppressions. diff --git a/scripts/lint-suppressions-ratchet.test.ts b/scripts/lint-suppressions-ratchet.test.ts new file mode 100644 index 00000000000..ab6bf6ee79e --- /dev/null +++ b/scripts/lint-suppressions-ratchet.test.ts @@ -0,0 +1,23 @@ +import { jest } from '@jest/globals'; + +// `jest.mock` does not apply to ES modules, so the module registry is stubbed +// with `jest.unstable_mockModule` and the modules under test are imported +// dynamically afterwards. +jest.unstable_mockModule('./lib/lint-suppressions-ratchet.ts', () => ({ + lintSuppressionsRatchet: jest.fn(), +})); + +const { lintSuppressionsRatchet } = + await import('./lib/lint-suppressions-ratchet.ts'); + +describe('lint-suppressions-ratchet', () => { + it('runs the check with the arguments it was given', async () => { + jest.mocked(lintSuppressionsRatchet).mockResolvedValue(undefined); + + // Importing the entry point runs it, which is the behaviour under test. + await import('./lint-suppressions-ratchet.ts'); + + expect(lintSuppressionsRatchet).toHaveBeenCalledTimes(1); + expect(lintSuppressionsRatchet).toHaveBeenCalledWith(process.argv.slice(2)); + }); +}); diff --git a/scripts/lint-suppressions-ratchet.ts b/scripts/lint-suppressions-ratchet.ts new file mode 100644 index 00000000000..a895b569d84 --- /dev/null +++ b/scripts/lint-suppressions-ratchet.ts @@ -0,0 +1,7 @@ +/** + * Entry point file for the `lint:suppressions:ratchet` script. + */ + +import { lintSuppressionsRatchet } from './lib/lint-suppressions-ratchet.ts'; + +await lintSuppressionsRatchet(process.argv.slice(2)); diff --git a/scripts/lint-tsc-ratchet.test.ts b/scripts/lint-tsc-ratchet.test.ts deleted file mode 100644 index 4c806f28cdb..00000000000 --- a/scripts/lint-tsc-ratchet.test.ts +++ /dev/null @@ -1,22 +0,0 @@ -import { jest } from '@jest/globals'; - -// `jest.mock` does not apply to ES modules, so the module registry is stubbed -// with `jest.unstable_mockModule` and the modules under test are imported -// dynamically afterwards. -jest.unstable_mockModule('./lib/lint-tsc-ratchet.ts', () => ({ - lintTscRatchet: jest.fn(), -})); - -const { lintTscRatchet } = await import('./lib/lint-tsc-ratchet.ts'); - -describe('lint-tsc-ratchet', () => { - it('runs the check with the arguments it was given', async () => { - jest.mocked(lintTscRatchet).mockResolvedValue(undefined); - - // Importing the entry point runs it, which is the behaviour under test. - await import('./lint-tsc-ratchet.ts'); - - expect(lintTscRatchet).toHaveBeenCalledTimes(1); - expect(lintTscRatchet).toHaveBeenCalledWith(process.argv.slice(2)); - }); -}); diff --git a/scripts/lint-tsc-ratchet.ts b/scripts/lint-tsc-ratchet.ts deleted file mode 100644 index 3c8ad9ae5f6..00000000000 --- a/scripts/lint-tsc-ratchet.ts +++ /dev/null @@ -1,7 +0,0 @@ -/** - * Entry point file for the `lint:tsc:ratchet` script. - */ - -import { lintTscRatchet } from './lib/lint-tsc-ratchet.ts'; - -await lintTscRatchet(process.argv.slice(2)); From 197abc34e182c8932d1ddc138dad446cc61bc5ca Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 30 Sep 2026 21:06:29 +0200 Subject: [PATCH 07/25] chore: drop the comment above fetch-depth --- .github/workflows/lint-build-test.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index 68b3d23f1f0..49439c85f32 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -180,8 +180,6 @@ jobs: is-high-risk-environment: false persist-credentials: false node-version: ${{ matrix.node-version }} - # The pull request is checked out as the merge of the branch into its - # base, and the check reads the base from the first parent. fetch-depth: 2 - name: Run yarn lint:suppressions:ratchet run: yarn lint:suppressions:ratchet From 15b2767346539abb61f64f186d7e129b42815a36 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 30 Sep 2026 21:08:28 +0200 Subject: [PATCH 08/25] chore: drop the jest.unstable_mockModule comment --- scripts/lib/lint-suppressions-ratchet.test.ts | 3 --- scripts/lint-suppressions-ratchet.test.ts | 3 --- 2 files changed, 6 deletions(-) diff --git a/scripts/lib/lint-suppressions-ratchet.test.ts b/scripts/lib/lint-suppressions-ratchet.test.ts index fcc46c67920..40315d43705 100644 --- a/scripts/lib/lint-suppressions-ratchet.test.ts +++ b/scripts/lib/lint-suppressions-ratchet.test.ts @@ -1,8 +1,5 @@ import { jest } from '@jest/globals'; -// `jest.mock` does not apply to ES modules, so the module registry is stubbed -// with `jest.unstable_mockModule` and the modules under test are imported -// dynamically afterwards. jest.unstable_mockModule('execa', () => ({ execa: jest.fn(), })); diff --git a/scripts/lint-suppressions-ratchet.test.ts b/scripts/lint-suppressions-ratchet.test.ts index ab6bf6ee79e..35f51b5a518 100644 --- a/scripts/lint-suppressions-ratchet.test.ts +++ b/scripts/lint-suppressions-ratchet.test.ts @@ -1,8 +1,5 @@ import { jest } from '@jest/globals'; -// `jest.mock` does not apply to ES modules, so the module registry is stubbed -// with `jest.unstable_mockModule` and the modules under test are imported -// dynamically afterwards. jest.unstable_mockModule('./lib/lint-suppressions-ratchet.ts', () => ({ lintSuppressionsRatchet: jest.fn(), })); From f565e57dc66d1bb2cd720229e784c6017a9194ee Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 30 Sep 2026 21:13:22 +0200 Subject: [PATCH 09/25] chore: name the oxlint suppressions file --- scripts/lib/lint-suppressions-ratchet.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/lib/lint-suppressions-ratchet.ts b/scripts/lib/lint-suppressions-ratchet.ts index b155f990215..59a3eeecd2b 100644 --- a/scripts/lib/lint-suppressions-ratchet.ts +++ b/scripts/lib/lint-suppressions-ratchet.ts @@ -13,12 +13,17 @@ const REPO_ROOT = path.join( '..', ); +/** + * The file in which the lint problems that are knowingly ignored are recorded. + */ +const OXLINT_SUPPRESSIONS_FILE_NAME = 'oxlint-suppressions.json'; + /** * The suppressions files this guards, which Oxlint and the type error checker * write in the same shape. */ const SUPPRESSIONS_FILE_NAMES = [ - 'oxlint-suppressions.json', + OXLINT_SUPPRESSIONS_FILE_NAME, TSC_SUPPRESSIONS_FILE_NAME, ]; From 70e0a7cde677c3768d66332d9488cd575407c2ba Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 30 Sep 2026 22:15:08 +0200 Subject: [PATCH 10/25] chore: drop the strip types flag Node 24 strips types by default, which is what CI runs and what .nvmrc pins. --- package.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/package.json b/package.json index c7396af30b1..4ce505a0d20 100644 --- a/package.json +++ b/package.json @@ -38,14 +38,14 @@ "lint:misc": "oxfmt --ignore-path .gitignore", "lint:misc:check": "yarn lint:misc --check", "lint:oxlint": "oxlint", - "lint:suppressions:ratchet": "node --experimental-strip-types scripts/lint-suppressions-ratchet.ts", + "lint:suppressions:ratchet": "node scripts/lint-suppressions-ratchet.ts", "lint:teams": "node --import ./scripts/resolver/register.ts --experimental-transform-types scripts/lint-teams-json.ts", "lint:tsc": "tsc --build tsconfig.lint.json", - "lint:tsc:check": "node --experimental-strip-types scripts/lint-tsc.ts", + "lint:tsc:check": "node scripts/lint-tsc.ts", "lint:tsc:clean": "yarn lint:tsc:only-clean && yarn lint:tsc", "lint:tsc:only-clean": "rimraf -g 'packages/*/.tsc-lint-cache' '.tsc-lint-cache'", - "lint:tsc:prune": "node --experimental-strip-types scripts/lint-tsc.ts --prune-suppressions", - "lint:tsc:suppress": "node --experimental-strip-types scripts/lint-tsc.ts --suppress-all", + "lint:tsc:prune": "node scripts/lint-tsc.ts --prune-suppressions", + "lint:tsc:suppress": "node scripts/lint-tsc.ts --suppress-all", "lint:tsconfigs": "node --import ./scripts/resolver/register.ts --experimental-transform-types scripts/lint-tsconfigs/lint-tsconfigs.ts", "lint:tsconfigs:all": "yarn workspaces foreach --all --parallel --interlaced --verbose run lint:tsconfigs", "lint:tsconfigs:fix": "node --import ./scripts/resolver/register.ts --experimental-transform-types scripts/lint-tsconfigs/lint-tsconfigs.ts --fix", From 4feb1d04415888d76a03768c769f3e700cc0e5ab Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 30 Sep 2026 22:20:37 +0200 Subject: [PATCH 11/25] chore: rename to lint:suppressions --- .github/workflows/lint-build-test.yml | 8 +++---- package.json | 2 +- ...chet.test.ts => lint-suppressions.test.ts} | 21 +++++++------------ ...ssions-ratchet.ts => lint-suppressions.ts} | 4 +--- scripts/lint-suppressions-ratchet.test.ts | 20 ------------------ scripts/lint-suppressions-ratchet.ts | 7 ------- scripts/lint-suppressions.test.ts | 19 +++++++++++++++++ scripts/lint-suppressions.ts | 7 +++++++ 8 files changed, 40 insertions(+), 48 deletions(-) rename scripts/lib/{lint-suppressions-ratchet.test.ts => lint-suppressions.test.ts} (91%) rename scripts/lib/{lint-suppressions-ratchet.ts => lint-suppressions.ts} (98%) delete mode 100644 scripts/lint-suppressions-ratchet.test.ts delete mode 100644 scripts/lint-suppressions-ratchet.ts create mode 100644 scripts/lint-suppressions.test.ts create mode 100644 scripts/lint-suppressions.ts diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index 49439c85f32..6ecab983fd8 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -166,8 +166,8 @@ jobs: exit 1 fi - lint-suppressions-ratchet: - name: Lint (lint:suppressions:ratchet) + lint-suppressions: + name: Lint (lint:suppressions) if: github.event_name == 'pull_request' || github.event_name == 'merge_group' runs-on: ubuntu-latest strategy: @@ -181,8 +181,8 @@ jobs: persist-credentials: false node-version: ${{ matrix.node-version }} fetch-depth: 2 - - name: Run yarn lint:suppressions:ratchet - run: yarn lint:suppressions:ratchet + - name: Run yarn lint:suppressions + run: yarn lint:suppressions - name: Require clean working directory shell: bash run: | diff --git a/package.json b/package.json index 4ce505a0d20..c01020afd41 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "lint:misc": "oxfmt --ignore-path .gitignore", "lint:misc:check": "yarn lint:misc --check", "lint:oxlint": "oxlint", - "lint:suppressions:ratchet": "node scripts/lint-suppressions-ratchet.ts", + "lint:suppressions": "node scripts/lint-suppressions.ts", "lint:teams": "node --import ./scripts/resolver/register.ts --experimental-transform-types scripts/lint-teams-json.ts", "lint:tsc": "tsc --build tsconfig.lint.json", "lint:tsc:check": "node scripts/lint-tsc.ts", diff --git a/scripts/lib/lint-suppressions-ratchet.test.ts b/scripts/lib/lint-suppressions.test.ts similarity index 91% rename from scripts/lib/lint-suppressions-ratchet.test.ts rename to scripts/lib/lint-suppressions.test.ts index 40315d43705..c4807d4d125 100644 --- a/scripts/lib/lint-suppressions-ratchet.test.ts +++ b/scripts/lib/lint-suppressions.test.ts @@ -11,11 +11,8 @@ jest.unstable_mockModule('./tsc-suppressions.ts', () => ({ const { execa } = await import('execa'); const tscSuppressions = await import('./tsc-suppressions.ts'); -const { - findAddedSuppressions, - printAddedSuppressions, - lintSuppressionsRatchet, -} = await import('./lint-suppressions-ratchet.ts'); +const { findAddedSuppressions, printAddedSuppressions, lintSuppressions } = + await import('./lint-suppressions.ts'); describe('findAddedSuppressions', () => { it('flags a file that the baseline does not suppress at all', () => { @@ -120,7 +117,7 @@ describe('printAddedSuppressions', () => { }); }); -describe('lintSuppressionsRatchet', () => { +describe('lintSuppressions', () => { let originalProcess: typeof globalThis.process; beforeEach(() => { @@ -138,7 +135,7 @@ describe('lintSuppressionsRatchet', () => { }); it('checks both suppressions files against the merge commit CI checks out', async () => { - await lintSuppressionsRatchet([]); + await lintSuppressions([]); expect(jest.mocked(execa).mock.calls.map((call) => call[1])).toStrictEqual([ ['show', 'HEAD^1:oxlint-suppressions.json'], @@ -147,7 +144,7 @@ describe('lintSuppressionsRatchet', () => { }); it('checks them against the ref it is given', async () => { - await lintSuppressionsRatchet(['origin/main']); + await lintSuppressions(['origin/main']); expect(jest.mocked(execa).mock.calls.map((call) => call[1])).toStrictEqual([ ['show', 'origin/main:oxlint-suppressions.json'], @@ -156,7 +153,7 @@ describe('lintSuppressionsRatchet', () => { }); it('leaves the exit code alone when nothing has been added', async () => { - await lintSuppressionsRatchet([]); + await lintSuppressions([]); expect(process.exitCode).toBeUndefined(); }); @@ -166,7 +163,7 @@ describe('lintSuppressionsRatchet', () => { .mocked(tscSuppressions.readSuppressions) .mockResolvedValue({ 'a.ts': { 'no-shadow': { count: 1 } } }); - await lintSuppressionsRatchet([]); + await lintSuppressions([]); expect(process.exitCode).toBe(1); }); @@ -174,8 +171,6 @@ describe('lintSuppressionsRatchet', () => { it('throws when a baseline cannot be read, rather than passing', async () => { jest.mocked(execa).mockRejectedValue(new Error('unknown revision')); - await expect(lintSuppressionsRatchet([])).rejects.toThrow( - 'unknown revision', - ); + await expect(lintSuppressions([])).rejects.toThrow('unknown revision'); }); }); diff --git a/scripts/lib/lint-suppressions-ratchet.ts b/scripts/lib/lint-suppressions.ts similarity index 98% rename from scripts/lib/lint-suppressions-ratchet.ts rename to scripts/lib/lint-suppressions.ts index 59a3eeecd2b..580996296f6 100644 --- a/scripts/lib/lint-suppressions-ratchet.ts +++ b/scripts/lib/lint-suppressions.ts @@ -128,9 +128,7 @@ export function printAddedSuppressions( * * @param argv - The arguments passed to this script. */ -export async function lintSuppressionsRatchet( - argv: readonly string[], -): Promise { +export async function lintSuppressions(argv: readonly string[]): Promise { const baseRef = argv[0] ?? DEFAULT_BASE_REF; let didPass = true; diff --git a/scripts/lint-suppressions-ratchet.test.ts b/scripts/lint-suppressions-ratchet.test.ts deleted file mode 100644 index 35f51b5a518..00000000000 --- a/scripts/lint-suppressions-ratchet.test.ts +++ /dev/null @@ -1,20 +0,0 @@ -import { jest } from '@jest/globals'; - -jest.unstable_mockModule('./lib/lint-suppressions-ratchet.ts', () => ({ - lintSuppressionsRatchet: jest.fn(), -})); - -const { lintSuppressionsRatchet } = - await import('./lib/lint-suppressions-ratchet.ts'); - -describe('lint-suppressions-ratchet', () => { - it('runs the check with the arguments it was given', async () => { - jest.mocked(lintSuppressionsRatchet).mockResolvedValue(undefined); - - // Importing the entry point runs it, which is the behaviour under test. - await import('./lint-suppressions-ratchet.ts'); - - expect(lintSuppressionsRatchet).toHaveBeenCalledTimes(1); - expect(lintSuppressionsRatchet).toHaveBeenCalledWith(process.argv.slice(2)); - }); -}); diff --git a/scripts/lint-suppressions-ratchet.ts b/scripts/lint-suppressions-ratchet.ts deleted file mode 100644 index a895b569d84..00000000000 --- a/scripts/lint-suppressions-ratchet.ts +++ /dev/null @@ -1,7 +0,0 @@ -/** - * Entry point file for the `lint:suppressions:ratchet` script. - */ - -import { lintSuppressionsRatchet } from './lib/lint-suppressions-ratchet.ts'; - -await lintSuppressionsRatchet(process.argv.slice(2)); diff --git a/scripts/lint-suppressions.test.ts b/scripts/lint-suppressions.test.ts new file mode 100644 index 00000000000..f925459eae4 --- /dev/null +++ b/scripts/lint-suppressions.test.ts @@ -0,0 +1,19 @@ +import { jest } from '@jest/globals'; + +jest.unstable_mockModule('./lib/lint-suppressions.ts', () => ({ + lintSuppressions: jest.fn(), +})); + +const { lintSuppressions } = await import('./lib/lint-suppressions.ts'); + +describe('lint-suppressions', () => { + it('runs the check with the arguments it was given', async () => { + jest.mocked(lintSuppressions).mockResolvedValue(undefined); + + // Importing the entry point runs it, which is the behaviour under test. + await import('./lint-suppressions.ts'); + + expect(lintSuppressions).toHaveBeenCalledTimes(1); + expect(lintSuppressions).toHaveBeenCalledWith(process.argv.slice(2)); + }); +}); diff --git a/scripts/lint-suppressions.ts b/scripts/lint-suppressions.ts new file mode 100644 index 00000000000..0ecda08fda5 --- /dev/null +++ b/scripts/lint-suppressions.ts @@ -0,0 +1,7 @@ +/** + * Entry point file for the `lint:suppressions` script. + */ + +import { lintSuppressions } from './lib/lint-suppressions.ts'; + +await lintSuppressions(process.argv.slice(2)); From fe17f7b0058a2f45dd02da7cd0bd54a925ed4f1c Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 30 Sep 2026 22:29:19 +0200 Subject: [PATCH 12/25] chore: make the check work locally without arguments It read the baseline from the first parent, which is the base only when CI checks the pull request out as a merge commit. Anywhere else that is just the previous commit, so a suppression added earlier on the branch went unnoticed. It now falls back to the merge base with the target branch. --- scripts/lib/lint-suppressions.test.ts | 77 ++++++++++++++++++++++++--- scripts/lib/lint-suppressions.ts | 47 ++++++++++++---- 2 files changed, 105 insertions(+), 19 deletions(-) diff --git a/scripts/lib/lint-suppressions.test.ts b/scripts/lib/lint-suppressions.test.ts index c4807d4d125..4fdceb99bd4 100644 --- a/scripts/lib/lint-suppressions.test.ts +++ b/scripts/lib/lint-suppressions.test.ts @@ -117,6 +117,36 @@ describe('printAddedSuppressions', () => { }); }); +/** + * Stubs the Git commands the check runs. + * + * @param args - The arguments to this function. + * @param args.isMergeCommit - Whether HEAD should look like a merge commit. + * @param args.suppressions - The baseline the commands should produce. + */ +function mockGit({ + isMergeCommit, + suppressions = '{}', +}: { + isMergeCommit: boolean; + suppressions?: string; +}): void { + jest.mocked(execa).mockImplementation((async ( + _file: string, + args: string[], + ) => { + if (args[0] === 'rev-list') { + return { + stdout: isMergeCommit ? 'head parentA parentB' : 'head parentA', + }; + } + if (args[0] === 'merge-base') { + return { stdout: 'abc123\n' }; + } + return { stdout: suppressions }; + }) as never); +} + describe('lintSuppressions', () => { let originalProcess: typeof globalThis.process; @@ -126,7 +156,7 @@ describe('lintSuppressions', () => { // may have set. globalThis.process = { ...globalThis.process, exitCode: undefined }; jest.spyOn(console, 'log').mockReturnValue(undefined); - jest.mocked(execa).mockResolvedValue({ stdout: '{}' } as never); + mockGit({ isMergeCommit: true }); jest.mocked(tscSuppressions.readSuppressions).mockResolvedValue({}); }); @@ -134,24 +164,55 @@ describe('lintSuppressions', () => { globalThis.process = originalProcess; }); - it('checks both suppressions files against the merge commit CI checks out', async () => { + it('reads the baseline from the first parent of the merge commit CI checks out', async () => { + mockGit({ isMergeCommit: true }); + await lintSuppressions([]); - expect(jest.mocked(execa).mock.calls.map((call) => call[1])).toStrictEqual([ + expect( + jest + .mocked(execa) + .mock.calls.map((call) => call[1]) + .filter((args) => args[0] === 'show'), + ).toStrictEqual([ ['show', 'HEAD^1:oxlint-suppressions.json'], ['show', 'HEAD^1:tsc-suppressions.json'], ]); }); - it('checks them against the ref it is given', async () => { - await lintSuppressions(['origin/main']); + it('falls back to the merge base where there is no merge commit, as when run locally', async () => { + mockGit({ isMergeCommit: false }); - expect(jest.mocked(execa).mock.calls.map((call) => call[1])).toStrictEqual([ - ['show', 'origin/main:oxlint-suppressions.json'], - ['show', 'origin/main:tsc-suppressions.json'], + await lintSuppressions([]); + + expect(jest.mocked(execa)).toHaveBeenCalledWith( + 'git', + ['merge-base', 'HEAD', 'origin/main'], + expect.anything(), + ); + expect( + jest + .mocked(execa) + .mock.calls.map((call) => call[1]) + .filter((args) => args[0] === 'show'), + ).toStrictEqual([ + ['show', 'abc123:oxlint-suppressions.json'], + ['show', 'abc123:tsc-suppressions.json'], ]); }); + it('takes the merge base against the branch it is given', async () => { + mockGit({ isMergeCommit: false }); + + await lintSuppressions(['origin/release']); + + expect(jest.mocked(execa)).toHaveBeenCalledWith( + 'git', + ['merge-base', 'HEAD', 'origin/release'], + expect.anything(), + ); + }); + it('leaves the exit code alone when nothing has been added', async () => { await lintSuppressions([]); diff --git a/scripts/lib/lint-suppressions.ts b/scripts/lib/lint-suppressions.ts index 580996296f6..41c6c474360 100644 --- a/scripts/lib/lint-suppressions.ts +++ b/scripts/lib/lint-suppressions.ts @@ -28,10 +28,10 @@ const SUPPRESSIONS_FILE_NAMES = [ ]; /** - * The first parent of the merge commit that CI checks a pull request out as, - * which is the branch the work is destined for. + * The branch this work is destined for when there is no merge commit to read it + * from, as there is not when running this locally. */ -const DEFAULT_BASE_REF = 'HEAD^1'; +const FALLBACK_BASE_REF = 'origin/main'; /** * Problems that are knowingly ignored, counted by file and then by the rule or @@ -110,6 +110,37 @@ export function printAddedSuppressions( ); } +/** + * Finds the commit holding the suppressions files to measure against. + * + * CI checks a pull request out as the merge of the branch into its base, so the + * base is simply the first parent, and the files in the working tree already + * have the base's own changes folded in. Elsewhere there is no merge commit, so + * the merge base with the target branch stands in for it. + * + * @param baseRef - The branch this work is destined for. + * @returns The ref to read the baseline from. + */ +async function resolveBaseRef(baseRef: string): Promise { + const { stdout: parents } = await execa( + 'git', + ['rev-list', '--parents', '-n', '1', 'HEAD'], + { cwd: REPO_ROOT }, + ); + + // A merge commit lists two parents alongside its own hash. + if (parents.split(' ').length > 2) { + return 'HEAD^1'; + } + + const { stdout: mergeBase } = await execa( + 'git', + ['merge-base', 'HEAD', baseRef], + { cwd: REPO_ROOT }, + ); + return mergeBase.trim(); +} + /** * Checks that no problems have been added to any of the suppressions files. * @@ -118,18 +149,12 @@ export function printAddedSuppressions( * one rather than fix it. This closes that hatch: measured against the base * branch, these files may only shrink. * - * CI checks a pull request out as the merge of the branch into its base, so the - * base is simply the first parent, and the files in the working tree already - * have the base's own changes folded in. That leaves this to compare two files, - * with no merge base to work out and nothing to fetch. - * - * Pass a ref to compare against something else, which is useful when running - * this outside of CI, where there is no merge commit. + * Pass a branch to measure against one other than `origin/main`. * * @param argv - The arguments passed to this script. */ export async function lintSuppressions(argv: readonly string[]): Promise { - const baseRef = argv[0] ?? DEFAULT_BASE_REF; + const baseRef = await resolveBaseRef(argv[0] ?? FALLBACK_BASE_REF); let didPass = true; for (const fileName of SUPPRESSIONS_FILE_NAMES) { From c78ee097fb317cf4eb06f724e99aa79251519038 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 30 Sep 2026 22:33:27 +0200 Subject: [PATCH 13/25] chore: assert the git calls without indexing them --- scripts/lib/lint-suppressions.test.ts | 30 ++++++++++++++------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/scripts/lib/lint-suppressions.test.ts b/scripts/lib/lint-suppressions.test.ts index 4fdceb99bd4..5375c46d69d 100644 --- a/scripts/lib/lint-suppressions.test.ts +++ b/scripts/lib/lint-suppressions.test.ts @@ -169,15 +169,16 @@ describe('lintSuppressions', () => { await lintSuppressions([]); - expect( - jest - .mocked(execa) - .mock.calls.map((call) => call[1]) - .filter((args) => args[0] === 'show'), - ).toStrictEqual([ + expect(execa).toHaveBeenCalledWith( + 'git', ['show', 'HEAD^1:oxlint-suppressions.json'], + expect.anything(), + ); + expect(execa).toHaveBeenCalledWith( + 'git', ['show', 'HEAD^1:tsc-suppressions.json'], - ]); + expect.anything(), + ); }); it('falls back to the merge base where there is no merge commit, as when run locally', async () => { @@ -190,15 +191,16 @@ describe('lintSuppressions', () => { ['merge-base', 'HEAD', 'origin/main'], expect.anything(), ); - expect( - jest - .mocked(execa) - .mock.calls.map((call) => call[1]) - .filter((args) => args[0] === 'show'), - ).toStrictEqual([ + expect(execa).toHaveBeenCalledWith( + 'git', ['show', 'abc123:oxlint-suppressions.json'], + expect.anything(), + ); + expect(execa).toHaveBeenCalledWith( + 'git', ['show', 'abc123:tsc-suppressions.json'], - ]); + expect.anything(), + ); }); it('takes the merge base against the branch it is given', async () => { From 9b97e79b53f5ecc274e4cfeee94086e373d4307b Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 30 Sep 2026 22:46:08 +0200 Subject: [PATCH 14/25] fix: only trust the first parent in CI CI checks a pull request out as the merge of the branch into its base, so the first parent is the base. A merge commit made by hand is the other way round, its first parent being the branch, so merging main into a branch locally had the check measure against the branch's own tip and miss what it had added. --- scripts/lib/lint-suppressions.test.ts | 36 +++++++++------------------ scripts/lib/lint-suppressions.ts | 29 ++++++++------------- 2 files changed, 23 insertions(+), 42 deletions(-) diff --git a/scripts/lib/lint-suppressions.test.ts b/scripts/lib/lint-suppressions.test.ts index 5375c46d69d..7b7e2c977ef 100644 --- a/scripts/lib/lint-suppressions.test.ts +++ b/scripts/lib/lint-suppressions.test.ts @@ -120,26 +120,13 @@ describe('printAddedSuppressions', () => { /** * Stubs the Git commands the check runs. * - * @param args - The arguments to this function. - * @param args.isMergeCommit - Whether HEAD should look like a merge commit. - * @param args.suppressions - The baseline the commands should produce. + * @param suppressions - The baseline the commands should produce. */ -function mockGit({ - isMergeCommit, - suppressions = '{}', -}: { - isMergeCommit: boolean; - suppressions?: string; -}): void { +function mockGit(suppressions = '{}'): void { jest.mocked(execa).mockImplementation((async ( _file: string, args: string[], ) => { - if (args[0] === 'rev-list') { - return { - stdout: isMergeCommit ? 'head parentA parentB' : 'head parentA', - }; - } if (args[0] === 'merge-base') { return { stdout: 'abc123\n' }; } @@ -153,10 +140,15 @@ describe('lintSuppressions', () => { beforeEach(() => { originalProcess = globalThis.process; // The exit code is reset because it is global state that another test file - // may have set. - globalThis.process = { ...globalThis.process, exitCode: undefined }; + // may have set, and `GITHUB_ACTIONS` because this suite itself runs in CI, + // where it would otherwise be set for every test. + globalThis.process = { + ...globalThis.process, + exitCode: undefined, + env: { ...globalThis.process.env, GITHUB_ACTIONS: undefined }, + }; jest.spyOn(console, 'log').mockReturnValue(undefined); - mockGit({ isMergeCommit: true }); + mockGit(); jest.mocked(tscSuppressions.readSuppressions).mockResolvedValue({}); }); @@ -165,7 +157,7 @@ describe('lintSuppressions', () => { }); it('reads the baseline from the first parent of the merge commit CI checks out', async () => { - mockGit({ isMergeCommit: true }); + process.env.GITHUB_ACTIONS = 'true'; await lintSuppressions([]); @@ -181,9 +173,7 @@ describe('lintSuppressions', () => { ); }); - it('falls back to the merge base where there is no merge commit, as when run locally', async () => { - mockGit({ isMergeCommit: false }); - + it('takes the merge base when running outside of CI, where a merge commit means something else', async () => { await lintSuppressions([]); expect(jest.mocked(execa)).toHaveBeenCalledWith( @@ -204,8 +194,6 @@ describe('lintSuppressions', () => { }); it('takes the merge base against the branch it is given', async () => { - mockGit({ isMergeCommit: false }); - await lintSuppressions(['origin/release']); expect(jest.mocked(execa)).toHaveBeenCalledWith( diff --git a/scripts/lib/lint-suppressions.ts b/scripts/lib/lint-suppressions.ts index 41c6c474360..2b40d0b0411 100644 --- a/scripts/lib/lint-suppressions.ts +++ b/scripts/lib/lint-suppressions.ts @@ -114,31 +114,24 @@ export function printAddedSuppressions( * Finds the commit holding the suppressions files to measure against. * * CI checks a pull request out as the merge of the branch into its base, so the - * base is simply the first parent, and the files in the working tree already - * have the base's own changes folded in. Elsewhere there is no merge commit, so + * base is its first parent, and the files in the working tree already have the + * base's own changes folded in. A merge commit made by hand is the other way + * round, its first parent being the branch, so the shape of the commit is not + * enough to go on and only CI's checkout is taken at face value. Anywhere else * the merge base with the target branch stands in for it. * - * @param baseRef - The branch this work is destined for. + * @param targetRef - The branch this work is destined for. * @returns The ref to read the baseline from. */ -async function resolveBaseRef(baseRef: string): Promise { - const { stdout: parents } = await execa( - 'git', - ['rev-list', '--parents', '-n', '1', 'HEAD'], - { cwd: REPO_ROOT }, - ); - - // A merge commit lists two parents alongside its own hash. - if (parents.split(' ').length > 2) { +async function resolveBaseRef(targetRef: string): Promise { + if (process.env.GITHUB_ACTIONS === 'true') { return 'HEAD^1'; } - const { stdout: mergeBase } = await execa( - 'git', - ['merge-base', 'HEAD', baseRef], - { cwd: REPO_ROOT }, - ); - return mergeBase.trim(); + const { stdout } = await execa('git', ['merge-base', 'HEAD', targetRef], { + cwd: REPO_ROOT, + }); + return stdout.trim(); } /** From 131f07e29d3a85e5fa99c4d5cfcb373e27f5c93f Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 30 Sep 2026 22:48:59 +0200 Subject: [PATCH 15/25] chore: reword the comment about resetting global state --- scripts/lib/lint-suppressions.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/lib/lint-suppressions.test.ts b/scripts/lib/lint-suppressions.test.ts index 7b7e2c977ef..e951e31e254 100644 --- a/scripts/lib/lint-suppressions.test.ts +++ b/scripts/lib/lint-suppressions.test.ts @@ -139,9 +139,9 @@ describe('lintSuppressions', () => { beforeEach(() => { originalProcess = globalThis.process; - // The exit code is reset because it is global state that another test file - // may have set, and `GITHUB_ACTIONS` because this suite itself runs in CI, - // where it would otherwise be set for every test. + // The exit code is reset because another test file may have set it. + // `GITHUB_ACTIONS` is cleared because this suite runs in CI, where it is + // set, which would otherwise send every test down the CI path. globalThis.process = { ...globalThis.process, exitCode: undefined, From 39074d137b5da8a71cf0aa81a99b69b559000f28 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 1 Oct 2026 10:51:14 +0200 Subject: [PATCH 16/25] feat: let a label waive the suppressions check Adding a suppression is sometimes the right call, updating the Oxlint configs being the example, so the check can be waived with the allow-new-suppressions label. It still reports what was added either way, leaving the addition in the diff and the waiver in the pull request's history. It now runs on pull requests only, as the merge queue has neither the merge commit nor the label in reach. --- .github/workflows/lint-build-test.yml | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index 6ecab983fd8..1fe5ca9d5bb 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -168,7 +168,9 @@ jobs: lint-suppressions: name: Lint (lint:suppressions) - if: github.event_name == 'pull_request' || github.event_name == 'merge_group' + # Only on pull requests, where the branch is checked out as the merge into + # its base and the label that waives the check is in reach. + if: github.event_name == 'pull_request' runs-on: ubuntu-latest strategy: matrix: @@ -182,7 +184,20 @@ jobs: node-version: ${{ matrix.node-version }} fetch-depth: 2 - name: Run yarn lint:suppressions - run: yarn lint:suppressions + shell: bash + run: | + if yarn lint:suppressions; then + exit 0 + fi + if [[ "$IS_WAIVED" == 'true' ]]; then + echo "::warning::Suppressions were added, which the '$WAIVER_LABEL' label allows." + exit 0 + fi + echo "Apply the '$WAIVER_LABEL' label if these suppressions have to be added." + exit 1 + env: + WAIVER_LABEL: allow-new-suppressions + IS_WAIVED: ${{ contains(github.event.pull_request.labels.*.name, 'allow-new-suppressions') }} - name: Require clean working directory shell: bash run: | From d888d1a22572b35dfdd994b0ff945f818446f499 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 1 Oct 2026 11:09:58 +0200 Subject: [PATCH 17/25] fix: read the waiver label back over the API A label applied after the check fails was invisible to it: the pull_request trigger does not fire on labeled, and a re-run replays the original event payload. Reading the labels back over the API means applying the label and re-running the job now works. --- .github/workflows/lint-build-test.yml | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index 1fe5ca9d5bb..942f3d859ea 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -168,10 +168,11 @@ jobs: lint-suppressions: name: Lint (lint:suppressions) - # Only on pull requests, where the branch is checked out as the merge into - # its base and the label that waives the check is in reach. if: github.event_name == 'pull_request' runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read strategy: matrix: node-version: [24.x] @@ -189,15 +190,19 @@ jobs: if yarn lint:suppressions; then exit 0 fi - if [[ "$IS_WAIVED" == 'true' ]]; then + # The labels are read back over the API rather than taken from the + # event, which is fixed when the run starts and replayed as it was on + # a re-run, so that applying the label and re-running this job works. + if gh api "repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/labels" --jq '.[].name' | grep --line-regexp --quiet "$WAIVER_LABEL"; then echo "::warning::Suppressions were added, which the '$WAIVER_LABEL' label allows." exit 0 fi - echo "Apply the '$WAIVER_LABEL' label if these suppressions have to be added." + echo "Apply the '$WAIVER_LABEL' label and re-run this job if these suppressions have to be added." exit 1 env: WAIVER_LABEL: allow-new-suppressions - IS_WAIVED: ${{ contains(github.event.pull_request.labels.*.name, 'allow-new-suppressions') }} + PR_NUMBER: ${{ github.event.pull_request.number }} + GH_TOKEN: ${{ github.token }} - name: Require clean working directory shell: bash run: | From 18da785db819ee7bda09be96d7a877c288d7254d Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 1 Oct 2026 11:14:42 +0200 Subject: [PATCH 18/25] refactor: move the suppressions check to its own workflow Reading the waiver label off the event payload means a label applied after the run started is invisible, and a re-run replays the original payload. The other label driven checks here solve that by firing a fresh run on labeled and unlabeled, which needs a trigger of its own. The job used nothing from lint-build-test.yml anyway, so it moves out and picks up the same trigger. --- .github/workflows/lint-build-test.yml | 45 --------------------- .github/workflows/lint-suppressions.yml | 53 +++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 45 deletions(-) create mode 100644 .github/workflows/lint-suppressions.yml diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index 942f3d859ea..29af55a6f66 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -166,51 +166,6 @@ jobs: exit 1 fi - lint-suppressions: - name: Lint (lint:suppressions) - if: github.event_name == 'pull_request' - runs-on: ubuntu-latest - permissions: - contents: read - pull-requests: read - strategy: - matrix: - node-version: [24.x] - steps: - - name: Checkout and setup environment - uses: MetaMask/action-checkout-and-setup@v3 - with: - is-high-risk-environment: false - persist-credentials: false - node-version: ${{ matrix.node-version }} - fetch-depth: 2 - - name: Run yarn lint:suppressions - shell: bash - run: | - if yarn lint:suppressions; then - exit 0 - fi - # The labels are read back over the API rather than taken from the - # event, which is fixed when the run starts and replayed as it was on - # a re-run, so that applying the label and re-running this job works. - if gh api "repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/labels" --jq '.[].name' | grep --line-regexp --quiet "$WAIVER_LABEL"; then - echo "::warning::Suppressions were added, which the '$WAIVER_LABEL' label allows." - exit 0 - fi - echo "Apply the '$WAIVER_LABEL' label and re-run this job if these suppressions have to be added." - exit 1 - env: - WAIVER_LABEL: allow-new-suppressions - PR_NUMBER: ${{ github.event.pull_request.number }} - GH_TOKEN: ${{ github.token }} - - name: Require clean working directory - shell: bash - run: | - if ! git diff --exit-code; then - echo "Working tree dirty at end of job" - exit 1 - fi - validate-changelog-diffs: name: Validate changelog diffs if: github.event_name == 'pull_request' || github.event_name == 'merge_group' diff --git a/.github/workflows/lint-suppressions.yml b/.github/workflows/lint-suppressions.yml new file mode 100644 index 00000000000..2cc7e83bf67 --- /dev/null +++ b/.github/workflows/lint-suppressions.yml @@ -0,0 +1,53 @@ +name: Lint Suppressions + +on: + pull_request: + # `labeled` and `unlabeled` are here so that applying the waiver label + # starts a fresh run, whose payload then carries the label. A run already + # finished keeps the labels it started with, even when it is re-run. + types: [opened, synchronize, labeled, unlabeled] + merge_group: + +permissions: + contents: read + +jobs: + lint-suppressions: + name: Lint suppressions + # The branch is only checked out as the merge into its base, which is where + # the check reads its baseline from, on a pull request. + if: ${{ github.event_name != 'merge_group' }} + runs-on: ubuntu-latest + strategy: + matrix: + node-version: [24.x] + steps: + - name: Checkout and setup environment + uses: MetaMask/action-checkout-and-setup@v3 + with: + is-high-risk-environment: false + persist-credentials: false + node-version: ${{ matrix.node-version }} + fetch-depth: 2 + - name: Run yarn lint:suppressions + shell: bash + run: | + if yarn lint:suppressions; then + exit 0 + fi + if [[ "$IS_WAIVED" == 'true' ]]; then + echo "::warning::Suppressions were added, which the '$WAIVER_LABEL' label allows." + exit 0 + fi + echo "Apply the '$WAIVER_LABEL' label if these suppressions have to be added." + exit 1 + env: + WAIVER_LABEL: allow-new-suppressions + IS_WAIVED: ${{ contains(github.event.pull_request.labels.*.name, 'allow-new-suppressions') }} + - name: Require clean working directory + shell: bash + run: | + if ! git diff --exit-code; then + echo "Working tree dirty at end of job" + exit 1 + fi From bc5d8d81a861110f1c4981a2d4b97a1453e83543 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 1 Oct 2026 11:27:06 +0200 Subject: [PATCH 19/25] chore: drop a comment --- .github/workflows/lint-suppressions.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.github/workflows/lint-suppressions.yml b/.github/workflows/lint-suppressions.yml index 2cc7e83bf67..9d99a164229 100644 --- a/.github/workflows/lint-suppressions.yml +++ b/.github/workflows/lint-suppressions.yml @@ -2,9 +2,6 @@ name: Lint Suppressions on: pull_request: - # `labeled` and `unlabeled` are here so that applying the waiver label - # starts a fresh run, whose payload then carries the label. A run already - # finished keeps the labels it started with, even when it is re-run. types: [opened, synchronize, labeled, unlabeled] merge_group: From 19e0119843366a90d75e940b85836c636ddf61a4 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 1 Oct 2026 11:27:28 +0200 Subject: [PATCH 20/25] chore: drop a comment --- .github/workflows/lint-suppressions.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/lint-suppressions.yml b/.github/workflows/lint-suppressions.yml index 9d99a164229..5cf5fad0402 100644 --- a/.github/workflows/lint-suppressions.yml +++ b/.github/workflows/lint-suppressions.yml @@ -11,8 +11,6 @@ permissions: jobs: lint-suppressions: name: Lint suppressions - # The branch is only checked out as the merge into its base, which is where - # the check reads its baseline from, on a pull request. if: ${{ github.event_name != 'merge_group' }} runs-on: ubuntu-latest strategy: From d5f622db603d50f8d5aca80fe51782089e75dd2f Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 1 Oct 2026 11:30:04 +0200 Subject: [PATCH 21/25] feat: give core platform ownership of the suppressions files A label anyone can apply is not much of a gate. CODEOWNERS is, so the two suppressions files now need a core platform review to change at all. The label stays as the way to turn CI green, but on its own it no longer gets anything merged. --- .github/CODEOWNERS | 2 ++ .github/workflows/lint-suppressions.yml | 2 +- codeowners.ts | 5 +++++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 50ed0ed16cc..ca901240874 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -760,5 +760,7 @@ # --------- /.github/ @MetaMask/core-platform +/oxlint-suppressions.json @MetaMask/core-platform +/tsc-suppressions.json @MetaMask/core-platform /packages/eth-json-rpc-middleware/src/methods @MetaMask/confirmations @MetaMask/core-platform /packages/eth-json-rpc-middleware/src/wallet.* @MetaMask/confirmations @MetaMask/core-platform \ No newline at end of file diff --git a/.github/workflows/lint-suppressions.yml b/.github/workflows/lint-suppressions.yml index 5cf5fad0402..6a9149447ab 100644 --- a/.github/workflows/lint-suppressions.yml +++ b/.github/workflows/lint-suppressions.yml @@ -34,7 +34,7 @@ jobs: echo "::warning::Suppressions were added, which the '$WAIVER_LABEL' label allows." exit 0 fi - echo "Apply the '$WAIVER_LABEL' label if these suppressions have to be added." + echo "These files are owned by @MetaMask/core-platform, who can apply the '$WAIVER_LABEL' label if the suppressions have to be added." exit 1 env: WAIVER_LABEL: allow-new-suppressions diff --git a/codeowners.ts b/codeowners.ts index c660cbe15a3..abb0d4a7e19 100644 --- a/codeowners.ts +++ b/codeowners.ts @@ -385,6 +385,11 @@ const config = { }, overrides: [ { pattern: '/.github/', owners: ['@MetaMask/core-platform'] }, + { + pattern: '/oxlint-suppressions.json', + owners: ['@MetaMask/core-platform'], + }, + { pattern: '/tsc-suppressions.json', owners: ['@MetaMask/core-platform'] }, { pattern: '/packages/eth-json-rpc-middleware/src/methods', owners: ['@MetaMask/confirmations', '@MetaMask/core-platform'], From dd64190bf9c37a30b9de9e87edcac6928c94adf5 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 1 Oct 2026 11:32:58 +0200 Subject: [PATCH 22/25] chore: stop pointing everyone at the waiver label Reverts the CODEOWNERS change, which would have made removals need a core platform review too, and those are the ones we want to be easy. The failure now just points at core platform instead of telling whoever hits it which label turns the check green. --- .github/CODEOWNERS | 2 -- .github/workflows/lint-suppressions.yml | 2 +- codeowners.ts | 5 ----- 3 files changed, 1 insertion(+), 8 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index ca901240874..50ed0ed16cc 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -760,7 +760,5 @@ # --------- /.github/ @MetaMask/core-platform -/oxlint-suppressions.json @MetaMask/core-platform -/tsc-suppressions.json @MetaMask/core-platform /packages/eth-json-rpc-middleware/src/methods @MetaMask/confirmations @MetaMask/core-platform /packages/eth-json-rpc-middleware/src/wallet.* @MetaMask/confirmations @MetaMask/core-platform \ No newline at end of file diff --git a/.github/workflows/lint-suppressions.yml b/.github/workflows/lint-suppressions.yml index 6a9149447ab..93e01caf4bc 100644 --- a/.github/workflows/lint-suppressions.yml +++ b/.github/workflows/lint-suppressions.yml @@ -34,7 +34,7 @@ jobs: echo "::warning::Suppressions were added, which the '$WAIVER_LABEL' label allows." exit 0 fi - echo "These files are owned by @MetaMask/core-platform, who can apply the '$WAIVER_LABEL' label if the suppressions have to be added." + echo "Reach out to @MetaMask/core-platform if these suppressions have to be added." exit 1 env: WAIVER_LABEL: allow-new-suppressions diff --git a/codeowners.ts b/codeowners.ts index abb0d4a7e19..c660cbe15a3 100644 --- a/codeowners.ts +++ b/codeowners.ts @@ -385,11 +385,6 @@ const config = { }, overrides: [ { pattern: '/.github/', owners: ['@MetaMask/core-platform'] }, - { - pattern: '/oxlint-suppressions.json', - owners: ['@MetaMask/core-platform'], - }, - { pattern: '/tsc-suppressions.json', owners: ['@MetaMask/core-platform'] }, { pattern: '/packages/eth-json-rpc-middleware/src/methods', owners: ['@MetaMask/confirmations', '@MetaMask/core-platform'], From 22907bfa891548b8554cab275704b35bcd41087f Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 1 Oct 2026 11:34:09 +0200 Subject: [PATCH 23/25] chore: let the check speak for itself on failure --- .github/workflows/lint-suppressions.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/lint-suppressions.yml b/.github/workflows/lint-suppressions.yml index 93e01caf4bc..b5cf50553f0 100644 --- a/.github/workflows/lint-suppressions.yml +++ b/.github/workflows/lint-suppressions.yml @@ -34,7 +34,6 @@ jobs: echo "::warning::Suppressions were added, which the '$WAIVER_LABEL' label allows." exit 0 fi - echo "Reach out to @MetaMask/core-platform if these suppressions have to be added." exit 1 env: WAIVER_LABEL: allow-new-suppressions From 2f63e00f405a1dd8062c6b12c15e0aa72bc6af21 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 1 Oct 2026 11:40:11 +0200 Subject: [PATCH 24/25] chore: check the waiver label before running the check Running it first only told us what was waived, which the failing run before the label went on already did. Checking the label first reads simpler, and this is all going to be replaced by core approval anyway. --- .github/workflows/lint-suppressions.yml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/.github/workflows/lint-suppressions.yml b/.github/workflows/lint-suppressions.yml index b5cf50553f0..84182a8093e 100644 --- a/.github/workflows/lint-suppressions.yml +++ b/.github/workflows/lint-suppressions.yml @@ -27,14 +27,11 @@ jobs: - name: Run yarn lint:suppressions shell: bash run: | - if yarn lint:suppressions; then - exit 0 - fi if [[ "$IS_WAIVED" == 'true' ]]; then - echo "::warning::Suppressions were added, which the '$WAIVER_LABEL' label allows." + echo "::warning::Skipped, as the '$WAIVER_LABEL' label allows suppressions to be added." exit 0 fi - exit 1 + yarn lint:suppressions env: WAIVER_LABEL: allow-new-suppressions IS_WAIVED: ${{ contains(github.event.pull_request.labels.*.name, 'allow-new-suppressions') }} From 76171d763521c6dee7c45950af4374b615782322 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 1 Oct 2026 11:40:58 +0200 Subject: [PATCH 25/25] chore: guard the whole job with the waiver label No point checking out and installing just to decide we are not going to run the check. --- .github/workflows/lint-suppressions.yml | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/.github/workflows/lint-suppressions.yml b/.github/workflows/lint-suppressions.yml index 84182a8093e..9c75ffe25b0 100644 --- a/.github/workflows/lint-suppressions.yml +++ b/.github/workflows/lint-suppressions.yml @@ -11,7 +11,7 @@ permissions: jobs: lint-suppressions: name: Lint suppressions - if: ${{ github.event_name != 'merge_group' }} + if: ${{ github.event_name != 'merge_group' && !contains(github.event.pull_request.labels.*.name, 'allow-new-suppressions') }} runs-on: ubuntu-latest strategy: matrix: @@ -25,16 +25,7 @@ jobs: node-version: ${{ matrix.node-version }} fetch-depth: 2 - name: Run yarn lint:suppressions - shell: bash - run: | - if [[ "$IS_WAIVED" == 'true' ]]; then - echo "::warning::Skipped, as the '$WAIVER_LABEL' label allows suppressions to be added." - exit 0 - fi - yarn lint:suppressions - env: - WAIVER_LABEL: allow-new-suppressions - IS_WAIVED: ${{ contains(github.event.pull_request.labels.*.name, 'allow-new-suppressions') }} + run: yarn lint:suppressions - name: Require clean working directory shell: bash run: |