refactor(android): separate adb transport addressing from the command payload - #2632
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at 1dc9dea. One contract for the invocation is a good direction, but a leased executor may now accept a caller's server port, provider-scoped calls may lose addressing, and the PR closes an investigation issue without its output. On main, The provider serial branch now passes Managed argv is not byte-identical to main for a few edge forms: a repeated
#2617 asks for a design trace, a route table and a continue/stop recommendation before any migration, and says the issue should not close as an implementation. Can "Closes #2617" be removed, the route table and recommendation posted on the issue, and the body corrected where it claims a single serializer and no re-slicing? Would a smaller change do: keep the three host methods and flat argv, and move the three hand-written grammars ( All checks pass at 1dc9dea and there are no conflicts. A live run is still needed on the routes this changes: a managed-lease Android session on a private adb server whose |
|
Addressed in Lease port. Yes, a caller could move a leased route: options passed through and root lowering preferred them. Provider argv. It did drop Grammar parity. Restored where adb itself is preserved, stated where it is not. A repeated Second serializer. #2617. Would a smaller change do? I tried that half first. The table plus one parse helper removes the duplicate parsing but not the double channel: Live runs still owed. Both remain unavailable here, so no evidence is claimed for them: a managed lease needs a Simlock host and token ( |
|
Reviewed at 8af64ef, as a follow-up to the review at 1dc9dea. The server-mismatch rule now has more than one owner, and the managed-lease and Limrun routes still have no live run.
Under a lease, doesn't Your answer on the smaller design holds: the table plus parser alone keeps six Only the local emulator route ran live, and this PR changes two more. A One iOS Smoke Tests job is still running; this diff does not touch the iOS route, so it looks unrelated. There are no conflicts. Next: one server-mismatch check, the two live runs, and the #2617 decision. |
8af64ef to
e49c2bd
Compare
|
Rebased on Point by point: 1 — one server check. Agreed, and taken further than a call. 2 — a lease dropping 3 — remove While doing that round I cut more, since the contract allowed: 4 — live runs. Ambient route now has more evidence than a snapshot: on 5 — #2617 disposition. Understood as yours. The PR does not close it and says so; the route table and the cost claim are on the issue. If the answer is "stop here", the useful residue is the grammar table and the provider handoff, which are the parts a different design would still want. 6 — rebase. Done; #2599's process-group handling and #2636's chunking are intact ( One unrelated observation from the live pass: |
e49c2bd to
d3162a0
Compare
|
Reviewed at d3162a0, as a follow-up to the review at 8af64ef. The lease refusal through Isn't the argv channel still different? Under a lease, a caller passing On size: removing Live evidence for the two routes is still missing. For a managed lease: a Simlock lease with a server port other than 5037 running All 20 checks pass and there are no conflicts. Next: the two live runs, and a decision on argv |
|
One consequence worth your eye: the rule also covers a host call whose two channels disagree, so The tightening is documented in the code doc on Live evidence gathered here. Built the missing helper assets ( Then against a real second adb server (
Still blocked, stated as blocked rather than waived.
Keeping merge on those two rows as you set it. #2617's typed-addressing direction continues here, and #2640 stays out of this branch. |
|
Reviewed at e795aa3. The argv The lease-override case at adb-provider-scope.test.ts#L181 passes Under a private adb server, a conflicting argv On design: is removing Live evidence is still missing for two routes. Limrun Android ships changed code in provider-limrun/src/android.ts and app-log-reconnect.ts, so it needs a run of Smoke Tests is still running. This PR changes |
|
Test literal. CHANGELOG. Added under Unreleased as #2640. Still the plan, and still gated on your semver call. Removing Live, on the two shapes you named. Ran them in process against a real adb server on 15037 and the real emulator:
One routing fact I ran into while building that, pre-existing and unchanged here: under a port scope Per your fallback, the in-process reachability test now stands in for the lease route on a non-5037 port: it drives a real adb binary through the lease port and asserts the refused request appears in no dispatch log. The allocator's own grant/renew/release around that transport remains unverified — no Limrun is unchanged and still owed: Smoke Tests passed; CI was 0 non-green at |
… payload
One array carried two meanings: which device and which adb server a call was
for, and what to run on it. Five relays read and rewrote the first meaning out
of the second — findAdbSerialIndex, readAdbSerial, stripAdbSerialArgs,
withServerPort, assertManagedAdbCommand — and a provider handed that array had
to undo the stitching to recover the command its caller actually asked for.
An invocation now carries the two apart:
`{ target: { selector, server, waitFor?, hostGlobals? }, command, rawArgv? }`.
One table holds the argv grammar — global option arity, the 20
`wait-for[-TRANSPORT][-STATE]` forms, the server and transport commands a
device-scoped call must never reach — and one parse/serialize pair reads and
writes it. `command` is appended and never re-parsed; a request nobody rewrote
is emitted as the argv it arrived in.
Four rules stop being implied:
- a private adb server travels in the addressing that owns it, so a per-call
`serverPort` cannot move a leased route onto another server;
- under a private server, globals that transport cannot restate are refused
rather than answered with addressing quietly left behind;
- a forwarding provider receives the caller's argv with only the scope's own
`-s` pair removed, so a readiness token survives the hop;
- one typed refusal answers every mismatch, over device, target, or server.
`provider-limrun` drops its structural copy of the invocation type and its own
argv projection, and hands an ADB failure the invocation it addressed. The root
host lowers a typed request in one function, which the test host now shares
instead of imitating.
Design trace and route table: #2617. Production: 12 files, +722/-265.
…sport A `-P` naming another server used to be rewritten onto the port the transport holds, so a caller who asked for 5037 could read a zero exit as an answer about 5037. One rule now covers every channel, argv or option: name no port and the transport uses its own; name this one and it is accepted; name another and the call is refused before dispatch. The provider-forwarding branch checks the parsed argv as well, because a provider receives argv and would hand the caller's `-P` to an adb it does not address. Ambient adb is untouched: with no private server named, the caller's argv is what runs. Each refusal is pinned at the process boundary — the fake adb records what it was asked, and the refused requests appear nowhere in that log.
…branch A `-P` of `9_999` parses to no port at all, so the pair stayed an unowned global and the older host-global refusal answered the call before the server rule ever saw it: the assertion passed without reaching the check it was written to pin. The port now parses, the assertion names the server refusal, and a port naming the server this lease holds is pinned on the same branch as what the provider is handed. Removing the server check from `requireManagedAndroidAdbAddressing` now fails four tests across the provider branch, the device route, the reachability run, and the root host.
…rite Names the three surfaces it reaches, and what does not change: a request naming no server or the server its route holds, and ambient adb.
1de97dd to
b246b95
Compare
|
Rebased on The only conflict was Integration was not just textual, so it was re-verified rather than trusted. #2604 moved device-ownership reads off adb onto the device shell, which adds Live re-run at head with helpers rebuilt for 0.21.4 ( The two owed rows are unchanged by the rebase and still open as stated above: Limrun needs |
|
Reviewed at b246b95, as a follow-up to the review at e795aa3. Both points are fixed. The lease-override test now passes Removing CI is green, and there are no conflicts. The Limrun Android route is still not live-validated, and this PR changes provider-limrun/src/android.ts and app-log-reconnect.ts. It needs a |
… host port The live Limrun lane cannot run here, and these are the two call sites it would have validated, so the shapes it checks are pinned where they are decided. The tunnel serial rides on the invocation's target and never enters the command array the caller wrote; ending the tunnel is a server-level command that selects no device and names the serial where adb expects to read it; and reader cleanup leaves no serial addressable on the session.
|
Head is The Limrun run is still not done, and I could not get it done here. I looked for a way rather than restating the blocker: no To make the run cost nothing but typing, it is scripted below, and to keep the changed code from being only unit-tested by inference I pinned the two shapes that run would have checked (
Those cover the tunnel addressing, the disconnect on cleanup, and the "provider passes the caller's argv, not a rebuild of it" claim. They do not cover the WebSocket tunnel, a real instance's adb handshake, or the real export LIMRUN_API_KEY=… # optional: LIMRUN_REGION
agent-device connect limrun --platform android --json
agent-device install path/to/app.apk --json
agent-device open <package> --foreground --json --debug
agent-device snapshot -i --json --debug # provider argv must lack the scope's `-s` pair
agent-device press <ref> --settle --json --debug
agent-device logs start && sleep 5 && agent-device logs stop --json
agent-device close --json
agent-device disconnect --json # cleanup: tunnel close + adb disconnect <serial>
# one forced adb failure, addressed output must name the command it addressed:
agent-device open com.example.definitely-not-installed --json --debugIf you would rather have this as a lane in the repo than as a comment, say so and I will land it as a script. Decision requested: run it with credentials you have, or tell me to treat the PR as accepted without the Limrun run. The Simlock lease row stays covered by the in-process reachability path plus the real-private-server run, as agreed. |
Live Limrun Android lane: ran itCredentials are available in my environment, so the second merge-gating route is no longer blocked. Everything below ran at head Happy path on the remote instance
Tunnel addressing, from the session request log ( The provider receives payload and the serial travels as transport addressing, which is exactly the claim in the second commit's tests: The last line is the cleanup shape Forced adb failure Injected at the transport boundary mid-session by dropping the tunnel serial from the local adb server ( The matching request-log record shows the command that failed with its addressing intact: Two failures observed on the lane that are not adb-level, recorded so the negative space is explicit: Cleanup: remote session closed and disconnected, connection profile gone; local emulator verified unchanged ( Remaining validation status
CI note
|
|
Rerun of the failed |
|
Reviewed at 164a9fd. The change since b246b95 adds only tests. The Limrun Android run you posted reaches the changed route, and the argv and error shapes it reports match the code. One reuse point remains in code that the b246b95 review did not flag. The forced adb failure ran with All 20 checks pass after the Smoke Tests rerun. The earlier failure was an iOS runner test that this diff does not touch. There are no conflicts. Next: the builder reuse above. |
The provider wrote an AndroidAdbTarget by hand, so a field added to that type could drift without an error. The composition root now hands it deviceAdbInvocation and hostAdbInvocation, both built with androidAdbInvocation over androidAdbSerialTarget / the new androidAdbHostTarget, which is also what parseAndroidAdbArgv reads an unaddressed argv as. The provider keeps the addressing decision, not its syntax. It cannot import the mechanics facet itself: ADR-0019 keeps a provider's eager closure off concrete platform implementation, which eager-closure-budgets.test.ts reports as soon as the edge appears.
Builder reuse: yes, but not through the import you pointed at
That import is a runtime edge, so the provider's eager closure starts evaluating the Android mechanics facet, which ADR-0019 keeps behind discovery or binding. The gate's own advice — a function-scoped
deviceAdbInvocation(serial: string, command: readonly string[]): AndroidAdbInvocation;
hostAdbInvocation(command: readonly string[]): AndroidAdbInvocation;and the composition root implements them with the shared builders ( deviceAdbInvocation: (serial, command) => androidAdbInvocation(androidAdbSerialTarget(serial), command),
hostAdbInvocation: (command) => androidAdbInvocation(androidAdbHostTarget(), command),
The typed refusal is worse than unit-onlyI chased live coverage for it, and the answer is that it cannot be reached in the shipped wiring. Dropping the tunnel serial mid-session and running That message is Where coverage went
Gates at |
|
Reviewed at 0688fd0. This answers the builder reuse point. The provider no longer writes an
Smoke Tests is still running on 0688fd0, and the other checks pass. There are no conflicts. Next: human review. |
thymikee
added
the
ready-for-human
|
Re-ran the Limrun lane at |
1 participant
Summary
Separates ADB transport addressing from the device-command payload, the production path traced in #2617. An invocation carries
{ target: { selector, server, waitFor?, hostGlobals? }, command, rawArgv? }; flat argv comes from one serializer, and the argv grammar — option arity, the 20wait-for[-TRANSPORT][-STATE]forms, managed-forbidden commands — is one table read by parser, policy and tests alike.commandis appended and never re-parsed; a request nobody rewrote is emitted as the argv it arrived in.Four rules are now explicit:
-PorserverPortoption: name no port and the transport uses its own, name this one and it is accepted, name another and the call is refused withmanaged-device-transport-mismatchbefore dispatch. Main rewrote a conflicting-Ponto the transport's port, which let a caller read a zero exit as an answer about the server it had named; this tightening is intentional. Ambient adb is untouched — with no private server named, argv runs as typed.-t,-H,-a,-d, a second-s, an unparseablewait-for-…) are refused, not answered with addressing dropped.-spair, so readiness tokens survive the hop. It is checked against the same server rule, because a provider would otherwise hand the caller's-Pto an adb it does not address.provider-limrundrops its structural type copy and its own argv projection, and hands an ADB failure the invocation it addressed. The root host lowers a typed request in one function, which the test host now shares instead of imitating. The provider builds noAndroidAdbTargetat all: the composition root hands itdeviceAdbInvocationandhostAdbInvocationbuilt byandroidAdbInvocationoverandroidAdbSerialTargetand the newandroidAdbHostTarget, because ADR-0019 keeps a provider's eager closure off the mechanics facet.Route table and cost: #2617 (comment). 27 files; gross churn exceeds the 1,000-line budget: one contract moves across platform, provider and root binding. Not closing #2617; the published-options change stays in #2640.
Validation
0688fd0e7conorigin/main9173d3c797(0.21.4), with #2599, #2604, #2629, #2636 and #2639 integrated.pnpm test:unit(1326 files / 10345 tests), provider-integration (66 files / 210 tests), typecheck, lint, layering (243 pass / 0 fail) and package gates pass locally. CI at head is green apart from oneSmoke Testsred onRunnerTests.testAlertCannotProveAnIdenticalReplacementAndDoesNotActivateIt, which arrived with27a97ee619 fix(ios): confirm alerts without repeating activation (#2326), touches no Apple code here, and passed onmainatfffa01e0; the rerun of that job passed on the same head, so the red was environmental.check:affected'smutation-modelownership assertion fails identically atorigin/mainhere.Refusals are pinned at the process boundary, not just at the thrown error: the fake adb appends every call it actually serves to a log, and the refused requests appear nowhere in it. Deleting the server check from
requireManagedAndroidAdbAddressingfails four tests across the provider branch, the device route, the reachability run, and the root host. The user-visible part of the tightening is inCHANGELOG.md, including that ambient adb is unchanged.Live at head, with helper assets built for 0.21.4 (
pnpm build:android; snapshot and IME APKs inandroid/*/dist) onemulator-5554:openreturning interactive@refs,press @e18 --settlesettling +18/−10,close. (Pixel 7 CI, SDK 36) through the built CLI:devices,open com.callstack.agentdevicelab --foregroundreturning an interactive snapshot with@refs,press @e18 --settlesettling with a +18/−10 diff onto the Settings screen,snapshot -i,apps,keyboard status,screenshot(1080x2400),logs start/mark/stop(15 KB),close.Live against a real second adb server on port 15037 (
adb -P 15037 start-server, separate daemon state dir):createLocalAndroidAdbProvider(device, { serverPort: 15037 })answeredshell id(uid=2000(shell)…) andgetprop ro.serialno(EMULATOR36X2X12X0); while a call was in flight the OS process table showed exactlyadb -P 15037 -s emulator-5554 shell sleep 3, which is external proof of the lowered argv; a backgroundlogcatspawn through the same route returned 961 lines;exec(['-P','9999','shell','id'])threwmanaged-device-transport-mismatchand pollinglsof -iTCP:9999 -sTCP:LISTENfor two seconds showed no server ever forked there, so nothing dispatched; the ambient host route kept working unchanged. Driving the whole CLI with adb's server on 15037 (ANDROID_ADB_SERVER_PORT=15037) also completeddevices,open,press --settle,screenshot,logs start/stop,close.Live in process, on the two shapes the review asked to see, against a real adb server on 15037 and the real emulator:
withAndroidAdbProvider(provider, { serial, serverPort: 15037 }), the shape a lease installs):runAndroidHostAdb(['-s', serial, 'shell', 'id'])answereduid=2000(shell)…; with a call in flight the process table readadb -P 15037 -s emulator-5554 shell sleep 3; a server-leveldeviceswas answered by the lease transport;-P 9999threwmanaged-device-transport-mismatchand added zero calls to the provider; a forced failure renderedadb -s emulator-5554 shell definitely-not-a-command exited with code 127.[["shell","id"], ["wait-for-device","shell","getprop","ro.serialno"], ["shell","definitely-not-a-command"]]— the scope's-spair removed, the readiness token preserved — while real adb answered underneath (uid=2000(shell),EMULATOR36X2X12X0), and the forced failure surfaced through the same addressed shape.Worth naming while doing that run: under a port scope,
runAndroidHostAdbis answered by the lease transport, so a forwarding provider installed alongside a private server receives nothing on that channel; it is reached through the executor-override channel instead, where the same server rule applies. Pre-existing routing, unchanged here, and no shipped combination uses both.Live on a real Limrun Android instance (
connect limrun --platform android --session apex-2632-lane --force), at head: the uploaded-asset catalog listed;open "com.callstack.agentdevicelab.apk" --foregroundleased in 6.9s and opened in 13.1s into an interactive snapshot (9 visible / 29 total nodes,@refs);snapshot -igreen;press @e2 --settlesettled+0 -0 (~29 unchanged)andpress @e26 --settlenavigated with+33 -24 (~5 unchanged);logs start/stopcaptured 1273 lines, 38 naming the package, so logcat ran over the tunnel through the changed reconnect/poller path;closethendisconnectreleased the session. The session request log records the adb commands the provider served —["-s","127.0.0.1:65049","shell","am","instrument","-w",…],["-s","127.0.0.1:65049","reverse","--list"], and cleanup as["disconnect","127.0.0.1:65049"], a device-less server command. A forced adb failure was injected by dropping that tunnel serial mid-session:keyboard statusreturnedCOMMAND_FAILED/device '127.0.0.1:65439' not foundwithexitCode: 1, the hint intact, and the request log shows the addressed command["-s","127.0.0.1:65439","shell","dumpsys","input_method"].APP_NOT_INSTALLEDfor a missing package and the service'sGiven archive does not contain AndroidManifest.xmlfor a truncated archive were also observed, both answered before any adb command. See the evidence comment for the full lane.Blocked validation, with what each needs
agent-device hostdoes not exist in this build (the ADR 0021 front-end is unshipped), andconnect --lease-id <id> --lease-backend android-instanceneeds a Host plus co-resident Simlock to mint one and hold an Android slot. Per review, the in-process path stands in, and it now runs both ways: the reachability test drives a real fake-binary adb through the lease port and asserts the refused request appears in no dispatch log, and the private-port scope above runs against a real adb server. What still stands unverified is the allocator's grant, renewal and release around that transport.