diff --git a/scripts/__tests__/committed-source-tree.ts b/scripts/__tests__/committed-source-tree.ts index 72190ceb3..c7742ac88 100644 --- a/scripts/__tests__/committed-source-tree.ts +++ b/scripts/__tests__/committed-source-tree.ts @@ -32,6 +32,16 @@ export function mergeBaseWithMain(repoRoot: string): string { } } +/** + * The commit under measurement. Compared against `mergeBaseWithMain`, this answers whether the + * head carries work of its own: they are equal exactly when `main` already carries this commit -- + * a push to `main`, or a branch that has not committed anything yet -- and then no entry is + * first-introduced, whatever the tree contains. + */ +export function headCommit(repoRoot: string): string { + return git(repoRoot, ['rev-parse', 'HEAD']).toString('utf8').trim(); +} + /** Files renamed since `base`, current path -> path at `base`, so a rename is not a new entry. */ export function renamedSince(repoRoot: string, base: string): ReadonlyMap { const renamed = new Map(); diff --git a/scripts/__tests__/eager-closure-budgets.test.ts b/scripts/__tests__/eager-closure-budgets.test.ts index dbe97275a..b686507e5 100644 --- a/scripts/__tests__/eager-closure-budgets.test.ts +++ b/scripts/__tests__/eager-closure-budgets.test.ts @@ -6,6 +6,7 @@ import path from 'node:path'; import { eagerClosureGraphOf } from '../../src/__tests__/eager-import-closure.fixtures.ts'; import { createCommittedSourceTree, + headCommit, mergeBaseWithMain, renamedSince, } from './committed-source-tree.ts'; @@ -23,6 +24,7 @@ import { NEW_ENTRY_CEILINGS, PLATFORM_FACADE_CLOSURE, PLATFORM_IMPLEMENTATION_PATTERNS, + staleApprovalRows, } from './eager-closure-budgets.ts'; /** @@ -77,6 +79,32 @@ test('a first-introduced entry fits its category ceiling or carries an approval' expect(classifyNewEntry('x.ts', 'vocabulary-facade', 5, true)).toBeNull(); }); +test('a stale approval is reported on a branch and deferred where no row is readable', () => { + // The hole this closes: on a commit `main` already carries, the merge-base IS the head, so the + // introduced set is empty and every row reads as stale however live it is. That is the shape of + // the approving PR's own merge commit, which is why #2329 turned `main` red the moment it + // landed. Both directions are pinned here: a row that has really died is still reported on the + // branch that could have read it. + const ceiling = NEW_ENTRY_CEILINGS['domain-facade']; + const overCeiling = new Map([ + ['new.ts', { category: 'domain-facade' as const, closureSize: ceiling + 1 }], + ]); + const underCeiling = new Map([ + ['new.ts', { category: 'domain-facade' as const, closureSize: ceiling }], + ]); + expect(staleApprovalRows(['new.ts'], overCeiling, false), 'live: read by the ceiling').toEqual( + [], + ); + expect(staleApprovalRows(['new.ts'], underCeiling, false), 'stale: now fits').toEqual(['new.ts']); + expect(staleApprovalRows(['gone.ts'], overCeiling, false), 'stale: not introduced').toEqual([ + 'gone.ts', + ]); + expect( + staleApprovalRows(['gone.ts'], new Map(), true), + 'the merge-base is the head: nothing is first-introduced, so no row can be judged', + ).toEqual([]); +}); + test('the category is derived from the path, never hand-listed', () => { expect(entryCategoryOf('packages/platform-vega/src/index.ts')).toBe('platform-facade'); expect(entryCategoryOf('packages/contracts/src/facades/device.ts')).toBe('vocabulary-facade'); @@ -408,12 +436,22 @@ test.for(introduced)( test('no APPROVED_OVER_CEILING row is stale', () => { // Only a first-introduced entry consults a ceiling. Once the merge-base carries the entry, the // no-growth rule governs it and nothing reads the row again, so a carried entry's row is stale - // for the same reason a shrunk one is: it can no longer change any verdict. - const introducedById = new Map(introduced.map((entry) => [entry.entryFile, entry])); - const stale = Object.keys(APPROVED_OVER_CEILING).filter((id) => { - const entry = introducedById.get(id); - return !entry || eagerClosureGraphOf(absolute(id)).size <= NEW_ENTRY_CEILINGS[entry.category]; - }); + // for the same reason a shrunk one is: it can no longer change any verdict. Deferred where the + // merge-base is the head itself and no row is readable at all -- see `staleApprovalRows`. + const introducedById = new Map( + introduced.map((entry) => [ + entry.entryFile, + { + category: entry.category, + closureSize: eagerClosureGraphOf(absolute(entry.entryFile)).size, + }, + ]), + ); + const stale = staleApprovalRows( + Object.keys(APPROVED_OVER_CEILING), + introducedById, + mergeBase === headCommit(repoRoot), + ); expect( stale, 'These approvals name an entry that no longer exists, that the merge-base now carries, or ' + diff --git a/scripts/__tests__/eager-closure-budgets.ts b/scripts/__tests__/eager-closure-budgets.ts index 29aeba14e..6f61185b1 100644 --- a/scripts/__tests__/eager-closure-budgets.ts +++ b/scripts/__tests__/eager-closure-budgets.ts @@ -21,7 +21,8 @@ // under it, nothing to write. Over it, one `APPROVED_OVER_CEILING` row naming the issue, the // reason, and an owner; the row records no number, and the merge-base carries the entry from // the next PR on. A row is stale once nothing can read it -- the entry is gone, the merge-base -// now carries it, or its closure fits the ceiling -- and a stale row fails. +// now carries it, or its closure fits the ceiling -- and a stale row fails, EXCEPT where the +// merge-base is the head itself and no row is readable at all (`staleApprovalRows`). // // Independent of size, a façade entry's closure must never reach a concrete platform // implementation (`PLATFORM_IMPLEMENTATION_PATTERNS`) before discovery or binding selects an @@ -126,16 +127,7 @@ export const NEW_ENTRY_CEILINGS: Readonly> = Objec */ export const APPROVED_OVER_CEILING: Readonly< Record -> = Object.freeze({ - 'packages/command-registry/src/planned-operations.ts': { - issue: '#2198', - reason: - 'Flattens the required runtime operations of the remaining batch steps from the registry, ' + - 'so its closure is the registry entry itself plus the operation-name vocabulary; a lighter ' + - 'closure would mean a second copy of the descriptors.', - owner: 'thymikee', - }, -}); +> = Object.freeze({}); /** The category is a function of the path, never a hand-written column. */ export function entryCategoryOf(entryFile: string): EntryCategory { @@ -221,6 +213,32 @@ export function classifyNewEntry( ); } +/** + * The `APPROVED_OVER_CEILING` rows that can no longer change any verdict, so their removal is the + * only thing left to do with them: the entry is gone, the merge-base now carries it, or its + * closure fits the ceiling after all. + * + * The verdict is only readable from a commit that carries work of its own. When the merge-base IS + * the head -- a push to `main`, or any commit `main` already carries -- nothing is + * first-introduced by construction, so EVERY row reads as stale whatever its real state. The + * approving PR's own merge commit is exactly that shape, so judging staleness there made each + * approval a guaranteed red `main` one commit after it landed (#2329, run 34099687663): the row + * is required to merge the PR, and the merge that follows it is the run that calls the row dead. + * Deferring to the next branch loses no enforcement -- a row that outlives its PR is reported + * there, on the first commit whose merge-base could have read it. + */ +export function staleApprovalRows( + approvals: readonly string[], + introduced: ReadonlyMap, + mergeBaseIsHead: boolean, +): string[] { + if (mergeBaseIsHead) return []; + return approvals.filter((id) => { + const entry = introduced.get(id); + return entry === undefined || entry.closureSize <= NEW_ENTRY_CEILINGS[entry.category]; + }); +} + /** * Failure-output caps. A violation has to fit in a terminal to be read: `src/cli.ts` evaluates * 363 modules, and one eagerly-imported platform subtree can pull in hundreds, so both