diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index 77c38c546ac..3d412df7ba8 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -1,3 +1,5 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' import process from 'node:process' import { pathToFileURL } from 'node:url' @@ -261,6 +263,49 @@ const DESKTOP_IRRELEVANT_PREFIXES = [ '.github/workflows/mobile-android-release.yml' ] +const STATIC_ANALYSIS_AUDIT_SCRIPTS = [ + 'audit:code-quality:native', + 'audit:code-quality:type-aware', + 'audit:anti-slop' +] + +// Positional arguments of an oxlint invocation are the trees it lints. `--config` consumes the +// next token; every other flag here is valueless. +function oxlintScanRoots(command) { + const roots = [] + for (const segment of command.split('&&')) { + const tokens = segment.trim().split(/\s+/).filter(Boolean) + if (tokens[0] !== 'oxlint') { + continue + } + for (let index = 1; index < tokens.length; index += 1) { + if (tokens[index] === '--config') { + index += 1 + } else if (!tokens[index].startsWith('-')) { + roots.push(tokens[index]) + } + } + } + return roots +} + +// Why derived from the commands rather than listed here: `mobile/` is desktop-irrelevant for every +// other job, yet these audits lint it. A second, hand-maintained copy of "which trees the gate +// reads" is what let #20702 land violations no PR check ran, so read it off the argv instead. +function readStaticAnalysisScanRoots() { + const manifest = join(import.meta.dirname, '../../package.json') + const { scripts = {} } = JSON.parse(readFileSync(manifest, 'utf8')) + return [ + ...new Set( + STATIC_ANALYSIS_AUDIT_SCRIPTS.flatMap((name) => oxlintScanRoots(scripts[name] ?? '')) + ) + ] +} + +export const STATIC_ANALYSIS_SCAN_ROOTS = readStaticAnalysisScanRoots() + +const STATIC_ANALYSIS_SCAN_PREFIXES = STATIC_ANALYSIS_SCAN_ROOTS.map((root) => `${root}/`) + export function isDocsOnlyPath(file) { if (DOCS_ONLY_FILES.has(file)) { return true @@ -298,10 +343,15 @@ export function classifyPrJobs(changedFiles) { shouldRun && (forceAll || ALWAYS_ON_CODE_JOBS.has(job) || jobDetector(job)(changedFiles)) ]) ) + // Why outside should_run: a mobile-only diff is desktop-irrelevant and skips every job above, + // but the repo-wide audits lint mobile/, and skipping them lands the violation on main, where + // it then fails this same gate on every later PR's merge ref. + jobs.static_analysis = jobs.static_analysis || changedFiles.some(isStaticAnalysisScannedPath) return { should_run: shouldRun, native_cache_changed: shouldRun && (emptyDiff || changedFiles.some(isNativeCacheInputPath)), - mobile_dependencies: shouldRun && needsMobileDependencies(changedFiles), + mobile_dependencies: + (shouldRun || jobs.static_analysis) && needsMobileDependencies(changedFiles), ...jobs } } @@ -358,6 +408,13 @@ function isDesktopIrrelevantPath(file) { return matchesPrefix(file, DESKTOP_IRRELEVANT_PREFIXES) } +function isStaticAnalysisScannedPath(file) { + // Fail closed: roots we failed to parse must keep the gate, not silently drop it. + return ( + STATIC_ANALYSIS_SCAN_PREFIXES.length === 0 || matchesPrefix(file, STATIC_ANALYSIS_SCAN_PREFIXES) + ) +} + function isNativeCacheInputPath(file) { return NATIVE_CACHE_FILES.has(file) || matchesPrefix(file, NATIVE_CACHE_PREFIXES) } diff --git a/config/scripts/pr-code-change-scope.test.mjs b/config/scripts/pr-code-change-scope.test.mjs index e622dd8603a..6e39bd9b20a 100644 --- a/config/scripts/pr-code-change-scope.test.mjs +++ b/config/scripts/pr-code-change-scope.test.mjs @@ -7,7 +7,8 @@ import { classifyPrJobs, isDocsOnlyPath, PR_CHECK_JOBS, - shouldRunPrChecks + shouldRunPrChecks, + STATIC_ANALYSIS_SCAN_ROOTS } from './pr-code-change-scope.mjs' const projectDir = resolve(import.meta.dirname, '../..') @@ -327,11 +328,41 @@ describe('per-job path classification', () => { expect( classifyPrJobs(['src/main/index.ts', 'mobile/src/session/a.test.ts']).mobile_dependencies ).toBe(true) - // Why false: a mobile-only diff skips every desktop job, so the install step's own - // job never runs and claiming the install is needed contradicts should_run. - expect(classifyPrJobs(['mobile/package.json']).mobile_dependencies).toBe(false) + // Why true: a mobile-only diff still skips the desktop suite, but the repo-wide audits lint + // mobile/, so static analysis runs and its changed-code pass needs the mobile types. + expect(classifyPrJobs(['mobile/package.json']).mobile_dependencies).toBe(true) expect(classifyPrJobs(['mobile/package.json']).should_run).toBe(false) - expect(classifyPrJobs(['README.md', 'mobile/src/a.ts']).mobile_dependencies).toBe(false) + expect(classifyPrJobs(['README.md', 'mobile/src/a.ts']).mobile_dependencies).toBe(true) + }) + + // Why: `mobile/` is desktop-irrelevant for every other job, so a mobile-only diff used to skip + // the audits that do lint it. That is how #20702 landed two duplicate imports which then failed + // this gate on every later PR's merge ref until #20895 swept them. + it('runs static analysis for a mobile-only diff without dragging in the desktop suite', () => { + const result = classifyPrJobs([ + 'mobile/src/test-support/rpc-recording/adapters/push-registration-mount-adapters.ts' + ]) + expect(result.static_analysis).toBe(true) + expect(result.mobile_dependencies).toBe(true) + expect(result.should_run).toBe(false) + for (const job of ['typecheck', 'test', 'package', 'package_windows', 'git_compatibility']) { + expect(result[job], job).toBe(false) + } + }) + + // The ratchet: adding a tree to an audit command has to widen this trigger on its own. + it('runs static analysis for every tree the audit commands scan', () => { + expect(STATIC_ANALYSIS_SCAN_ROOTS).toEqual( + expect.arrayContaining(['src', 'config', 'tests', 'mobile']) + ) + for (const root of STATIC_ANALYSIS_SCAN_ROOTS) { + expect(classifyPrJobs([`${root}/changed-file.ts`]).static_analysis, root).toBe(true) + } + }) + + it('leaves diffs the audits never read out of static analysis', () => { + expect(classifyPrJobs(['README.md']).static_analysis).toBe(false) + expect(classifyPrJobs(['cloud/apps/relay/src/index.ts']).static_analysis).toBe(false) }) it('keeps unit-test-only diffs out of packaging', () => {