diff --git a/.gitignore b/.gitignore index 225f42bf516..56d8de2846d 100644 --- a/.gitignore +++ b/.gitignore @@ -132,6 +132,12 @@ validation-screenshots/ /notes/ /pr-evidence/ +# Per-task agent scratch trees, named for the Orca task id. They hold evidence and sometimes whole +# nested repo checkouts; a foreign checkout is not this repo's changed code, and 39,079 files from +# one overflowed the argument list of `check:code-quality:changed`. Unanchored because a lane can +# create one from any working directory. +.orca-task-*/ + # Playwright test-results/ playwright-report/ diff --git a/config/scripts/check-changed-code-quality.mjs b/config/scripts/check-changed-code-quality.mjs index 615a2402bcf..adf9c9f365a 100644 --- a/config/scripts/check-changed-code-quality.mjs +++ b/config/scripts/check-changed-code-quality.mjs @@ -1,5 +1,6 @@ import { execFileSync, spawnSync } from 'node:child_process' import { existsSync, readFileSync } from 'node:fs' +import { createRequire } from 'node:module' import path from 'node:path' import process from 'node:process' import { pathToFileURL } from 'node:url' @@ -114,10 +115,19 @@ export function collectAddedLineRanges(root, requestedBase) { function parseOxlintOutput(stdout, label) { const start = stdout.indexOf('{') const end = stdout.lastIndexOf('}') - if (start === -1 || end === -1) { - throw new Error(`${label} did not return Oxlint JSON output.`) + if (start !== -1 && end !== -1) { + try { + return JSON.parse(stdout.slice(start, end + 1)) + } catch { + // Fall through so the caller sees what Oxlint actually printed. + } } - return JSON.parse(stdout.slice(start, end + 1)) + // Why echo it: Oxlint writes configuration failures to stdout, and a wrapper's own + // warning can carry braces that this slice mistakes for the report, so discarding the + // output leaves the gate dying with no reason anywhere in the log. + throw new Error( + `${label} did not return Oxlint JSON output. Oxlint printed:\n${stdout.trim().slice(0, 2000)}` + ) } function normalizedDiagnosticPath(root, filename) { @@ -263,21 +273,145 @@ function printDiagnostic(diagnostic, root) { console.error(`${file}:${line} ${code}: ${diagnostic.message}`) } -function runOxlintScan(root, scan, files) { - const pnpm = process.platform === 'win32' ? 'pnpm.cmd' : 'pnpm' - const result = spawnSync(pnpm, ['exec', 'oxlint', ...scan.args, '--format', 'json', ...files], { +// Why: Oxlint takes paths as positional arguments only — no stdin, no @file — so one +// oversized changed set makes the spawn itself fail with E2BIG and the gate never runs. +// The file list is only part of what the spawn carries, so budget against the whole of it. +// +// POSIX execve() charges argv strings, the inherited environment, AND one pointer per +// entry against a single ceiling: macOS caps that at kern.argmax (1 MiB), Linux at +// max(min(6 MiB, RLIMIT_STACK/4), 128 KiB) — 2 MiB at the default 8 MiB stack. +// +// Budgeting just under the kernel's own ceiling is not enough. macOS copies argv and the +// environment into the child's stack, and a Node child dies from the squeeze well before +// the kernel refuses the exec: measured here, a total near 970 KiB SIGSEGVs on Node 24 and +// throws a stack-overflow RangeError on Node 26, while E2BIG only starts around 1,048 KiB. +// Half of kern.argmax leaves that cliff ~450 KiB away. +// +// Windows is a different shape entirely: CreateProcess caps only the command line, at +// 32,767 UTF-16 units, and passes the environment in a separate block that does not count. +// Counting UTF-8 bytes against a UTF-16 ceiling over-estimates, which is the safe direction. +const POSIX_SPAWN_CEILING_BYTES = 512 * 1024 +const WINDOWS_COMMAND_LINE_CEILING_BYTES = 32767 +// Every entry costs a pointer beside its bytes, and libuv may wrap a Windows argument in +// quotes. Measured on macOS: 6,096 paths cost ~48 KiB in pointers alone. +const PER_ENTRY_OVERHEAD_BYTES = 12 +// Slack for kernel padding and the exec path the kernel copies alongside argv. +const SPAWN_HEADROOM_BYTES = 8 * 1024 +// A full ceiling's worth of paths would be a single enormous batch; hold the file list to +// what the gate already used so an ordinary changed set still spawns once per scan. +const MAX_BATCH_ARGUMENT_BYTES = 256 * 1024 +// A pathological environment must not drive the budget to zero, which would spawn Oxlint +// once per path. +const MIN_BATCH_ARGUMENT_BYTES = 8 * 1024 + +function spawnEntryBytes(entries) { + let total = 0 + for (const entry of entries) { + total += Buffer.byteLength(entry, 'utf8') + PER_ENTRY_OVERHEAD_BYTES + } + return total +} + +export function environmentEntries(env = process.env) { + const entries = [] + for (const [key, value] of Object.entries(env)) { + if (value !== undefined) { + entries.push(`${key}=${value}`) + } + } + return entries +} + +export function maxBatchArgumentBytes({ + platform = process.platform, + fixedArguments = [], + env = process.env +} = {}) { + const windows = platform === 'win32' + const ceiling = windows ? WINDOWS_COMMAND_LINE_CEILING_BYTES : POSIX_SPAWN_CEILING_BYTES + const carried = + spawnEntryBytes(fixedArguments) + (windows ? 0 : spawnEntryBytes(environmentEntries(env))) + return Math.min( + MAX_BATCH_ARGUMENT_BYTES, + Math.max(MIN_BATCH_ARGUMENT_BYTES, ceiling - SPAWN_HEADROOM_BYTES - carried) + ) +} + +export function batchFilesByArgumentBytes(files, limit = maxBatchArgumentBytes()) { + const batches = [] + let batch = [] + let bytes = 0 + for (const file of files) { + // A single path over the limit still gets its own batch: an empty argument list + // would make Oxlint lint the whole working directory instead. + const cost = Buffer.byteLength(file, 'utf8') + PER_ENTRY_OVERHEAD_BYTES + if (batch.length > 0 && bytes + cost > limit) { + batches.push(batch) + batch = [] + bytes = 0 + } + batch.push(file) + bytes += cost + } + if (batch.length > 0) { + batches.push(batch) + } + return batches +} + +// Why resolve the manifest rather than joining node_modules: pnpm's store is not a flat +// tree, and `bin` is the launcher the package itself declares. Not `pnpm exec`, which on +// Windows routes through pnpm.cmd and cmd.exe, whose command line caps at 8,191 characters +// instead of CreateProcess' 32,767. +let cachedOxlintCliPath = null +function oxlintCliPath() { + if (cachedOxlintCliPath === null) { + const manifestPath = createRequire(import.meta.url).resolve('oxlint/package.json') + const manifest = JSON.parse(readFileSync(manifestPath, 'utf8')) + cachedOxlintCliPath = path.join(path.dirname(manifestPath), manifest.bin.oxlint) + } + return cachedOxlintCliPath +} + +function oxlintCommand(scan) { + return { + command: process.execPath, + fixedArguments: [oxlintCliPath(), ...scan.args, '--format', 'json'] + } +} + +function spawnOxlintBatch(root, scan, batch) { + const { command, fixedArguments } = oxlintCommand(scan) + const result = spawnSync(command, [...fixedArguments, ...batch], { cwd: root, encoding: 'utf8', maxBuffer: 128 * 1024 * 1024 }) if (result.error) { - throw result.error + // Why restate it: a bare spawn error reads as a crash in the gate rather than a + // failure to launch Oxlint over this batch. + throw new Error( + `${scan.label} could not run Oxlint over ${batch.length} file(s): ${result.error.message}`, + { cause: result.error } + ) } if (!result.stdout.trim()) { process.stderr.write(result.stderr) throw new Error(`${scan.label} failed before producing diagnostics.`) } - return parseOxlintOutput(result.stdout, scan.label).diagnostics ?? [] + return result.stdout +} + +export function runOxlintScan(root, scan, files, spawnBatch = spawnOxlintBatch) { + const { command, fixedArguments } = oxlintCommand(scan) + const limit = maxBatchArgumentBytes({ fixedArguments: [command, ...fixedArguments] }) + const diagnostics = [] + for (const batch of batchFilesByArgumentBytes(files, limit)) { + diagnostics.push( + ...(parseOxlintOutput(spawnBatch(root, scan, batch), scan.label).diagnostics ?? []) + ) + } + return diagnostics } export function main( diff --git a/config/scripts/check-changed-code-quality.test.mjs b/config/scripts/check-changed-code-quality.test.mjs index 76a25802e5c..e85ddc8f880 100644 --- a/config/scripts/check-changed-code-quality.test.mjs +++ b/config/scripts/check-changed-code-quality.test.mjs @@ -1,10 +1,14 @@ import { describe, expect, it } from 'vitest' import { OXLINT_SCANS, + batchFilesByArgumentBytes, diagnosticTouchesAddedLines, + environmentEntries, isMovedCode, + maxBatchArgumentBytes, overlapsAddedLines, - parseAddedLineRanges + parseAddedLineRanges, + runOxlintScan } from './check-changed-code-quality.mjs' describe('changed-code quality line matching', () => { @@ -104,3 +108,199 @@ describe('moved-code exemption', () => { expect(isMovedCode(['', ' '], [['a()']])).toBe(false) }) }) + +describe('argument batching', () => { + // Mirrors the script's own accounting: every argv/envp entry costs a pointer beside its bytes. + const spawnBytes = (entries) => + entries.reduce((total, entry) => total + Buffer.byteLength(entry, 'utf8') + 12, 0) + // Large enough to exceed the real byte budget, so the batching that ships is what runs. + const oversizedSet = Array.from( + { length: 20000 }, + (_, index) => `src/generated/module-${index}.ts` + ) + const fixedArguments = [ + '/usr/local/bin/node', + '/repo/node_modules/oxlint/bin/oxlint', + '--format', + 'json' + ] + // Big enough that the budget must shrink below the cap, small enough to still batch. + const largeEnvironment = { PATH: '/usr/bin', ORCA_LARGE_VARIABLE: 'x'.repeat(300 * 1024) } + const fatEnvironment = { PATH: '/usr/bin', ORCA_FAT_VARIABLE: 'x'.repeat(900 * 1024) } + // Half of macOS kern.argmax: spawning Node fails from stack pressure near 970 KiB, well + // before the kernel's own 1 MiB E2BIG. + const POSIX_CEILING = 512 * 1024 + // CreateProcess' documented lpCommandLine maximum. + const WINDOWS_CEILING = 32767 + + it('runs a normal changed set as a single invocation', () => { + const files = Array.from({ length: 50 }, (_, index) => `src/renderer/src/module-${index}.tsx`) + + expect(batchFilesByArgumentBytes(files)).toEqual([files]) + }) + + it('uses a Windows-safe budget without shrinking Unix batches', () => { + expect(maxBatchArgumentBytes({ platform: 'win32', env: {} })).toBe(32767 - 8 * 1024) + expect(maxBatchArgumentBytes({ platform: 'linux', env: {} })).toBe(256 * 1024) + expect(maxBatchArgumentBytes({ platform: 'darwin', env: {} })).toBe(256 * 1024) + }) + + it('splits an oversized set into batches that each fit the limit', () => { + const batches = batchFilesByArgumentBytes(oversizedSet) + + expect(batches.length).toBeGreaterThan(1) + expect(batches.flat()).toEqual(oversizedSet) + for (const batch of batches) { + expect(spawnBytes(batch)).toBeLessThanOrEqual(256 * 1024) + } + }) + + // Why: an empty argument list makes Oxlint lint the whole working directory. + it('never emits an empty batch, even when the first path is longer than the limit', () => { + const batches = batchFilesByArgumentBytes(['src/very-long-path.ts', 'src/a.ts'], 10) + + expect(batches.every((batch) => batch.length > 0)).toBe(true) + expect(batches.flat()).toEqual(['src/very-long-path.ts', 'src/a.ts']) + }) + + it('counts multi-byte paths by their byte length, not their character count', () => { + expect(batchFilesByArgumentBytes(['src/\u00e9.ts', 'src/b.ts'], 30)).toEqual([ + ['src/\u00e9.ts'], + ['src/b.ts'] + ]) + }) + + // Why: execve() charges argv and the inherited environment against one ceiling, so a + // budget computed from the file list alone stacks 256 KiB of paths on top of whatever the + // shell already carries, and the spawn fails before Oxlint ever starts. + it('keeps the whole POSIX spawn — arguments and environment — under the ceiling', () => { + const limit = maxBatchArgumentBytes({ + platform: 'darwin', + fixedArguments, + env: largeEnvironment + }) + const batches = batchFilesByArgumentBytes(oversizedSet, limit) + const environmentCost = spawnBytes(environmentEntries(largeEnvironment)) + + expect(batches.length).toBeGreaterThan(1) + for (const batch of batches) { + expect(spawnBytes([...fixedArguments, ...batch]) + environmentCost).toBeLessThanOrEqual( + POSIX_CEILING + ) + } + }) + + it('shrinks the POSIX budget as the environment grows', () => { + const small = maxBatchArgumentBytes({ platform: 'darwin', fixedArguments, env: { A: 'a' } }) + const large = maxBatchArgumentBytes({ + platform: 'darwin', + fixedArguments, + env: largeEnvironment + }) + + expect(large).toBeLessThan(small) + }) + + // Why: CreateProcess caps the command line only; the environment ships in its own block. + it('budgets Windows from the command line alone and stays inside CreateProcess', () => { + const limit = maxBatchArgumentBytes({ + platform: 'win32', + fixedArguments, + env: fatEnvironment + }) + + expect(limit).toBe(maxBatchArgumentBytes({ platform: 'win32', fixedArguments, env: {} })) + for (const batch of batchFilesByArgumentBytes(oversizedSet, limit)) { + expect(spawnBytes([...fixedArguments, ...batch])).toBeLessThanOrEqual(WINDOWS_CEILING) + } + }) + + // Why: a zero budget would spawn Oxlint once per path instead of failing usefully. + it('floors the budget when the environment alone exceeds the ceiling', () => { + const limit = maxBatchArgumentBytes({ + platform: 'darwin', + fixedArguments, + env: { ORCA_FAT_VARIABLE: 'x'.repeat(4 * 1024 * 1024) } + }) + + expect(limit).toBe(8 * 1024) + }) + + it('ignores environment entries that carry no value', () => { + expect(environmentEntries({ A: 'a', B: undefined })).toEqual(['A=a']) + }) +}) + +describe('diagnostic collection across batches', () => { + const scan = { label: 'code quality', args: [] } + const diagnosticFor = (file) => ({ filename: file, message: `finding in ${file}` }) + const files = Array.from({ length: 20000 }, (_, index) => `src/generated/module-${index}.ts`) + const observeBatches = () => { + const batches = [] + runOxlintScan('/repo', scan, files, (_root, _scan, batch) => { + batches.push(batch) + return JSON.stringify({ diagnostics: [] }) + }) + return batches + } + const reportOnly = (failing) => (_root, _scan, batch) => + JSON.stringify({ diagnostics: batch.includes(failing) ? [diagnosticFor(failing)] : [] }) + + it('keeps the diagnostics of every batch, not just the last one', () => { + const spawnBatch = (_root, _scan, batch) => + JSON.stringify({ diagnostics: batch.map(diagnosticFor) }) + + expect(observeBatches().length).toBeGreaterThan(1) + expect(runOxlintScan('/repo', scan, files, spawnBatch)).toEqual(files.map(diagnosticFor)) + }) + + it('reports a finding that only a middle batch produces', () => { + const batches = observeBatches() + expect(batches.length).toBeGreaterThan(2) + const failing = batches.at(Math.floor(batches.length / 2)).at(0) + + expect(batches.at(0)).not.toContain(failing) + expect(batches.at(-1)).not.toContain(failing) + expect(runOxlintScan('/repo', scan, files, reportOnly(failing))).toEqual([ + diagnosticFor(failing) + ]) + }) + + it('reports a finding that only the last batch produces', () => { + const batches = observeBatches() + const failing = batches.at(-1).at(-1) + + expect(batches.at(0)).not.toContain(failing) + expect(runOxlintScan('/repo', scan, files, reportOnly(failing))).toEqual([ + diagnosticFor(failing) + ]) + }) + + // Why: Oxlint writes configuration failures to stdout, so swallowing it leaves the gate + // dying with no reason in the log. + it('surfaces what Oxlint printed when the output is not a report', () => { + const failure = 'Failed to parse oxlint configuration file.\n\n x Rule not found\n' + + expect(() => runOxlintScan('/repo', scan, ['src/a.ts'], () => failure)).toThrow( + /Rule not found/ + ) + }) + + // Why: the slice from the first brace to the last one takes a wrapper's own warning for + // the report, and the raw SyntaxError names neither the scan nor the warning. + it('surfaces a wrapper warning whose braces shadow the report', () => { + const polluted = ` WARN Unsupported engine: wanted: {"node":"24"}\n${JSON.stringify({ diagnostics: [] })}` + + expect(() => runOxlintScan('/repo', scan, ['src/a.ts'], () => polluted)).toThrow( + /Unsupported engine/ + ) + }) + + // Why: the whole point is that no single invocation carries the full argument list. + it('never hands the whole oversized set to one invocation', () => { + const batchSizes = observeBatches().map((batch) => batch.length) + + expect(batchSizes.length).toBeGreaterThan(1) + expect(Math.max(...batchSizes)).toBeLessThan(files.length) + }) +})