refactor: isolate Limrun construction dependencies - #1502
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Code review is clean for Not merge-ready yet: #1490 W1c and the repository readiness rules require live Limrun evidence. Run a credentialed Android and iOS Residual risk: no separately authorized cross-vendor review was performed. |
|
Blocking lazy-loading review addressed in 6c2ec0b. Concrete operation/platform helpers now load through async dependency methods; the new module-load regression test proves construction loads none of app-helpers, logcat, Apple app-resolution, or Apple install-artifact and verifies each loads only when called. The emitted provider chunk now references those helpers only through dynamic imports. Focused tests (22) and pnpm check:affected --run are green. Keeping this PR in draft pending live Limrun evidence. |
6c2ec0b to
fa5c53b
Compare
|
Reviewed at What's right
I checked two things that looked like risks and are not:
1. Does this seam have two real adapters, or one plus a test double?
Right now I don't think that makes this wrong, because the goal here is breaking import edges for W1c extraction rather than earning a port, and the edges are genuinely broken. But 2. Android and iOS dependencies are injected two different waysAndroid capabilities ride on the session object: type LimrunAndroidAdbSession = {
dependencies: Pick<LimrunRuntimeDependencies, 'android' | 'host'>;
};
createLimrunAndroidInteractor(session) // reads session.dependencies.androidiOS capabilities are threaded as a separate parameter: createLimrunIosInteractor(session, ios)
createLimrunDeviceSession(session, ios)
installLimrunIosApp(limrun, session, installablePath, dependencies, options)So Not a correctness bug, but it's the kind of thing that gets copied. Picking one style before this becomes a package would make the eventual façade easier to state. 3.
|
|
Reviewed Not ready to merge yet: #1490 W1c requires live Limrun evidence. This branch still has no credentialed Android/iOS allocation, Android tunnel/reverse, install, disconnect, and teardown run, as the PR itself notes. Please return it to draft or attach that evidence before readiness labeling. Residual risk: no separately authorized cross-vendor review was performed. |
|
Addressed the review from Exact resolution:
Validation on the pushed head:
Live credentialed evidence is still unavailable in this environment. Residual risk remains real |
|
Independent follow-up review at |
|
Re-reviewed The draft status remains appropriate: no credentialed Limrun Android/iOS lifecycle run was available. Before promotion, capture live allocation → interaction/install as applicable → Android reverse/tunnel → release/teardown evidence with |
12e4ab4 to
79d2a40
Compare
|
Live Limrun evidence completed on exact head Android (
iOS (
This exercises both extracted per-session dependency paths plus the Android persistent Metro/ADB route and teardown ownership. Together with the already-green exact-head CI and deterministic install-path coverage, the requested promotion gate is satisfied. |
|
Summary
Prepare the Limrun runtime seam for later package extraction without packaging it yet.
This is an extraction-enabling construction seam, not a claimed domain port. It currently has one
production adapter (
createLimrunRuntimeDependencies) plus deterministic test doubles, not two realadapters, and must not be cited as precedent for satisfying the two-real-adapter rule. Its purpose in
W1c is to remove package-blocking imports while preserving behavior and the released interface.
and iOS host-adapter operations
src/utils/exec.tsownership/threading style without widening
ProviderDeviceRuntimemethods
Promise<AppError>; it remains async specifically so the platformerror helper is imported only on failure
LimrunAdbProvideronly for interactor creation andLimrunAdbExecutorfor command-only andport-reverse construction dependencies
ProviderDeviceRuntime, and the releasednew LimrunRuntime(options)SDK shapesrc/providers/limrun/**into root core/platform/utils from 18 to0
createAndroidInteractordeliberately remains the one static construction edge. The releasedProviderDeviceRuntime.getInteractorcontract is synchronous; hiding that factory behind a dynamicimport would require a lazy proxy interactor and broaden this extraction-only change. All
operation-specific helper imports remain lazy.
Related: #1490
Validation
pnpm exec vitest run src/sdk/limrun-runtime-dependencies.test.ts src/providers/limrun/runtime-dependencies.test.ts src/providers/limrun/device-session.test.ts src/__tests__/limrun-runtime.test.ts src/__tests__/provider-device-runtimes.test.ts(22 passed)pnpm test:integration:provider(150 passed before the review follow-up; rerun outside the sandboxafter the sandbox denied localhost binding)
pnpm check:affected --runpnpm check:layeringpnpm check:quickThe module-load regression test proves runtime import/construction loads none of the Android
app-inventory/log or Apple app-resolution/install helpers, then verifies each module loads only when
its capability is called. The emitted
dist/src/provider-limrun-runtime.jshas dynamic imports forapp-helpers,logcat,app-resolution, and Appleinstall-artifact; none is in its static importheader.
Live credentialed Limrun evidence was not run because this environment has no
LIMRUN_API_KEY,LIMRUN_REGION, or.env.local. Residual risk remains real Android and iOS allocation/lifecyclebehavior, especially Android tunnel establishment, reverse setup, disconnect, and teardown. The PR
stays draft until that evidence is available.