diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index bfb5e3064c..293a31ca93 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -55,8 +55,10 @@ jobs: # Whole-repo escape-hatch ratchet — runs unconditionally (not via `nx # affected`, and not via `make ci-lint`, which this job no longer calls), # so the gate can't be silently skipped for a PR that touches few projects. - - name: Escape-hatch ratchet tests - run: node --test scripts/ci/__tests__/ratchet.test.mjs + - name: Escape-hatch ratchet and ESLint wrapper tests + run: + node --test scripts/ci/__tests__/ratchet.test.mjs + scripts/ci/__tests__/eslint-warnings.test.mjs # Also unconditional, and against the PR's own tree. claude-code-review.yml # runs this file too, but from a base-branch checkout, so a PR that breaks # what it pins (the AGENTS.md heading every review footer links to) passes diff --git a/knip.json b/knip.json index 7d88c3d288..40740423bb 100644 --- a/knip.json +++ b/knip.json @@ -2,7 +2,10 @@ "$schema": "https://unpkg.com/knip@6/schema.json", "workspaces": { ".": { - "entry": ["scripts/__tests__/*.test.mjs"] + "entry": [ + "scripts/__tests__/*.test.mjs", + "scripts/ci/__tests__/*.test.mjs" + ] }, "packages/app": { "entry": ["pages/**/*.{ts,tsx}", "scripts/*.js", "tests/e2e/**/*.ts"], diff --git a/nx.json b/nx.json index 88bf780a5f..a0cba57eee 100644 --- a/nx.json +++ b/nx.json @@ -24,7 +24,13 @@ "ci:lint": { "cache": true, "dependsOn": ["^ci:build"], - "inputs": ["default", "^production", { "runtime": "node --version" }] + "inputs": [ + "default", + "^production", + // Each package's `lint` runs ESLint through this wrapper. + "{workspaceRoot}/scripts/ci/eslint-warnings.mjs", + { "runtime": "node --version" } + ] }, // Unit tests import the compiled `dist` of workspace dependencies (e.g. // `@hyperdx/common-utils/dist/types`), so they need the same `^ci:build` diff --git a/package.json b/package.json index cdcdf52b50..532bd58308 100644 --- a/package.json +++ b/package.json @@ -60,12 +60,12 @@ "lint-staged": { "packages/api/src/{routers/external-api/**/*.ts,utils/swagger.ts}": [ "prettier --write --ignore-unknown", - "eslint --flag v10_config_lookup_from_file --fix --quiet", + "node scripts/ci/eslint-warnings.mjs --staged --flag v10_config_lookup_from_file --fix", "sh -c 'cd packages/api && yarn run lint:openapi && git add openapi.json'" ], "**/*.{ts,tsx}": [ "prettier --write --ignore-unknown", - "eslint --flag v10_config_lookup_from_file --fix --quiet" + "node scripts/ci/eslint-warnings.mjs --staged --flag v10_config_lookup_from_file --fix" ], "**/*.{mdx,json,yml}": [ "prettier --write --ignore-unknown" diff --git a/packages/api/package.json b/packages/api/package.json index 0b8f2e9fe1..e360295a72 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -107,7 +107,7 @@ "dev-task": "DOTENV_CONFIG_PATH=.env.development nodemon --exec 'ts-node' --transpile-only -r tsconfig-paths/register -r dotenv-expand/config -r ./src/telemetry.ts ./src/tasks/index.ts", "build": "rimraf ./build && tsc -p tsconfig.build.json && tsc-alias -p tsconfig.build.json && cp -r ./src/opamp/proto ./build/opamp/", "build:vercel": "rimraf ./build && tsc -p tsconfig.vercel.json && tsc-alias -p tsconfig.vercel.json && cp -r ./src/opamp/proto ./build/opamp/", - "lint": "npx eslint . --ext .ts --max-warnings 302", + "lint": "node ../../scripts/ci/eslint-warnings.mjs . --ext .ts --max-warnings 302", "lint:fix": "npx eslint . --ext .ts --fix", "ci:lint": "yarn lint && yarn tsc --noEmit && yarn lint:openapi", "ci:unit": "jest --ci --coverage", diff --git a/packages/app/package.json b/packages/app/package.json index 101e226cce..1e4744e274 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -14,7 +14,7 @@ "build:clickhouse": "NEXT_PUBLIC_IS_LOCAL_MODE=true NEXT_PUBLIC_CLICKHOUSE_BUILD=true next build --webpack && node scripts/prepare-clickhouse-build-export.js", "run:clickhouse": "test -d out && npx rimraf tmp && mkdir tmp && cp -r out tmp/clickstack && echo 'visit http://localhost:3000/clickstack to start' && npx serve tmp -l 3000 || echo 'run build:clickhouse first'", "start": "next start", - "lint": "npx eslint . --ext .ts,.tsx --max-warnings 568", + "lint": "node ../../scripts/ci/eslint-warnings.mjs . --ext .ts,.tsx --max-warnings 568", "lint:fix": "npx eslint . --ext .ts,.tsx --fix", "lint:styles": "stylelint **/*/*.{css,scss}", "ci:lint": "yarn lint && yarn tsc --noEmit && yarn lint:styles --quiet", diff --git a/packages/cli/package.json b/packages/cli/package.json index 24d75c8850..d186c9ee1a 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -33,7 +33,7 @@ "compile:linux": "bun build src/cli.tsx --compile --target=bun-linux-x64 --outfile dist/hdx-linux-x64", "compile:macos": "bun build src/cli.tsx --compile --target=bun-darwin-arm64 --outfile dist/hdx-darwin-arm64", "compile:macos-x64": "bun build src/cli.tsx --compile --target=bun-darwin-x64 --outfile dist/hdx-darwin-x64", - "lint": "npx eslint . --ext .ts,.tsx --max-warnings 0", + "lint": "node ../../scripts/ci/eslint-warnings.mjs . --ext .ts,.tsx --max-warnings 0", "ci:build": "tsup", "ci:lint": "yarn lint && yarn tsc --noEmit", "ci:unit": "NODE_OPTIONS=--experimental-vm-modules jest --runInBand --ci --passWithNoTests", diff --git a/packages/common-utils/package.json b/packages/common-utils/package.json index fb650e0b2d..ca74a0d547 100644 --- a/packages/common-utils/package.json +++ b/packages/common-utils/package.json @@ -51,7 +51,7 @@ "build": "tsup && yarn build:types", "build:types": "tsc -p tsconfig.build.json && tsc-alias -p tsconfig.build.json", "ci:build": "yarn build", - "lint": "npx eslint . --ext .ts --max-warnings 80", + "lint": "node ../../scripts/ci/eslint-warnings.mjs . --ext .ts --max-warnings 80", "lint:fix": "npx eslint . --ext .ts --fix", "ci:lint": "yarn lint && yarn tsc --noEmit", "ci:unit": "jest --ci --coverage", diff --git a/packages/hdx-eval/package.json b/packages/hdx-eval/package.json index c012565af2..15549f6f32 100644 --- a/packages/hdx-eval/package.json +++ b/packages/hdx-eval/package.json @@ -36,7 +36,7 @@ "scripts": { "build": "rimraf ./dist && tsc -p tsconfig.build.json && tsc-alias -p tsconfig.build.json", "dev": "NODE_OPTIONS=--max-old-space-size=8192 ./scripts/env.sh tsx src/cli.ts", - "lint": "npx eslint . --ext .ts --max-warnings 0", + "lint": "node ../../scripts/ci/eslint-warnings.mjs . --ext .ts --max-warnings 0", "lint:fix": "npx eslint . --ext .ts --fix", "ci:lint": "yarn lint && yarn tsc --noEmit", "ci:unit": "jest --ci --coverage --forceExit", diff --git a/scripts/ci/__tests__/eslint-warnings.test.mjs b/scripts/ci/__tests__/eslint-warnings.test.mjs new file mode 100644 index 0000000000..0d99104ec1 --- /dev/null +++ b/scripts/ci/__tests__/eslint-warnings.test.mjs @@ -0,0 +1,115 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { + collectWarnings, + newWarnings, + parseAddedLines, + parseArgs, +} from '../eslint-warnings.mjs'; + +const ROOT = '/repo'; + +test('parseArgs splits wrapper flags from ESLint args', () => { + assert.deepEqual(parseArgs(['.', '--ext', '.ts', '--max-warnings', '302']), { + eslintArgs: ['.', '--ext', '.ts'], + maxWarnings: 302, + staged: false, + }); + assert.deepEqual(parseArgs(['--staged', '--fix', '/repo/a.ts']), { + eslintArgs: ['--fix', '/repo/a.ts'], + maxWarnings: undefined, + staged: true, + }); + assert.equal(parseArgs(['--max-warnings=0']).maxWarnings, 0); + assert.throws(() => parseArgs(['--max-warnings', 'lots'])); +}); + +test('parseAddedLines maps new-side hunk ranges per file', () => { + const diff = [ + 'diff --git a/packages/api/src/a.ts b/packages/api/src/a.ts', + '--- a/packages/api/src/a.ts', + '+++ b/packages/api/src/a.ts', + '@@ -3 +3 @@ x', + '-old', + '+new', + '@@ -10,0 +11,3 @@', + '+a', + '+b', + '+c', + '@@ -20,2 +23,0 @@', + '-gone', + '-gone', + 'diff --git a/packages/api/src/deleted.ts b/packages/api/src/deleted.ts', + '--- a/packages/api/src/deleted.ts', + '+++ /dev/null', + '@@ -1,2 +0,0 @@', + '-x', + '-y', + 'diff --git a/packages/app/b.tsx b/packages/app/b.tsx', + 'new file mode 100644', + '--- /dev/null', + '+++ b/packages/app/b.tsx', + '@@ -0,0 +1,2 @@', + '+one', + '+two', + ].join('\n'); + + const added = parseAddedLines(diff, ROOT); + assert.deepEqual( + [...added.get('/repo/packages/api/src/a.ts')], + [3, 11, 12, 13], + ); + assert.deepEqual([...added.get('/repo/packages/app/b.tsx')], [1, 2]); + assert.equal(added.has('/repo/packages/api/src/deleted.ts'), false); +}); + +const results = [ + { + filePath: '/repo/packages/api/src/a.ts', + messages: [ + { severity: 1, line: 3, column: 1, ruleId: 'r1', message: 'new' }, + { severity: 1, line: 40, column: 1, ruleId: 'r1', message: 'old' }, + { severity: 2, line: 3, column: 5, ruleId: 'r2', message: 'error' }, + ], + }, + { + filePath: '/repo/packages/api/src/untouched.ts', + messages: [ + { severity: 1, line: 3, column: 1, ruleId: 'r1', message: 'other' }, + ], + }, +]; + +test('collectWarnings keeps only warnings, tagged with their file', () => { + const warnings = collectWarnings(results); + assert.deepEqual( + warnings.map(w => [w.filePath, w.line, w.message]), + [ + ['/repo/packages/api/src/a.ts', 3, 'new'], + ['/repo/packages/api/src/a.ts', 40, 'old'], + ['/repo/packages/api/src/untouched.ts', 3, 'other'], + ], + ); + assert.equal(warnings[0].raw, results[0].messages[0]); +}); + +test('newWarnings prefers warnings on changed lines', () => { + const added = new Map([['/repo/packages/api/src/a.ts', new Set([3])]]); + const { scope, warnings } = newWarnings(collectWarnings(results), added); + assert.equal(scope, 'lines'); + assert.deepEqual( + warnings.map(w => w.message), + ['new'], + ); +}); + +test('newWarnings falls back to changed files when no changed line warns', () => { + const added = new Map([['/repo/packages/api/src/a.ts', new Set([7])]]); + const { scope, warnings } = newWarnings(collectWarnings(results), added); + assert.equal(scope, 'files'); + assert.deepEqual( + warnings.map(w => w.message), + ['new', 'old'], + ); +}); diff --git a/scripts/ci/eslint-warnings.mjs b/scripts/ci/eslint-warnings.mjs new file mode 100644 index 0000000000..dad9772042 --- /dev/null +++ b/scripts/ci/eslint-warnings.mjs @@ -0,0 +1,277 @@ +#!/usr/bin/env node +/** + * ESLint wrapper that says which warnings are new. + * + * Ceiling mode (each package's `lint` script): + * node ../../scripts/ci/eslint-warnings.mjs . --ext .ts --max-warnings 302 + * Runs ESLint, then, when the warning count exceeds the ceiling, lists the + * warnings on lines this branch changed (vs the merge-base with the PR base or + * origin/main) and emits GitHub annotations for them. Plain + * `eslint --max-warnings` only prints "too many warnings" somewhere inside + * hundreds of pre-existing ones. + * + * Staged mode (lint-staged pre-commit): + * node scripts/ci/eslint-warnings.mjs --staged --fix + * Fails only on warnings on lines the commit adds or changes, so touching a + * file with old warnings doesn't block the commit. + * + * Every other argument is passed through to ESLint. + */ +import { execFileSync, spawnSync } from 'node:child_process'; +import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { createRequire } from 'node:module'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +export function parseArgs(argv) { + const eslintArgs = []; + let maxWarnings; + let staged = false; + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + if (arg === '--staged') { + staged = true; + } else if (arg === '--max-warnings') { + maxWarnings = Number(argv[++i]); + } else if (arg.startsWith('--max-warnings=')) { + maxWarnings = Number(arg.slice('--max-warnings='.length)); + } else { + eslintArgs.push(arg); + } + } + if (maxWarnings !== undefined && !Number.isInteger(maxWarnings)) { + throw new Error('--max-warnings needs an integer'); + } + return { eslintArgs, maxWarnings, staged }; +} + +/** + * Parses `git diff --unified=0` output into absolute path -> Set of line + * numbers that exist on the new side of the diff. + */ +export function parseAddedLines(diff, root) { + const added = new Map(); + let lines; + for (const line of diff.split('\n')) { + if (line.startsWith('+++ ')) { + const target = line.slice(4); + lines = target === '/dev/null' ? undefined : new Set(); + if (lines) + added.set(path.resolve(root, target.replace(/^b\//, '')), lines); + continue; + } + const hunk = /^@@ -\S+ \+(\d+)(?:,(\d+))? @@/.exec(line); + if (hunk && lines) { + const start = Number(hunk[1]); + const count = hunk[2] === undefined ? 1 : Number(hunk[2]); + for (let n = start; n < start + count; n++) lines.add(n); + } + } + return added; +} + +export function collectWarnings(results) { + return results.flatMap(result => + result.messages + .filter(message => message.severity === 1) + .map(message => ({ + ...message, + filePath: result.filePath, + raw: message, + })), + ); +} + +/** + * Warnings on changed lines. When there are none (a type change can make a + * type-aware rule fire on untouched code), falls back to every warning in a + * changed file. `scope` says which one the list is. + */ +export function newWarnings(warnings, addedLines) { + const onLines = warnings.filter(w => addedLines.get(w.filePath)?.has(w.line)); + if (onLines.length) return { scope: 'lines', warnings: onLines }; + return { + scope: 'files', + warnings: warnings.filter(w => addedLines.has(w.filePath)), + }; +} + +function git(args, cwd) { + return execFileSync('git', args, { + cwd, + encoding: 'utf8', + maxBuffer: 256 * 1024 * 1024, + stdio: ['ignore', 'pipe', 'ignore'], + }); +} + +const DIFF_ARGS = [ + 'diff', + '--unified=0', + '--no-color', + '--no-ext-diff', + '--src-prefix=a/', + '--dst-prefix=b/', +]; + +/** Returns { base, addedLines }, or undefined when git can't tell. */ +function changedLines(root, staged) { + try { + // lint-staged hides unstaged edits, so the working tree is the staged + // content plus whatever --fix just wrote. + if (staged) { + return { + base: 'HEAD', + addedLines: parseAddedLines(git([...DIFF_ARGS, 'HEAD'], root), root), + }; + } + const baseRef = `origin/${process.env.GITHUB_BASE_REF || 'main'}`; + const mergeBase = git(['merge-base', 'HEAD', baseRef], root).trim(); + return { + base: baseRef, + addedLines: parseAddedLines(git([...DIFF_ARGS, mergeBase], root), root), + }; + } catch { + return undefined; + } +} + +function formatWarning(w, root) { + const where = `${path.relative(root, w.filePath)}:${w.line}:${w.column}`; + return ` ${where} ${w.message} ${w.ruleId ?? ''}`.trimEnd(); +} + +function annotate(warnings, root) { + if (process.env.GITHUB_ACTIONS !== 'true') return; + for (const w of warnings) { + const file = path.relative(root, w.filePath); + const message = `${w.message} (${w.ruleId})`.replace(/\r?\n/g, ' '); + console.log( + `::error file=${file},line=${w.line},col=${w.column},title=New ESLint warning::${message}`, + ); + } +} + +function runEslint(cwd, eslintArgs) { + const require = createRequire(path.join(cwd, 'noop.js')); + const pkgJson = require.resolve('eslint/package.json'); + const bin = path.join(path.dirname(pkgJson), 'bin/eslint.js'); + const tmp = mkdtempSync(path.join(tmpdir(), 'eslint-warnings-')); + const outFile = path.join(tmp, 'results.json'); + try { + const { status } = spawnSync( + process.execPath, + [bin, ...eslintArgs, '--format', 'json', '--output-file', outFile], + { cwd, stdio: 'inherit' }, + ); + let results; + try { + results = JSON.parse(readFileSync(outFile, 'utf8')); + } catch { + // ESLint crashed (bad config, bad flag) and has already said why. + process.exit(status || 2); + } + return { status, results, ESLint: require('eslint').ESLint }; + } finally { + rmSync(tmp, { recursive: true, force: true }); + } +} + +/** Keeps errors plus the given warnings, recomputing the counts. */ +function onlyErrorsAnd(results, kept) { + return results.map(result => { + const messages = result.messages.filter( + m => m.severity === 2 || kept.has(m), + ); + return { + ...result, + messages, + errorCount: messages.filter(m => m.severity === 2).length, + warningCount: messages.filter(m => m.severity === 1).length, + }; + }); +} + +async function main() { + const root = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '../..', + ); + const cwd = process.cwd(); + const { eslintArgs, maxWarnings, staged } = parseArgs(process.argv.slice(2)); + const { status, results, ESLint } = runEslint(cwd, eslintArgs); + const stylish = await new ESLint({ cwd }).loadFormatter('stylish'); + const warnings = collectWarnings(results); + const hasErrors = results.some(r => r.errorCount > 0) || status === 2; + + if (staged) { + const changed = changedLines(root, true); + const fresh = changed + ? warnings.filter(w => changed.addedLines.get(w.filePath)?.has(w.line)) + : []; + process.stdout.write( + await stylish.format( + onlyErrorsAnd(results, new Set(fresh.map(w => w.raw))), + ), + ); + if (fresh.length) { + console.error( + `\nāœ– ${fresh.length} new ESLint warning(s) on lines this commit changes. ` + + 'Fix them before committing; CI caps the warning count per package.', + ); + } + return hasErrors || fresh.length ? 1 : 0; + } + + process.stdout.write(await stylish.format(results)); + if (maxWarnings === undefined || warnings.length <= maxWarnings) { + if (maxWarnings !== undefined && warnings.length < maxWarnings) { + console.log( + `${warnings.length} ESLint warnings, ceiling ${maxWarnings}. ` + + `Lower --max-warnings to ${warnings.length} to lock in the improvement.`, + ); + } + return hasErrors ? 1 : 0; + } + + const pkg = path.relative(root, cwd) || '.'; + const summary = + `${pkg}: ${warnings.length} ESLint warnings, ceiling is ${maxWarnings} ` + + `(--max-warnings in ${path.join(pkg, 'package.json')}).`; + console.error(`\nāœ– ${summary}`); + const changed = changedLines(root, false); + if (!changed) { + console.error( + 'Could not diff against the base branch to find new warnings.', + ); + } else { + const { scope, warnings: fresh } = newWarnings( + warnings, + changed.addedLines, + ); + if (fresh.length) { + console.error( + scope === 'lines' + ? `Warnings on lines changed since ${changed.base}:` + : `No warnings on changed lines (a type change can flag untouched code). Warnings in files changed since ${changed.base}:`, + ); + for (const w of fresh) console.error(formatWarning(w, root)); + annotate(fresh, root); + } else { + console.error(`No warnings in files changed since ${changed.base}.`); + } + } + console.error( + 'Fix the new warnings. Raise the ceiling only if the increase is deliberate.', + ); + if (process.env.GITHUB_ACTIONS === 'true') { + console.log(`::error title=ESLint warning ceiling exceeded::${summary}`); + } + return 1; +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + // Not process.exit(): it drops stdout still queued for a pipe (CI, nx). + process.exitCode = await main(); +}