refactor(request): move the device-inventory context out of core - #1993
Merged
Conversation
`device-inventory-context.ts` is an AsyncLocalStorage holder: it imports only `node:async_hooks`, kernel errors, and contracts types, and nothing from `core`. `daemon-modularity.ts` already describes `src/request/` as "request-global daemon plumbing (progress sinks, cancellation, AsyncLocalStorage)", and `request/progress.ts` is the same shape — a `withX`/accessor pair over one store. Living in `core` (rank 2) made the platform-runtime composition root reach up to rank 2 for `listLocalDeviceInventory`. Injecting the lookup would not have fixed that: the only caller, `platform-runtime-operation-host.ts`, is in the same zone, so the edge would move rather than disappear. Relocating the module does remove it, and every other consumer — daemon (rank 4) and `core/dispatch-resolve.ts` (rank 2) — now reaches down instead of sideways or up. Pure rename; the module body is unchanged and 14 import specifiers are retargeted. R17's `DEVICES_INVENTORY_IMPORT_SOURCES` pin follows the module: the rule still asserts the devices handler imports the neutral gateway, does not shadow it, and calls it. This clears the last composition-root upward value edge that was not deliberate. Confirmed with `pnpm depgraph`: those edges drop from 3 to 2, and the two that remain are the documented ones — `provider-limrun-runtime.ts` (an in-file comment marks it a deliberate static seam) and `runtime.ts -> commands/index.ts` (`bindCommands` is the aggregate binder). Cycle, back-edge and R6 counts unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WWLBCBDBpdR8z1aCmDerXS
Size Report
npm unpacked components
Startup median (7 runs, lower is better):
Top changed chunks:
Top changed packed files
|
|
thymikee
added a commit
that referenced
this pull request
Aug 24, 2026
The previous commit intersected the `src/facades/` walk with tracked sources but left manifest-derived entry surfaces bypassing that intersection entirely — the same bug class, on the path it did not close. Two distinct leaks: 1. `readWorkspacePackages` enumerated `packages/` with `readdirSync`, so an uncommitted scratch package contributed a name, export targets, and dependency edges to every R11 rule built on it, not just to facade discovery. Closed at the source: it now reads tracked manifests via `listTrackedPackageManifests`, mirroring how R13's `readTrackedPlatformPackageDeclarations` has always enumerated its own. 2. A TRACKED manifest can still name an uncommitted target in its working-tree content, which the trailing `existsSync` admitted. `facadeEntryFiles` now intersects manifest-derived targets with the tracked production set as well, so every path it returns is committed whatever its origin. Three fixtures, each verified failing before the fix and passing after: - `readWorkspacePackages reads tracked manifests only` (in R11's own suite, since leak 1 affects every rule that reads packages, not only this gate) — red when manifest enumeration reverts to `readdirSync`. - `an untracked PACKAGE contributes no entry surface` — red when both fixes are reverted. - `a dirty manifest naming an UNTRACKED target contributes no entry surface` — red when the target intersection is dropped. The shared git fixture builder is factored out of the existing discovery test so all four use one committed baseline. Pins reseeded on 8d280a2: six new contracts entry surfaces from #1993, which the exhaustiveness gate caught on its own, plus dispatch 100 -> 94 and platform-runtime 31 -> 36.
thymikee
added a commit
that referenced
this pull request
Aug 24, 2026
…ding-shape probe (#1739) (#1965) * test(structure): per-package eager-closure budgets (#1960) ADR-0019 requires platform-package façades to stay implementation-lazy and is explicit that a startup threshold alone is not a substitute for preserving the loading shape. #1950 built the AST-level walker (eager-import-closure.fixtures.ts) and proved the planted-red procedure on one file (session-teardown.ts's android-helper denylist); this generalizes it into a data-driven budget table so any workspace-package façade -- or a designated hub module -- can get an eager-closure ceiling without a bespoke test. Seeds a budget for every packages/*/src/facades/*.ts file (discovered the same way package-boundaries.test.ts discovers façades, not hand-listed) from its measured current closure size, plus a platform-implementation denylist for façades whose contract is implementation-neutral vocabulary. Also demonstrates the mechanism on two designated hub modules (cli.ts, session-teardown.ts) alongside their existing, more specific ad hoc pins. Closes #1960 * test(structure): derive facade roots from manifests, add edge chains, reseed tight Review findings on #1965: 1. Discovery scanned only `packages/*/src/facades/*.ts`, which omits every package that publishes its entry surface straight from the manifest — including all six `packages/platform-*/src/index.ts` façades, the exact subject of ADR-0019's implementation-laziness rule. Discovery now derives from `readWorkspacePackages(...).exportTargets` and then adds `/src/facades/` files, reusing the R11 helper rather than reimplementing it so the two gates cannot disagree about what an entry surface is. The table grows from 14 façades + 2 hubs to 95 entry surfaces + 8 hubs. 2. Budgets carried a few files of slack each. They are now exact ratchets with no headroom, matching how the repo pins R9/R10 and test-file size: growth is allowed, it just has to be a visible number change in the diff of the PR that causes it. Every budget is reseeded from post-#1969 measurement. 3. Violations printed a flat sorted set, which named the offender but not the route. `eagerClosureGraphOf` records each file's discoverer, so failures now print the transitive chain entry -> ... -> offender. `eagerClosureOf` keeps its contract and is expressed in terms of the new walk; per-file edges are memoized, which also cuts the existing pins' runtime (cli closure test 2336ms -> ~550ms). The platform-package façades evaluate exactly one module each — themselves — so their budget of 1 is the tightest statement of "metadata-eager, implementation-lazy" the walker can make. * test(structure): make the eager-closure pins exact, bounded, and single-owner Second review pass on #1965 found four holes, two of which were places the PR text claimed a property the code did not have. 1. Rows were documented as exact ratchets but asserted with `<=`, so a shrink silently became headroom a later regression could grow back into. The comparison is now equality, in a pure `classifyBudget` with a separate message for each direction ("lower its pin to N in this PR so the ratchet keeps the gain"), matching test-file-size-ratchet.ts and the R9/R10 pins. 2. An over-pin failure printed a chain per evaluated module — 361 of them for src/cli.ts. It now prints a bounded attribution: the entry's heaviest direct edges (capped at 4) with a couple of representative deep routes each, ranked so a newly added import sorts first. The comment states plainly that this attributes by shortest import route and does NOT diff against a recorded baseline; naming a true delta would mean checking in ~1,500 module paths and rewriting them on every contracts refactor. 3. Discovery reimplemented a one-level `src/facades` scan while canonical R11 discovery is recursive, so a nested façade file could be covered by R11 and silently missing here. `facadeEntryFiles` is now a single exported owner in package-boundaries.ts that both R11's façade gate and this table consume. 4. Rows were converted to Sets before any uniqueness check, so a duplicate was unobservable. The table is now two `Record<string, number>` literals keyed by path, making an in-record duplicate a TypeScript error (ts1117); the only remaining case — one path in both records — is asserted on the array. Each of the four holes gets a test that fails when the rule is broken, since a tree that happens to satisfy its pins cannot distinguish a correct rule from a vacuous one. Writing those found a real bug in the duplicate check itself (`Set.add` returns the Set, so the filter never matched). Pins reseeded on 04e4c23. * test(structure): restore tracked-only layering discovery and bound platform diagnostics Two findings from the third #1965 review. 1. R11 REGRESSION, introduced by the previous commit in this PR. Extracting one recursive facade-discovery owner was right, but it was implemented as a raw filesystem walk, which silently changed R11 from tracked-source discovery (listSourceFiles) to "whatever is on disk". A layering gate describes committed state, so an uncommitted scratch facade under a scanned path must be invisible; instead it could fail both R11 and the budget gate on a contributor's working directory. Worse, the test asserted the new behaviour, pinning the regression in place. The tracked-source enumerator now lives in a leaf module, scripts/layering/tracked-sources.ts, that both check.ts and package-boundaries.ts consume (check.ts imports package-boundaries.ts, so the dependency cannot run the other way). facadeEntryFiles reads listTrackedProductionSources, keeping the single recursive owner while restoring tracked-only scope. The temp-dir test is replaced by a real git fixture following platform-package-repository.test.ts: a tracked nested facade that must be discovered, plus untracked siblings — one beside it and one beside the nested file — that must not be. Reverting to the filesystem walk turns it red. 2. Only the numeric-pressure diagnostic was bounded; the platform-implementation assertion still emitted every offender with a full chain, so one eager platform subtree buried the single import that caused it under hundreds of lines. Both diagnostics now share one renderer that groups targets by the entry's own import they arrived through, caps edges and routes, and states the omitted counts. A wide planted-red fixture (300 offenders under one owning edge plus extra edges) proves the cap engages rather than assuming it. src/cli.ts reseeded 361 -> 362 for drift from the rebase onto 7aaa559. * test(structure): close the manifest half of tracked-only discovery The previous commit intersected the `src/facades/` walk with tracked sources but left manifest-derived entry surfaces bypassing that intersection entirely — the same bug class, on the path it did not close. Two distinct leaks: 1. `readWorkspacePackages` enumerated `packages/` with `readdirSync`, so an uncommitted scratch package contributed a name, export targets, and dependency edges to every R11 rule built on it, not just to facade discovery. Closed at the source: it now reads tracked manifests via `listTrackedPackageManifests`, mirroring how R13's `readTrackedPlatformPackageDeclarations` has always enumerated its own. 2. A TRACKED manifest can still name an uncommitted target in its working-tree content, which the trailing `existsSync` admitted. `facadeEntryFiles` now intersects manifest-derived targets with the tracked production set as well, so every path it returns is committed whatever its origin. Three fixtures, each verified failing before the fix and passing after: - `readWorkspacePackages reads tracked manifests only` (in R11's own suite, since leak 1 affects every rule that reads packages, not only this gate) — red when manifest enumeration reverts to `readdirSync`. - `an untracked PACKAGE contributes no entry surface` — red when both fixes are reverted. - `a dirty manifest naming an UNTRACKED target contributes no entry surface` — red when the target intersection is dropped. The shared git fixture builder is factored out of the existing discovery test so all four use one committed baseline. Pins reseeded on 8d280a2: six new contracts entry surfaces from #1993, which the exhaustiveness gate caught on its own, plus dispatch 100 -> 94 and platform-runtime 31 -> 36.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
No behavior change.
device-inventory-context.tsis anAsyncLocalStorageholder — it imports onlynode:async_hooks, kernel errors and contracts types, and nothing fromcore. It moves fromsrc/core/(rank 2) tosrc/request/(rank 1).scripts/layering/daemon-modularity.tsalready describessrc/request/as "request-global daemonplumbing (progress sinks, cancellation, AsyncLocalStorage)", and
request/progress.tsis the sameshape: a
withX/ accessor pair over one store. This module iswithDeviceInventoryContextplusaccessors that throw outside request execution, so the zone description already covers it.
Why relocation rather than injection. The visible symptom was
platform-runtime-android-emulator-host.tsimportinglistLocalDeviceInventoryfromcore, and theobvious fix is to inject the lookup. That does not work: the only caller,
platform-runtime-operation-host.ts, sits in the same composition-root zone, so injecting moves theedge rather than removing it. Relocating the module removes it, and every other consumer improves at
the same time — daemon (rank 4) and
core/dispatch-resolve.ts(rank 2) now both reach down.Effect on the spine. This clears the last composition-root upward value edge that was not
deliberate:
platform-runtime-android-emulator-host.tscore/device-inventory-context.tsprovider-limrun-runtime.tssdk/limrun-runtime-dependencies.tsruntime.tscommands/index.tsbindCommandsaggregating every command is the purpose of that moduleTogether with #1985 the count goes 4 → 3 → 2, and both survivors are documented as intentional.
Gate pin. R17's
DEVICES_INVENTORY_IMPORT_SOURCESnames the neutral inventory owner by modulespecifier, so the pin follows the module. The rule's three assertions are unchanged: the devices
handler must import the neutral gateway, must not shadow it, and must call it. Its own test exercises
the retained
core/dispatch-resolve.tsentry and is unaffected.The module body is byte-identical; the diff is the rename plus 14 retargeted specifiers across 12
files (including three
vi.mockpaths and one dynamicawait import).Validation
pnpm check:affected --runpassed on this head — "all runnable checks passed".pnpm typecheck,pnpm check:layeringandpnpm formatare clean; the layering summary is unchanged frommain(R6 still 7, R11 still 17 packages / 95 subpaths / zero root back-imports, R13 still one composition
root).
The spine claim is from
pnpm depgraphbefore and after: composition-root upward value edges 3 → 2with the
coretarget gone, and value-import cycles (0), spine back-edges (0) and type-onlyinversions (7) all unchanged.
An independent adversarial review of the diff returned no findings. It separately confirmed that the
moved file has no relative imports (so unchanged content is safe at the new depth), that all three
vi.mockspecifiers still resolve to the same absolute module the handlers import, that nopackages/replay-test/file imports this module (which would trip the rule forbiddingsrc/request/there), and that no
.fallowrc.json,fallow-baselines/,vitest.config.tsortest/wire-compat/ledger.jsonkey names the old path.Two failures during validation were environmental rather than diff-caused and are recorded for
completeness: a formatter width violation introduced by the longer specifier (fixed with repo-wide
pnpm format), andcheck:replay-compatrefusing to run against a shallow clone — its own errornames the fix (
git fetch --unshallow --tags), after which it passes.No device-facing path is touched, so this owes no live device evidence. Published and reported; CI on
this head is the authority still to come.
Generated by Claude Code