feat: expose Limrun device session capabilities - #1485
Conversation
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Review at
No |
|
Addressed the ship-readiness requests in
I also checked the agent-device-cloud parity boundary. The public facade now covers the provider capabilities that are reusable there: inventory, interaction/viewport, key input, logs, recording, remote install, Android semantic ADB/keyboard/reverse removal, and iOS simctl. Tenant/run/lease authorization, Metro policy, artifact materialization, and the iOS runner protocol remain cloud-owned orchestration rather than Limrun provider API. iOS foreground-app/keyboard/clipboard queries and provider-unsound rotate/alert operations are intentionally not claimed as facade capabilities. Local validation is in the updated PR body. The aggregate unit run reached 4,758/4,761 tests; the only failures were unrelated five-second load timeouts, and all three affected files then passed in isolation with one worker (97/97). |
thymikee
added
the
ready-for-human
|
Re-review at Both prior blockers are resolved:
The production export route remains sound, focused tests pass, and exact-head CI is green. No code findings remain. |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Deep code-quality review at 876e9bf0, focused on structure, abstraction quality, and boundary cleanliness. (Would be a Request Changes, but GitHub doesn't allow that on one's own PR.) Behavior looks right and the validation story (live Android + iOS runs, mirrored tests) is genuinely strong — this is about the shape of the facade, not whether it works.
The central finding
LimrunDeviceSessionBase is over-unified, and that one modeling choice generates most of the incidental complexity in the new file. Four separate smells trace back to it:
- iOS
getForegroundAppis a stub that always resolvesundefined, conflating "unsupported" with "no foreground app" — inconsistent with this provider's own convention of throwingUNSUPPORTED_OPERATION. readLogs(appId: string | undefined, lineLimit)— Android ignoresappId; iOS requires it and enforces that with a runtime throw for what could be a compile-time signature difference.- Android
installRemoteAppsilently dropsmd5andrelaunch, and returns the caller's ownappIdentifierHintechoed back as a "result", while the iOS counterpart returns a genuinely verified bundle id. Same method name, very different guarantees. removePortReverseis a pass-through wrapper overadb.reverse.remove(already exposed on the same object), with a silent?.no-op guarding an invariant that always holds for Limrun sessions.
The fix is a restructuring, not a rewrite: the discriminated union already exists, and Android-only capabilities already live on the Android variant. Shrink the base to what is genuinely symmetric and move getForegroundApp, readLogs, and installRemoteApp onto the platform variants with honest per-platform signatures. The stub, the runtime throw, the ignored parameter, and the dropped options all disappear, and no call site gets harder — consumers must narrow on platform to reach adb/runSimctl anyway.
Secondary findings (inline)
- Canonical-helper duplication introduced by this PR:
isUserInstalledIosApp(verbatim copy ofios.ts:341) andtcpEndpoint(verbatim copy ofandroid.ts:209), plus a third, subtly differentunsupported()in the same directory. TheisUserInstalledIosAppcopy is the risky one — inventory filtering and install verification can silently diverge. runSimctl'sas LimrunIosCommandExecutioncast couples a ~25-line hand-rolled mirror of the SDK's execution type to reality by assertion only. Make the mirror checked (typed assignment or an assignability assertion) so SDK drift fails the build.listAppsdefaults to'all'while the contract module theAppsFiltertype comes from definesDEFAULT_APPS_FILTER = 'user-installed'and providesresolveAppsFilter()— the facade quietly disagrees with the canonical default.stopRecordingdiscards the client's return value — question inline about whether that's deliberate data loss.- Minor, no inline comment:
Awaited<ReturnType<LimrunIosSession['client']['listApps']>>is spelled out four times inios.ts— a namedLimrunIosAppalias would help legibility.
What's good
- The layering goal is right and achieved: raw Limrun clients stay private, the runtime's
getDeviceSessionmirrorsgetInteractor, and theinstallLimrunIosApprefactor to route throughinstallLimrunIosRemoteAppwith bounded inventory verification is a real improvement over trustingresult.bundleId. - Tests live in a focused mirrored module, no file approaches the 1k-line boundary, and the eventual-consistency test is exactly the coverage that path needed.
I'd hold merge on the base-type restructuring and the helper duplication; the rest are smaller but worth doing while the file is fresh.
Generated by Claude Code
| type LimrunDeviceSessionBase = { | ||
| readonly platform: 'android' | 'ios'; | ||
| readonly device: DeviceInfo; | ||
| readonly interactor: Interactor; | ||
| readonly viewport?: { width: number; height: number }; | ||
| listApps(filter?: AppsFilter): Promise<LimrunInstalledApp[]>; | ||
| getForegroundApp(): Promise<LimrunForegroundApp | undefined>; | ||
| pressKey(key: string, modifiers?: string[]): Promise<void>; | ||
| readLogs(appId: string | undefined, lineLimit: number): Promise<string>; | ||
| startRecording(options?: { quality?: LimrunRecordingQuality }): Promise<void>; | ||
| stopRecording(options: { outPath: string }): Promise<void>; | ||
| installRemoteApp( | ||
| url: string, | ||
| options?: LimrunRemoteInstallOptions, | ||
| ): Promise<LimrunRemoteInstallResult>; | ||
| }; |
There was a problem hiding this comment.
Structural: LimrunDeviceSessionBase is too wide, and it's the root cause of most of the special cases in this file.
You already have a discriminated union (LimrunAndroidDeviceSession | LimrunIosDeviceSession), and Android-only capabilities (keyboard, adb, removePortReverse) already live on the Android variant. But the base type forces false symmetry for everything else, and each forced-symmetric member generates a smell downstream:
getForegroundApp()— iOS has no implementation, so it's a stub that always resolvesundefined(line 176), making "unsupported" indistinguishable from "no foreground app". Everywhere else in this provider, unsupported operations throwAppError('UNSUPPORTED_OPERATION').readLogs(appId: string | undefined, lineLimit)— Android ignoresappIdentirely; iOS requires it and throws at runtime for what could be a compile-time contract (readLogs(lineLimit)on Android vsreadLogs(appId, lineLimit)on iOS).installRemoteApp(url, LimrunRemoteInstallOptions)— Android silently dropsmd5andrelaunch(see separate comment).
i think there's a code-judo move here that makes this much simpler: shrink the base to what is genuinely symmetric (platform, device, interactor, listApps, pressKey, recording), and move getForegroundApp, readLogs, and installRemoteApp onto the platform variants with honest per-platform signatures. That deletes the iOS stub, the runtime throw, the ignored parameter, and the silently-dropped options in one restructuring — consumers of the union already have to narrow on platform to use adb/runSimctl anyway, so nothing gets harder to call.
Generated by Claude Code
| installRemoteApp: async (url, options) => { | ||
| await session.client.sendAsset(url); | ||
| return { appId: options?.appIdentifierHint }; | ||
| }, |
There was a problem hiding this comment.
Contract problem: Android installRemoteApp silently ignores its options and returns unverified data.
Two issues here:
LimrunRemoteInstallOptionspromisesmd5andrelaunchon both platforms, but this implementation drops both on the floor.relaunchis implementable —installLimrunAndroidAppinandroid.tsalready does theam force-stopdance for exactly this. Either implement it or narrow the Android option type so the contract doesn't lie.{ appId: options?.appIdentifierHint }echoes the caller's own hint back as the result. On iOS,appIdmeans "verified present in post-install inventory, with bounded retries"; on Android it means "whatever you told me". Same method name, wildly different guarantees — a consumer can't tell which one they got without checkingplatform. If Android can't verify, the result type for Android shouldn't pretend to carry anappIdat all (which the per-platform split suggested on the base type would make natural).
Generated by Claude Code
| removePortReverse: async (devicePort) => { | ||
| await session.adbProvider.reverse?.remove(tcpEndpoint(devicePort)); | ||
| }, |
There was a problem hiding this comment.
This abstraction seems unnecessary — it's a pass-through over a capability the same object already exposes.
The facade exposes the full AndroidAdbProvider as session.adb, so a consumer can already write session.adb.reverse?.remove('tcp:8081') — with owner-aware tracking via the reverse manager. removePortReverse adds nothing except:
- a silent no-op when
reverseis absent (?.) — for Limrun sessionsreverseis always assigned increateLimrunAndroidSession, so the optional chain papers over a non-invariant instead of asserting it; - a re-validation of the port via a duplicated
tcpEndpoint(see comment below).
Suggest deleting this method and letting adb.reverse be the one canonical port-reverse surface, or — if you want removal to be a first-class facade capability — making it throw when reverse is missing rather than pretending success.
Generated by Claude Code
| function isUserInstalledIosApp(app: { bundleId: string; installType: string }): boolean { | ||
| return ( | ||
| !app.bundleId.startsWith('com.apple.') && !app.installType.toLowerCase().includes('system') | ||
| ); | ||
| } | ||
|
|
||
| function tcpEndpoint(port: number): `tcp:${number}` { | ||
| if (!Number.isInteger(port) || port < 1 || port > 65_535) { | ||
| throw new AppError('INVALID_ARGS', `Invalid Android tcp reverse port: ${port}`); | ||
| } | ||
| return `tcp:${port}`; | ||
| } | ||
|
|
There was a problem hiding this comment.
Verbatim helper duplication introduced by this same PR.
isUserInstalledIosAppis character-for-character identical toios.ts:341(also added in this PR). If the "what counts as a user app" heuristic ever changes,listApps('user-installed')and install verification will silently disagree.tcpEndpoint(including theAppErrorvalidation) is a copy ofandroid.ts:209.- This file also adds a third
unsupported()variant in the same directory (ios.tsandruntime.tseach have one), and this one returns the error while the siblings throw — subtle divergence in a copy-pasted name.
Export the canonical helpers from ios.ts/android.ts (or a shared module in this provider dir) and reuse them.
Generated by Claude Code
| ...createRecordingOperations(session.client), | ||
| installRemoteApp: async (url, options) => | ||
| await installLimrunIosRemoteApp(session, url, options), | ||
| runSimctl: (args) => session.client.simctl(args) as LimrunIosCommandExecution, |
There was a problem hiding this comment.
Why does this need a cast? LimrunIosCommandExecution is ~25 lines of hand-rolled event-emitter overloads mirroring the @limrun/api client's simctl return type, coupled to it only by this as. If the SDK's execution type drifts (event renamed, wait() result reshaped), this compiles fine and consumers break at runtime.
I get the motivation — not leaking @limrun/api types into the public d.ts — but then make the mirror checked: if session.client.simctl(args) is assignable to LimrunIosCommandExecution, a plain typed return needs no cast; if it isn't assignable without the cast, the exported type is already wrong today. Either drop the as, or add a compile-time assignability assertion (e.g. satisfies-style check against ReturnType<LimrunIosClient['simctl']>) next to the type so drift fails the build instead of the bridge.
Generated by Claude Code
| device: session.device, | ||
| interactor: createLimrunAndroidInteractor(session), | ||
| adb: session.adbProvider, | ||
| listApps: async (filter = 'all') => { |
There was a problem hiding this comment.
Boundary inconsistency: both listApps implementations default to 'all', but the contract module this AppsFilter type is imported from defines DEFAULT_APPS_FILTER = 'user-installed' and ships resolveAppsFilter() for exactly this. A bridge calling session.listApps() and the CLI apps command will disagree on what the default inventory means. Use resolveAppsFilter(filter) here (both platforms), or document why the facade deliberately diverges from the canonical default.
Generated by Claude Code
| startRecording: async (options?: { quality?: LimrunRecordingQuality }) => { | ||
| await client.startRecording(options); | ||
| }, | ||
| stopRecording: async ({ outPath }: { outPath: string }) => { | ||
| await client.stopRecording({ localPath: outPath }); | ||
| }, |
There was a problem hiding this comment.
Question: the underlying client's stopRecording returns a value (Promise<string> per LimrunRecordingClient and the tests, which stub it with a URL), and the facade discards it and returns void. Is that reference (remote recording URL?) genuinely useless to bridges, or is the facade throwing away data a consumer would otherwise have to re-derive? If it's intentional, worth a one-line note on LimrunRecordingClient; if not, surface it in the result.
Generated by Claude Code
|
Exact-head code review is clean. The discriminated Limrun device-session facade exposes honest platform-specific capabilities without leaking raw clients; app filtering uses the canonical rule; iOS remote installation is verified against eventually consistent inventory; and focused tests plus live Android/iOS evidence exercise the public facade and cleanup. Completed static, integration, coverage, Fallow, and non-iOS smoke checks are green. The iOS smoke is still in progress, not failing, so the existing |
1 participant
Summary
Validation
pnpm check:affected --base origin/main --runpnpm exec vitest run src/providers/limrun/device-session.test.ts src/__tests__/limrun-runtime.test.tspnpm exec vitest run --project unit-core --maxWorkers 1 scripts/__tests__/help-conformance-bench.test.ts src/screenshot-diff/__tests__/screenshot-diff.test.ts src/platforms/apple/core/__tests__/runner-client.test.tspnpm --filter @agent-device-cloud/bridge checkpnpm --filter @agent-device-cloud/bridge test -- test/limrun-session.test.tsdev.agentdevice.LimrunParityE2E, both facade inventory andrunSimctl(["listapps", "booted"])verified the install, recording produced 27,380 bytes, and no raw client was exposed; lease and temporary uploaded asset were cleaned up