From db42b15eec04492794b1819bedfe7f4b4e73624b Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Sat, 18 Jul 2026 14:56:54 -0500 Subject: [PATCH 01/15] Add SARIF lint regression compare (gtb lint:eslint:compare) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lay the CLI groundwork for the #38 lint regression gate: - lint:eslint loads a bundled wrapper formatter that writes dist/eslint.sarif (via @microsoft/eslint-formatter-sarif) and returns compact console output; the file is a turbo task output so cached runs restore it - gtb lint:eslint:compare walks each lint cwd, runs sarif-multitool match-results-forward against dist/eslint-base.sarif, and fails only on results the baseliner classifies as new — fingerprint and content-based matching absorbs line shifts without any git-diff offset math - a pnpm override drops the formatter package''s wrongly-declared eslint v8 dependency, whose bin otherwise shadows the workspace ESLint inside packages/cli CI wiring (base-SHA lint via turbo remote cache, PR comment, label override) is follow-up work tracked in #38. Co-Authored-By: Claude Fable 5 --- .changeset/sarif-lint-regression-compare.md | 10 + packages/cli/package.json | 8 +- packages/cli/src/commands/index.ts | 2 + .../src/commands/root/lint-eslint-compare.ts | 17 ++ packages/cli/src/commands/root/names.ts | 1 + packages/cli/src/commands/task/lint-eslint.ts | 11 + .../cli/src/lib/eslint-sarif-formatter.ts | 59 +++++ packages/cli/src/lib/lint-compare.ts | 208 ++++++++++++++++++ packages/cli/src/lib/turbo-config.ts | 4 +- .../microsoft-eslint-formatter-sarif.d.ts | 5 + .../cli/test/eslint-sarif-formatter.test.ts | 55 +++++ packages/cli/test/lint-compare.test.ts | 208 ++++++++++++++++++ pnpm-lock.yaml | 76 +++++++ pnpm-workspace.yaml | 9 + turbo.json | 6 +- 15 files changed, 673 insertions(+), 6 deletions(-) create mode 100644 .changeset/sarif-lint-regression-compare.md create mode 100644 packages/cli/src/commands/root/lint-eslint-compare.ts create mode 100644 packages/cli/src/lib/eslint-sarif-formatter.ts create mode 100644 packages/cli/src/lib/lint-compare.ts create mode 100644 packages/cli/src/types/microsoft-eslint-formatter-sarif.d.ts create mode 100644 packages/cli/test/eslint-sarif-formatter.test.ts create mode 100644 packages/cli/test/lint-compare.test.ts diff --git a/.changeset/sarif-lint-regression-compare.md b/.changeset/sarif-lint-regression-compare.md new file mode 100644 index 00000000..7802693e --- /dev/null +++ b/.changeset/sarif-lint-regression-compare.md @@ -0,0 +1,10 @@ +--- +'@gtbuchanan/cli': minor +--- + +Add SARIF-based lint regression tooling: `lint:eslint` now writes a SARIF +log (`dist/eslint.sarif`) alongside its console output via a bundled +formatter, and the new `gtb lint:eslint:compare` command diffs each lint +cwd's current SARIF log against a baseline +(`dist/eslint-base.sarif`) using `sarif-multitool match-results-forward`, +failing only on violations not present in the baseline. diff --git a/packages/cli/package.json b/packages/cli/package.json index 6e5c6a83..ebe1bea9 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -7,7 +7,8 @@ "#src/*": "./src/*" }, "exports": { - ".": "./src/index.ts" + ".": "./src/index.ts", + "./eslint-sarif-formatter": "./src/lib/eslint-sarif-formatter.ts" }, "bin": { "gtb": "./dist/source/bin/gtb.js" @@ -27,6 +28,8 @@ "typecheck:ts": "pnpm run gtb task typecheck:ts" }, "dependencies": { + "@microsoft/eslint-formatter-sarif": "catalog:", + "@microsoft/sarif-multitool": "catalog:", "citty": "catalog:", "cross-spawn": "catalog:", "find-up-simple": "catalog:", @@ -63,7 +66,8 @@ }, "directory": "dist/source", "exports": { - ".": "./src/index.js" + ".": "./src/index.js", + "./eslint-sarif-formatter": "./src/lib/eslint-sarif-formatter.js" }, "imports": { "#src/*.ts": "./src/*.js" diff --git a/packages/cli/src/commands/index.ts b/packages/cli/src/commands/index.ts index fd394176..996bd2e7 100644 --- a/packages/cli/src/commands/index.ts +++ b/packages/cli/src/commands/index.ts @@ -1,5 +1,6 @@ import { defineCommand } from 'citty'; import { hk } from './root/hk.ts'; +import { lintEslintCompare } from './root/lint-eslint-compare.ts'; import { rootNames } from './root/names.ts'; import { prepare } from './root/prepare.ts'; import { publish } from './root/publish.ts'; @@ -18,6 +19,7 @@ export const main = defineCommand({ }, subCommands: { [rootNames.hk]: hk, + [rootNames.lintEslintCompare]: lintEslintCompare, [rootNames.prepare]: prepare, [rootNames.publish]: publish, [rootNames.sync]: sync, diff --git a/packages/cli/src/commands/root/lint-eslint-compare.ts b/packages/cli/src/commands/root/lint-eslint-compare.ts new file mode 100644 index 00000000..01cbba5e --- /dev/null +++ b/packages/cli/src/commands/root/lint-eslint-compare.ts @@ -0,0 +1,17 @@ +import { defineCommand } from 'citty'; +import { executeLintEslintCompare } from '../../lib/lint-compare.ts'; +import { rootNames } from './names.ts'; + +/** + * `gtb lint:eslint:compare` — gates on lint regressions by diffing the + * current SARIF logs against baseline logs from the merge base. A user + * command rather than a turbo task: it consumes two generations of the + * same task output, which turbo's task model can't express. + */ +export const lintEslintCompare = defineCommand({ + meta: { + description: 'Fail when lint violations are new relative to the SARIF baseline', + name: rootNames.lintEslintCompare, + }, + run: () => executeLintEslintCompare(), +}); diff --git a/packages/cli/src/commands/root/names.ts b/packages/cli/src/commands/root/names.ts index f8545277..26c657e6 100644 --- a/packages/cli/src/commands/root/names.ts +++ b/packages/cli/src/commands/root/names.ts @@ -1,6 +1,7 @@ /** CLI names for root-level commands. */ export const rootNames = { hk: 'hk', + lintEslintCompare: 'lint:eslint:compare', prepare: 'prepare', publish: 'publish', sync: 'sync', diff --git a/packages/cli/src/commands/task/lint-eslint.ts b/packages/cli/src/commands/task/lint-eslint.ts index b5877176..a9b3e020 100644 --- a/packages/cli/src/commands/task/lint-eslint.ts +++ b/packages/cli/src/commands/task/lint-eslint.ts @@ -1,6 +1,16 @@ +import { fileURLToPath } from 'node:url'; import { defineCommand } from 'citty'; import { run } from '../../lib/process.ts'; +/** + * The SARIF-writing formatter, resolved through the package's own export + * map so the same specifier works from TS source (self-host) and the + * compiled publish layout. + */ +const formatterPath = fileURLToPath( + import.meta.resolve('@gtbuchanan/cli/eslint-sarif-formatter'), +); + /** Runs ESLint with caching and zero-warning threshold. */ export const lintEslint = defineCommand({ meta: { @@ -12,6 +22,7 @@ export const lintEslint = defineCommand({ args: [ '--cache', '--cache-location', 'dist/.eslintcache', '--max-warnings=0', + '--format', formatterPath, ...rawArgs, ], }); diff --git a/packages/cli/src/lib/eslint-sarif-formatter.ts b/packages/cli/src/lib/eslint-sarif-formatter.ts new file mode 100644 index 00000000..1d479e47 --- /dev/null +++ b/packages/cli/src/lib/eslint-sarif-formatter.ts @@ -0,0 +1,59 @@ +import { mkdirSync, writeFileSync } from 'node:fs'; +import path from 'node:path'; +import sarifFormat from '@microsoft/eslint-formatter-sarif'; + +/** SARIF log path written by the formatter, relative to the lint cwd. */ +export const sarifOutputPath = 'dist/eslint.sarif'; + +/** Structural subset of an ESLint lint message the console output needs. */ +export interface FormatterMessage { + readonly column: number; + readonly line: number; + readonly message: string; + readonly ruleId: string | null; + readonly severity: number; +} + +/** Structural subset of an ESLint lint result the console output needs. */ +export interface FormatterResult { + readonly filePath: string; + readonly messages: readonly FormatterMessage[]; +} + +/** ESLint's numeric severity for an error (1 is a warning). */ +const errorSeverity = 2; + +/** Builds compact per-violation console output (one line per message). */ +export const formatConsole = (results: readonly FormatterResult[]): string => { + const lines = results.flatMap(result => + result.messages.map((message) => { + const severity = message.severity === errorSeverity ? 'error' : 'warning'; + const rule = message.ruleId ?? 'internal'; + const position = `${String(message.line)}:${String(message.column)}`; + return `${result.filePath}:${position} ${severity} ${message.message} ${rule}`; + }), + ); + if (lines.length === 0) { + return ''; + } + const problems = `${String(lines.length)} problem${lines.length === 1 ? '' : 's'}`; + return `${lines.join('\n')}\n\n✖ ${problems}\n`; +}; + +/** + * ESLint formatter that writes a SARIF log to {@link sarifOutputPath} + * (for the CI lint regression compare) and returns compact console + * output. ESLint accepts a single `--format`, so the side-effecting + * file write is how one run feeds both consumers. + */ +const eslintSarifFormatter = ( + results: readonly FormatterResult[], + data?: unknown, +): string => { + const outputFile = path.resolve(sarifOutputPath); + mkdirSync(path.dirname(outputFile), { recursive: true }); + writeFileSync(outputFile, sarifFormat(results, data)); + return formatConsole(results); +}; + +export default eslintSarifFormatter; diff --git a/packages/cli/src/lib/lint-compare.ts b/packages/cli/src/lib/lint-compare.ts new file mode 100644 index 00000000..733ed0e6 --- /dev/null +++ b/packages/cli/src/lib/lint-compare.ts @@ -0,0 +1,208 @@ +import { existsSync } from 'node:fs'; +import { createRequire } from 'node:module'; +import path from 'node:path'; +import * as v from 'valibot'; +import { readJsonFile } from './file-writer.ts'; +import { type Logger, createLogger } from './logger.ts'; +import { type RunOptions, run } from './process.ts'; +import { type WorkspaceContext, resolveWorkspace } from './workspace.ts'; + +/** SARIF artifact filenames under a lint cwd's `dist/`. */ +export const sarifFileNames = { + base: 'eslint-base.sarif', + current: 'eslint.sarif', + matched: 'eslint-matched.sarif', +} as const; + +const SarifRegionSchema = v.object({ + startColumn: v.optional(v.number()), + startLine: v.optional(v.number()), +}); + +const SarifArtifactLocationSchema = v.object({ + uri: v.optional(v.string()), +}); + +const SarifPhysicalLocationSchema = v.object({ + artifactLocation: v.optional(SarifArtifactLocationSchema), + region: v.optional(SarifRegionSchema), +}); + +const SarifLocationSchema = v.object({ + physicalLocation: v.optional(SarifPhysicalLocationSchema), +}); + +const SarifResultSchema = v.object({ + baselineState: v.optional(v.string()), + level: v.optional(v.string()), + locations: v.optional(v.array(SarifLocationSchema)), + message: v.object({ text: v.string() }), + ruleId: v.optional(v.string()), +}); + +const SarifRunSchema = v.object({ + results: v.optional(v.array(SarifResultSchema)), +}); + +const SarifLogSchema = v.object({ + runs: v.array(SarifRunSchema), +}); + +/** Parsed subset of a SARIF log the compare consumes. */ +export type SarifLog = v.InferOutput; + +type SarifResult = v.InferOutput; + +/** Validates untrusted JSON as a {@link SarifLog}. */ +export const parseSarifLog = (data: unknown): SarifLog => + v.parse(SarifLogSchema, data); + +/** A lint violation present in HEAD but not matched to the baseline. */ +export interface NewViolation { + readonly column: number | undefined; + readonly level: string; + readonly line: number | undefined; + readonly message: string; + readonly ruleId: string; + readonly uri: string; +} + +interface LocationParts { + readonly column: number | undefined; + readonly line: number | undefined; + readonly uri: string | undefined; +} + +const toLocationParts = (result: SarifResult): LocationParts => { + const location = result.locations?.[0]?.physicalLocation; + return { + column: location?.region?.startColumn, + line: location?.region?.startLine, + uri: location?.artifactLocation?.uri, + }; +}; + +const toNewViolation = (result: SarifResult): NewViolation => { + const { column, line, uri } = toLocationParts(result); + return { + column, + level: result.level ?? 'error', + line, + message: result.message.text, + ruleId: result.ruleId ?? 'internal', + uri: uri ?? '', + }; +}; + +/** Extracts results the baseliner classified as `new`. */ +export const extractNewViolations = (log: SarifLog): readonly NewViolation[] => + log.runs.flatMap(run_ => + (run_.results ?? []) + .filter(result => result.baselineState === 'new') + .map(toNewViolation), + ); + +const formatPosition = (violation: NewViolation): string => { + if (violation.line === undefined) return ''; + const column = violation.column === undefined ? '' : `:${String(violation.column)}`; + return `:${String(violation.line)}${column}`; +}; + +const formatViolation = (violation: NewViolation): string => { + const { level, message, ruleId, uri } = violation; + return `${uri}${formatPosition(violation)} ${level} ${message} ${ruleId}`; +}; + +/** Renders new violations for console output, one line per violation. */ +export const formatNewViolations = ( + violations: readonly NewViolation[], +): string => violations.map(formatViolation).join('\n'); + +/** + * Side-effecting I/O the compare depends on. Injected so the + * orchestration (per-package matching, gating) is unit-testable without + * spawning the SARIF multitool. + */ +export interface LintCompareDeps { + readonly exists: (filePath: string) => boolean; + readonly logger: Logger; + readonly readJson: (filePath: string) => unknown; + readonly resolveMultitool: () => string; + readonly run: (command: string, options?: RunOptions) => Promise; + readonly workspace: () => WorkspaceContext; +} + +/** + * The multitool npm package exports the path to its platform-specific + * self-contained binary. Resolved lazily (and spawned directly, skipping + * the package's `shell: true` bin shim) so merely loading the CLI never + * requires the optional platform package. + */ +const resolveMultitoolBinary = (): string => { + const require = createRequire(import.meta.url); + return v.parse(v.string(), require('@microsoft/sarif-multitool')); +}; + +const defaultDeps: LintCompareDeps = { + exists: existsSync, + logger: createLogger(), + readJson: readJsonFile, + resolveMultitool: resolveMultitoolBinary, + run, + workspace: resolveWorkspace, +}; + +const matchDirForward = async ( + dir: string, + deps: LintCompareDeps, +): Promise => { + const distDir = path.join(dir, 'dist'); + const current = path.join(distDir, sarifFileNames.current); + if (!deps.exists(current)) { + return []; + } + const base = path.join(distDir, sarifFileNames.base); + if (!deps.exists(base)) { + deps.logger.error(`No baseline SARIF for ${dir}; skipping`); + return []; + } + const matched = path.join(distDir, sarifFileNames.matched); + await deps.run(deps.resolveMultitool(), { + args: [ + 'match-results-forward', current, + '--previous', base, + '--output-file-path', matched, + // Reruns are routine (retries, local iteration); replace stale output. + '--log', 'ForceOverwrite', + ], + }); + const log = parseSarifLog(deps.readJson(matched)); + return extractNewViolations(log); +}; + +/** + * Compares each lint cwd's current SARIF log against its baseline via + * `sarif-multitool match-results-forward` and rejects when any result + * is classified `new`. Matching is fingerprint/content-based, so + * baseline violations that merely moved (edits above them) stay + * matched — only genuine regressions gate. + */ +export const executeLintEslintCompare = async ( + deps: LintCompareDeps = defaultDeps, +): Promise => { + const { packageDirs, rootDir } = deps.workspace(); + const lintDirs = [...new Set([rootDir, ...packageDirs])]; + const violations: NewViolation[] = []; + + for (const dir of lintDirs) { + violations.push(...await matchDirForward(dir, deps)); + } + + if (violations.length > 0) { + deps.logger.error(formatNewViolations(violations)); + throw new Error( + `${String(violations.length)} new lint violation(s) not present in the baseline`, + ); + } + deps.logger.info('No new lint violations'); +}; diff --git a/packages/cli/src/lib/turbo-config.ts b/packages/cli/src/lib/turbo-config.ts index 9a58f71f..3e3dfb22 100644 --- a/packages/cli/src/lib/turbo-config.ts +++ b/packages/cli/src/lib/turbo-config.ts @@ -259,7 +259,7 @@ const lintTasks = (flags: ToolFlags): readonly ConditionalEntry[] => value: { dependsOn: deps, inputs: ['$TURBO_ROOT$/eslint.config.*', ...inputs, 'eslint.config.*'], - outputs: ['dist/.eslintcache'], + outputs: ['dist/.eslintcache', 'dist/eslint.sarif'], }, }, ]; @@ -294,7 +294,7 @@ const rootLintTasks = ( '$TURBO_DEFAULT$', ...packageGlobs.map(glob => `!${toPackageIgnore(glob)}`), ], - outputs: ['dist/.eslintcache'], + outputs: ['dist/.eslintcache', 'dist/eslint.sarif'], }, }, ]; diff --git a/packages/cli/src/types/microsoft-eslint-formatter-sarif.d.ts b/packages/cli/src/types/microsoft-eslint-formatter-sarif.d.ts new file mode 100644 index 00000000..62120555 --- /dev/null +++ b/packages/cli/src/types/microsoft-eslint-formatter-sarif.d.ts @@ -0,0 +1,5 @@ +/** Untyped CJS formatter; minimal structural signature for the wrapper. */ +declare module '@microsoft/eslint-formatter-sarif' { + const format: (results: readonly unknown[], data?: unknown) => string; + export = format; +} diff --git a/packages/cli/test/eslint-sarif-formatter.test.ts b/packages/cli/test/eslint-sarif-formatter.test.ts new file mode 100644 index 00000000..ea1d6e84 --- /dev/null +++ b/packages/cli/test/eslint-sarif-formatter.test.ts @@ -0,0 +1,55 @@ +import { faker } from '@faker-js/faker'; +import { describe, it } from 'vitest'; +import { + type FormatterResult, + formatConsole, +} from '#src/lib/eslint-sarif-formatter.js'; + +describe.concurrent(formatConsole, () => { + it('returns empty output for a clean run', ({ expect }) => { + const clean: FormatterResult = { filePath: faker.system.filePath(), messages: [] }; + + expect(formatConsole([clean])).toBe(''); + }); + + it('renders one line per message plus a problem count', ({ expect }) => { + const filePath = faker.system.filePath(); + const result: FormatterResult = { + filePath, + messages: [ + { + column: 3, line: 2, message: 'Unexpected console statement.', + ruleId: 'no-console', severity: 2, + }, + { + column: 1, line: 9, message: 'Missing JSDoc.', + ruleId: 'jsdoc/require-jsdoc', severity: 1, + }, + ], + }; + + const output = formatConsole([result]); + + expect(output).toContain(`${filePath}:2:3 error Unexpected console statement. no-console`); + expect(output).toContain(`${filePath}:9:1 warning Missing JSDoc. jsdoc/require-jsdoc`); + expect(output).toContain('✖ 2 problems'); + }); + + it('labels a message with no rule as internal', ({ expect }) => { + const result: FormatterResult = { + filePath: faker.system.filePath(), + messages: [{ + column: 0, + line: 0, + message: 'Parsing error', + /* eslint-disable-next-line unicorn/no-null -- + ESLint reports a null ruleId for parse/internal errors; the + test mirrors that external shape. */ + ruleId: null, + severity: 2, + }], + }; + + expect(formatConsole([result])).toContain('internal'); + }); +}); diff --git a/packages/cli/test/lint-compare.test.ts b/packages/cli/test/lint-compare.test.ts new file mode 100644 index 00000000..aab2dccd --- /dev/null +++ b/packages/cli/test/lint-compare.test.ts @@ -0,0 +1,208 @@ +import { faker } from '@faker-js/faker'; +import { describe, it } from 'vitest'; +import { + type LintCompareDeps, + type NewViolation, + executeLintEslintCompare, + extractNewViolations, + formatNewViolations, + parseSarifLog, + sarifFileNames, +} from '#src/lib/lint-compare.js'; +import type { Logger } from '#src/lib/logger.js'; +import type { WorkspaceContext } from '#src/lib/workspace.js'; + +const sarifResult = (baselineState: string, overrides?: { + readonly ruleId?: string; + readonly startLine?: number; +}): object => ({ + baselineState, + level: 'error', + locations: [{ + physicalLocation: { + artifactLocation: { uri: faker.system.filePath() }, + region: { startColumn: 1, startLine: overrides?.startLine ?? 1 }, + }, + }], + message: { text: faker.lorem.sentence() }, + ruleId: overrides?.ruleId ?? 'no-unused-vars', +}); + +const sarifLog = (results: readonly object[]): object => ({ + runs: [{ results }], +}); + +interface RunCall { + readonly args: readonly string[]; + readonly command: string; +} + +interface StubDeps { + readonly deps: LintCompareDeps; + readonly errors: readonly string[]; + readonly infos: readonly string[]; + readonly runCalls: readonly RunCall[]; +} + +const normalize = (filePath: string): string => filePath.replaceAll('\\', '/'); + +/** + * Fakes a workspace whose SARIF files are the keys of `files`; reading + * any matched path yields `matched`. + */ +const stubDeps = ( + workspace: WorkspaceContext, + files: readonly string[], + matched: object, +): StubDeps => { + const errors: string[] = []; + const infos: string[] = []; + const runCalls: RunCall[] = []; + const logger: Logger = { + error: (...args) => void errors.push(args.join(' ')), + info: (...args) => void infos.push(args.join(' ')), + }; + return { + deps: { + exists: filePath => files.includes(normalize(filePath)), + logger, + readJson: () => matched, + resolveMultitool: () => 'sarif-multitool', + run: (command, options) => { + runCalls.push({ args: options?.args ?? [], command }); + return Promise.resolve(); + }, + workspace: () => workspace, + }, + errors, + infos, + runCalls, + }; +}; + +describe.concurrent(extractNewViolations, () => { + it('keeps only results the baseliner classified as new', ({ expect }) => { + const newResult = sarifResult('new', { ruleId: 'no-console' }); + const log = sarifLog([ + sarifResult('unchanged'), + sarifResult('updated'), + newResult, + sarifResult('absent'), + ]); + + const violations = extractNewViolations(parseSarifLog(log)); + + expect(violations).toHaveLength(1); + expect(violations[0]).toMatchObject({ level: 'error', ruleId: 'no-console' }); + }); + + it('falls back to placeholders when location or rule is absent', ({ expect }) => { + const log = parseSarifLog(sarifLog([ + { baselineState: 'new', message: { text: 'boom' } }, + ])); + + expect(extractNewViolations(log)).toStrictEqual([{ + column: undefined, + level: 'error', + line: undefined, + message: 'boom', + ruleId: 'internal', + uri: '', + }]); + }); +}); + +describe.concurrent(formatNewViolations, () => { + it('renders uri, position, level, message, and rule per line', ({ expect }) => { + const violation: NewViolation = { + column: 3, + level: 'error', + line: 7, + message: 'Unexpected console statement.', + ruleId: 'no-console', + uri: 'file:///repo/src/app.js', + }; + + expect(formatNewViolations([violation])).toBe( + 'file:///repo/src/app.js:7:3 error Unexpected console statement. no-console', + ); + }); + + it('omits the position when the result has no region', ({ expect }) => { + const violation: NewViolation = { + column: undefined, + level: 'warning', + line: undefined, + message: 'boom', + ruleId: 'internal', + uri: '', + }; + + expect(formatNewViolations([violation])).toBe(' warning boom internal'); + }); +}); + +describe.concurrent(executeLintEslintCompare, () => { + const workspace: WorkspaceContext = { + packageDirs: ['/repo/packages/a'], + packageGlobs: ['packages/*'], + rootDir: '/repo', + }; + const currentFiles = [ + `/repo/dist/${sarifFileNames.current}`, + `/repo/dist/${sarifFileNames.base}`, + `/repo/packages/a/dist/${sarifFileNames.current}`, + `/repo/packages/a/dist/${sarifFileNames.base}`, + ]; + + it('matches every lint dir forward against its baseline', async ({ expect }) => { + const { deps, runCalls } = stubDeps(workspace, currentFiles, sarifLog([])); + + await executeLintEslintCompare(deps); + + expect(runCalls).toHaveLength(2); + expect(runCalls[0]).toMatchObject({ command: 'sarif-multitool' }); + expect(runCalls[0]?.args[0]).toBe('match-results-forward'); + }); + + it('resolves and reports when no results are new', async ({ expect }) => { + const { deps, infos } = stubDeps( + workspace, + currentFiles, + sarifLog([sarifResult('unchanged'), sarifResult('updated')]), + ); + + await executeLintEslintCompare(deps); + + expect(infos).toContain('No new lint violations'); + }); + + it('rejects when any result is new', async ({ expect }) => { + const { deps } = stubDeps(workspace, currentFiles, sarifLog([sarifResult('new')])); + + await expect(executeLintEslintCompare(deps)).rejects.toThrow( + /new lint violation/v, + ); + }); + + it('skips dirs with no current SARIF log', async ({ expect }) => { + const { deps, runCalls } = stubDeps(workspace, [], sarifLog([])); + + await executeLintEslintCompare(deps); + + expect(runCalls).toHaveLength(0); + }); + + it('warns and skips dirs missing only the baseline', async ({ expect }) => { + const { deps, errors, runCalls } = stubDeps( + workspace, + [`/repo/dist/${sarifFileNames.current}`], + sarifLog([]), + ); + + await executeLintEslintCompare(deps); + + expect(runCalls).toHaveLength(0); + expect(errors.some(line => line.includes('No baseline SARIF'))).toBe(true); + }); +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 393d30e7..de2e9146 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -21,6 +21,12 @@ catalogs: '@faker-js/faker': specifier: ^10.4.0 version: 10.5.0 + '@microsoft/eslint-formatter-sarif': + specifier: ^3.1.0 + version: 3.1.0 + '@microsoft/sarif-multitool': + specifier: ^5.5.0 + version: 5.5.0 '@pnpm/lockfile.types': specifier: ^1100.0.5 version: 1100.0.14 @@ -160,6 +166,9 @@ catalogs: specifier: ^2.8.3 version: 2.9.0 +overrides: + '@microsoft/eslint-formatter-sarif>eslint': '-' + pnpmfileChecksum: sha256-hgl/TpYs239ORonvRcYC8xhrwWZfg+PQ6jMTDNGBxxQ= importers: @@ -235,6 +244,12 @@ importers: packages/cli: dependencies: + '@microsoft/eslint-formatter-sarif': + specifier: 'catalog:' + version: 3.1.0 + '@microsoft/sarif-multitool': + specifier: 'catalog:' + version: 5.5.0 citty: specifier: 'catalog:' version: 0.2.2 @@ -783,6 +798,30 @@ packages: '@manypkg/get-packages@1.1.3': resolution: {integrity: sha512-fo+QhuU3qE/2TQMQmbVMqaQ6EWbMhi4ABWP+O4AM1NqPBuy0OrApV5LO6BrrgnhtAHS2NH6RrVk9OL181tTi8A==} + '@microsoft/eslint-formatter-sarif@3.1.0': + resolution: {integrity: sha512-/mn4UXziHzGXnKCg+r8HGgPy+w4RzpgdoqFuqaKOqUVBT5x2CygGefIrO4SusaY7t0C4gyIWMNu6YQT6Jw64Cw==} + engines: {node: '>= 14'} + + '@microsoft/sarif-multitool-darwin@5.5.0': + resolution: {integrity: sha512-HgCqNs9XKfvHjFtC6lJocj9o2zXXv7NjgeLIW2iCnCfsm7IOUQ6JKMXXb6C8GVmyCuEg7Xl8vwNudTTV0D1paA==} + os: [darwin] + hasBin: true + + '@microsoft/sarif-multitool-linux@5.5.0': + resolution: {integrity: sha512-k/86uqCJFIo33WSiDMfHkRMF1/5S/bbMFhh+doAlBGxvaV8+zDK7hY/tjO8+AQxhU18UdTLCu/VacuK4+uOmNQ==} + os: [linux] + hasBin: true + + '@microsoft/sarif-multitool-win32@5.5.0': + resolution: {integrity: sha512-h9axK25H+FmHTWViBqoywQI0tfKRn9txwI+W/IrRFCuFmKUXHpNmpQl9JMFz77PVsNM+pCOde3QT7qo573/SKQ==} + os: [win32] + hasBin: true + + '@microsoft/sarif-multitool@5.5.0': + resolution: {integrity: sha512-KCO8ELLnoV0Z/4QeoKxe+YL3061Kolg1he18iTrWCEoq7+Zme9oUQ2NCuLD0he+lML5TnWtER7fG1zE1tObjxQ==} + os: [darwin, linux, win32] + hasBin: true + '@napi-rs/wasm-runtime@0.2.12': resolution: {integrity: sha512-ZVWUcfwY4E/yPitQJl481FjFo3K22D6qF0DuFH6Y/nbnE11GY5uguDxZMGXPQ8WQ0128MXQD7TnfHyK4oWoIJQ==} @@ -2077,6 +2116,10 @@ packages: resolution: {integrity: sha512-qQKT4zQxXl8lLwBtHMWwaTcGfFOZviOJet3Oy/xmGk2gZH677CJM9EvtfdSkgWcATZhj/55JZ0rmy3myCT5lsA==} hasBin: true + jschardet@3.1.4: + resolution: {integrity: sha512-/kmVISmrwVwtyYU40iQUOp3SUPk2dhNCMsZBQX0R1/jZ8maaXJ/oZIzUOiyOqcgtLnETFKYChbJ5iDC/eWmFHg==} + engines: {node: '>=0.1.90'} + jsdoc-type-pratt-parser@7.2.0: resolution: {integrity: sha512-dh140MMgjyg3JhJZY/+iEzW+NO5xR2gpbDFKHqotCmexElVntw7GjWjt511+C/Ef02RU5TKYrJo/Xlzk+OLaTw==} engines: {node: '>=20.0.0'} @@ -2210,6 +2253,9 @@ packages: lodash.startcase@4.4.0: resolution: {integrity: sha512-+WKqsK294HMSc2jEbNgpHpd0JfIBhp7rEV4aqXWqFr6AlXov+SlcgB1Fv01y2kGe3Gc8nMW7VA0SrGuSkRfIEg==} + lodash@4.18.1: + resolution: {integrity: sha512-dMInicTPVE8d1e5otfwmmjlxkZoUpiVLwyeTdUsi/Caj/gfzzblBcCE5sRHV/AsjuCmxWrte2TNGSYuCeCq+0Q==} + longest-streak@3.1.0: resolution: {integrity: sha512-9Ri+o0JYgehTaVBBDoMqIl8GXtbWg711O3srftcHhZ0dqnETqLaoIK0x17fUw9rFSlK/0NlsKe0Ahhyl5pXE2g==} @@ -2905,6 +2951,9 @@ packages: uri-js@4.4.1: resolution: {integrity: sha512-7rKUyy33Q1yc98pQ1DAmLtwX109F7TIfWlW1Ydo8Wl1ii1SeHieeh0HHfPeL2fMXK6z0s8ecKs9frCuLJvndBg==} + utf8@3.0.0: + resolution: {integrity: sha512-E8VjFIQ/TyQgp+TZfS6l8yp/xWppSAHzidGiRrqe4bK4XP9pTRyKFgGJpO3SN7zdX4DeomTrwaseCHovfpFcqQ==} + valibot@1.4.2: resolution: {integrity: sha512-gjdCvJ6d3RyHAneqxMYMW9QMCwYMb3jpOO0IyHZV1bnRHFBHrX3VkIILt5XYR0WhwHiH7Mty8ovuPZ/O3gamrg==} peerDependencies: @@ -3419,6 +3468,27 @@ snapshots: globby: 11.1.0 read-yaml-file: 1.1.0 + '@microsoft/eslint-formatter-sarif@3.1.0': + dependencies: + jschardet: 3.1.4 + lodash: 4.18.1 + utf8: 3.0.0 + + '@microsoft/sarif-multitool-darwin@5.5.0': + optional: true + + '@microsoft/sarif-multitool-linux@5.5.0': + optional: true + + '@microsoft/sarif-multitool-win32@5.5.0': + optional: true + + '@microsoft/sarif-multitool@5.5.0': + optionalDependencies: + '@microsoft/sarif-multitool-darwin': 5.5.0 + '@microsoft/sarif-multitool-linux': 5.5.0 + '@microsoft/sarif-multitool-win32': 5.5.0 + '@napi-rs/wasm-runtime@0.2.12': dependencies: '@emnapi/core': 1.9.2 @@ -4569,6 +4639,8 @@ snapshots: dependencies: argparse: 2.0.1 + jschardet@3.1.4: {} + jsdoc-type-pratt-parser@7.2.0: {} jsesc@3.1.0: {} @@ -4667,6 +4739,8 @@ snapshots: lodash.startcase@4.4.0: {} + lodash@4.18.1: {} + longest-streak@3.1.0: {} lru-cache@11.5.1: {} @@ -5591,6 +5665,8 @@ snapshots: dependencies: punycode: 2.3.1 + utf8@3.0.0: {} + valibot@1.4.2(typescript@6.0.3): optionalDependencies: typescript: 6.0.3 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 95747309..cdf7f001 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -8,6 +8,8 @@ catalog: '@eslint/json': 2.0.1 '@eslint/markdown': 8.0.3 '@faker-js/faker': ^10.4.0 + '@microsoft/eslint-formatter-sarif': ^3.1.0 + '@microsoft/sarif-multitool': ^5.5.0 '@pnpm/lockfile.types': ^1100.0.5 '@prettier/plugin-xml': ^3.4.2 '@stylistic/eslint-plugin': 5.10.0 @@ -66,6 +68,13 @@ minimumReleaseAge: 4320 minimumReleaseAgeExclude: - '@gtbuchanan/*' +overrides: + # The SARIF formatter wrongly declares eslint v8 as a regular dependency + # (it only uses the host ESLint via require.main.require). Left in place, + # its v8 bin shadows the workspace's ESLint in packages that depend on + # the formatter. Remove it from the graph entirely. + '@microsoft/eslint-formatter-sarif>eslint': '-' + packages: - 'packages/*' diff --git a/turbo.json b/turbo.json index 49c9b0c9..dd3e9380 100644 --- a/turbo.json +++ b/turbo.json @@ -11,7 +11,8 @@ "!packages/*/**" ], "outputs": [ - "dist/.eslintcache" + "dist/.eslintcache", + "dist/eslint.sarif" ] }, "build": { @@ -127,7 +128,8 @@ "eslint.config.*" ], "outputs": [ - "dist/.eslintcache" + "dist/.eslintcache", + "dist/eslint.sarif" ] }, "pack": { From 317b765b77f3b6c9ee9a2f6b7a2c845a1bf139c7 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Sat, 18 Jul 2026 22:04:39 -0500 Subject: [PATCH 02/15] Switch lint gating to SARIF regression ratchet PRs now fail only on lint violations that are new relative to their merge base; pre-existing (accepted) violations never block. - lint:eslint drops --max-warnings=0 and becomes a reporter: warnings never fail it (fatals still do), so every commit produces a cacheable SARIF baseline. Local changed-file enforcement stays in the hk pre-commit eslint step and the IDE. - gtb lint:eslint:compare gains --base : it lints the merge base of that ref and HEAD in a throwaway git worktree to produce the baselines itself, so the same invocation works locally and in CI. A failing base lint is tolerated (pre-ratchet gtb wrote the SARIF log before exiting non-zero), and base commits that predate SARIF output just skip, which makes rollout graceful. - New lint-regression.yml reusable workflow wired into pr.yml: lints HEAD, compares against the merge base, posts an idempotent PR comment on failure, and honors a maintainer-applied override label (dismissed on every new push) as a run-but-neutral acceptance. - E2E coverage runs the real multitool against fixture SARIF logs. Per-test fixtures, not the file-scoped one: two compares sharing a project dir race on the matched-output write. Co-Authored-By: Claude Fable 5 --- .changeset/sarif-lint-regression-compare.md | 17 ++- .github/workflows/lint-regression.yml | 112 +++++++++++++++ .github/workflows/pr.yml | 6 + AGENTS.md | 52 ++++++- packages/cli/e2e/lint-compare.test.ts | 103 ++++++++++++++ .../src/commands/root/lint-eslint-compare.ts | 10 +- packages/cli/src/commands/task/lint-eslint.ts | 13 +- packages/cli/src/lib/lint-compare.ts | 90 +++++++++++- packages/cli/test/lint-compare.test.ts | 132 ++++++++++++++++-- 9 files changed, 505 insertions(+), 30 deletions(-) create mode 100644 .github/workflows/lint-regression.yml create mode 100644 packages/cli/e2e/lint-compare.test.ts diff --git a/.changeset/sarif-lint-regression-compare.md b/.changeset/sarif-lint-regression-compare.md index 7802693e..0b2396eb 100644 --- a/.changeset/sarif-lint-regression-compare.md +++ b/.changeset/sarif-lint-regression-compare.md @@ -2,9 +2,14 @@ '@gtbuchanan/cli': minor --- -Add SARIF-based lint regression tooling: `lint:eslint` now writes a SARIF -log (`dist/eslint.sarif`) alongside its console output via a bundled -formatter, and the new `gtb lint:eslint:compare` command diffs each lint -cwd's current SARIF log against a baseline -(`dist/eslint-base.sarif`) using `sarif-multitool match-results-forward`, -failing only on violations not present in the baseline. +Rework lint enforcement around SARIF baselining. `lint:eslint` is now a +reporter: it writes `dist/eslint.sarif` via a bundled formatter, prints +compact console output, and no longer fails on warnings (fatal errors +still fail). Enforcement moves to the new `gtb lint:eslint:compare` +command, which classifies each violation against a baseline SARIF log +using `sarif-multitool` and fails only on violations not present in the +baseline. With `--base ` it produces the baseline itself by linting +the merge base in a temporary git worktree, so the same command works +locally and in CI (see the new `lint-regression.yml` reusable workflow). +Local changed-file enforcement via the pre-commit ESLint step is +unaffected. diff --git a/.github/workflows/lint-regression.yml b/.github/workflows/lint-regression.yml new file mode 100644 index 00000000..8a4b9b60 --- /dev/null +++ b/.github/workflows/lint-regression.yml @@ -0,0 +1,112 @@ +--- +env: + # GitHub Actions has no TTY, so force color; Actions renders the ANSI. + FORCE_COLOR: '1' + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true + +jobs: + run: + name: Lint Regression + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + with: + # The compare lints the merge base in a throwaway worktree, so + # the base branch history must be reachable locally. + fetch-depth: 0 + + - uses: gtbuchanan/tooling/.github/actions/mise-setup@main + + - uses: gtbuchanan/tooling/.github/actions/pnpm-tasks@main + + # A label only vouches for the commits it was applied to. Drop it + # on every new push so a maintainer has to re-accept. + - env: + GH_TOKEN: ${{ github.token }} + PR: ${{ github.event.pull_request.number }} + if: >- + github.event.action == 'synchronize' && + contains(github.event.pull_request.labels.*.name, inputs.override-label) + name: Dismiss stale regression acceptance + run: gh pr edit "$PR" --remove-label '${{ inputs.override-label }}' + + # Produce this commit's SARIF logs. Turbo dedupes against the CI + # workflow's lint run via the shared remote cache when configured. + - name: Lint HEAD + run: >- + ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} + turbo run lint + + - continue-on-error: true + env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + id: compare + name: Compare lint results against the merge base + run: | + ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} \ + lint:eslint:compare --base "origin/$BASE_REF" 2>&1 \ + | tee lint-regression.txt + exit "${PIPESTATUS[0]}" + + # Fork PRs run with a read-only token; the check failure alone + # carries the signal there. + - env: + GH_TOKEN: ${{ github.token }} + PR: ${{ github.event.pull_request.number }} + if: >- + steps.compare.outcome == 'failure' && + !github.event.pull_request.head.repo.fork + name: Report new violations on the PR + run: | + { + echo '### New lint violations relative to the merge base' + echo + echo '```text' + cat lint-regression.txt + echo '```' + echo + echo 'Apply the `${{ inputs.override-label }}` label to accept.' + } > comment.md + gh pr comment "$PR" --body-file comment.md \ + --edit-last --create-if-none + + # Run-but-neutral override: the label lets the check pass, but the + # comment above still records what was accepted. A label present + # on a `synchronize` run was just dismissed, so it doesn't count. + - if: >- + steps.compare.outcome == 'failure' && + (github.event.action == 'synchronize' || + !contains(github.event.pull_request.labels.*.name, inputs.override-label)) + name: Enforce + run: exit 1 + +name: Lint Regression + +# Reusable: fail a PR only on lint violations that are new relative to +# its merge base. `gtb lint:eslint:compare --base` lints the merge base +# in a temporary git worktree to build the SARIF baseline, then +# `sarif-multitool` classifies each HEAD result as new or matched — +# pre-existing (accepted) violations never block. A maintainer-applied +# override label turns a failing compare into a pass while the PR +# comment keeps the accepted violations on record. +on: + workflow_call: + inputs: + gtb-from-source: + default: false + description: >- + Run gtb from the workspace source (`pnpm run gtb`) instead of + the installed bin (`pnpm exec gtb`). Set true only by the repo + that vendors `@gtbuchanan/cli` as a workspace package (tooling + itself). Consumers leave it false. + type: boolean + override-label: + default: accepted-lint-regression + description: >- + PR label that accepts the new violations (maintainer-applied; + dismissed automatically on every new push). + type: string + +permissions: + contents: read + pull-requests: write diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index b2748e7c..8ae690f6 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -17,6 +17,12 @@ jobs: with: config-file: .github/dependency-review-config.yml + lint-regression: + name: Lint + uses: ./.github/workflows/lint-regression.yml + with: + gtb-from-source: true + pre-commit: name: Pre-Commit # This repo's eslint step shells out to `pnpm exec eslint`, so hk diff --git a/AGENTS.md b/AGENTS.md index c578ea2a..be6f7695 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,7 +25,8 @@ mise.toml — Pin dev-tool versions for local + CI; postinstall hoo changeset-check.yml — Reusable: verify a changeset exists ci.yml — Reusable: build + slow + e2e + coverage dependency-review.yml — Reusable: scan dep changes (vulns + licenses) - pr.yml — Pipeline (PR): ci + changeset + deps + pre-commit + lint-regression.yml — Reusable: fail PRs only on lint violations new vs. the merge base (SARIF diff) + pr.yml — Pipeline (PR): ci + changeset + deps + lint regression + pre-commit pre-commit.yml — Reusable: run hk hooks release.yml — Pipeline (push): CI gate + CD packages/ @@ -87,6 +88,37 @@ coverage, setupFiles, and mock reset. - **Vitest** — Per-package `vitest.config.ts` using `configurePackage()` from `@gtbuchanan/vitest-config/configure`. +### SARIF lint baselining + +Lint enforcement is a ratchet: PRs may not introduce _new_ ESLint +violations, while pre-existing (accepted) ones never block. Three +layers: + +- **`lint:eslint` is a reporter, not a gate.** It writes a SARIF log to + `dist/eslint.sarif` (a turbo task output) via a formatter bundled in + `@gtbuchanan/cli` and prints compact console output. Warnings never + fail it — the eslint-config downgrades every rule to a warning — but + fatal errors (parse/config breakage) still do. +- **`gtb lint:eslint:compare` is the gate.** It diffs each lint cwd's + `dist/eslint.sarif` against `dist/eslint-base.sarif` with + `sarif-multitool match-results-forward` and fails only on results + classified `new`. Matching is fingerprint/content-based, so baseline + violations that merely moved stay matched — no git-diff offset math. + `--base ` makes it self-sufficient: it lints the merge base of + that ref and HEAD in a throwaway git worktree to produce the + baselines, so the same invocation works locally + (`gtb lint:eslint:compare --base origin/main`) and in CI + (`lint-regression.yml`). Dirs whose base commit wrote no SARIF are + skipped with a warning, which is what makes rollout graceful. +- **Changed-file enforcement stays local.** The hk pre-commit `eslint` + step keeps `--max-warnings=0` on staged files, and the IDE shows + warnings inline — new violations are still caught at authoring time; + the ratchet only governs what lands. + +Accepted violations enter the baseline by merging with the override +label (see `lint-regression.yml`) and surface in every SARIF log until +paid down. + ### Pre-commit hooks - **hk** — Rust pre-commit hook runner configured in Pkl (`hk.pkl`). No @@ -202,12 +234,13 @@ Two **pipeline** workflows own this repo's triggers and define the work as peer jobs, each calling a single-concern **reusable** workflow: - **`pr.yml`** (on `pull_request`) — jobs `CI`, `Changeset`, - `Dependencies`, `Pre-Commit`. + `Dependencies`, `Lint`, `Pre-Commit`. - **`release.yml`** (on `push` to main) — jobs `CI` (gate), `CD` (`needs: ci`). `CI` and `CD` are peers. The reusables (`workflow_call`-only): `ci.yml`, `cd.yml`, -`changeset-check.yml`, `dependency-review.yml`, `pre-commit.yml`. +`changeset-check.yml`, `dependency-review.yml`, `lint-regression.yml`, +`pre-commit.yml`. Consumers copy `pr.yml` / `release.yml`, swapping `./` for `gtbuchanan/tooling/.github/workflows/@main`: @@ -231,6 +264,9 @@ jobs: dependencies: name: Dependencies uses: gtbuchanan/tooling/.github/workflows/dependency-review.yml@main + lint-regression: + name: Lint + uses: gtbuchanan/tooling/.github/workflows/lint-regression.yml@main pre-commit: name: Pre-Commit uses: gtbuchanan/tooling/.github/workflows/pre-commit.yml@main @@ -356,6 +392,16 @@ through `package.json` scripts backed by `gtb` leaf commands. to ecosystems it indexes (npm + Actions here), so mise tools and `hk.pkl` steps aren't covered — Renovate's managers handle those independently. +- **`lint-regression.yml`** — Fails a PR only on lint violations that + are new relative to its merge base (the ratchet gate; see the SARIF + lint baselining section). Lints HEAD, then runs + `gtb lint:eslint:compare --base origin/`, which lints the + merge base in a throwaway git worktree and diffs the SARIF logs via + `sarif-multitool`. A maintainer-applied override label (default + `accepted-lint-regression`, dismissed on every new push) turns a + failing compare into a pass while a PR comment records the accepted + violations. Caller must grant `pull-requests: write`; fork PRs get + the check failure without the comment (read-only token). - **`pre-commit.yml`** — Runs the `hk:base` mise task on PR changed files (hk resolved from mise). The `use-pnpm` input (default `false`) opts into `pnpm install` for steps that shell out to the diff --git a/packages/cli/e2e/lint-compare.test.ts b/packages/cli/e2e/lint-compare.test.ts new file mode 100644 index 00000000..9c1b7252 --- /dev/null +++ b/packages/cli/e2e/lint-compare.test.ts @@ -0,0 +1,103 @@ +import path from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { type ProjectFixture, createProjectFixture } from '@gtbuchanan/test-utils'; +import { describe, it } from 'vitest'; + +const createFixture = (): ProjectFixture => + createProjectFixture({ + packageName: '@gtbuchanan/cli', + }); + +const jsonIndent = 2; + +interface SarifViolation { + readonly line: number; + readonly message: string; + readonly ruleId: string; + readonly snippet: string; +} + +/** Builds a SARIF log in the shape the bundled ESLint formatter emits. */ +const sarifLog = (fileUri: string, violations: readonly SarifViolation[]): string => + `${JSON.stringify({ + $schema: 'https://json.schemastore.org/sarif-2.1.0-rtm.5', + runs: [{ + artifacts: [{ location: { uri: fileUri } }], + results: violations.map(violation => ({ + level: 'warning', + locations: [{ + physicalLocation: { + artifactLocation: { index: 0, uri: fileUri }, + region: { + snippet: { text: violation.snippet }, + startColumn: 1, + startLine: violation.line, + }, + }, + }], + message: { text: violation.message }, + ruleId: violation.ruleId, + })), + tool: { driver: { name: 'ESLint', rules: [] } }, + }], + version: '2.1.0', + }, undefined, jsonIndent)}\n`; + +const existing: SarifViolation = { + line: 2, + message: "'unused' is assigned a value but never used.", + ruleId: 'no-unused-vars', + snippet: 'const unused = 1;', +}; + +const source = 'export const app = () => {\n const unused = 1;\n console.log("hi");\n};\n'; + +describe.concurrent('gtb lint:eslint:compare', () => { + it('passes when every violation is in the baseline', async ({ expect }) => { + using fixture = createFixture(); + const file = fixture.writeFile(path.join('src', 'app.js'), source); + const uri = pathToFileURL(file).href; + fixture.writeFile(path.join('dist', 'eslint-base.sarif'), sarifLog(uri, [existing])); + fixture.writeFile(path.join('dist', 'eslint.sarif'), sarifLog(uri, [existing])); + + const result = await fixture.run('gtb', ['lint:eslint:compare']); + + expect(result).toMatchObject({ exitCode: 0 }); + expect(result.stdout).toContain('No new lint violations'); + }); + + it('fails on a violation missing from the baseline', async ({ expect }) => { + using fixture = createFixture(); + const file = fixture.writeFile(path.join('src', 'app.js'), source); + const uri = pathToFileURL(file).href; + const added: SarifViolation = { + line: 3, + message: 'Unexpected console statement.', + ruleId: 'no-console', + snippet: 'console.log("hi");', + }; + fixture.writeFile(path.join('dist', 'eslint-base.sarif'), sarifLog(uri, [existing])); + fixture.writeFile( + path.join('dist', 'eslint.sarif'), + sarifLog(uri, [existing, added]), + ); + + const result = await fixture.run('gtb', ['lint:eslint:compare']); + + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toContain('no-console'); + expect(result.stderr).not.toContain('no-unused-vars'); + }); + + it('skips and passes when no baseline exists', async ({ expect }) => { + using fixture = createFixture(); + const file = fixture.writeFile(path.join('src', 'app.js'), source); + const uri = pathToFileURL(file).href; + fixture.writeFile(path.join('dist', 'eslint.sarif'), sarifLog(uri, [existing])); + + const result = await fixture.run('gtb', ['lint:eslint:compare']); + + expect(result).toMatchObject({ exitCode: 0 }); + expect(result.stderr).toContain('No baseline SARIF'); + }); +}); diff --git a/packages/cli/src/commands/root/lint-eslint-compare.ts b/packages/cli/src/commands/root/lint-eslint-compare.ts index 01cbba5e..84e909cb 100644 --- a/packages/cli/src/commands/root/lint-eslint-compare.ts +++ b/packages/cli/src/commands/root/lint-eslint-compare.ts @@ -9,9 +9,17 @@ import { rootNames } from './names.ts'; * same task output, which turbo's task model can't express. */ export const lintEslintCompare = defineCommand({ + args: { + base: { + description: + 'Git ref to diff against; lints its merge base with HEAD in a ' + + 'temporary worktree to produce the baseline (e.g. origin/main)', + type: 'string', + }, + }, meta: { description: 'Fail when lint violations are new relative to the SARIF baseline', name: rootNames.lintEslintCompare, }, - run: () => executeLintEslintCompare(), + run: ({ args }) => executeLintEslintCompare({ baseRef: args.base }), }); diff --git a/packages/cli/src/commands/task/lint-eslint.ts b/packages/cli/src/commands/task/lint-eslint.ts index a9b3e020..43b3db24 100644 --- a/packages/cli/src/commands/task/lint-eslint.ts +++ b/packages/cli/src/commands/task/lint-eslint.ts @@ -11,17 +11,24 @@ const formatterPath = fileURLToPath( import.meta.resolve('@gtbuchanan/cli/eslint-sarif-formatter'), ); -/** Runs ESLint with caching and zero-warning threshold. */ +/** + * Runs ESLint as a reporter, not a gate: warnings never fail the task + * (the repo convention downgrades every rule to a warning), while fatal + * errors — parse or config breakage — still do. Enforcement lives in + * the changed-files pre-commit step locally and in + * `gtb lint:eslint:compare` (new-violations-only) in CI, so a baseline + * SARIF log exists for every commit, including ones carrying accepted + * violations. + */ export const lintEslint = defineCommand({ meta: { - description: 'Run ESLint with caching and --max-warnings=0', + description: 'Run ESLint with caching, reporting to dist/eslint.sarif', name: 'lint:eslint', }, run: async ({ rawArgs }) => { await run('eslint', { args: [ '--cache', '--cache-location', 'dist/.eslintcache', - '--max-warnings=0', '--format', formatterPath, ...rawArgs, ], diff --git a/packages/cli/src/lib/lint-compare.ts b/packages/cli/src/lib/lint-compare.ts index 733ed0e6..f3150e3d 100644 --- a/packages/cli/src/lib/lint-compare.ts +++ b/packages/cli/src/lib/lint-compare.ts @@ -1,10 +1,11 @@ -import { existsSync } from 'node:fs'; +import { copyFileSync, existsSync, mkdirSync, mkdtempSync } from 'node:fs'; import { createRequire } from 'node:module'; +import { tmpdir } from 'node:os'; import path from 'node:path'; import * as v from 'valibot'; import { readJsonFile } from './file-writer.ts'; import { type Logger, createLogger } from './logger.ts'; -import { type RunOptions, run } from './process.ts'; +import { type RunOptions, capture, run } from './process.ts'; import { type WorkspaceContext, resolveWorkspace } from './workspace.ts'; /** SARIF artifact filenames under a lint cwd's `dist/`. */ @@ -120,16 +121,19 @@ export const formatNewViolations = ( /** * Side-effecting I/O the compare depends on. Injected so the - * orchestration (per-package matching, gating) is unit-testable without - * spawning the SARIF multitool. + * orchestration (baseline production, per-package matching, gating) is + * unit-testable without spawning git, turbo, or the SARIF multitool. */ export interface LintCompareDeps { + readonly capture: (command: string, args: readonly string[]) => Promise; + readonly copyFile: (source: string, destination: string) => void; readonly exists: (filePath: string) => boolean; readonly logger: Logger; + readonly makeTempDir: () => string; readonly readJson: (filePath: string) => unknown; readonly resolveMultitool: () => string; readonly run: (command: string, options?: RunOptions) => Promise; - readonly workspace: () => WorkspaceContext; + readonly workspace: (cwd?: string) => WorkspaceContext; } /** @@ -144,12 +148,71 @@ const resolveMultitoolBinary = (): string => { }; const defaultDeps: LintCompareDeps = { + capture, + copyFile: (source, destination) => { + mkdirSync(path.dirname(destination), { recursive: true }); + copyFileSync(source, destination); + }, exists: existsSync, logger: createLogger(), + makeTempDir: () => mkdtempSync(path.join(tmpdir(), 'gtb-lint-base-')), readJson: readJsonFile, resolveMultitool: resolveMultitoolBinary, run, - workspace: resolveWorkspace, + workspace: cwd => resolveWorkspace(cwd === undefined ? undefined : { cwd }), +}; + +/** + * Produces per-package baseline SARIF logs by linting the merge base in + * a throwaway git worktree and copying each `dist/eslint.sarif` into + * the corresponding head package as `dist/eslint-base.sarif`. A failing + * base lint is tolerated: the SARIF log is written before ESLint + * exits, and a baseline carrying violations is exactly what the ratchet + * diffs against. Base commits that predate SARIF output simply produce + * no baseline, and the compare skips those packages. + */ +export const produceBaseline = async ( + baseRef: string, + deps: LintCompareDeps, +): Promise => { + const sha = await deps.capture('git', ['merge-base', baseRef, 'HEAD']); + const baseDir = deps.makeTempDir(); + try { + await deps.run('git', { args: ['worktree', 'add', '--detach', baseDir, sha] }); + await deps.run('pnpm', { + args: ['install', '--frozen-lockfile', '--prefer-offline'], + cwd: baseDir, + }); + try { + await deps.run('pnpm', { + args: ['exec', 'turbo', 'run', 'lint', '--output-logs=errors-only'], + cwd: baseDir, + }); + } catch { + // Pre-ratchet gtb fails lint on warnings after writing the SARIF log. + deps.logger.error(`Base lint at ${sha} failed; using whatever SARIF it wrote`); + } + copyBaselineSarifs(baseDir, deps); + } finally { + await deps.run('git', { args: ['worktree', 'remove', '--force', baseDir] }); + } +}; + +const copyBaselineSarifs = (baseDir: string, deps: LintCompareDeps): void => { + const base = deps.workspace(baseDir); + const head = deps.workspace(); + const dirs = new Set([base.rootDir, ...base.packageDirs]); + for (const dir of dirs) { + const source = path.join(dir, 'dist', sarifFileNames.current); + if (!deps.exists(source)) { + continue; + } + const relative = path.relative(base.rootDir, dir); + const destination = path.join( + head.rootDir, relative, 'dist', sarifFileNames.base, + ); + deps.copyFile(source, destination); + } }; const matchDirForward = async ( @@ -180,6 +243,17 @@ const matchDirForward = async ( return extractNewViolations(log); }; +/** Options for {@link executeLintEslintCompare}. */ +export interface LintEslintCompareOptions { + /** + * Git ref to diff against. When set, the baseline is produced by + * linting the merge base of this ref and HEAD in a throwaway + * worktree. When unset, `dist/eslint-base.sarif` files must already + * be in place (e.g. restored from a cache or a prior run). + */ + readonly baseRef?: string | undefined; +} + /** * Compares each lint cwd's current SARIF log against its baseline via * `sarif-multitool match-results-forward` and rejects when any result @@ -188,8 +262,12 @@ const matchDirForward = async ( * matched — only genuine regressions gate. */ export const executeLintEslintCompare = async ( + options: LintEslintCompareOptions = {}, deps: LintCompareDeps = defaultDeps, ): Promise => { + if (options.baseRef !== undefined) { + await produceBaseline(options.baseRef, deps); + } const { packageDirs, rootDir } = deps.workspace(); const lintDirs = [...new Set([rootDir, ...packageDirs])]; const violations: NewViolation[] = []; diff --git a/packages/cli/test/lint-compare.test.ts b/packages/cli/test/lint-compare.test.ts index aab2dccd..320b89f8 100644 --- a/packages/cli/test/lint-compare.test.ts +++ b/packages/cli/test/lint-compare.test.ts @@ -7,6 +7,7 @@ import { extractNewViolations, formatNewViolations, parseSarifLog, + produceBaseline, sarifFileNames, } from '#src/lib/lint-compare.js'; import type { Logger } from '#src/lib/logger.js'; @@ -37,24 +38,39 @@ interface RunCall { readonly command: string; } +interface CopyCall { + readonly destination: string; + readonly source: string; +} + interface StubDeps { + readonly copyCalls: readonly CopyCall[]; readonly deps: LintCompareDeps; readonly errors: readonly string[]; readonly infos: readonly string[]; readonly runCalls: readonly RunCall[]; } +interface StubOptions { + /** Workspace returned for the base worktree cwd. Defaults to `workspace`. */ + readonly baseWorkspace?: WorkspaceContext; + /** `" "` prefixes whose run() rejects. */ + readonly failing?: readonly string[]; +} + const normalize = (filePath: string): string => filePath.replaceAll('\\', '/'); /** - * Fakes a workspace whose SARIF files are the keys of `files`; reading - * any matched path yields `matched`. + * Fakes a workspace whose SARIF files are the members of `files`; + * reading any matched path yields `matched`. */ const stubDeps = ( workspace: WorkspaceContext, files: readonly string[], matched: object, + options?: StubOptions, ): StubDeps => { + const copyCalls: CopyCall[] = []; const errors: string[] = []; const infos: string[] = []; const runCalls: RunCall[] = []; @@ -63,16 +79,29 @@ const stubDeps = ( info: (...args) => void infos.push(args.join(' ')), }; return { + copyCalls, deps: { + capture: () => Promise.resolve('abc1234'), + copyFile: (source, destination) => { + copyCalls.push({ + destination: normalize(destination), + source: normalize(source), + }); + }, exists: filePath => files.includes(normalize(filePath)), logger, + makeTempDir: () => '/tmp/base', readJson: () => matched, resolveMultitool: () => 'sarif-multitool', - run: (command, options) => { - runCalls.push({ args: options?.args ?? [], command }); - return Promise.resolve(); + run: (command, runOptions) => { + runCalls.push({ args: runOptions?.args ?? [], command }); + const key = `${command} ${runOptions?.args?.[0] ?? ''}`; + return options?.failing?.includes(key) === true + ? Promise.reject(new Error(`${key} failed`)) + : Promise.resolve(); }, - workspace: () => workspace, + workspace: cwd => + (cwd === undefined ? workspace : options?.baseWorkspace ?? workspace), }, errors, infos, @@ -158,7 +187,7 @@ describe.concurrent(executeLintEslintCompare, () => { it('matches every lint dir forward against its baseline', async ({ expect }) => { const { deps, runCalls } = stubDeps(workspace, currentFiles, sarifLog([])); - await executeLintEslintCompare(deps); + await executeLintEslintCompare({}, deps); expect(runCalls).toHaveLength(2); expect(runCalls[0]).toMatchObject({ command: 'sarif-multitool' }); @@ -172,7 +201,7 @@ describe.concurrent(executeLintEslintCompare, () => { sarifLog([sarifResult('unchanged'), sarifResult('updated')]), ); - await executeLintEslintCompare(deps); + await executeLintEslintCompare({}, deps); expect(infos).toContain('No new lint violations'); }); @@ -180,7 +209,7 @@ describe.concurrent(executeLintEslintCompare, () => { it('rejects when any result is new', async ({ expect }) => { const { deps } = stubDeps(workspace, currentFiles, sarifLog([sarifResult('new')])); - await expect(executeLintEslintCompare(deps)).rejects.toThrow( + await expect(executeLintEslintCompare({}, deps)).rejects.toThrow( /new lint violation/v, ); }); @@ -188,7 +217,7 @@ describe.concurrent(executeLintEslintCompare, () => { it('skips dirs with no current SARIF log', async ({ expect }) => { const { deps, runCalls } = stubDeps(workspace, [], sarifLog([])); - await executeLintEslintCompare(deps); + await executeLintEslintCompare({}, deps); expect(runCalls).toHaveLength(0); }); @@ -200,9 +229,90 @@ describe.concurrent(executeLintEslintCompare, () => { sarifLog([]), ); - await executeLintEslintCompare(deps); + await executeLintEslintCompare({}, deps); expect(runCalls).toHaveLength(0); expect(errors.some(line => line.includes('No baseline SARIF'))).toBe(true); }); }); + +describe.concurrent(produceBaseline, () => { + const headWorkspace: WorkspaceContext = { + packageDirs: ['/repo/packages/a'], + packageGlobs: ['packages/*'], + rootDir: '/repo', + }; + const baseWorkspace: WorkspaceContext = { + packageDirs: ['/tmp/base/packages/a'], + packageGlobs: ['packages/*'], + rootDir: '/tmp/base', + }; + const baseFiles = [ + `/tmp/base/dist/${sarifFileNames.current}`, + `/tmp/base/packages/a/dist/${sarifFileNames.current}`, + ]; + + it('lints the merge base in a temp worktree and copies baselines', async ({ + expect, + }) => { + const { copyCalls, deps, runCalls } = stubDeps( + headWorkspace, baseFiles, sarifLog([]), { baseWorkspace }, + ); + + await produceBaseline('origin/main', deps); + + expect(runCalls.map(call => `${call.command} ${call.args[0] ?? ''}`)).toStrictEqual([ + 'git worktree', + 'pnpm install', + 'pnpm exec', + 'git worktree', + ]); + expect(runCalls[0]?.args).toContain('abc1234'); + expect(copyCalls).toStrictEqual([ + { + destination: `/repo/dist/${sarifFileNames.base}`, + source: `/tmp/base/dist/${sarifFileNames.current}`, + }, + { + destination: `/repo/packages/a/dist/${sarifFileNames.base}`, + source: `/tmp/base/packages/a/dist/${sarifFileNames.current}`, + }, + ]); + }); + + it('tolerates a failing base lint and copies what it wrote', async ({ expect }) => { + const { copyCalls, deps, errors } = stubDeps( + headWorkspace, baseFiles, sarifLog([]), + { baseWorkspace, failing: ['pnpm exec'] }, + ); + + await produceBaseline('origin/main', deps); + + expect(errors.some(line => line.includes('Base lint'))).toBe(true); + expect(copyCalls).toHaveLength(2); + }); + + it('copies nothing when the base wrote no SARIF logs', async ({ expect }) => { + const { copyCalls, deps } = stubDeps( + headWorkspace, [], sarifLog([]), { baseWorkspace }, + ); + + await produceBaseline('origin/main', deps); + + expect(copyCalls).toHaveLength(0); + }); + + it('removes the worktree when the base install fails', async ({ expect }) => { + const { deps, runCalls } = stubDeps( + headWorkspace, baseFiles, sarifLog([]), + { baseWorkspace, failing: ['pnpm install'] }, + ); + + await expect(produceBaseline('origin/main', deps)).rejects.toThrow( + /pnpm install failed/v, + ); + expect(runCalls.at(-1)?.args).toStrictEqual([ + 'worktree', 'remove', '--force', '/tmp/base', + ]); + }); +}); From 93072d6e69780fdf939aff231a69c2ead9f1f3da Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Mon, 20 Jul 2026 16:46:02 -0500 Subject: [PATCH 03/15] Generalize SARIF gate: tool-agnostic layout, caching, seeding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The compare machinery never inspects anything ESLint-specific, so stop naming it that way, and stop scattering loose artifacts in dist/. - SARIF artifacts move to a dedicated dist/sarif/ tree: .sarif from reporters, base/.sarif baselines, matched/.sarif multitool output, and base.ref (the merge-base stamp, root only). The compare pairs every dist/sarif/*.sarif with its baseline by filename, so a future reporter is gated by dropping a file there. - Commands become a `gtb sarif` hub (compare/baseline), reserving colon names for turbo tasks. `sarif compare --base` skips baseline production when the base.ref stamp already records the merge base — repeat local runs drop from ~35s to multitool time, and CI restores the stamped baselines from an actions/cache keyed on the merge-base SHA. release.yml seeds that cache cross-PR from each main commit via `gtb sarif baseline` (merge base == HEAD, so the baseline is the commit's own reporter output; pure file copy). - A missing baseline is an empty baseline, not a pass: every finding in an unbaselined log is new and needs acceptance, so the bootstrap PR and newly added reporters can't slip findings in silently. Findings carrying in-source suppressions are exempt — a reasoned suppression is already the accepted mechanism — but stay in the logs for visibility. - The multitool won't create its output file's parent directory; ensure dist/sarif/matched/ exists before matching. Co-Authored-By: Claude Fable 5 --- .changeset/sarif-lint-regression-compare.md | 28 +- .github/workflows/lint-regression.yml | 66 ++- .github/workflows/release.yml | 11 + AGENTS.md | 48 ++- ...-compare.test.ts => sarif-compare.test.ts} | 33 +- packages/cli/src/commands/index.ts | 4 +- .../src/commands/root/lint-eslint-compare.ts | 25 -- packages/cli/src/commands/root/names.ts | 2 +- packages/cli/src/commands/root/sarif.ts | 49 +++ packages/cli/src/commands/task/lint-eslint.ts | 2 +- .../cli/src/lib/eslint-sarif-formatter.ts | 3 +- packages/cli/src/lib/lint-compare.ts | 286 ------------- packages/cli/src/lib/sarif-compare.ts | 387 ++++++++++++++++++ packages/cli/src/lib/sarif-paths.ts | 20 + packages/cli/src/lib/turbo-config.ts | 4 +- packages/cli/test/lint-compare.test.ts | 318 -------------- packages/cli/test/sarif-baseline.test.ts | 196 +++++++++ packages/cli/test/sarif-compare.stub.ts | 146 +++++++ packages/cli/test/sarif-compare.test.ts | 168 ++++++++ turbo.json | 4 +- 20 files changed, 1118 insertions(+), 682 deletions(-) rename packages/cli/e2e/{lint-compare.test.ts => sarif-compare.test.ts} (72%) delete mode 100644 packages/cli/src/commands/root/lint-eslint-compare.ts create mode 100644 packages/cli/src/commands/root/sarif.ts delete mode 100644 packages/cli/src/lib/lint-compare.ts create mode 100644 packages/cli/src/lib/sarif-compare.ts create mode 100644 packages/cli/src/lib/sarif-paths.ts delete mode 100644 packages/cli/test/lint-compare.test.ts create mode 100644 packages/cli/test/sarif-baseline.test.ts create mode 100644 packages/cli/test/sarif-compare.stub.ts create mode 100644 packages/cli/test/sarif-compare.test.ts diff --git a/.changeset/sarif-lint-regression-compare.md b/.changeset/sarif-lint-regression-compare.md index 0b2396eb..3ee62cce 100644 --- a/.changeset/sarif-lint-regression-compare.md +++ b/.changeset/sarif-lint-regression-compare.md @@ -2,14 +2,20 @@ '@gtbuchanan/cli': minor --- -Rework lint enforcement around SARIF baselining. `lint:eslint` is now a -reporter: it writes `dist/eslint.sarif` via a bundled formatter, prints -compact console output, and no longer fails on warnings (fatal errors -still fail). Enforcement moves to the new `gtb lint:eslint:compare` -command, which classifies each violation against a baseline SARIF log -using `sarif-multitool` and fails only on violations not present in the -baseline. With `--base ` it produces the baseline itself by linting -the merge base in a temporary git worktree, so the same command works -locally and in CI (see the new `lint-regression.yml` reusable workflow). -Local changed-file enforcement via the pre-commit ESLint step is -unaffected. +Rework lint enforcement around tool-agnostic SARIF baselining. +`lint:eslint` is now a reporter: it writes `dist/sarif/eslint.sarif` +via a bundled formatter, prints compact console output, and no longer +fails on warnings (fatal errors still fail). Enforcement moves to the +new `gtb sarif compare` command, which pairs every `dist/sarif/*.sarif` +with `dist/sarif/base/.sarif` and fails only on findings the +`sarif-multitool` baseliner classifies as new — any tool that drops a +SARIF log into `dist/sarif/` is gated with no extra wiring. A missing +baseline counts as empty (all findings new), and findings carrying +in-source suppressions are exempt. With +`--base ` it produces the baseline itself by linting the merge +base in a temporary git worktree, skipping production when the +`dist/sarif/base.ref` stamp already records that merge base (locally or +via CI caches). `gtb sarif baseline` snapshots HEAD's logs as the +baseline so default-branch CI can seed a cross-PR cache (see the new +`lint-regression.yml` reusable workflow). Local changed-file +enforcement via the pre-commit ESLint step is unaffected. diff --git a/.github/workflows/lint-regression.yml b/.github/workflows/lint-regression.yml index 8a4b9b60..e02e4e43 100644 --- a/.github/workflows/lint-regression.yml +++ b/.github/workflows/lint-regression.yml @@ -6,6 +6,7 @@ env: jobs: run: + if: ${{ !inputs.seed }} name: Lint Regression runs-on: ubuntu-latest steps: @@ -37,6 +38,24 @@ jobs: ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} turbo run lint + - env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + id: merge-base + name: Resolve the merge base + run: echo "sha=$(git merge-base "origin/$BASE_REF" HEAD)" >> "$GITHUB_OUTPUT" + + # Restore baselines produced by an earlier run against the same + # merge base; the compare's stamp check skips the worktree lint on + # a hit. Exact key only — a near-miss baseline is a wrong baseline + # (the stamp check would reject it anyway). + - uses: actions/cache@v6 + with: + key: lint-baseline-${{ steps.merge-base.outputs.sha }} + path: | + **/dist/sarif/base/** + !**/node_modules/** + dist/sarif/base.ref + - continue-on-error: true env: BASE_REF: ${{ github.event.pull_request.base.ref }} @@ -44,7 +63,7 @@ jobs: name: Compare lint results against the merge base run: | ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} \ - lint:eslint:compare --base "origin/$BASE_REF" 2>&1 \ + sarif compare --base "origin/$BASE_REF" 2>&1 \ | tee lint-regression.txt exit "${PIPESTATUS[0]}" @@ -80,10 +99,46 @@ jobs: name: Enforce run: exit 1 + # On the default branch, any future PR's merge base is HEAD itself, + # so its baseline is just this commit's own lint output. Caches + # created on the default branch are readable from every PR, so this + # seeds a cross-PR baseline cache — PR compare runs restore it and + # skip merge-base production entirely. + seed: + if: ${{ inputs.seed }} + name: Lint Baseline Seed + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - uses: gtbuchanan/tooling/.github/actions/mise-setup@main + + - uses: gtbuchanan/tooling/.github/actions/pnpm-tasks@main + + # Cache-hits against the CI workflow's lint run of this commit + # when the turbo remote cache is configured. + - name: Lint HEAD + run: >- + ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} + turbo run lint + + - name: Snapshot baselines + run: >- + ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} + sarif baseline + + - uses: actions/cache/save@v6 + with: + key: lint-baseline-${{ github.sha }} + path: | + **/dist/sarif/base/** + !**/node_modules/** + dist/sarif/base.ref + name: Lint Regression # Reusable: fail a PR only on lint violations that are new relative to -# its merge base. `gtb lint:eslint:compare --base` lints the merge base +# its merge base. `gtb sarif compare --base` lints the merge base # in a temporary git worktree to build the SARIF baseline, then # `sarif-multitool` classifies each HEAD result as new or matched — # pre-existing (accepted) violations never block. A maintainer-applied @@ -106,6 +161,13 @@ on: PR label that accepts the new violations (maintainer-applied; dismissed automatically on every new push). type: string + seed: + default: false + description: >- + Snapshot HEAD's lint output as the cross-PR baseline cache + instead of gating. Set on default-branch pushes; the PR gate + restores the cache keyed on its merge-base SHA. + type: boolean permissions: contents: read diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a85d7c2b..1b48c6e1 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -18,6 +18,17 @@ jobs: secrets: inherit uses: ./.github/workflows/ci.yml + # Seed the cross-PR lint baseline cache from this commit's own lint + # output. `needs: ci` lets the seed's lint hit the remote cache that + # CI just warmed instead of re-linting. + lint-baseline: + name: Lint + needs: ci + uses: ./.github/workflows/lint-regression.yml + with: + gtb-from-source: true + seed: true + name: Release # The main-branch pipeline: re-run CI as a gate, then CD (version + diff --git a/AGENTS.md b/AGENTS.md index be6f7695..02386b96 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,32 +90,44 @@ coverage, setupFiles, and mock reset. ### SARIF lint baselining -Lint enforcement is a ratchet: PRs may not introduce _new_ ESLint -violations, while pre-existing (accepted) ones never block. Three -layers: - -- **`lint:eslint` is a reporter, not a gate.** It writes a SARIF log to - `dist/eslint.sarif` (a turbo task output) via a formatter bundled in - `@gtbuchanan/cli` and prints compact console output. Warnings never - fail it — the eslint-config downgrades every rule to a warning — but - fatal errors (parse/config breakage) still do. -- **`gtb lint:eslint:compare` is the gate.** It diffs each lint cwd's - `dist/eslint.sarif` against `dist/eslint-base.sarif` with - `sarif-multitool match-results-forward` and fails only on results +Lint enforcement is a ratchet: PRs may not introduce _new_ findings, +while pre-existing (accepted) ones never block. The machinery is +tool-agnostic — any reporter that drops a `.sarif` into +`dist/sarif/` is gated with no extra wiring; ESLint is the first such +reporter. Three layers: + +- **Reporters, not gates.** `lint:eslint` writes + `dist/sarif/eslint.sarif` (a turbo task output) via a formatter + bundled in `@gtbuchanan/cli` and prints compact console output. + Warnings never fail it — the eslint-config downgrades every rule to a + warning — but fatal errors (parse/config breakage) still do. +- **`gtb sarif compare` is the gate.** It pairs every + `dist/sarif/*.sarif` in each lint cwd with + `dist/sarif/base/.sarif` and diffs them with + `sarif-multitool match-results-forward`, failing only on results classified `new`. Matching is fingerprint/content-based, so baseline - violations that merely moved stay matched — no git-diff offset math. + findings that merely moved stay matched — no git-diff offset math. `--base ` makes it self-sufficient: it lints the merge base of that ref and HEAD in a throwaway git worktree to produce the baselines, so the same invocation works locally - (`gtb lint:eslint:compare --base origin/main`) and in CI - (`lint-regression.yml`). Dirs whose base commit wrote no SARIF are - skipped with a warning, which is what makes rollout graceful. + (`gtb sarif compare --base origin/main`) and in CI + (`lint-regression.yml`). A stamp (`dist/sarif/base.ref`, recording + the merge-base SHA) skips production when the on-disk baselines are + already current; CI seeds a cross-PR baseline cache from each main + commit via `gtb sarif baseline` (where merge base == HEAD, the + baseline is just that commit's own reporter output). A missing + baseline is an empty baseline, not a pass: every finding in a log + with no baseline counterpart is new and needs acceptance, so a newly + added reporter (or the bootstrap PR) can't slip findings in silently. + Suppressed findings (reasoned in-source suppressions) are exempt from + the gate — the suppression is already the accepted mechanism — but + stay in the logs for visibility. - **Changed-file enforcement stays local.** The hk pre-commit `eslint` step keeps `--max-warnings=0` on staged files, and the IDE shows warnings inline — new violations are still caught at authoring time; the ratchet only governs what lands. -Accepted violations enter the baseline by merging with the override +Accepted findings enter the baseline by merging with the override label (see `lint-regression.yml`) and surface in every SARIF log until paid down. @@ -395,7 +407,7 @@ through `package.json` scripts backed by `gtb` leaf commands. - **`lint-regression.yml`** — Fails a PR only on lint violations that are new relative to its merge base (the ratchet gate; see the SARIF lint baselining section). Lints HEAD, then runs - `gtb lint:eslint:compare --base origin/`, which lints the + `gtb sarif compare --base origin/`, which lints the merge base in a throwaway git worktree and diffs the SARIF logs via `sarif-multitool`. A maintainer-applied override label (default `accepted-lint-regression`, dismissed on every new push) turns a diff --git a/packages/cli/e2e/lint-compare.test.ts b/packages/cli/e2e/sarif-compare.test.ts similarity index 72% rename from packages/cli/e2e/lint-compare.test.ts rename to packages/cli/e2e/sarif-compare.test.ts index 9c1b7252..aa7ab442 100644 --- a/packages/cli/e2e/lint-compare.test.ts +++ b/packages/cli/e2e/sarif-compare.test.ts @@ -52,18 +52,21 @@ const existing: SarifViolation = { const source = 'export const app = () => {\n const unused = 1;\n console.log("hi");\n};\n'; -describe.concurrent('gtb lint:eslint:compare', () => { +describe.concurrent('gtb sarif compare', () => { it('passes when every violation is in the baseline', async ({ expect }) => { using fixture = createFixture(); const file = fixture.writeFile(path.join('src', 'app.js'), source); const uri = pathToFileURL(file).href; - fixture.writeFile(path.join('dist', 'eslint-base.sarif'), sarifLog(uri, [existing])); - fixture.writeFile(path.join('dist', 'eslint.sarif'), sarifLog(uri, [existing])); + fixture.writeFile( + path.join('dist', 'sarif', 'base', 'eslint.sarif'), + sarifLog(uri, [existing]), + ); + fixture.writeFile(path.join('dist', 'sarif', 'eslint.sarif'), sarifLog(uri, [existing])); - const result = await fixture.run('gtb', ['lint:eslint:compare']); + const result = await fixture.run('gtb', ['sarif', 'compare']); expect(result).toMatchObject({ exitCode: 0 }); - expect(result.stdout).toContain('No new lint violations'); + expect(result.stdout).toContain('No new findings'); }); it('fails on a violation missing from the baseline', async ({ expect }) => { @@ -76,28 +79,32 @@ describe.concurrent('gtb lint:eslint:compare', () => { ruleId: 'no-console', snippet: 'console.log("hi");', }; - fixture.writeFile(path.join('dist', 'eslint-base.sarif'), sarifLog(uri, [existing])); fixture.writeFile( - path.join('dist', 'eslint.sarif'), + path.join('dist', 'sarif', 'base', 'eslint.sarif'), + sarifLog(uri, [existing]), + ); + fixture.writeFile( + path.join('dist', 'sarif', 'eslint.sarif'), sarifLog(uri, [existing, added]), ); - const result = await fixture.run('gtb', ['lint:eslint:compare']); + const result = await fixture.run('gtb', ['sarif', 'compare']); expect(result.exitCode).not.toBe(0); expect(result.stderr).toContain('no-console'); expect(result.stderr).not.toContain('no-unused-vars'); }); - it('skips and passes when no baseline exists', async ({ expect }) => { + it('treats every finding as new when no baseline exists', async ({ expect }) => { using fixture = createFixture(); const file = fixture.writeFile(path.join('src', 'app.js'), source); const uri = pathToFileURL(file).href; - fixture.writeFile(path.join('dist', 'eslint.sarif'), sarifLog(uri, [existing])); + fixture.writeFile(path.join('dist', 'sarif', 'eslint.sarif'), sarifLog(uri, [existing])); - const result = await fixture.run('gtb', ['lint:eslint:compare']); + const result = await fixture.run('gtb', ['sarif', 'compare']); - expect(result).toMatchObject({ exitCode: 0 }); - expect(result.stderr).toContain('No baseline SARIF'); + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toContain('No baseline'); + expect(result.stderr).toContain('no-unused-vars'); }); }); diff --git a/packages/cli/src/commands/index.ts b/packages/cli/src/commands/index.ts index 996bd2e7..ca25002f 100644 --- a/packages/cli/src/commands/index.ts +++ b/packages/cli/src/commands/index.ts @@ -1,9 +1,9 @@ import { defineCommand } from 'citty'; import { hk } from './root/hk.ts'; -import { lintEslintCompare } from './root/lint-eslint-compare.ts'; import { rootNames } from './root/names.ts'; import { prepare } from './root/prepare.ts'; import { publish } from './root/publish.ts'; +import { sarif } from './root/sarif.ts'; import { sync } from './root/sync.ts'; import { turbo } from './root/turbo.ts'; import { verify } from './root/verify.ts'; @@ -19,9 +19,9 @@ export const main = defineCommand({ }, subCommands: { [rootNames.hk]: hk, - [rootNames.lintEslintCompare]: lintEslintCompare, [rootNames.prepare]: prepare, [rootNames.publish]: publish, + [rootNames.sarif]: sarif, [rootNames.sync]: sync, [rootNames.turbo]: turbo, [rootNames.verify]: verify, diff --git a/packages/cli/src/commands/root/lint-eslint-compare.ts b/packages/cli/src/commands/root/lint-eslint-compare.ts deleted file mode 100644 index 84e909cb..00000000 --- a/packages/cli/src/commands/root/lint-eslint-compare.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { defineCommand } from 'citty'; -import { executeLintEslintCompare } from '../../lib/lint-compare.ts'; -import { rootNames } from './names.ts'; - -/** - * `gtb lint:eslint:compare` — gates on lint regressions by diffing the - * current SARIF logs against baseline logs from the merge base. A user - * command rather than a turbo task: it consumes two generations of the - * same task output, which turbo's task model can't express. - */ -export const lintEslintCompare = defineCommand({ - args: { - base: { - description: - 'Git ref to diff against; lints its merge base with HEAD in a ' + - 'temporary worktree to produce the baseline (e.g. origin/main)', - type: 'string', - }, - }, - meta: { - description: 'Fail when lint violations are new relative to the SARIF baseline', - name: rootNames.lintEslintCompare, - }, - run: ({ args }) => executeLintEslintCompare({ baseRef: args.base }), -}); diff --git a/packages/cli/src/commands/root/names.ts b/packages/cli/src/commands/root/names.ts index 26c657e6..38d55e6f 100644 --- a/packages/cli/src/commands/root/names.ts +++ b/packages/cli/src/commands/root/names.ts @@ -1,9 +1,9 @@ /** CLI names for root-level commands. */ export const rootNames = { hk: 'hk', - lintEslintCompare: 'lint:eslint:compare', prepare: 'prepare', publish: 'publish', + sarif: 'sarif', sync: 'sync', turbo: 'turbo', verify: 'verify', diff --git a/packages/cli/src/commands/root/sarif.ts b/packages/cli/src/commands/root/sarif.ts new file mode 100644 index 00000000..1d0d8278 --- /dev/null +++ b/packages/cli/src/commands/root/sarif.ts @@ -0,0 +1,49 @@ +import { defineCommand } from 'citty'; +import { executeSarifBaseline, executeSarifCompare } from '../../lib/sarif-compare.ts'; +import { rootNames } from './names.ts'; + +/** + * `gtb sarif compare` — gates on regressions by diffing the current + * SARIF logs against baseline logs from the merge base. A user command + * rather than a turbo task: it consumes two generations of the same + * task outputs, which turbo's task model can't express, and its result + * depends on git state turbo can't hash. + */ +const compare = defineCommand({ + args: { + base: { + description: + 'Git ref to diff against; lints its merge base with HEAD in a ' + + 'temporary worktree to produce the baseline (e.g. origin/main)', + type: 'string', + }, + }, + meta: { + description: 'Fail when SARIF findings are new relative to the baseline', + name: 'compare', + }, + run: ({ args }) => executeSarifCompare({ baseRef: args.base }), +}); + +/** + * `gtb sarif baseline` — snapshots HEAD's SARIF logs as the compare + * baseline. Run on the default branch (where any future PR's merge base + * is HEAD itself) so CI can cache the result for PR `sarif compare` + * runs to restore. + */ +const baseline = defineCommand({ + meta: { + description: "Snapshot HEAD's SARIF logs as the compare baseline", + name: 'baseline', + }, + run: () => executeSarifBaseline(), +}); + +/** `gtb sarif` — SARIF baselining: regression compare and baseline snapshot. */ +export const sarif = defineCommand({ + meta: { + description: 'Compare or snapshot SARIF static-analysis baselines', + name: rootNames.sarif, + }, + subCommands: { baseline, compare }, +}); diff --git a/packages/cli/src/commands/task/lint-eslint.ts b/packages/cli/src/commands/task/lint-eslint.ts index 43b3db24..c0fb0627 100644 --- a/packages/cli/src/commands/task/lint-eslint.ts +++ b/packages/cli/src/commands/task/lint-eslint.ts @@ -16,7 +16,7 @@ const formatterPath = fileURLToPath( * (the repo convention downgrades every rule to a warning), while fatal * errors — parse or config breakage — still do. Enforcement lives in * the changed-files pre-commit step locally and in - * `gtb lint:eslint:compare` (new-violations-only) in CI, so a baseline + * `gtb sarif compare` (new-findings-only) in CI, so a baseline * SARIF log exists for every commit, including ones carrying accepted * violations. */ diff --git a/packages/cli/src/lib/eslint-sarif-formatter.ts b/packages/cli/src/lib/eslint-sarif-formatter.ts index 1d479e47..ecbe3d95 100644 --- a/packages/cli/src/lib/eslint-sarif-formatter.ts +++ b/packages/cli/src/lib/eslint-sarif-formatter.ts @@ -1,9 +1,10 @@ import { mkdirSync, writeFileSync } from 'node:fs'; import path from 'node:path'; import sarifFormat from '@microsoft/eslint-formatter-sarif'; +import { sarifPaths } from './sarif-paths.ts'; /** SARIF log path written by the formatter, relative to the lint cwd. */ -export const sarifOutputPath = 'dist/eslint.sarif'; +export const sarifOutputPath = path.join(sarifPaths.dir, 'eslint.sarif'); /** Structural subset of an ESLint lint message the console output needs. */ export interface FormatterMessage { diff --git a/packages/cli/src/lib/lint-compare.ts b/packages/cli/src/lib/lint-compare.ts deleted file mode 100644 index f3150e3d..00000000 --- a/packages/cli/src/lib/lint-compare.ts +++ /dev/null @@ -1,286 +0,0 @@ -import { copyFileSync, existsSync, mkdirSync, mkdtempSync } from 'node:fs'; -import { createRequire } from 'node:module'; -import { tmpdir } from 'node:os'; -import path from 'node:path'; -import * as v from 'valibot'; -import { readJsonFile } from './file-writer.ts'; -import { type Logger, createLogger } from './logger.ts'; -import { type RunOptions, capture, run } from './process.ts'; -import { type WorkspaceContext, resolveWorkspace } from './workspace.ts'; - -/** SARIF artifact filenames under a lint cwd's `dist/`. */ -export const sarifFileNames = { - base: 'eslint-base.sarif', - current: 'eslint.sarif', - matched: 'eslint-matched.sarif', -} as const; - -const SarifRegionSchema = v.object({ - startColumn: v.optional(v.number()), - startLine: v.optional(v.number()), -}); - -const SarifArtifactLocationSchema = v.object({ - uri: v.optional(v.string()), -}); - -const SarifPhysicalLocationSchema = v.object({ - artifactLocation: v.optional(SarifArtifactLocationSchema), - region: v.optional(SarifRegionSchema), -}); - -const SarifLocationSchema = v.object({ - physicalLocation: v.optional(SarifPhysicalLocationSchema), -}); - -const SarifResultSchema = v.object({ - baselineState: v.optional(v.string()), - level: v.optional(v.string()), - locations: v.optional(v.array(SarifLocationSchema)), - message: v.object({ text: v.string() }), - ruleId: v.optional(v.string()), -}); - -const SarifRunSchema = v.object({ - results: v.optional(v.array(SarifResultSchema)), -}); - -const SarifLogSchema = v.object({ - runs: v.array(SarifRunSchema), -}); - -/** Parsed subset of a SARIF log the compare consumes. */ -export type SarifLog = v.InferOutput; - -type SarifResult = v.InferOutput; - -/** Validates untrusted JSON as a {@link SarifLog}. */ -export const parseSarifLog = (data: unknown): SarifLog => - v.parse(SarifLogSchema, data); - -/** A lint violation present in HEAD but not matched to the baseline. */ -export interface NewViolation { - readonly column: number | undefined; - readonly level: string; - readonly line: number | undefined; - readonly message: string; - readonly ruleId: string; - readonly uri: string; -} - -interface LocationParts { - readonly column: number | undefined; - readonly line: number | undefined; - readonly uri: string | undefined; -} - -const toLocationParts = (result: SarifResult): LocationParts => { - const location = result.locations?.[0]?.physicalLocation; - return { - column: location?.region?.startColumn, - line: location?.region?.startLine, - uri: location?.artifactLocation?.uri, - }; -}; - -const toNewViolation = (result: SarifResult): NewViolation => { - const { column, line, uri } = toLocationParts(result); - return { - column, - level: result.level ?? 'error', - line, - message: result.message.text, - ruleId: result.ruleId ?? 'internal', - uri: uri ?? '', - }; -}; - -/** Extracts results the baseliner classified as `new`. */ -export const extractNewViolations = (log: SarifLog): readonly NewViolation[] => - log.runs.flatMap(run_ => - (run_.results ?? []) - .filter(result => result.baselineState === 'new') - .map(toNewViolation), - ); - -const formatPosition = (violation: NewViolation): string => { - if (violation.line === undefined) return ''; - const column = violation.column === undefined ? '' : `:${String(violation.column)}`; - return `:${String(violation.line)}${column}`; -}; - -const formatViolation = (violation: NewViolation): string => { - const { level, message, ruleId, uri } = violation; - return `${uri}${formatPosition(violation)} ${level} ${message} ${ruleId}`; -}; - -/** Renders new violations for console output, one line per violation. */ -export const formatNewViolations = ( - violations: readonly NewViolation[], -): string => violations.map(formatViolation).join('\n'); - -/** - * Side-effecting I/O the compare depends on. Injected so the - * orchestration (baseline production, per-package matching, gating) is - * unit-testable without spawning git, turbo, or the SARIF multitool. - */ -export interface LintCompareDeps { - readonly capture: (command: string, args: readonly string[]) => Promise; - readonly copyFile: (source: string, destination: string) => void; - readonly exists: (filePath: string) => boolean; - readonly logger: Logger; - readonly makeTempDir: () => string; - readonly readJson: (filePath: string) => unknown; - readonly resolveMultitool: () => string; - readonly run: (command: string, options?: RunOptions) => Promise; - readonly workspace: (cwd?: string) => WorkspaceContext; -} - -/** - * The multitool npm package exports the path to its platform-specific - * self-contained binary. Resolved lazily (and spawned directly, skipping - * the package's `shell: true` bin shim) so merely loading the CLI never - * requires the optional platform package. - */ -const resolveMultitoolBinary = (): string => { - const require = createRequire(import.meta.url); - return v.parse(v.string(), require('@microsoft/sarif-multitool')); -}; - -const defaultDeps: LintCompareDeps = { - capture, - copyFile: (source, destination) => { - mkdirSync(path.dirname(destination), { recursive: true }); - copyFileSync(source, destination); - }, - exists: existsSync, - logger: createLogger(), - makeTempDir: () => mkdtempSync(path.join(tmpdir(), 'gtb-lint-base-')), - readJson: readJsonFile, - resolveMultitool: resolveMultitoolBinary, - run, - workspace: cwd => resolveWorkspace(cwd === undefined ? undefined : { cwd }), -}; - -/** - * Produces per-package baseline SARIF logs by linting the merge base in - * a throwaway git worktree and copying each `dist/eslint.sarif` into - * the corresponding head package as `dist/eslint-base.sarif`. A failing - * base lint is tolerated: the SARIF log is written before ESLint - * exits, and a baseline carrying violations is exactly what the ratchet - * diffs against. Base commits that predate SARIF output simply produce - * no baseline, and the compare skips those packages. - */ -export const produceBaseline = async ( - baseRef: string, - deps: LintCompareDeps, -): Promise => { - const sha = await deps.capture('git', ['merge-base', baseRef, 'HEAD']); - const baseDir = deps.makeTempDir(); - try { - await deps.run('git', { args: ['worktree', 'add', '--detach', baseDir, sha] }); - await deps.run('pnpm', { - args: ['install', '--frozen-lockfile', '--prefer-offline'], - cwd: baseDir, - }); - try { - await deps.run('pnpm', { - args: ['exec', 'turbo', 'run', 'lint', '--output-logs=errors-only'], - cwd: baseDir, - }); - } catch { - // Pre-ratchet gtb fails lint on warnings after writing the SARIF log. - deps.logger.error(`Base lint at ${sha} failed; using whatever SARIF it wrote`); - } - copyBaselineSarifs(baseDir, deps); - } finally { - await deps.run('git', { args: ['worktree', 'remove', '--force', baseDir] }); - } -}; - -const copyBaselineSarifs = (baseDir: string, deps: LintCompareDeps): void => { - const base = deps.workspace(baseDir); - const head = deps.workspace(); - const dirs = new Set([base.rootDir, ...base.packageDirs]); - for (const dir of dirs) { - const source = path.join(dir, 'dist', sarifFileNames.current); - if (!deps.exists(source)) { - continue; - } - const relative = path.relative(base.rootDir, dir); - const destination = path.join( - head.rootDir, relative, 'dist', sarifFileNames.base, - ); - deps.copyFile(source, destination); - } -}; - -const matchDirForward = async ( - dir: string, - deps: LintCompareDeps, -): Promise => { - const distDir = path.join(dir, 'dist'); - const current = path.join(distDir, sarifFileNames.current); - if (!deps.exists(current)) { - return []; - } - const base = path.join(distDir, sarifFileNames.base); - if (!deps.exists(base)) { - deps.logger.error(`No baseline SARIF for ${dir}; skipping`); - return []; - } - const matched = path.join(distDir, sarifFileNames.matched); - await deps.run(deps.resolveMultitool(), { - args: [ - 'match-results-forward', current, - '--previous', base, - '--output-file-path', matched, - // Reruns are routine (retries, local iteration); replace stale output. - '--log', 'ForceOverwrite', - ], - }); - const log = parseSarifLog(deps.readJson(matched)); - return extractNewViolations(log); -}; - -/** Options for {@link executeLintEslintCompare}. */ -export interface LintEslintCompareOptions { - /** - * Git ref to diff against. When set, the baseline is produced by - * linting the merge base of this ref and HEAD in a throwaway - * worktree. When unset, `dist/eslint-base.sarif` files must already - * be in place (e.g. restored from a cache or a prior run). - */ - readonly baseRef?: string | undefined; -} - -/** - * Compares each lint cwd's current SARIF log against its baseline via - * `sarif-multitool match-results-forward` and rejects when any result - * is classified `new`. Matching is fingerprint/content-based, so - * baseline violations that merely moved (edits above them) stay - * matched — only genuine regressions gate. - */ -export const executeLintEslintCompare = async ( - options: LintEslintCompareOptions = {}, - deps: LintCompareDeps = defaultDeps, -): Promise => { - if (options.baseRef !== undefined) { - await produceBaseline(options.baseRef, deps); - } - const { packageDirs, rootDir } = deps.workspace(); - const lintDirs = [...new Set([rootDir, ...packageDirs])]; - const violations: NewViolation[] = []; - - for (const dir of lintDirs) { - violations.push(...await matchDirForward(dir, deps)); - } - - if (violations.length > 0) { - deps.logger.error(formatNewViolations(violations)); - throw new Error( - `${String(violations.length)} new lint violation(s) not present in the baseline`, - ); - } - deps.logger.info('No new lint violations'); -}; diff --git a/packages/cli/src/lib/sarif-compare.ts b/packages/cli/src/lib/sarif-compare.ts new file mode 100644 index 00000000..b8082eb1 --- /dev/null +++ b/packages/cli/src/lib/sarif-compare.ts @@ -0,0 +1,387 @@ +import { + copyFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, + rmSync, writeFileSync, +} from 'node:fs'; +import { createRequire } from 'node:module'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import * as v from 'valibot'; +import { readJsonFile } from './file-writer.ts'; +import { type Logger, createLogger } from './logger.ts'; +import { type RunOptions, capture, run } from './process.ts'; +import { sarifPaths } from './sarif-paths.ts'; +import { localeComparer } from './sort.ts'; +import { type WorkspaceContext, resolveWorkspace } from './workspace.ts'; + +export { sarifPaths } from './sarif-paths.ts'; + +const SarifRegionSchema = v.object({ + startColumn: v.optional(v.number()), + startLine: v.optional(v.number()), +}); + +const SarifArtifactLocationSchema = v.object({ + uri: v.optional(v.string()), +}); + +const SarifPhysicalLocationSchema = v.object({ + artifactLocation: v.optional(SarifArtifactLocationSchema), + region: v.optional(SarifRegionSchema), +}); + +const SarifLocationSchema = v.object({ + physicalLocation: v.optional(SarifPhysicalLocationSchema), +}); + +const SarifSuppressionsSchema = v.array(v.unknown()); + +const SarifResultSchema = v.object({ + baselineState: v.optional(v.string()), + level: v.optional(v.string()), + locations: v.optional(v.array(SarifLocationSchema)), + message: v.object({ text: v.string() }), + ruleId: v.optional(v.string()), + suppressions: v.optional(SarifSuppressionsSchema), +}); + +const SarifRunSchema = v.object({ + results: v.optional(v.array(SarifResultSchema)), +}); + +const SarifLogSchema = v.object({ + runs: v.array(SarifRunSchema), +}); + +/** Parsed subset of a SARIF log the compare consumes. */ +export type SarifLog = v.InferOutput; + +type SarifResult = v.InferOutput; + +/** Validates untrusted JSON as a {@link SarifLog}. */ +export const parseSarifLog = (data: unknown): SarifLog => + v.parse(SarifLogSchema, data); + +/** A finding present in HEAD but not matched to the baseline. */ +export interface NewFinding { + readonly column: number | undefined; + readonly level: string; + readonly line: number | undefined; + readonly message: string; + readonly ruleId: string; + readonly uri: string; +} + +interface LocationParts { + readonly column: number | undefined; + readonly line: number | undefined; + readonly uri: string | undefined; +} + +const toLocationParts = (result: SarifResult): LocationParts => { + const location = result.locations?.[0]?.physicalLocation; + return { + column: location?.region?.startColumn, + line: location?.region?.startLine, + uri: location?.artifactLocation?.uri, + }; +}; + +const toNewFinding = (result: SarifResult): NewFinding => { + const { column, line, uri } = toLocationParts(result); + return { + column, + level: result.level ?? 'error', + line, + message: result.message.text, + ruleId: result.ruleId ?? 'internal', + uri: uri ?? '', + }; +}; + +/** + * Suppressed findings (e.g. reasoned `eslint-disable` comments) are + * exempt from the gate: an in-source suppression is already the + * accepted mechanism for carrying a finding, reviewed with the code. + * They stay in the SARIF logs for visibility; they just never block. + */ +const isUnsuppressed = (result: SarifResult): boolean => + (result.suppressions ?? []).length === 0; + +/** Extracts unsuppressed results the baseliner classified as `new`. */ +export const extractNewFindings = (log: SarifLog): readonly NewFinding[] => + log.runs.flatMap(run_ => + (run_.results ?? []) + .filter(isUnsuppressed) + .filter(result => result.baselineState === 'new') + .map(toNewFinding), + ); + +/** + * Extracts every unsuppressed result — the classification of a log + * whose baseline is empty (all findings are new by definition). + */ +export const extractAllFindings = (log: SarifLog): readonly NewFinding[] => + log.runs.flatMap(run_ => + (run_.results ?? []).filter(isUnsuppressed).map(toNewFinding), + ); + +const formatPosition = (finding: NewFinding): string => { + if (finding.line === undefined) return ''; + const column = finding.column === undefined ? '' : `:${String(finding.column)}`; + return `:${String(finding.line)}${column}`; +}; + +const formatFinding = (finding: NewFinding): string => { + const { level, message, ruleId, uri } = finding; + return `${uri}${formatPosition(finding)} ${level} ${message} ${ruleId}`; +}; + +/** Renders new findings for console output, one line per finding. */ +export const formatNewFindings = ( + findings: readonly NewFinding[], +): string => findings.map(formatFinding).join('\n'); + +/** + * Side-effecting I/O the compare depends on. Injected so the + * orchestration (baseline production, per-package matching, gating) is + * unit-testable without spawning git, turbo, or the SARIF multitool. + */ +export interface SarifCompareDeps { + readonly capture: (command: string, args: readonly string[]) => Promise; + readonly copyFile: (source: string, destination: string) => void; + readonly ensureDir: (dir: string) => void; + readonly exists: (filePath: string) => boolean; + /** Names of `*.sarif` files directly in `dir` (empty when missing). */ + readonly list: (dir: string) => readonly string[]; + readonly logger: Logger; + readonly makeTempDir: () => string; + readonly readJson: (filePath: string) => unknown; + readonly readText: (filePath: string) => string; + readonly remove: (filePath: string) => void; + readonly resolveMultitool: () => string; + readonly run: (command: string, options?: RunOptions) => Promise; + readonly workspace: (cwd?: string) => WorkspaceContext; + readonly writeText: (filePath: string, content: string) => void; +} + +/** + * The multitool npm package exports the path to its platform-specific + * self-contained binary. Resolved lazily (and spawned directly, skipping + * the package's `shell: true` bin shim) so merely loading the CLI never + * requires the optional platform package. + */ +const resolveMultitoolBinary = (): string => { + const require = createRequire(import.meta.url); + return v.parse(v.string(), require('@microsoft/sarif-multitool')); +}; + +const listSarifFiles = (dir: string): readonly string[] => { + if (!existsSync(dir)) return []; + return readdirSync(dir, { withFileTypes: true }) + .filter(entry => entry.isFile() && entry.name.endsWith('.sarif')) + .map(entry => entry.name) + .toSorted(localeComparer); +}; + +const defaultDeps: SarifCompareDeps = { + capture, + copyFile: (source, destination) => { + mkdirSync(path.dirname(destination), { recursive: true }); + copyFileSync(source, destination); + }, + ensureDir: (dir) => { + mkdirSync(dir, { recursive: true }); + }, + exists: existsSync, + list: listSarifFiles, + logger: createLogger(), + makeTempDir: () => mkdtempSync(path.join(tmpdir(), 'gtb-sarif-base-')), + readJson: readJsonFile, + readText: filePath => readFileSync(filePath, 'utf8'), + remove: (filePath) => { + rmSync(filePath, { force: true, recursive: true }); + }, + resolveMultitool: resolveMultitoolBinary, + run, + workspace: cwd => resolveWorkspace(cwd === undefined ? undefined : { cwd }), + writeText: (filePath, content) => { + mkdirSync(path.dirname(filePath), { recursive: true }); + writeFileSync(filePath, content); + }, +}; + +/** Absolute path of the baseline stamp file for the head workspace. */ +const baselineStampPath = (deps: SarifCompareDeps): string => + path.join(deps.workspace().rootDir, sarifPaths.stamp); + +/** + * Whether the on-disk baselines were produced from the given merge-base + * SHA (per the stamp file), making production skippable. + */ +export const hasCurrentBaseline = (sha: string, deps: SarifCompareDeps): boolean => { + const stamp = baselineStampPath(deps); + return deps.exists(stamp) && deps.readText(stamp).trim() === sha; +}; + +/** + * Produces per-package baseline SARIF logs by linting the given + * merge-base SHA in a throwaway git worktree and copying each + * `dist/sarif/*.sarif` into the corresponding head package under + * `dist/sarif/base/`, then stamps the SHA. A failing base lint is + * tolerated: reporters write their SARIF logs before exiting, and a + * baseline carrying findings is exactly what the ratchet diffs against. + * Base commits that predate SARIF output simply produce no baseline, + * and the compare skips those packages. + */ +export const produceBaseline = async ( + sha: string, + deps: SarifCompareDeps, +): Promise => { + const baseDir = deps.makeTempDir(); + try { + await deps.run('git', { args: ['worktree', 'add', '--detach', baseDir, sha] }); + await deps.run('pnpm', { + args: ['install', '--frozen-lockfile', '--prefer-offline'], + cwd: baseDir, + }); + try { + await deps.run('pnpm', { + args: ['exec', 'turbo', 'run', 'lint', '--output-logs=errors-only'], + cwd: baseDir, + }); + } catch { + // Pre-ratchet gtb fails lint on warnings after writing the SARIF log. + deps.logger.error(`Base lint at ${sha} failed; using whatever SARIF it wrote`); + } + copyBaselineSarifs(baseDir, deps); + deps.writeText(baselineStampPath(deps), `${sha}\n`); + } finally { + await deps.run('git', { args: ['worktree', 'remove', '--force', baseDir] }); + } +}; + +const copyBaselineSarifs = (baseDir: string, deps: SarifCompareDeps): void => { + const base = deps.workspace(baseDir); + const head = deps.workspace(); + /* + * Clear baselines from any earlier production first: a package whose + * new merge base wrote no SARIF must not keep a stale baseline from a + * previous merge base. + */ + const headDirs = new Set([head.rootDir, ...head.packageDirs]); + for (const dir of headDirs) { + deps.remove(path.join(dir, sarifPaths.base)); + } + const baseDirs = new Set([base.rootDir, ...base.packageDirs]); + for (const dir of baseDirs) { + const relative = path.relative(base.rootDir, dir); + const names = deps.list(path.join(dir, sarifPaths.dir)); + for (const name of names) { + deps.copyFile( + path.join(dir, sarifPaths.dir, name), + path.join(head.rootDir, relative, sarifPaths.base, name), + ); + } + } +}; + +const matchFileForward = async ( + dir: string, + name: string, + deps: SarifCompareDeps, +): Promise => { + const base = path.join(dir, sarifPaths.base, name); + if (!deps.exists(base)) { + /* + * A missing baseline is an empty baseline, not a pass: every + * finding is new and needs explicit acceptance. This keeps a newly + * added reporter (or the bootstrap PR) from slipping findings in + * silently. + */ + deps.logger.error(`No baseline for ${name} in ${dir}; all findings are new`); + const current = deps.readJson(path.join(dir, sarifPaths.dir, name)); + return extractAllFindings(parseSarifLog(current)); + } + const matched = path.join(dir, sarifPaths.matched, name); + // The multitool won't create the output file's parent directory. + deps.ensureDir(path.join(dir, sarifPaths.matched)); + await deps.run(deps.resolveMultitool(), { + args: [ + 'match-results-forward', path.join(dir, sarifPaths.dir, name), + '--previous', base, + '--output-file-path', matched, + // Reruns are routine (retries, local iteration); replace stale output. + '--log', 'ForceOverwrite', + ], + }); + return extractNewFindings(parseSarifLog(deps.readJson(matched))); +}; + +/** + * Snapshots the current SARIF logs as the baseline. On the default + * branch the merge base of any future PR is HEAD itself, so those PRs' + * baseline is just this commit's own reporter output: copy each + * `dist/sarif/*.sarif` under `dist/sarif/base/` and stamp HEAD's SHA. + * CI saves the result in a cache keyed on that SHA for PR compare runs + * to restore. + */ +export const executeSarifBaseline = async ( + deps: SarifCompareDeps = defaultDeps, +): Promise => { + const sha = await deps.capture('git', ['rev-parse', 'HEAD']); + const { rootDir } = deps.workspace(); + copyBaselineSarifs(rootDir, deps); + deps.writeText(baselineStampPath(deps), `${sha}\n`); + deps.logger.info(`Seeded SARIF baselines for ${sha}`); +}; + +/** Options for {@link executeSarifCompare}. */ +export interface SarifCompareOptions { + /** + * Git ref to diff against. When set, the baseline is produced by + * linting the merge base of this ref and HEAD in a throwaway + * worktree (skipped when the stamp already records that merge base). + * When unset, `dist/sarif/base/` must already be populated (e.g. + * restored from a cache or a prior run). + */ + readonly baseRef?: string | undefined; +} + +/** + * Compares every SARIF log under each lint cwd's `dist/sarif/` against + * its baseline via `sarif-multitool match-results-forward` and rejects + * when any result is classified `new`. Matching is fingerprint and + * content based, so baseline findings that merely moved (edits above + * them) stay matched — only genuine regressions gate. + */ +export const executeSarifCompare = async ( + options: SarifCompareOptions = {}, + deps: SarifCompareDeps = defaultDeps, +): Promise => { + if (options.baseRef !== undefined) { + const sha = await deps.capture('git', ['merge-base', options.baseRef, 'HEAD']); + if (hasCurrentBaseline(sha, deps)) { + deps.logger.info(`Baselines for merge base ${sha} already present; reusing`); + } else { + await produceBaseline(sha, deps); + } + } + const { packageDirs, rootDir } = deps.workspace(); + const lintDirs = [...new Set([rootDir, ...packageDirs])]; + const findings: NewFinding[] = []; + + for (const dir of lintDirs) { + const names = deps.list(path.join(dir, sarifPaths.dir)); + for (const name of names) { + findings.push(...await matchFileForward(dir, name, deps)); + } + } + + if (findings.length > 0) { + deps.logger.error(formatNewFindings(findings)); + throw new Error( + `${String(findings.length)} new finding(s) not present in the baseline`, + ); + } + deps.logger.info('No new findings'); +}; diff --git a/packages/cli/src/lib/sarif-paths.ts b/packages/cli/src/lib/sarif-paths.ts new file mode 100644 index 00000000..3fb9a9d0 --- /dev/null +++ b/packages/cli/src/lib/sarif-paths.ts @@ -0,0 +1,20 @@ +import path from 'node:path'; + +/** + * SARIF artifact layout under each lint cwd. Reporters (any tool) drop + * `.sarif` files into `dist/sarif/`; the compare pairs each with + * `dist/sarif/base/.sarif` by filename and writes match results + * to `dist/sarif/matched/.sarif`. The stamp lives only at the + * workspace root and records the merge-base SHA the on-disk baselines + * were produced from, letting `--base` skip production when current — + * locally across repeat runs, and in CI via a cache keyed on that SHA. + * + * Kept dependency-free: the ESLint formatter imports this from inside + * the ESLint process. + */ +export const sarifPaths = { + base: path.join('dist', 'sarif', 'base'), + dir: path.join('dist', 'sarif'), + matched: path.join('dist', 'sarif', 'matched'), + stamp: path.join('dist', 'sarif', 'base.ref'), +} as const; diff --git a/packages/cli/src/lib/turbo-config.ts b/packages/cli/src/lib/turbo-config.ts index 3e3dfb22..98a10eec 100644 --- a/packages/cli/src/lib/turbo-config.ts +++ b/packages/cli/src/lib/turbo-config.ts @@ -259,7 +259,7 @@ const lintTasks = (flags: ToolFlags): readonly ConditionalEntry[] => value: { dependsOn: deps, inputs: ['$TURBO_ROOT$/eslint.config.*', ...inputs, 'eslint.config.*'], - outputs: ['dist/.eslintcache', 'dist/eslint.sarif'], + outputs: ['dist/.eslintcache', 'dist/sarif/eslint.sarif'], }, }, ]; @@ -294,7 +294,7 @@ const rootLintTasks = ( '$TURBO_DEFAULT$', ...packageGlobs.map(glob => `!${toPackageIgnore(glob)}`), ], - outputs: ['dist/.eslintcache', 'dist/eslint.sarif'], + outputs: ['dist/.eslintcache', 'dist/sarif/eslint.sarif'], }, }, ]; diff --git a/packages/cli/test/lint-compare.test.ts b/packages/cli/test/lint-compare.test.ts deleted file mode 100644 index 320b89f8..00000000 --- a/packages/cli/test/lint-compare.test.ts +++ /dev/null @@ -1,318 +0,0 @@ -import { faker } from '@faker-js/faker'; -import { describe, it } from 'vitest'; -import { - type LintCompareDeps, - type NewViolation, - executeLintEslintCompare, - extractNewViolations, - formatNewViolations, - parseSarifLog, - produceBaseline, - sarifFileNames, -} from '#src/lib/lint-compare.js'; -import type { Logger } from '#src/lib/logger.js'; -import type { WorkspaceContext } from '#src/lib/workspace.js'; - -const sarifResult = (baselineState: string, overrides?: { - readonly ruleId?: string; - readonly startLine?: number; -}): object => ({ - baselineState, - level: 'error', - locations: [{ - physicalLocation: { - artifactLocation: { uri: faker.system.filePath() }, - region: { startColumn: 1, startLine: overrides?.startLine ?? 1 }, - }, - }], - message: { text: faker.lorem.sentence() }, - ruleId: overrides?.ruleId ?? 'no-unused-vars', -}); - -const sarifLog = (results: readonly object[]): object => ({ - runs: [{ results }], -}); - -interface RunCall { - readonly args: readonly string[]; - readonly command: string; -} - -interface CopyCall { - readonly destination: string; - readonly source: string; -} - -interface StubDeps { - readonly copyCalls: readonly CopyCall[]; - readonly deps: LintCompareDeps; - readonly errors: readonly string[]; - readonly infos: readonly string[]; - readonly runCalls: readonly RunCall[]; -} - -interface StubOptions { - /** Workspace returned for the base worktree cwd. Defaults to `workspace`. */ - readonly baseWorkspace?: WorkspaceContext; - /** `" "` prefixes whose run() rejects. */ - readonly failing?: readonly string[]; -} - -const normalize = (filePath: string): string => filePath.replaceAll('\\', '/'); - -/** - * Fakes a workspace whose SARIF files are the members of `files`; - * reading any matched path yields `matched`. - */ -const stubDeps = ( - workspace: WorkspaceContext, - files: readonly string[], - matched: object, - options?: StubOptions, -): StubDeps => { - const copyCalls: CopyCall[] = []; - const errors: string[] = []; - const infos: string[] = []; - const runCalls: RunCall[] = []; - const logger: Logger = { - error: (...args) => void errors.push(args.join(' ')), - info: (...args) => void infos.push(args.join(' ')), - }; - return { - copyCalls, - deps: { - capture: () => Promise.resolve('abc1234'), - copyFile: (source, destination) => { - copyCalls.push({ - destination: normalize(destination), - source: normalize(source), - }); - }, - exists: filePath => files.includes(normalize(filePath)), - logger, - makeTempDir: () => '/tmp/base', - readJson: () => matched, - resolveMultitool: () => 'sarif-multitool', - run: (command, runOptions) => { - runCalls.push({ args: runOptions?.args ?? [], command }); - const key = `${command} ${runOptions?.args?.[0] ?? ''}`; - return options?.failing?.includes(key) === true - ? Promise.reject(new Error(`${key} failed`)) - : Promise.resolve(); - }, - workspace: cwd => - (cwd === undefined ? workspace : options?.baseWorkspace ?? workspace), - }, - errors, - infos, - runCalls, - }; -}; - -describe.concurrent(extractNewViolations, () => { - it('keeps only results the baseliner classified as new', ({ expect }) => { - const newResult = sarifResult('new', { ruleId: 'no-console' }); - const log = sarifLog([ - sarifResult('unchanged'), - sarifResult('updated'), - newResult, - sarifResult('absent'), - ]); - - const violations = extractNewViolations(parseSarifLog(log)); - - expect(violations).toHaveLength(1); - expect(violations[0]).toMatchObject({ level: 'error', ruleId: 'no-console' }); - }); - - it('falls back to placeholders when location or rule is absent', ({ expect }) => { - const log = parseSarifLog(sarifLog([ - { baselineState: 'new', message: { text: 'boom' } }, - ])); - - expect(extractNewViolations(log)).toStrictEqual([{ - column: undefined, - level: 'error', - line: undefined, - message: 'boom', - ruleId: 'internal', - uri: '', - }]); - }); -}); - -describe.concurrent(formatNewViolations, () => { - it('renders uri, position, level, message, and rule per line', ({ expect }) => { - const violation: NewViolation = { - column: 3, - level: 'error', - line: 7, - message: 'Unexpected console statement.', - ruleId: 'no-console', - uri: 'file:///repo/src/app.js', - }; - - expect(formatNewViolations([violation])).toBe( - 'file:///repo/src/app.js:7:3 error Unexpected console statement. no-console', - ); - }); - - it('omits the position when the result has no region', ({ expect }) => { - const violation: NewViolation = { - column: undefined, - level: 'warning', - line: undefined, - message: 'boom', - ruleId: 'internal', - uri: '', - }; - - expect(formatNewViolations([violation])).toBe(' warning boom internal'); - }); -}); - -describe.concurrent(executeLintEslintCompare, () => { - const workspace: WorkspaceContext = { - packageDirs: ['/repo/packages/a'], - packageGlobs: ['packages/*'], - rootDir: '/repo', - }; - const currentFiles = [ - `/repo/dist/${sarifFileNames.current}`, - `/repo/dist/${sarifFileNames.base}`, - `/repo/packages/a/dist/${sarifFileNames.current}`, - `/repo/packages/a/dist/${sarifFileNames.base}`, - ]; - - it('matches every lint dir forward against its baseline', async ({ expect }) => { - const { deps, runCalls } = stubDeps(workspace, currentFiles, sarifLog([])); - - await executeLintEslintCompare({}, deps); - - expect(runCalls).toHaveLength(2); - expect(runCalls[0]).toMatchObject({ command: 'sarif-multitool' }); - expect(runCalls[0]?.args[0]).toBe('match-results-forward'); - }); - - it('resolves and reports when no results are new', async ({ expect }) => { - const { deps, infos } = stubDeps( - workspace, - currentFiles, - sarifLog([sarifResult('unchanged'), sarifResult('updated')]), - ); - - await executeLintEslintCompare({}, deps); - - expect(infos).toContain('No new lint violations'); - }); - - it('rejects when any result is new', async ({ expect }) => { - const { deps } = stubDeps(workspace, currentFiles, sarifLog([sarifResult('new')])); - - await expect(executeLintEslintCompare({}, deps)).rejects.toThrow( - /new lint violation/v, - ); - }); - - it('skips dirs with no current SARIF log', async ({ expect }) => { - const { deps, runCalls } = stubDeps(workspace, [], sarifLog([])); - - await executeLintEslintCompare({}, deps); - - expect(runCalls).toHaveLength(0); - }); - - it('warns and skips dirs missing only the baseline', async ({ expect }) => { - const { deps, errors, runCalls } = stubDeps( - workspace, - [`/repo/dist/${sarifFileNames.current}`], - sarifLog([]), - ); - - await executeLintEslintCompare({}, deps); - - expect(runCalls).toHaveLength(0); - expect(errors.some(line => line.includes('No baseline SARIF'))).toBe(true); - }); -}); - -describe.concurrent(produceBaseline, () => { - const headWorkspace: WorkspaceContext = { - packageDirs: ['/repo/packages/a'], - packageGlobs: ['packages/*'], - rootDir: '/repo', - }; - const baseWorkspace: WorkspaceContext = { - packageDirs: ['/tmp/base/packages/a'], - packageGlobs: ['packages/*'], - rootDir: '/tmp/base', - }; - const baseFiles = [ - `/tmp/base/dist/${sarifFileNames.current}`, - `/tmp/base/packages/a/dist/${sarifFileNames.current}`, - ]; - - it('lints the merge base in a temp worktree and copies baselines', async ({ - expect, - }) => { - const { copyCalls, deps, runCalls } = stubDeps( - headWorkspace, baseFiles, sarifLog([]), { baseWorkspace }, - ); - - await produceBaseline('origin/main', deps); - - expect(runCalls.map(call => `${call.command} ${call.args[0] ?? ''}`)).toStrictEqual([ - 'git worktree', - 'pnpm install', - 'pnpm exec', - 'git worktree', - ]); - expect(runCalls[0]?.args).toContain('abc1234'); - expect(copyCalls).toStrictEqual([ - { - destination: `/repo/dist/${sarifFileNames.base}`, - source: `/tmp/base/dist/${sarifFileNames.current}`, - }, - { - destination: `/repo/packages/a/dist/${sarifFileNames.base}`, - source: `/tmp/base/packages/a/dist/${sarifFileNames.current}`, - }, - ]); - }); - - it('tolerates a failing base lint and copies what it wrote', async ({ expect }) => { - const { copyCalls, deps, errors } = stubDeps( - headWorkspace, baseFiles, sarifLog([]), - { baseWorkspace, failing: ['pnpm exec'] }, - ); - - await produceBaseline('origin/main', deps); - - expect(errors.some(line => line.includes('Base lint'))).toBe(true); - expect(copyCalls).toHaveLength(2); - }); - - it('copies nothing when the base wrote no SARIF logs', async ({ expect }) => { - const { copyCalls, deps } = stubDeps( - headWorkspace, [], sarifLog([]), { baseWorkspace }, - ); - - await produceBaseline('origin/main', deps); - - expect(copyCalls).toHaveLength(0); - }); - - it('removes the worktree when the base install fails', async ({ expect }) => { - const { deps, runCalls } = stubDeps( - headWorkspace, baseFiles, sarifLog([]), - { baseWorkspace, failing: ['pnpm install'] }, - ); - - await expect(produceBaseline('origin/main', deps)).rejects.toThrow( - /pnpm install failed/v, - ); - expect(runCalls.at(-1)?.args).toStrictEqual([ - 'worktree', 'remove', '--force', '/tmp/base', - ]); - }); -}); diff --git a/packages/cli/test/sarif-baseline.test.ts b/packages/cli/test/sarif-baseline.test.ts new file mode 100644 index 00000000..c390693e --- /dev/null +++ b/packages/cli/test/sarif-baseline.test.ts @@ -0,0 +1,196 @@ +import { describe, it } from 'vitest'; +import { + executeSarifBaseline, + executeSarifCompare, + produceBaseline, +} from '#src/lib/sarif-compare.js'; +import type { WorkspaceContext } from '#src/lib/workspace.js'; +import { sarifLog, stubDeps } from './sarif-compare.stub.ts'; + +describe.concurrent(produceBaseline, () => { + const headWorkspace: WorkspaceContext = { + packageDirs: ['/repo/packages/a'], + packageGlobs: ['packages/*'], + rootDir: '/repo', + }; + const baseWorkspace: WorkspaceContext = { + packageDirs: ['/tmp/base/packages/a'], + packageGlobs: ['packages/*'], + rootDir: '/tmp/base', + }; + const baseFiles = [ + '/tmp/base/dist/sarif/eslint.sarif', + '/tmp/base/packages/a/dist/sarif/eslint.sarif', + ]; + + it('lints the merge base in a temp worktree and copies baselines', async ({ + expect, + }) => { + const { copyCalls, deps, removedPaths, runCalls, writeTextCalls } = stubDeps( + headWorkspace, baseFiles, sarifLog([]), { baseWorkspace }, + ); + + await produceBaseline('abc1234', deps); + + expect(runCalls.map(call => `${call.command} ${call.args[0] ?? ''}`)).toStrictEqual([ + 'git worktree', + 'pnpm install', + 'pnpm exec', + 'git worktree', + ]); + expect(runCalls[0]?.args).toContain('abc1234'); + expect(removedPaths).toStrictEqual([ + '/repo/dist/sarif/base', + '/repo/packages/a/dist/sarif/base', + ]); + expect(copyCalls).toStrictEqual([ + { + destination: '/repo/dist/sarif/base/eslint.sarif', + source: '/tmp/base/dist/sarif/eslint.sarif', + }, + { + destination: '/repo/packages/a/dist/sarif/base/eslint.sarif', + source: '/tmp/base/packages/a/dist/sarif/eslint.sarif', + }, + ]); + expect(writeTextCalls).toStrictEqual([ + { content: 'abc1234\n', filePath: '/repo/dist/sarif/base.ref' }, + ]); + }); + + it('tolerates a failing base lint and copies what it wrote', async ({ expect }) => { + const { copyCalls, deps, errors } = stubDeps( + headWorkspace, baseFiles, sarifLog([]), + { baseWorkspace, failing: ['pnpm exec'] }, + ); + + await produceBaseline('abc1234', deps); + + expect(errors.some(line => line.includes('Base lint'))).toBe(true); + expect(copyCalls).toHaveLength(2); + }); + + it('copies nothing when the base wrote no SARIF logs', async ({ expect }) => { + const { copyCalls, deps } = stubDeps( + headWorkspace, [], sarifLog([]), { baseWorkspace }, + ); + + await produceBaseline('abc1234', deps); + + expect(copyCalls).toHaveLength(0); + }); + + it('removes the worktree when the base install fails', async ({ expect }) => { + const { deps, runCalls, writeTextCalls } = stubDeps( + headWorkspace, baseFiles, sarifLog([]), + { baseWorkspace, failing: ['pnpm install'] }, + ); + + await expect(produceBaseline('abc1234', deps)).rejects.toThrow( + /pnpm install failed/v, + ); + expect(runCalls.at(-1)?.args).toStrictEqual([ + 'worktree', 'remove', '--force', '/tmp/base', + ]); + expect(writeTextCalls).toHaveLength(0); + }); +}); + +describe.concurrent(executeSarifBaseline, () => { + const workspace: WorkspaceContext = { + packageDirs: ['/repo/packages/a'], + packageGlobs: ['packages/*'], + rootDir: '/repo', + }; + + it('copies current SARIF logs to baselines and stamps HEAD', async ({ + expect, + }) => { + const files = [ + '/repo/dist/sarif/eslint.sarif', + '/repo/packages/a/dist/sarif/eslint.sarif', + ]; + const { copyCalls, deps, writeTextCalls } = stubDeps(workspace, files, sarifLog([])); + + await executeSarifBaseline(deps); + + expect(copyCalls).toStrictEqual([ + { + destination: '/repo/dist/sarif/base/eslint.sarif', + source: '/repo/dist/sarif/eslint.sarif', + }, + { + destination: '/repo/packages/a/dist/sarif/base/eslint.sarif', + source: '/repo/packages/a/dist/sarif/eslint.sarif', + }, + ]); + expect(writeTextCalls).toStrictEqual([ + { content: 'abc1234\n', filePath: '/repo/dist/sarif/base.ref' }, + ]); + }); + + it('spawns nothing: seeding is pure file copying', async ({ expect }) => { + const { deps, runCalls } = stubDeps( + workspace, + ['/repo/dist/sarif/eslint.sarif'], + sarifLog([]), + ); + + await executeSarifBaseline(deps); + + expect(runCalls).toHaveLength(0); + }); +}); + +describe.concurrent('executeSarifCompare --base', () => { + const workspace: WorkspaceContext = { + packageDirs: [], + packageGlobs: [], + rootDir: '/repo', + }; + const stampedFiles = [ + '/repo/dist/sarif/base.ref', + '/repo/dist/sarif/base/eslint.sarif', + '/repo/dist/sarif/eslint.sarif', + ]; + + it('reuses on-disk baselines when the stamp matches the merge base', async ({ + expect, + }) => { + const { deps, infos, runCalls } = stubDeps( + workspace, stampedFiles, sarifLog([]), + { readTextContent: 'abc1234\n' }, + ); + + await executeSarifCompare({ baseRef: 'origin/main' }, deps); + + expect(infos.some(line => line.includes('already present'))).toBe(true); + expect(runCalls.map(call => call.command)).toStrictEqual(['sarif-multitool']); + }); + + it('produces the baseline when the stamp records another merge base', async ({ + expect, + }) => { + const { deps, runCalls } = stubDeps( + workspace, stampedFiles, sarifLog([]), + { readTextContent: 'other999\n' }, + ); + + await executeSarifCompare({ baseRef: 'origin/main' }, deps); + + expect(runCalls[0]).toMatchObject({ command: 'git' }); + expect(runCalls[0]?.args.slice(0, 2)).toStrictEqual(['worktree', 'add']); + }); + + it('produces the baseline when no stamp exists', async ({ expect }) => { + const { deps, runCalls } = stubDeps( + workspace, + ['/repo/dist/sarif/eslint.sarif', '/repo/dist/sarif/base/eslint.sarif'], + sarifLog([]), + ); + + await executeSarifCompare({ baseRef: 'origin/main' }, deps); + + expect(runCalls[0]?.args.slice(0, 2)).toStrictEqual(['worktree', 'add']); + }); +}); diff --git a/packages/cli/test/sarif-compare.stub.ts b/packages/cli/test/sarif-compare.stub.ts new file mode 100644 index 00000000..1e64c046 --- /dev/null +++ b/packages/cli/test/sarif-compare.stub.ts @@ -0,0 +1,146 @@ +import { faker } from '@faker-js/faker'; +import { vi } from 'vitest'; +import type { Logger } from '#src/lib/logger.js'; +import type { SarifCompareDeps } from '#src/lib/sarif-compare.js'; +import { localeComparer } from '#src/lib/sort.js'; +import type { WorkspaceContext } from '#src/lib/workspace.js'; + +/** Overrides for {@link sarifResult}. */ +export interface SarifResultOverrides { + readonly ruleId?: string; + readonly startLine?: number; + readonly suppressions?: readonly object[]; +} + +/** Builds a SARIF result in the subset shape the compare consumes. */ +export const sarifResult = ( + baselineState: string, + overrides?: SarifResultOverrides, +): object => ({ + baselineState, + level: 'error', + locations: [{ + physicalLocation: { + artifactLocation: { uri: faker.system.filePath() }, + region: { startColumn: 1, startLine: overrides?.startLine ?? 1 }, + }, + }], + message: { text: faker.lorem.sentence() }, + ruleId: overrides?.ruleId ?? 'no-unused-vars', + ...(overrides?.suppressions !== undefined && + { suppressions: overrides.suppressions }), +}); + +/** Wraps results in a single-run SARIF log. */ +export const sarifLog = (results: readonly object[]): object => ({ + runs: [{ results }], +}); + +/** A recorded deps.run invocation. */ +export interface RunCall { + readonly args: readonly string[]; + readonly command: string; +} + +/** A recorded deps.copyFile invocation. */ +export interface CopyCall { + readonly destination: string; + readonly source: string; +} + +/** A recorded deps.writeText invocation. */ +export interface WriteTextCall { + readonly content: string; + readonly filePath: string; +} + +/** Stubbed deps plus the calls they record. */ +export interface StubDeps { + readonly copyCalls: readonly CopyCall[]; + readonly deps: SarifCompareDeps; + readonly errors: readonly string[]; + readonly infos: readonly string[]; + readonly removedPaths: readonly string[]; + readonly runCalls: readonly RunCall[]; + readonly writeTextCalls: readonly WriteTextCall[]; +} + +/** Options for {@link stubDeps}. */ +export interface StubOptions { + /** Workspace returned for the base worktree cwd. Defaults to `workspace`. */ + readonly baseWorkspace?: WorkspaceContext; + /** `" "` prefixes whose run() rejects. */ + readonly failing?: readonly string[]; + /** Content returned by readText for any path (e.g. the stamp file). */ + readonly readTextContent?: string; +} + +const normalize = (filePath: string): string => filePath.replaceAll('\\', '/'); + +/** + * Fakes a workspace whose files are the members of `files`; `list` + * derives directory listings from it, and readJson yields `log` for + * any path (matched output or current log alike). + */ +export const stubDeps = ( + workspace: WorkspaceContext, + files: readonly string[], + log: object, + options?: StubOptions, +): StubDeps => { + const copyCalls: CopyCall[] = []; + const errors: string[] = []; + const infos: string[] = []; + const removedPaths: string[] = []; + const runCalls: RunCall[] = []; + const writeTextCalls: WriteTextCall[] = []; + const logger: Logger = { + error: (...args) => void errors.push(args.join(' ')), + info: (...args) => void infos.push(args.join(' ')), + }; + return { + copyCalls, + deps: { + capture: () => Promise.resolve('abc1234'), + copyFile: (source, destination) => { + copyCalls.push({ + destination: normalize(destination), + source: normalize(source), + }); + }, + ensureDir: vi.fn<(dir: string) => void>(), + exists: filePath => files.includes(normalize(filePath)), + list: (dir) => { + const prefix = `${normalize(dir)}/`; + return files + .filter(filePath => filePath.startsWith(prefix)) + .map(filePath => filePath.slice(prefix.length)) + .filter(name => !name.includes('/') && name.endsWith('.sarif')) + .toSorted(localeComparer); + }, + logger, + makeTempDir: () => '/tmp/base', + readJson: () => log, + readText: () => options?.readTextContent ?? '', + remove: filePath => void removedPaths.push(normalize(filePath)), + resolveMultitool: () => 'sarif-multitool', + run: (command, runOptions) => { + runCalls.push({ args: runOptions?.args ?? [], command }); + const key = `${command} ${runOptions?.args?.[0] ?? ''}`; + return options?.failing?.includes(key) === true + ? Promise.reject(new Error(`${key} failed`)) + : Promise.resolve(); + }, + workspace: cwd => + (cwd === undefined ? workspace : options?.baseWorkspace ?? workspace), + writeText: (filePath, content) => { + writeTextCalls.push({ content, filePath: normalize(filePath) }); + }, + }, + errors, + infos, + removedPaths, + runCalls, + writeTextCalls, + }; +}; diff --git a/packages/cli/test/sarif-compare.test.ts b/packages/cli/test/sarif-compare.test.ts new file mode 100644 index 00000000..31b52d3b --- /dev/null +++ b/packages/cli/test/sarif-compare.test.ts @@ -0,0 +1,168 @@ +import { describe, it } from 'vitest'; +import { + type NewFinding, + executeSarifCompare, + extractAllFindings, + extractNewFindings, + formatNewFindings, + parseSarifLog, +} from '#src/lib/sarif-compare.js'; +import type { WorkspaceContext } from '#src/lib/workspace.js'; +import { sarifLog, sarifResult, stubDeps } from './sarif-compare.stub.ts'; + +describe.concurrent(extractNewFindings, () => { + it('keeps only results the baseliner classified as new', ({ expect }) => { + const newResult = sarifResult('new', { ruleId: 'no-console' }); + const log = sarifLog([ + sarifResult('unchanged'), + sarifResult('updated'), + newResult, + sarifResult('absent'), + ]); + + const findings = extractNewFindings(parseSarifLog(log)); + + expect(findings).toHaveLength(1); + expect(findings[0]).toMatchObject({ level: 'error', ruleId: 'no-console' }); + }); + + it('exempts suppressed findings from the gate', ({ expect }) => { + const log = parseSarifLog(sarifLog([ + sarifResult('new', { suppressions: [{ kind: 'inSource' }] }), + ])); + + expect(extractNewFindings(log)).toHaveLength(0); + }); + + it('falls back to placeholders when location or rule is absent', ({ expect }) => { + const log = parseSarifLog(sarifLog([ + { baselineState: 'new', message: { text: 'boom' } }, + ])); + + expect(extractNewFindings(log)).toStrictEqual([{ + column: undefined, + level: 'error', + line: undefined, + message: 'boom', + ruleId: 'internal', + uri: '', + }]); + }); +}); + +describe.concurrent(extractAllFindings, () => { + it('treats every unsuppressed result as new regardless of state', ({ expect }) => { + const log = parseSarifLog(sarifLog([ + sarifResult('unchanged'), + sarifResult('unchanged', { suppressions: [{ kind: 'inSource' }] }), + ])); + + expect(extractAllFindings(log)).toHaveLength(1); + }); +}); + +describe.concurrent(formatNewFindings, () => { + it('renders uri, position, level, message, and rule per line', ({ expect }) => { + const finding: NewFinding = { + column: 3, + level: 'error', + line: 7, + message: 'Unexpected console statement.', + ruleId: 'no-console', + uri: 'file:///repo/src/app.js', + }; + + expect(formatNewFindings([finding])).toBe( + 'file:///repo/src/app.js:7:3 error Unexpected console statement. no-console', + ); + }); + + it('omits the position when the result has no region', ({ expect }) => { + const finding: NewFinding = { + column: undefined, + level: 'warning', + line: undefined, + message: 'boom', + ruleId: 'internal', + uri: '', + }; + + expect(formatNewFindings([finding])).toBe(' warning boom internal'); + }); +}); + +describe.concurrent(executeSarifCompare, () => { + const workspace: WorkspaceContext = { + packageDirs: ['/repo/packages/a'], + packageGlobs: ['packages/*'], + rootDir: '/repo', + }; + const currentFiles = [ + '/repo/dist/sarif/eslint.sarif', + '/repo/dist/sarif/base/eslint.sarif', + '/repo/packages/a/dist/sarif/eslint.sarif', + '/repo/packages/a/dist/sarif/base/eslint.sarif', + ]; + + it('matches every SARIF log forward against its baseline', async ({ expect }) => { + const { deps, runCalls } = stubDeps(workspace, currentFiles, sarifLog([])); + + await executeSarifCompare({}, deps); + + expect(runCalls).toHaveLength(2); + expect(runCalls[0]).toMatchObject({ command: 'sarif-multitool' }); + expect(runCalls[0]?.args[0]).toBe('match-results-forward'); + }); + + it('resolves and reports when no results are new', async ({ expect }) => { + const { deps, infos } = stubDeps( + workspace, + currentFiles, + sarifLog([sarifResult('unchanged'), sarifResult('updated')]), + ); + + await executeSarifCompare({}, deps); + + expect(infos).toContain('No new findings'); + }); + + it('rejects when any result is new', async ({ expect }) => { + const { deps } = stubDeps(workspace, currentFiles, sarifLog([sarifResult('new')])); + + await expect(executeSarifCompare({}, deps)).rejects.toThrow(/new finding/v); + }); + + it('skips dirs with no current SARIF logs', async ({ expect }) => { + const { deps, runCalls } = stubDeps(workspace, [], sarifLog([])); + + await executeSarifCompare({}, deps); + + expect(runCalls).toHaveLength(0); + }); + + it('treats a missing baseline as empty: all findings are new', async ({ + expect, + }) => { + const { deps, errors, runCalls } = stubDeps( + workspace, + ['/repo/dist/sarif/eslint.sarif'], + sarifLog([sarifResult('unchanged')]), + ); + + await expect(executeSarifCompare({}, deps)).rejects.toThrow(/new finding/v); + expect(runCalls).toHaveLength(0); + expect(errors.some(line => line.includes('all findings are new'))).toBe(true); + }); + + it('passes with a missing baseline when the log is clean', async ({ expect }) => { + const { deps, infos } = stubDeps( + workspace, + ['/repo/dist/sarif/eslint.sarif'], + sarifLog([sarifResult('unchanged', { suppressions: [{ kind: 'inSource' }] })]), + ); + + await executeSarifCompare({}, deps); + + expect(infos).toContain('No new findings'); + }); +}); diff --git a/turbo.json b/turbo.json index dd3e9380..d7deef7d 100644 --- a/turbo.json +++ b/turbo.json @@ -12,7 +12,7 @@ ], "outputs": [ "dist/.eslintcache", - "dist/eslint.sarif" + "dist/sarif/eslint.sarif" ] }, "build": { @@ -129,7 +129,7 @@ ], "outputs": [ "dist/.eslintcache", - "dist/eslint.sarif" + "dist/sarif/eslint.sarif" ] }, "pack": { From 6720268a100d9699906ccb0b7e479e0aa16ba736 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Tue, 21 Jul 2026 09:01:39 -0500 Subject: [PATCH 04/15] Resolve CI baseline from merge ref parent, not history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the full-history checkout (fetch-depth: 0) with the merge-ref first-parent technique: on the PR merge commit both CI providers check out, HEAD^1 is the target branch head — which is the merge base of the merged checkout — so a fetch-depth: 2 checkout resolves it with no branch fetch and no merge-base computation. A guard fails fast if the job is not on a two-parent merge ref, where HEAD^1 would silently name the wrong commit. - `gtb sarif compare` gains --base-sha (exact commit, mutually exclusive with --base); CI passes `git rev-parse HEAD^1`. Local runs keep --base merge-base semantics, where full history is free. - Baseline production now ensures the commit exists locally, fetching it by SHA at depth 1 when absent — which also fixes local shallow clones. - Cache keys become base-tip SHAs, matching exactly what the default-branch seed job stamps, so cross-PR baseline cache hits stop depending on the branch being up to date with main. Co-Authored-By: Claude Fable 5 --- .changeset/sarif-lint-regression-compare.md | 9 ++-- .github/workflows/lint-regression.yml | 25 ++++++----- AGENTS.md | 9 ++-- packages/cli/src/commands/root/sarif.ts | 11 ++++- packages/cli/src/lib/sarif-compare.ts | 50 ++++++++++++++++++--- packages/cli/test/sarif-baseline.test.ts | 41 +++++++++++++++++ packages/cli/test/sarif-compare.stub.ts | 9 +++- 7 files changed, 127 insertions(+), 27 deletions(-) diff --git a/.changeset/sarif-lint-regression-compare.md b/.changeset/sarif-lint-regression-compare.md index 3ee62cce..0fe997a3 100644 --- a/.changeset/sarif-lint-regression-compare.md +++ b/.changeset/sarif-lint-regression-compare.md @@ -12,10 +12,11 @@ with `dist/sarif/base/.sarif` and fails only on findings the SARIF log into `dist/sarif/` is gated with no extra wiring. A missing baseline counts as empty (all findings new), and findings carrying in-source suppressions are exempt. With -`--base ` it produces the baseline itself by linting the merge -base in a temporary git worktree, skipping production when the -`dist/sarif/base.ref` stamp already records that merge base (locally or -via CI caches). `gtb sarif baseline` snapshots HEAD's logs as the +`--base ` (merge-base of a ref, for local runs) or `--base-sha +` (exact commit, for CI's shallow merge-ref checkouts) it produces +the baseline itself by linting that commit in a temporary git worktree, +skipping production when the `dist/sarif/base.ref` stamp already +records it (locally or via CI caches). `gtb sarif baseline` snapshots HEAD's logs as the baseline so default-branch CI can seed a cross-PR cache (see the new `lint-regression.yml` reusable workflow). Local changed-file enforcement via the pre-commit ESLint step is unaffected. diff --git a/.github/workflows/lint-regression.yml b/.github/workflows/lint-regression.yml index e02e4e43..fd7dcbf4 100644 --- a/.github/workflows/lint-regression.yml +++ b/.github/workflows/lint-regression.yml @@ -12,9 +12,11 @@ jobs: steps: - uses: actions/checkout@v7 with: - # The compare lints the merge base in a throwaway worktree, so - # the base branch history must be reachable locally. - fetch-depth: 0 + # Depth 2 resolves both parents of the PR merge ref the + # checkout lands on; the first parent is the target branch + # head — the merge base of the merged checkout — so no branch + # fetch or merge-base resolution is needed. + fetch-depth: 2 - uses: gtbuchanan/tooling/.github/actions/mise-setup@main @@ -38,11 +40,14 @@ jobs: ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} turbo run lint - - env: - BASE_REF: ${{ github.event.pull_request.base.ref }} - id: merge-base - name: Resolve the merge base - run: echo "sha=$(git merge-base "origin/$BASE_REF" HEAD)" >> "$GITHUB_OUTPUT" + # Fail fast if the checkout is not the merge ref (a two-parent + # commit): re-checking-out the PR head would make HEAD^1 the + # head's own parent, silently gating against the wrong commit. + - id: merge-base + name: Resolve the merge base (merge ref first parent) + run: | + git rev-parse --verify HEAD^2 > /dev/null + echo "sha=$(git rev-parse HEAD^1)" >> "$GITHUB_OUTPUT" # Restore baselines produced by an earlier run against the same # merge base; the compare's stamp check skips the worktree lint on @@ -58,12 +63,12 @@ jobs: - continue-on-error: true env: - BASE_REF: ${{ github.event.pull_request.base.ref }} + BASE_SHA: ${{ steps.merge-base.outputs.sha }} id: compare name: Compare lint results against the merge base run: | ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} \ - sarif compare --base "origin/$BASE_REF" 2>&1 \ + sarif compare --base-sha "$BASE_SHA" 2>&1 \ | tee lint-regression.txt exit "${PIPESTATUS[0]}" diff --git a/AGENTS.md b/AGENTS.md index 02386b96..15c814fb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,9 +109,12 @@ reporter. Three layers: findings that merely moved stay matched — no git-diff offset math. `--base ` makes it self-sufficient: it lints the merge base of that ref and HEAD in a throwaway git worktree to produce the - baselines, so the same invocation works locally - (`gtb sarif compare --base origin/main`) and in CI - (`lint-regression.yml`). A stamp (`dist/sarif/base.ref`, recording + baselines (`gtb sarif compare --base origin/main` locally, where + full history makes `git merge-base` cheap). CI + (`lint-regression.yml`) passes `--base-sha` with the PR merge ref's + first parent instead — on the merged checkout the target branch head + _is_ the merge base — so a `fetch-depth: 2` checkout suffices, and a + missing commit is fetched by SHA at depth 1. A stamp (`dist/sarif/base.ref`, recording the merge-base SHA) skips production when the on-disk baselines are already current; CI seeds a cross-PR baseline cache from each main commit via `gtb sarif baseline` (where merge base == HEAD, the diff --git a/packages/cli/src/commands/root/sarif.ts b/packages/cli/src/commands/root/sarif.ts index 1d0d8278..036fcd11 100644 --- a/packages/cli/src/commands/root/sarif.ts +++ b/packages/cli/src/commands/root/sarif.ts @@ -11,18 +11,25 @@ import { rootNames } from './names.ts'; */ const compare = defineCommand({ args: { - base: { + 'base': { description: 'Git ref to diff against; lints its merge base with HEAD in a ' + 'temporary worktree to produce the baseline (e.g. origin/main)', type: 'string', }, + 'base-sha': { + description: + 'Exact baseline commit, skipping merge-base resolution; CI ' + + "passes the PR merge ref's first parent (git rev-parse HEAD^1)", + type: 'string', + }, }, meta: { description: 'Fail when SARIF findings are new relative to the baseline', name: 'compare', }, - run: ({ args }) => executeSarifCompare({ baseRef: args.base }), + run: ({ args }) => + executeSarifCompare({ baseRef: args.base, baseSha: args['base-sha'] }), }); /** diff --git a/packages/cli/src/lib/sarif-compare.ts b/packages/cli/src/lib/sarif-compare.ts index b8082eb1..9cb42967 100644 --- a/packages/cli/src/lib/sarif-compare.ts +++ b/packages/cli/src/lib/sarif-compare.ts @@ -233,10 +233,23 @@ export const hasCurrentBaseline = (sha: string, deps: SarifCompareDeps): boolean * Base commits that predate SARIF output simply produce no baseline, * and the compare skips those packages. */ +/** + * Ensures the commit's objects exist locally, fetching just that commit + * on shallow clones (GitHub serves reachable SHAs directly). + */ +const ensureCommit = async (sha: string, deps: SarifCompareDeps): Promise => { + try { + await deps.capture('git', ['cat-file', '-e', sha]); + } catch { + await deps.run('git', { args: ['fetch', '--depth=1', 'origin', sha] }); + } +}; + export const produceBaseline = async ( sha: string, deps: SarifCompareDeps, ): Promise => { + await ensureCommit(sha, deps); const baseDir = deps.makeTempDir(); try { await deps.run('git', { args: ['worktree', 'add', '--detach', baseDir, sha] }); @@ -338,15 +351,38 @@ export const executeSarifBaseline = async ( /** Options for {@link executeSarifCompare}. */ export interface SarifCompareOptions { /** - * Git ref to diff against. When set, the baseline is produced by - * linting the merge base of this ref and HEAD in a throwaway - * worktree (skipped when the stamp already records that merge base). - * When unset, `dist/sarif/base/` must already be populated (e.g. - * restored from a cache or a prior run). + * Git ref to diff against. The baseline commit is the merge base of + * this ref and HEAD (a `git merge-base` call, so it needs local + * history — the local mode). */ readonly baseRef?: string | undefined; + /** + * Exact baseline commit, no merge-base resolution. CI passes the PR + * merge ref's first parent (`git rev-parse HEAD^1`): on the merged + * checkout, the target branch head *is* the merge base, so this + * needs no branch fetch or history. Mutually exclusive with + * `baseRef`. When neither is set, `dist/sarif/base/` must already be + * populated (e.g. restored from a cache or a prior run). + */ + readonly baseSha?: string | undefined; } +const resolveBaselineSha = async ( + options: SarifCompareOptions, + deps: SarifCompareDeps, +): Promise => { + if (options.baseRef !== undefined && options.baseSha !== undefined) { + throw new Error('--base and --base-sha are mutually exclusive'); + } + if (options.baseSha !== undefined) { + return options.baseSha; + } + if (options.baseRef !== undefined) { + return deps.capture('git', ['merge-base', options.baseRef, 'HEAD']); + } + return undefined; +}; + /** * Compares every SARIF log under each lint cwd's `dist/sarif/` against * its baseline via `sarif-multitool match-results-forward` and rejects @@ -358,8 +394,8 @@ export const executeSarifCompare = async ( options: SarifCompareOptions = {}, deps: SarifCompareDeps = defaultDeps, ): Promise => { - if (options.baseRef !== undefined) { - const sha = await deps.capture('git', ['merge-base', options.baseRef, 'HEAD']); + const sha = await resolveBaselineSha(options, deps); + if (sha !== undefined) { if (hasCurrentBaseline(sha, deps)) { deps.logger.info(`Baselines for merge base ${sha} already present; reusing`); } else { diff --git a/packages/cli/test/sarif-baseline.test.ts b/packages/cli/test/sarif-baseline.test.ts index c390693e..42933c90 100644 --- a/packages/cli/test/sarif-baseline.test.ts +++ b/packages/cli/test/sarif-baseline.test.ts @@ -193,4 +193,45 @@ describe.concurrent('executeSarifCompare --base', () => { expect(runCalls[0]?.args.slice(0, 2)).toStrictEqual(['worktree', 'add']); }); + + it('uses an exact commit from --base-sha without merge-base resolution', async ({ + expect, + }) => { + const { deps, runCalls } = stubDeps( + workspace, stampedFiles, sarifLog([]), + { readTextContent: 'fedc987\n' }, + ); + + await executeSarifCompare({ baseSha: 'fedc987' }, deps); + + // Stamp matches the given SHA, so no git commands run at all. + expect(runCalls.map(call => call.command)).toStrictEqual(['sarif-multitool']); + }); + + it('rejects when --base and --base-sha are both given', async ({ expect }) => { + const { deps } = stubDeps(workspace, stampedFiles, sarifLog([])); + + await expect( + executeSarifCompare({ baseRef: 'origin/main', baseSha: 'fedc987' }, deps), + ).rejects.toThrow(/mutually exclusive/v); + }); + + it('fetches the baseline commit when absent from a shallow clone', async ({ + expect, + }) => { + const { deps, runCalls } = stubDeps( + workspace, + ['/repo/dist/sarif/eslint.sarif', '/repo/dist/sarif/base/eslint.sarif'], + sarifLog([]), + { failingCapture: ['cat-file -e'] }, + ); + + await executeSarifCompare({ baseSha: 'fedc987' }, deps); + + expect(runCalls[0]).toMatchObject({ + args: ['fetch', '--depth=1', 'origin', 'fedc987'], + command: 'git', + }); + expect(runCalls[1]?.args.slice(0, 2)).toStrictEqual(['worktree', 'add']); + }); }); diff --git a/packages/cli/test/sarif-compare.stub.ts b/packages/cli/test/sarif-compare.stub.ts index 1e64c046..9123f8db 100644 --- a/packages/cli/test/sarif-compare.stub.ts +++ b/packages/cli/test/sarif-compare.stub.ts @@ -71,6 +71,8 @@ export interface StubOptions { readonly baseWorkspace?: WorkspaceContext; /** `" "` prefixes whose run() rejects. */ readonly failing?: readonly string[]; + /** `" "` prefixes whose capture() rejects. */ + readonly failingCapture?: readonly string[]; /** Content returned by readText for any path (e.g. the stamp file). */ readonly readTextContent?: string; } @@ -101,7 +103,12 @@ export const stubDeps = ( return { copyCalls, deps: { - capture: () => Promise.resolve('abc1234'), + capture: (_command, args) => { + const key = `${args[0] ?? ''} ${args[1] ?? ''}`; + return options?.failingCapture?.includes(key) === true + ? Promise.reject(new Error(`${key} failed`)) + : Promise.resolve('abc1234'); + }, copyFile: (source, destination) => { copyCalls.push({ destination: normalize(destination), From c1d2ca94c554f38d83a16d8ea53276d053ba3c0f Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Tue, 21 Jul 2026 09:41:46 -0500 Subject: [PATCH 05/15] Honor override label via live reads and a job re-run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Enforce step read labels from the event payload, but a re-run replays the original payload — so a label applied after the failure could never take effect, and no automatic path re-evaluated the check either (label events don't trigger the pipeline, deliberately: that would rebuild everything on any label change). Accept flow is now: apply the label, re-run the failed job. Enforce queries the live label state, so the stale payload doesn't matter. Dismissal only runs on the first attempt of a synchronize — a re-run of the replayed push event must not strip a label applied after it — and skips fork PRs, whose read-only token can't edit labels (their labels are still honored on re-runs, which only read). The PR comment now spells out the label + re-run ceremony. Co-Authored-By: Claude Fable 5 --- .github/workflows/lint-regression.yml | 52 ++++++++++++++++++++------- .github/workflows/release.yml | 4 +-- AGENTS.md | 5 +-- 3 files changed, 44 insertions(+), 17 deletions(-) diff --git a/.github/workflows/lint-regression.yml b/.github/workflows/lint-regression.yml index fd7dcbf4..97c3b4ce 100644 --- a/.github/workflows/lint-regression.yml +++ b/.github/workflows/lint-regression.yml @@ -23,13 +23,22 @@ jobs: - uses: gtbuchanan/tooling/.github/actions/pnpm-tasks@main # A label only vouches for the commits it was applied to. Drop it - # on every new push so a maintainer has to re-accept. + # on the first attempt of every new push so a maintainer has to + # re-accept — but never on re-runs, which replay the original + # synchronize payload and would otherwise strip a label applied + # after the push (re-running the job is exactly how a fresh label + # takes effect; see Enforce). Fork PRs run with a read-only token + # that can't edit labels; Enforce discounts the label on + # first-attempt synchronize, so skipping the dismissal there + # costs nothing. - env: GH_TOKEN: ${{ github.token }} PR: ${{ github.event.pull_request.number }} if: >- github.event.action == 'synchronize' && - contains(github.event.pull_request.labels.*.name, inputs.override-label) + github.run_attempt == '1' && + contains(github.event.pull_request.labels.*.name, inputs.override-label) && + !github.event.pull_request.head.repo.fork name: Dismiss stale regression acceptance run: gh pr edit "$PR" --remove-label '${{ inputs.override-label }}' @@ -89,20 +98,35 @@ jobs: cat lint-regression.txt echo '```' echo - echo 'Apply the `${{ inputs.override-label }}` label to accept.' + echo 'Apply the `${{ inputs.override-label }}` label and' \ + 're-run this job to accept.' } > comment.md gh pr comment "$PR" --body-file comment.md \ --edit-last --create-if-none # Run-but-neutral override: the label lets the check pass, but the - # comment above still records what was accepted. A label present - # on a `synchronize` run was just dismissed, so it doesn't count. - - if: >- - steps.compare.outcome == 'failure' && - (github.event.action == 'synchronize' || - !contains(github.event.pull_request.labels.*.name, inputs.override-label)) + # comment above still records what was accepted. Labels are read + # live (not from the event payload) so the accept flow is: apply + # the label, then re-run this job — a re-run replays the original + # payload, which predates the label. The label doesn't count on a + # first-attempt synchronize, where the dismissal step just + # dropped it (or would have, for forks). + - env: + EVENT_ACTION: ${{ github.event.action }} + GH_TOKEN: ${{ github.token }} + LABEL: ${{ inputs.override-label }} + PR: ${{ github.event.pull_request.number }} + if: steps.compare.outcome == 'failure' name: Enforce - run: exit 1 + run: | + if { [ "$GITHUB_RUN_ATTEMPT" -gt 1 ] || + [ "$EVENT_ACTION" != 'synchronize' ]; } && + gh pr view "$PR" --json labels --jq '.labels[].name' | + grep -Fxq "$LABEL"; then + echo "'$LABEL' label present; accepting the new findings" + exit 0 + fi + exit 1 # On the default branch, any future PR's merge base is HEAD itself, # so its baseline is just this commit's own lint output. Caches @@ -147,8 +171,9 @@ name: Lint Regression # in a temporary git worktree to build the SARIF baseline, then # `sarif-multitool` classifies each HEAD result as new or matched — # pre-existing (accepted) violations never block. A maintainer-applied -# override label turns a failing compare into a pass while the PR -# comment keeps the accepted violations on record. +# override label turns a failing compare into a pass — apply it, then +# re-run the failed job — while the PR comment keeps the accepted +# violations on record. on: workflow_call: inputs: @@ -164,7 +189,8 @@ on: default: accepted-lint-regression description: >- PR label that accepts the new violations (maintainer-applied; - dismissed automatically on every new push). + takes effect on a job re-run; dismissed automatically on + every new push). type: string seed: default: false diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1b48c6e1..2aa7eeb9 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -19,8 +19,8 @@ jobs: uses: ./.github/workflows/ci.yml # Seed the cross-PR lint baseline cache from this commit's own lint - # output. `needs: ci` lets the seed's lint hit the remote cache that - # CI just warmed instead of re-linting. + # output. `needs: ci` orders it after the CI gate; with a turbo + # remote cache configured its lint also becomes a cache hit. lint-baseline: name: Lint needs: ci diff --git a/AGENTS.md b/AGENTS.md index 15c814fb..d0957415 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -414,8 +414,9 @@ through `package.json` scripts backed by `gtb` leaf commands. merge base in a throwaway git worktree and diffs the SARIF logs via `sarif-multitool`. A maintainer-applied override label (default `accepted-lint-regression`, dismissed on every new push) turns a - failing compare into a pass while a PR comment records the accepted - violations. Caller must grant `pull-requests: write`; fork PRs get + failing compare into a pass — apply it, then re-run the failed job + (labels are read live, so the replayed event payload doesn't matter) + — while a PR comment records the accepted violations. Caller must grant `pull-requests: write`; fork PRs get the check failure without the comment (read-only token). - **`pre-commit.yml`** — Runs the `hk:base` mise task on PR changed files (hk resolved from mise). The `use-pnpm` input (default From c18c431b93d0f54a6996920ba08eb7e67bae3724 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Tue, 21 Jul 2026 10:48:13 -0500 Subject: [PATCH 06/15] Gate the override label by applier role MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub has no per-label permission rules — anyone with triage can apply any label, and a PR author with write could self-accept their own regressions. The Enforce step now resolves the label's most recent applier from the issue events API and honors it only when their repository role meets the new `override-role` input (default `maintain`, so ordinary committers can't self-accept); unresolvable appliers or roles fail closed. Also reframe the docs and PR comment: the routine response to new findings is fixing or suppressing them in-source (suppressed findings are already gate-exempt); the label is the escape hatch for bulk introductions like a dependency bump shipping a new rule, where per-instance suppression is noise and disabling the rule would let new violations creep in while it's off. Co-Authored-By: Claude Fable 5 --- .github/workflows/lint-regression.yml | 66 ++++++++++++++++++++++----- AGENTS.md | 24 ++++++---- 2 files changed, 70 insertions(+), 20 deletions(-) diff --git a/.github/workflows/lint-regression.yml b/.github/workflows/lint-regression.yml index 97c3b4ce..95ce777a 100644 --- a/.github/workflows/lint-regression.yml +++ b/.github/workflows/lint-regression.yml @@ -98,8 +98,11 @@ jobs: cat lint-regression.txt echo '```' echo - echo 'Apply the `${{ inputs.override-label }}` label and' \ - 're-run this job to accept.' + echo 'Fix the new findings or suppress them in-source with' \ + 'a reason (suppressed findings are exempt). For a bulk' \ + 'introduction (e.g. a dependency bump adding a rule), a' \ + 'maintainer can apply the `${{ inputs.override-label }}`' \ + 'label and re-run this job to accept them.' } > comment.md gh pr comment "$PR" --body-file comment.md \ --edit-last --create-if-none @@ -110,12 +113,17 @@ jobs: # the label, then re-run this job — a re-run replays the original # payload, which predates the label. The label doesn't count on a # first-attempt synchronize, where the dismissal step just - # dropped it (or would have, for forks). + # dropped it (or would have, for forks). GitHub has no per-label + # permission rules — anyone with triage can apply any label — so + # the label is honored only when its most recent applier holds + # `override-role` or higher; anything unresolvable fails closed. - env: EVENT_ACTION: ${{ github.event.action }} GH_TOKEN: ${{ github.token }} LABEL: ${{ inputs.override-label }} + OVERRIDE_ROLE: ${{ inputs.override-role }} PR: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} if: steps.compare.outcome == 'failure' name: Enforce run: | @@ -123,8 +131,30 @@ jobs: [ "$EVENT_ACTION" != 'synchronize' ]; } && gh pr view "$PR" --json labels --jq '.labels[].name' | grep -Fxq "$LABEL"; then - echo "'$LABEL' label present; accepting the new findings" - exit 0 + applier=$(gh api "repos/$REPO/issues/$PR/events" --paginate \ + --jq '.[] | select(.event == "labeled") + | "\(.label.name)\t\(.actor.login)"' | + awk -F '\t' -v l="$LABEL" '$1 == l { a = $2 } END { print a }' \ + || true) + role=$(gh api "repos/$REPO/collaborators/$applier/permission" \ + --jq '.role_name' || true) + case "$OVERRIDE_ROLE" in + admin) allowed='admin' ;; + write) allowed='admin maintain write' ;; + *) allowed='admin maintain' ;; + esac + case " $allowed " in + *" $role "*) + echo "'$LABEL' applied by $applier ($role); accepting" \ + 'the new findings' + exit 0 + ;; + *) + echo "'$LABEL' applied by ${applier:-unknown}" \ + "(${role:-unknown}) — needs $OVERRIDE_ROLE or higher;" \ + 'ignoring the label' + ;; + esac fi exit 1 @@ -170,10 +200,14 @@ name: Lint Regression # its merge base. `gtb sarif compare --base` lints the merge base # in a temporary git worktree to build the SARIF baseline, then # `sarif-multitool` classifies each HEAD result as new or matched — -# pre-existing (accepted) violations never block. A maintainer-applied -# override label turns a failing compare into a pass — apply it, then -# re-run the failed job — while the PR comment keeps the accepted -# violations on record. +# pre-existing (accepted) violations never block. The routine response +# to new findings is fixing or suppressing them in-source (suppressed +# findings are gate-exempt); for bulk introductions (e.g. a dependency +# bump adding a rule) the override label turns a failing compare into a +# pass for one merge — apply it, then re-run the failed job — while the +# PR comment keeps the accepted violations on record. The label counts +# only when applied by `override-role` or higher, since GitHub has no +# per-label permission rules of its own. on: workflow_call: inputs: @@ -188,9 +222,17 @@ on: override-label: default: accepted-lint-regression description: >- - PR label that accepts the new violations (maintainer-applied; - takes effect on a job re-run; dismissed automatically on - every new push). + PR label that accepts the new violations for one merge (takes + effect on a job re-run; dismissed automatically on every new + push; honored only when applied by `override-role` or + higher). The escape hatch for bulk introductions — the + routine path is fixing or suppressing findings in-source. + type: string + override-role: + default: maintain + description: >- + Minimum repository role (`write`, `maintain`, or `admin`) + the override label's applier must hold for it to count. type: string seed: default: false diff --git a/AGENTS.md b/AGENTS.md index d0957415..eb4fb0cd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -130,9 +130,14 @@ reporter. Three layers: warnings inline — new violations are still caught at authoring time; the ratchet only governs what lands. -Accepted findings enter the baseline by merging with the override -label (see `lint-regression.yml`) and surface in every SARIF log until -paid down. +Accepting new findings is an in-source act by default: fix them, or +suppress them with a reason (suppressed findings are gate-exempt but +stay in the logs). The override label (see `lint-regression.yml`) is +the escape hatch for bulk introductions — e.g. a dependency bump +shipping a new rule — where per-instance suppression would be noise +and disabling the rule would let new violations creep in while it's +off. Findings accepted either way enter the ephemeral baseline once +merged and surface in every SARIF log until paid down. ### Pre-commit hooks @@ -412,11 +417,14 @@ through `package.json` scripts backed by `gtb` leaf commands. lint baselining section). Lints HEAD, then runs `gtb sarif compare --base origin/`, which lints the merge base in a throwaway git worktree and diffs the SARIF logs via - `sarif-multitool`. A maintainer-applied override label (default - `accepted-lint-regression`, dismissed on every new push) turns a - failing compare into a pass — apply it, then re-run the failed job - (labels are read live, so the replayed event payload doesn't matter) - — while a PR comment records the accepted violations. Caller must grant `pull-requests: write`; fork PRs get + `sarif-multitool`. New findings are routinely fixed or suppressed + in-source; for bulk introductions the override label (default + `accepted-lint-regression`, dismissed on every new push, honored + only when applied by `override-role`+ — default `maintain`) turns a + failing compare into a pass for one merge — apply it, then re-run + the failed job (labels are read live, so the replayed event payload + doesn't matter) — while a PR comment records the accepted + violations. Caller must grant `pull-requests: write`; fork PRs get the check failure without the comment (read-only token). - **`pre-commit.yml`** — Runs the `hk:base` mise task on PR changed files (hk resolved from mise). The `use-pnpm` input (default From 8be015b4666090580d0d7fa4d49daf117d56a006 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Tue, 21 Jul 2026 10:54:15 -0500 Subject: [PATCH 07/15] Discount the override label on reopened PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Labels survive PR closure, and pushes to a closed PR emit no synchronize events — so a reopened head may have moved under a stale label with nothing dismissing it. Treat reopened like synchronize in both the dismissal step and Enforce: the label is dropped and discounted on the first attempt, so acceptance always postdates the code it vouches for. Also record the known residual: cancelling a run before its dismissal step and re-running preserves a stale label, which needs write access and deliberate timing. Co-Authored-By: Claude Fable 5 --- .github/workflows/lint-regression.yml | 36 ++++++++++++++++----------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/.github/workflows/lint-regression.yml b/.github/workflows/lint-regression.yml index 95ce777a..a1cc1814 100644 --- a/.github/workflows/lint-regression.yml +++ b/.github/workflows/lint-regression.yml @@ -23,19 +23,20 @@ jobs: - uses: gtbuchanan/tooling/.github/actions/pnpm-tasks@main # A label only vouches for the commits it was applied to. Drop it - # on the first attempt of every new push so a maintainer has to - # re-accept — but never on re-runs, which replay the original - # synchronize payload and would otherwise strip a label applied - # after the push (re-running the job is exactly how a fresh label - # takes effect; see Enforce). Fork PRs run with a read-only token - # that can't edit labels; Enforce discounts the label on - # first-attempt synchronize, so skipping the dismissal there - # costs nothing. + # on the first attempt of every new push — and on reopen, since + # labels survive closure and pushes to a closed PR emit no + # synchronize events, so a reopened head may have moved under a + # stale label — but never on re-runs, which replay the original + # payload and would otherwise strip a label applied after the + # push (re-running the job is exactly how a fresh label takes + # effect; see Enforce). Fork PRs run with a read-only token that + # can't edit labels; Enforce discounts the label on these same + # events, so skipping the dismissal there costs nothing. - env: GH_TOKEN: ${{ github.token }} PR: ${{ github.event.pull_request.number }} if: >- - github.event.action == 'synchronize' && + contains(fromJSON('["synchronize", "reopened"]'), github.event.action) && github.run_attempt == '1' && contains(github.event.pull_request.labels.*.name, inputs.override-label) && !github.event.pull_request.head.repo.fork @@ -112,11 +113,15 @@ jobs: # live (not from the event payload) so the accept flow is: apply # the label, then re-run this job — a re-run replays the original # payload, which predates the label. The label doesn't count on a - # first-attempt synchronize, where the dismissal step just - # dropped it (or would have, for forks). GitHub has no per-label - # permission rules — anyone with triage can apply any label — so - # the label is honored only when its most recent applier holds - # `override-role` or higher; anything unresolvable fails closed. + # first-attempt synchronize or reopened run, where the dismissal + # step just dropped it (or would have, for forks). GitHub has no + # per-label permission rules — anyone with triage can apply any + # label — so the label is honored only when its most recent + # applier holds `override-role` or higher; anything unresolvable + # fails closed. Known residual: cancelling a run before its + # dismissal step and then re-running preserves a stale label, but + # that needs write access and deliberate timing from someone + # already looking at the PR. - env: EVENT_ACTION: ${{ github.event.action }} GH_TOKEN: ${{ github.token }} @@ -128,7 +133,8 @@ jobs: name: Enforce run: | if { [ "$GITHUB_RUN_ATTEMPT" -gt 1 ] || - [ "$EVENT_ACTION" != 'synchronize' ]; } && + { [ "$EVENT_ACTION" != 'synchronize' ] && + [ "$EVENT_ACTION" != 'reopened' ]; }; } && gh pr view "$PR" --json labels --jq '.labels[].name' | grep -Fxq "$LABEL"; then applier=$(gh api "repos/$REPO/issues/$PR/events" --paginate \ From 51d337e75688e53ea6a21fe727f37ca9a24b7cd4 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Tue, 21 Jul 2026 12:04:53 -0500 Subject: [PATCH 08/15] Unblock CI for the sarif-multitool and formatter deps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two packaging consequences of the new SARIF dependencies surfaced on the first PR run: - pnpm blocks the multitool platform packages' build scripts (they chmod the bundled self-contained binary), failing every job at install on Linux. Allow all three; entries are consulted only when a script exists, so the scriptless win32 one is inert. - jschardet (LGPL-2.1-or-later) rides in via the formatter's SARIF_ESLINT_EMBED feature, which the lint pipeline never enables. Every consumer of @gtbuchanan/cli inherits it, so the name-scoped exception belongs in the shared dependency-review config — same pattern as the lightningcss MPL carve-out. Retire it if sarif-sdk makes the embed deps optional. Co-Authored-By: Claude Fable 5 --- .github/dependency-review-config.yml | 7 +++++++ pnpm-workspace.yaml | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/.github/dependency-review-config.yml b/.github/dependency-review-config.yml index 13f997d3..f9eabaaa 100644 --- a/.github/dependency-review-config.yml +++ b/.github/dependency-review-config.yml @@ -1,5 +1,12 @@ --- allow-dependencies-licenses: + # jschardet LGPL-2.1-or-later: transitive dep of + # @microsoft/eslint-formatter-sarif, used only by its + # SARIF_ESLINT_EMBED feature (which the gtb lint pipeline never + # enables). Unmodified and installed separately from the registry — + # the clean LGPL case. Name-scoped so LGPL stays gated elsewhere; + # retire if sarif-sdk makes its embed deps optional. + - pkg:npm/jschardet # lightningcss MPL-2.0 (dev-only build tool): excluded by name rather # than allow-listing MPL globally, so future MPL deps still get gated. # All platform binaries enumerated (no wildcard support). diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index cdf7f001..a4ef24fa 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -1,4 +1,8 @@ allowBuilds: + # Platform packages chmod their bundled self-contained binary. + '@microsoft/sarif-multitool-darwin': true + '@microsoft/sarif-multitool-linux': true + '@microsoft/sarif-multitool-win32': true esbuild: true unrs-resolver: false From bda6308a0f25be28e218e83af385c9c9f582e624 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Tue, 21 Jul 2026 12:37:23 -0500 Subject: [PATCH 09/15] Pass the override label to scripts via env, not inline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit actionlint's shellcheck pass flags `${{ }}` inside single-quoted script strings (SC2016) — only on Linux, since the shared preset disables shellcheck under actionlint on Windows, which is why local pre-commit missed it. Env-var passing is the safer pattern anyway: the label value never touches shell parsing. Co-Authored-By: Claude Fable 5 --- .github/workflows/lint-regression.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/lint-regression.yml b/.github/workflows/lint-regression.yml index a1cc1814..503646af 100644 --- a/.github/workflows/lint-regression.yml +++ b/.github/workflows/lint-regression.yml @@ -34,6 +34,7 @@ jobs: # events, so skipping the dismissal there costs nothing. - env: GH_TOKEN: ${{ github.token }} + LABEL: ${{ inputs.override-label }} PR: ${{ github.event.pull_request.number }} if: >- contains(fromJSON('["synchronize", "reopened"]'), github.event.action) && @@ -41,7 +42,7 @@ jobs: contains(github.event.pull_request.labels.*.name, inputs.override-label) && !github.event.pull_request.head.repo.fork name: Dismiss stale regression acceptance - run: gh pr edit "$PR" --remove-label '${{ inputs.override-label }}' + run: gh pr edit "$PR" --remove-label "$LABEL" # Produce this commit's SARIF logs. Turbo dedupes against the CI # workflow's lint run via the shared remote cache when configured. @@ -86,6 +87,7 @@ jobs: # carries the signal there. - env: GH_TOKEN: ${{ github.token }} + LABEL: ${{ inputs.override-label }} PR: ${{ github.event.pull_request.number }} if: >- steps.compare.outcome == 'failure' && @@ -102,7 +104,7 @@ jobs: echo 'Fix the new findings or suppress them in-source with' \ 'a reason (suppressed findings are exempt). For a bulk' \ 'introduction (e.g. a dependency bump adding a rule), a' \ - 'maintainer can apply the `${{ inputs.override-label }}`' \ + "maintainer can apply the \`$LABEL\`" \ 'label and re-run this job to accept them.' } > comment.md gh pr comment "$PR" --body-file comment.md \ From b62f11ddfc04f52c56b2776ab169de9546b29d5c Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Tue, 21 Jul 2026 13:42:25 -0500 Subject: [PATCH 10/15] Cover the SARIF deps and formatter I/O directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codecov flagged the real I/O layer as untested: the unit suites inject stubs and the e2e suites collect no source coverage by convention. Export the default deps and test each closure against a temp sandbox (list filtering, recursive removal, mkdir-before-write, multitool resolution, single-package workspace fallback), and test the formatter's SARIF write end to end. The formatter now resolves its output under the ESLint-provided context cwd instead of the process cwd — what it should have done anyway, and what makes the write testable without cwd games. The citty command wrappers stay uncovered like every other command file: they only delegate to the tested executors. Co-Authored-By: Claude Fable 5 --- .../cli/src/lib/eslint-sarif-formatter.ts | 13 +- packages/cli/src/lib/sarif-compare.ts | 11 +- .../cli/test/eslint-sarif-formatter.test.ts | 39 +++++- packages/cli/test/sarif-deps.test.ts | 116 ++++++++++++++++++ 4 files changed, 172 insertions(+), 7 deletions(-) create mode 100644 packages/cli/test/sarif-deps.test.ts diff --git a/packages/cli/src/lib/eslint-sarif-formatter.ts b/packages/cli/src/lib/eslint-sarif-formatter.ts index ecbe3d95..0747c719 100644 --- a/packages/cli/src/lib/eslint-sarif-formatter.ts +++ b/packages/cli/src/lib/eslint-sarif-formatter.ts @@ -41,17 +41,24 @@ export const formatConsole = (results: readonly FormatterResult[]): string => { return `${lines.join('\n')}\n\n✖ ${problems}\n`; }; +/** Structural subset of ESLint's formatter context the writer needs. */ +export interface FormatterContext { + readonly cwd?: string; +} + /** * ESLint formatter that writes a SARIF log to {@link sarifOutputPath} * (for the CI lint regression compare) and returns compact console * output. ESLint accepts a single `--format`, so the side-effecting - * file write is how one run feeds both consumers. + * file write is how one run feeds both consumers. The log lands under + * the lint cwd from the formatter context (ESLint always provides it; + * the process cwd is the fallback for direct calls). */ const eslintSarifFormatter = ( results: readonly FormatterResult[], - data?: unknown, + data?: FormatterContext, ): string => { - const outputFile = path.resolve(sarifOutputPath); + const outputFile = path.resolve(data?.cwd ?? process.cwd(), sarifOutputPath); mkdirSync(path.dirname(outputFile), { recursive: true }); writeFileSync(outputFile, sarifFormat(results, data)); return formatConsole(results); diff --git a/packages/cli/src/lib/sarif-compare.ts b/packages/cli/src/lib/sarif-compare.ts index 9cb42967..84e4065b 100644 --- a/packages/cli/src/lib/sarif-compare.ts +++ b/packages/cli/src/lib/sarif-compare.ts @@ -183,7 +183,12 @@ const listSarifFiles = (dir: string): readonly string[] => { .toSorted(localeComparer); }; -const defaultDeps: SarifCompareDeps = { +/** + * Real I/O implementations backing {@link SarifCompareDeps}. Exported + * for direct unit coverage; the commands use them via default params. + * @internal + */ +export const defaultSarifDeps: SarifCompareDeps = { capture, copyFile: (source, destination) => { mkdirSync(path.dirname(destination), { recursive: true }); @@ -339,7 +344,7 @@ const matchFileForward = async ( * to restore. */ export const executeSarifBaseline = async ( - deps: SarifCompareDeps = defaultDeps, + deps: SarifCompareDeps = defaultSarifDeps, ): Promise => { const sha = await deps.capture('git', ['rev-parse', 'HEAD']); const { rootDir } = deps.workspace(); @@ -392,7 +397,7 @@ const resolveBaselineSha = async ( */ export const executeSarifCompare = async ( options: SarifCompareOptions = {}, - deps: SarifCompareDeps = defaultDeps, + deps: SarifCompareDeps = defaultSarifDeps, ): Promise => { const sha = await resolveBaselineSha(options, deps); if (sha !== undefined) { diff --git a/packages/cli/test/eslint-sarif-formatter.test.ts b/packages/cli/test/eslint-sarif-formatter.test.ts index ea1d6e84..63378489 100644 --- a/packages/cli/test/eslint-sarif-formatter.test.ts +++ b/packages/cli/test/eslint-sarif-formatter.test.ts @@ -1,9 +1,14 @@ +import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; import { faker } from '@faker-js/faker'; import { describe, it } from 'vitest'; -import { +import eslintSarifFormatter, { type FormatterResult, formatConsole, + sarifOutputPath, } from '#src/lib/eslint-sarif-formatter.js'; +import { parseSarifLog } from '#src/lib/sarif-compare.js'; describe.concurrent(formatConsole, () => { it('returns empty output for a clean run', ({ expect }) => { @@ -53,3 +58,35 @@ describe.concurrent(formatConsole, () => { expect(formatConsole([result])).toContain('internal'); }); }); + +describe.concurrent('eslintSarifFormatter', () => { + it('writes a SARIF log under the context cwd and returns console output', ({ + expect, + }) => { + const cwd = mkdtempSync(path.join(tmpdir(), 'sarif-formatter-test-')); + try { + const filePath = faker.system.filePath(); + const results: FormatterResult[] = [{ + filePath, + messages: [{ + column: 3, + line: 2, + message: 'Unexpected console statement.', + ruleId: 'no-console', + severity: 1, + }], + }]; + + const output = eslintSarifFormatter(results, { cwd }); + + expect(output).toContain('✖ 1 problem'); + + const written = readFileSync(path.join(cwd, sarifOutputPath), 'utf8'); + const log = parseSarifLog(JSON.parse(written)); + + expect(log.runs[0]?.results).toHaveLength(1); + } finally { + rmSync(cwd, { force: true, recursive: true }); + } + }); +}); diff --git a/packages/cli/test/sarif-deps.test.ts b/packages/cli/test/sarif-deps.test.ts new file mode 100644 index 00000000..6bcca3fe --- /dev/null +++ b/packages/cli/test/sarif-deps.test.ts @@ -0,0 +1,116 @@ +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { describe, it } from 'vitest'; +import { defaultSarifDeps } from '#src/lib/sarif-compare.js'; + +interface SandboxFixture { + readonly dir: string; + readonly [Symbol.dispose]: () => void; +} + +const createSandbox = (): SandboxFixture => { + const dir = mkdtempSync(path.join(tmpdir(), 'sarif-deps-test-')); + return { + dir, + [Symbol.dispose]() { + rmSync(dir, { force: true, recursive: true }); + }, + }; +}; + +describe.concurrent('defaultSarifDeps', () => { + it('writeText creates parent directories and readText round-trips', ({ + expect, + }) => { + using sandbox = createSandbox(); + const filePath = path.join(sandbox.dir, 'dist', 'sarif', 'base.ref'); + + defaultSarifDeps.writeText(filePath, 'abc1234\n'); + + expect(defaultSarifDeps.readText(filePath)).toBe('abc1234\n'); + }); + + it('copyFile creates the destination directory', ({ expect }) => { + using sandbox = createSandbox(); + const source = path.join(sandbox.dir, 'eslint.sarif'); + writeFileSync(source, '{}'); + const destination = path.join(sandbox.dir, 'dist', 'sarif', 'base', 'eslint.sarif'); + + defaultSarifDeps.copyFile(source, destination); + + expect(readFileSync(destination, 'utf8')).toBe('{}'); + }); + + it('list returns only top-level sarif files, sorted', ({ expect }) => { + using sandbox = createSandbox(); + const dir = path.join(sandbox.dir, 'dist', 'sarif'); + defaultSarifDeps.ensureDir(path.join(dir, 'base')); + writeFileSync(path.join(dir, 'oxlint.sarif'), '{}'); + writeFileSync(path.join(dir, 'eslint.sarif'), '{}'); + writeFileSync(path.join(dir, 'base.ref'), 'abc\n'); + writeFileSync(path.join(dir, 'base', 'eslint.sarif'), '{}'); + + expect(defaultSarifDeps.list(dir)).toStrictEqual([ + 'eslint.sarif', + 'oxlint.sarif', + ]); + }); + + it('list returns empty for a missing directory', ({ expect }) => { + using sandbox = createSandbox(); + + expect(defaultSarifDeps.list(path.join(sandbox.dir, 'missing'))).toStrictEqual([]); + }); + + it('remove deletes directories recursively and ignores missing paths', ({ + expect, + }) => { + using sandbox = createSandbox(); + const dir = path.join(sandbox.dir, 'dist', 'sarif', 'base'); + defaultSarifDeps.ensureDir(dir); + writeFileSync(path.join(dir, 'eslint.sarif'), '{}'); + + defaultSarifDeps.remove(dir); + defaultSarifDeps.remove(dir); + + expect(existsSync(dir)).toBe(false); + }); + + it('ensureDir is recursive and idempotent', ({ expect }) => { + using sandbox = createSandbox(); + const dir = path.join(sandbox.dir, 'a', 'b', 'c'); + + defaultSarifDeps.ensureDir(dir); + defaultSarifDeps.ensureDir(dir); + + expect(defaultSarifDeps.exists(dir)).toBe(true); + }); + + it('makeTempDir creates a fresh directory', ({ expect }) => { + const dir = defaultSarifDeps.makeTempDir(); + try { + expect(existsSync(dir)).toBe(true); + expect(path.basename(dir)).toMatch(/^gtb-sarif-base-/v); + } finally { + rmSync(dir, { force: true, recursive: true }); + } + }); + + it('resolveMultitool resolves an existing binary path', ({ expect }) => { + const binary = defaultSarifDeps.resolveMultitool(); + + expect(existsSync(binary)).toBe(true); + }); + + it('workspace resolves single-package mode from an isolated cwd', ({ + expect, + }) => { + using sandbox = createSandbox(); + + expect(defaultSarifDeps.workspace(sandbox.dir)).toMatchObject({ + packageDirs: [sandbox.dir], + rootDir: sandbox.dir, + }); + }); +}); From 96604377369cf54c0dbd52876c6c0e0217da830a Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Thu, 30 Jul 2026 20:52:59 -0500 Subject: [PATCH 11/15] Trim the SARIF ratchet changeset to a release note The changeset recapped the full architecture; the deep detail lives in AGENTS.md and the README. Keep only what a consumer reading the CHANGELOG needs. Co-Authored-By: Claude Fable 5 --- .changeset/sarif-lint-regression-compare.md | 25 ++++++--------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/.changeset/sarif-lint-regression-compare.md b/.changeset/sarif-lint-regression-compare.md index 0fe997a3..1006af14 100644 --- a/.changeset/sarif-lint-regression-compare.md +++ b/.changeset/sarif-lint-regression-compare.md @@ -2,21 +2,10 @@ '@gtbuchanan/cli': minor --- -Rework lint enforcement around tool-agnostic SARIF baselining. -`lint:eslint` is now a reporter: it writes `dist/sarif/eslint.sarif` -via a bundled formatter, prints compact console output, and no longer -fails on warnings (fatal errors still fail). Enforcement moves to the -new `gtb sarif compare` command, which pairs every `dist/sarif/*.sarif` -with `dist/sarif/base/.sarif` and fails only on findings the -`sarif-multitool` baseliner classifies as new — any tool that drops a -SARIF log into `dist/sarif/` is gated with no extra wiring. A missing -baseline counts as empty (all findings new), and findings carrying -in-source suppressions are exempt. With -`--base ` (merge-base of a ref, for local runs) or `--base-sha -` (exact commit, for CI's shallow merge-ref checkouts) it produces -the baseline itself by linting that commit in a temporary git worktree, -skipping production when the `dist/sarif/base.ref` stamp already -records it (locally or via CI caches). `gtb sarif baseline` snapshots HEAD's logs as the -baseline so default-branch CI can seed a cross-PR cache (see the new -`lint-regression.yml` reusable workflow). Local changed-file -enforcement via the pre-commit ESLint step is unaffected. +Rework lint enforcement as a SARIF ratchet. Lint tasks are now +reporters: `lint:eslint` writes `dist/sarif/eslint.sarif` and no +longer fails on warnings (fatal errors still fail). The new +`gtb sarif compare` command fails only on findings that are new +relative to the merge base, and `gtb sarif baseline` snapshots HEAD's +logs so CI can seed a cross-PR baseline cache — see the new +`lint-regression.yml` and `lint-baseline.yml` reusable workflows. From f0d6c81387fd4f3602e0131a3a92c957a5fc41aa Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Thu, 30 Jul 2026 20:53:49 -0500 Subject: [PATCH 12/15] Split the lint baseline seed into its own reusable workflow With both modes in lint-regression.yml behind a seed input, every PR run showed a permanently skipped Lint Baseline Seed job and every release run a skipped Lint Regression job. A dedicated lint-baseline reusable removes the skipped peers; release.yml calls it directly. Co-Authored-By: Claude Fable 5 --- .github/workflows/lint-baseline.yml | 61 +++++++++++++++++++++++++++++ .github/workflows/release.yml | 3 +- 2 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/lint-baseline.yml diff --git a/.github/workflows/lint-baseline.yml b/.github/workflows/lint-baseline.yml new file mode 100644 index 00000000..50e14cbf --- /dev/null +++ b/.github/workflows/lint-baseline.yml @@ -0,0 +1,61 @@ +--- +env: + # GitHub Actions has no TTY, so force color; Actions renders the ANSI. + FORCE_COLOR: '1' + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true + +jobs: + seed: + name: Lint Baseline Seed + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - uses: gtbuchanan/tooling/.github/actions/mise-setup@main + + - uses: gtbuchanan/tooling/.github/actions/pnpm-tasks@main + + # Cache-hits against the CI workflow's lint run of this commit + # when the turbo remote cache is configured. + - name: Lint HEAD + run: >- + ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} + turbo run lint + + - name: Snapshot baselines + run: >- + ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} + sarif baseline + + - uses: actions/cache/save@v6 + with: + key: lint-baseline-${{ github.sha }} + path: | + **/dist/sarif/base/** + !**/node_modules/** + dist/sarif/base.ref + +name: Lint Baseline + +# Reusable: seed the cross-PR lint baseline cache from a default-branch +# commit. Any future PR's merge base is that commit itself, so its +# baseline is just the commit's own lint output — snapshot it via +# `gtb sarif baseline` and save it keyed on the SHA. Caches created on +# the default branch are readable from every PR, so the +# `lint-regression.yml` compare restores it and skips merge-base +# production entirely. The gate stays self-sufficient without it — a +# cache miss just means the compare lints the merge base itself. +on: + workflow_call: + inputs: + gtb-from-source: + default: false + description: >- + Run gtb from the workspace source (`pnpm run gtb`) instead of + the installed bin (`pnpm exec gtb`). Set true only by the repo + that vendors `@gtbuchanan/cli` as a workspace package (tooling + itself). Consumers leave it false. + type: boolean + +permissions: + contents: read diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2aa7eeb9..b1b547ea 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -24,10 +24,9 @@ jobs: lint-baseline: name: Lint needs: ci - uses: ./.github/workflows/lint-regression.yml + uses: ./.github/workflows/lint-baseline.yml with: gtb-from-source: true - seed: true name: Release From a187198610f3bb0e79e5cfae0803baee8f28d6fc Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Thu, 30 Jul 2026 20:54:29 -0500 Subject: [PATCH 13/15] Report lint regressions in the job summary; drop seed mode The PR-comment report could go stale after a green re-run (nothing edited it) and gh's --edit-last targets the bot's last comment, which another workflow (dependency review) may own. A job summary is pinned to its run, so neither failure mode exists, and it needs no write token, so fork PRs get the full report too. Also fail the merge-ref guard with an explicit ::error:: instead of git's bare "Needed a single revision", and remove the seed job now that lint-baseline.yml owns seeding. Co-Authored-By: Claude Fable 5 --- .github/workflows/lint-regression.yml | 111 +++++++++----------------- 1 file changed, 36 insertions(+), 75 deletions(-) diff --git a/.github/workflows/lint-regression.yml b/.github/workflows/lint-regression.yml index 503646af..4d0abbe5 100644 --- a/.github/workflows/lint-regression.yml +++ b/.github/workflows/lint-regression.yml @@ -6,7 +6,6 @@ env: jobs: run: - if: ${{ !inputs.seed }} name: Lint Regression runs-on: ubuntu-latest steps: @@ -51,13 +50,18 @@ jobs: ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} turbo run lint - # Fail fast if the checkout is not the merge ref (a two-parent - # commit): re-checking-out the PR head would make HEAD^1 the - # head's own parent, silently gating against the wrong commit. + # The checkout must be the merge ref (a two-parent commit): + # re-checking-out the PR head would make HEAD^1 the head's own + # parent, silently gating against the wrong commit. The guard + # fails the step with an explicit error rather than proceeding. - id: merge-base name: Resolve the merge base (merge ref first parent) run: | - git rev-parse --verify HEAD^2 > /dev/null + git rev-parse --verify HEAD^2 > /dev/null 2>&1 || { + echo '::error::Checkout is not the PR merge ref; gating' \ + 'against a PR-head parent would use the wrong baseline' + exit 1 + } echo "sha=$(git rev-parse HEAD^1)" >> "$GITHUB_OUTPUT" # Restore baselines produced by an earlier run against the same @@ -83,16 +87,14 @@ jobs: | tee lint-regression.txt exit "${PIPESTATUS[0]}" - # Fork PRs run with a read-only token; the check failure alone - # carries the signal there. + # The findings land in the run's job summary rather than a PR + # comment: a summary is pinned to its run, so it can't go stale + # after a green re-run or collide with other bot comments, and it + # needs no write token, so fork PRs get the full report too. - env: - GH_TOKEN: ${{ github.token }} LABEL: ${{ inputs.override-label }} - PR: ${{ github.event.pull_request.number }} - if: >- - steps.compare.outcome == 'failure' && - !github.event.pull_request.head.repo.fork - name: Report new violations on the PR + if: steps.compare.outcome == 'failure' + name: Report new violations in the job summary run: | { echo '### New lint violations relative to the merge base' @@ -106,24 +108,22 @@ jobs: 'introduction (e.g. a dependency bump adding a rule), a' \ "maintainer can apply the \`$LABEL\`" \ 'label and re-run this job to accept them.' - } > comment.md - gh pr comment "$PR" --body-file comment.md \ - --edit-last --create-if-none + } >> "$GITHUB_STEP_SUMMARY" # Run-but-neutral override: the label lets the check pass, but the - # comment above still records what was accepted. Labels are read - # live (not from the event payload) so the accept flow is: apply - # the label, then re-run this job — a re-run replays the original - # payload, which predates the label. The label doesn't count on a - # first-attempt synchronize or reopened run, where the dismissal - # step just dropped it (or would have, for forks). GitHub has no - # per-label permission rules — anyone with triage can apply any - # label — so the label is honored only when its most recent - # applier holds `override-role` or higher; anything unresolvable - # fails closed. Known residual: cancelling a run before its - # dismissal step and then re-running preserves a stale label, but - # that needs write access and deliberate timing from someone - # already looking at the PR. + # job summary above still records what was accepted. Labels are + # read live (not from the event payload) so the accept flow is: + # apply the label, then re-run this job — a re-run replays the + # original payload, which predates the label. The label doesn't + # count on a first-attempt synchronize or reopened run, where the + # dismissal step just dropped it (or would have, for forks). + # GitHub has no per-label permission rules — anyone with triage + # can apply any label — so the label is honored only when its most + # recent applier holds `override-role` or higher; anything + # unresolvable fails closed. Known residual: cancelling a run + # before its dismissal step and then re-running preserves a stale + # label, but that needs write access and deliberate timing from + # someone already looking at the PR. - env: EVENT_ACTION: ${{ github.event.action }} GH_TOKEN: ${{ github.token }} @@ -166,42 +166,6 @@ jobs: fi exit 1 - # On the default branch, any future PR's merge base is HEAD itself, - # so its baseline is just this commit's own lint output. Caches - # created on the default branch are readable from every PR, so this - # seeds a cross-PR baseline cache — PR compare runs restore it and - # skip merge-base production entirely. - seed: - if: ${{ inputs.seed }} - name: Lint Baseline Seed - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v7 - - - uses: gtbuchanan/tooling/.github/actions/mise-setup@main - - - uses: gtbuchanan/tooling/.github/actions/pnpm-tasks@main - - # Cache-hits against the CI workflow's lint run of this commit - # when the turbo remote cache is configured. - - name: Lint HEAD - run: >- - ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} - turbo run lint - - - name: Snapshot baselines - run: >- - ${{ inputs.gtb-from-source && 'pnpm run gtb' || 'pnpm exec gtb' }} - sarif baseline - - - uses: actions/cache/save@v6 - with: - key: lint-baseline-${{ github.sha }} - path: | - **/dist/sarif/base/** - !**/node_modules/** - dist/sarif/base.ref - name: Lint Regression # Reusable: fail a PR only on lint violations that are new relative to @@ -213,9 +177,11 @@ name: Lint Regression # findings are gate-exempt); for bulk introductions (e.g. a dependency # bump adding a rule) the override label turns a failing compare into a # pass for one merge — apply it, then re-run the failed job — while the -# PR comment keeps the accepted violations on record. The label counts -# only when applied by `override-role` or higher, since GitHub has no -# per-label permission rules of its own. +# failing run's job summary keeps the accepted violations on record. +# The label counts only when applied by `override-role` or higher, +# since GitHub has no per-label permission rules of its own. The +# cross-PR baseline cache is seeded by the `lint-baseline.yml` reusable +# on default-branch pushes. on: workflow_call: inputs: @@ -242,14 +208,9 @@ on: Minimum repository role (`write`, `maintain`, or `admin`) the override label's applier must hold for it to count. type: string - seed: - default: false - description: >- - Snapshot HEAD's lint output as the cross-PR baseline cache - instead of gating. Set on default-branch pushes; the PR gate - restores the cache keyed on its merge-base SHA. - type: boolean +# `pull-requests: write` covers the label dismissal; everything else +# reads. permissions: contents: read pull-requests: write From d6f045c815e9ab192db251b07fd89c3dedefc747 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Thu, 30 Jul 2026 20:55:17 -0500 Subject: [PATCH 14/15] Document SARIF rationale; hoist the internal ruleId fallback The duplicated 'internal' literal covers ESLint's null ruleId (fatal parse/config errors); hoist it into a shared, documented module. Also record why formatConsole is hand-rolled (ESLint v9 exposes no importable formatter, and the line shape matches the compare report) and that the baseline worktree bootstrap assumes a node-only lint graph (mise never activates outside the repo). Co-Authored-By: Claude Fable 5 --- packages/cli/src/lib/eslint-sarif-formatter.ts | 11 +++++++++-- packages/cli/src/lib/internal-rule-id.ts | 12 ++++++++++++ packages/cli/src/lib/sarif-compare.ts | 12 +++++++++++- 3 files changed, 32 insertions(+), 3 deletions(-) create mode 100644 packages/cli/src/lib/internal-rule-id.ts diff --git a/packages/cli/src/lib/eslint-sarif-formatter.ts b/packages/cli/src/lib/eslint-sarif-formatter.ts index 0747c719..9fcf7a65 100644 --- a/packages/cli/src/lib/eslint-sarif-formatter.ts +++ b/packages/cli/src/lib/eslint-sarif-formatter.ts @@ -1,6 +1,7 @@ import { mkdirSync, writeFileSync } from 'node:fs'; import path from 'node:path'; import sarifFormat from '@microsoft/eslint-formatter-sarif'; +import { internalRuleId } from './internal-rule-id.ts'; import { sarifPaths } from './sarif-paths.ts'; /** SARIF log path written by the formatter, relative to the lint cwd. */ @@ -24,12 +25,18 @@ export interface FormatterResult { /** ESLint's numeric severity for an error (1 is a warning). */ const errorSeverity = 2; -/** Builds compact per-violation console output (one line per message). */ +/** + * Builds compact per-violation console output (one line per message). + * Hand-rolled rather than delegated: ESLint v9 exposes no importable + * formatter (`stylish` is CLI-internal; `compact`/`unix` were removed + * from core), and the line shape deliberately matches the compare's + * new-findings report so lint output and the CI gate read identically. + */ export const formatConsole = (results: readonly FormatterResult[]): string => { const lines = results.flatMap(result => result.messages.map((message) => { const severity = message.severity === errorSeverity ? 'error' : 'warning'; - const rule = message.ruleId ?? 'internal'; + const rule = message.ruleId ?? internalRuleId; const position = `${String(message.line)}:${String(message.column)}`; return `${result.filePath}:${position} ${severity} ${message.message} ${rule}`; }), diff --git a/packages/cli/src/lib/internal-rule-id.ts b/packages/cli/src/lib/internal-rule-id.ts new file mode 100644 index 00000000..8036a589 --- /dev/null +++ b/packages/cli/src/lib/internal-rule-id.ts @@ -0,0 +1,12 @@ +/** + * Rule label substituted when a lint message carries no rule at all. + * ESLint reports `ruleId: null` for messages that don't originate from + * a rule — fatal parse or config errors — and SARIF results inherit + * that absence. Labeling them `internal` marks the finding as + * tool-internal breakage rather than leaving a hole in the output + * line. + * + * Kept dependency-free: the ESLint formatter imports this from inside + * the ESLint process. + */ +export const internalRuleId = 'internal'; diff --git a/packages/cli/src/lib/sarif-compare.ts b/packages/cli/src/lib/sarif-compare.ts index 84e4065b..7883fccb 100644 --- a/packages/cli/src/lib/sarif-compare.ts +++ b/packages/cli/src/lib/sarif-compare.ts @@ -7,6 +7,7 @@ import { tmpdir } from 'node:os'; import path from 'node:path'; import * as v from 'valibot'; import { readJsonFile } from './file-writer.ts'; +import { internalRuleId } from './internal-rule-id.ts'; import { type Logger, createLogger } from './logger.ts'; import { type RunOptions, capture, run } from './process.ts'; import { sarifPaths } from './sarif-paths.ts'; @@ -93,7 +94,7 @@ const toNewFinding = (result: SarifResult): NewFinding => { level: result.level ?? 'error', line, message: result.message.text, - ruleId: result.ruleId ?? 'internal', + ruleId: result.ruleId ?? internalRuleId, uri: uri ?? '', }; }; @@ -258,6 +259,15 @@ export const produceBaseline = async ( const baseDir = deps.makeTempDir(); try { await deps.run('git', { args: ['worktree', 'add', '--detach', baseDir, sha] }); + /* + * The bootstrap assumes the lint graph needs only the node + * toolchain: pnpm install plus whatever the ambient PATH provides. + * The temp worktree lives outside the repo, so per-directory tool + * managers (mise) never activate there — a base commit pinning + * different tool versions resolves the head environment's binaries + * instead. Fine while every SARIF reporter is node-based; revisit + * with a bootstrap seam when a non-node reporter joins the graph. + */ await deps.run('pnpm', { args: ['install', '--frozen-lockfile', '--prefer-offline'], cwd: baseDir, From 94bc3ced66e3ccfaf9f5fb6cc35baf8c063ea1e4 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Thu, 30 Jul 2026 20:56:10 -0500 Subject: [PATCH 15/15] Move consumer lint gate docs into the README The ratchet documentation lived only in AGENTS.md while the README -- the consumer-facing surface -- never mentioned the new workflows. README gains the Lint jobs in both pipeline examples, the reusables table rows, and a lint regression section (ratchet semantics, override label flow, inputs, permissions). AGENTS.md keeps a condensed section scoped to the invariants agents need and points to the README for usage; its workflow bullets now reflect the job-summary report and the lint-baseline split. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 109 +++++++++++++++++++++++++----------------------------- README.md | 53 +++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 59 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index eb4fb0cd..5a0bfaa8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,10 +25,11 @@ mise.toml — Pin dev-tool versions for local + CI; postinstall hoo changeset-check.yml — Reusable: verify a changeset exists ci.yml — Reusable: build + slow + e2e + coverage dependency-review.yml — Reusable: scan dep changes (vulns + licenses) + lint-baseline.yml — Reusable: seed the cross-PR lint baseline cache (default branch) lint-regression.yml — Reusable: fail PRs only on lint violations new vs. the merge base (SARIF diff) pr.yml — Pipeline (PR): ci + changeset + deps + lint regression + pre-commit pre-commit.yml — Reusable: run hk hooks - release.yml — Pipeline (push): CI gate + CD + release.yml — Pipeline (push): CI gate + CD + lint baseline seed packages/ cli/ — @gtbuchanan/cli (gtb build CLI for consumers) skills/ — Authored Agent Skills deployed by `gtb task deploy:skills` @@ -91,53 +92,36 @@ coverage, setupFiles, and mock reset. ### SARIF lint baselining Lint enforcement is a ratchet: PRs may not introduce _new_ findings, -while pre-existing (accepted) ones never block. The machinery is -tool-agnostic — any reporter that drops a `.sarif` into -`dist/sarif/` is gated with no extra wiring; ESLint is the first such -reporter. Three layers: +while pre-existing (accepted) ones never block. Consumer-facing usage +(override label flow, workflow wiring) lives in `README.md`; the +invariants agents need: - **Reporters, not gates.** `lint:eslint` writes `dist/sarif/eslint.sarif` (a turbo task output) via a formatter - bundled in `@gtbuchanan/cli` and prints compact console output. - Warnings never fail it — the eslint-config downgrades every rule to a - warning — but fatal errors (parse/config breakage) still do. -- **`gtb sarif compare` is the gate.** It pairs every + bundled in `@gtbuchanan/cli`. Warnings never fail it — fatal errors + (parse/config breakage) still do. Any reporter that drops a + `.sarif` into `dist/sarif/` is gated with no extra wiring. +- **`gtb sarif compare` is the gate.** Pairs every `dist/sarif/*.sarif` in each lint cwd with - `dist/sarif/base/.sarif` and diffs them with + `dist/sarif/base/.sarif` via `sarif-multitool match-results-forward`, failing only on results - classified `new`. Matching is fingerprint/content-based, so baseline - findings that merely moved stay matched — no git-diff offset math. - `--base ` makes it self-sufficient: it lints the merge base of - that ref and HEAD in a throwaway git worktree to produce the - baselines (`gtb sarif compare --base origin/main` locally, where - full history makes `git merge-base` cheap). CI - (`lint-regression.yml`) passes `--base-sha` with the PR merge ref's - first parent instead — on the merged checkout the target branch head - _is_ the merge base — so a `fetch-depth: 2` checkout suffices, and a - missing commit is fetched by SHA at depth 1. A stamp (`dist/sarif/base.ref`, recording - the merge-base SHA) skips production when the on-disk baselines are - already current; CI seeds a cross-PR baseline cache from each main - commit via `gtb sarif baseline` (where merge base == HEAD, the - baseline is just that commit's own reporter output). A missing - baseline is an empty baseline, not a pass: every finding in a log - with no baseline counterpart is new and needs acceptance, so a newly - added reporter (or the bootstrap PR) can't slip findings in silently. - Suppressed findings (reasoned in-source suppressions) are exempt from - the gate — the suppression is already the accepted mechanism — but - stay in the logs for visibility. + classified `new` (fingerprint/content matching, so moved findings + stay matched). + `--base ` lints the merge base in a throwaway git worktree to + produce baselines (`gtb sarif compare --base origin/main` locally); + CI passes `--base-sha` with the PR merge ref's first parent, which + _is_ the merge base on the merged checkout. The + `dist/sarif/base.ref` stamp skips production when the on-disk + baselines are current; `gtb sarif baseline` snapshots HEAD's own + logs for the default-branch cache seed (`lint-baseline.yml`). A + missing baseline is an empty baseline, not a pass — a new reporter + can't slip findings in silently. In-source-suppressed findings are + gate-exempt but stay in the logs. - **Changed-file enforcement stays local.** The hk pre-commit `eslint` - step keeps `--max-warnings=0` on staged files, and the IDE shows - warnings inline — new violations are still caught at authoring time; - the ratchet only governs what lands. - -Accepting new findings is an in-source act by default: fix them, or -suppress them with a reason (suppressed findings are gate-exempt but -stay in the logs). The override label (see `lint-regression.yml`) is -the escape hatch for bulk introductions — e.g. a dependency bump -shipping a new rule — where per-instance suppression would be noise -and disabling the rule would let new violations creep in while it's -off. Findings accepted either way enter the ephemeral baseline once -merged and surface in every SARIF log until paid down. + step keeps `--max-warnings=0` on staged files; the ratchet only + governs what lands. Findings accepted via the override label or a + suppression enter the baseline once merged and surface in every + SARIF log until paid down. ### Pre-commit hooks @@ -255,12 +239,12 @@ as peer jobs, each calling a single-concern **reusable** workflow: - **`pr.yml`** (on `pull_request`) — jobs `CI`, `Changeset`, `Dependencies`, `Lint`, `Pre-Commit`. -- **`release.yml`** (on `push` to main) — jobs `CI` (gate), `CD` - (`needs: ci`). `CI` and `CD` are peers. +- **`release.yml`** (on `push` to main) — jobs `CI` (gate), `CD` and + `Lint` (baseline seed), both `needs: ci`. The reusables (`workflow_call`-only): `ci.yml`, `cd.yml`, -`changeset-check.yml`, `dependency-review.yml`, `lint-regression.yml`, -`pre-commit.yml`. +`changeset-check.yml`, `dependency-review.yml`, `lint-baseline.yml`, +`lint-regression.yml`, `pre-commit.yml`. Consumers copy `pr.yml` / `release.yml`, swapping `./` for `gtbuchanan/tooling/.github/workflows/@main`: @@ -272,7 +256,8 @@ on: branches: [main] permissions: contents: read - pull-requests: write # Dependencies posts a PR comment + # Dependencies posts a PR comment; Lint dismisses override labels + pull-requests: write jobs: ci: name: CI @@ -313,6 +298,10 @@ jobs: id-token: write # npm trusted publishing (OIDC) uses: gtbuchanan/tooling/.github/workflows/cd.yml@main secrets: inherit + lint-baseline: + name: Lint + needs: ci + uses: gtbuchanan/tooling/.github/workflows/lint-baseline.yml@main ``` **Naming / required checks.** Branch protection keys on the **leaf job @@ -412,20 +401,24 @@ through `package.json` scripts backed by `gtb` leaf commands. to ecosystems it indexes (npm + Actions here), so mise tools and `hk.pkl` steps aren't covered — Renovate's managers handle those independently. +- **`lint-baseline.yml`** — Seeds the cross-PR lint baseline cache + from a default-branch commit (its own lint output, snapshotted via + `gtb sarif baseline` and cached on the SHA). An optimization only: + on a cache miss the gate lints the merge base itself. - **`lint-regression.yml`** — Fails a PR only on lint violations that are new relative to its merge base (the ratchet gate; see the SARIF lint baselining section). Lints HEAD, then runs - `gtb sarif compare --base origin/`, which lints the - merge base in a throwaway git worktree and diffs the SARIF logs via - `sarif-multitool`. New findings are routinely fixed or suppressed - in-source; for bulk introductions the override label (default - `accepted-lint-regression`, dismissed on every new push, honored - only when applied by `override-role`+ — default `maintain`) turns a - failing compare into a pass for one merge — apply it, then re-run - the failed job (labels are read live, so the replayed event payload - doesn't matter) — while a PR comment records the accepted - violations. Caller must grant `pull-requests: write`; fork PRs get - the check failure without the comment (read-only token). + `gtb sarif compare --base-sha` with the merge ref's first parent, + which restores or produces the baseline and diffs the SARIF logs via + `sarif-multitool`. New findings land in the run's job summary + (fork-PR safe — no write token needed) and are routinely fixed or + suppressed in-source; for bulk introductions the override label + (default `accepted-lint-regression`, dismissed on every new push, + honored only when applied by `override-role`+ — default `maintain`) + turns a failing compare into a pass for one merge — apply it, then + re-run the failed job (labels are read live, so the replayed event + payload doesn't matter). Caller must grant `pull-requests: write` + for the label dismissal. - **`pre-commit.yml`** — Runs the `hk:base` mise task on PR changed files (hk resolved from mise). The `use-pnpm` input (default `false`) opts into `pnpm install` for steps that shell out to the diff --git a/README.md b/README.md index fe62c1a5..0c7bb19a 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,8 @@ on: branches: [main] permissions: contents: read - pull-requests: write # Dependencies posts a PR comment + # Dependencies posts a PR comment; Lint dismisses override labels + pull-requests: write jobs: ci: name: CI @@ -43,6 +44,9 @@ jobs: dependencies: name: Dependencies uses: gtbuchanan/tooling/.github/workflows/dependency-review.yml@main + lint-regression: + name: Lint + uses: gtbuchanan/tooling/.github/workflows/lint-regression.yml@main pre-commit: name: Pre-Commit uses: gtbuchanan/tooling/.github/workflows/pre-commit.yml@main @@ -69,6 +73,10 @@ jobs: id-token: write # npm trusted publishing (OIDC) uses: gtbuchanan/tooling/.github/workflows/cd.yml@main secrets: inherit + lint-baseline: + name: Lint + needs: ci + uses: gtbuchanan/tooling/.github/workflows/lint-baseline.yml@main ``` The reusable workflows each job calls: @@ -79,6 +87,8 @@ The reusable workflows each job calls: | `cd.yml` | changesets version + publish (OIDC) | | `changeset-check.yml` | Verify changeset exists | | `dependency-review.yml` | Scan PR dep changes for vulns + licenses | +| `lint-baseline.yml` | Seed the cross-PR lint baseline cache | +| `lint-regression.yml` | Fail PRs only on new lint violations | | `pre-commit.yml` | Run hk hooks on changed files | Permissions narrow down the call chain but never elevate, so each @@ -171,6 +181,47 @@ jobs: fail-on-severity: low ``` +### Lint regression gate + +`lint-regression.yml` fails a PR only on lint violations that are +_new_ relative to its merge base — pre-existing findings never block. +Lint tasks are reporters that write SARIF logs (`dist/sarif/*.sarif`); +`gtb sarif compare` diffs them against the merge base's logs (linting +the merge base in a throwaway git worktree on a cache miss) and fails +only on findings classified as new. The findings are reported in the +run's job summary. + +The routine response is to fix the new findings or suppress them +in-source with a reason (suppressed findings are exempt from the +gate). For bulk introductions — e.g. a dependency bump shipping a new +rule, where per-instance suppression would be noise — a maintainer +applies the override label (`accepted-lint-regression` by default) and +re-runs the failed job to accept the findings for that merge. The +label is dismissed automatically on every new push, and it only counts +when its applier holds the `override-role` repository role or higher +(default `maintain`) — GitHub has no per-label permission rules of its +own. + +`lint-baseline.yml` seeds a cross-PR baseline cache from each +default-branch commit (the `Lint` job in the release pipeline above). +It's an optimization, not a requirement — on a cache miss the gate +lints the merge base itself. + +Customize via `with:` on the `Lint` job: + +```yaml +jobs: + lint-regression: + name: Lint + uses: gtbuchanan/tooling/.github/workflows/lint-regression.yml@main + with: + override-label: accepted-lint-regression + override-role: maintain +``` + +The PR pipeline must grant `pull-requests: write` (shown in the setup +above) so the gate can dismiss stale override labels. + ### Build pipeline conventions Turborepo manages task orchestration. The task graph is defined in