From 89fdc210484340a604adc64dc9d02f0668e105f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 10 Sep 2026 17:29:19 +0200 Subject: [PATCH 1/3] chore(gates): name the added modules and their import paths when an eager closure grows The no-growth diagnostic in scripts/__tests__/eager-closure-budgets.ts only named the FIRST newly evaluated module and always advised a dynamic import. On #2423 that sent five reviewers toward the wrong fix when the growth was a small new module that belonged in a module every affected entry already evaluated -- the dynamic-import advice was never coherent for a brand-new module with no old edge to defer. - describeClosureGrowth now lists every added module (bounded to 10), each with the shortest static import route from the entry to it. - describeSharedGrowthHomes runs once after every entry is evaluated: when two or more entries grew by the same added module, it names the modules they already evaluate at the merge-base under that module's own package -- candidate homes, not a verdict. - classifyGrowth's closing advice now states the two common causes (a new static edge, or something that used to load lazily) and the two remedies (give the symbol a home in a module already in the closure, or make the new edge lazy) instead of prescribing one fix. The verdict logic (when an entry is flagged as having grown) is unchanged. --- .../__tests__/eager-closure-budgets.test.ts | 139 ++++++++++-- scripts/__tests__/eager-closure-budgets.ts | 200 ++++++++++++++++-- 2 files changed, 311 insertions(+), 28 deletions(-) diff --git a/scripts/__tests__/eager-closure-budgets.test.ts b/scripts/__tests__/eager-closure-budgets.test.ts index b686507e5d..e858ee050d 100644 --- a/scripts/__tests__/eager-closure-budgets.test.ts +++ b/scripts/__tests__/eager-closure-budgets.test.ts @@ -11,12 +11,14 @@ import { renamedSince, } from './committed-source-tree.ts'; import { + addedModules, APPROVED_OVER_CEILING, classifyGrowth, classifyNewEntry, describeClosureGrowth, describeClosurePressure, describePlatformOffenders, + describeSharedGrowthHomes, discoverFacadeEntryFiles, eagerClosureEntries, entryCategoryOf, @@ -71,6 +73,40 @@ test('no-growth fails growth with both counts and passes an equal or smaller clo expect(classifyGrowth('x.ts', 42, 43)).toMatch(/evaluates 43 modules.*merge-base evaluated 42/); }); +test('growth advice names both causes and both remedies, not one prescribed fix', () => { + // #2423's review: a message that only ever says "move it behind a dynamic import" is wrong + // advice when the growth is a new module that belongs in an existing one. This pins that the + // verdict states both common causes and leaves the remedy to the reader. + const finding = classifyGrowth('x.ts', 42, 43) ?? ''; + expect(finding).toMatch(/new static edge/); + expect(finding).toMatch(/used to load on demand/); + expect(finding).toMatch(/home in a module the closure already evaluates/); + expect(finding).toMatch(/function-scoped `await import`/); +}); + +test('growth against the merge-base lists every added module, each with its shortest route, bounded', () => { + // The hole: the old diagnostic named only the FIRST added module, which hid the pattern when a + // whole PR's growth was really "everyone reached one new module" (#2423 review) rather than one + // entry's own new edge. This pins that every added module is named (bounded), not just the + // first, and that the route shown is the whole chain, not just the leaf. + const entry = '/repo/entry.ts'; + const graph = new Map([[entry, null]]); + graph.set('/repo/old.ts', entry); // present at the merge-base too: not "added" + graph.set('/repo/new-a.ts', entry); + graph.set('/repo/new-b.ts', '/repo/new-a.ts'); + for (let index = 0; index < 12; index += 1) graph.set(`/repo/new-filler-${index}.ts`, entry); + const base = new Set(['/repo/entry.ts', '/repo/old.ts']); + + const described = describeClosureGrowth(graph, base, entry, '/repo'); + expect(described, 'the route must show the whole chain, not just the leaf').toContain( + 'new-a.ts → new-b.ts', + ); + expect(described, 'output must be capped and say how much it omitted').toMatch( + /\(\+\d+ more newly evaluated module\(s\)\)/, + ); + expect(described.split('\n').length).toBeLessThan(15); +}); + test('a first-introduced entry fits its category ceiling or carries an approval', () => { expect(classifyNewEntry('x.ts', 'vocabulary-facade', 4, false)).toBeNull(); expect(classifyNewEntry('x.ts', 'vocabulary-facade', 5, false)).toMatch( @@ -179,6 +215,55 @@ test('an entry that evaluates only itself is described without pretending to an expect(describeClosurePressure(graph, '/repo/solo.ts', '/repo')).toContain('only itself'); }); +test('growth shared by two entries names the merge-base modules they already have in common', () => { + // The aggregation #2423's review asked for: two entries independently grow by the SAME new + // module. Per-entry diagnostics would each show that same chain with no hint that a shared + // existing home was available -- this names the modules BOTH entries already evaluate, scoped + // to the added module's own package, once. + const addedFile = '/repo/packages/demo/src/new-thing.ts'; + const sharedHome = '/repo/packages/demo/src/shared-home.ts'; + const otherPkgFile = '/repo/packages/other/src/not-a-home.ts'; // different package: excluded + const onlyInOne = '/repo/packages/demo/src/only-in-a.ts'; // not common to both: excluded + + const graphA = new Map([ + ['/repo/entry-a.ts', null], + [sharedHome, '/repo/entry-a.ts'], + [onlyInOne, '/repo/entry-a.ts'], + [otherPkgFile, '/repo/entry-a.ts'], + ]); + const graphB = new Map([ + ['/repo/entry-b.ts', null], + [sharedHome, '/repo/entry-b.ts'], + [otherPkgFile, '/repo/entry-b.ts'], + ]); + + const shared = describeSharedGrowthHomes( + [ + { id: 'entry-a.ts', added: [addedFile], baseGraph: graphA }, + { id: 'entry-b.ts', added: [addedFile], baseGraph: graphB }, + ], + '/repo', + ); + + expect(shared, 'labeled neutrally, not as a prescribed fix').toMatch(/possible homes/i); + expect(shared).toContain('packages/demo/src/shared-home.ts'); + expect(shared, 'only in one entry: not a shared home').not.toContain('only-in-a.ts'); + expect(shared, 'a different package than the added module: not a candidate home').not.toContain( + 'not-a-home.ts', + ); +}); + +test('growth that no other entry shares produces no shared-homes note', () => { + const graphA = new Map([['/repo/entry-a.ts', null]]); + expect( + describeSharedGrowthHomes( + [{ id: 'entry-a.ts', added: ['/repo/new.ts'], baseGraph: graphA }], + '/repo', + ), + 'nothing to aggregate when only one entry grew by this module', + ).toBeNull(); +}); + /** * A committed fixture repository: one workspace package with a manifest export target, a * top-level façade, a nested façade, and a test source. Everything here is TRACKED, so anything a @@ -350,14 +435,14 @@ function basePathOf(entryFile: string): string | null { return baseTree.isFile(absolute(file)) ? file : null; } -const baseClosures = new Map>(); -function baseClosureOf(baseFile: string): ReadonlySet { - let closure = baseClosures.get(baseFile); - if (!closure) { - closure = new Set(eagerClosureGraphOf(absolute(baseFile), baseTree).keys()); - baseClosures.set(baseFile, closure); +const baseGraphs = new Map>(); +function baseGraphOf(baseFile: string): ReadonlyMap { + let graph = baseGraphs.get(baseFile); + if (!graph) { + graph = eagerClosureGraphOf(absolute(baseFile), baseTree); + baseGraphs.set(baseFile, graph); } - return closure; + return graph; } const platformFacades = entries.filter((entry) => entry.category === 'platform-facade'); @@ -368,6 +453,19 @@ const carried = others.flatMap((entry) => { }); const introduced = others.filter((entry) => basePathOf(entry.entryFile) === null); +/** + * Every carried entry's growth data, computed once so the per-entry NO-GROWTH test below and the + * cross-entry shared-homes note after it read the same graphs instead of walking each closure + * twice. + */ +const carriedGrowth = carried.map((entry) => { + const entryPath = absolute(entry.entryFile); + const graph = eagerClosureGraphOf(entryPath); + const baseGraph = baseGraphOf(entry.baseFile); + const base = new Set(baseGraph.keys()); + return { entry, entryPath, graph, baseGraph, base, added: addedModules(graph, base, entryPath) }; +}); + test('every hub exists and is not also a discovered façade', () => { const discovered = new Set(discoverFacadeEntryFiles(repoRoot)); expect(HUB_ENTRY_FILES.filter((file) => !fs.existsSync(absolute(file)))).toEqual([]); @@ -402,20 +500,37 @@ test.for(platformFacades)('$id evaluates exactly one module: itself', (entry) => ).toBe(PLATFORM_FACADE_CLOSURE); }); -test.for(carried)('$id evaluates no more modules than at the merge-base', (entry) => { - const entryPath = absolute(entry.entryFile); - const graph = eagerClosureGraphOf(entryPath); - const base = baseClosureOf(entry.baseFile); +test.for(carriedGrowth)('$entry.id evaluates no more modules than at the merge-base', (growth) => { + const { entry, entryPath, graph, base } = growth; const finding = classifyGrowth(entry.id, base.size, graph.size); expect( finding, finding === null ? '' - : `${finding}\n\nFirst newly evaluated module, by shortest import route:\n` + + : `${finding}\n\nNewly evaluated module(s), each by shortest import route from the entry:\n` + describeClosureGrowth(graph, base, entryPath, repoRoot), ).toBeNull(); }); +test('entries that grow by the same new module report shared merge-base homes once', () => { + // #2423's review: when several entries grow because they all reached the same new module, the + // per-entry test above already fails once per entry -- this adds ONE more diagnostic naming the + // modules they already have in common, instead of leaving a reader to notice the repetition + // across several separate failures by hand. It can only fail alongside at least two already-red + // entries above, so it never changes the gate's own pass/fail verdict. + const growths = carriedGrowth + .filter((growth) => growth.added.length > 0) + .map((growth) => ({ id: growth.entry.id, added: growth.added, baseGraph: growth.baseGraph })); + const shared = describeSharedGrowthHomes(growths, repoRoot); + expect( + shared, + shared === null + ? '' + : 'More than one entry grew by the same new module -- see the shared homes below instead ' + + `of chasing each entry's own diagnostic separately:\n\n${shared}`, + ).toBeNull(); +}); + test.for(introduced)( '$id is first-introduced and fits the $category ceiling or carries an approval', (entry) => { diff --git a/scripts/__tests__/eager-closure-budgets.ts b/scripts/__tests__/eager-closure-budgets.ts index 6f61185b14..a0b61046b6 100644 --- a/scripts/__tests__/eager-closure-budgets.ts +++ b/scripts/__tests__/eager-closure-budgets.ts @@ -187,13 +187,24 @@ export function eagerClosureEntries(repoRoot: string): EagerClosureEntry[] { ]; } -/** The no-growth verdict: `null` unless the head closure is larger than the merge-base one. */ +/** + * The no-growth verdict: `null` unless the head closure is larger than the merge-base one. + * + * The closing sentence deliberately does not prescribe one fix. #2423's review found that a + * generic "move it behind a dynamic import" sent five reviewers toward the wrong change: the + * growth there was a small new module that belonged in a module every affected entry already + * evaluated, not behind a lazy boundary. There are two common causes and two remedies, and which + * applies is exactly what the added-module listing this verdict is always printed alongside + * (`describeClosureGrowth`) is for. + */ export function classifyGrowth(id: string, base: number, head: number): string | null { if (head <= base) return null; return ( - `${id} evaluates ${head} modules on import; the merge-base evaluated ${base}. Something ` + - 'that used to load on demand now loads eagerly, or a new static edge was added: move it ' + - 'behind a function-scoped `await import`.' + `${id} evaluates ${head} modules on import; the merge-base evaluated ${base}. That means ` + + 'either a new static edge was added, or something that used to load on demand now loads ' + + 'eagerly. The fix is either to give the new code a home in a module the closure already ' + + 'evaluates, or to move the new edge behind a function-scoped `await import` -- see the ' + + 'added module(s) below for which one fits.' ); } @@ -358,10 +369,29 @@ export function describeClosurePressure( ); } +/** How many of `added` a growth diagnostic names individually before it just counts the rest. */ +const REPORTED_ADDED_MODULES = 10; + +/** Head-closure files absent from the merge-base closure -- what actually grew, entry excluded. */ +export function addedModules( + graph: ReadonlyMap, + baseClosure: ReadonlySet, + entryPath: string, +): string[] { + return [...graph.keys()].filter((file) => file !== entryPath && !baseClosure.has(file)); +} + /** - * Where an existing entry grew: the shortest import route to the first module the merge-base did - * not evaluate, plus how many more there are. The walk is breadth-first, so the first one in - * closure order is the shallowest, which is where the new edge almost always is. + * Every newly evaluated module against the merge-base (bounded to `REPORTED_ADDED_MODULES`), each + * with the shortest static import route from the entry down to it. + * + * #2423's review is why this names more than one module: the previous version of this diagnostic + * printed only the FIRST added module, which hid the pattern when the real growth was one small + * new module reached from several places. A reader saw one chain, read it as "this one edge + * should be lazy", and proposed a dynamic import for what was actually a shared constant -- five + * times, across five separate CI failures, because nothing in any one entry's message showed that + * the "new" modules were mostly the same one. Listing every added module (still bounded) lets a + * reader see that shape from a single failure. */ export function describeClosureGrowth( graph: ReadonlyMap, @@ -369,11 +399,13 @@ export function describeClosureGrowth( entryPath: string, repoRoot: string, ): string { - const added = [...graph.keys()].filter((file) => file !== entryPath && !baseClosure.has(file)); - const first = added[0]; - if (first === undefined) return ' (no module is new against the merge-base)'; - const more = added.length > 1 ? `\n (+${added.length - 1} more newly evaluated module(s))` : ''; - return ` ${formatImportChain(graph, first, repoRoot)}${more}`; + const added = addedModules(graph, baseClosure, entryPath); + if (added.length === 0) return ' (no module is new against the merge-base)'; + const shown = added.slice(0, REPORTED_ADDED_MODULES); + const lines = shown.map((file) => ` ${formatImportChainArrow(graph, file, repoRoot)}`); + const hidden = added.length - shown.length; + const more = hidden > 0 ? `\n (+${hidden} more newly evaluated module(s))` : ''; + return `${lines.join('\n')}${more}`; } /** @@ -400,24 +432,46 @@ export function describePlatformOffenders( } /** - * The import chain from a closure's entry down to `target`, rendered one edge per line. + * The hops from a closure's entry down to `target`, repo-relative, shallowest first. * * #1960 asks a violation to "name the offending edge chain". A sorted set of evaluated files names * the destination but not the route, which leaves the reader to rediscover by hand which import * actually pulled it in. `eagerClosureGraphOf` records each file's discoverer, so the route is * just a walk back up, and because that walk is breadth-first the route is the shortest one. */ -function formatImportChain( +function chainTo( graph: ReadonlyMap, target: string, repoRoot: string, -): string { +): string[] { const chain: string[] = []; for (let at: string | null | undefined = target; at != null; at = graph.get(at)) { chain.push(path.relative(repoRoot, at)); if (chain.length > 64) break; // defensive: a cycle would otherwise spin here } - return chain.reverse().join('\n -> '); + return chain.reverse(); +} + +/** The import chain from a closure's entry down to `target`, rendered one edge per line. */ +function formatImportChain( + graph: ReadonlyMap, + target: string, + repoRoot: string, +): string { + return chainTo(graph, target, repoRoot).join('\n -> '); +} + +/** + * The same route as `formatImportChain`, arrow-joined on one line -- compact enough to list + * several of them (`describeClosureGrowth`) without the multi-line chain format burying the list + * itself under indentation. + */ +function formatImportChainArrow( + graph: ReadonlyMap, + target: string, + repoRoot: string, +): string { + return chainTo(graph, target, repoRoot).join(' → '); } /** How many hops from the entry down to `target`, bounded so a cycle cannot spin. */ @@ -429,3 +483,117 @@ function chainLength(graph: ReadonlyMap, target: string): } return length; } + +/** A growing entry's contribution to the cross-entry shared-homes aggregation below. */ +export type GrowthForAggregation = { + /** The entry's label, matching `EagerClosureEntry.id`. */ + id: string; + /** This entry's added modules (absolute paths), as `addedModules` returns them. */ + added: readonly string[]; + /** This entry's merge-base closure graph -- gives both membership and forward edges. */ + baseGraph: ReadonlyMap; +}; + +/** How many candidate homes `describeSharedGrowthHomes` names before it just counts the rest. */ +const MAX_SHARED_HOMES = 30; + +/** + * The workspace package (or the root `src/` mechanics tree) a repo-relative path lives under -- + * the scope `describeSharedGrowthHomes` searches for an existing home, since a candidate outside + * the added module's own package is not a home the added module could plausibly move into. + */ +function packageOf(relativeFile: string): string { + const match = /^(packages\/[^/]+)\//.exec(relativeFile); + return match ? match[1] : 'src'; +} + +/** True when nothing in `graph` records `candidate` as the direct importer of a same-package file. */ +function hasNoInPackageChild( + candidate: string, + pkg: string, + graph: ReadonlyMap, + repoRoot: string, +): boolean { + for (const [child, parent] of graph) { + if (parent === candidate && packageOf(path.relative(repoRoot, child)) === pkg) return false; + } + return true; +} + +/** + * When two or more entries grow by the SAME newly-added module, the per-entry diagnostic above + * cannot show the shape that actually matters: there is no old edge to make lazy, because the + * module is brand new, so "move it behind a dynamic import" is not even coherent advice. The + * useful question is "where does this already have a home", and the answer is scoped to what + * every affected entry already evaluates, under the added module's own package -- a real + * candidate list instead of "somewhere in the repo". + * + * #2423's review: five reviewers each independently proposed a dynamic import for a constant that + * a module already imported eagerly wherever it was needed; the fix was moving the constant + * there. Nothing in any one entry's own message could show that shape, because each entry's + * diagnostic only ever describes that one entry's own closure. This runs once, after every entry + * has been evaluated, and is silent (`null`) unless at least two entries actually share an added + * module and that intersection is non-empty. + * + * Sorted with modules that have no in-package eager import of their own first when that is cheap + * to compute -- it already is here, since a candidate's forward edges are read straight off the + * base graph that produced it, no extra tree read required -- because a leaf module is the safer + * home: adding a symbol to it cannot itself grow anyone else's closure. + */ +export function describeSharedGrowthHomes( + growths: readonly GrowthForAggregation[], + repoRoot: string, +): string | null { + const entriesByAddedModule = new Map(); + for (const growth of growths) { + for (const added of growth.added) { + const group = entriesByAddedModule.get(added); + if (group) group.push(growth); + else entriesByAddedModule.set(added, [growth]); + } + } + + const homePackage = new Map(); + const homeGraph = new Map>(); + for (const [added, group] of entriesByAddedModule) { + if (group.length < 2) continue; + const [first, ...rest] = group; + if (!first) continue; + const pkg = packageOf(path.relative(repoRoot, added)); + for (const candidate of first.baseGraph.keys()) { + if (homePackage.has(candidate)) continue; + if (packageOf(path.relative(repoRoot, candidate)) !== pkg) continue; + if (rest.every((other) => other.baseGraph.has(candidate))) { + homePackage.set(candidate, pkg); + homeGraph.set(candidate, first.baseGraph); + } + } + } + if (homePackage.size === 0) return null; + + const sorted = [...homePackage.keys()].sort((left, right) => { + const leftLeaf = hasNoInPackageChild( + left, + homePackage.get(left) as string, + homeGraph.get(left) as ReadonlyMap, + repoRoot, + ); + const rightLeaf = hasNoInPackageChild( + right, + homePackage.get(right) as string, + homeGraph.get(right) as ReadonlyMap, + repoRoot, + ); + if (leftLeaf !== rightLeaf) return leftLeaf ? -1 : 1; + return path.relative(repoRoot, left).localeCompare(path.relative(repoRoot, right)); + }); + + const shown = sorted.slice(0, MAX_SHARED_HOMES); + const hidden = sorted.length - shown.length; + const lines = shown.map((file) => ` ${path.relative(repoRoot, file)}`); + const more = hidden > 0 ? `\n (+${hidden} more)` : ''; + return ( + 'Modules every failing entry already evaluates (possible homes for a shared symbol; not a ' + + `statement that any of them is the right owner):\n${lines.join('\n')}${more}` + ); +} From 80ff4e626928e860278010211ea9012a0ea38e5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 10 Sep 2026 18:50:16 +0200 Subject: [PATCH 2/3] chore(gates): split the shared-growth-homes diagnostic into small helpers --- scripts/__tests__/eager-closure-budgets.ts | 152 ++++++++++++++------- 1 file changed, 103 insertions(+), 49 deletions(-) diff --git a/scripts/__tests__/eager-closure-budgets.ts b/scripts/__tests__/eager-closure-budgets.ts index a0b61046b6..2d12abe58e 100644 --- a/scripts/__tests__/eager-closure-budgets.ts +++ b/scripts/__tests__/eager-closure-budgets.ts @@ -520,30 +520,10 @@ function hasNoInPackageChild( return true; } -/** - * When two or more entries grow by the SAME newly-added module, the per-entry diagnostic above - * cannot show the shape that actually matters: there is no old edge to make lazy, because the - * module is brand new, so "move it behind a dynamic import" is not even coherent advice. The - * useful question is "where does this already have a home", and the answer is scoped to what - * every affected entry already evaluates, under the added module's own package -- a real - * candidate list instead of "somewhere in the repo". - * - * #2423's review: five reviewers each independently proposed a dynamic import for a constant that - * a module already imported eagerly wherever it was needed; the fix was moving the constant - * there. Nothing in any one entry's own message could show that shape, because each entry's - * diagnostic only ever describes that one entry's own closure. This runs once, after every entry - * has been evaluated, and is silent (`null`) unless at least two entries actually share an added - * module and that intersection is non-empty. - * - * Sorted with modules that have no in-package eager import of their own first when that is cheap - * to compute -- it already is here, since a candidate's forward edges are read straight off the - * base graph that produced it, no extra tree read required -- because a leaf module is the safer - * home: adding a symbol to it cannot itself grow anyone else's closure. - */ -export function describeSharedGrowthHomes( +/** Every entry that grew, indexed by each of its added modules -- one entry may appear under several. */ +function groupGrowthsByAddedModule( growths: readonly GrowthForAggregation[], - repoRoot: string, -): string | null { +): Map { const entriesByAddedModule = new Map(); for (const growth of growths) { for (const added of growth.added) { @@ -552,44 +532,86 @@ export function describeSharedGrowthHomes( else entriesByAddedModule.set(added, [growth]); } } + return entriesByAddedModule; +} + +/** + * One added-module group's contribution to the candidate homes: when at least two entries grew + * by `added`, every merge-base module the first entry evaluates that is (a) in the added + * module's own package, (b) not already claimed as a home for a different added module, and (c) + * also evaluated by every other entry in the group, is recorded as a home -- mutating + * `homePackage` and `homeGraph` in place. + */ +function collectSharedHomesForGroup( + added: string, + group: readonly GrowthForAggregation[], + repoRoot: string, + homePackage: Map, + homeGraph: Map>, +): void { + if (group.length < 2) return; + const [first, ...rest] = group; + if (!first) return; + const pkg = packageOf(path.relative(repoRoot, added)); + for (const candidate of first.baseGraph.keys()) { + if (homePackage.has(candidate)) continue; + if (packageOf(path.relative(repoRoot, candidate)) !== pkg) continue; + if (!rest.every((other) => other.baseGraph.has(candidate))) continue; + homePackage.set(candidate, pkg); + homeGraph.set(candidate, first.baseGraph); + } +} +/** + * For each added module shared by two or more entries, the modules every one of those entries + * already evaluates at the merge-base, scoped to the added module's own package -- the candidate + * homes a shared symbol could plausibly move into. A module already claimed as a home for one + * added module is not reconsidered for another. + */ +function sharedGrowthHomeCandidates( + entriesByAddedModule: ReadonlyMap, + repoRoot: string, +): { + homePackage: Map; + homeGraph: Map>; +} { const homePackage = new Map(); const homeGraph = new Map>(); for (const [added, group] of entriesByAddedModule) { - if (group.length < 2) continue; - const [first, ...rest] = group; - if (!first) continue; - const pkg = packageOf(path.relative(repoRoot, added)); - for (const candidate of first.baseGraph.keys()) { - if (homePackage.has(candidate)) continue; - if (packageOf(path.relative(repoRoot, candidate)) !== pkg) continue; - if (rest.every((other) => other.baseGraph.has(candidate))) { - homePackage.set(candidate, pkg); - homeGraph.set(candidate, first.baseGraph); - } - } + collectSharedHomesForGroup(added, group, repoRoot, homePackage, homeGraph); } - if (homePackage.size === 0) return null; + return { homePackage, homeGraph }; +} - const sorted = [...homePackage.keys()].sort((left, right) => { - const leftLeaf = hasNoInPackageChild( - left, - homePackage.get(left) as string, - homeGraph.get(left) as ReadonlyMap, - repoRoot, - ); - const rightLeaf = hasNoInPackageChild( - right, - homePackage.get(right) as string, - homeGraph.get(right) as ReadonlyMap, +/** + * Candidate homes with modules that have no in-package eager import of their own first -- + * a leaf module is the safer home, since adding a symbol to it cannot itself grow anyone else's + * closure -- then alphabetically by repo-relative path. + */ +function sortHomesLeavesFirst( + homePackage: ReadonlyMap, + homeGraph: ReadonlyMap>, + repoRoot: string, +): string[] { + const isLeaf = (candidate: string): boolean => + hasNoInPackageChild( + candidate, + homePackage.get(candidate) as string, + homeGraph.get(candidate) as ReadonlyMap, repoRoot, ); + return [...homePackage.keys()].sort((left, right) => { + const leftLeaf = isLeaf(left); + const rightLeaf = isLeaf(right); if (leftLeaf !== rightLeaf) return leftLeaf ? -1 : 1; return path.relative(repoRoot, left).localeCompare(path.relative(repoRoot, right)); }); +} - const shown = sorted.slice(0, MAX_SHARED_HOMES); - const hidden = sorted.length - shown.length; +/** Renders the sorted candidate homes as the bounded, capped message block. */ +function formatSharedGrowthHomes(sortedHomes: readonly string[], repoRoot: string): string { + const shown = sortedHomes.slice(0, MAX_SHARED_HOMES); + const hidden = sortedHomes.length - shown.length; const lines = shown.map((file) => ` ${path.relative(repoRoot, file)}`); const more = hidden > 0 ? `\n (+${hidden} more)` : ''; return ( @@ -597,3 +619,35 @@ export function describeSharedGrowthHomes( `statement that any of them is the right owner):\n${lines.join('\n')}${more}` ); } + +/** + * When two or more entries grow by the SAME newly-added module, the per-entry diagnostic above + * cannot show the shape that actually matters: there is no old edge to make lazy, because the + * module is brand new, so "move it behind a dynamic import" is not even coherent advice. The + * useful question is "where does this already have a home", and the answer is scoped to what + * every affected entry already evaluates, under the added module's own package -- a real + * candidate list instead of "somewhere in the repo". + * + * #2423's review: five reviewers each independently proposed a dynamic import for a constant that + * a module already imported eagerly wherever it was needed; the fix was moving the constant + * there. Nothing in any one entry's own message could show that shape, because each entry's + * diagnostic only ever describes that one entry's own closure. This runs once, after every entry + * has been evaluated, and is silent (`null`) unless at least two entries actually share an added + * module and that intersection is non-empty. + * + * Sorted with modules that have no in-package eager import of their own first when that is cheap + * to compute -- it already is here, since a candidate's forward edges are read straight off the + * base graph that produced it, no extra tree read required -- because a leaf module is the safer + * home: adding a symbol to it cannot itself grow anyone else's closure. + */ +export function describeSharedGrowthHomes( + growths: readonly GrowthForAggregation[], + repoRoot: string, +): string | null { + const entriesByAddedModule = groupGrowthsByAddedModule(growths); + const { homePackage, homeGraph } = sharedGrowthHomeCandidates(entriesByAddedModule, repoRoot); + if (homePackage.size === 0) return null; + + const sorted = sortHomesLeavesFirst(homePackage, homeGraph, repoRoot); + return formatSharedGrowthHomes(sorted, repoRoot); +} From 343fca181eda3b90d21b795e23c8b2a5dff0acd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 10 Sep 2026 19:00:36 +0200 Subject: [PATCH 3/3] chore(gates): aggregate only net growth and keep shared homes per added module The cross-entry shared-homes note took every entry with a newly evaluated module, which is not the condition the per-entry rule applies: a closure that swaps one module for another, or shrinks while adding one, has added modules and no growth. `classifyGrowth` passes it, so the aggregate must too -- entries now carry their head closure size and the grouping keeps only the ones whose closure actually grew. Candidate homes are no longer unioned across added modules. Each added module shared by two or more grown entries gets its own block naming those entries with how much each grew and the merge-base modules exactly those entries evaluate, so the label no longer claims a home is common to every failing entry when two independent groups are in play. --- .../__tests__/eager-closure-budgets.test.ts | 112 +++++++++- scripts/__tests__/eager-closure-budgets.ts | 211 +++++++++++------- 2 files changed, 229 insertions(+), 94 deletions(-) diff --git a/scripts/__tests__/eager-closure-budgets.test.ts b/scripts/__tests__/eager-closure-budgets.test.ts index e858ee050d..df93899c9b 100644 --- a/scripts/__tests__/eager-closure-budgets.test.ts +++ b/scripts/__tests__/eager-closure-budgets.test.ts @@ -239,8 +239,8 @@ test('growth shared by two entries names the merge-base modules they already hav const shared = describeSharedGrowthHomes( [ - { id: 'entry-a.ts', added: [addedFile], baseGraph: graphA }, - { id: 'entry-b.ts', added: [addedFile], baseGraph: graphB }, + { id: 'entry-a.ts', added: [addedFile], baseGraph: graphA, headClosureSize: graphA.size + 1 }, + { id: 'entry-b.ts', added: [addedFile], baseGraph: graphB, headClosureSize: graphB.size + 1 }, ], '/repo', ); @@ -257,13 +257,104 @@ test('growth that no other entry shares produces no shared-homes note', () => { const graphA = new Map([['/repo/entry-a.ts', null]]); expect( describeSharedGrowthHomes( - [{ id: 'entry-a.ts', added: ['/repo/new.ts'], baseGraph: graphA }], + [{ id: 'entry-a.ts', added: ['/repo/new.ts'], baseGraph: graphA, headClosureSize: 2 }], '/repo', ), 'nothing to aggregate when only one entry grew by this module', ).toBeNull(); }); +test('an equal-size replacement is not growth, so the aggregate stays silent', () => { + // #2471's review: the aggregate used to take every entry with a newly evaluated module, which is + // not the condition the per-entry rule applies. Both entries below swapped one module for + // another -- same closure size, one module the merge-base did not evaluate, and the SAME one, so + // the grouping would fire. `classifyGrowth` passes both, so this must print nothing at all. + const addedFile = '/repo/packages/demo/src/new-thing.ts'; + const sharedHome = '/repo/packages/demo/src/shared-home.ts'; + const graphA = new Map([ + ['/repo/entry-a.ts', null], + [sharedHome, '/repo/entry-a.ts'], + ['/repo/packages/demo/src/dropped-by-a.ts', '/repo/entry-a.ts'], + ]); + const graphB = new Map([ + ['/repo/entry-b.ts', null], + [sharedHome, '/repo/entry-b.ts'], + ['/repo/packages/demo/src/dropped-by-b.ts', '/repo/entry-b.ts'], + ]); + + expect(classifyGrowth('entry-a.ts', graphA.size, graphA.size)).toBeNull(); + expect(classifyGrowth('entry-b.ts', graphB.size, graphB.size)).toBeNull(); + expect( + describeSharedGrowthHomes( + [ + { id: 'entry-a.ts', added: [addedFile], baseGraph: graphA, headClosureSize: graphA.size }, + { id: 'entry-b.ts', added: [addedFile], baseGraph: graphB, headClosureSize: graphB.size }, + ], + '/repo', + ), + 'a swap at equal size is no violation: the aggregate may not fail where the rule passes', + ).toBeNull(); + expect( + describeSharedGrowthHomes( + [ + { id: 'entry-a.ts', added: [addedFile], baseGraph: graphA, headClosureSize: graphA.size }, + { + id: 'entry-b.ts', + added: [addedFile], + baseGraph: graphB, + headClosureSize: graphB.size - 1, + }, + ], + '/repo', + ), + 'a closure that shrank while adding a module is likewise not growth', + ).toBeNull(); +}); + +test('disjoint growth groups keep their own entries and homes in separate blocks', () => { + // #2471's review: candidate homes from separate added-module groups were unioned into one list + // labelled as common to every failing entry, which is false as soon as two groups exist. Both + // added modules live in the same package here, so only the grouping -- not the package scope -- + // can keep them apart: new-x.ts grew a1/a2, new-y.ts grew b1/b2, and no closure is shared. + const addedX = '/repo/packages/demo/src/new-x.ts'; + const addedY = '/repo/packages/demo/src/new-y.ts'; + const homeX = '/repo/packages/demo/src/home-x.ts'; + const homeY = '/repo/packages/demo/src/home-y.ts'; + const growth = (id: string, added: string, home: string) => { + const entryPath = `/repo/${id}`; + const baseGraph = new Map([ + [entryPath, null], + [home, entryPath], + ]); + return { id, added: [added], baseGraph, headClosureSize: baseGraph.size + 1 }; + }; + + const shared = describeSharedGrowthHomes( + [ + growth('entry-a1.ts', addedX, homeX), + growth('entry-a2.ts', addedX, homeX), + growth('entry-b1.ts', addedY, homeY), + growth('entry-b2.ts', addedY, homeY), + ], + '/repo', + ); + + const blocks = (shared ?? '').split('\n\n'); + expect(blocks, 'one block per added module, never one merged list').toHaveLength(2); + const blockX = blocks.find((block) => block.startsWith('packages/demo/src/new-x.ts')) ?? ''; + const blockY = blocks.find((block) => block.startsWith('packages/demo/src/new-y.ts')) ?? ''; + + expect(blockX, 'each entry is named with how much it grew').toContain('entry-a1.ts (+1)'); + expect(blockX).toContain('entry-a2.ts (+1)'); + expect(blockX).toContain('packages/demo/src/home-x.ts'); + expect(blockX, 'b1/b2 did not grow by new-x.ts').not.toContain('entry-b1.ts'); + expect(blockX, 'no entry that grew by new-x.ts evaluates home-y.ts').not.toContain('home-y.ts'); + expect(blockY).toContain('entry-b1.ts (+1)'); + expect(blockY).toContain('packages/demo/src/home-y.ts'); + expect(blockY, 'a1/a2 did not grow by new-y.ts').not.toContain('entry-a1.ts'); + expect(blockY, 'no entry that grew by new-y.ts evaluates home-x.ts').not.toContain('home-x.ts'); +}); + /** * A committed fixture repository: one workspace package with a manifest export target, a * top-level façade, a nested façade, and a test source. Everything here is TRACKED, so anything a @@ -516,11 +607,16 @@ test('entries that grow by the same new module report shared merge-base homes on // #2423's review: when several entries grow because they all reached the same new module, the // per-entry test above already fails once per entry -- this adds ONE more diagnostic naming the // modules they already have in common, instead of leaving a reader to notice the repetition - // across several separate failures by hand. It can only fail alongside at least two already-red - // entries above, so it never changes the gate's own pass/fail verdict. - const growths = carriedGrowth - .filter((growth) => growth.added.length > 0) - .map((growth) => ({ id: growth.entry.id, added: growth.added, baseGraph: growth.baseGraph })); + // across several separate failures by hand. Every carried entry is handed over with its head + // closure size, and the aggregation itself keeps only the ones the no-growth rule fails + // (#2471 review) -- so it can only fail alongside at least two already-red entries above and + // never changes the gate's own pass/fail verdict. + const growths = carriedGrowth.map((growth) => ({ + id: growth.entry.id, + added: growth.added, + baseGraph: growth.baseGraph, + headClosureSize: growth.graph.size, + })); const shared = describeSharedGrowthHomes(growths, repoRoot); expect( shared, diff --git a/scripts/__tests__/eager-closure-budgets.ts b/scripts/__tests__/eager-closure-budgets.ts index 2d12abe58e..337a4b6e5a 100644 --- a/scripts/__tests__/eager-closure-budgets.ts +++ b/scripts/__tests__/eager-closure-budgets.ts @@ -492,11 +492,41 @@ export type GrowthForAggregation = { added: readonly string[]; /** This entry's merge-base closure graph -- gives both membership and forward edges. */ baseGraph: ReadonlyMap; + /** + * This entry's head closure size, so the aggregation can apply the same condition + * `classifyGrowth` does: an entry only grew when this exceeds `baseGraph.size`. A closure that + * swapped one module for another, or shrank while adding one, has newly evaluated modules and + * no growth -- `classifyGrowth` passes it, so nothing here may report it (#2471 review). + */ + headClosureSize: number; }; -/** How many candidate homes `describeSharedGrowthHomes` names before it just counts the rest. */ +/** How many candidate homes one added-module block names before it just counts the rest. */ const MAX_SHARED_HOMES = 30; +/** How many added-module blocks the whole note prints before it just counts the rest. */ +const MAX_SHARED_GROUPS = 5; + +/** How many grown entries one block names before it just counts the rest. */ +const MAX_GROUP_ENTRIES = 8; + +/** + * One added module with the entries that grew by it and the merge-base modules exactly those + * entries share. Groups stay separate all the way to the message: a home common to one group's + * entries says nothing about another group's, so unioning them would label modules as common to + * entries that never evaluate them (#2471 review). + */ +type SharedGrowthGroup = { + addedModule: string; + entries: readonly GrowthForAggregation[]; + homes: readonly string[]; +}; + +/** How many modules an entry evaluates beyond its merge-base closure; `<= 0` is not growth. */ +function netGrowth(growth: GrowthForAggregation): number { + return growth.headClosureSize - growth.baseGraph.size; +} + /** * The workspace package (or the root `src/` mechanics tree) a repo-relative path lives under -- * the scope `describeSharedGrowthHomes` searches for an existing home, since a candidate outside @@ -520,67 +550,24 @@ function hasNoInPackageChild( return true; } -/** Every entry that grew, indexed by each of its added modules -- one entry may appear under several. */ -function groupGrowthsByAddedModule( +/** + * Every entry that actually GREW, indexed by each of its added modules -- one entry may appear + * under several. Entries whose head closure is no larger than their merge-base one are dropped + * here, which is what keeps the aggregation from reporting a swap the per-entry rule passes. + */ +function growthsByAddedModule( growths: readonly GrowthForAggregation[], ): Map { - const entriesByAddedModule = new Map(); + const byAddedModule = new Map(); for (const growth of growths) { + if (netGrowth(growth) <= 0) continue; for (const added of growth.added) { - const group = entriesByAddedModule.get(added); + const group = byAddedModule.get(added); if (group) group.push(growth); - else entriesByAddedModule.set(added, [growth]); + else byAddedModule.set(added, [growth]); } } - return entriesByAddedModule; -} - -/** - * One added-module group's contribution to the candidate homes: when at least two entries grew - * by `added`, every merge-base module the first entry evaluates that is (a) in the added - * module's own package, (b) not already claimed as a home for a different added module, and (c) - * also evaluated by every other entry in the group, is recorded as a home -- mutating - * `homePackage` and `homeGraph` in place. - */ -function collectSharedHomesForGroup( - added: string, - group: readonly GrowthForAggregation[], - repoRoot: string, - homePackage: Map, - homeGraph: Map>, -): void { - if (group.length < 2) return; - const [first, ...rest] = group; - if (!first) return; - const pkg = packageOf(path.relative(repoRoot, added)); - for (const candidate of first.baseGraph.keys()) { - if (homePackage.has(candidate)) continue; - if (packageOf(path.relative(repoRoot, candidate)) !== pkg) continue; - if (!rest.every((other) => other.baseGraph.has(candidate))) continue; - homePackage.set(candidate, pkg); - homeGraph.set(candidate, first.baseGraph); - } -} - -/** - * For each added module shared by two or more entries, the modules every one of those entries - * already evaluates at the merge-base, scoped to the added module's own package -- the candidate - * homes a shared symbol could plausibly move into. A module already claimed as a home for one - * added module is not reconsidered for another. - */ -function sharedGrowthHomeCandidates( - entriesByAddedModule: ReadonlyMap, - repoRoot: string, -): { - homePackage: Map; - homeGraph: Map>; -} { - const homePackage = new Map(); - const homeGraph = new Map>(); - for (const [added, group] of entriesByAddedModule) { - collectSharedHomesForGroup(added, group, repoRoot, homePackage, homeGraph); - } - return { homePackage, homeGraph }; + return byAddedModule; } /** @@ -589,34 +576,80 @@ function sharedGrowthHomeCandidates( * closure -- then alphabetically by repo-relative path. */ function sortHomesLeavesFirst( - homePackage: ReadonlyMap, - homeGraph: ReadonlyMap>, + homes: readonly string[], + pkg: string, + graph: ReadonlyMap, repoRoot: string, ): string[] { - const isLeaf = (candidate: string): boolean => - hasNoInPackageChild( - candidate, - homePackage.get(candidate) as string, - homeGraph.get(candidate) as ReadonlyMap, - repoRoot, - ); - return [...homePackage.keys()].sort((left, right) => { - const leftLeaf = isLeaf(left); - const rightLeaf = isLeaf(right); + return [...homes].sort((left, right) => { + const leftLeaf = hasNoInPackageChild(left, pkg, graph, repoRoot); + const rightLeaf = hasNoInPackageChild(right, pkg, graph, repoRoot); if (leftLeaf !== rightLeaf) return leftLeaf ? -1 : 1; return path.relative(repoRoot, left).localeCompare(path.relative(repoRoot, right)); }); } -/** Renders the sorted candidate homes as the bounded, capped message block. */ -function formatSharedGrowthHomes(sortedHomes: readonly string[], repoRoot: string): string { - const shown = sortedHomes.slice(0, MAX_SHARED_HOMES); - const hidden = sortedHomes.length - shown.length; - const lines = shown.map((file) => ` ${path.relative(repoRoot, file)}`); +/** + * The merge-base modules every entry in `group` already evaluates, scoped to the added module's + * own package -- the candidate homes a shared symbol could plausibly move into, for this group + * only. + */ +function sharedHomesForGroup( + addedModule: string, + group: readonly GrowthForAggregation[], + repoRoot: string, +): string[] { + const [first, ...rest] = group; + if (!first) return []; + const pkg = packageOf(path.relative(repoRoot, addedModule)); + const homes = [...first.baseGraph.keys()].filter( + (candidate) => + packageOf(path.relative(repoRoot, candidate)) === pkg && + rest.every((other) => other.baseGraph.has(candidate)), + ); + return sortHomesLeavesFirst(homes, pkg, first.baseGraph, repoRoot); +} + +/** Every added module two or more grown entries share and that has at least one candidate home. */ +function sharedGrowthGroups( + growths: readonly GrowthForAggregation[], + repoRoot: string, +): SharedGrowthGroup[] { + const groups: SharedGrowthGroup[] = []; + for (const [addedModule, entries] of growthsByAddedModule(growths)) { + if (entries.length < 2) continue; + const homes = sharedHomesForGroup(addedModule, entries, repoRoot); + if (homes.length > 0) groups.push({ addedModule, entries, homes }); + } + return groups; +} + +/** The grown entries of one group with how much each grew, bounded to `MAX_GROUP_ENTRIES`. */ +function formatGrownEntries(entries: readonly GrowthForAggregation[]): string { + const shown = entries + .slice(0, MAX_GROUP_ENTRIES) + .map((entry) => `${entry.id} (+${netGrowth(entry)})`); + const hidden = entries.length - shown.length; + const more = hidden > 0 ? `, and ${hidden} more entry(ies)` : ''; + return `${shown.join(', ')}${more}`; +} + +/** One group's candidate homes, one per indented line, bounded to `MAX_SHARED_HOMES`. */ +function formatCandidateHomes(homes: readonly string[], repoRoot: string): string { + const shown = homes.slice(0, MAX_SHARED_HOMES); + const hidden = homes.length - shown.length; const more = hidden > 0 ? `\n (+${hidden} more)` : ''; + return `${shown.map((file) => ` ${path.relative(repoRoot, file)}`).join('\n')}${more}`; +} + +/** One added module's block: what grew by it, and the homes those same entries already evaluate. */ +function formatSharedGrowthGroup(group: SharedGrowthGroup, repoRoot: string): string { + const added = path.relative(repoRoot, group.addedModule); return ( - 'Modules every failing entry already evaluates (possible homes for a shared symbol; not a ' + - `statement that any of them is the right owner):\n${lines.join('\n')}${more}` + `${added} -- newly evaluated by ${formatGrownEntries(group.entries)}\n` + + `Entries that grew by ${added} already evaluate these modules at the merge-base (possible ` + + `homes for a shared symbol; not a statement of ownership):\n` + + formatCandidateHomes(group.homes, repoRoot) ); } @@ -632,22 +665,28 @@ function formatSharedGrowthHomes(sortedHomes: readonly string[], repoRoot: strin * a module already imported eagerly wherever it was needed; the fix was moving the constant * there. Nothing in any one entry's own message could show that shape, because each entry's * diagnostic only ever describes that one entry's own closure. This runs once, after every entry - * has been evaluated, and is silent (`null`) unless at least two entries actually share an added - * module and that intersection is non-empty. + * has been evaluated, and is silent (`null`) unless at least two entries that actually GREW share + * an added module and their merge-base intersection is non-empty. + * + * One block per added module (#2471 review). Two properties the shape has to keep: an entry whose + * closure did not grow contributes nothing however many modules are new to it, and a home is only + * ever printed under the added module whose grown entries all evaluate it -- never unioned across + * groups and labelled as common to every failing entry. * - * Sorted with modules that have no in-package eager import of their own first when that is cheap - * to compute -- it already is here, since a candidate's forward edges are read straight off the - * base graph that produced it, no extra tree read required -- because a leaf module is the safer - * home: adding a symbol to it cannot itself grow anyone else's closure. + * Homes are sorted with modules that have no in-package eager import of their own first -- it is + * cheap here, since a candidate's forward edges are read straight off the base graph that + * produced it, no extra tree read required -- because a leaf module is the safer home: adding a + * symbol to it cannot itself grow anyone else's closure. */ export function describeSharedGrowthHomes( growths: readonly GrowthForAggregation[], repoRoot: string, ): string | null { - const entriesByAddedModule = groupGrowthsByAddedModule(growths); - const { homePackage, homeGraph } = sharedGrowthHomeCandidates(entriesByAddedModule, repoRoot); - if (homePackage.size === 0) return null; - - const sorted = sortHomesLeavesFirst(homePackage, homeGraph, repoRoot); - return formatSharedGrowthHomes(sorted, repoRoot); + const groups = sharedGrowthGroups(growths, repoRoot); + if (groups.length === 0) return null; + const shown = groups.slice(0, MAX_SHARED_GROUPS); + const hidden = groups.length - shown.length; + const blocks = shown.map((group) => formatSharedGrowthGroup(group, repoRoot)); + if (hidden > 0) blocks.push(`(+${hidden} more added module(s) shared by two or more entries)`); + return blocks.join('\n\n'); }