Skip to content

Commit 3f022b0

Browse files
thymikeeclaude
andauthored
fix(gates): stop an eager-closure approval from turning main red on merge (#2375)
`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. Claude-Session: https://claude.ai/code/session_01SfQqXj7JKQVgBA8eg9SMVB Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8299d5b commit 3f022b0

3 files changed

Lines changed: 83 additions & 17 deletions

File tree

scripts/__tests__/committed-source-tree.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,16 @@ export function mergeBaseWithMain(repoRoot: string): string {
3232
}
3333
}
3434

35+
/**
36+
* The commit under measurement. Compared against `mergeBaseWithMain`, this answers whether the
37+
* head carries work of its own: they are equal exactly when `main` already carries this commit --
38+
* a push to `main`, or a branch that has not committed anything yet -- and then no entry is
39+
* first-introduced, whatever the tree contains.
40+
*/
41+
export function headCommit(repoRoot: string): string {
42+
return git(repoRoot, ['rev-parse', 'HEAD']).toString('utf8').trim();
43+
}
44+
3545
/** Files renamed since `base`, current path -> path at `base`, so a rename is not a new entry. */
3646
export function renamedSince(repoRoot: string, base: string): ReadonlyMap<string, string> {
3747
const renamed = new Map<string, string>();

scripts/__tests__/eager-closure-budgets.test.ts

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import path from 'node:path';
66
import { eagerClosureGraphOf } from '../../src/__tests__/eager-import-closure.fixtures.ts';
77
import {
88
createCommittedSourceTree,
9+
headCommit,
910
mergeBaseWithMain,
1011
renamedSince,
1112
} from './committed-source-tree.ts';
@@ -23,6 +24,7 @@ import {
2324
NEW_ENTRY_CEILINGS,
2425
PLATFORM_FACADE_CLOSURE,
2526
PLATFORM_IMPLEMENTATION_PATTERNS,
27+
staleApprovalRows,
2628
} from './eager-closure-budgets.ts';
2729

2830
/**
@@ -77,6 +79,32 @@ test('a first-introduced entry fits its category ceiling or carries an approval'
7779
expect(classifyNewEntry('x.ts', 'vocabulary-facade', 5, true)).toBeNull();
7880
});
7981

82+
test('a stale approval is reported on a branch and deferred where no row is readable', () => {
83+
// The hole this closes: on a commit `main` already carries, the merge-base IS the head, so the
84+
// introduced set is empty and every row reads as stale however live it is. That is the shape of
85+
// the approving PR's own merge commit, which is why #2329 turned `main` red the moment it
86+
// landed. Both directions are pinned here: a row that has really died is still reported on the
87+
// branch that could have read it.
88+
const ceiling = NEW_ENTRY_CEILINGS['domain-facade'];
89+
const overCeiling = new Map([
90+
['new.ts', { category: 'domain-facade' as const, closureSize: ceiling + 1 }],
91+
]);
92+
const underCeiling = new Map([
93+
['new.ts', { category: 'domain-facade' as const, closureSize: ceiling }],
94+
]);
95+
expect(staleApprovalRows(['new.ts'], overCeiling, false), 'live: read by the ceiling').toEqual(
96+
[],
97+
);
98+
expect(staleApprovalRows(['new.ts'], underCeiling, false), 'stale: now fits').toEqual(['new.ts']);
99+
expect(staleApprovalRows(['gone.ts'], overCeiling, false), 'stale: not introduced').toEqual([
100+
'gone.ts',
101+
]);
102+
expect(
103+
staleApprovalRows(['gone.ts'], new Map(), true),
104+
'the merge-base is the head: nothing is first-introduced, so no row can be judged',
105+
).toEqual([]);
106+
});
107+
80108
test('the category is derived from the path, never hand-listed', () => {
81109
expect(entryCategoryOf('packages/platform-vega/src/index.ts')).toBe('platform-facade');
82110
expect(entryCategoryOf('packages/contracts/src/facades/device.ts')).toBe('vocabulary-facade');
@@ -408,12 +436,22 @@ test.for(introduced)(
408436
test('no APPROVED_OVER_CEILING row is stale', () => {
409437
// Only a first-introduced entry consults a ceiling. Once the merge-base carries the entry, the
410438
// no-growth rule governs it and nothing reads the row again, so a carried entry's row is stale
411-
// for the same reason a shrunk one is: it can no longer change any verdict.
412-
const introducedById = new Map(introduced.map((entry) => [entry.entryFile, entry]));
413-
const stale = Object.keys(APPROVED_OVER_CEILING).filter((id) => {
414-
const entry = introducedById.get(id);
415-
return !entry || eagerClosureGraphOf(absolute(id)).size <= NEW_ENTRY_CEILINGS[entry.category];
416-
});
439+
// for the same reason a shrunk one is: it can no longer change any verdict. Deferred where the
440+
// merge-base is the head itself and no row is readable at all -- see `staleApprovalRows`.
441+
const introducedById = new Map(
442+
introduced.map((entry) => [
443+
entry.entryFile,
444+
{
445+
category: entry.category,
446+
closureSize: eagerClosureGraphOf(absolute(entry.entryFile)).size,
447+
},
448+
]),
449+
);
450+
const stale = staleApprovalRows(
451+
Object.keys(APPROVED_OVER_CEILING),
452+
introducedById,
453+
mergeBase === headCommit(repoRoot),
454+
);
417455
expect(
418456
stale,
419457
'These approvals name an entry that no longer exists, that the merge-base now carries, or ' +

scripts/__tests__/eager-closure-budgets.ts

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@
2121
// under it, nothing to write. Over it, one `APPROVED_OVER_CEILING` row naming the issue, the
2222
// reason, and an owner; the row records no number, and the merge-base carries the entry from
2323
// the next PR on. A row is stale once nothing can read it -- the entry is gone, the merge-base
24-
// now carries it, or its closure fits the ceiling -- and a stale row fails.
24+
// now carries it, or its closure fits the ceiling -- and a stale row fails, EXCEPT where the
25+
// merge-base is the head itself and no row is readable at all (`staleApprovalRows`).
2526
//
2627
// Independent of size, a façade entry's closure must never reach a concrete platform
2728
// implementation (`PLATFORM_IMPLEMENTATION_PATTERNS`) before discovery or binding selects an
@@ -126,16 +127,7 @@ export const NEW_ENTRY_CEILINGS: Readonly<Record<EntryCategory, number>> = Objec
126127
*/
127128
export const APPROVED_OVER_CEILING: Readonly<
128129
Record<string, { issue: string; reason: string; owner: string }>
129-
> = Object.freeze({
130-
'packages/command-registry/src/planned-operations.ts': {
131-
issue: '#2198',
132-
reason:
133-
'Flattens the required runtime operations of the remaining batch steps from the registry, ' +
134-
'so its closure is the registry entry itself plus the operation-name vocabulary; a lighter ' +
135-
'closure would mean a second copy of the descriptors.',
136-
owner: 'thymikee',
137-
},
138-
});
130+
> = Object.freeze({});
139131

140132
/** The category is a function of the path, never a hand-written column. */
141133
export function entryCategoryOf(entryFile: string): EntryCategory {
@@ -221,6 +213,32 @@ export function classifyNewEntry(
221213
);
222214
}
223215

216+
/**
217+
* The `APPROVED_OVER_CEILING` rows that can no longer change any verdict, so their removal is the
218+
* only thing left to do with them: the entry is gone, the merge-base now carries it, or its
219+
* closure fits the ceiling after all.
220+
*
221+
* The verdict is only readable from a commit that carries work of its own. When the merge-base IS
222+
* the head -- a push to `main`, or any commit `main` already carries -- nothing is
223+
* first-introduced by construction, so EVERY row reads as stale whatever its real state. The
224+
* approving PR's own merge commit is exactly that shape, so judging staleness there made each
225+
* approval a guaranteed red `main` one commit after it landed (#2329, run 34099687663): the row
226+
* is required to merge the PR, and the merge that follows it is the run that calls the row dead.
227+
* Deferring to the next branch loses no enforcement -- a row that outlives its PR is reported
228+
* there, on the first commit whose merge-base could have read it.
229+
*/
230+
export function staleApprovalRows(
231+
approvals: readonly string[],
232+
introduced: ReadonlyMap<string, { category: EntryCategory; closureSize: number }>,
233+
mergeBaseIsHead: boolean,
234+
): string[] {
235+
if (mergeBaseIsHead) return [];
236+
return approvals.filter((id) => {
237+
const entry = introduced.get(id);
238+
return entry === undefined || entry.closureSize <= NEW_ENTRY_CEILINGS[entry.category];
239+
});
240+
}
241+
224242
/**
225243
* Failure-output caps. A violation has to fit in a terminal to be read: `src/cli.ts` evaluates
226244
* 363 modules, and one eagerly-imported platform subtree can pull in hundreds, so both

0 commit comments

Comments
 (0)