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 |
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.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
b246b9546conorigin/main9173d3c797(0.21.4), with #2599, #2604, #2629, #2636 and #2639 integrated.pnpm test:unit(1325 files / 10340 tests), provider-integration (66 files / 210 tests), typecheck, lint, layering (243 pass / 0 fail) and package gates pass locally; CI green at the previous head.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.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.connect limrun --platform androidreturnsINVALID_ARGS: connect limrun requires LIMRUN_API_KEY. An API key (LIMRUN_API_KEY, optionalLIMRUN_REGION) and an available Android instance are the only missing pieces. With them the lane isconnect limrun --platform android→install→open→snapshot -i→press→logs start/stop→close→disconnect, each under--debugto show the provider receiving argv without the scope's-spair, plus one forced adb failure to show the addressed failure output refactor(android): separate adb transport addressing from the command payload #2632 adds. The forwarding shape itself is live-verified locally (above); what a key buys is the tunnel, the real instance, and cleanup.