From 283c2fcc90aaf64e9a1a81d19f182a04f6a38391 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 7 Oct 2026 21:36:41 +0200 Subject: [PATCH 1/9] 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. --- ...descriptor-root-vs-eager-closure-budget.md | 78 ++-- .../0028-capability-family-cell-vocabulary.md | 8 +- docs/adr/README.md | 2 +- .../__tests__/runner-cache-metadata.test.ts | 4 +- .../__tests__/runner-recovery-wiring.test.ts | 7 +- .../src/runner/runner-cache-metadata.ts | 344 +----------------- .../platform-apple/src/runner/runner-cache.ts | 7 +- .../src/runner/runner-device-set.ts | 2 +- .../src/runner/runner-phase-budget.ts | 59 +++ .../src/runner/runner-toolchain-probe.ts | 289 +++++++++++++++ .../src/runner/runner-xctestrun.ts | 8 +- scripts/__tests__/closure-growth-rule.test.ts | 238 ++++++++++++ scripts/__tests__/closure-growth-rule.ts | 98 +++++ .../__tests__/eager-closure-budgets.test.ts | 214 +++++++---- scripts/__tests__/eager-closure-budgets.ts | 55 ++- .../__tests__/eager-closure-gate-fixtures.ts | 46 +++ .../eager-import-closure.fixtures.ts | 87 ++++- vitest.config.ts | 1 + 18 files changed, 1061 insertions(+), 486 deletions(-) create mode 100644 packages/platform-apple/src/runner/runner-phase-budget.ts create mode 100644 packages/platform-apple/src/runner/runner-toolchain-probe.ts create mode 100644 scripts/__tests__/closure-growth-rule.test.ts create mode 100644 scripts/__tests__/closure-growth-rule.ts create mode 100644 scripts/__tests__/eager-closure-gate-fixtures.ts diff --git a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md index 53d17f99a3..64c0b31697 100644 --- a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md +++ b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md @@ -1,11 +1,15 @@ -# ADR 0027: Descriptor Root Size vs Eager-Closure Budget (Proposed) +# ADR 0027: Descriptor Root Size vs Eager-Closure Budget ## Status -Proposed (2026-09-23). This ADR **decides nothing**. It records a conflict between two invariants -that are both enforced and both correct, and states the one question a repo owner must answer before -either invariant can be relied on for module placement. Until then, follow both: do not split the -descriptor root, and do not add an approval path to the budget gate to let the split through. +Accepted (2026-10-07, #2469). The conflict below is resolved in favor of allowing +**byte-neutral (statement-neutral) re-homing**: the eager-closure budget's NO-GROWTH rule now +tolerates exactly the split shape — the head closure still evaluates every merge-base module and +the closure's total module-scope statement count does not grow. The rule lives in +`scripts/__tests__/closure-growth-rule.ts`, planted in both directions by +`scripts/__tests__/closure-growth-rule.test.ts`. The hard ADR-0019 checks (façade exactness, no +platform implementation before binding) are untouched: the tolerance relaxes the module-count +proxy, never the loading property. ## The conflict @@ -26,16 +30,18 @@ Splitting a hub module necessarily adds static value edges beneath it, so every reaches it grows by the number of new modules. The two rules have no shared approval path: one says the file must be divided, the other says the division must not be visible to any importer. -## Rules at a glance, while unresolved +## Rules at a glance, resolved -- Do not split `registry.ts`. The budget gate is the binding constraint and has no waiver. -- Do not add an approved-growth row, a baseline entry, or a suppression to pass it. That is the - allowlist `AGENTS.md` forbids. +- A hub split is allowed when it is statement-neutral: every merge-base module stays in every + affected closure, and total module-scope statements do not grow. The gate enforces this; an + unexplained count growth still fails with no approval path, as before. +- Do not add an approved-growth row, a baseline entry, or a suppression to pass the gate. That is + the allowlist `AGENTS.md` forbids; the split tolerance is a rule, not a waiver — it decides from + the two graphs, and nothing is hand-entered per split. - Do not make registry construction asynchronous to dodge the walker. ADR 0008's synchronous root is load-bearing for compile-time totality. -- Closure-neutral edits inside the descriptor root are still welcome: relocating guards beside the - union they constrain, correcting stale claims, and moving non-eager content out are fine so long as - every measured entry surface is unchanged. +- The split tolerance says a re-home adds no eager *work*; it does not license adding eager + *edges* under a façade. The ADR-0019 checks stay hard. ## Measured @@ -69,23 +75,43 @@ The cost is genuinely a *count*, not weight: the same bytes are evaluated either makes the conflict worth deciding rather than absorbing silently, because the gate's proxy and the debt rule's proxy disagree at exactly this shape. -## Decision required +## Decision -May a byte-neutral re-homing of a hub module's contents into new modules raise the eager module count -of the entry surfaces that reach it — or must a hub module's contents stay inside the file its -consumers already evaluate? +May a byte-neutral re-homing of a hub module's contents into new modules raise the eager module +count of the entry surfaces that reach it? **Yes — exactly when it is a split**, decided by two +facts computed from the two closure graphs, not by an entered number: -Answering "the count may rise" needs a bounded, auditable growth allowance on an invariant that -currently refuses one. Answering "contents stay put" needs `AGENTS.md`'s 1,000-line rule to name the -exception for a hub whose consumers evaluate it as one unit, so the size is treated as accepted and -measured rather than as unaddressed debt. Either answer is fine; leaving it unstated means the next -agent at this file discovers the conflict from a red gate and picks one silently. +1. **Containment:** the head closure evaluates every module the merge-base closure evaluated + (renames canonicalized), so the growth is purely additive. +2. **Flat weight:** the closure's total top-level statement count — wiring lines excluded, so + re-homing is invisible to it — does not exceed the merge-base closure's. + +This is issue #2469's candidate 4 (structure ∧ weight), implemented as the minimal slice: each +half alone is refutable — containment without weight accepts a "split" that smuggles a new heavy +edge; weight without containment accepts dropping a merge-base module behind an equal-sized +replacement — and together they are the smallest pair that admits healthy extraction and refuses +new eager work. Statements were chosen over bytes because formatting and comment churn move bytes +while statements track the thing being preserved: module-scope evaluation work. Both are proxies; +the planted tests in `scripts/__tests__/closure-growth-rule.test.ts` pin the acceptance pair in +both directions, and the hard ADR-0019 checks remain the non-proxy backstop under it. + +Maintenance cost of an ordinary extraction under this rule: zero configuration edits. No row, +baseline, or number is touched per split — the merge-base comparison recomputes everything. (The +candidate-3 approval-row design instead costs one hand-edited row per split plus its retirement.) + +The `AGENTS.md` 1,000-line rule now has no hub exception to name: a statement-neutral split is +shippable, so an oversized hub is addressable debt rather than accepted state. ## Alternatives considered and refuted -- **Approved-growth row mirroring `APPROVED_OVER_CEILING` (issue, reason, owner):** refuted pending the - decision above. It would be auditable, but it is an allowlist on the one invariant deliberately - given no approval path, and `AGENTS.md` forbids adding one to obtain a pass. +- **Approved-growth row mirroring `APPROVED_OVER_CEILING` (issue, reason, owner):** refuted. It + would be auditable, but it is an allowlist on the one invariant deliberately given no approval + path, `AGENTS.md` forbids adding one to obtain a pass, and every split would owe a row plus its + retirement — maintenance the chosen rule does not. +- **Structure alone (#2469 candidate 2):** refuted by the planted smuggle test — a "new" module can + carry a new heavy edge, so containment proves nothing about eager work. +- **Weight alone (#2469 candidate 1):** refuted by the planted drop test — a swap can keep the + statement total flat while quietly making the closure evaluate different modules. - **Leave the array in place:** the status quo, not a resolution — it leaves the size debt unowned and the conflict undocumented. - **Function-scoped `await import` per family:** passes the gate by making registry construction @@ -99,5 +125,5 @@ agent at this file discovers the conflict from a red gate and picks one silently A complete, byte-faithful split of the descriptor array exists on `refactor/collocation-registry-family-split` (`40bf719a8b`, split commit `ad3aaa520e`) and is pushed. -It is kept rather than deleted so that answering the question above does not require redoing the -mechanical work; 80 descriptors, 79 byte-identical including comments. +With this decision accepted, that branch is now shippable: its +11 is a statement-neutral split of +exactly the tolerated shape, so it needs no gate change beyond this one. diff --git a/docs/adr/0028-capability-family-cell-vocabulary.md b/docs/adr/0028-capability-family-cell-vocabulary.md index b65dd49862..10509c5f9d 100644 --- a/docs/adr/0028-capability-family-cell-vocabulary.md +++ b/docs/adr/0028-capability-family-cell-vocabulary.md @@ -70,10 +70,10 @@ family rather than arguing from the shape: 2. Report the change in files-touched for that family against the recorded baseline of the four recurring eight-package commits. 3. Report eager-closure deltas per entry surface. `scripts/__tests__/eager-closure-budgets.ts` has no - growth-approval path, and [ADR 0027](0027-descriptor-root-vs-eager-closure-budget.md) shows a - byte-neutral re-home can be unshippable here. A row table is a **new eager value**; if it costs - closure slots, it is blocked on the same ground and this ADR should be withdrawn rather than - granted an exception. + growth-approval path; [ADR 0027](0027-descriptor-root-vs-eager-closure-budget.md) tolerates only a + statement-neutral re-home. A row table is a **new eager value**, so it passes only if its + module-scope weight comes with a statement-neutral re-home somewhere in the same closure — + otherwise the gate stays red and this ADR should be withdrawn rather than granted an exception. If the fan-out does not visibly shrink, close this as rejected and leave the two authored lists in place — a compile-checked duplicate is a legitimate resting state. diff --git a/docs/adr/README.md b/docs/adr/README.md index dabc53d7b1..9fa192dc6f 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -28,7 +28,7 @@ | [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 | | [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 | | [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 | -| [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 | +| [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 (containment + flat top-level-statement weight) and nothing else; splitting `packages/command-registry/src/registry.ts` is unblocked | | [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 | | [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 | | [0030 Process Lock Exclusion](0030-process-lock-exclusion.md) | process-lock publication/reclaim/release, retained mutation guards, and the single-protocol upgrade boundary | diff --git a/packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts b/packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts index 9ace0ef253..4025b7a879 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts @@ -8,7 +8,6 @@ import type { ExecOptions } from '@agent-device/host-kit/command'; import fs from 'node:fs'; import path from 'node:path'; import { - createRunnerPhaseBudget, requireRunnerBuildSettingsMatchBuildLog, diffComparableRunnerCacheMetadata, resolveRunnerBundleBuildSettings, @@ -18,8 +17,9 @@ import { resolveRunnerSandboxBuildArgs, resolveExpectedRunnerCacheMetadata, resolveRunnerDerivedPath, - memoizedRunnerXcodeVersion, } from '../runner-cache-metadata.ts'; +import { createRunnerPhaseBudget } from '../runner-phase-budget.ts'; +import { memoizedRunnerXcodeVersion } from '../runner-toolchain-probe.ts'; import { COLD_TOOLCHAIN_PROBE_TIMEOUT_MS } from '../apple-runner-platform.ts'; import { appleToolchainProbeResult, diff --git a/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts b/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts index 9f862ab941..4d8e4106fe 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts @@ -11,11 +11,8 @@ import { appleRunnerTestHost } from '../test-host.ts'; import { withAppleRunnerProvider } from '../runner-provider.ts'; import { classifyRunnerReportedError, type RunnerCommand } from '../runner-contract.ts'; import { RUNNER_REPLY_LOST_REASON } from '../runner-error-classification.ts'; -import { - createRunnerPhaseBudget, - requireRunnerPhaseRemainingMs, - resolveExpectedRunnerCacheMetadata, -} from '../runner-cache-metadata.ts'; +import { resolveExpectedRunnerCacheMetadata } from '../runner-cache-metadata.ts'; +import { createRunnerPhaseBudget, requireRunnerPhaseRemainingMs } from '../runner-phase-budget.ts'; import { captureDiagnostics } from './runner-session-fixtures.ts'; import { startFakeRunnerServer, diff --git a/packages/platform-apple/src/runner/runner-cache-metadata.ts b/packages/platform-apple/src/runner/runner-cache-metadata.ts index 79954c917f..587b94069a 100644 --- a/packages/platform-apple/src/runner/runner-cache-metadata.ts +++ b/packages/platform-apple/src/runner/runner-cache-metadata.ts @@ -3,29 +3,19 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { isMacOs, type DeviceInfo } from '@agent-device/kernel/device'; +import { AppError } from '@agent-device/kernel/errors'; +import { isEnvTruthy, findProjectRoot, readVersion } from './host.ts'; import { - AppError, - createRequestCanceledError, - isRequestCanceledError, -} from '@agent-device/kernel/errors'; -import { - commandDeveloperDir, - createTtlMemo, - Deadline, - isCommandTimeoutError, - isEnvTruthy, - findProjectRoot, - readVersion, - runCmdSync, -} from './host.ts'; -import type { TtlMemo } from '@agent-device/kernel/ttl-memo'; -import { - COLD_TOOLCHAIN_PROBE_TIMEOUT_MS, resolveRunnerBuildDestinationFamily, resolveRunnerDerivedBaseName, resolveRunnerPlatformName, resolveRunnerSdkName, } from './apple-runner-platform.ts'; +import { + requireRunnerToolchainFingerprint, + type RunnerToolchainFingerprint, +} from './runner-toolchain-probe.ts'; +import type { RunnerPhaseBudget } from './runner-phase-budget.ts'; import { computeRunnerSourceFingerprint } from './runner-source.ts'; const DEFAULT_IOS_RUNNER_APP_BUNDLE_ID = 'com.callstack.agentdevice.runner'; @@ -34,16 +24,6 @@ export const RUNNER_CACHE_METADATA_FILE = '.agent-device-runner-cache.json'; const RUNNER_CACHE_SCHEMA_VERSION = 3; const RUNNER_CACHE_METADATA_VALUE_MAX_LENGTH = 300; -/** - * Ceiling on the wall clock the whole toolchain fingerprint may spend, across all three - * probes and their retries, when the owning phase carries no shorter budget: one stalled - * probe, its warm retry, and the two probes still to run (#2422). - */ -const TOOLCHAIN_FINGERPRINT_BUDGET_MS = 45_000; -const TOOLCHAIN_PROBE_MAX_BUFFER = 128 * 1024; -const TOOLCHAIN_PROBE_DETAIL_MAX_LENGTH = 200; -const TOOLCHAIN_PROBE_HINT = - 'The Apple runner cache is keyed on the toolchain version, so a cache decision cannot be made without it. Retry once the host is less loaded, or check `xcode-select -p` and `xcodebuild -version`.'; const RUNNER_SANDBOX_BUILD_ARGS = [ '-IDEPackageSupportDisableManifestSandbox=1', '-IDEPackageSupportDisablePluginExecutionSandbox=1', @@ -58,111 +38,6 @@ const RUNNER_RUNTIME_SWIFT_FLAGS = '$(inherited) -disable-sandbox -D AGENT_DEVICE_RUNNER_ISOLATION_CANARY'; const RUNNER_UNIT_TEST_SWIFT_FLAGS = `${RUNNER_RUNTIME_SWIFT_FLAGS} -D AGENT_DEVICE_RUNNER_UNIT_TESTS`; -/** Toolchain half of the runner cache key. Every field is a probed value. */ -export type RunnerToolchainFingerprint = { - xcodeVersion: string; - xcodeBuildVersion: string; - sdkName: string; - sdkVersion: string; - sdkBuildVersion: string; -}; - -type ToolchainProbeFailure = { - probe: string; - reason: 'probe_error' | 'nonzero_exit' | 'empty_output' | 'unparsable_output'; - detail: string; -}; - -/** - * Everything one runner phase may spend: the single clock every step of the phase reads, - * and the owning request's cancellation. Created once, where the phase begins, and handed - * on as this object — no step below receives a timeout number it could open a second phase - * with, which is how a cold probe stall and the build each spent the same budget (#2422). - */ -export type RunnerPhaseBudget = Readonly<{ - /** The phase's clock; absent when its owner carries no budget at all. */ - deadline?: Deadline; - /** The owning request's cancellation signal, if it carries one. */ - signal?: AbortSignal; -}>; - -/** - * Opens a phase from the numeric timeout its public option carries: the one place a number - * becomes a budget, so every boundary below it takes the {@link RunnerPhaseBudget} instead. - */ -export function createRunnerPhaseBudget( - timeoutMs: number | undefined, - signal: AbortSignal | undefined, -): RunnerPhaseBudget { - const bounded = timeoutMs !== undefined && Number.isFinite(timeoutMs); - return { - deadline: bounded ? Deadline.fromTimeoutMs(Math.max(0, timeoutMs)) : undefined, - signal, - }; -} - -/** - * What the phase has left for its next step, or `undefined` when it carries no deadline. - * Throws rather than returning zero, so a spent phase fails before it spawns. - */ -export function requireRunnerPhaseRemainingMs( - budget: RunnerPhaseBudget | undefined, - phase: string, -): number | undefined { - const deadline = budget?.deadline; - if (!deadline) return undefined; - const remainingMs = Math.floor(deadline.remainingMs()); - if (remainingMs <= 0) throw runnerPhaseBudgetExhaustedError(phase); - return remainingMs; -} - -/** Says the phase budget ran out, not that the step it would have run is broken. */ -function runnerPhaseBudgetExhaustedError(phase: string): AppError { - return new AppError('COMMAND_FAILED', 'The Apple runner budget ran out before this step began', { - phase, - reason: 'runner_phase_budget_exhausted', - retriable: true, - }); -} - -/** - * The remaining-time and cancellation view the probes consult: one per fingerprint read, - * so the three probes and their retries share a single budget. A phase with no deadline - * still gets {@link TOOLCHAIN_FINGERPRINT_BUDGET_MS} as the ceiling. - * - * `spawnSync` cannot be interrupted once it has started, so cancellation is observed - * between attempts; the per-attempt cap is what bounds how long that takes. - */ -type ToolchainProbeClock = { - /** Milliseconds the next attempt may block for; 0 once the budget is spent. */ - attemptTimeoutMs(): number; - /** Throws the owning request's cancellation error once it has aborted. */ - throwIfCanceled(): void; -}; - -function createToolchainProbeClock(budget: RunnerPhaseBudget | undefined): ToolchainProbeClock { - const phaseDeadline = budget?.deadline; - const deadline = Deadline.fromTimeoutMs( - Math.min( - TOOLCHAIN_FINGERPRINT_BUDGET_MS, - phaseDeadline ? phaseDeadline.remainingMs() : Number.POSITIVE_INFINITY, - ), - ); - return { - attemptTimeoutMs: () => - Math.min(COLD_TOOLCHAIN_PROBE_TIMEOUT_MS, Math.floor(deadline.remainingMs())), - throwIfCanceled: () => { - if (budget?.signal?.aborted) { - throw createRequestCanceledError({ phase: 'apple_toolchain_probe' }); - } - }, - }; -} - -type ProbeResult = - | { ok: true; value: Value } - | { ok: false; failure: ToolchainProbeFailure }; - export type RunnerXctestrunCacheMetadata = RunnerToolchainFingerprint & { schemaVersion: number; packageVersion: string; @@ -273,211 +148,6 @@ export function resolveExpectedRunnerCacheMetadata( }; } -// Lazy: createTtlMemo is a host capability, and module evaluation happens -// before the composition root binds the host. Only a complete, parsed -// fingerprint is ever memoized, so nothing unavailable can outlive the probe -// that could not answer. The key carries a client-chosen DEVELOPER_DIR, so an -// entry expires once no request has used it for TOOLCHAIN_FINGERPRINT_TTL_MS: -// a dir no client uses any more must not stay for the daemon's lifetime. A hit -// renews the entry, so a failure report can name the Xcode a decision read for -// at least TOOLCHAIN_FINGERPRINT_TTL_MS after that read (the default start budget). -const TOOLCHAIN_FINGERPRINT_TTL_MS = 10 * 60_000; -let lazyToolchainFingerprintCache: TtlMemo | undefined; -function toolchainFingerprintCache(): TtlMemo { - lazyToolchainFingerprintCache ??= createTtlMemo({ - ttlMs: TOOLCHAIN_FINGERPRINT_TTL_MS, - scheduleExpiry: true, - }); - return lazyToolchainFingerprintCache; -} - -/** - * The toolchain half of the cache key. It also names the derived-data directory, so an - * unreadable toolchain fails the cache decision instead of standing in for one. - */ -function requireRunnerToolchainFingerprint( - sdkName: string, - budget: RunnerPhaseBudget | undefined, -): RunnerToolchainFingerprint { - // Before the cache, not just before the probes: a hit must not hide a cancellation. - const clock = createToolchainProbeClock(budget); - clock.throwIfCanceled(); - const cacheKey = toolchainFingerprintCacheKey(sdkName); - const cached = toolchainFingerprintCache().get(cacheKey); - if (cached) { - toolchainFingerprintCache().set(cacheKey, cached); - return cached; - } - const fingerprint = readRunnerToolchainFingerprint(sdkName, clock); - if (!fingerprint.ok) throw unavailableToolchainError(fingerprint.failures); - toolchainFingerprintCache().set(cacheKey, fingerprint.value); - return fingerprint.value; -} - -/** - * A daemon serves clients that select different Xcodes through `DEVELOPER_DIR`, so a fingerprint - * read under one developer dir answers only for that dir. An empty dir means xcode-select's. - */ -function toolchainFingerprintCacheKey(sdkName: string): string { - return `${commandDeveloperDir() ?? ''}\0${sdkName}`; -} - -/** - * The selected Xcode's version as this process's runner cache decision memoized it, for a failure - * report that names it; undefined when no decision has read the toolchain in the last - * `TOOLCHAIN_FINGERPRINT_TTL_MS`. Never probes: a report must not wait on the toolchain it - * describes. - */ -export function memoizedRunnerXcodeVersion(device: DeviceInfo): string | undefined { - return toolchainFingerprintCache().get( - toolchainFingerprintCacheKey( - resolveRunnerSdkName(resolveRunnerPlatformName(device), device.kind), - ), - )?.xcodeVersion; -} - -function readRunnerToolchainFingerprint( - sdkName: string, - clock: ToolchainProbeClock, -): - | { ok: true; value: RunnerToolchainFingerprint } - | { ok: false; failures: readonly ToolchainProbeFailure[] } { - const xcode = parseXcodeVersionOutput(runToolchainProbe('xcodebuild', ['-version'], clock)); - const sdkVersion = runToolchainProbe('xcrun', ['--sdk', sdkName, '--show-sdk-version'], clock); - const sdkBuildVersion = runToolchainProbe( - 'xcrun', - ['--sdk', sdkName, '--show-sdk-build-version'], - clock, - ); - if (!xcode.ok || !sdkVersion.ok || !sdkBuildVersion.ok) { - return { - ok: false, - failures: [xcode, sdkVersion, sdkBuildVersion].flatMap((probe) => - probe.ok ? [] : [probe.failure], - ), - }; - } - return { - ok: true, - value: { - xcodeVersion: xcode.value.version, - xcodeBuildVersion: xcode.value.buildVersion, - sdkName, - sdkVersion: sdkVersion.value, - sdkBuildVersion: sdkBuildVersion.value, - }, - }; -} - -function unavailableToolchainError(failures: readonly ToolchainProbeFailure[]): AppError { - return new AppError( - 'COMMAND_FAILED', - `Could not read the Xcode toolchain versions the Apple runner cache is keyed on (${failures - .map((failure) => `${failure.probe}: ${failure.detail}`) - .join('; ')})`, - { - reason: 'apple_toolchain_probe_unavailable', - retriable: true, - probes: failures, - hint: TOOLCHAIN_PROBE_HINT, - }, - ); -} - -function runToolchainProbe( - cmd: string, - args: string[], - clock: ToolchainProbeClock, -): ProbeResult { - const probe = [cmd, ...args].join(' '); - let output: { exitCode: number; stdout: string; stderr: string }; - try { - output = runToolchainProbeCommand(cmd, args, clock); - } catch (error) { - // A cancellation or a spent budget is the caller's error, not an unreadable toolchain. - clock.throwIfCanceled(); - if (isRequestCanceledError(error) || isRunnerPhaseBudgetExhaustedError(error)) throw error; - return probeFailure(probe, 'probe_error', error instanceof Error ? error.message : `${error}`); - } - if (output.exitCode !== 0) { - return probeFailure( - probe, - 'nonzero_exit', - `exit ${output.exitCode}${output.stderr.trim() ? `: ${output.stderr.trim()}` : ''}`, - ); - } - const value = output.stdout.trim(); - return value ? { ok: true, value } : probeFailure(probe, 'empty_output', 'no output'); -} - -/** - * Retries exactly once, and only the exec layer's structured timeout: the stall - * {@link COLD_TOOLCHAIN_PROBE_TIMEOUT_MS} names clears on the next exec of the same tool, - * while a tool that failed on its own and said "timed out" in its output is not it. - */ -function runToolchainProbeCommand( - cmd: string, - args: string[], - clock: ToolchainProbeClock, -): { exitCode: number; stdout: string; stderr: string } { - try { - return attemptToolchainProbe(cmd, args, clock); - } catch (error) { - if (!isCommandTimeoutError(error)) throw error; - return attemptToolchainProbe(cmd, args, clock); - } -} - -/** The one guard site: cancellation and a spent budget both throw here, before any exec. */ -function attemptToolchainProbe( - cmd: string, - args: string[], - clock: ToolchainProbeClock, -): { exitCode: number; stdout: string; stderr: string } { - clock.throwIfCanceled(); - const timeoutMs = clock.attemptTimeoutMs(); - if (timeoutMs <= 0) throw runnerPhaseBudgetExhaustedError('apple_toolchain_probe'); - return runCmdSync(cmd, args, { - allowFailure: true, - timeoutMs, - maxBuffer: TOOLCHAIN_PROBE_MAX_BUFFER, - }); -} - -function isRunnerPhaseBudgetExhaustedError(error: unknown): boolean { - return error instanceof AppError && error.details?.reason === 'runner_phase_budget_exhausted'; -} - -function parseXcodeVersionOutput( - output: ProbeResult, -): ProbeResult<{ version: string; buildVersion: string }> { - if (!output.ok) { - return output; - } - const version = output.value.match(/^Xcode\s+(.+)$/m)?.[1]?.trim(); - const buildVersion = output.value.match(/^Build version\s+(.+)$/m)?.[1]?.trim(); - if (!version || !buildVersion) { - return probeFailure( - 'xcodebuild -version', - 'unparsable_output', - `unrecognized output: ${output.value.replaceAll('\n', ' ')}`, - ); - } - return { ok: true, value: { version, buildVersion } }; -} - -function probeFailure( - probe: string, - reason: ToolchainProbeFailure['reason'], - detail: string, -): { ok: false; failure: ToolchainProbeFailure } { - const bounded = - detail.length > TOOLCHAIN_PROBE_DETAIL_MAX_LENGTH - ? `${detail.slice(0, TOOLCHAIN_PROBE_DETAIL_MAX_LENGTH)}…` - : detail; - return { ok: false, failure: { probe, reason, detail: bounded } }; -} - export function resolveRunnerDerivedPath( device: DeviceInfo, metadata: RunnerXctestrunCacheMetadata, diff --git a/packages/platform-apple/src/runner/runner-cache.ts b/packages/platform-apple/src/runner/runner-cache.ts index 71499098ea..96a26760e2 100644 --- a/packages/platform-apple/src/runner/runner-cache.ts +++ b/packages/platform-apple/src/runner/runner-cache.ts @@ -22,7 +22,6 @@ import type { RunnerCacheRefusal, } from './runner-artifact-manifest.ts'; export { - requireRunnerPhaseRemainingMs, resolveExpectedRunnerCacheMetadata, resolveRunnerArchBuildSettings, resolveRunnerBuildLocationSettings, @@ -32,9 +31,13 @@ export { resolveRunnerPerformanceBuildSettings, resolveRunnerSandboxBuildArgs, resolveRunnerSigningBuildSettings, - type RunnerPhaseBudget, type RunnerXctestrunCacheMetadata, } from './runner-cache-metadata.ts'; +export { + createRunnerPhaseBudget, + requireRunnerPhaseRemainingMs, + type RunnerPhaseBudget, +} from './runner-phase-budget.ts'; const RUNNER_XCTESTRUN_CACHE_LOCK_TIMEOUT_MS = 10 * 60_000; const RUNNER_XCTESTRUN_CACHE_LOCK_POLL_MS = 100; diff --git a/packages/platform-apple/src/runner/runner-device-set.ts b/packages/platform-apple/src/runner/runner-device-set.ts index 36930f4030..1a86aa4954 100644 --- a/packages/platform-apple/src/runner/runner-device-set.ts +++ b/packages/platform-apple/src/runner/runner-device-set.ts @@ -1,6 +1,6 @@ import type { DeviceInfo } from '@agent-device/kernel/device'; import { simulatorAddressFor } from './host.ts'; -import { memoizedRunnerXcodeVersion } from './runner-cache-metadata.ts'; +import { memoizedRunnerXcodeVersion } from './runner-toolchain-probe.ts'; /** The scoped simulator set that holds this runner's simulator, or undefined for the default set. */ export function runnerSimulatorSetPath(device: DeviceInfo): string | undefined { diff --git a/packages/platform-apple/src/runner/runner-phase-budget.ts b/packages/platform-apple/src/runner/runner-phase-budget.ts new file mode 100644 index 0000000000..b6e6bf9d67 --- /dev/null +++ b/packages/platform-apple/src/runner/runner-phase-budget.ts @@ -0,0 +1,59 @@ +import { AppError } from '@agent-device/kernel/errors'; +import { Deadline } from './host.ts'; + +/** + * Everything one runner phase may spend: the single clock every step of the phase reads, + * and the owning request's cancellation. Created once, where the phase begins, and handed + * on as this object — no step below receives a timeout number it could open a second phase + * with, which is how a cold probe stall and the build each spent the same budget (#2422). + */ +export type RunnerPhaseBudget = Readonly<{ + /** The phase's clock; absent when its owner carries no budget at all. */ + deadline?: Deadline; + /** The owning request's cancellation signal, if it carries one. */ + signal?: AbortSignal; +}>; + +/** + * Opens a phase from the numeric timeout its public option carries: the one place a number + * becomes a budget, so every boundary below it takes the {@link RunnerPhaseBudget} instead. + */ +export function createRunnerPhaseBudget( + timeoutMs: number | undefined, + signal: AbortSignal | undefined, +): RunnerPhaseBudget { + const bounded = timeoutMs !== undefined && Number.isFinite(timeoutMs); + return { + deadline: bounded ? Deadline.fromTimeoutMs(Math.max(0, timeoutMs)) : undefined, + signal, + }; +} + +/** + * What the phase has left for its next step, or `undefined` when it carries no deadline. + * Throws rather than returning zero, so a spent phase fails before it spawns. + */ +export function requireRunnerPhaseRemainingMs( + budget: RunnerPhaseBudget | undefined, + phase: string, +): number | undefined { + const deadline = budget?.deadline; + if (!deadline) return undefined; + const remainingMs = Math.floor(deadline.remainingMs()); + if (remainingMs <= 0) throw runnerPhaseBudgetExhaustedError(phase); + return remainingMs; +} + +/** Says the phase budget ran out, not that the step it would have run is broken. */ +export function runnerPhaseBudgetExhaustedError(phase: string): AppError { + return new AppError('COMMAND_FAILED', 'The Apple runner budget ran out before this step began', { + phase, + reason: 'runner_phase_budget_exhausted', + retriable: true, + }); +} + +/** The step that threw named its own phase; the caller only needs to know it was the budget. */ +export function isRunnerPhaseBudgetExhaustedError(error: unknown): boolean { + return error instanceof AppError && error.details?.reason === 'runner_phase_budget_exhausted'; +} diff --git a/packages/platform-apple/src/runner/runner-toolchain-probe.ts b/packages/platform-apple/src/runner/runner-toolchain-probe.ts new file mode 100644 index 0000000000..547939152a --- /dev/null +++ b/packages/platform-apple/src/runner/runner-toolchain-probe.ts @@ -0,0 +1,289 @@ +import { + AppError, + createRequestCanceledError, + isRequestCanceledError, +} from '@agent-device/kernel/errors'; +import type { DeviceInfo } from '@agent-device/kernel/device'; +import type { TtlMemo } from '@agent-device/kernel/ttl-memo'; +import { + commandDeveloperDir, + createTtlMemo, + Deadline, + isCommandTimeoutError, + runCmdSync, +} from './host.ts'; +import { + COLD_TOOLCHAIN_PROBE_TIMEOUT_MS, + resolveRunnerPlatformName, + resolveRunnerSdkName, +} from './apple-runner-platform.ts'; +import { + isRunnerPhaseBudgetExhaustedError, + runnerPhaseBudgetExhaustedError, + type RunnerPhaseBudget, +} from './runner-phase-budget.ts'; + +/** + * Ceiling on the wall clock the whole toolchain fingerprint may spend, across all three + * probes and their retries, when the owning phase carries no shorter budget: one stalled + * probe, its warm retry, and the two probes still to run (#2422). + */ +const TOOLCHAIN_FINGERPRINT_BUDGET_MS = 45_000; +const TOOLCHAIN_PROBE_MAX_BUFFER = 128 * 1024; +const TOOLCHAIN_PROBE_DETAIL_MAX_LENGTH = 200; +const TOOLCHAIN_PROBE_HINT = + 'The Apple runner cache is keyed on the toolchain version, so a cache decision cannot be made without it. Retry once the host is less loaded, or check `xcode-select -p` and `xcodebuild -version`.'; + +/** Toolchain half of the runner cache key. Every field is a probed value. */ +export type RunnerToolchainFingerprint = { + xcodeVersion: string; + xcodeBuildVersion: string; + sdkName: string; + sdkVersion: string; + sdkBuildVersion: string; +}; + +type ToolchainProbeFailure = { + probe: string; + reason: 'probe_error' | 'nonzero_exit' | 'empty_output' | 'unparsable_output'; + detail: string; +}; + +type ProbeResult = + | { ok: true; value: Value } + | { ok: false; failure: ToolchainProbeFailure }; + +/** + * The remaining-time and cancellation view the probes consult: one per fingerprint read, + * so the three probes and their retries share a single budget. A phase with no deadline + * still gets {@link TOOLCHAIN_FINGERPRINT_BUDGET_MS} as the ceiling. + * + * `spawnSync` cannot be interrupted once it has started, so cancellation is observed + * between attempts; the per-attempt cap is what bounds how long that takes. + */ +type ToolchainProbeClock = { + /** Milliseconds the next attempt may block for; 0 once the budget is spent. */ + attemptTimeoutMs(): number; + /** Throws the owning request's cancellation error once it has aborted. */ + throwIfCanceled(): void; +}; + +function createToolchainProbeClock(budget: RunnerPhaseBudget | undefined): ToolchainProbeClock { + const phaseDeadline = budget?.deadline; + const deadline = Deadline.fromTimeoutMs( + Math.min( + TOOLCHAIN_FINGERPRINT_BUDGET_MS, + phaseDeadline ? phaseDeadline.remainingMs() : Number.POSITIVE_INFINITY, + ), + ); + return { + attemptTimeoutMs: () => + Math.min(COLD_TOOLCHAIN_PROBE_TIMEOUT_MS, Math.floor(deadline.remainingMs())), + throwIfCanceled: () => { + if (budget?.signal?.aborted) { + throw createRequestCanceledError({ phase: 'apple_toolchain_probe' }); + } + }, + }; +} + +// Lazy: createTtlMemo is a host capability, and module evaluation happens +// before the composition root binds the host. Only a complete, parsed +// fingerprint is ever memoized, so nothing unavailable can outlive the probe +// that could not answer. The key carries a client-chosen DEVELOPER_DIR, so an +// entry expires once no request has used it for TOOLCHAIN_FINGERPRINT_TTL_MS: +// a dir no client uses any more must not stay for the daemon's lifetime. A hit +// renews the entry, so a failure report can name the Xcode a decision read for +// at least TOOLCHAIN_FINGERPRINT_TTL_MS after that read (the default start budget). +const TOOLCHAIN_FINGERPRINT_TTL_MS = 10 * 60_000; +let lazyToolchainFingerprintCache: TtlMemo | undefined; +function toolchainFingerprintCache(): TtlMemo { + lazyToolchainFingerprintCache ??= createTtlMemo({ + ttlMs: TOOLCHAIN_FINGERPRINT_TTL_MS, + scheduleExpiry: true, + }); + return lazyToolchainFingerprintCache; +} + +/** + * The toolchain half of the cache key. It also names the derived-data directory, so an + * unreadable toolchain fails the cache decision instead of standing in for one. + */ +export function requireRunnerToolchainFingerprint( + sdkName: string, + budget: RunnerPhaseBudget | undefined, +): RunnerToolchainFingerprint { + // Before the cache, not just before the probes: a hit must not hide a cancellation. + const clock = createToolchainProbeClock(budget); + clock.throwIfCanceled(); + const cacheKey = toolchainFingerprintCacheKey(sdkName); + const cached = toolchainFingerprintCache().get(cacheKey); + if (cached) { + toolchainFingerprintCache().set(cacheKey, cached); + return cached; + } + const fingerprint = readRunnerToolchainFingerprint(sdkName, clock); + if (!fingerprint.ok) throw unavailableToolchainError(fingerprint.failures); + toolchainFingerprintCache().set(cacheKey, fingerprint.value); + return fingerprint.value; +} + +/** + * A daemon serves clients that select different Xcodes through `DEVELOPER_DIR`, so a fingerprint + * read under one developer dir answers only for that dir. An empty dir means xcode-select's. + */ +function toolchainFingerprintCacheKey(sdkName: string): string { + return `${commandDeveloperDir() ?? ''}\0${sdkName}`; +} + +/** + * The selected Xcode's version as this process's runner cache decision memoized it, for a failure + * report that names it; undefined when no decision has read the toolchain in the last + * `TOOLCHAIN_FINGERPRINT_TTL_MS`. Never probes: a report must not wait on the toolchain it + * describes. + */ +export function memoizedRunnerXcodeVersion(device: DeviceInfo): string | undefined { + return toolchainFingerprintCache().get( + toolchainFingerprintCacheKey( + resolveRunnerSdkName(resolveRunnerPlatformName(device), device.kind), + ), + )?.xcodeVersion; +} + +function readRunnerToolchainFingerprint( + sdkName: string, + clock: ToolchainProbeClock, +): + | { ok: true; value: RunnerToolchainFingerprint } + | { ok: false; failures: readonly ToolchainProbeFailure[] } { + const xcode = parseXcodeVersionOutput(runToolchainProbe('xcodebuild', ['-version'], clock)); + const sdkVersion = runToolchainProbe('xcrun', ['--sdk', sdkName, '--show-sdk-version'], clock); + const sdkBuildVersion = runToolchainProbe( + 'xcrun', + ['--sdk', sdkName, '--show-sdk-build-version'], + clock, + ); + if (!xcode.ok || !sdkVersion.ok || !sdkBuildVersion.ok) { + return { + ok: false, + failures: [xcode, sdkVersion, sdkBuildVersion].flatMap((probe) => + probe.ok ? [] : [probe.failure], + ), + }; + } + return { + ok: true, + value: { + xcodeVersion: xcode.value.version, + xcodeBuildVersion: xcode.value.buildVersion, + sdkName, + sdkVersion: sdkVersion.value, + sdkBuildVersion: sdkBuildVersion.value, + }, + }; +} + +function unavailableToolchainError(failures: readonly ToolchainProbeFailure[]): AppError { + return new AppError( + 'COMMAND_FAILED', + `Could not read the Xcode toolchain versions the Apple runner cache is keyed on (${failures + .map((failure) => `${failure.probe}: ${failure.detail}`) + .join('; ')})`, + { + reason: 'apple_toolchain_probe_unavailable', + retriable: true, + probes: failures, + hint: TOOLCHAIN_PROBE_HINT, + }, + ); +} + +function runToolchainProbe( + cmd: string, + args: string[], + clock: ToolchainProbeClock, +): ProbeResult { + const probe = [cmd, ...args].join(' '); + let output: { exitCode: number; stdout: string; stderr: string }; + try { + output = runToolchainProbeCommand(cmd, args, clock); + } catch (error) { + // A cancellation or a spent budget is the caller's error, not an unreadable toolchain. + clock.throwIfCanceled(); + if (isRequestCanceledError(error) || isRunnerPhaseBudgetExhaustedError(error)) throw error; + return probeFailure(probe, 'probe_error', error instanceof Error ? error.message : `${error}`); + } + if (output.exitCode !== 0) { + return probeFailure( + probe, + 'nonzero_exit', + `exit ${output.exitCode}${output.stderr.trim() ? `: ${output.stderr.trim()}` : ''}`, + ); + } + const value = output.stdout.trim(); + return value ? { ok: true, value } : probeFailure(probe, 'empty_output', 'no output'); +} + +/** + * Retries exactly once, and only the exec layer's structured timeout: the stall + * {@link COLD_TOOLCHAIN_PROBE_TIMEOUT_MS} names clears on the next exec of the same tool, + * while a tool that failed on its own and said "timed out" in its output is not it. + */ +function runToolchainProbeCommand( + cmd: string, + args: string[], + clock: ToolchainProbeClock, +): { exitCode: number; stdout: string; stderr: string } { + try { + return attemptToolchainProbe(cmd, args, clock); + } catch (error) { + if (!isCommandTimeoutError(error)) throw error; + return attemptToolchainProbe(cmd, args, clock); + } +} + +/** The one guard site: cancellation and a spent budget both throw here, before any exec. */ +function attemptToolchainProbe( + cmd: string, + args: string[], + clock: ToolchainProbeClock, +): { exitCode: number; stdout: string; stderr: string } { + clock.throwIfCanceled(); + const timeoutMs = clock.attemptTimeoutMs(); + if (timeoutMs <= 0) throw runnerPhaseBudgetExhaustedError('apple_toolchain_probe'); + return runCmdSync(cmd, args, { + allowFailure: true, + timeoutMs, + maxBuffer: TOOLCHAIN_PROBE_MAX_BUFFER, + }); +} + +function parseXcodeVersionOutput( + output: ProbeResult, +): ProbeResult<{ version: string; buildVersion: string }> { + if (!output.ok) { + return output; + } + const version = output.value.match(/^Xcode\s+(.+)$/m)?.[1]?.trim(); + const buildVersion = output.value.match(/^Build version\s+(.+)$/m)?.[1]?.trim(); + if (!version || !buildVersion) { + return probeFailure( + 'xcodebuild -version', + 'unparsable_output', + `unrecognized output: ${output.value.replaceAll('\n', ' ')}`, + ); + } + return { ok: true, value: { version, buildVersion } }; +} + +function probeFailure( + probe: string, + reason: ToolchainProbeFailure['reason'], + detail: string, +): { ok: false; failure: ToolchainProbeFailure } { + const bounded = + detail.length > TOOLCHAIN_PROBE_DETAIL_MAX_LENGTH + ? `${detail.slice(0, TOOLCHAIN_PROBE_DETAIL_MAX_LENGTH)}…` + : detail; + return { ok: false, failure: { probe, reason, detail: bounded } }; +} diff --git a/packages/platform-apple/src/runner/runner-xctestrun.ts b/packages/platform-apple/src/runner/runner-xctestrun.ts index b3c9de1f48..278554e7aa 100644 --- a/packages/platform-apple/src/runner/runner-xctestrun.ts +++ b/packages/platform-apple/src/runner/runner-xctestrun.ts @@ -28,11 +28,13 @@ export { type RunnerXctestrunCacheKind, } from './runner-cache.ts'; export { - createRunnerPhaseBudget, IOS_RUNNER_CONTAINER_BUNDLE_IDS, - requireRunnerPhaseRemainingMs, resolveExpectedRunnerCacheMetadata, resolveRunnerAppBundleId, resolveRunnerDerivedPath, - type RunnerPhaseBudget, } from './runner-cache-metadata.ts'; +export { + createRunnerPhaseBudget, + requireRunnerPhaseRemainingMs, + type RunnerPhaseBudget, +} from './runner-phase-budget.ts'; diff --git a/scripts/__tests__/closure-growth-rule.test.ts b/scripts/__tests__/closure-growth-rule.test.ts new file mode 100644 index 0000000000..e49165b35b --- /dev/null +++ b/scripts/__tests__/closure-growth-rule.test.ts @@ -0,0 +1,238 @@ +import { expect, test } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import path from 'node:path'; +import { + eagerClosureGraphOf, + topLevelStatementCount, + topLevelStatementWeightsOf, + type SourceTreeReader, +} from '../../src/__tests__/eager-import-closure.fixtures.ts'; +import { createCommittedSourceTree } from './committed-source-tree.ts'; +import { mkGitFixtureRepo } from './eager-closure-gate-fixtures.ts'; +import { + classifyGrowth, + closureGrowthEvidence, + type ClosureGrowthEvidence, +} from './closure-growth-rule.ts'; + +/** + * The NO-GROWTH rule and its split tolerance (#2469, ADR 0027), tested in both failing + * directions. The gate's real-tree lane (`eager-closure-budgets.test.ts`) cannot distinguish a + * correct tolerance from a vacuous one while the tree happens to contain no splits, so every + * direction the two facts (containment, flat weight) guard gets its own planted repo here: + * a pure split that must pass, a split smuggling a new heavy edge that must fail, a swap that + * drops a merge-base module that must fail at flat weight, and a rename that must not read as a + * drop. + */ + +test('no-growth fails growth with both counts and passes an equal or smaller closure', () => { + const at = (over: Partial = {}) => ({ + baseCount: 42, + headCount: 42, + baseWeight: 200, + headWeight: 200, + preservedBaseClosure: true, + ...over, + }); + expect(classifyGrowth('x.ts', at())).toBeNull(); + expect(classifyGrowth('x.ts', at({ headCount: 40 }))).toBeNull(); + expect( + classifyGrowth('x.ts', at({ headCount: 43, headWeight: 210 })), + 'count growth with weight growth fails', + ).toMatch(/evaluates 43 modules.*merge-base evaluated 42/); + expect( + classifyGrowth('x.ts', at({ headCount: 43, preservedBaseClosure: false })), + 'count growth that dropped a merge-base module fails even at flat weight', + ).toMatch(/evaluates 43 modules.*merge-base evaluated 42/); +}); + +// --- the split tolerance (#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 (ADR 0027). +// The tolerance is exactly two facts: the head still evaluates EVERY merge-base module, and the +// closure's total top-level-statement weight did not grow. Both failing directions are planted. + +test('a pure split passes: every base module survives and total weight is flat', () => { + // entry -> hub(8 statements) becomes entry -> hub(wiring only) -> part-a + part-b carrying the + // declarations: count 2 -> 4, weight unchanged. + const at = (over: Partial = {}) => ({ + baseCount: 2, + headCount: 4, + baseWeight: 10, + headWeight: 10, + preservedBaseClosure: true, + ...over, + }); + expect(classifyGrowth('x.ts', at())).toBeNull(); + expect(classifyGrowth('x.ts', at({ headWeight: 9 })), 'shrinking needs no edit').toBeNull(); +}); + +test('a split smuggling eager work fails and names both the modules and the statements', () => { + const finding = classifyGrowth('x.ts', { + baseCount: 2, + headCount: 4, + baseWeight: 10, + headWeight: 14, + preservedBaseClosure: true, + }); + expect(finding).toMatch(/evaluates 4 modules.*merge-base evaluated 2/); + expect(finding).toMatch(/adds 2 module\(s\) and 4 top-level statement\(s\)/); + expect(finding, 'a pure split must never print the rewrite warning').not.toMatch(/not a split/); +}); + +test('growth that drops a merge-base module fails even at flat weight: not a split', () => { + const finding = classifyGrowth('x.ts', { + baseCount: 2, + headCount: 4, + baseWeight: 10, + headWeight: 10, + preservedBaseClosure: false, + }); + expect(finding).toMatch(/evaluates 4 modules.*merge-base evaluated 2/); + expect(finding).toMatch(/not a split of existing code/); +}); + +/** + * A hub repo at two states: the hub commit carrying three declarations, then the working tree + * holding the pure split of two of them into parts. The smuggle test mutates this state BEFORE + * its first working-tree walk, because the walker memoizes edges per path -- one walk per repo. + */ +function mkSplitFixtureRepo(prefix: string): { + repo: string; + entry: string; + hubTree: SourceTreeReader; + write: (rel: string, content: string) => void; +} { + const repo = mkGitFixtureRepo(prefix); + const write = (rel: string, content: string) => fs.writeFileSync(path.join(repo, rel), content); + write( + 'packages/demo/src/entry.ts', + 'export const a = 1;\nexport const b = 2;\nexport const c = 3;\n', + ); + execFileSync('git', ['add', '-A'], { cwd: repo }); + execFileSync( + 'git', + ['-c', 'user.name=Gate', '-c', 'user.email=gate@example.test', 'commit', '-qm', 'hub'], + { cwd: repo }, + ); + const hubTree = createCommittedSourceTree(repo, 'HEAD'); + // b and c move out verbatim; the hub keeps a and re-exports the parts. + write( + 'packages/demo/src/entry.ts', + "export const a = 1;\nexport * from './part-b.ts';\nexport * from './part-c.ts';\n", + ); + write('packages/demo/src/part-b.ts', 'export const b = 2;\n'); + write('packages/demo/src/part-c.ts', 'export const c = 3;\n'); + return { repo, entry: path.join(repo, 'packages/demo/src/entry.ts'), hubTree, write }; +} + +function evidenceFromHub(entry: string, hubTree: SourceTreeReader): ClosureGrowthEvidence { + const baseGraph = eagerClosureGraphOf(entry, hubTree); + const headGraph = eagerClosureGraphOf(entry); + return closureGrowthEvidence({ + baseGraph, + headGraph, + baseWeights: topLevelStatementWeightsOf(baseGraph.keys(), hubTree), + headWeights: topLevelStatementWeightsOf(headGraph.keys()), + }); +} + +test('planted pure split passes end to end: count grows, every base module survives, weight flat', () => { + const { entry, hubTree } = mkSplitFixtureRepo('eager-closure-split-pure-'); + const evidence = evidenceFromHub(entry, hubTree); + expect(evidence, 'one module became three: the count grows by two').toMatchObject({ + baseCount: 1, + headCount: 3, + }); + expect( + evidence, + 'three declarations before, three after; the wiring lines are invisible', + ).toMatchObject({ baseWeight: 3, headWeight: 3, preservedBaseClosure: true }); + expect( + classifyGrowth('demo/entry', evidence), + 'the extraction ADR 0027 could not ship', + ).toBeNull(); +}); + +test('planted split smuggling a new heavy edge fails, naming the smuggled weight', () => { + const { entry, hubTree, write } = mkSplitFixtureRepo('eager-closure-split-smuggled-'); + write( + 'packages/demo/src/part-c.ts', + "import { scan } from './scanner.ts';\nexport const c = scan();\n", + ); + write('packages/demo/src/scanner.ts', 'export function scan() {\n return 4;\n}\n'); + const evidence = evidenceFromHub(entry, hubTree); + expect(evidence.headWeight, 'scan() runs at module scope: that is eager work').toBe(4); + expect(classifyGrowth('demo/entry', evidence)).toMatch( + /adds 3 module\(s\) and 1 top-level statement\(s\)/, + ); +}); + +test('a renamed module inside the growth is not a dropped module', () => { + const repo = mkGitFixtureRepo('eager-closure-split-rename-'); + const baseEntry = path.join(repo, 'packages/demo/src/entry.ts'); + const headEntry = path.join(repo, 'packages/demo/src/hub.ts'); + execFileSync('git', ['mv', 'packages/demo/src/entry.ts', 'packages/demo/src/hub.ts'], { + cwd: repo, + }); + const baseWeights = topLevelStatementWeightsOf( + [baseEntry], + createCommittedSourceTree(repo, 'HEAD'), + ); + const headWeights = topLevelStatementWeightsOf([headEntry]); + const shape = { + baseGraph: new Map([[baseEntry, null]]), + headGraph: new Map([[headEntry, null]]), + baseWeights, + headWeights, + }; + expect( + closureGrowthEvidence({ + ...shape, + renamedBaseToHead: new Map([[baseEntry, headEntry]]), + }).preservedBaseClosure, + 'the rename is the same module', + ).toBe(true); + expect( + closureGrowthEvidence(shape).preservedBaseClosure, + 'without the rename map the same shape reads as a dropped module', + ).toBe(false); +}); + +test('the weight counts module-scope work, not wiring or re-exporting', () => { + // The whole tolerance rests on this number being invisible to re-homing wiring and sensitive to + // module-scope execution. Both sides planted on one file each. + expect( + topLevelStatementCount( + 'hub.ts', + "import { a } from './a.ts';\nexport * from './b.ts';\nexport const c = 3;\nexport function d() {}\n", + ), + 'two wiring lines, two real statements', + ).toBe(2); + expect( + topLevelStatementCount( + 'smuggle.ts', + "import { scan } from './scanner.ts';\nexport const c = scan();\n", + ), + 'the scan() call is module-scope work, only the import is wiring', + ).toBe(1); +}); + +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', { + baseCount: 42, + headCount: 43, + baseWeight: 200, + headWeight: 210, + preservedBaseClosure: true, + }) ?? ''; + 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`/); +}); diff --git a/scripts/__tests__/closure-growth-rule.ts b/scripts/__tests__/closure-growth-rule.ts new file mode 100644 index 0000000000..5bbb83cf20 --- /dev/null +++ b/scripts/__tests__/closure-growth-rule.ts @@ -0,0 +1,98 @@ +// The eager-closure NO-GROWTH rule and its split tolerance (#2469, ADR 0027). +// +// The gate in `eager-closure-budgets.ts` holds every entry that exists at the merge-base to +// "no more modules than the merge-base evaluated". The module count is a proxy for eager work, +// and the proxy disagrees with the repository's oversized-file rule at exactly one shape: a +// split of a hub module re-homes declarations into new files while evaluating the same total +// module-scope work, and every entry reaching the hub grows by the new files (#2423 had to +// defer its extraction; ADR 0027 measured the descriptor-root split at +11 per surface). +// +// The tolerance is two facts about the growth, both read from the closure graphs: +// +// 1. CONTAINMENT: the head closure still evaluates EVERY module the merge-base closure did +// (renames canonicalized), so nothing was dropped or swapped -- the growth is purely +// addition. +// 2. FLAT WEIGHT: the closure's total top-level statement count (`topLevelStatementCount`: +// wiring excluded) did not grow, so the added files carry re-homed declarations, not new +// module-scope work. Any smuggled eager work -- a module-scope call into a new heavy edge -- +// adds statements and stays red. +// +// Each alone is insufficient: containment without weight accepts a split that smuggles eager +// work; weight without containment accepts dropping a merge-base module while adding a heavier +// one at the same statement count. Together they are the smallest rule that admits healthy +// extraction and refuses new eager edges, which is why they replace neither the count (the +// comparison still fires first: growth must exist to be tolerated) nor the hard ADR-0019 checks +// (façade exactness, no platform implementation before binding), which stay count- and +// pattern-based and untouched. + +/** + * What one entry's no-growth verdict reads. Counts come from the closure graphs; weights are the + * closures' total top-level statements (`topLevelStatementCount`: wiring excluded, so re-homing + * code between modules is invisible to it) and the subset fact says whether the head still + * evaluates every module the merge-base did. + */ +export type ClosureGrowthEvidence = { + baseCount: number; + headCount: number; + baseWeight: number; + headWeight: number; + /** Head evaluates every module of the base closure (renames mapped through `renamedBaseToHead`). */ + preservedBaseClosure: boolean; +}; + +/** + * The one comparison both the real-tree lane and the planted split tests run: counts straight off + * the graphs, weights summed over each closure's own tree (a base module's weight read from the + * base tree, its head counterpart from the head tree), and the split's containment shape with + * renames canonicalized so a `git mv` inside a split stays the split it is. + */ +export function closureGrowthEvidence(params: { + baseGraph: ReadonlyMap; + headGraph: ReadonlyMap; + baseWeights: ReadonlyMap; + headWeights: ReadonlyMap; + renamedBaseToHead?: ReadonlyMap; +}): ClosureGrowthEvidence { + const { baseGraph, headGraph, baseWeights, headWeights, renamedBaseToHead } = params; + const sum = (files: Iterable, weights: ReadonlyMap) => + [...files].reduce((total, file) => total + (weights.get(file) ?? 0), 0); + return { + baseCount: baseGraph.size, + headCount: headGraph.size, + baseWeight: sum(baseGraph.keys(), baseWeights), + headWeight: sum(headGraph.keys(), headWeights), + preservedBaseClosure: [...baseGraph.keys()].every((baseFile) => + headGraph.has(renamedBaseToHead?.get(baseFile) ?? baseFile), + ), + }; +} + +/** + * The no-growth verdict: `null` unless the head closure is larger than the merge-base one and the + * growth is not a tolerated pure split (containment plus flat weight, per the header). + * + * 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, evidence: ClosureGrowthEvidence): string | null { + const { baseCount, headCount, baseWeight, headWeight, preservedBaseClosure } = evidence; + if (headCount <= baseCount) return null; + if (preservedBaseClosure && headWeight <= baseWeight) return null; + const weightNote = preservedBaseClosure + ? ` It adds ${headCount - baseCount} module(s) and ${ + headWeight - baseWeight + } top-level statement(s) of eager work on top of every module the merge-base already evaluated, which a pure split does not.` + : ' It also stops evaluating a module the merge-base did, so it is not a split of existing code.'; + return ( + `${id} evaluates ${headCount} modules on import; the merge-base evaluated ${baseCount}.` + + weightNote + + ' 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.' + ); +} diff --git a/scripts/__tests__/eager-closure-budgets.test.ts b/scripts/__tests__/eager-closure-budgets.test.ts index 1d7122f16e..83a2a996a1 100644 --- a/scripts/__tests__/eager-closure-budgets.test.ts +++ b/scripts/__tests__/eager-closure-budgets.test.ts @@ -1,9 +1,11 @@ import { expect, test } from 'vitest'; import { execFileSync } from 'node:child_process'; import fs from 'node:fs'; -import os from 'node:os'; import path from 'node:path'; -import { eagerClosureGraphOf } from '../../src/__tests__/eager-import-closure.fixtures.ts'; +import { + eagerClosureGraphOf, + topLevelStatementWeightsOf, +} from '../../src/__tests__/eager-import-closure.fixtures.ts'; import { baseProductionPathOf, createCommittedSourceTree, @@ -11,10 +13,15 @@ import { mergeBaseWithMain, renamedSince, } from './committed-source-tree.ts'; +import { mkGitFixtureRepo } from './eager-closure-gate-fixtures.ts'; +import { + classifyGrowth, + closureGrowthEvidence, + type ClosureGrowthEvidence, +} from './closure-growth-rule.ts'; import { addedModules, APPROVED_OVER_CEILING, - classifyGrowth, classifyNewEntry, describeClosureGrowth, describeClosurePressure, @@ -67,23 +74,35 @@ const absolute = (file: string) => path.resolve(repoRoot, file); // Each of these covers a hole that the real-tree assertions below cannot see: while the tree // satisfies its rules, a wrong comparison, an unfollowed rename, a reader that quietly falls // back to the working directory, and a one-level discovery scan all look like correct rules. +// The NO-GROWTH rule, its split tolerance, and the growth-advice text are tested in +// `closure-growth-rule.test.ts`, mirroring `closure-growth-rule.ts`; what follows covers this +// module's own rules. + +/** Aggregation fixtures need evidence the rule FAILS: count growth it cannot excuse. */ +function grew(baseCount: number): ClosureGrowthEvidence { + return { + baseCount, + headCount: baseCount + 1, + baseWeight: 10, + headWeight: 14, + preservedBaseClosure: true, + }; +} -test('no-growth fails growth with both counts and passes an equal or smaller closure', () => { - expect(classifyGrowth('x.ts', 42, 42)).toBeNull(); - expect(classifyGrowth('x.ts', 42, 40)).toBeNull(); - 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`/); -}); +/** Aggregation fixtures need evidence the rule PASSES, for each passing shape separately. */ +function passed(params: { + baseCount: number; + headCount: number; + preservedBaseClosure?: boolean; +}): ClosureGrowthEvidence { + return { + baseCount: params.baseCount, + headCount: params.headCount, + baseWeight: 10, + headWeight: 10, + preservedBaseClosure: params.preservedBaseClosure ?? true, + }; +} 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 @@ -240,8 +259,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, headClosureSize: graphA.size + 1 }, - { id: 'entry-b.ts', added: [addedFile], baseGraph: graphB, headClosureSize: graphB.size + 1 }, + { id: 'entry-a.ts', added: [addedFile], baseGraph: graphA, evidence: grew(graphA.size) }, + { id: 'entry-b.ts', added: [addedFile], baseGraph: graphB, evidence: grew(graphB.size) }, ], '/repo', ); @@ -258,7 +277,7 @@ 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, headClosureSize: 2 }], + [{ id: 'entry-a.ts', added: ['/repo/new.ts'], baseGraph: graphA, evidence: grew(1) }], '/repo', ), 'nothing to aggregate when only one entry grew by this module', @@ -283,13 +302,35 @@ test('an equal-size replacement is not growth, so the aggregate stays silent', ( ['/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( + classifyGrowth( + 'entry-a.ts', + passed({ baseCount: graphA.size, headCount: graphA.size, preservedBaseClosure: false }), + ), + ).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 }, + { + id: 'entry-a.ts', + added: [addedFile], + baseGraph: graphA, + evidence: passed({ + baseCount: graphA.size, + headCount: graphA.size, + preservedBaseClosure: false, + }), + }, + { + id: 'entry-b.ts', + added: [addedFile], + baseGraph: graphB, + evidence: passed({ + baseCount: graphB.size, + headCount: graphB.size, + preservedBaseClosure: false, + }), + }, ], '/repo', ), @@ -298,12 +339,25 @@ test('an equal-size replacement is not growth, so the aggregate stays silent', ( expect( describeSharedGrowthHomes( [ - { id: 'entry-a.ts', added: [addedFile], baseGraph: graphA, headClosureSize: graphA.size }, + { + id: 'entry-a.ts', + added: [addedFile], + baseGraph: graphA, + evidence: passed({ + baseCount: graphA.size, + headCount: graphA.size, + preservedBaseClosure: false, + }), + }, { id: 'entry-b.ts', added: [addedFile], baseGraph: graphB, - headClosureSize: graphB.size - 1, + evidence: passed({ + baseCount: graphB.size, + headCount: graphB.size - 1, + preservedBaseClosure: false, + }), }, ], '/repo', @@ -312,6 +366,40 @@ test('an equal-size replacement is not growth, so the aggregate stays silent', ( ).toBeNull(); }); +test('a tolerated pure split produces no shared-homes note', () => { + // #2469: two entries that both grew by the SAME new modules because a shared hub split. The + // per-entry rule tolerates it (containment holds, weight flat), so the aggregate -- whose whole + // premise is entries the rule FAILED -- may not report it. Without this, every tolerated split + // would turn one green lane into two red aggregate failures. + const addedPart = '/repo/packages/demo/src/part.ts'; + const sharedHome = '/repo/packages/demo/src/shared-home.ts'; + const graph = (entry: string) => + new Map([ + [entry, null], + [sharedHome, entry], + ]); + expect( + describeSharedGrowthHomes( + [ + { + id: 'entry-a.ts', + added: [addedPart], + baseGraph: graph('/repo/entry-a.ts'), + evidence: passed({ baseCount: 2, headCount: 3 }), + }, + { + id: 'entry-b.ts', + added: [addedPart], + baseGraph: graph('/repo/entry-b.ts'), + evidence: passed({ baseCount: 2, headCount: 3 }), + }, + ], + '/repo', + ), + 'both entries passed as pure splits: nothing failed, nothing to explain', + ).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 @@ -327,7 +415,7 @@ test('disjoint growth groups keep their own entries and homes in separate blocks [entryPath, null], [home, entryPath], ]); - return { id, added: [added], baseGraph, headClosureSize: baseGraph.size + 1 }; + return { id, added: [added], baseGraph, evidence: grew(baseGraph.size) }; }; const shared = describeSharedGrowthHomes( @@ -357,40 +445,10 @@ test('disjoint growth groups keep their own entries and homes in separate blocks }); /** - * 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 - * caller writes afterwards is by definition uncommitted scratch. - * - * A real git repo rather than a bare tmpdir because tracked-vs-untracked only exists relative to - * git, following `scripts/layering/platform-package-repository.test.ts`. + * A committed fixture repository (`mkGitFixtureRepo`, shared with + * `closure-growth-rule.test.ts`): one workspace package with a manifest export target, a + * top-level façade, a nested façade, and a test source. */ -function mkGitFixtureRepo(prefix: string): string { - const repo = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); - const pkgDir = path.join(repo, 'packages/demo'); - fs.mkdirSync(path.join(pkgDir, 'src/facades/nested'), { recursive: true }); - fs.writeFileSync( - path.join(pkgDir, 'package.json'), - JSON.stringify({ - name: '@agent-device/demo', - exports: { '.': './src/entry.ts' }, - }), - ); - fs.writeFileSync(path.join(pkgDir, 'src/entry.ts'), 'export const a = 1;\n'); - fs.writeFileSync(path.join(pkgDir, 'src/facades/top.ts'), 'export const b = 2;\n'); - fs.writeFileSync(path.join(pkgDir, 'src/facades/nested/deep.ts'), 'export const c = 3;\n'); - fs.writeFileSync(path.join(pkgDir, 'src/facades/skip.test.ts'), 'export const d = 4;\n'); - fs.mkdirSync(path.join(repo, 'scripts')); - fs.writeFileSync(path.join(repo, 'scripts/standalone.ts'), 'export const outside = 1;\n'); - execFileSync('git', ['init', '-q'], { cwd: repo }); - execFileSync('git', ['add', '.'], { cwd: repo }); - execFileSync( - 'git', - ['-c', 'user.name=Gate', '-c', 'user.email=gate@example.test', 'commit', '-qm', 'base'], - { cwd: repo }, - ); - return repo; -} - test('the committed-tree reader walks what was committed, not the working directory', () => { // The hole: a reader that falls back to `fs` for anything it cannot answer from git turns the // merge-base side into a second copy of the head side, and no-growth passes every growth. @@ -569,6 +627,16 @@ const carried = others.flatMap((entry) => { }); const introduced = others.filter((entry) => basePathOf(entry.entryFile) === null); +/** Base-path -> head-path, the containment direction `closureGrowthEvidence` canonicalizes with. */ +const renamedBaseToHead = new Map( + [...renamedFrom].map(([headPath, basePath]) => [absolute(basePath), absolute(headPath)]), +); + +/** Head-closure weights for `graph`, off the working tree (memoized per file). */ +function headWeightsOf(graph: ReadonlyMap): Map { + return topLevelStatementWeightsOf(graph.keys()); +} + /** * 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 @@ -579,7 +647,21 @@ const carriedGrowth = carried.map((entry) => { 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) }; + return { + entry, + entryPath, + graph, + baseGraph, + base, + added: addedModules(graph, base, entryPath), + evidence: closureGrowthEvidence({ + baseGraph, + headGraph: graph, + baseWeights: topLevelStatementWeightsOf(baseGraph.keys(), baseTree), + headWeights: headWeightsOf(graph), + renamedBaseToHead, + }), + }; }); test('every hub exists and is not also a discovered façade', () => { @@ -617,8 +699,8 @@ test.for(platformFacades)('$id evaluates exactly one module: itself', (entry) => }); 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); + const { entry, entryPath, graph, base, evidence } = growth; + const finding = classifyGrowth(entry.id, evidence); expect( finding, finding === null @@ -640,7 +722,7 @@ test('entries that grow by the same new module report shared merge-base homes on id: growth.entry.id, added: growth.added, baseGraph: growth.baseGraph, - headClosureSize: growth.graph.size, + evidence: growth.evidence, })); const shared = describeSharedGrowthHomes(growths, repoRoot); expect( diff --git a/scripts/__tests__/eager-closure-budgets.ts b/scripts/__tests__/eager-closure-budgets.ts index 118558ace2..351a00821d 100644 --- a/scripts/__tests__/eager-closure-budgets.ts +++ b/scripts/__tests__/eager-closure-budgets.ts @@ -16,7 +16,10 @@ // - Every other entry that exists at the merge-base with origin/main: NO GROWTH. Its closure may // not be larger than the closure of the same file (renames followed) in the committed // merge-base tree, read through `committed-source-tree.ts`. Shrinking needs no edit: there is -// no number to keep in step, and the next merge-base keeps the gain. +// no number to keep in step, and the next merge-base keeps the gain. Module COUNT growth is +// tolerated for exactly one shape -- a pure SPLIT -- by the rule in `closure-growth-rule.ts` +// (#2469, ADR 0027). The hard ADR-0019 checks (façade exactness, platform implementation never +// eager) stay count- and pattern-based and are untouched. // - An entry absent at the merge-base: a per-category CEILING (`NEW_ENTRY_CEILINGS`). At or // 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 @@ -30,6 +33,7 @@ import path from 'node:path'; import { facadeEntryFiles } from '../layering/package-boundaries.ts'; +import { classifyGrowth, type ClosureGrowthEvidence } from './closure-growth-rule.ts'; export type EntryCategory = | 'platform-facade' @@ -190,25 +194,9 @@ export function eagerClosureEntries(repoRoot: string): EagerClosureEntry[] { } /** - * 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. + * What one entry's no-growth verdict reads, and the verdict itself, live in + * `closure-growth-rule.ts` (#2469); this module applies them and owns everything else. */ -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}. 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.' - ); -} /** The ceiling verdict for a first-introduced entry: `null` when it fits or is approved. */ export function classifyNewEntry( @@ -495,12 +483,12 @@ export type GrowthForAggregation = { /** 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). + * The same evidence the per-entry NO-GROWTH test judged, so the aggregation applies the SAME + * condition `classifyGrowth` does -- including the split tolerance. A closure that swapped one + * module for another, shrank, or passed as a pure split has newly evaluated modules and no + * growth -- the rule passes it, so nothing here may report it (#2471 review). */ - headClosureSize: number; + evidence: ClosureGrowthEvidence; }; /** How many candidate homes one added-module block names before it just counts the rest. */ @@ -526,7 +514,16 @@ type SharedGrowthGroup = { /** 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; + return growth.evidence.headCount - growth.evidence.baseCount; +} + +/** + * Did the per-entry rule actually fail this growth? The aggregation may fire only where + * `classifyGrowth` does -- equal-size swaps, shrinks, and tolerated pure splits have newly + * evaluated modules and no violation, so they must contribute nothing (#2471 review, #2469). + */ +function grewPastTheRule(growth: GrowthForAggregation): boolean { + return classifyGrowth(growth.id, growth.evidence) !== null; } /** @@ -553,16 +550,16 @@ function hasNoInPackageChild( } /** - * 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. + * Every entry the NO-GROWTH rule actually failed, indexed by each of its added modules -- one + * entry may appear under several. Entries the per-entry rule passes are dropped here, which is + * what keeps the aggregation from reporting a swap or a tolerated split (#2471 review, #2469). */ function growthsByAddedModule( growths: readonly GrowthForAggregation[], ): Map { const byAddedModule = new Map(); for (const growth of growths) { - if (netGrowth(growth) <= 0) continue; + if (!grewPastTheRule(growth)) continue; for (const added of growth.added) { const group = byAddedModule.get(added); if (group) group.push(growth); diff --git a/scripts/__tests__/eager-closure-gate-fixtures.ts b/scripts/__tests__/eager-closure-gate-fixtures.ts new file mode 100644 index 0000000000..02e24f8c20 --- /dev/null +++ b/scripts/__tests__/eager-closure-gate-fixtures.ts @@ -0,0 +1,46 @@ +/** + * Creates a minimal git fixture repository (tracked demo package + scratch helpers) used by the + * eager-closure gate tests. Returns the repo root. Everything committed here is TRACKED, so + * anything a caller writes afterwards is by definition uncommitted scratch. + * + * Lives beside the gate tests that use it (`eager-closure-budgets.test.ts`, + * `closure-growth-rule.test.ts`). A real git repo rather than a bare tmpdir because + * tracked-vs-untracked only exists relative to git, following + * `scripts/layering/platform-package-repository.test.ts`. + */ + +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +/** + * 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. + */ +export function mkGitFixtureRepo(prefix: string): string { + const repo = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + const pkgDir = path.join(repo, 'packages/demo'); + fs.mkdirSync(path.join(pkgDir, 'src/facades/nested'), { recursive: true }); + fs.writeFileSync( + path.join(pkgDir, 'package.json'), + JSON.stringify({ + name: '@agent-device/demo', + exports: { '.': './src/entry.ts' }, + }), + ); + fs.writeFileSync(path.join(pkgDir, 'src/entry.ts'), 'export const a = 1;\n'); + fs.writeFileSync(path.join(pkgDir, 'src/facades/top.ts'), 'export const b = 2;\n'); + fs.writeFileSync(path.join(pkgDir, 'src/facades/nested/deep.ts'), 'export const c = 3;\n'); + fs.writeFileSync(path.join(pkgDir, 'src/facades/skip.test.ts'), 'export const d = 4;\n'); + fs.mkdirSync(path.join(repo, 'scripts')); + fs.writeFileSync(path.join(repo, 'scripts/standalone.ts'), 'export const outside = 1;\n'); + execFileSync('git', ['init', '-q'], { cwd: repo }); + execFileSync('git', ['add', '.'], { cwd: repo }); + execFileSync( + 'git', + ['-c', 'user.name=Gate', '-c', 'user.email=gate@example.test', 'commit', '-qm', 'base'], + { cwd: repo }, + ); + return repo; +} diff --git a/src/__tests__/eager-import-closure.fixtures.ts b/src/__tests__/eager-import-closure.fixtures.ts index 19a9a6455b..1d5e34453f 100644 --- a/src/__tests__/eager-import-closure.fixtures.ts +++ b/src/__tests__/eager-import-closure.fixtures.ts @@ -100,10 +100,51 @@ function collectEagerDynamicImports(node: unknown, eager: boolean, found: string /** Every specifier this file evaluates at load time, static or dynamic. */ export function eagerlyEvaluatedModules(fileName: string, source: string): string[] { + return parseEagerModule(fileName, source).specifiers; +} + +/** + * The file's eager WEIGHT: its count of top-level statements, excluding the statements that + * only name other modules (`import ...`, `export ... from ...`). Comments and formatting are + * invisible to it, and an `export` keyword on a moved declaration does not create a statement, + * so re-homing code between modules keeps the number whatever the wiring lines become; adding + * module-scope work does not. It is the weight compared by the split tolerance in + * `scripts/__tests__/closure-growth-rule.ts` (#2469). + */ +export function topLevelStatementCount(fileName: string, source: string): number { + return parseEagerModule(fileName, source).topLevelStatements; +} + +type EagerModule = { specifiers: string[]; topLevelStatements: number }; + +function parseEagerModule(fileName: string, source: string): EagerModule { const parsed = parseSync(fileName, source); const dynamic: string[] = []; collectEagerDynamicImports(parsed.program, true, dynamic); - return [...new Set([...staticEvaluatedRefs(parsed.module), ...dynamic])]; + return { + specifiers: [...new Set([...staticEvaluatedRefs(parsed.module), ...dynamic])], + topLevelStatements: countTopLevelStatements(parsed.program), + }; +} + +function isWiringOnlyStatement(statement: AstNode): boolean { + if (statement.type === 'ImportDeclaration') return true; + // `export * from 'x'` and `export { a } from 'x'` name a module; `export const a = 1` does not. + if (statement.type === 'ExportNamedDeclaration' || statement.type === 'ExportAllDeclaration') { + return statement.source !== undefined && statement.source !== null; + } + return false; +} + +function countTopLevelStatements(program: unknown): number { + const body = (program as { body?: unknown[] } | undefined)?.body; + if (!Array.isArray(body)) return 0; + return body.filter( + (statement) => + statement !== null && + typeof statement === 'object' && + !isWiringOnlyStatement(statement as AstNode), + ).length; } /** @@ -194,6 +235,8 @@ function resolveWorkspace( type TreeMemo = { packageDirs: Map; directEdges: Map; + /** Weight of each walked file, memoized with its edges (#2469). */ + weights: Map; }; const treeMemos = new WeakMap(); @@ -203,6 +246,7 @@ function memoOf(tree: SourceTreeReader): TreeMemo { memo = { packageDirs: readWorkspacePackageDirs(tree), directEdges: new Map(), + weights: new Map(), }; treeMemos.set(tree, memo); } @@ -210,17 +254,31 @@ function memoOf(tree: SourceTreeReader): TreeMemo { } /** - * Specifiers per file, keyed by content: a merge-base and a working tree share almost every file - * byte-for-byte and parsing is the expensive step, so a second tree parses only what differs. + * Specifiers and top-level weight per file, keyed by content: a merge-base and a working tree + * share almost every file byte-for-byte and parsing is the expensive step, so a second tree + * parses only what differs. */ -const parsedByFile = new Map(); +const parsedByFile = new Map(); -function specifiersOf(file: string, source: string): string[] { +function parsedModuleOf(file: string, source: string): EagerModule { const cached = parsedByFile.get(file); - if (cached && cached.source === source) return cached.specifiers; - const specifiers = eagerlyEvaluatedModules(file, source); - parsedByFile.set(file, { source, specifiers }); - return specifiers; + if (cached && cached.source === source) return cached.module; + const module = parseEagerModule(file, source); + parsedByFile.set(file, { source, module }); + return module; +} + +/** + * Top-level-statement weight per file, as the split tolerance measures it. One read per path + * through the tree's reader; the parse itself is shared with the edge walk via `parsedByFile`. + */ +export function topLevelStatementWeightsOf( + files: Iterable, + tree: SourceTreeReader = workingTreeReader, +): Map { + const weights = new Map(); + for (const file of files) weights.set(file, weightOf(file, tree)); + return weights; } /** The repo files `file` evaluates directly, already resolved to absolute paths. */ @@ -229,7 +287,7 @@ function directEagerEdges(file: string, tree: SourceTreeReader): string[] { const cached = memo.directEdges.get(file); if (cached) return cached; const resolvedEdges: string[] = []; - for (const specifier of specifiersOf(file, tree.readFile(file))) { + for (const specifier of parsedModuleOf(file, tree.readFile(file)).specifiers) { const resolved = specifier.startsWith('.') ? resolveRelative(file, specifier, tree) : resolveWorkspace(specifier, memo.packageDirs, tree); @@ -239,6 +297,15 @@ function directEagerEdges(file: string, tree: SourceTreeReader): string[] { return resolvedEdges; } +function weightOf(file: string, tree: SourceTreeReader): number { + const memo = memoOf(tree); + const cached = memo.weights.get(file); + if (cached !== undefined) return cached; + const weight = parsedModuleOf(file, tree.readFile(file)).topLevelStatements; + memo.weights.set(file, weight); + return weight; +} + /** * Every repo file evaluated as a consequence of importing `entryFile`, mapped to the * file that first pulled it in (`null` for the entry itself). diff --git a/vitest.config.ts b/vitest.config.ts index 1bcd155b27..0e8c6e2960 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -126,6 +126,7 @@ export default defineConfig({ // scripts/mutation/modules.ts. 'scripts/__tests__/test-file-size-ratchet.test.ts', 'scripts/__tests__/eager-closure-budgets.test.ts', + 'scripts/__tests__/closure-growth-rule.test.ts', 'scripts/__tests__/agent-setup-startup-contract.test.ts', 'scripts/__tests__/npm-skills-exclusion.test.ts', 'scripts/__tests__/simulator-skills-contract.test.ts', From 5e96a8785dd010bde7a0d79a4f6941924944478c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 7 Oct 2026 21:38:17 +0200 Subject: [PATCH 2/9] 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. --- packages/platform-apple/src/runner/runner-cache.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/platform-apple/src/runner/runner-cache.ts b/packages/platform-apple/src/runner/runner-cache.ts index 96a26760e2..8a825049d1 100644 --- a/packages/platform-apple/src/runner/runner-cache.ts +++ b/packages/platform-apple/src/runner/runner-cache.ts @@ -33,11 +33,7 @@ export { resolveRunnerSigningBuildSettings, type RunnerXctestrunCacheMetadata, } from './runner-cache-metadata.ts'; -export { - createRunnerPhaseBudget, - requireRunnerPhaseRemainingMs, - type RunnerPhaseBudget, -} from './runner-phase-budget.ts'; +export { requireRunnerPhaseRemainingMs, type RunnerPhaseBudget } from './runner-phase-budget.ts'; const RUNNER_XCTESTRUN_CACHE_LOCK_TIMEOUT_MS = 10 * 60_000; const RUNNER_XCTESTRUN_CACHE_LOCK_POLL_MS = 100; From bdf8876ce59e5dcc9ce7aa0f387e2c84bd2b2aa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 7 Oct 2026 22:24:30 +0200 Subject: [PATCH 3/9] 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. --- ...descriptor-root-vs-eager-closure-budget.md | 22 +++++++++++++------ .../0028-capability-family-cell-vocabulary.md | 7 +++--- scripts/__tests__/closure-growth-rule.test.ts | 7 ++++++ scripts/__tests__/closure-growth-rule.ts | 21 +++++++++--------- .../eager-import-closure.fixtures.ts | 10 ++++++--- 5 files changed, 44 insertions(+), 23 deletions(-) diff --git a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md index 64c0b31697..06c792ff95 100644 --- a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md +++ b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md @@ -40,8 +40,9 @@ the file must be divided, the other says the division must not be visible to any the two graphs, and nothing is hand-entered per split. - Do not make registry construction asynchronous to dodge the walker. ADR 0008's synchronous root is load-bearing for compile-time totality. -- The split tolerance says a re-home adds no eager *work*; it does not license adding eager - *edges* under a façade. The ADR-0019 checks stay hard. +- The split tolerance guarantees flat statement weight over a preserved closure; it does not + license adding eager *edges* under a façade — the head must still evaluate every merge-base + module, and the ADR-0019 checks stay hard. ## Measured @@ -89,11 +90,18 @@ facts computed from the two closure graphs, not by an entered number: This is issue #2469's candidate 4 (structure ∧ weight), implemented as the minimal slice: each half alone is refutable — containment without weight accepts a "split" that smuggles a new heavy edge; weight without containment accepts dropping a merge-base module behind an equal-sized -replacement — and together they are the smallest pair that admits healthy extraction and refuses -new eager work. Statements were chosen over bytes because formatting and comment churn move bytes -while statements track the thing being preserved: module-scope evaluation work. Both are proxies; -the planted tests in `scripts/__tests__/closure-growth-rule.test.ts` pin the acceptance pair in -both directions, and the hard ADR-0019 checks remain the non-proxy backstop under it. +replacement — and together they admit healthy extraction while refusing any growth in the +closure's module-scope statement weight. Statements were chosen over bytes because formatting and +comment churn move bytes while statements track the thing being preserved: module-scope +declarations and calls. The guarantee is exactly **flat statement weight over a preserved +closure**, not equality of eager behavior: a split that REPLACES an existing statement with a +more expensive top-level call moves no count and no weight, so it passes — nothing +count-based, in modules or statements, can see a like-for-like replacement. That residue is +bounded by what the tolerance cannot hide: the head still evaluates only merge-base modules plus +what they pull in, the hard ADR-0019 checks (façade exactness, no platform implementation before +binding) stay count- and pattern-based under it, and the added-module listing every failure +prints is what a reviewer reads. The planted tests in +`scripts/__tests__/closure-growth-rule.test.ts` pin the acceptance pair in both directions. Maintenance cost of an ordinary extraction under this rule: zero configuration edits. No row, baseline, or number is touched per split — the merge-base comparison recomputes everything. (The diff --git a/docs/adr/0028-capability-family-cell-vocabulary.md b/docs/adr/0028-capability-family-cell-vocabulary.md index 10509c5f9d..9e9fea840c 100644 --- a/docs/adr/0028-capability-family-cell-vocabulary.md +++ b/docs/adr/0028-capability-family-cell-vocabulary.md @@ -70,9 +70,10 @@ family rather than arguing from the shape: 2. Report the change in files-touched for that family against the recorded baseline of the four recurring eight-package commits. 3. Report eager-closure deltas per entry surface. `scripts/__tests__/eager-closure-budgets.ts` has no - growth-approval path; [ADR 0027](0027-descriptor-root-vs-eager-closure-budget.md) tolerates only a - statement-neutral re-home. A row table is a **new eager value**, so it passes only if its - module-scope weight comes with a statement-neutral re-home somewhere in the same closure — + growth-approval path; [ADR 0027](0027-descriptor-root-vs-eager-closure-budget.md) tolerates + module-count growth only when the head closure preserves every merge-base module and its total + module-scope weight does not grow. A row table is a **new eager value**: it passes only by + satisfying both conditions — preserving the base closure and keeping total weight no greater — otherwise the gate stays red and this ADR should be withdrawn rather than granted an exception. If the fan-out does not visibly shrink, close this as rejected and leave the two authored lists in diff --git a/scripts/__tests__/closure-growth-rule.test.ts b/scripts/__tests__/closure-growth-rule.test.ts index e49165b35b..5490f0ba73 100644 --- a/scripts/__tests__/closure-growth-rule.test.ts +++ b/scripts/__tests__/closure-growth-rule.test.ts @@ -217,6 +217,13 @@ test('the weight counts module-scope work, not wiring or re-exporting', () => { ), 'the scan() call is module-scope work, only the import is wiring', ).toBe(1); + expect( + topLevelStatementCount( + 're-export.ts', + "import { a } from './a.ts';\nexport { a };\nexport const b = 2;\n", + ), + 'a source-less `export { a }` re-names a local binding: the other wiring half of a split', + ).toBe(1); }); test('growth advice names both causes and both remedies, not one prescribed fix', () => { diff --git a/scripts/__tests__/closure-growth-rule.ts b/scripts/__tests__/closure-growth-rule.ts index 5bbb83cf20..747ca37401 100644 --- a/scripts/__tests__/closure-growth-rule.ts +++ b/scripts/__tests__/closure-growth-rule.ts @@ -13,17 +13,18 @@ // (renames canonicalized), so nothing was dropped or swapped -- the growth is purely // addition. // 2. FLAT WEIGHT: the closure's total top-level statement count (`topLevelStatementCount`: -// wiring excluded) did not grow, so the added files carry re-homed declarations, not new -// module-scope work. Any smuggled eager work -- a module-scope call into a new heavy edge -- -// adds statements and stays red. +// wiring excluded) did not grow. Smuggled eager work that ADDS a module-scope statement -- +// a call into a new heavy edge -- raises the weight and stays red. // -// Each alone is insufficient: containment without weight accepts a split that smuggles eager -// work; weight without containment accepts dropping a merge-base module while adding a heavier -// one at the same statement count. Together they are the smallest rule that admits healthy -// extraction and refuses new eager edges, which is why they replace neither the count (the -// comparison still fires first: growth must exist to be tolerated) nor the hard ADR-0019 checks -// (façade exactness, no platform implementation before binding), which stay count- and -// pattern-based and untouched. +// Each alone is insufficient: containment without weight accepts a split that smuggles an +// added statement; weight without containment accepts dropping a merge-base module while +// adding a heavier one at the same statement count. The guarantee the pair states is exactly +// "flat statement weight over a preserved closure" -- a split that REPLACES a statement with +// a more expensive one is invisible to any count, in modules or in statements, and is not +// what this tolerance was built to admit. It replaces neither the count comparison (which +// still fires first: growth must exist to be tolerated) nor the hard ADR-0019 checks (façade +// exactness, no platform implementation before binding), which stay count- and pattern-based +// and untouched. /** * What one entry's no-growth verdict reads. Counts come from the closure graphs; weights are the diff --git a/src/__tests__/eager-import-closure.fixtures.ts b/src/__tests__/eager-import-closure.fixtures.ts index 1d5e34453f..8631f4fbec 100644 --- a/src/__tests__/eager-import-closure.fixtures.ts +++ b/src/__tests__/eager-import-closure.fixtures.ts @@ -129,9 +129,13 @@ function parseEagerModule(fileName: string, source: string): EagerModule { function isWiringOnlyStatement(statement: AstNode): boolean { if (statement.type === 'ImportDeclaration') return true; - // `export * from 'x'` and `export { a } from 'x'` name a module; `export const a = 1` does not. - if (statement.type === 'ExportNamedDeclaration' || statement.type === 'ExportAllDeclaration') { - return statement.source !== undefined && statement.source !== null; + if (statement.type === 'ExportAllDeclaration') return true; // `export * from 'x'` names a module. + if (statement.type === 'ExportNamedDeclaration') { + // `export { a } from 'x'` names a module, and a source-less `export { a }` only re-names a + // local binding -- the re-export half of a split's wiring. What still counts is the shape + // that OWNS a declaration: `export const a = 1`, `export function a() {}`. + if (statement.source != null) return true; + return statement.declaration == null; } return false; } From b1cc79e884b9ba1be48f76e93f167fe4c165443f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 8 Oct 2026 01:21:26 +0200 Subject: [PATCH 4/9] 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. --- ...descriptor-root-vs-eager-closure-budget.md | 59 ++++++---- .../0028-capability-family-cell-vocabulary.md | 6 +- docs/adr/README.md | 2 +- scripts/__tests__/closure-growth-rule.test.ts | 103 +++++++++++++++--- scripts/__tests__/closure-growth-rule.ts | 96 +++++++++++----- .../__tests__/eager-closure-budgets.test.ts | 3 + 6 files changed, 201 insertions(+), 68 deletions(-) diff --git a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md index 06c792ff95..2c466ac8e7 100644 --- a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md +++ b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md @@ -4,9 +4,10 @@ Accepted (2026-10-07, #2469). The conflict below is resolved in favor of allowing **byte-neutral (statement-neutral) re-homing**: the eager-closure budget's NO-GROWTH rule now -tolerates exactly the split shape — the head closure still evaluates every merge-base module and -the closure's total module-scope statement count does not grow. The rule lives in -`scripts/__tests__/closure-growth-rule.ts`, planted in both directions by +tolerates exactly the split shape — the head closure still evaluates every merge-base module, +every module it newly evaluates is new to the merge-base tree (or a rename), and the closure's +total module-scope statement count does not grow. The rule lives in +`scripts/__tests__/closure-growth-rule.ts`, planted in every failing direction by `scripts/__tests__/closure-growth-rule.test.ts`. The hard ADR-0019 checks (façade exactness, no platform implementation before binding) are untouched: the tolerance relaxes the module-count proxy, never the loading property. @@ -33,8 +34,9 @@ the file must be divided, the other says the division must not be visible to any ## Rules at a glance, resolved - A hub split is allowed when it is statement-neutral: every merge-base module stays in every - affected closure, and total module-scope statements do not grow. The gate enforces this; an - unexplained count growth still fails with no approval path, as before. + affected closure, every newly evaluated module is new to the merge-base tree (or a rename), and + total module-scope statements do not grow. The gate enforces this; an unexplained count growth + still fails with no approval path, as before. - Do not add an approved-growth row, a baseline entry, or a suppression to pass the gate. That is the allowlist `AGENTS.md` forbids; the split tolerance is a rule, not a waiver — it decides from the two graphs, and nothing is hand-entered per split. @@ -79,29 +81,37 @@ debt rule's proxy disagree at exactly this shape. ## Decision May a byte-neutral re-homing of a hub module's contents into new modules raise the eager module -count of the entry surfaces that reach it? **Yes — exactly when it is a split**, decided by two -facts computed from the two closure graphs, not by an entered number: +count of the entry surfaces that reach it? **Yes — exactly when it is a split**, decided by three +facts computed from the two closure graphs and the merge-base tree, not by an entered number: 1. **Containment:** the head closure evaluates every module the merge-base closure evaluated - (renames canonicalized), so the growth is purely additive. -2. **Flat weight:** the closure's total top-level statement count — wiring lines excluded, so + (renames canonicalized), so nothing was dropped or swapped. +2. **Added-module novelty:** every module the head closure evaluates that the base closure did + not is absent from the merge-base tree at its rename-canonicalized path. A module the tree + already had, sitting outside the closure, becoming eager — under its own name or a renamed + one — is a new edge, not re-homed code; and without this fact, weight deleted elsewhere in + the closure would fund it. +3. **Flat weight:** the closure's total top-level statement count — wiring lines excluded, so re-homing is invisible to it — does not exceed the merge-base closure's. -This is issue #2469's candidate 4 (structure ∧ weight), implemented as the minimal slice: each -half alone is refutable — containment without weight accepts a "split" that smuggles a new heavy -edge; weight without containment accepts dropping a merge-base module behind an equal-sized -replacement — and together they admit healthy extraction while refusing any growth in the -closure's module-scope statement weight. Statements were chosen over bytes because formatting and -comment churn move bytes while statements track the thing being preserved: module-scope -declarations and calls. The guarantee is exactly **flat statement weight over a preserved -closure**, not equality of eager behavior: a split that REPLACES an existing statement with a -more expensive top-level call moves no count and no weight, so it passes — nothing +This is issue #2469's candidate 4 (structure ∧ weight), with the structure half implemented as +containment + novelty rather than candidate 2's importer-set shape. Any subset is refutable — +containment without novelty accepts pulling a pre-existing module in at flat weight, novelty +without containment accepts dropping a merge-base module, weight without the other two accepts +unrelated shrinkage paying for a smuggled edge — and the three together admit healthy extraction +while refusing any growth in the closure's module-scope statement weight. Statements were chosen +over bytes because formatting and comment churn move bytes while statements track the thing being +preserved: module-scope declarations and calls. The guarantee is exactly **newly evaluated +modules are new files, and total statement weight is flat over a preserved closure**, not +equality of eager behavior: a split that REPLACES an existing statement with a more expensive +top-level call inside an existing file moves no count and no weight, so it passes — nothing count-based, in modules or statements, can see a like-for-like replacement. That residue is -bounded by what the tolerance cannot hide: the head still evaluates only merge-base modules plus -what they pull in, the hard ADR-0019 checks (façade exactness, no platform implementation before -binding) stay count- and pattern-based under it, and the added-module listing every failure -prints is what a reviewer reads. The planted tests in -`scripts/__tests__/closure-growth-rule.test.ts` pin the acceptance pair in both directions. +bounded by what the tolerance cannot hide: every module the head newly evaluates is a file the +merge-base tree did not have (so it cannot smuggle pre-existing implementation into eager scope), +the hard ADR-0019 checks (façade exactness, no platform implementation before binding) stay +count- and pattern-based under it, and the added-module listing every failure prints is what a +reviewer reads. The planted tests in `scripts/__tests__/closure-growth-rule.test.ts` pin all +three failing directions. Maintenance cost of an ordinary extraction under this rule: zero configuration edits. No row, baseline, or number is touched per split — the merge-base comparison recomputes everything. (The @@ -118,6 +128,9 @@ shippable, so an oversized hub is addressable debt rather than accepted state. retirement — maintenance the chosen rule does not. - **Structure alone (#2469 candidate 2):** refuted by the planted smuggle test — a "new" module can carry a new heavy edge, so containment proves nothing about eager work. +- **Containment + weight without added-module novelty (the rule's first draft on #3298):** refuted + by the planted pre-existing-heavy test — deleting statements elsewhere in the closure funds a + static edge to a merge-base module that sat outside it, which is real growth, not a split. - **Weight alone (#2469 candidate 1):** refuted by the planted drop test — a swap can keep the statement total flat while quietly making the closure evaluate different modules. - **Leave the array in place:** the status quo, not a resolution — it leaves the size debt unowned and diff --git a/docs/adr/0028-capability-family-cell-vocabulary.md b/docs/adr/0028-capability-family-cell-vocabulary.md index 9e9fea840c..0bdbfc0e1f 100644 --- a/docs/adr/0028-capability-family-cell-vocabulary.md +++ b/docs/adr/0028-capability-family-cell-vocabulary.md @@ -71,9 +71,9 @@ family rather than arguing from the shape: recurring eight-package commits. 3. Report eager-closure deltas per entry surface. `scripts/__tests__/eager-closure-budgets.ts` has no growth-approval path; [ADR 0027](0027-descriptor-root-vs-eager-closure-budget.md) tolerates - module-count growth only when the head closure preserves every merge-base module and its total - module-scope weight does not grow. A row table is a **new eager value**: it passes only by - satisfying both conditions — preserving the base closure and keeping total weight no greater — + module-count growth only when the head closure preserves every merge-base module, every newly + evaluated module is new to the merge-base tree (or a rename), and total module-scope weight does + not grow. A row table is a **new eager value**: it passes only by satisfying all three conditions; otherwise the gate stays red and this ADR should be withdrawn rather than granted an exception. If the fan-out does not visibly shrink, close this as rejected and leave the two authored lists in diff --git a/docs/adr/README.md b/docs/adr/README.md index 9fa192dc6f..61264e667f 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -28,7 +28,7 @@ | [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 | | [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 | | [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 | -| [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 (containment + flat top-level-statement weight) and nothing else; splitting `packages/command-registry/src/registry.ts` is unblocked | +| [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 | | [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 | | [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 | | [0030 Process Lock Exclusion](0030-process-lock-exclusion.md) | process-lock publication/reclaim/release, retained mutation guards, and the single-protocol upgrade boundary | diff --git a/scripts/__tests__/closure-growth-rule.test.ts b/scripts/__tests__/closure-growth-rule.test.ts index 5490f0ba73..7e6f97827d 100644 --- a/scripts/__tests__/closure-growth-rule.test.ts +++ b/scripts/__tests__/closure-growth-rule.test.ts @@ -20,10 +20,11 @@ import { * The NO-GROWTH rule and its split tolerance (#2469, ADR 0027), tested in both failing * directions. The gate's real-tree lane (`eager-closure-budgets.test.ts`) cannot distinguish a * correct tolerance from a vacuous one while the tree happens to contain no splits, so every - * direction the two facts (containment, flat weight) guard gets its own planted repo here: - * a pure split that must pass, a split smuggling a new heavy edge that must fail, a swap that - * drops a merge-base module that must fail at flat weight, and a rename that must not read as a - * drop. + * direction the three facts (containment, added-module novelty, flat weight) guard gets its own + * planted repo here: a pure split that must pass, a split smuggling a new heavy edge that must + * fail, a split pulling in a PRE-EXISTING closure-external module at weight offset by hub + * shrinkage that must fail, a swap that drops a merge-base module that must fail at flat + * weight, and a rename that must not read as a drop or as a new module. */ test('no-growth fails growth with both counts and passes an equal or smaller closure', () => { @@ -33,6 +34,7 @@ test('no-growth fails growth with both counts and passes an equal or smaller clo baseWeight: 200, headWeight: 200, preservedBaseClosure: true, + addedModulesAreNew: true, ...over, }); expect(classifyGrowth('x.ts', at())).toBeNull(); @@ -50,10 +52,11 @@ test('no-growth fails growth with both counts and passes an equal or smaller clo // --- the split tolerance (#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 (ADR 0027). -// The tolerance is exactly two facts: the head still evaluates EVERY merge-base module, and the -// closure's total top-level-statement weight did not grow. Both failing directions are planted. +// The tolerance is exactly three facts: the head still evaluates EVERY merge-base module, every +// newly evaluated module is new to the tree, and the closure's total top-level-statement weight +// did not grow. Every failing direction is planted. -test('a pure split passes: every base module survives and total weight is flat', () => { +test('a pure split passes: every base module survives, added modules are new, weight flat', () => { // entry -> hub(8 statements) becomes entry -> hub(wiring only) -> part-a + part-b carrying the // declarations: count 2 -> 4, weight unchanged. const at = (over: Partial = {}) => ({ @@ -62,6 +65,7 @@ test('a pure split passes: every base module survives and total weight is flat', baseWeight: 10, headWeight: 10, preservedBaseClosure: true, + addedModulesAreNew: true, ...over, }); expect(classifyGrowth('x.ts', at())).toBeNull(); @@ -75,6 +79,7 @@ test('a split smuggling eager work fails and names both the modules and the stat baseWeight: 10, headWeight: 14, preservedBaseClosure: true, + addedModulesAreNew: true, }); expect(finding).toMatch(/evaluates 4 modules.*merge-base evaluated 2/); expect(finding).toMatch(/adds 2 module\(s\) and 4 top-level statement\(s\)/); @@ -88,17 +93,40 @@ test('growth that drops a merge-base module fails even at flat weight: not a spl baseWeight: 10, headWeight: 10, preservedBaseClosure: false, + addedModulesAreNew: true, }); expect(finding).toMatch(/evaluates 4 modules.*merge-base evaluated 2/); expect(finding).toMatch(/not a split of existing code/); }); +test('a pre-existing module newly made eager fails at flat weight: shrinkage cannot fund it', () => { + // The maintainer-review hole: containment holds and weight is flat only because the PR + // deleted statements elsewhere in the closure; the added module sat in the merge-base tree + // outside the closure, so it is a new eager edge, not re-homed code. + const finding = classifyGrowth('x.ts', { + baseCount: 2, + headCount: 4, + baseWeight: 10, + headWeight: 10, + preservedBaseClosure: true, + addedModulesAreNew: false, + }); + expect(finding).toMatch(/evaluates 4 modules.*merge-base evaluated 2/); + expect(finding).toMatch(/newly evaluates a module the merge-base tree already had/); + expect(finding).toMatch(/shrinkage elsewhere cannot fund a new eager edge/); +}); + /** - * A hub repo at two states: the hub commit carrying three declarations, then the working tree - * holding the pure split of two of them into parts. The smuggle test mutates this state BEFORE - * its first working-tree walk, because the walker memoizes edges per path -- one walk per repo. + * A hub repo at two states: the hub commit carrying three declarations plus any committed + * extras (files that EXIST in the base tree but sit outside the entry's closure), then the + * working tree holding the pure split of two of them into parts. The smuggle tests mutate this + * state BEFORE their first working-tree walk, because the walker memoizes edges per path -- one + * walk per repo. */ -function mkSplitFixtureRepo(prefix: string): { +function mkSplitFixtureRepo( + prefix: string, + committedExtras: Record = {}, +): { repo: string; entry: string; hubTree: SourceTreeReader; @@ -110,6 +138,7 @@ function mkSplitFixtureRepo(prefix: string): { 'packages/demo/src/entry.ts', 'export const a = 1;\nexport const b = 2;\nexport const c = 3;\n', ); + for (const [rel, content] of Object.entries(committedExtras)) write(rel, content); execFileSync('git', ['add', '-A'], { cwd: repo }); execFileSync( 'git', @@ -135,6 +164,7 @@ function evidenceFromHub(entry: string, hubTree: SourceTreeReader): ClosureGrowt headGraph, baseWeights: topLevelStatementWeightsOf(baseGraph.keys(), hubTree), headWeights: topLevelStatementWeightsOf(headGraph.keys()), + baseTree: hubTree, }); } @@ -169,6 +199,33 @@ test('planted split smuggling a new heavy edge fails, naming the smuggled weight ); }); +test('planted split pulling in a PRE-EXISTING closure-external module fails at offset weight', () => { + // The maintainer-review hole: `pre-existing-heavy.ts` is committed at the hub commit but no + // file imports it, so it sits in the merge-base tree OUTSIDE the closure. The split's part-c + // side-effect-imports it, and the weight is offset by merging b and c into one declaration -- + // containment holds and weight is flat, so a two-fact rule passes it. The added module is not + // new to the tree, so the novelty fact fails and the verdict names shrinkage-funding. + const { entry, hubTree, write } = mkSplitFixtureRepo('eager-closure-split-pre-existing-', { + 'packages/demo/src/pre-existing-heavy.ts': 'export const heavy = 42;\n', + }); + write('packages/demo/src/part-b.ts', 'export const bc = { b: 2, c: 3 };\n'); + write('packages/demo/src/part-c.ts', "import './pre-existing-heavy.ts';\n"); + const evidence = evidenceFromHub(entry, hubTree); + expect( + evidence, + 'the two facts this shape defeats both hold: 1+1+0+1 weight against 3, containment intact', + ).toMatchObject({ + baseWeight: 3, + headWeight: 3, + preservedBaseClosure: true, + addedModulesAreNew: false, + }); + const finding = classifyGrowth('demo/entry', evidence); + expect(finding, 'the third fact is the one that refuses it').toMatch( + /newly evaluates a module the merge-base tree already had/, + ); +}); + test('a renamed module inside the growth is not a dropped module', () => { const repo = mkGitFixtureRepo('eager-closure-split-rename-'); const baseEntry = path.join(repo, 'packages/demo/src/entry.ts'); @@ -176,16 +233,15 @@ test('a renamed module inside the growth is not a dropped module', () => { execFileSync('git', ['mv', 'packages/demo/src/entry.ts', 'packages/demo/src/hub.ts'], { cwd: repo, }); - const baseWeights = topLevelStatementWeightsOf( - [baseEntry], - createCommittedSourceTree(repo, 'HEAD'), - ); + const baseTree = createCommittedSourceTree(repo, 'HEAD'); + const baseWeights = topLevelStatementWeightsOf([baseEntry], baseTree); const headWeights = topLevelStatementWeightsOf([headEntry]); const shape = { baseGraph: new Map([[baseEntry, null]]), headGraph: new Map([[headEntry, null]]), baseWeights, headWeights, + baseTree, }; expect( closureGrowthEvidence({ @@ -198,6 +254,22 @@ test('a renamed module inside the growth is not a dropped module', () => { closureGrowthEvidence(shape).preservedBaseClosure, 'without the rename map the same shape reads as a dropped module', ).toBe(false); + // Novelty canonicalization: when the head path of a rename PRE-EXisted in the base tree (a + // rename landing on a path that sat outside the closure), only the rename map can tell the + // rename's head from a pre-existing module newly made eager. + const collidingTree = { exists: (file: string) => file === headEntry }; + expect( + closureGrowthEvidence({ + ...shape, + baseTree: collidingTree, + renamedBaseToHead: new Map([[baseEntry, headEntry]]), + }).addedModulesAreNew, + 'the rename map says the pre-existing head path is the same module, not new eager code', + ).toBe(true); + expect( + closureGrowthEvidence({ ...shape, baseTree: collidingTree }).addedModulesAreNew, + 'without the map the same path reads as a pre-existing module newly made eager', + ).toBe(false); }); test('the weight counts module-scope work, not wiring or re-exporting', () => { @@ -237,6 +309,7 @@ test('growth advice names both causes and both remedies, not one prescribed fix' baseWeight: 200, headWeight: 210, preservedBaseClosure: true, + addedModulesAreNew: true, }) ?? ''; expect(finding).toMatch(/new static edge/); expect(finding).toMatch(/used to load on demand/); diff --git a/scripts/__tests__/closure-growth-rule.ts b/scripts/__tests__/closure-growth-rule.ts index 747ca37401..9b5a81ed0a 100644 --- a/scripts/__tests__/closure-growth-rule.ts +++ b/scripts/__tests__/closure-growth-rule.ts @@ -7,30 +7,37 @@ // module-scope work, and every entry reaching the hub grows by the new files (#2423 had to // defer its extraction; ADR 0027 measured the descriptor-root split at +11 per surface). // -// The tolerance is two facts about the growth, both read from the closure graphs: +// The tolerance is three facts about the growth, all read from the closure graphs and the +// merge-base tree: // // 1. CONTAINMENT: the head closure still evaluates EVERY module the merge-base closure did // (renames canonicalized), so nothing was dropped or swapped -- the growth is purely // addition. -// 2. FLAT WEIGHT: the closure's total top-level statement count (`topLevelStatementCount`: +// 2. ADDED-MODULE NOVELTY: every module the head closure evaluates that the base closure did +// not is absent from the merge-base tree at its rename-canonicalized path. A module the +// merge-base tree already had, sitting outside the closure, becoming eager -- under its own +// name or a new one -- is a NEW EDGE, not re-homed code; without this fact, weight deleted +// elsewhere in the closure would fund it. +// 3. FLAT WEIGHT: the closure's total top-level statement count (`topLevelStatementCount`: // wiring excluded) did not grow. Smuggled eager work that ADDS a module-scope statement -- // a call into a new heavy edge -- raises the weight and stays red. // -// Each alone is insufficient: containment without weight accepts a split that smuggles an -// added statement; weight without containment accepts dropping a merge-base module while -// adding a heavier one at the same statement count. The guarantee the pair states is exactly -// "flat statement weight over a preserved closure" -- a split that REPLACES a statement with -// a more expensive one is invisible to any count, in modules or in statements, and is not -// what this tolerance was built to admit. It replaces neither the count comparison (which -// still fires first: growth must exist to be tolerated) nor the hard ADR-0019 checks (façade -// exactness, no platform implementation before binding), which stay count- and pattern-based -// and untouched. +// Any one or two alone are refutable: containment without novelty accepts pulling a +// pre-existing module in at flat weight; novelty without containment accepts dropping a +// merge-base module; weight without the other two accepts an unrelated shrinkage paying for a +// smuggled edge. The guarantee the triple states is exactly "newly evaluated modules are new +// files, and total statement weight is flat over a preserved closure" -- a split that REPLACES +// a statement with a more expensive one inside an existing file is invisible to any count, in +// modules or in statements, and is not what this tolerance was built to admit. It replaces +// neither the count comparison (which still fires first: growth must exist to be tolerated) +// nor the hard ADR-0019 checks (façade exactness, no platform implementation before binding), +// which stay count- and pattern-based and untouched. /** * What one entry's no-growth verdict reads. Counts come from the closure graphs; weights are the * closures' total top-level statements (`topLevelStatementCount`: wiring excluded, so re-homing - * code between modules is invisible to it) and the subset fact says whether the head still - * evaluates every module the merge-base did. + * code between modules is invisible to it); the subset facts say whether the head still evaluates + * every module the merge-base did and whether every module it evaluates NEWLY is new to the tree. */ export type ClosureGrowthEvidence = { baseCount: number; @@ -39,24 +46,36 @@ export type ClosureGrowthEvidence = { headWeight: number; /** Head evaluates every module of the base closure (renames mapped through `renamedBaseToHead`). */ preservedBaseClosure: boolean; + /** + * Every head-closure module the base closure did not evaluate is absent from the merge-base + * tree at its rename-canonicalized path -- i.e. the added modules are split artifacts, not + * pre-existing code newly made eager, under its own name or a renamed one. + */ + addedModulesAreNew: boolean; }; /** * The one comparison both the real-tree lane and the planted split tests run: counts straight off * the graphs, weights summed over each closure's own tree (a base module's weight read from the - * base tree, its head counterpart from the head tree), and the split's containment shape with - * renames canonicalized so a `git mv` inside a split stays the split it is. + * base tree, its head counterpart from the head tree), and the split's two containment shapes + * with renames canonicalized so a `git mv` inside a split stays the split it is. */ export function closureGrowthEvidence(params: { baseGraph: ReadonlyMap; headGraph: ReadonlyMap; baseWeights: ReadonlyMap; headWeights: ReadonlyMap; + /** The committed merge-base tree, for the question every added module must answer: did you exist? */ + baseTree: { exists(file: string): boolean }; renamedBaseToHead?: ReadonlyMap; }): ClosureGrowthEvidence { - const { baseGraph, headGraph, baseWeights, headWeights, renamedBaseToHead } = params; + const { baseGraph, headGraph, baseWeights, headWeights, baseTree, renamedBaseToHead } = params; const sum = (files: Iterable, weights: ReadonlyMap) => [...files].reduce((total, file) => total + (weights.get(file) ?? 0), 0); + const baseOfRenamedHead = new Map(); + for (const [baseFile, headFile] of renamedBaseToHead ?? []) { + baseOfRenamedHead.set(headFile, baseFile); + } return { baseCount: baseGraph.size, headCount: headGraph.size, @@ -65,12 +84,21 @@ export function closureGrowthEvidence(params: { preservedBaseClosure: [...baseGraph.keys()].every((baseFile) => headGraph.has(renamedBaseToHead?.get(baseFile) ?? baseFile), ), + // A module the base closure evaluated (under its pre-rename path) is not an added one; + // every other head module must be absent from the merge-base tree AT ITS CANONICALIZED + // PATH. A rename head is checked at its BASE path, which the merge-base necessarily has -- + // so moving a closure-external module into the closure under a new name reads as what it + // is: pre-existing code newly made eager, not a split artifact. + addedModulesAreNew: [...headGraph.keys()].every((headFile) => { + const baseFile = baseOfRenamedHead.get(headFile) ?? headFile; + return baseGraph.has(baseFile) || !baseTree.exists(baseFile); + }), }; } /** * The no-growth verdict: `null` unless the head closure is larger than the merge-base one and the - * growth is not a tolerated pure split (containment plus flat weight, per the header). + * growth fails one of the three split facts (containment, added-module novelty, flat weight). * * 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 @@ -80,17 +108,33 @@ export function closureGrowthEvidence(params: { * (`describeClosureGrowth`) is for. */ export function classifyGrowth(id: string, evidence: ClosureGrowthEvidence): string | null { - const { baseCount, headCount, baseWeight, headWeight, preservedBaseClosure } = evidence; + const { baseCount, headCount, baseWeight, headWeight, preservedBaseClosure, addedModulesAreNew } = + evidence; if (headCount <= baseCount) return null; - if (preservedBaseClosure && headWeight <= baseWeight) return null; - const weightNote = preservedBaseClosure - ? ` It adds ${headCount - baseCount} module(s) and ${ - headWeight - baseWeight - } top-level statement(s) of eager work on top of every module the merge-base already evaluated, which a pure split does not.` - : ' It also stops evaluating a module the merge-base did, so it is not a split of existing code.'; + if (preservedBaseClosure && addedModulesAreNew && headWeight <= baseWeight) return null; + const reasons: string[] = []; + if (!preservedBaseClosure) { + reasons.push( + 'It stops evaluating a module the merge-base did, so it is not a split of existing code.', + ); + } + if (!addedModulesAreNew) { + reasons.push( + 'It newly evaluates a module the merge-base tree already had, which no split of existing code explains; shrinkage elsewhere cannot fund a new eager edge.', + ); + } + if (headWeight > baseWeight) { + reasons.push( + preservedBaseClosure && addedModulesAreNew + ? `It adds ${headCount - baseCount} module(s) and ${ + headWeight - baseWeight + } top-level statement(s) of eager work on top of every module the merge-base already evaluated, which a pure split does not.` + : `It adds ${headWeight - baseWeight} top-level statement(s) of eager work beyond the merge-base closure.`, + ); + } return ( - `${id} evaluates ${headCount} modules on import; the merge-base evaluated ${baseCount}.` + - weightNote + + `${id} evaluates ${headCount} modules on import; the merge-base evaluated ${baseCount}. ` + + reasons.join(' ') + ' 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 ' + diff --git a/scripts/__tests__/eager-closure-budgets.test.ts b/scripts/__tests__/eager-closure-budgets.test.ts index 83a2a996a1..c7ca4d4991 100644 --- a/scripts/__tests__/eager-closure-budgets.test.ts +++ b/scripts/__tests__/eager-closure-budgets.test.ts @@ -86,6 +86,7 @@ function grew(baseCount: number): ClosureGrowthEvidence { baseWeight: 10, headWeight: 14, preservedBaseClosure: true, + addedModulesAreNew: true, }; } @@ -101,6 +102,7 @@ function passed(params: { baseWeight: 10, headWeight: 10, preservedBaseClosure: params.preservedBaseClosure ?? true, + addedModulesAreNew: true, }; } @@ -659,6 +661,7 @@ const carriedGrowth = carried.map((entry) => { headGraph: graph, baseWeights: topLevelStatementWeightsOf(baseGraph.keys(), baseTree), headWeights: headWeightsOf(graph), + baseTree, renamedBaseToHead, }), }; From efb0176ad286b7d321ab7a9f5bb70be537954bd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 8 Oct 2026 01:54:19 +0200 Subject: [PATCH 5/9] 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. --- ...descriptor-root-vs-eager-closure-budget.md | 52 +++--- scripts/__tests__/closure-growth-rule.test.ts | 170 ++++++++++++++++-- scripts/__tests__/closure-growth-rule.ts | 81 +++++++-- scripts/__tests__/committed-source-tree.ts | 14 ++ .../__tests__/eager-closure-budgets.test.ts | 14 ++ .../eager-import-closure.fixtures.ts | 62 ++++++- 6 files changed, 344 insertions(+), 49 deletions(-) diff --git a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md index 2c466ac8e7..67b0ca9d90 100644 --- a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md +++ b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md @@ -5,9 +5,11 @@ Accepted (2026-10-07, #2469). The conflict below is resolved in favor of allowing **byte-neutral (statement-neutral) re-homing**: the eager-closure budget's NO-GROWTH rule now tolerates exactly the split shape — the head closure still evaluates every merge-base module, -every module it newly evaluates is new to the merge-base tree (or a rename), and the closure's -total module-scope statement count does not grow. The rule lives in -`scripts/__tests__/closure-growth-rule.ts`, planted in every failing direction by +every module it newly evaluates is new to the merge-base tree and to its deleted sources (a +`git mv` inside the base closure stays the same module through rename canonicalization, which +preserves modules already in the closure and is no pass for a pre-existing external module +moved in), and the closure's total module-scope statement count does not grow. The rule lives +in `scripts/__tests__/closure-growth-rule.ts`, planted in every failing direction by `scripts/__tests__/closure-growth-rule.test.ts`. The hard ADR-0019 checks (façade exactness, no platform implementation before binding) are untouched: the tolerance relaxes the module-count proxy, never the loading property. @@ -34,9 +36,11 @@ the file must be divided, the other says the division must not be visible to any ## Rules at a glance, resolved - A hub split is allowed when it is statement-neutral: every merge-base module stays in every - affected closure, every newly evaluated module is new to the merge-base tree (or a rename), and - total module-scope statements do not grow. The gate enforces this; an unexplained count growth - still fails with no approval path, as before. + affected closure, every newly evaluated module is new to the merge-base tree and to its + deleted sources (rename canonicalization preserves modules already in the base closure — it + does not make a moved-in external module novel), and total module-scope statements do not + grow. The gate enforces this; an unexplained count growth still fails with no approval path, + as before. - Do not add an approved-growth row, a baseline entry, or a suppression to pass the gate. That is the allowlist `AGENTS.md` forbids; the split tolerance is a rule, not a waiver — it decides from the two graphs, and nothing is hand-entered per split. @@ -87,10 +91,13 @@ facts computed from the two closure graphs and the merge-base tree, not by an en 1. **Containment:** the head closure evaluates every module the merge-base closure evaluated (renames canonicalized), so nothing was dropped or swapped. 2. **Added-module novelty:** every module the head closure evaluates that the base closure did - not is absent from the merge-base tree at its rename-canonicalized path. A module the tree - already had, sitting outside the closure, becoming eager — under its own name or a renamed - one — is a new edge, not re-homed code; and without this fact, weight deleted elsewhere in - the closure would fund it. + not is absent from the merge-base tree at its rename-canonicalized path AND carries no code + from a source deleted since the merge-base. A module the tree already had, sitting outside + the closure, becoming eager — under its own name, a renamed one, or a rewritten move that + evades git's `-M` similarity pairing — is a new edge, not re-homed code; and without this + fact, weight deleted elsewhere in the closure would fund it. Rename canonicalization + preserves modules ALREADY IN the base closure; it is not a pass for an external module + moved in, which fails the check at the base path the merge-base necessarily still has. 3. **Flat weight:** the closure's total top-level statement count — wiring lines excluded, so re-homing is invisible to it — does not exceed the merge-base closure's. @@ -102,16 +109,17 @@ unrelated shrinkage paying for a smuggled edge — and the three together admit while refusing any growth in the closure's module-scope statement weight. Statements were chosen over bytes because formatting and comment churn move bytes while statements track the thing being preserved: module-scope declarations and calls. The guarantee is exactly **newly evaluated -modules are new files, and total statement weight is flat over a preserved closure**, not -equality of eager behavior: a split that REPLACES an existing statement with a more expensive -top-level call inside an existing file moves no count and no weight, so it passes — nothing -count-based, in modules or statements, can see a like-for-like replacement. That residue is -bounded by what the tolerance cannot hide: every module the head newly evaluates is a file the -merge-base tree did not have (so it cannot smuggle pre-existing implementation into eager scope), -the hard ADR-0019 checks (façade exactness, no platform implementation before binding) stay -count- and pattern-based under it, and the added-module listing every failure prints is what a -reviewer reads. The planted tests in `scripts/__tests__/closure-growth-rule.test.ts` pin all -three failing directions. +modules are new code — new paths carrying no deleted source's statements — and total statement +weight is flat over a preserved closure**, not equality of eager behavior: a split that REPLACES +an existing statement with a more expensive top-level call inside an existing file moves no +count and no weight, so it passes — nothing count-based, in modules or statements, can see a +like-for-like replacement. That residue is bounded by what the tolerance cannot hide: every +module the head newly evaluates is a path the merge-base tree did not have AND carries none of +the statements of any source deleted since the merge-base (so it cannot smuggle pre-existing +implementation into eager scope, rewritten or not), the hard ADR-0019 checks (façade exactness, +no platform implementation before binding) stay count- and pattern-based under it, and the +added-module listing every failure prints is what a reviewer reads. The planted tests in +`scripts/__tests__/closure-growth-rule.test.ts` pin every failing direction. Maintenance cost of an ordinary extraction under this rule: zero configuration edits. No row, baseline, or number is touched per split — the merge-base comparison recomputes everything. (The @@ -131,6 +139,10 @@ shippable, so an oversized hub is addressable debt rather than accepted state. - **Containment + weight without added-module novelty (the rule's first draft on #3298):** refuted by the planted pre-existing-heavy test — deleting statements elsewhere in the closure funds a static edge to a merge-base module that sat outside it, which is real growth, not a split. +- **Novelty by merge-base tree path alone (the rule's second draft on #3298):** refuted by the + planted rewritten-move test — a move that changes the path and churns formatting reports to + git as delete + add, so an absent destination is not proof of new code. The deleted sources' + statement texts are the heuristic-free provenance that closes it. - **Weight alone (#2469 candidate 1):** refuted by the planted drop test — a swap can keep the statement total flat while quietly making the closure evaluate different modules. - **Leave the array in place:** the status quo, not a resolution — it leaves the size debt unowned and diff --git a/scripts/__tests__/closure-growth-rule.test.ts b/scripts/__tests__/closure-growth-rule.test.ts index 7e6f97827d..ec0a713e96 100644 --- a/scripts/__tests__/closure-growth-rule.test.ts +++ b/scripts/__tests__/closure-growth-rule.test.ts @@ -4,11 +4,12 @@ import fs from 'node:fs'; import path from 'node:path'; import { eagerClosureGraphOf, + statementTextsOf, topLevelStatementCount, topLevelStatementWeightsOf, type SourceTreeReader, } from '../../src/__tests__/eager-import-closure.fixtures.ts'; -import { createCommittedSourceTree } from './committed-source-tree.ts'; +import { createCommittedSourceTree, deletedSourcesSince } from './committed-source-tree.ts'; import { mkGitFixtureRepo } from './eager-closure-gate-fixtures.ts'; import { classifyGrowth, @@ -112,7 +113,7 @@ test('a pre-existing module newly made eager fails at flat weight: shrinkage can addedModulesAreNew: false, }); expect(finding).toMatch(/evaluates 4 modules.*merge-base evaluated 2/); - expect(finding).toMatch(/newly evaluates a module the merge-base tree already had/); + expect(finding).toMatch(/newly evaluates code the merge-base tree already had/); expect(finding).toMatch(/shrinkage elsewhere cannot fund a new eager edge/); }); @@ -156,7 +157,12 @@ function mkSplitFixtureRepo( return { repo, entry: path.join(repo, 'packages/demo/src/entry.ts'), hubTree, write }; } -function evidenceFromHub(entry: string, hubTree: SourceTreeReader): ClosureGrowthEvidence { +function evidenceFromHub( + entry: string, + hubTree: SourceTreeReader, + repo: string, + baseRef = 'HEAD', +): ClosureGrowthEvidence { const baseGraph = eagerClosureGraphOf(entry, hubTree); const headGraph = eagerClosureGraphOf(entry); return closureGrowthEvidence({ @@ -164,13 +170,20 @@ function evidenceFromHub(entry: string, hubTree: SourceTreeReader): ClosureGrowt headGraph, baseWeights: topLevelStatementWeightsOf(baseGraph.keys(), hubTree), headWeights: topLevelStatementWeightsOf(headGraph.keys()), + headStatementTexts: statementTextsOf(headGraph.keys()), baseTree: hubTree, + deletedSourceTexts: [ + ...statementTextsOf( + deletedSourcesSince(repo, baseRef).map((file) => path.join(repo, file)), + hubTree, + ).values(), + ], }); } test('planted pure split passes end to end: count grows, every base module survives, weight flat', () => { - const { entry, hubTree } = mkSplitFixtureRepo('eager-closure-split-pure-'); - const evidence = evidenceFromHub(entry, hubTree); + const { entry, hubTree, repo } = mkSplitFixtureRepo('eager-closure-split-pure-'); + const evidence = evidenceFromHub(entry, hubTree, repo); expect(evidence, 'one module became three: the count grows by two').toMatchObject({ baseCount: 1, headCount: 3, @@ -186,13 +199,13 @@ test('planted pure split passes end to end: count grows, every base module survi }); test('planted split smuggling a new heavy edge fails, naming the smuggled weight', () => { - const { entry, hubTree, write } = mkSplitFixtureRepo('eager-closure-split-smuggled-'); + const { entry, hubTree, write, repo } = mkSplitFixtureRepo('eager-closure-split-smuggled-'); write( 'packages/demo/src/part-c.ts', "import { scan } from './scanner.ts';\nexport const c = scan();\n", ); write('packages/demo/src/scanner.ts', 'export function scan() {\n return 4;\n}\n'); - const evidence = evidenceFromHub(entry, hubTree); + const evidence = evidenceFromHub(entry, hubTree, repo); expect(evidence.headWeight, 'scan() runs at module scope: that is eager work').toBe(4); expect(classifyGrowth('demo/entry', evidence)).toMatch( /adds 3 module\(s\) and 1 top-level statement\(s\)/, @@ -205,12 +218,12 @@ test('planted split pulling in a PRE-EXISTING closure-external module fails at o // side-effect-imports it, and the weight is offset by merging b and c into one declaration -- // containment holds and weight is flat, so a two-fact rule passes it. The added module is not // new to the tree, so the novelty fact fails and the verdict names shrinkage-funding. - const { entry, hubTree, write } = mkSplitFixtureRepo('eager-closure-split-pre-existing-', { + const { entry, hubTree, write, repo } = mkSplitFixtureRepo('eager-closure-split-pre-existing-', { 'packages/demo/src/pre-existing-heavy.ts': 'export const heavy = 42;\n', }); write('packages/demo/src/part-b.ts', 'export const bc = { b: 2, c: 3 };\n'); write('packages/demo/src/part-c.ts', "import './pre-existing-heavy.ts';\n"); - const evidence = evidenceFromHub(entry, hubTree); + const evidence = evidenceFromHub(entry, hubTree, repo); expect( evidence, 'the two facts this shape defeats both hold: 1+1+0+1 weight against 3, containment intact', @@ -222,10 +235,146 @@ test('planted split pulling in a PRE-EXISTING closure-external module fails at o }); const finding = classifyGrowth('demo/entry', evidence); expect(finding, 'the third fact is the one that refuses it').toMatch( - /newly evaluates a module the merge-base tree already had/, + /newly evaluates code the merge-base tree already had/, ); }); +test('a moved-in pre-existing module survives only as a detected rename', () => { + // The rename-canonicalization scope, pinned: canonicalization preserves modules ALREADY IN + // the base closure. Here `facades/top.ts` sat outside the entry's closure at the merge-base; + // the PR renames it (a `git mv` so `-M` pairs it) under a new name and imports it. At the + // HEAD path it is new to the tree, so a head-path check would pass it; checking the + // canonicalized BASE path -- where the merge-base necessarily has it -- reads it as what it + // is: pre-existing code newly made eager. + const repo = mkGitFixtureRepo('eager-closure-split-move-in-'); + fs.writeFileSync( + path.join(repo, 'packages/demo/src/entry.ts'), + "import { b } from './facades/nested/moved.ts';\nexport { b };\n", + ); + execFileSync( + 'git', + ['mv', 'packages/demo/src/facades/top.ts', 'packages/demo/src/facades/nested/moved.ts'], + { + cwd: repo, + }, + ); + const baseTree = createCommittedSourceTree(repo, 'HEAD'); + const entry = path.join(repo, 'packages/demo/src/entry.ts'); + const evidence = closureGrowthEvidence({ + baseGraph: eagerClosureGraphOf(entry, baseTree), + headGraph: eagerClosureGraphOf(entry), + baseWeights: topLevelStatementWeightsOf(eagerClosureGraphOf(entry, baseTree).keys(), baseTree), + headWeights: topLevelStatementWeightsOf(eagerClosureGraphOf(entry).keys()), + headStatementTexts: statementTextsOf(eagerClosureGraphOf(entry).keys()), + baseTree, + renamedBaseToHead: new Map([ + [ + path.join(repo, 'packages/demo/src/facades/top.ts'), + path.join(repo, 'packages/demo/src/facades/nested/moved.ts'), + ], + ]), + }); + expect( + evidence, + 'count 1 -> 2 at flat weight: path-level novelty is what refuses it', + ).toMatchObject({ + baseWeight: 1, + headWeight: 1, + preservedBaseClosure: true, + addedModulesAreNew: false, + }); +}); + +test('planted rewritten move of a pre-existing heavy module fails at offset weight, beyond -M', () => { + // The Cubic follow-up: a PR deletes `heavy.ts` (committed, closure-external), rewrites it into + // a much smaller `moved-heavy.ts` under a new path -- similar enough to still carry real code, + // different enough that git's `-M` reports D + A rather than R -- imports it from the split's + // part-c, and pays for the carried statements by flattening the hub's own declarations. Path + // novelty alone reads moved-heavy.ts as brand new; the deleted-source content comparison is + // what refuses it. + const repo = mkGitFixtureRepo('eager-closure-split-rewritten-move-'); + const write = (rel: string, content: string) => fs.writeFileSync(path.join(repo, rel), content); + write( + 'packages/demo/src/entry.ts', + 'export const a = 1;\nexport const b = 2;\nexport const c = 3;\n', + ); + write( + 'packages/demo/src/heavy.ts', + [ + 'const tableA = { alpha: 1, beta: 2 };', + 'const tableB = { gamma: 3, delta: 4 };', + 'const tableC = { epsilon: 5, zeta: 6 };', + 'function padOne() {', + ' return tableA.alpha;', + '}', + 'function padTwo() {', + ' return tableB.gamma;', + '}', + 'function padThree() {', + ' return tableC.epsilon;', + '}', + 'function padFour() {', + ' return tableA.beta + tableB.delta;', + '}', + 'function padFive() {', + ' return tableC.zeta;', + '}', + 'export function lookupA(key: string) {', + ' return tableA[key];', + '}', + 'export function lookupB(key: string) {', + ' return tableB[key];', + '}', + 'export function lookupC(key: string) {', + ' return tableC[key];', + '}', + '', + ].join('\n'), + ); + execFileSync('git', ['add', '-A'], { cwd: repo }); + execFileSync( + 'git', + ['-c', 'user.name=Gate', '-c', 'user.email=gate@example.test', 'commit', '-qm', 'hub'], + { cwd: repo }, + ); + const hubTree = createCommittedSourceTree(repo, 'HEAD'); + // The rewritten move: only two statements survive, one with layout churn and one with a + // re-export, so `-M` calls it delete + new file. + fs.rmSync(path.join(repo, 'packages/demo/src/heavy.ts')); + write( + 'packages/demo/src/moved-heavy.ts', + '// relocated from heavy.ts during the split\n\n\nconst tableA = { alpha: 1, beta: 2 };\n\n\n/** the surviving lookup */\nexport function lookupA(key: string) {\n\n\n return tableA[key];\n}\n', + ); + // Hub flattened to wiring; b and c merged into one declaration: the offset payment. + write( + 'packages/demo/src/entry.ts', + "export * from './part-b.ts';\nexport * from './part-c.ts';\n", + ); + write('packages/demo/src/part-b.ts', 'export const bc = { b: 2, c: 3 };\n'); + write('packages/demo/src/part-c.ts', "import './moved-heavy.ts';\n"); + const renameStatus = execFileSync('git', ['diff', '--name-status', '-M', 'HEAD'], { + cwd: repo, + encoding: 'utf8', + }); + expect( + renameStatus, + 'the planted evasion must hold: -M must NOT pair the move, or this tests rename handling', + ).toMatch(/^D\tpackages\/demo\/src\/heavy\.ts$/m); + const entry = path.join(repo, 'packages/demo/src/entry.ts'); + const evidence = evidenceFromHub(entry, hubTree, repo); + expect( + evidence, + 'count 1 -> 4 at flat weight 3 -> 3 with containment intact: only content provenance refuses it', + ).toMatchObject({ + baseWeight: 3, + headWeight: 3, + preservedBaseClosure: true, + addedModulesAreNew: false, + }); + const finding = classifyGrowth('demo/entry', evidence); + expect(finding).toMatch(/newly evaluates code the merge-base tree already had/); +}); + test('a renamed module inside the growth is not a dropped module', () => { const repo = mkGitFixtureRepo('eager-closure-split-rename-'); const baseEntry = path.join(repo, 'packages/demo/src/entry.ts'); @@ -241,6 +390,7 @@ test('a renamed module inside the growth is not a dropped module', () => { headGraph: new Map([[headEntry, null]]), baseWeights, headWeights, + headStatementTexts: statementTextsOf([headEntry]), baseTree, }; expect( diff --git a/scripts/__tests__/closure-growth-rule.ts b/scripts/__tests__/closure-growth-rule.ts index 9b5a81ed0a..5ecb324ce6 100644 --- a/scripts/__tests__/closure-growth-rule.ts +++ b/scripts/__tests__/closure-growth-rule.ts @@ -14,10 +14,13 @@ // (renames canonicalized), so nothing was dropped or swapped -- the growth is purely // addition. // 2. ADDED-MODULE NOVELTY: every module the head closure evaluates that the base closure did -// not is absent from the merge-base tree at its rename-canonicalized path. A module the -// merge-base tree already had, sitting outside the closure, becoming eager -- under its own -// name or a new one -- is a NEW EDGE, not re-homed code; without this fact, weight deleted -// elsewhere in the closure would fund it. +// not is absent from the merge-base tree AT ITS RENAME-CANONICALIZED PATH and carries no +// statement text from a source deleted since the merge-base. A module the merge-base tree +// already had, sitting outside the closure, becoming eager -- under its own name, a new +// one, or a rewritten move `-M` cannot pair -- is a NEW EDGE, not re-homed code; without +// this fact, weight deleted elsewhere in the closure would fund it. Canonicalization +// preserves modules ALREADY IN the base closure; a moved-in external module fails novelty +// at the base path the merge-base still has. // 3. FLAT WEIGHT: the closure's total top-level statement count (`topLevelStatementCount`: // wiring excluded) did not grow. Smuggled eager work that ADDS a module-scope statement -- // a call into a new heavy edge -- raises the weight and stays red. @@ -26,12 +29,13 @@ // pre-existing module in at flat weight; novelty without containment accepts dropping a // merge-base module; weight without the other two accepts an unrelated shrinkage paying for a // smuggled edge. The guarantee the triple states is exactly "newly evaluated modules are new -// files, and total statement weight is flat over a preserved closure" -- a split that REPLACES -// a statement with a more expensive one inside an existing file is invisible to any count, in -// modules or in statements, and is not what this tolerance was built to admit. It replaces -// neither the count comparison (which still fires first: growth must exist to be tolerated) -// nor the hard ADR-0019 checks (façade exactness, no platform implementation before binding), -// which stay count- and pattern-based and untouched. +// CODE -- new paths carrying no deleted source's statements -- and total statement weight is +// flat over a preserved closure" -- a split that REPLACES a statement with a more expensive +// one inside an existing file is invisible to any count, in modules or in statements, and is +// not what this tolerance was built to admit. It replaces neither the count comparison (which +// still fires first: growth must exist to be tolerated) nor the hard ADR-0019 checks (façade +// exactness, no platform implementation before binding), which stay count- and pattern-based +// and untouched. /** * What one entry's no-growth verdict reads. Counts come from the closure graphs; weights are the @@ -65,11 +69,30 @@ export function closureGrowthEvidence(params: { headGraph: ReadonlyMap; baseWeights: ReadonlyMap; headWeights: ReadonlyMap; + /** Statement texts of each head-closure file, for the deleted-source comparison below. */ + headStatementTexts: ReadonlyMap; /** The committed merge-base tree, for the question every added module must answer: did you exist? */ baseTree: { exists(file: string): boolean }; renamedBaseToHead?: ReadonlyMap; + /** + * Weight-bearing statement texts of each merge-base production source DELETED since the + * merge-base (`deletedSourcesSince`, which deliberately skips `-M`, mapped through + * `statementTextsOf`). Git's rename detection is a similarity heuristic and a rewritten move + * evades it, showing up as delete + brand-new path; content is not a heuristic, so an added + * module carrying a deleted source's code is a move of pre-existing code, not a new file. + */ + deletedSourceTexts?: readonly (readonly string[])[]; }): ClosureGrowthEvidence { - const { baseGraph, headGraph, baseWeights, headWeights, baseTree, renamedBaseToHead } = params; + const { + baseGraph, + headGraph, + baseWeights, + headWeights, + headStatementTexts, + baseTree, + renamedBaseToHead, + deletedSourceTexts = [], + } = params; const sum = (files: Iterable, weights: ReadonlyMap) => [...files].reduce((total, file) => total + (weights.get(file) ?? 0), 0); const baseOfRenamedHead = new Map(); @@ -84,18 +107,40 @@ export function closureGrowthEvidence(params: { preservedBaseClosure: [...baseGraph.keys()].every((baseFile) => headGraph.has(renamedBaseToHead?.get(baseFile) ?? baseFile), ), - // A module the base closure evaluated (under its pre-rename path) is not an added one; - // every other head module must be absent from the merge-base tree AT ITS CANONICALIZED - // PATH. A rename head is checked at its BASE path, which the merge-base necessarily has -- - // so moving a closure-external module into the closure under a new name reads as what it - // is: pre-existing code newly made eager, not a split artifact. + // A module the base closure evaluated (under its pre-rename path) is not an added one -- + // canonicalization preserves modules ALREADY IN the base closure, and is not a pass for a + // pre-existing external module moved in under a new name: that fails here on its + // canonicalized path, or, when the move was rewritten past `-M`, on its content. addedModulesAreNew: [...headGraph.keys()].every((headFile) => { const baseFile = baseOfRenamedHead.get(headFile) ?? headFile; - return baseGraph.has(baseFile) || !baseTree.exists(baseFile); + if (baseGraph.has(baseFile)) return true; + if (baseTree.exists(baseFile)) return false; + return !movedFromDeletedSource(headStatementTexts.get(headFile) ?? [], deletedSourceTexts); }), }; } +/** + * Two identical weight-bearing statements from the SAME deleted source is a move signal; one + * shared line is coincidence (short constants like a schema version repeat across the repo), so + * a single match does not brand a new file as pre-existing code. + */ +function movedFromDeletedSource( + texts: readonly string[], + deletedSourceTexts: readonly (readonly string[])[], +): boolean { + if (texts.length === 0) return false; + const distinct = new Set(texts); + return deletedSourceTexts.some((deleted) => { + const deletedSet = new Set(deleted); + let shared = 0; + for (const text of distinct) { + if (deletedSet.has(text) && ++shared >= 2) return true; + } + return false; + }); +} + /** * The no-growth verdict: `null` unless the head closure is larger than the merge-base one and the * growth fails one of the three split facts (containment, added-module novelty, flat weight). @@ -120,7 +165,7 @@ export function classifyGrowth(id: string, evidence: ClosureGrowthEvidence): str } if (!addedModulesAreNew) { reasons.push( - 'It newly evaluates a module the merge-base tree already had, which no split of existing code explains; shrinkage elsewhere cannot fund a new eager edge.', + 'It newly evaluates code the merge-base tree already had -- at that path, or in a source deleted since the merge-base -- which no split of existing code explains; shrinkage elsewhere cannot fund a new eager edge.', ); } if (headWeight > baseWeight) { diff --git a/scripts/__tests__/committed-source-tree.ts b/scripts/__tests__/committed-source-tree.ts index 0a8f39b4c6..3f340d527c 100644 --- a/scripts/__tests__/committed-source-tree.ts +++ b/scripts/__tests__/committed-source-tree.ts @@ -54,6 +54,20 @@ export function renamedSince(repoRoot: string, base: string): ReadonlyMap file !== '' && WALKED_SOURCE.test(file) && isProductionSourceFile(file)); +} + /** A carried entry must have been a source the base closure walker could actually read. */ export function baseProductionPathOf( repoRoot: string, diff --git a/scripts/__tests__/eager-closure-budgets.test.ts b/scripts/__tests__/eager-closure-budgets.test.ts index c7ca4d4991..f48f1a79f8 100644 --- a/scripts/__tests__/eager-closure-budgets.test.ts +++ b/scripts/__tests__/eager-closure-budgets.test.ts @@ -4,11 +4,13 @@ import fs from 'node:fs'; import path from 'node:path'; import { eagerClosureGraphOf, + statementTextsOf, topLevelStatementWeightsOf, } from '../../src/__tests__/eager-import-closure.fixtures.ts'; import { baseProductionPathOf, createCommittedSourceTree, + deletedSourcesSince, headCommit, mergeBaseWithMain, renamedSince, @@ -639,6 +641,16 @@ function headWeightsOf(graph: ReadonlyMap): Map { headGraph: graph, baseWeights: topLevelStatementWeightsOf(baseGraph.keys(), baseTree), headWeights: headWeightsOf(graph), + headStatementTexts: statementTextsOf(graph.keys()), baseTree, renamedBaseToHead, + deletedSourceTexts, }), }; }); diff --git a/src/__tests__/eager-import-closure.fixtures.ts b/src/__tests__/eager-import-closure.fixtures.ts index 8631f4fbec..384ccb1078 100644 --- a/src/__tests__/eager-import-closure.fixtures.ts +++ b/src/__tests__/eager-import-closure.fixtures.ts @@ -115,7 +115,7 @@ export function topLevelStatementCount(fileName: string, source: string): number return parseEagerModule(fileName, source).topLevelStatements; } -type EagerModule = { specifiers: string[]; topLevelStatements: number }; +type EagerModule = { specifiers: string[]; topLevelStatements: number; statementTexts: string[] }; function parseEagerModule(fileName: string, source: string): EagerModule { const parsed = parseSync(fileName, source); @@ -124,9 +124,39 @@ function parseEagerModule(fileName: string, source: string): EagerModule { return { specifiers: [...new Set([...staticEvaluatedRefs(parsed.module), ...dynamic])], topLevelStatements: countTopLevelStatements(parsed.program), + statementTexts: statementTexts(parsed.program, source), }; } +function statementTexts(program: unknown, source: string): string[] { + const body = (program as { body?: unknown[] } | undefined)?.body; + if (!Array.isArray(body)) return []; + const texts: string[] = []; + for (const statement of body) { + if (statement === null || typeof statement !== 'object') continue; + const node = statement as AstNode; + if (isWiringOnlyStatement(node)) continue; + const start = node.start; + const end = node.end; + if (typeof start !== 'number' || typeof end !== 'number') continue; + texts.push(normalizeStatementText(source.slice(start, end))); + } + return texts; +} + +/** + * Comments sit outside statement spans, so a move that rewrites only comments and layout keeps + * texts equal; the `export` keyword is stripped because re-exporting a moved declaration is + * still the same module-scope code. + */ +function normalizeStatementText(text: string): string { + return text + .replaceAll(/\s+/g, ' ') + .trim() + .replace(/^export\s+default\s+/, '') + .replace(/^export\s+/, ''); +} + function isWiringOnlyStatement(statement: AstNode): boolean { if (statement.type === 'ImportDeclaration') return true; if (statement.type === 'ExportAllDeclaration') return true; // `export * from 'x'` names a module. @@ -241,6 +271,8 @@ type TreeMemo = { directEdges: Map; /** Weight of each walked file, memoized with its edges (#2469). */ weights: Map; + /** Statement texts of each read file, memoized beside the weights (#3298 review). */ + statementTexts: Map; }; const treeMemos = new WeakMap(); @@ -251,6 +283,7 @@ function memoOf(tree: SourceTreeReader): TreeMemo { packageDirs: readWorkspacePackageDirs(tree), directEdges: new Map(), weights: new Map(), + statementTexts: new Map(), }; treeMemos.set(tree, memo); } @@ -285,6 +318,24 @@ export function topLevelStatementWeightsOf( return weights; } +/** + * Per-file weight-bearing statement texts, as whitespace-normalized source text with the same + * exclusions as {@link topLevelStatementCount} plus a leading `export` keyword, so a declaration + * re-exported when it moved still matches the text it had as a private binding. Two files whose + * texts agree carried the same module-scope code; that is the equality the split tolerance's + * deleted-source provenance uses (#3298 review), so a move that evades git's rename detection + * still reads as pre-existing code. Comments sit outside statement spans, and an `export` + * keyword or reformatting is invisible to it, so only rewritten code differs. + */ +export function statementTextsOf( + files: Iterable, + tree: SourceTreeReader = workingTreeReader, +): Map { + const texts = new Map(); + for (const file of files) texts.set(file, textsOf(file, tree)); + return texts; +} + /** The repo files `file` evaluates directly, already resolved to absolute paths. */ function directEagerEdges(file: string, tree: SourceTreeReader): string[] { const memo = memoOf(tree); @@ -310,6 +361,15 @@ function weightOf(file: string, tree: SourceTreeReader): number { return weight; } +function textsOf(file: string, tree: SourceTreeReader): string[] { + const memo = memoOf(tree); + const cached = memo.statementTexts.get(file); + if (cached) return cached; + const texts = parsedModuleOf(file, tree.readFile(file)).statementTexts; + memo.statementTexts.set(file, texts); + return texts; +} + /** * Every repo file evaluated as a consequence of importing `entryFile`, mapped to the * file that first pulled it in (`null` for the entry itself). From 0749922d94e233889e662cf6620209a110fd4fd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 8 Oct 2026 02:12:37 +0200 Subject: [PATCH 6/9] 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. --- ...descriptor-root-vs-eager-closure-budget.md | 44 ++++++++++++------- scripts/__tests__/closure-growth-rule.ts | 16 ++++--- .../__tests__/eager-closure-budgets.test.ts | 6 ++- 3 files changed, 43 insertions(+), 23 deletions(-) diff --git a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md index 67b0ca9d90..0a971f35e4 100644 --- a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md +++ b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md @@ -92,12 +92,16 @@ facts computed from the two closure graphs and the merge-base tree, not by an en (renames canonicalized), so nothing was dropped or swapped. 2. **Added-module novelty:** every module the head closure evaluates that the base closure did not is absent from the merge-base tree at its rename-canonicalized path AND carries no code - from a source deleted since the merge-base. A module the tree already had, sitting outside - the closure, becoming eager — under its own name, a renamed one, or a rewritten move that - evades git's `-M` similarity pairing — is a new edge, not re-homed code; and without this - fact, weight deleted elsewhere in the closure would fund it. Rename canonicalization - preserves modules ALREADY IN the base closure; it is not a pass for an external module - moved in, which fails the check at the base path the merge-base necessarily still has. + from a source deleted since the merge-base — matched when the added module shares **two or + more identical normalized top-level statements** with one deleted source; a single shared + statement is treated as coincidence, because short constants (a schema version, a default + timeout) repeat across the repo and branding every reuse a move would poison new files). + A module the tree already had, sitting outside the closure, becoming eager — under its own + name, a renamed one, or a rewritten move that evades git's `-M` similarity pairing — is a + new edge, not re-homed code; and without this fact, weight deleted elsewhere in the closure + would fund it. Rename canonicalization preserves modules ALREADY IN the base closure; it is + not a pass for an external module moved in, which fails the check at the base path the + merge-base necessarily still has. 3. **Flat weight:** the closure's total top-level statement count — wiring lines excluded, so re-homing is invisible to it — does not exceed the merge-base closure's. @@ -109,14 +113,20 @@ unrelated shrinkage paying for a smuggled edge — and the three together admit while refusing any growth in the closure's module-scope statement weight. Statements were chosen over bytes because formatting and comment churn move bytes while statements track the thing being preserved: module-scope declarations and calls. The guarantee is exactly **newly evaluated -modules are new code — new paths carrying no deleted source's statements — and total statement -weight is flat over a preserved closure**, not equality of eager behavior: a split that REPLACES -an existing statement with a more expensive top-level call inside an existing file moves no -count and no weight, so it passes — nothing count-based, in modules or statements, can see a -like-for-like replacement. That residue is bounded by what the tolerance cannot hide: every -module the head newly evaluates is a path the merge-base tree did not have AND carries none of -the statements of any source deleted since the merge-base (so it cannot smuggle pre-existing -implementation into eager scope, rewritten or not), the hard ADR-0019 checks (façade exactness, +modules are new code — new paths that share at most one coincidental statement with any source +deleted since the merge-base (`movedFromDeletedSource`'s deliberate two-statement move-signal +threshold) — and total statement weight is flat over a preserved closure**, not +equality of eager behavior: a split that REPLACES an existing statement with a more expensive +top-level call inside an existing file moves no count and no weight, so it passes — nothing +count-based, in modules or statements, can see a like-for-like replacement. That residue is +bounded by what the tolerance cannot hide: every module the head newly evaluates is a path the +merge-base tree did not have AND is not a verbatim transplant — the move probe above brands any +added module sharing two or more statements with one deleted source as pre-existing code. +The probe is deliberately conservative and textual: +code REWRITTEN or reformulated during a move does not match it and would pass as novel, so what +is verified is "no verbatim transplant of a deleted source," not "no repurposed logic" (the +weight fact is what still refuses a transplant made heavier). What keeps the residue small is +everything around the probe: the hard ADR-0019 checks (façade exactness, no platform implementation before binding) stay count- and pattern-based under it, and the added-module listing every failure prints is what a reviewer reads. The planted tests in `scripts/__tests__/closure-growth-rule.test.ts` pin every failing direction. @@ -142,7 +152,11 @@ shippable, so an oversized hub is addressable debt rather than accepted state. - **Novelty by merge-base tree path alone (the rule's second draft on #3298):** refuted by the planted rewritten-move test — a move that changes the path and churns formatting reports to git as delete + add, so an absent destination is not proof of new code. The deleted sources' - statement texts are the heuristic-free provenance that closes it. + statement texts close it down to a stated threshold: two identical normalized statements from + one deleted source is the move signal, one is tolerated as coincidence (short constants repeat + across the repo), and a transplant rewritten statement-by-statement stays outside a textual + probe — what remains heuristic-free is the LIST of deleted sources (an `-M`-less diff), not the + content match. - **Weight alone (#2469 candidate 1):** refuted by the planted drop test — a swap can keep the statement total flat while quietly making the closure evaluate different modules. - **Leave the array in place:** the status quo, not a resolution — it leaves the size debt unowned and diff --git a/scripts/__tests__/closure-growth-rule.ts b/scripts/__tests__/closure-growth-rule.ts index 5ecb324ce6..cd3c4d40af 100644 --- a/scripts/__tests__/closure-growth-rule.ts +++ b/scripts/__tests__/closure-growth-rule.ts @@ -14,10 +14,14 @@ // (renames canonicalized), so nothing was dropped or swapped -- the growth is purely // addition. // 2. ADDED-MODULE NOVELTY: every module the head closure evaluates that the base closure did -// not is absent from the merge-base tree AT ITS RENAME-CANONICALIZED PATH and carries no -// statement text from a source deleted since the merge-base. A module the merge-base tree -// already had, sitting outside the closure, becoming eager -- under its own name, a new -// one, or a rewritten move `-M` cannot pair -- is a NEW EDGE, not re-homed code; without +// not is absent from the merge-base tree AT ITS RENAME-CANONICALIZED PATH and is not a +// verbatim transplant of a deleted source: two or more identical normalized top-level +// statements shared with one source deleted since the merge-base mark a move +// (`movedFromDeletedSource`); a rewritten transplant evades a textual probe and would pass +// as novel, which the flat-weight fact then has to catch if it is heavier. A module the +// merge-base tree already had, sitting outside the closure, becoming eager -- under its own +// name, a new one, or a rewritten move `-M` cannot pair -- is a NEW EDGE, not re-homed code; +// without // this fact, weight deleted elsewhere in the closure would fund it. Canonicalization // preserves modules ALREADY IN the base closure; a moved-in external module fails novelty // at the base path the merge-base still has. @@ -29,8 +33,8 @@ // pre-existing module in at flat weight; novelty without containment accepts dropping a // merge-base module; weight without the other two accepts an unrelated shrinkage paying for a // smuggled edge. The guarantee the triple states is exactly "newly evaluated modules are new -// CODE -- new paths carrying no deleted source's statements -- and total statement weight is -// flat over a preserved closure" -- a split that REPLACES a statement with a more expensive +// CODE -- new paths the deleted-source probe cannot match to a deleted source's statements -- +// and total statement weight is flat over a preserved closure" -- a split that REPLACES a statement with a more expensive // one inside an existing file is invisible to any count, in modules or in statements, and is // not what this tolerance was built to admit. It replaces neither the count comparison (which // still fires first: growth must exist to be tolerated) nor the hard ADR-0019 checks (façade diff --git a/scripts/__tests__/eager-closure-budgets.test.ts b/scripts/__tests__/eager-closure-budgets.test.ts index f48f1a79f8..1080b74043 100644 --- a/scripts/__tests__/eager-closure-budgets.test.ts +++ b/scripts/__tests__/eager-closure-budgets.test.ts @@ -60,8 +60,10 @@ import { * ceilings, the committed-tree reader, rename following, the bounded attribution, recursive * discovery) additionally has its own failing-direction test below, because a real tree that * happens to satisfy its rules cannot distinguish a correct rule from a vacuous one. - * - Cost: one unit-lane test file plus two data/reader modules; four git processes for the - * merge-base side (merge-base, ls-tree, one cat-file batch, one rename diff), no device. The + * - Cost: one unit-lane test file plus two data/reader modules; five git processes for the + * merge-base side (merge-base, ls-tree, one cat-file batch for the whole base tree — which is + * also where the novelty check reads a deleted source's statements — one rename diff, and one + * `-M`-less delete diff listing the sources the rename diff cannot pair), no device. The * walker memoizes per-file edges per tree and parses each file once per distinct content, so * the base tree pays only for the files the branch changed. * - Kill criterion: if two consecutive quarters show no rule ever firing, or From e407ad338fc84ca3a6acca2e392fa66e68bb3267 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 8 Oct 2026 05:52:46 +0200 Subject: [PATCH 7/9] 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. --- .../0027-descriptor-root-vs-eager-closure-budget.md | 3 ++- scripts/__tests__/closure-growth-rule.ts | 9 +++++---- scripts/__tests__/committed-source-tree.ts | 12 ++++++++---- scripts/__tests__/eager-closure-budgets.test.ts | 3 ++- 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md index 0a971f35e4..d8a282e42a 100644 --- a/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md +++ b/docs/adr/0027-descriptor-root-vs-eager-closure-budget.md @@ -155,7 +155,8 @@ shippable, so an oversized hub is addressable debt rather than accepted state. statement texts close it down to a stated threshold: two identical normalized statements from one deleted source is the move signal, one is tolerated as coincidence (short constants repeat across the repo), and a transplant rewritten statement-by-statement stays outside a textual - probe — what remains heuristic-free is the LIST of deleted sources (an `-M`-less diff), not the + probe — what remains heuristic-free is the LIST of deleted sources (git's own delete listing, + needing no similarity pairing to have been attempted), not the content match. - **Weight alone (#2469 candidate 1):** refuted by the planted drop test — a swap can keep the statement total flat while quietly making the closure evaluate different modules. diff --git a/scripts/__tests__/closure-growth-rule.ts b/scripts/__tests__/closure-growth-rule.ts index cd3c4d40af..3ae97697e3 100644 --- a/scripts/__tests__/closure-growth-rule.ts +++ b/scripts/__tests__/closure-growth-rule.ts @@ -80,10 +80,11 @@ export function closureGrowthEvidence(params: { renamedBaseToHead?: ReadonlyMap; /** * Weight-bearing statement texts of each merge-base production source DELETED since the - * merge-base (`deletedSourcesSince`, which deliberately skips `-M`, mapped through - * `statementTextsOf`). Git's rename detection is a similarity heuristic and a rewritten move - * evades it, showing up as delete + brand-new path; content is not a heuristic, so an added - * module carrying a deleted source's code is a move of pre-existing code, not a new file. + * merge-base (`deletedSourcesSince`, which lists only the deletions git's DEFAULT rename + * pairing did not absorb, mapped through `statementTextsOf`). Git's rename detection is a + * similarity heuristic and a rewritten move evades it, showing up as delete + brand-new path; + * content is not a heuristic, so an added module carrying a deleted source's code is a move of + * pre-existing code, not a new file. */ deletedSourceTexts?: readonly (readonly string[])[]; }): ClosureGrowthEvidence { diff --git a/scripts/__tests__/committed-source-tree.ts b/scripts/__tests__/committed-source-tree.ts index 3f340d527c..d2ad20bb7b 100644 --- a/scripts/__tests__/committed-source-tree.ts +++ b/scripts/__tests__/committed-source-tree.ts @@ -55,10 +55,14 @@ export function renamedSince(repoRoot: string, base: string): ReadonlyMap Date: Thu, 8 Oct 2026 06:05:04 +0200 Subject: [PATCH 8/9] 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. --- scripts/__tests__/committed-source-tree.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/__tests__/committed-source-tree.ts b/scripts/__tests__/committed-source-tree.ts index d2ad20bb7b..196dcec7b0 100644 --- a/scripts/__tests__/committed-source-tree.ts +++ b/scripts/__tests__/committed-source-tree.ts @@ -58,8 +58,12 @@ export function renamedSince(repoRoot: string, base: string): ReadonlyMap Date: Thu, 8 Oct 2026 09:12:50 +0200 Subject: [PATCH 9/9] 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. --- scripts/__tests__/committed-source-tree.ts | 25 +++++++++++++------ .../__tests__/eager-closure-budgets.test.ts | 3 ++- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/scripts/__tests__/committed-source-tree.ts b/scripts/__tests__/committed-source-tree.ts index 196dcec7b0..2c61e4bee9 100644 --- a/scripts/__tests__/committed-source-tree.ts +++ b/scripts/__tests__/committed-source-tree.ts @@ -55,21 +55,30 @@ export function renamedSince(repoRoot: string, base: string): ReadonlyMap