Skip to content

refactor: isolate Limrun construction dependencies - #1502

Merged
thymikee merged 3 commits into
mainfrom
agent/1490-w1c-limrun-seam
Jul 30, 2026
Merged

refactor: isolate Limrun construction dependencies#1502
thymikee merged 3 commits into
mainfrom
agent/1490-w1c-limrun-seam

Conversation

@thymikee

@thymikee thymikee commented Jul 30, 2026

Copy link
Copy Markdown
Member

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 real
adapters, 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.

  • inject a narrow construction dependency object for Android interactor, ADB/reverse, host-command,
    and iOS host-adapter operations
  • keep concrete construction in root/SDK composition, backed by the existing platform modules and
    src/utils/exec.ts
  • store each platform's dependencies on its allocated session, so Android and iOS use one coherent
    ownership/threading style without widening ProviderDeviceRuntime
  • acquire operation-specific Android helpers and Apple capabilities lazily through async dependency
    methods
  • type ADB error construction as Promise<AppError>; it remains async specifically so the platform
    error helper is imported only on failure
  • use LimrunAdbProvider only for interactor creation and LimrunAdbExecutor for command-only and
    port-reverse construction dependencies
  • delegate wrapper lifecycle/inventory properties through getters instead of copying references
  • exercise the same construction contract through deterministic in-memory test doubles
  • retain conditional Limrun loading, ProviderDeviceRuntime, and the released
    new LimrunRuntime(options) SDK shape
  • reduce production imports from src/providers/limrun/** into root core/platform/utils from 18 to
    0

createAndroidInteractor deliberately remains the one static construction edge. The released
ProviderDeviceRuntime.getInteractor contract is synchronous; hiding that factory behind a dynamic
import 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 sandbox
    after the sandbox denied localhost binding)
  • pnpm check:affected --run
  • pnpm check:layering
  • pnpm check:quick

The 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.js has dynamic imports for
app-helpers, logcat, app-resolution, and Apple install-artifact; none is in its static import
header.

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/lifecycle
behavior, especially Android tunnel establishment, reverse setup, disconnect, and teardown. The PR
stays draft until that evidence is available.

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.91 MB 1.91 MB +2.2 kB
JS gzip 611.7 kB 612.2 kB +477 B
npm tarball 729.4 kB 729.6 kB +271 B
npm unpacked 2.55 MB 2.56 MB +4.1 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 29.5 ms 30.3 ms +0.8 ms
CLI --help 60.0 ms 61.3 ms +1.3 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/ime-lifecycle.js +62.5 kB +18.5 kB
dist/src/internal/daemon.js +21 B +2 B
dist/src/session.js +6 B +1 B

@thymikee

Copy link
Copy Markdown
Member Author

Code review is clean for 975d6b3. The Limrun factory now receives bounded construction adapters without widening ProviderDeviceRuntime; root composition preserves conditional loading and the released new LimrunRuntime(options) SDK path. Deterministic factory coverage and production-wrapper tests cover the seam, including Android reverse/tunnel cleanup and iOS adapter behavior.

Not merge-ready yet: #1490 W1c and the repository readiness rules require live Limrun evidence. Run a credentialed Android and iOS connect limrun lifecycle, including Android reverse and close/disconnect teardown, before promotion. Android/iOS smoke jobs are still running, and the PR remains draft.

Residual risk: no separately authorized cross-vendor review was performed.

@thymikee

Copy link
Copy Markdown
Member Author

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.

@thymikee
thymikee force-pushed the agent/1490-w1c-limrun-seam branch from 6c2ec0b to fa5c53b Compare July 30, 2026 16:13

Copy link
Copy Markdown
Member Author

Reviewed at fa5c53b4. The mechanical work is clean and the 18→0 import reduction is real. Two design questions and some smaller notes.

What's right

  • The lazy-load regression test is the strongest part of this PR. Counting mocked-module factory invocations is a genuine assertion that construction doesn't pull app-helpers, logcat, app-resolution, or install-artifact, and it verifies each loads on first capability use rather than just asserting absence.
  • Moving the ?? inferAppNameFromPath(...) fallback out of the old readIosBundleAppName and into prepareLimrunIosAsset keeps behavior identical while letting the adapter stay a thin readIosBundleInfo(...).appName. Correct refactor.
  • Dropping the two fallow-ignore-next-line unused-class-member comments in runtime.ts is right, since the new wrapper genuinely reads deviceInventoryProvider and ownsDevice.

I checked two things that looked like risks and are not:

  • The utils/timeouts.sleepnode:timers/promises swap is behaviorally identical (sleep is literally new Promise(r => setTimeout(r, ms))) and already the convention in four other production files.
  • The appName fallback restructure preserves the original semantics exactly.

1. Does this seam have two real adapters, or one plus a test double?

#1478 states the rule as "A seam exists when its port has two real adapters", and lists as a STOP condition "a test only passes by widening a module façade or adding test-only dependency injection."

Right now LimrunRuntimeDependencies has exactly one production implementation — createLimrunRuntimeDependencies() — and one in-memory implementation that exists only in runtime-dependencies.test.ts. By the letter of that rule, the second adapter is a test double, not a real one.

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 #1478 also says "packaging enforces an already-earned module interface; it does not itself earn a port or justify widening one" — so it's worth stating explicitly in the PR body which of those two things this is, rather than leaving a reviewer to infer it. If the answer is "this is an extraction-enabling seam, not a claimed port," saying so avoids it being cited later as precedent for a port that has no second real implementation.

2. Android and iOS dependencies are injected two different ways

Android capabilities ride on the session object:

type LimrunAndroidAdbSession = {
  dependencies: Pick<LimrunRuntimeDependencies, 'android' | 'host'>;
};
createLimrunAndroidInteractor(session)          // reads session.dependencies.android

iOS capabilities are threaded as a separate parameter:

createLimrunIosInteractor(session, ios)
createLimrunDeviceSession(session, ios)
installLimrunIosApp(limrun, session, installablePath, dependencies, options)

So createLimrunDeviceSession(session, ios) takes iOS deps explicitly while reaching through session.dependencies for Android ones. That asymmetry is visible in the tests too — device-session.test.ts has to build both TEST_ANDROID_DEPENDENCIES (on the session) and TEST_IOS_ADAPTER (as an argument) for what is conceptually one dependency set.

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. adbError widens the error type from AppError to Error

adbError(message, result, details?): Promise<Error>;

The production adapter returns androidAdbResultError(...), an AppError carrying code and details, so runtime behavior is preserved. But the declared type is now plain Error, and the test fixture already takes advantage of that (adbError: async (message) => new Error(message)). Anything downstream relying on ADR 0010 fields would degrade silently if a future adapter did the same in production. Suggest typing the return as AppError — nothing in the seam needs it wider.

Minor, related: adbError is async only so the adapter can await import(...) the error constructor, which forces throw await dependencies.android.adbError(...) at the call site. That reads oddly for error construction. Not worth restructuring if the lazy import is load-bearing, but worth a comment saying that's why.

4. Smaller notes

  • LimrunAndroidRuntimeAdapter mixes parameter types: createInteractor(device, adb: LimrunAdbProvider) and createPortReverse(adb: LimrunAdbProvider) versus listApps/getForegroundApp/getKeyboardState/dismissKeyboard/readLogs(adb: LimrunAdbExecutor). Probably deliberate — only some need reverse/text — but it's easy to misread.
  • The wrapper in provider-limrun-runtime.ts copies leaseLifecycle, recoverExpiredLease, and deviceInventoryProvider into fields at construction. Safe today because all three are readonly on the implementation, but it's an aliasing assumption that isn't stated. A getter delegating to this.implementation would be immune.
  • src/sdk/limrun-runtime-dependencies.ts statically imports createAndroidInteractor, so core/interactors/android.ts loads at construction. That's consistent with what the PR body claims — it lists only the four helper modules — but if the intent is that construction is cheap in general, the interactor is the remaining static edge.

Validation

The stated gates look appropriate. The absence of live Limrun evidence is disclosed with the right residual-risk framing. Given this changes tunnel teardown paths (cleanupLimrunAndroidAdbTunnel now goes through host.runAdb), a real allocate/reverse/teardown cycle would be worth running before this leaves draft — the in-memory port-reverse fixture asserts call ordering but not that a real adb disconnect still lands.


Generated by Claude Code

@thymikee
thymikee marked this pull request as ready for review July 30, 2026 17:00
@thymikee

Copy link
Copy Markdown
Member Author

Reviewed fa5c53b: the lazy-capability follow-up is sound. The selected Android inventory/log and Apple resolution/install helpers now load only when their injected capability is invoked; the regression test is counterfactual, and the async adapter contract is propagated through Android/iOS call sites. The narrow construction seam and unchanged ProviderDeviceRuntime remain aligned with #1490, and exact-head CI is green.

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.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed the review from
#issuecomment-5133789262
in 12e4ab440ad1180d3468ffeb3e1f60a6884aa786.

Exact resolution:

  • Updated the PR body to classify this as an extraction-enabling construction seam, with one
    production adapter plus test doubles. It is not a claimed domain port and is not precedent for the
    two-real-adapter rule.
  • Unified dependency ownership: both Android and iOS sessions now retain their narrow platform/host
    dependencies. iOS no longer threads a separate adapter through runtime, device-session,
    interactor, and install calls. This is the smallest style consistent with session lifetime and
    keeps operation-specific imports lazy.
  • Preserved the released new LimrunRuntime(options) interface and did not widen
    ProviderDeviceRuntime.
  • Tightened adbError to Promise<AppError>, updated test doubles to construct AppError, and
    documented at the adapter that async construction is required to keep the platform error helper
    lazy until failure.
  • Tightened port-reverse construction to LimrunAdbExecutor; interactor construction intentionally
    retains LimrunAdbProvider because it consumes provider-scoped capabilities.
  • Replaced copied lifecycle/recovery/inventory wrapper references with delegation getters.
  • Kept createAndroidInteractor as the deliberate static edge because
    ProviderDeviceRuntime.getInteractor is synchronous. Making it lazy would require a proxy
    interactor and is outside this extraction-only change; the decision is documented at the import
    site.

Validation on the pushed head:

  • focused Limrun/runtime tests: 5 files, 22/22 tests passed
  • pnpm check:affected --run: all 7 selected gates passed; related tests 33/33
  • layering remains green with 0 forbidden logical-module imports

Live credentialed evidence is still unavailable in this environment. Residual risk remains real
Android and iOS allocation/lifecycle behavior, especially Android tunnel/reverse/disconnect/teardown.
The PR remains draft until that evidence can be collected.

@thymikee
thymikee marked this pull request as draft July 30, 2026 17:05
@thymikee

Copy link
Copy Markdown
Member Author

Independent follow-up review at 12e4ab440 is clean. Android and iOS now consistently carry dependencies on the allocated session; the former mixed argument/session ownership is gone. adbError is constrained to Promise<AppError>, command-only reverse setup receives the narrower executor, wrapper lifecycle/inventory properties delegate through getters, and the one eager Android interactor edge is explicitly tied to synchronous ProviderDeviceRuntime.getInteractor. The PR body correctly classifies this as an extraction-enabling seam with one production adapter, not a two-real-adapter domain port. No new code findings. Keep the PR draft until exact-head CI completes and the disclosed credentialed Limrun Android/iOS lifecycle evidence is available.

@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed 12e4ab440: clean. Dependency ownership is now consistent per allocated Limrun session: Android and iOS interaction/install paths use the injected session dependencies, while port-reverse accepts only the ADB executor needed for the persistent-tunnel route. Wrapper lifecycle/inventory access still delegates to the same stable implementation functions. Exact-head CI is green, including provider integration, coverage, layering, and platform smokes.

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 LIMRUN_API_KEY and region configured.

@thymikee
thymikee force-pushed the agent/1490-w1c-limrun-seam branch from 12e4ab4 to 79d2a40 Compare July 30, 2026 19:02
@thymikee
thymikee marked this pull request as ready for review July 30, 2026 19:21
@thymikee

Copy link
Copy Markdown
Member Author

Live Limrun evidence completed on exact head 79d2a40f5353dd6ef197cf1b587d24adce4798f9 using isolated state directories. No credential values were persisted in the generated profiles or included in output.

Android (android-instance):

  • connect limrun --platform android generated the provider profile with deferred allocation.
  • open com.android.settings --metro-port 8081 allocated limrun:android:*, bound Metro to 10.0.2.2:8081, and opened Settings.
  • After building the source-checkout snapshot helper prerequisite, snapshot -i installed helper 0.20.3 through the provider path and returned a live 141-node hierarchy.
  • click @e14 succeeded against the live Apps row.
  • close returned the concrete Limrun Android instance id and released the lease; disconnect removed connection state.

iOS (ios-instance):

  • connect limrun --platform ios generated the provider profile with deferred allocation.
  • open com.apple.Preferences allocated limrun:ios:* and launched Settings.
  • click 360 500 succeeded through the Limrun iOS interaction client.
  • close returned the concrete Limrun iOS instance id and released the lease; disconnect removed connection state.

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.

@thymikee
thymikee merged commit 6067b9a into main Jul 30, 2026
30 checks passed
@thymikee
thymikee deleted the agent/1490-w1c-limrun-seam branch July 30, 2026 19:29
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-30 19:30 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant