Skip to content

feat(limrun): share Android text and performance reads - #4268

Merged
thymikee merged 3 commits into
callstack:mainfrom
muvaf:codex/limrun-android-parity
Nov 6, 2026
Merged

thymikee merged 3 commits into
callstack:mainfrom
muvaf:codex/limrun-android-parity

Conversation

@muvaf

@muvaf muvaf commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Limrun Android snapshots were available, but live text and performance reads were refused. This 23-file change binds the shared text reader and Android performance operations to the session's ADB tunnel. It enables frame statistics, memory samples, and downloaded HPROF snapshots with local parsing and artifact handling. One owner resolver now reports lost sessions consistently across text, lifecycle, runner, and catalog operations. Cancellation during binding prevents work from starting.

This follows #4225. #4275 adds instance lifecycle commands separately. Native profiling remains deferred.

Examples: agent-device get text @e1, agent-device perf frames sample, agent-device perf memory snapshot --kind android-hprof.

Validation

At 3848d114b36d6de352ee20827fbb8e4cda9996c1, pnpm check:affected --run --head HEAD passed with exactCommit: true. Receipt, regression tests, and live results. The catalog lost-lease regression fails on 99be56862 and passes here. Independent review found no issues.

A fresh Limrun Android run verified native helper text without fallback, frame/memory samples, and a 33,521,774-byte HPROF. All resources were cleaned up. Full coverage and 100% changed-line coverage previously passed at 99be56862; GitHub owns coverage and device checks for the new head.


Autofix with [code]smith
Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.

View guided diff

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 20 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

@thymikee

thymikee commented Nov 6, 2026

Copy link
Copy Markdown
Member

Thanks for this. At 7ba110a the binding reuses the shared Android text reader and perf owners through the session ADB provider, which is the right shape, but it still needs a live Limrun Android run that is tied to it. The recorded run (text, frames, memory sample, HPROF) belongs to be921a1, and no ref reaches that commit after the force-push, so the claim that only the capability annotation changed cannot be checked. The new production wiring in runtime.ts:237 has no automated coverage either, so the live run is the only proof of that route. After the coverage and thread fixes, please rerun the live script at the final head and record that SHA. The run must reach get text with zero interaction_read_fallback diagnostics, show perf memory sample and perf frames sample available, and download a perf memory snapshot --kind android-hprof file with a JAVA PROFILE header. Pushing a ref that keeps be921a1 reachable would also let us check the runtime-unchanged diff.

The Coverage check fails because it is the changed-line gate: 14/21 changed lines are covered (66.67%) against a 70% threshold. This is likely related to this PR, since every uncovered line is added here: android-perf-runtime.ts lines 21 and 23, interaction-operations.ts line 150, and runtime.ts lines 238-240 and 907. The fix is tests, not a CI change. One provider-scenario test that opens a Limrun Android session on the existing provider fixtures and runs perf memory sample through the runtime would cover the runtime.ts lines. It should assert the metric and that the call reached the session exec. Today every test injects or hand-builds the binding, so a wrong adb route or host would pass.

Not blocking, and fine to take or leave: (1) interaction-operations.ts:144 writes its own missing-interactor resolver, while the contracts package already owns that shape in providerInteractorSource and every sibling binder uses it. Could a bindProviderElementTextRuntime in contracts, built on it like bindProviderFocusInteractor, replace this for all providers? (2) The abort signal never reaches the perf adb commands in android-perf-runtime.ts:21. Local Android behaves the same, so a signal through AndroidPerfHost in platform-android would be a separate change. (3) app-log-runtime.ts:212 and interaction-operations.ts:143 gate on device.platform === 'android' while the facts are decided separately. Local Android binds from the fact with whenAdmitted, so binding perfFrames and gating text on the facts would keep one source.

The open inline threads that still apply are the lease-loss error class (r4492404766) and the two coverage threads (r4492404748, r4492404758). One lower-priority thread also still applies: r4492404780, a cheap throwIfAborted after bind. Please fix the coverage and lease-loss threads first, then attach the live run at the resulting head.

@muvaf

muvaf commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in 99be568. Both lease-loss and cancellation regressions fail on 7ba110a and pass here. The new provider scenario allocates a Limrun session, opens Settings, and runs memory sampling through daemon dispatch, the production runtime binding, and the session ADB executor. It checks the tunnel serial and parsed metric. Frame/HPROF forwarding and lost-lease text reads are covered too.

The full coverage suite passes, changed-line coverage is 26/26 (100%), and the exact-commit affected gate passes. I also took the shared text-binder and facts-admission suggestions. Cancellation of already-running ADB performance commands remains separate, as discussed.

The replacement live evidence is from this exact final head. The get-text debug trace records an Android helper capture and zero fallback events; frame and memory samples are available; the 33,515,185-byte HPROF has a JAVA PROFILE header. All test resources were cleaned up. The PR body now links this run instead of relying on the orphaned commit.

@thymikee

thymikee commented Nov 6, 2026

Copy link
Copy Markdown
Member

This PR is ready at 99be568. It fixes what the earlier review raised: the changed-line coverage gate now passes, because the new provider scenario covers runtime.ts:237-246 and :906. All 13 checks pass, and there are no conflicts. Nothing else must happen before merge.

Your live run at 99be568 covers the route: the get-text value equals the node label, so the silent fallback would return the same text, but the android_snapshot_helper_capture phase in the trace shows readTextAtPoint ran.

Not blocking, take or leave: the delta adds a third inline copy of the "get the interactor or throw DEVICE_NOT_FOUND" resolver at interaction-operations.ts:142, next to the two in app-log-runtime.ts:179 and :204, and app-log-runtime.ts:226 still gives the catalog binder the raw getInteractor, so after a lease loss those operations report UNSUPPORTED_OPERATION instead of DEVICE_NOT_FOUND; the rule is that every Limrun interactor resolution reports a lost session as DEVICE_NOT_FOUND, which one requireLimrunInteractor(device, runner) helper in the owner would satisfy at all four sites; also element-text-runtime.ts:138 imports ./interactor-operation-binding.ts dynamically on every read, while focus-runtime.ts imports it statically, so a static import fits unless an import-closure budget needs the dynamic one, in which case a one-line comment should say so.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Nov 6, 2026
@muvaf

muvaf commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both optional suggestions in 3848d114b. The owner now shares one required-interactor resolver across all binding sites, including the catalog, so lost leases consistently return DEVICE_NOT_FOUND. The dynamic import stays because a static import exceeds the eager-closure budget; its comment now states that constraint.

The new catalog regression fails at 99be56862 and passes here. Exact-commit validation and independent review pass. A fresh live run also passed native helper text with no fallback, frame/memory samples, and an HPROF download of 33,521,774 bytes. All resources were cleaned up. Evidence and receipt.

@thymikee

thymikee commented Nov 6, 2026

Copy link
Copy Markdown
Member

At 3848d11 the earlier clean review still holds. The new commit routes every Limrun interactor resolution through one helper, so a lost lease now reports DEVICE_NOT_FOUND at every binding site, including the catalog binder. All 13 checks pass and there are no conflicts. Nothing else needs to happen before merge.

@thymikee
thymikee merged commit 8cc3fbe into callstack:main Nov 6, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants