From 2a6ccac40c6ee5593e6545cf265a711c7eafa6c8 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 12 Jul 2026 22:04:36 -0700 Subject: [PATCH] ci: tolerate inherited TypeScript LOC drift (#106042) * ci: reconcile LOC baseline drift safely * ci: tolerate pre-existing LOC drift --- scripts/check-ts-max-loc.ts | 141 +++++++++++++++++++------- test/scripts/check-ts-max-loc.test.ts | 113 +++++++++++++++------ 2 files changed, 189 insertions(+), 65 deletions(-) diff --git a/scripts/check-ts-max-loc.ts b/scripts/check-ts-max-loc.ts index b9c0077efa80..ed9bb6aa6274 100644 --- a/scripts/check-ts-max-loc.ts +++ b/scripts/check-ts-max-loc.ts @@ -97,6 +97,10 @@ export function countPhysicalLines(content: string): number { return content.endsWith("\n") ? splitCount - 1 : splitCount; } +export function splitNullDelimitedPaths(output: string): string[] { + return output.split("\0").filter(Boolean); +} + async function countLines(filePath: string): Promise { const content = await readFile(filePath, "utf8"); return countPhysicalLines(content); @@ -151,21 +155,23 @@ export function findLocRatchetViolations(params: { ); } -export function findLocBaselineUpdateViolations(params: { +export function findVersionedBaselineViolations(params: { baseline: LocBaseline; - maxLines: number; - results: LocResult[]; + baseBaseline: LocBaseline; + baseLinesByChangedPath: ReadonlyMap; }): LocRatchetViolation[] { const violations: LocRatchetViolation[] = []; - for (const result of params.results) { - if (result.lines <= params.maxLines) { + for (const [filePath, lines] of Object.entries(params.baseline)) { + if (!params.baseLinesByChangedPath.has(filePath)) { + // Unchanged source may reconcile base drift; the current-tree check below still requires + // this baseline to equal the file's actual LOC, so arbitrary inflation remains stale. continue; } - const baselineLines = params.baseline[result.filePath]; - if (baselineLines === undefined) { - violations.push({ ...result, reason: "baseline-missing" }); - } else if (result.lines > baselineLines) { - violations.push({ ...result, baselineLines, reason: "grew" }); + const baseLines = params.baseLinesByChangedPath.get(filePath); + if (baseLines === undefined && params.baseBaseline[filePath] === undefined) { + violations.push({ filePath, lines, reason: "baseline-missing" }); + } else if (baseLines === undefined || lines > baseLines) { + violations.push({ filePath, lines, baselineLines: baseLines ?? 0, reason: "grew" }); } } return violations.toSorted( @@ -173,21 +179,16 @@ export function findLocBaselineUpdateViolations(params: { ); } -export function findVersionedBaselineViolations(params: { +export function filterPreexistingBaseDriftViolations(params: { baseline: LocBaseline; baseBaseline: LocBaseline; + changedPaths: ReadonlySet; + violations: LocRatchetViolation[]; }): LocRatchetViolation[] { - const violations: LocRatchetViolation[] = []; - for (const [filePath, lines] of Object.entries(params.baseline)) { - const baselineLines = params.baseBaseline[filePath]; - if (baselineLines === undefined) { - violations.push({ filePath, lines, reason: "baseline-missing" }); - } else if (lines > baselineLines) { - violations.push({ filePath, lines, baselineLines, reason: "grew" }); - } - } - return violations.toSorted( - (left, right) => right.lines - left.lines || left.filePath.localeCompare(right.filePath), + return params.violations.filter( + (violation) => + params.changedPaths.has(violation.filePath) || + params.baseline[violation.filePath] !== params.baseBaseline[violation.filePath], ); } @@ -254,6 +255,54 @@ function readBaselineAtRef( return content === undefined ? undefined : parseBaseline(content, `${baseRef}:${baselinePath}`); } +function readFileAtRef(baseRef: string, filePath: string): string | undefined { + try { + return execFileSync("git", ["show", `${baseRef}:${filePath}`], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }); + } catch { + return undefined; + } +} + +function readChangedPaths(baseRef: string): ReadonlySet { + const changedPaths = new Set( + splitNullDelimitedPaths( + execFileSync("git", ["diff", "--name-only", "-z", baseRef, "--"], { + encoding: "utf8", + }), + ), + ); + for (const filePath of splitNullDelimitedPaths( + execFileSync("git", ["ls-files", "--others", "--exclude-standard", "-z"], { + encoding: "utf8", + }), + )) { + changedPaths.add(filePath); + } + return changedPaths; +} + +function readBaseLinesForChangedBaselinePaths( + baseRef: string, + baseline: LocBaseline, + changedPaths: ReadonlySet, +): ReadonlyMap { + const baseLinesByChangedPath = new Map(); + for (const filePath of Object.keys(baseline)) { + if (!changedPaths.has(filePath)) { + continue; + } + const baseContent = readFileAtRef(baseRef, filePath); + baseLinesByChangedPath.set( + filePath, + baseContent === undefined ? undefined : countPhysicalLines(baseContent), + ); + } + return baseLinesByChangedPath; +} + function buildBaseline(results: LocResult[], maxLines: number): LocBaseline { return Object.fromEntries( results @@ -288,35 +337,59 @@ export async function main(argv = process.argv.slice(2)): Promise { ); if (writeBaseline) { - const baseline = await readBaseline(baselinePath); const comparisonBaseRef = resolveComparisonBaseRef(baselinePath, baseRef); if (!comparisonBaseRef) { throw new Error("Unable to resolve a comparison ref for the TypeScript LOC baseline update"); } const baseBaseline = readBaselineAtRef(comparisonBaseRef, baselinePath); + const updatedBaseline = buildBaseline(results, maxLines); + const changedPaths = readChangedPaths(comparisonBaseRef); // A missing baseline at a valid base ref is the one-time initialization path. - const violations = [ - ...(baseBaseline ? findVersionedBaselineViolations({ baseline, baseBaseline }) : []), - ...findLocBaselineUpdateViolations({ baseline, maxLines, results }), - ]; + const violations = baseBaseline + ? findVersionedBaselineViolations({ + baseline: updatedBaseline, + baseBaseline, + baseLinesByChangedPath: readBaseLinesForChangedBaselinePaths( + comparisonBaseRef, + updatedBaseline, + changedPaths, + ), + }) + : []; reportViolations(violations); if (violations.length > 0) { return 1; } - const updatedBaseline = buildBaseline(results, maxLines); await writeFile(baselinePath, `${JSON.stringify(updatedBaseline, null, 2)}\n`, "utf8"); writeStdoutLine(`updated ${baselinePath} (${Object.keys(updatedBaseline).length} files)`); return 0; } const baseline = await readBaseline(baselinePath); - const baseBaseline = readBaselineAtRef( - resolveComparisonBaseRef(baselinePath, baseRef), - baselinePath, - ); + const comparisonBaseRef = resolveComparisonBaseRef(baselinePath, baseRef); + const baseBaseline = readBaselineAtRef(comparisonBaseRef, baselinePath); + const changedPaths = comparisonBaseRef ? readChangedPaths(comparisonBaseRef) : new Set(); + const currentViolations = findLocRatchetViolations({ baseline, maxLines, results }); const violations = [ - ...(baseBaseline ? findVersionedBaselineViolations({ baseline, baseBaseline }) : []), - ...findLocRatchetViolations({ baseline, maxLines, results }), + ...(baseBaseline && comparisonBaseRef + ? findVersionedBaselineViolations({ + baseline, + baseBaseline, + baseLinesByChangedPath: readBaseLinesForChangedBaselinePaths( + comparisonBaseRef, + baseline, + changedPaths, + ), + }) + : []), + ...(baseBaseline + ? filterPreexistingBaseDriftViolations({ + baseline, + baseBaseline, + changedPaths, + violations: currentViolations, + }) + : currentViolations), ]; reportViolations(violations); return violations.length === 0 ? 0 : 1; diff --git a/test/scripts/check-ts-max-loc.test.ts b/test/scripts/check-ts-max-loc.test.ts index 396ab0b224a8..e0cb0177ac07 100644 --- a/test/scripts/check-ts-max-loc.test.ts +++ b/test/scripts/check-ts-max-loc.test.ts @@ -3,11 +3,12 @@ import { spawnSync } from "node:child_process"; import { describe, expect, it } from "vitest"; import { countPhysicalLines, - findLocBaselineUpdateViolations, + filterPreexistingBaseDriftViolations, findLocRatchetViolations, findVersionedBaselineViolations, isProductionTypeScriptFile, parseArgs, + splitNullDelimitedPaths, } from "../../scripts/check-ts-max-loc.js"; function runCheckTsMaxLoc(args: string[]) { @@ -93,6 +94,14 @@ describe("scripts/check-ts-max-loc", () => { expect(countPhysicalLines("one\ntwo\n")).toBe(2); }); + it("parses git paths without quoting or newline ambiguity", () => { + expect(splitNullDelimitedPaths("src/normal.ts\0src/café.ts\0src/line\nbreak.ts\0")).toEqual([ + "src/normal.ts", + "src/café.ts", + "src/line\nbreak.ts", + ]); + }); + it("excludes repository test and test-support naming conventions", () => { expect(isProductionTypeScriptFile("src/runtime.ts")).toBe(true); expect(isProductionTypeScriptFile("src/runtime.mts")).toBe(true); @@ -117,44 +126,86 @@ describe("scripts/check-ts-max-loc", () => { expect(isProductionTypeScriptFile("ui/src/i18n/lib/translate.ts")).toBe(true); }); - it("allows baseline updates only for decreases and removals", () => { - const violations = findLocBaselineUpdateViolations({ - maxLines: 500, - baseline: { + it("allows base drift reconciliation but rejects changed-source growth", () => { + const violations = findVersionedBaselineViolations({ + baseBaseline: { "src/grew.ts": 700, + "src/readded.ts": 700, "src/shrank.ts": 700, - "src/removed.ts": 700, + "src/preexisting-drift.ts": 700, }, - results: [ - { filePath: "src/grew.ts", lines: 701 }, - { filePath: "src/shrank.ts", lines: 650 }, - { filePath: "src/new.ts", lines: 501 }, + baseline: { + "src/grew.ts": 701, + "src/readded.ts": 650, + "src/shrank.ts": 650, + "src/new.ts": 501, + "src/preexisting-drift.ts": 710, + "src/preexisting-missing.ts": 510, + }, + baseLinesByChangedPath: new Map([ + ["src/grew.ts", 700], + ["src/readded.ts", undefined], + ["src/shrank.ts", 700], + ["src/new.ts", undefined], + ]), + }); + + expect(violations).toEqual([ + { filePath: "src/grew.ts", lines: 701, baselineLines: 700, reason: "grew" }, + { filePath: "src/readded.ts", lines: 650, baselineLines: 0, reason: "grew" }, + { filePath: "src/new.ts", lines: 501, reason: "baseline-missing" }, + ]); + }); + + it("ignores only drift already present on the comparison base", () => { + const violations = filterPreexistingBaseDriftViolations({ + baseBaseline: { + "src/changed.ts": 700, + "src/preexisting-growth.ts": 700, + "src/preexisting-shrink.ts": 700, + "src/removed-entry.ts": 700, + }, + baseline: { + "src/changed.ts": 700, + "src/preexisting-growth.ts": 700, + "src/preexisting-shrink.ts": 700, + "src/tampered.ts": 900, + }, + changedPaths: new Set(["src/changed.ts"]), + violations: [ + { filePath: "src/changed.ts", lines: 701, baselineLines: 700, reason: "grew" }, + { + filePath: "src/preexisting-growth.ts", + lines: 710, + baselineLines: 700, + reason: "grew", + }, + { + filePath: "src/preexisting-shrink.ts", + lines: 690, + baselineLines: 700, + reason: "baseline-stale", + }, + { filePath: "src/preexisting-new.ts", lines: 510, reason: "baseline-missing" }, + { filePath: "src/removed-entry.ts", lines: 700, reason: "baseline-missing" }, + { + filePath: "src/tampered.ts", + lines: 700, + baselineLines: 900, + reason: "baseline-stale", + }, ], }); expect(violations).toEqual([ - { filePath: "src/grew.ts", lines: 701, baselineLines: 700, reason: "grew" }, - { filePath: "src/new.ts", lines: 501, reason: "baseline-missing" }, - ]); - }); - - it("rejects versioned baseline additions and increases", () => { - const violations = findVersionedBaselineViolations({ - baseBaseline: { - "src/grew.ts": 700, - "src/shrank.ts": 700, - "src/removed.ts": 700, + { filePath: "src/changed.ts", lines: 701, baselineLines: 700, reason: "grew" }, + { filePath: "src/removed-entry.ts", lines: 700, reason: "baseline-missing" }, + { + filePath: "src/tampered.ts", + lines: 700, + baselineLines: 900, + reason: "baseline-stale", }, - baseline: { - "src/grew.ts": 701, - "src/shrank.ts": 650, - "src/new.ts": 501, - }, - }); - - expect(violations).toEqual([ - { filePath: "src/grew.ts", lines: 701, baselineLines: 700, reason: "grew" }, - { filePath: "src/new.ts", lines: 501, reason: "baseline-missing" }, ]); }); });