From 02e1ca2a3acdb2aafee9932e7460b85b66fa7d51 Mon Sep 17 00:00:00 2001 From: Kian Bazza Date: Tue, 29 Sep 2026 11:04:51 -0400 Subject: [PATCH] ci(lint): report `oxlint` exceptions on every run Adds `bun run lint:exceptions`, which lists every allowlisted file by rule, the rules turned off by pattern, and every inline lint exception with its reason. CI runs it after oxlint and adds the list to the job summary. It fails only on a malformed exception, which catches a bare `oxlint-disable` that has silenced the rule meant to report it. --- .github/workflows/ci.yml | 6 + package.json | 1 + packages/react/AGENTS.md | 2 + tooling/lint/harness.ts | 13 +- tooling/lint/report-exceptions.test.ts | 242 +++++++++++++++++++++++++ tooling/lint/report-exceptions.ts | 216 ++++++++++++++++++++++ tooling/lint/source-text.ts | 161 ++++++++++++++++ 7 files changed, 634 insertions(+), 7 deletions(-) create mode 100644 tooling/lint/report-exceptions.test.ts create mode 100644 tooling/lint/report-exceptions.ts create mode 100644 tooling/lint/source-text.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 61cc12ed..161fc1b0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,6 +44,12 @@ jobs: - name: Lint parts in packages/react (oxlint) run: bun run lint:oxlint + # Lists every allowlisted file and inline exception (also in the job + # summary). Fails only on a malformed exception comment. + - name: Report lint exceptions + if: ${{ !cancelled() }} + run: bun run lint:exceptions + type-check: runs-on: ubuntu-latest timeout-minutes: 20 diff --git a/package.json b/package.json index 5d5afd49..ba58785a 100644 --- a/package.json +++ b/package.json @@ -13,6 +13,7 @@ "check": "biome check . && bun run lint:oxlint", "check:fix": "biome check --fix . && bun run lint:oxlint --fix", "lint:oxlint": "oxlint --disable-nested-config packages/react", + "lint:exceptions": "bun tooling/lint/report-exceptions.ts", "format": "biome format .", "format:fix": "biome format --write .", "sgrep:watch": "sgrep watch .", diff --git a/packages/react/AGENTS.md b/packages/react/AGENTS.md index 9693c416..cc332701 100644 --- a/packages/react/AGENTS.md +++ b/packages/react/AGENTS.md @@ -199,3 +199,5 @@ When a rule is wrong for one line, disable it for that line and say why: `bazza/disable-needs-reason` rejects an exception with no rule or no reason, a `bazza/*` exception wider than one line, and `eslint-disable` comments (this repo has no ESLint, and oxlint ignores them). The `Allowlists` entries in `.oxlintrc.json` list files that broke a rule when it was added. Such a file is exempt from that rule until someone fixes it and deletes its line. Don't add files to an allowlist. New code follows the rules or uses a reasoned `oxlint-disable-next-line`. + +`bun run lint:exceptions` lists every allowlisted file by rule and every inline exception with its reason. CI runs it and adds the list to the job summary. It fails only on a malformed exception, which catches a bare `/* oxlint-disable */` that has silenced `bazza/disable-needs-reason` itself. diff --git a/tooling/lint/harness.ts b/tooling/lint/harness.ts index e27d669b..0d8ecb85 100644 --- a/tooling/lint/harness.ts +++ b/tooling/lint/harness.ts @@ -4,10 +4,10 @@ import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { fileURLToPath } from 'node:url' import { promisify } from 'node:util' +import { readOxlintConfig } from './source-text.ts' const run = promisify(execFile) const here = dirname(fileURLToPath(import.meta.url)) -const repoRoot = join(here, '../..') const oxlint = join(here, 'node_modules/.bin/oxlint') const bazzaPlugin = join(here, 'bazza-plugin.mjs') @@ -95,10 +95,9 @@ export function fixWith( }) } -/** The repo's `.oxlintrc.json`, with comments stripped and the plugin path made absolute. */ -async function repoConfig(): Promise { - const raw = await readFile(join(repoRoot, '.oxlintrc.json'), 'utf8') - const config = JSON.parse(raw.replace(/^\s*\/\/.*$/gm, '')) +/** The repo's `.oxlintrc.json`, with the plugin path made absolute. */ +function repoConfig(): object { + const config = readOxlintConfig() delete config.$schema config.jsPlugins = [bazzaPlugin] return config @@ -108,6 +107,6 @@ async function repoConfig(): Promise { * Lints `files` laid out at repo paths with the repo's own `.oxlintrc.json`, * so the tests see the same scoping CI does. */ -export async function lintWithRepoConfig(files: Files): Promise { - return lintIn(await repoConfig(), files) +export function lintWithRepoConfig(files: Files): Promise { + return lintIn(repoConfig(), files) } diff --git a/tooling/lint/report-exceptions.test.ts b/tooling/lint/report-exceptions.test.ts new file mode 100644 index 00000000..84f7c3d5 --- /dev/null +++ b/tooling/lint/report-exceptions.test.ts @@ -0,0 +1,242 @@ +import { existsSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' +import { + brokenExceptions, + findConfigExceptions, + findInlineExceptions, + formatReport, + type InlineException, +} from './report-exceptions.ts' +import { + comments, + parseJsonc, + readOxlintConfig, + repoRoot, +} from './source-text.ts' + +describe('comments', () => { + it("doesn't let an apostrophe in JSX text swallow later comments", () => { + const source = `export const A = () =>

Don't

/* on line 1 */ +import a from 'a' +const url = 'https://x' // on line 3 +const s = 'escaped \\ +newline' // on line 5 +` + expect(comments(source).map((c) => [c.line, c.value.trim()])).toEqual([ + [1, 'on line 1'], + [3, 'on line 3'], + [5, 'on line 5'], + ]) + }) + + it('ends an unclosed quote at the end of its line', () => { + const source = `const r = /"/ +// on line 2 +export const A = () =>

Bob's

+// on line 4 +` + expect(comments(source).map((c) => [c.line, c.value.trim()])).toEqual([ + [2, 'on line 2'], + [4, 'on line 4'], + ]) + }) + + it('skips comment markers inside strings and template literals', () => { + const source = `const url = 'https://example.com' // real one +const glob = "**/*.test.tsx" +const t = \`/* not a comment \${a /* inside code */ + '//'} still text // no\` +/* block + spans lines */ const b = 1 // after +` + expect(comments(source).map((c) => [c.line, c.value.trim()])).toEqual([ + [1, 'real one'], + [3, 'inside code'], + [4, 'block\n spans lines'], + [5, 'after'], + ]) + }) +}) + +describe('parseJsonc', () => { + it('drops comments and trailing commas but keeps strings intact', () => { + expect( + parseJsonc(`{ + // line comment + "url": "https://example.com", /* block */ + "glob": "src/*.{ts,}", + "list": [1, 2,], +}`), + ).toEqual({ + url: 'https://example.com', + glob: 'src/*.{ts,}', + list: [1, 2], + }) + }) +}) + +describe('findInlineExceptions', () => { + it('finds every disable directive, but not directive text in strings', () => { + const source = `// oxlint-disable-next-line bazza/use-client -- server-only helper +export const a = 1 +const b = 2 // eslint-disable-line react-hooks/exhaustive-deps +/* oxlint-disable */ +{/* oxlint-disable-next-line no-console -- JSX comment */} +const fixture = '// oxlint-disable-line' +const glob = 'src/**/*.ts' // oxlint-disable-line no-console -- after a glob +` + const found = findInlineExceptions(['a.tsx'], () => source) + expect(found.map((e) => [e.line, e.tool, e.rules, e.reason])).toEqual([ + [1, 'oxlint', ['bazza/use-client'], 'server-only helper'], + [3, 'eslint', ['react-hooks/exhaustive-deps'], ''], + [4, 'oxlint', [], ''], + [5, 'oxlint', ['no-console'], 'JSX comment'], + [7, 'oxlint', ['no-console'], 'after a glob'], + ]) + expect(found.map((e) => e.problem !== null)).toEqual([ + false, + true, + true, + false, + false, + ]) + }) +}) + +describe('findConfigExceptions', () => { + it('separates allowlists (literal paths) from rules turned off by pattern', () => { + const { allowlists, patterns } = findConfigExceptions({ + overrides: [ + { + files: ['packages/react/**/*.ts'], + rules: { 'bazza/use-client': 'error' }, + }, + { + files: ['packages/react/src/**/*.test.tsx'], + rules: { 'bazza/use-client': 'off' }, + }, + { + files: ['packages/react/src/b.ts', 'packages/react/src/a.ts'], + rules: { 'bazza/use-client': 'off' }, + }, + { + files: ['packages/react/src/c.ts'], + rules: { 'bazza/part-namespace': 'allow', 'bazza/use-client': 0 }, + }, + { + files: ['packages/react/src/d.ts'], + rules: { 'bazza/use-client': 'error' }, + }, + { + files: ['packages/react/src/e.ts'], + rules: { 'bazza/use-client': ['off', {}] }, + }, + ], + }) + expect(allowlists).toEqual([ + { rule: 'bazza/part-namespace', files: ['packages/react/src/c.ts'] }, + { + rule: 'bazza/use-client', + files: [ + 'packages/react/src/a.ts', + 'packages/react/src/b.ts', + 'packages/react/src/c.ts', + 'packages/react/src/e.ts', + ], + }, + ]) + expect(patterns).toEqual([ + { + files: ['packages/react/src/**/*.test.tsx'], + rules: ['bazza/use-client'], + }, + ]) + }) + + it("reads the repo's own config: allowlisted files exist, tests are scoped off", () => { + const { allowlists, patterns } = findConfigExceptions(readOxlintConfig()) + expect(allowlists.length).toBeGreaterThan(0) + for (const { files } of allowlists) { + for (const file of files) { + expect(existsSync(join(repoRoot, file)), file).toBe(true) + } + } + expect(patterns).toContainEqual({ + files: ['packages/react/src/**/*.test.{ts,tsx}'], + rules: expect.arrayContaining([ + 'bazza/use-client', + 'bazza/part-namespace', + ]), + }) + }) +}) + +describe('brokenExceptions and formatReport', () => { + const exception = (overrides: Partial): InlineException => ({ + file: 'a.ts', + line: 1, + tool: 'oxlint', + rules: ['no-console'], + reason: 'CLI output', + problem: null, + ...overrides, + }) + + it('fails on malformed exceptions, except eslint comments in a file already allowlisted for them', () => { + const allowlists = [{ rule: 'bazza/disable-needs-reason', files: ['a.ts'] }] + const staleEslint = exception({ + tool: 'eslint', + reason: '', + problem: 'no ESLint', + }) + const bareInAllowlisted = exception({ + line: 2, + rules: [], + reason: '', + problem: 'must name the rule', + }) + const bareElsewhere = exception({ + file: 'b.ts', + rules: [], + reason: '', + problem: 'must name the rule', + }) + const broken = brokenExceptions(allowlists, [ + staleEslint, + bareInAllowlisted, + bareElsewhere, + ]) + expect([...broken]).toEqual([bareInAllowlisted, bareElsewhere]) + }) + + it('counts entries and distinct files, and marks missing files and problems', () => { + const bad = exception({ + line: 4, + rules: [], + reason: '', + problem: 'must name the rule', + }) + const lines = formatReport( + { + allowlists: [ + { rule: 'bazza/part-namespace', files: ['here.ts'] }, + { rule: 'bazza/use-client', files: ['gone.ts', 'here.ts'] }, + ], + patterns: [{ files: ['**/*.test.ts'], rules: ['bazza/use-client'] }], + }, + [exception({}), bad], + new Set([bad]), + (file) => file === 'here.ts', + ) + expect(lines[0]).toBe( + 'Lint exceptions in packages/react: 3 allowlist entries across 2 file(s), 2 inline.', + ) + expect(lines).toContain(' gone.ts (file no longer exists)') + expect(lines).toContain(' **/*.test.ts: bazza/use-client') + expect(lines).toContain(' a.ts:1 no-console -- CLI output') + expect(lines).toContain( + ' a.ts:4 (all rules) -- (no reason) ✗ must name the rule', + ) + expect(lines.at(-1)).toContain("1 lint exception(s) aren't acceptable") + }) +}) diff --git a/tooling/lint/report-exceptions.ts b/tooling/lint/report-exceptions.ts new file mode 100644 index 00000000..1acc5c29 --- /dev/null +++ b/tooling/lint/report-exceptions.ts @@ -0,0 +1,216 @@ +/** + * Prints every lint exception in `packages/react`: each rule's allowlist in + * `.oxlintrc.json`, rules turned off by pattern, and every inline + * `oxlint-disable` comment with its reason. The counts are for people to + * watch and burn down, not a gate. + * + * It does fail (exit 1) on a malformed exception: no rule named, no reason, a + * file-wide `bazza/*` exception, or `eslint-disable`. That check has to live + * here too, because a bare `/* oxlint-disable *\/` can also silence + * `bazza/disable-needs-reason`, the lint rule meant to catch it, and a text + * scan can't be silenced. + */ +import { execFileSync } from 'node:child_process' +import { appendFileSync, existsSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import { directiveProblem, parseDirective } from './bazza-plugin.mjs' +import { comments, readOxlintConfig, repoRoot } from './source-text.ts' + +/** The extensions oxlint lints in `packages/react` (see `.oxlintrc.json`). */ +const extensions = ['ts', 'tsx', 'mts', 'cts', 'js', 'jsx', 'mjs', 'cjs'] + +export interface InlineException { + readonly file: string + readonly line: number + readonly tool: 'oxlint' | 'eslint' + readonly rules: readonly string[] + readonly reason: string + readonly problem: string | null +} + +export interface Allowlist { + readonly rule: string + readonly files: readonly string[] +} + +export interface PatternOff { + readonly files: readonly string[] + readonly rules: readonly string[] +} + +/** Every `oxlint-disable` / `eslint-disable` comment in `files`. */ +export function findInlineExceptions( + files: readonly string[], + read: (file: string) => string, +): InlineException[] { + const found: InlineException[] = [] + for (const file of files) { + for (const comment of comments(read(file))) { + const parsed = parseDirective(comment.value) + if (parsed?.action !== 'disable') continue + found.push({ + file, + line: comment.line, + tool: parsed.tool === 'eslint' ? 'eslint' : 'oxlint', + rules: parsed.rules, + reason: parsed.reason, + problem: directiveProblem(parsed), + }) + } + } + return found +} + +interface Override { + files?: string[] + rules?: Record +} + +/** oxlint's spellings of "off". */ +function isOff(level: unknown): boolean { + if (Array.isArray(level)) return isOff(level[0]) + return level === 'off' || level === 'allow' || level === 0 +} + +const isPattern = (path: string) => /[*?{[]/.test(path) + +/** + * The exceptions a config grants: allowlists (rules turned off for literal + * file paths) and rules turned off by pattern (like the override that exempts + * tests from the part rules). + */ +export function findConfigExceptions(config: { overrides?: Override[] }): { + allowlists: Allowlist[] + patterns: PatternOff[] +} { + const allowlisted = new Map>() + const patterns: PatternOff[] = [] + for (const override of config.overrides ?? []) { + const files = override.files ?? [] + const off = Object.entries(override.rules ?? {}) + .filter(([, level]) => isOff(level)) + .map(([rule]) => rule) + if (files.length === 0 || off.length === 0) continue + const literal = files.filter((file) => !isPattern(file)) + const patterned = files.filter(isPattern) + for (const rule of off) { + const set = allowlisted.get(rule) ?? new Set() + for (const file of literal) set.add(file) + if (literal.length > 0) allowlisted.set(rule, set) + } + if (patterned.length > 0) patterns.push({ files: patterned, rules: off }) + } + const allowlists = [...allowlisted] + .map(([rule, set]) => ({ rule, files: [...set].sort() })) + .sort((a, b) => a.rule.localeCompare(b.rule)) + return { allowlists, patterns } +} + +/** + * The malformed exceptions that fail the report. An `eslint-disable` comment + * in a file allowlisted for `bazza/disable-needs-reason` is on the burn-down + * list already, and oxlint ignores it, so it doesn't count. A malformed + * `oxlint-disable` always counts: it can silence rules. + */ +export function brokenExceptions( + allowlists: readonly Allowlist[], + inline: readonly InlineException[], +): Set { + const exempt = new Set( + allowlists.find((a) => a.rule === 'bazza/disable-needs-reason')?.files, + ) + return new Set( + inline.filter( + (exception) => + exception.problem && + !(exception.tool === 'eslint' && exempt.has(exception.file)), + ), + ) +} + +/** The report as lines of text. */ +export function formatReport( + { allowlists, patterns }: { allowlists: Allowlist[]; patterns: PatternOff[] }, + inline: readonly InlineException[], + broken: ReadonlySet, + exists: (file: string) => boolean, +): string[] { + const entries = allowlists.reduce((sum, a) => sum + a.files.length, 0) + const files = new Set(allowlists.flatMap((a) => a.files)).size + const lines = [ + `Lint exceptions in packages/react: ${entries} allowlist entries across ${files} file(s), ${inline.length} inline.`, + '', + 'Allowlisted files, by rule (fix a file, then delete its line in .oxlintrc.json):', + ] + for (const { rule, files: listed } of allowlists) { + lines.push(` ${rule} (${listed.length})`) + for (const file of listed) { + lines.push( + ` ${file}${exists(file) ? '' : ' (file no longer exists)'}`, + ) + } + } + lines.push('', 'Rules turned off by pattern:') + if (patterns.length === 0) lines.push(' none') + for (const pattern of patterns) { + lines.push(` ${pattern.files.join(', ')}: ${pattern.rules.join(', ')}`) + } + lines.push('', 'Inline exceptions:') + if (inline.length === 0) lines.push(' none') + for (const exception of inline) { + const rules = exception.rules.join(', ') || '(all rules)' + const reason = exception.reason || '(no reason)' + const mark = !exception.problem + ? '' + : broken.has(exception) + ? ` ✗ ${exception.problem}` + : ` (allowlisted) ${exception.problem}` + lines.push( + ` ${exception.file}:${exception.line} ${rules} -- ${reason}${mark}`, + ) + } + if (broken.size > 0) { + lines.push( + '', + `${broken.size} lint exception(s) aren't acceptable; see ✗ above and "Lint" in packages/react/AGENTS.md.`, + ) + } + return lines +} + +/** Tracked source files in `packages/react` that exist in the working tree. */ +function sourceFiles(): string[] { + const out = execFileSync( + 'git', + [ + 'ls-files', + '-z', + '--', + ...extensions.map((ext) => `packages/react/*.${ext}`), + ], + { cwd: repoRoot, encoding: 'utf8' }, + ) + return out + .split('\0') + .filter((file) => file && existsSync(join(repoRoot, file))) +} + +if ((import.meta as { main?: boolean }).main) { + const config = findConfigExceptions(readOxlintConfig()) + const inline = findInlineExceptions(sourceFiles(), (file) => + readFileSync(join(repoRoot, file), 'utf8'), + ) + const broken = brokenExceptions(config.allowlists, inline) + const lines = formatReport(config, inline, broken, (file) => + existsSync(join(repoRoot, file)), + ) + console.log(lines.join('\n')) + const summary = process.env.GITHUB_STEP_SUMMARY + if (summary) { + appendFileSync( + summary, + `## Lint exceptions\n\n\`\`\`text\n${lines.join('\n')}\n\`\`\`\n`, + ) + } + if (broken.size > 0) process.exitCode = 1 +} diff --git a/tooling/lint/source-text.ts b/tooling/lint/source-text.ts new file mode 100644 index 00000000..4662bced --- /dev/null +++ b/tooling/lint/source-text.ts @@ -0,0 +1,161 @@ +/** + * Reading comments and JSONC from source text without a parser. Shared by the + * exceptions report and the test harness. + */ +import { readFileSync } from 'node:fs' +import { dirname, join } from 'node:path' +import { fileURLToPath } from 'node:url' + +export const repoRoot = join(dirname(fileURLToPath(import.meta.url)), '../..') + +export interface Comment { + /** The text between `//` and the line end, or between `/*` and `*\/`. */ + readonly value: string + readonly start: number + readonly end: number + readonly line: number +} + +/** + * Every comment in `source`, skipping string and template literals, so a + * `//` in a URL or a `/*` in a glob isn't mistaken for one. An apostrophe in + * JSX text (`

Don't

`) doesn't open a string, and a quoted string ends at + * the end of its line, since JS strings can't span lines. + * + * Known limits: regular expression literals and JSX text aren't recognised. + * A quote after a non-word character (`Bob's`) or a `//` in either can + * hide the rest of that line; a `/*` in either can hide more. + */ +export function comments(source: string): Comment[] { + const found: Comment[] = [] + // Template literals nest through `${ … }`; each entry counts open braces. + const templateBraces: number[] = [] + let line = 1 + let i = 0 + const skipString = (quote: string) => { + i++ + while (i < source.length && source[i] !== quote && source[i] !== '\n') { + if (source[i] === '\\') { + i++ + if (source[i] === '\n') line++ + } + i++ + } + // Stop before an unescaped newline so the main loop counts it. + if (source[i] === quote) i++ + } + const skipTemplate = () => { + i++ + while (i < source.length) { + const char = source[i] + if (char === '\\') { + if (source[i + 1] === '\n') line++ + i += 2 + continue + } + if (char === '\n') line++ + if (char === '`') { + i++ + return + } + if (char === '$' && source[i + 1] === '{') { + templateBraces.push(0) + i += 2 + return + } + i++ + } + } + while (i < source.length) { + const char = source[i] + const next = source[i + 1] + if (char === '\n') { + line++ + i++ + } else if (char === '/' && next === '/') { + const end = source.indexOf('\n', i) + const stop = end === -1 ? source.length : end + found.push({ + value: source.slice(i + 2, stop), + start: i, + end: stop, + line, + }) + i = stop + } else if (char === '/' && next === '*') { + const close = source.indexOf('*/', i + 2) + const stop = close === -1 ? source.length : close + 2 + const value = source.slice(i + 2, close === -1 ? stop : close) + found.push({ value, start: i, end: stop, line }) + line += value.split('\n').length - 1 + i = stop + } else if ( + (char === '"' || char === "'") && + // A quote right after a letter or digit is text (`Don't`), not a + // string: JS never starts a string literal there. + !/[\w$]/.test(source[i - 1] ?? '') + ) { + skipString(char) + } else if (char === '`') { + skipTemplate() + } else if (templateBraces.length > 0 && (char === '{' || char === '}')) { + const depth = templateBraces.length - 1 + if (char === '{') { + templateBraces[depth] = (templateBraces[depth] ?? 0) + 1 + i++ + } else if (templateBraces[depth] === 0) { + // The `}` that closes `${`: back inside the template literal. + templateBraces.pop() + i-- + skipTemplate() + } else { + templateBraces[depth] = (templateBraces[depth] ?? 1) - 1 + i++ + } + } else { + i++ + } + } + return found +} + +/** Removes commas that directly precede `}` or `]`, outside JSON strings. */ +function dropTrailingCommas(json: string): string { + let out = '' + let inString = false + for (let i = 0; i < json.length; i++) { + const char = json[i] + if (inString) { + out += char + if (char === '\\') out += json[++i] ?? '' + else if (char === '"') inString = false + } else if (char === '"') { + inString = true + out += char + } else if (char === ',' && /^\s*[}\]]/.test(json.slice(i + 1))) { + // A trailing comma: drop it. + } else { + out += char + } + } + return out +} + +/** Parses JSONC: JSON with comments and trailing commas. */ +export function parseJsonc(source: string): unknown { + let text = '' + let from = 0 + for (const comment of comments(source)) { + text += source.slice(from, comment.start) + from = comment.end + } + text += source.slice(from) + return JSON.parse(dropTrailingCommas(text)) +} + +/** The repo's `.oxlintrc.json`, parsed. */ +export function readOxlintConfig(): Record { + return parseJsonc( + readFileSync(join(repoRoot, '.oxlintrc.json'), 'utf8'), + ) as Record +}