From d920f2f014b7f85d10339edf00dcc59401b2629a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 11:12:49 +0000 Subject: [PATCH] fix(gates): stop an eager-closure approval from turning main red on merge `no APPROVED_OVER_CEILING row is stale` reads the introduced-entry set, which is derived from `git merge-base origin/main HEAD`. On a push to main the merge-base IS the head, so nothing is first-introduced and every approval row reads as stale whatever its real state. That is exactly the shape of the approving PR's own merge commit: #2329 added the `packages/command-registry/src/planned-operations.ts` row to merge, and the merge that followed it called the row dead. Coverage has been red on main since (run 34099687663), and every branch cut from main after it inherits the same failure. - `staleApprovalRows` makes the verdict a named rule and defers it when the merge-base is the head, where no row is readable at all. Enforcement is not lost: a row that outlives its PR is still reported on the first branch whose merge-base could have read it, and the rule is pinned in both directions. - The `planned-operations.ts` row goes, which is what the rule asks for now that main carries the entry: its closure (74) is governed by the no-growth rule from here on, not by the domain-facade ceiling. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SfQqXj7JKQVgBA8eg9SMVB --- scripts/__tests__/committed-source-tree.ts | 10 ++++ .../__tests__/eager-closure-budgets.test.ts | 50 ++++++++++++++++--- scripts/__tests__/eager-closure-budgets.ts | 40 +++++++++++---- 3 files changed, 83 insertions(+), 17 deletions(-) diff --git a/scripts/__tests__/committed-source-tree.ts b/scripts/__tests__/committed-source-tree.ts index 72190ceb3a..c7742ac88e 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 dbe97275a8..b686507e5d 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 29aeba14ec..6f61185b14 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