Skip to content

Commit 81f4666

Browse files
thymikeethymikee
authored
refactor(eager-closure): tolerate a pure split in the no-growth rule (#2469) (#3298)
* refactor(eager-closure): tolerate a pure split in the no-growth rule (#2469) The NO-GROWTH rule counts modules, and a split of a hub module is count growth with no new eager work, which made extracting any module inside a gated closure impossible (#2423 extraction deferred, ADR 0027 measured the descriptor-root split at +11 per surface). Move the rule to closure-growth-rule.ts and tolerate exactly the split shape: the head closure still evaluates every merge-base module (renames canonicalized) and the closure's total top-level-statement weight (wiring excluded) did not grow. Each half alone is refutable; together they admit healthy extraction and refuse a smuggled heavy edge or a dropped module. The hard ADR-0019 checks stay untouched. Planted in both directions: a pure split passes end to end, a split smuggling a module-scope call into a new module fails naming the smuggled weight, and a flat-weight drop fails as not-a-split. First consumer: extract runner-phase-budget.ts and runner-toolchain- probe.ts out of runner-cache-metadata.ts (the #2423 review's ownership ask). All seven affected entries keep containment at flat weight, so the gate passes with zero approval rows. * fix(apple): keep runner-cache.ts's exported surface exactly as before the split createRunnerPhaseBudget reached consumers through runner-xctestrun.ts; the split briefly widened runner-cache.ts to export it too, which fallow's dead-code audit reports as a new unconsumed re-export. * fix(eager-closure): count a source-less re-export as wiring; state the guarantee precisely Review on #3298: - topLevelStatementCount treated `export { a }` (no source, no declaration) as module-scope work, so the re-export half of a split's wiring added false weight and could fail a pure split that re-exports through a local binding. Excluded at the owning predicate; planted in closure-growth-rule.test.ts (mutation: return false for declaration-free named exports re-fails exactly that case). - ADR 0027, its rule-module header, and ADR 0028 said the pair 'refuses new eager work'. It does not: nothing count-based sees a split that replaces a statement with a more expensive top-level call. The texts now state the actual guarantee -- flat statement weight over a preserved closure -- and name the residue and what bounds it. * fix(eager-closure): a tolerated split may only add new files; shrinkage cannot fund an eager edge Maintainer review on #3298: the two-fact tolerance (containment + flat weight) never checked WHERE the added modules came from, so a PR could statically import a heavy merge-base module that sat outside the closure and pay for it by deleting top-level statements elsewhere in the same closure. Both facts still held; real growth passed. The tolerance is now three facts, all read from the graphs plus the merge-base tree: containment, added-module NOVELTY (every head-closure module the base closure did not evaluate is absent from the merge-base tree at its rename-canonicalized path -- checked at the BASE path for a rename's head, so re-naming a closure-external module into the closure is still pre-existing code newly made eager), and flat weight. classifyGrowth requires all three and prints one reason per failed fact. Planted, in the maintainer's exact shape: the split fixture plus a committed pre-existing-heavy.ts pulled in by part-c, weight offset by merging the hub's two remaining declarations -- containment holds, weight flat 3->3, novelty false, verdict names shrinkage-funding. Unit-level pair for the fact itself, and rename-canonicalized novelty pinned with and without the rename map against a colliding tree. Mutation: dropping the novelty conjunct fails exactly the two planted novelty tests and nothing else. ADR 0027 (status, rules, decision, alternatives: the first draft is now a named refuted alternative) and ADR 0028 step 3 state the three facts; the residue paragraph drops the 'true of any closure' claim. * fix(eager-closure): novelty reads deleted-source content, not just git's rename pairs Cubic review on b1cc79e: - closure-growth-rule.ts:94 accepted a REWRITTEN move of a pre-existing external module. -M rename detection is a similarity heuristic; a move that changes the path and churns formatting reports as delete + add, so an absent destination proved nothing. closureGrowthEvidence now also receives the statement texts of every production source deleted since the merge-base (deletedSourcesSince runs --diff-filter=D WITHOUT -M; texts come from the base tree through statementTextsOf, normalized so comments/layout/export-keyword churn is invisible). An added module carrying two or more identical statements of a deleted source is that source's code -- not novel. Two, because a single shared line is coincidence at constant-declaration scale. - ADR 0027 said rename canonicalization covered 'a rename'; the gate checks the CANONICALIZED BASE path, which preserves modules already in the base closure and reads a moved-in external module as newly eager. Status, rules, decision fact 2, the rule-module header, and the verdict text now say exactly that; the second draft (path-novelty alone) joins the refuted alternatives, pinned by the planted rewritten-move test -- the evasion itself (-M reports D, not R) is asserted, so the test cannot silently degrade into rename-handling coverage. Mutation-verified: dropping the content comparison fails exactly the rewritten-move test; checking the head path instead of the canonicalized one fails exactly the moved-in test and the original rename test. * docs(eager-closure): price the fifth git process and state the gate's actual check Two review notes on PR #3298: - The gate's cost comment still promised four merge-base git processes; the novelty fact's fifth is the -M-less delete diff (deleted-source CONTENT comes from the base tree's existing cat-file batch). Comment now enumerates all five. - ADR 0027 stated the deleted-source guarantee absolutely. The gate checks movedFromDeletedSource: two identical normalized statements from one deleted source is the move signal, one is coincidence, and a rewritten transplant evades a textual probe. The ADR, the rule header, and the refuted-directions list now state exactly that boundary. * docs(eager-closure): state deletedSourcesSince's real rename semantics Approval-round note on PR #3298: plain `git diff` pairs renames by DEFAULT, and --diff-filter=D drops the paired ones, so the docstring's "WITHOUT -M, so a rename the similarity detector would pair stays visible" had it backwards -- the list covers only deletions pairing did NOT absorb, and paired renames are answered by renamedSince. Captured the maintainer's companion constraint too: dropping the default pairing (--no-renames) would put a re-homed hub in the deleted set and refuse the very splits the tolerance exists for. Fixed at the three siblings carrying the same claim: the function docstring, the rule's deletedSourceTexts field doc, and the gate's cost comment. * docs(eager-closure): state the deleted-source probe's threshold and evasion in deletedSourcesSince PR #3298 review: the sentence "such a move stays non-novel" was unqualified where the sibling docs are precise. The probe brands a move only on two or more identical normalized statements with ONE deleted source; one shared statement is tolerated as coincidence, and a statement-by-statement rewrite evades a textual match. Containment stated as the structural refusal this probe reinforces. * test(eager-closure): request rename pairing explicitly in the deleted-source probe PR #3298 re-review: the probe leaned on git's DEFAULT pairing, so a diff.renames=false gitconfig would have listed every re-homed split hub as deleted source and refused honest splits. --find-renames now states the requirement in the argv; verified in a scratch repo that the pairing survives the hostile config while the old argv leaks the hub under it. Docstrings follow the argv.
1 parent fb70e27 commit 81f4666

19 files changed

Lines changed: 1552 additions & 491 deletions

‎docs/adr/0027-descriptor-root-vs-eager-closure-budget.md‎

Lines changed: 103 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,18 @@
1-
# ADR 0027: Descriptor Root Size vs Eager-Closure Budget (Proposed)
1+
# ADR 0027: Descriptor Root Size vs Eager-Closure Budget
22

33
## Status
44

5-
Proposed (2026-09-23). This ADR **decides nothing**. It records a conflict between two invariants
6-
that are both enforced and both correct, and states the one question a repo owner must answer before
7-
either invariant can be relied on for module placement. Until then, follow both: do not split the
8-
descriptor root, and do not add an approval path to the budget gate to let the split through.
5+
Accepted (2026-10-07, #2469). The conflict below is resolved in favor of allowing
6+
**byte-neutral (statement-neutral) re-homing**: the eager-closure budget's NO-GROWTH rule now
7+
tolerates exactly the split shape — the head closure still evaluates every merge-base module,
8+
every module it newly evaluates is new to the merge-base tree and to its deleted sources (a
9+
`git mv` inside the base closure stays the same module through rename canonicalization, which
10+
preserves modules already in the closure and is no pass for a pre-existing external module
11+
moved in), and the closure's total module-scope statement count does not grow. The rule lives
12+
in `scripts/__tests__/closure-growth-rule.ts`, planted in every failing direction by
13+
`scripts/__tests__/closure-growth-rule.test.ts`. The hard ADR-0019 checks (façade exactness, no
14+
platform implementation before binding) are untouched: the tolerance relaxes the module-count
15+
proxy, never the loading property.
916

1017
## The conflict
1118

@@ -26,16 +33,22 @@ Splitting a hub module necessarily adds static value edges beneath it, so every
2633
reaches it grows by the number of new modules. The two rules have no shared approval path: one says
2734
the file must be divided, the other says the division must not be visible to any importer.
2835

29-
## Rules at a glance, while unresolved
30-
31-
- Do not split `registry.ts`. The budget gate is the binding constraint and has no waiver.
32-
- Do not add an approved-growth row, a baseline entry, or a suppression to pass it. That is the
33-
allowlist `AGENTS.md` forbids.
36+
## Rules at a glance, resolved
37+
38+
- A hub split is allowed when it is statement-neutral: every merge-base module stays in every
39+
affected closure, every newly evaluated module is new to the merge-base tree and to its
40+
deleted sources (rename canonicalization preserves modules already in the base closure — it
41+
does not make a moved-in external module novel), and total module-scope statements do not
42+
grow. The gate enforces this; an unexplained count growth still fails with no approval path,
43+
as before.
44+
- Do not add an approved-growth row, a baseline entry, or a suppression to pass the gate. That is
45+
the allowlist `AGENTS.md` forbids; the split tolerance is a rule, not a waiver — it decides from
46+
the two graphs, and nothing is hand-entered per split.
3447
- Do not make registry construction asynchronous to dodge the walker. ADR 0008's synchronous root is
3548
load-bearing for compile-time totality.
36-
- Closure-neutral edits inside the descriptor root are still welcome: relocating guards beside the
37-
union they constrain, correcting stale claims, and moving non-eager content out are fine so long as
38-
every measured entry surface is unchanged.
49+
- The split tolerance guarantees flat statement weight over a preserved closure; it does not
50+
license adding eager *edges* under a façade — the head must still evaluate every merge-base
51+
module, and the ADR-0019 checks stay hard.
3952

4053
## Measured
4154

@@ -69,23 +82,84 @@ The cost is genuinely a *count*, not weight: the same bytes are evaluated either
6982
makes the conflict worth deciding rather than absorbing silently, because the gate's proxy and the
7083
debt rule's proxy disagree at exactly this shape.
7184

72-
## Decision required
73-
74-
May a byte-neutral re-homing of a hub module's contents into new modules raise the eager module count
75-
of the entry surfaces that reach it — or must a hub module's contents stay inside the file its
76-
consumers already evaluate?
77-
78-
Answering "the count may rise" needs a bounded, auditable growth allowance on an invariant that
79-
currently refuses one. Answering "contents stay put" needs `AGENTS.md`'s 1,000-line rule to name the
80-
exception for a hub whose consumers evaluate it as one unit, so the size is treated as accepted and
81-
measured rather than as unaddressed debt. Either answer is fine; leaving it unstated means the next
82-
agent at this file discovers the conflict from a red gate and picks one silently.
85+
## Decision
86+
87+
May a byte-neutral re-homing of a hub module's contents into new modules raise the eager module
88+
count of the entry surfaces that reach it? **Yes — exactly when it is a split**, decided by three
89+
facts computed from the two closure graphs and the merge-base tree, not by an entered number:
90+
91+
1. **Containment:** the head closure evaluates every module the merge-base closure evaluated
92+
(renames canonicalized), so nothing was dropped or swapped.
93+
2. **Added-module novelty:** every module the head closure evaluates that the base closure did
94+
not is absent from the merge-base tree at its rename-canonicalized path AND carries no code
95+
from a source deleted since the merge-base — matched when the added module shares **two or
96+
more identical normalized top-level statements** with one deleted source; a single shared
97+
statement is treated as coincidence, because short constants (a schema version, a default
98+
timeout) repeat across the repo and branding every reuse a move would poison new files).
99+
A module the tree already had, sitting outside the closure, becoming eager — under its own
100+
name, a renamed one, or a rewritten move that evades git's `-M` similarity pairing — is a
101+
new edge, not re-homed code; and without this fact, weight deleted elsewhere in the closure
102+
would fund it. Rename canonicalization preserves modules ALREADY IN the base closure; it is
103+
not a pass for an external module moved in, which fails the check at the base path the
104+
merge-base necessarily still has.
105+
3. **Flat weight:** the closure's total top-level statement count — wiring lines excluded, so
106+
re-homing is invisible to it — does not exceed the merge-base closure's.
107+
108+
This is issue #2469's candidate 4 (structure ∧ weight), with the structure half implemented as
109+
containment + novelty rather than candidate 2's importer-set shape. Any subset is refutable —
110+
containment without novelty accepts pulling a pre-existing module in at flat weight, novelty
111+
without containment accepts dropping a merge-base module, weight without the other two accepts
112+
unrelated shrinkage paying for a smuggled edge — and the three together admit healthy extraction
113+
while refusing any growth in the closure's module-scope statement weight. Statements were chosen
114+
over bytes because formatting and comment churn move bytes while statements track the thing being
115+
preserved: module-scope declarations and calls. The guarantee is exactly **newly evaluated
116+
modules are new code — new paths that share at most one coincidental statement with any source
117+
deleted since the merge-base (`movedFromDeletedSource`'s deliberate two-statement move-signal
118+
threshold) — and total statement weight is flat over a preserved closure**, not
119+
equality of eager behavior: a split that REPLACES an existing statement with a more expensive
120+
top-level call inside an existing file moves no count and no weight, so it passes — nothing
121+
count-based, in modules or statements, can see a like-for-like replacement. That residue is
122+
bounded by what the tolerance cannot hide: every module the head newly evaluates is a path the
123+
merge-base tree did not have AND is not a verbatim transplant — the move probe above brands any
124+
added module sharing two or more statements with one deleted source as pre-existing code.
125+
The probe is deliberately conservative and textual:
126+
code REWRITTEN or reformulated during a move does not match it and would pass as novel, so what
127+
is verified is "no verbatim transplant of a deleted source," not "no repurposed logic" (the
128+
weight fact is what still refuses a transplant made heavier). What keeps the residue small is
129+
everything around the probe: the hard ADR-0019 checks (façade exactness,
130+
no platform implementation before binding) stay count- and pattern-based under it, and the
131+
added-module listing every failure prints is what a reviewer reads. The planted tests in
132+
`scripts/__tests__/closure-growth-rule.test.ts` pin every failing direction.
133+
134+
Maintenance cost of an ordinary extraction under this rule: zero configuration edits. No row,
135+
baseline, or number is touched per split — the merge-base comparison recomputes everything. (The
136+
candidate-3 approval-row design instead costs one hand-edited row per split plus its retirement.)
137+
138+
The `AGENTS.md` 1,000-line rule now has no hub exception to name: a statement-neutral split is
139+
shippable, so an oversized hub is addressable debt rather than accepted state.
83140

84141
## Alternatives considered and refuted
85142

86-
- **Approved-growth row mirroring `APPROVED_OVER_CEILING` (issue, reason, owner):** refuted pending the
87-
decision above. It would be auditable, but it is an allowlist on the one invariant deliberately
88-
given no approval path, and `AGENTS.md` forbids adding one to obtain a pass.
143+
- **Approved-growth row mirroring `APPROVED_OVER_CEILING` (issue, reason, owner):** refuted. It
144+
would be auditable, but it is an allowlist on the one invariant deliberately given no approval
145+
path, `AGENTS.md` forbids adding one to obtain a pass, and every split would owe a row plus its
146+
retirement — maintenance the chosen rule does not.
147+
- **Structure alone (#2469 candidate 2):** refuted by the planted smuggle test — a "new" module can
148+
carry a new heavy edge, so containment proves nothing about eager work.
149+
- **Containment + weight without added-module novelty (the rule's first draft on #3298):** refuted
150+
by the planted pre-existing-heavy test — deleting statements elsewhere in the closure funds a
151+
static edge to a merge-base module that sat outside it, which is real growth, not a split.
152+
- **Novelty by merge-base tree path alone (the rule's second draft on #3298):** refuted by the
153+
planted rewritten-move test — a move that changes the path and churns formatting reports to
154+
git as delete + add, so an absent destination is not proof of new code. The deleted sources'
155+
statement texts close it down to a stated threshold: two identical normalized statements from
156+
one deleted source is the move signal, one is tolerated as coincidence (short constants repeat
157+
across the repo), and a transplant rewritten statement-by-statement stays outside a textual
158+
probe — what remains heuristic-free is the LIST of deleted sources (git's own delete listing,
159+
needing no similarity pairing to have been attempted), not the
160+
content match.
161+
- **Weight alone (#2469 candidate 1):** refuted by the planted drop test — a swap can keep the
162+
statement total flat while quietly making the closure evaluate different modules.
89163
- **Leave the array in place:** the status quo, not a resolution — it leaves the size debt unowned and
90164
the conflict undocumented.
91165
- **Function-scoped `await import` per family:** passes the gate by making registry construction
@@ -99,5 +173,5 @@ agent at this file discovers the conflict from a red gate and picks one silently
99173

100174
A complete, byte-faithful split of the descriptor array exists on
101175
`refactor/collocation-registry-family-split` (`40bf719a8b`, split commit `ad3aaa520e`) and is pushed.
102-
It is kept rather than deleted so that answering the question above does not require redoing the
103-
mechanical work; 80 descriptors, 79 byte-identical including comments.
176+
With this decision accepted, that branch is now shippable: its +11 is a statement-neutral split of
177+
exactly the tolerated shape, so it needs no gate change beyond this one.

‎docs/adr/0028-capability-family-cell-vocabulary.md‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,11 @@ family rather than arguing from the shape:
7070
2. Report the change in files-touched for that family against the recorded baseline of the four
7171
recurring eight-package commits.
7272
3. Report eager-closure deltas per entry surface. `scripts/__tests__/eager-closure-budgets.ts` has no
73-
growth-approval path, and [ADR 0027](0027-descriptor-root-vs-eager-closure-budget.md) shows a
74-
byte-neutral re-home can be unshippable here. A row table is a **new eager value**; if it costs
75-
closure slots, it is blocked on the same ground and this ADR should be withdrawn rather than
76-
granted an exception.
73+
growth-approval path; [ADR 0027](0027-descriptor-root-vs-eager-closure-budget.md) tolerates
74+
module-count growth only when the head closure preserves every merge-base module, every newly
75+
evaluated module is new to the merge-base tree (or a rename), and total module-scope weight does
76+
not grow. A row table is a **new eager value**: it passes only by satisfying all three conditions;
77+
otherwise the gate stays red and this ADR should be withdrawn rather than granted an exception.
7778

7879
If the fan-out does not visibly shrink, close this as rejected and leave the two authored lists in
7980
place — a compile-checked duplicate is a legitimate resting state.

‎docs/adr/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
| [0024 Screen Recording — One Coordinator, Two Facts (Proposed)](0024-screen-recording-provable-signal.md) | `record start`/`stop` lifecycle, the export-exists / recorder-stopped split, backend stop-and-collect contract, recovery through the coordinator, the ADR 0019 §5 amendment and the failed-finish cleanup fix |
2929
| [0025 Foldable Apple Panels — Capture the Lit Panel](0025-foldable-apple-panels.md) | iPhone Duo outer/inner panels, CoreDevice display info as the only screen authority, why `simctl`'s implicit display default captures a black panel, derived (never asserted) pose, and why no pose-control command exists |
3030
| [0026 Scroll Clip Authority — Ownership Is the Parent Edge](0026-scroll-clip-authority.md) | iOS scroll indicators and the visible band, why ownership is read from the parent edge instead of an ancestor walk, what may remove a node from an interactive snapshot, and ejection dispositions |
31-
| [0027 Descriptor Root Size vs Eager-Closure Budget (Proposed)](0027-descriptor-root-vs-eager-closure-budget.md) | splitting `packages/command-registry/src/registry.ts`, the ADR-0019 eager-closure module-count budget, and why a byte-neutral hub split is currently unshippable |
31+
| [0027 Descriptor Root Size vs Eager-Closure Budget](0027-descriptor-root-vs-eager-closure-budget.md) | accepted: the eager-closure budget tolerates a statement-neutral hub split only (containment + added-module novelty + flat top-level-statement weight) and nothing else; splitting `packages/command-registry/src/registry.ts` is unblocked |
3232
| [0028 Capability-Family Cell Vocabulary — One Runtime Source (Proposed)](0028-capability-family-cell-vocabulary.md) | adding a capability operation family, `UnavailablePlatformRuntimeFacts` / `UNAVAILABLE_CELLS`, `INTERACTOR_OPERATIONS`, and why an eight-package fan-out recurs |
3333
| [0029 Daemon Policy](0029-daemon-policy.md) | `AGENT_DEVICE_DAEMON_POLICY`, confining a daemon's commands, devices, or device shutdown, and where operator rules are enforced for batch/replay steps |
3434
| [0030 Process Lock Exclusion](0030-process-lock-exclusion.md) | process-lock publication/reclaim/release, retained mutation guards, and the single-protocol upgrade boundary |

‎packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import type { ExecOptions } from '@agent-device/host-kit/command';
88
import fs from 'node:fs';
99
import path from 'node:path';
1010
import {
11-
createRunnerPhaseBudget,
1211
requireRunnerBuildSettingsMatchBuildLog,
1312
diffComparableRunnerCacheMetadata,
1413
resolveRunnerBundleBuildSettings,
@@ -18,8 +17,9 @@ import {
1817
resolveRunnerSandboxBuildArgs,
1918
resolveExpectedRunnerCacheMetadata,
2019
resolveRunnerDerivedPath,
21-
memoizedRunnerXcodeVersion,
2220
} from '../runner-cache-metadata.ts';
21+
import { createRunnerPhaseBudget } from '../runner-phase-budget.ts';
22+
import { memoizedRunnerXcodeVersion } from '../runner-toolchain-probe.ts';
2323
import { COLD_TOOLCHAIN_PROBE_TIMEOUT_MS } from '../apple-runner-platform.ts';
2424
import {
2525
appleToolchainProbeResult,

‎packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,8 @@ import { appleRunnerTestHost } from '../test-host.ts';
1111
import { withAppleRunnerProvider } from '../runner-provider.ts';
1212
import { classifyRunnerReportedError, type RunnerCommand } from '../runner-contract.ts';
1313
import { RUNNER_REPLY_LOST_REASON } from '../runner-error-classification.ts';
14-
import {
15-
createRunnerPhaseBudget,
16-
requireRunnerPhaseRemainingMs,
17-
resolveExpectedRunnerCacheMetadata,
18-
} from '../runner-cache-metadata.ts';
14+
import { resolveExpectedRunnerCacheMetadata } from '../runner-cache-metadata.ts';
15+
import { createRunnerPhaseBudget, requireRunnerPhaseRemainingMs } from '../runner-phase-budget.ts';
1916
import { captureDiagnostics } from './runner-session-fixtures.ts';
2017
import {
2118
startFakeRunnerServer,

0 commit comments

Comments
 (0)