From ad1b23117a084cc668397f9ed04f241dc309b381 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 11:00:22 +0200 Subject: [PATCH 1/2] docs(android): record the #2617 adb addressing design trace Route table, accepted grammar, measured identity and cut set for separating transport addressing from the device-command payload. Recommendation: continue with a narrower cut than the spike. --- docs/adb-transport-addressing-experiment.md | 188 ++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 docs/adb-transport-addressing-experiment.md diff --git a/docs/adb-transport-addressing-experiment.md b/docs/adb-transport-addressing-experiment.md new file mode 100644 index 0000000000..078f544863 --- /dev/null +++ b/docs/adb-transport-addressing-experiment.md @@ -0,0 +1,188 @@ +# #2617 experiment: typed ADB transport addressing beside the device-command payload + +Design trace and disposable prototype from 2026-09-15, base `origin/main` `2cafab3ad0`, prototype +branch `spike/2617-typed-adb-addressing` (`72a1467a93`, `b79e96ccd5`, `74c72809a6`). The prototype +is measurement scaffolding: it is not a production migration and must not be merged. + +Blocking dependency re-audit first: **#2611 is still open** (`headRefOid` `a3a0ea216`, +`mergedAt` null), and `packages/kernel/src/device-shell.ts` does not exist on `main`. Every +device-shell relay claim below therefore describes a branch shape, not merged transport reality, and +the prototype was traced against `main` with the #2611 relay sites read from +`git fetch origin pull/2611/head:pr-2611-audit`. #2026 owns shell quoting; nothing here changes +quoting or the kernel-owned shell fragment representation. + +## Accepted addressing grammar + +Read from `adb help` on the measurement host (adb `1.0.41`, version `35.0.2-12147458`), not from the +two fields the current code models: + +| Grammar form | Modelled today | Modelled in the prototype | +| --- | --- | --- | +| `-s SERIAL` | yes (scan + strip) | `target.selector = { kind: 'serial', serial }` | +| `-P PORT` | yes, managed rewrite only | `target.server = { kind: 'port', port }` | +| `-H HOST`, `-L SOCKET`, `-t ID`, `-d`, `-e`, `-a` | refused under a managed lease, invisible otherwise | `target.hostGlobals`, carried verbatim, refused under a managed lease | +| second `-s` (adb lets the later one win) | silently re-parsed positionally | kept as `hostGlobals` so it keeps its winning position | +| `wait-for[-TRANSPORT]-STATE` | prefix-stripped by `assertManagedAdbCommand` only | `target.waitFor`, then the real command is the tail | +| `-P9999` (attached form) | refused under a managed lease | `hostGlobals`, refused under a managed lease | + +`serial` and `serverPort` are two of six owned selections. A proposal that types only those two +reproduces today's blind spot: an ambient `-H foreign.example` is invisible to every router. + +## Routes, representation, and payload identity + +Measured with a throwaway probe against the real root host binding (`payload = ['shell','input','tap','13','42']`); +"identity" asks whether the array that reaches the exec boundary is the caller's array. + +| Route | Entry | Addressing on `main` | Addressing in the prototype | Payload identity | +| --- | --- | --- | --- | --- | +| Local device exec | `runAndroidAdb` → `createDeviceAdbExecutor` | `-s` stitched in the host binding, `-P`/`-s` re-scanned by `withServerPort` (`src/platform-runtime-android-adb-host.ts:164`) | `target` built once, host serialises | held (`command === payload`) | +| Ambient device, smuggled `-s` | same | emits `-s -s `; adb obeys the later one | `rawArgv` kept verbatim, same emitted argv | held | +| Managed device (lease port) | same, `serverPort` set | argv rebuilt twice (`adbInvocation` + `withServerPort`) | `applyManagedAndroidAdbServer` rewrites addressing only | held | +| Managed device, smuggled matching `-s` | same | slice + rebuild | re-slice at parse | **lost** (unavoidable: addressing arrives inside the payload) | +| Provider forwarding | `runCmd('adb', …)` inside `withAndroidAdbProvider` (`adb-provider-scope.ts:201` at HEAD, `:188`/`:204` at main) | `readAdbSerial` + strip, provider gets a rebuilt array | provider gets `invocation.command` | elementwise equal, **not** the caller's array | +| Managed host-level adb | host transport (`adb-provider-scope.ts:228` at HEAD) | `execHostAdb(['-s', scope.serial, …args])` | serial adopted into `target` | held at the host port | +| Managed server-port precedence | request `-P 9999` + lease `15038` | scan-and-replace, refusal on non `-P`/`-s` leading options | `applyManagedAndroidAdbServer` + `requireManagedAndroidAdbSerial` | argv identical (`-P 15038 -s …`), asserted in `src/platform-runtime-android-adb-host.test.ts` | +| Background spawn | `spawnSerialAdb` | `runCmdBackground` (always detaches internally) | `spawnAdb` | see Findings, detach policy | +| Limrun provider | `packages/provider-limrun/src/android.ts:193` | `['-s', serial, …args]` prefix | structural `LimrunAdbInvocation`, tunnel serial as addressing | provider payload kept argv-shaped for `@limrun/api` | +| Generic host command port | `host.commands.run({ executable: 'adb', args: ['-s', …] })` | flat argv, `[...request.args]` copies at `src/platform-runtime-host.ts:21` and `src/platform-runtime-operation-host.ts:60` | untouched by this spike | not measurable: still flat | + +## Proposed API + +`packages/platform-android/src/adb-addressing.ts` (241 lines at HEAD) owns grammar and managed policy: + +```ts +type AndroidAdbSelector = { kind: 'unspecified' } | { kind: 'serial'; serial: string }; +type AndroidAdbServer = { kind: 'ambient' } | { kind: 'port'; port: number }; +type AndroidAdbTarget = Readonly<{ + selector: AndroidAdbSelector; + server: AndroidAdbServer; + waitFor?: 'device' | 'usb-device' | 'local-device' | 'any-device'; + hostGlobals?: readonly string[]; +}>; +type AndroidAdbInvocation = Readonly<{ + target: AndroidAdbTarget; + command: readonly string[]; + rawArgv?: readonly string[]; +}>; +``` + +Two directions only: `parseAndroidAdbArgv` (the single place that re-slices a payload, and only when +addressing was actually present — `command` is the caller's array otherwise) and +`serializeAndroidAdbInvocation` (the single emitter; returns `rawArgv` verbatim when addressing was +never rewritten). Managed rules: `applyManagedAndroidAdbServer`, `requireManagedAndroidAdbSerial`, +`requireManagedAndroidAdbCommand`, `androidManagedAdbEnvironment`. The host port collapses +`execSerialAdb`/`spawnSerialAdb`/`execHostAdb` into `execAdb`/`spawnAdb` taking an invocation, so +`src/platform-runtime-android-adb-host.ts` nets **-53 lines**. The port-side test host still holds +its own copy of the lowering, but now calls the shared `applyManagedAndroidAdbServer` and serializer +instead of rebuilding `-P`/`-s` by hand. + +## Numbers + +| Measure | `main` | prototype | +| --- | --- | --- | +| Independently authored addressing/refusal owners | 11: serial scan + strip, scope-identity refusal, port precedence refusal, two host-level `-s` stitches, `withServerPort` scan + refusal, `assertManagedAdbCommand`, managed env lowering, detach-by-method-name, Limrun device prefix, Limrun host prefix, eight route-level `-s` stitches | 7 of them answer to `adb-addressing.ts`; scope-identity and port precedence stay in provider scope (they are scope questions, not grammar), detach became an explicit option on the device executor, and the eight route-level stitches wait for cut-set step 4 | +| Literal `'-s'` producers/consumers in production | 17 | 13 (3 in the owning module, 1 ambient escape, 8 untouched route sites) | +| New parsers/serializers | — | 1 parser, 1 serializer, 1 env builder | +| New bridges | — | 2, both in `provider-limrun` (structural `LimrunAdbInvocation`, and `limrunAdbArgv` to keep failure text identical) | +| Production LOC | — | `+481 -199` (net **+282**); outside the new 241-line module, net **+41** | +| Test LOC | — | `+255 -108`, all of it migrated assertions on the same argv expectations, no new synthetic executor | +| argv rebuild sites in the five transport files | 3 (`stripAdbSerialArgs`, `withServerPort`, app-log copy) | 2 (app-log copy, plus one `readonly`→mutable copy at the provider hand-off) | +| `readonly` payload propagation cost (executor payload → `readonly string[]`) | n/a | 96 type errors in 24 files, **1 in production source** (`adb-provider-scope.ts:57`, one `string[]` → `readonly string[]` widening) | + +Gate status on the prototype: 118 files / 846 tests green (`packages/platform-android`, +`packages/provider-limrun`, `src/platform-runtime-android-adb-host.test.ts`, +`src/managed-device-reachability.test.ts`), plus 12 files / 59 tests green across the released-surface, +SDK, provider-scope, dialog-readiness and routing-parity lanes. Root `tsc -p tsconfig.json` reports 0 +errors; `pnpm check:layering` and `pnpm lint` pass. + +## Findings a migration must carry + +- **The detach policy rode on the method name.** `execSerialAdb` detached, `execHostAdb` and + `spawnSerialAdb` did not (`2cafab3ad0:src/platform-runtime-android-adb-host.ts:85`, `:95`, `:105`). + Collapsing to one `execAdb` keyed on "target has a serial" silently detached host-level managed + calls, which changes timeout/`killProcessTree` behaviour. The prototype carries the decision as an + additive `AndroidAdbExecutorOptions.detached` set by the device-scoped executor only. +- **A hand-written typed guard disagreed with the shared rule on first contact.** The first version of + `requireManagedAndroidAdbCommand` checked `command[0]`, while `assertManagedAdbCommand` finds the + first token that is not `wait-for-*` (`2cafab3ad0:src/platform-runtime-android-adb-host.ts:189`). + Three refusal cases (`wait-for-device kill-server`, `wait-for-device disconnect`, + `wait-for-any-device pair …`) and every host-global selector (`-H`, `-L`, `-t`, `-d`, `-e`, `-P9999`) + passed silently. The existing 21-selector loop in + `src/platform-runtime-android-adb-host.test.ts` is what caught it; it is the oracle for this grammar + and belongs in a golden table under `contracts/fixtures/` if the rules go cross-language. +- **The port carrier differed between the real host and the unit-test host.** The real lowering strips + `serverPort` and emits `-P` plus `ANDROID_ADB_SERVER_PORT`/`ANDROID_ADB_SERVER_ADDRESS`; the + in-package test host passed `serverPort` through untouched, so + `packages/platform-android/src/__tests__/adb-executor.test.ts` asserted the stub's carrier, not the + production one. Sharing `lowerAndroidAdbInvocation` moved that assertion to argv, where it is now the + same contract. That shared lowering is a cut-set item, not a free win. +- **Flat ingress cannot be fixed downstream.** Provider forwarding and the eight generic host command + port sites (`logs/runtime.ts` ×2, `network/runtime.ts` ×2, `readiness/runtime.ts`, + `shutdown/runtime.ts`, `inventory.ts` via `host.commands.run`, and + `src/platform-runtime-app-log-process.ts:187`) deliver one argv. There the payload is by definition + re-sliced, and `[...request.args]` at `src/platform-runtime-host.ts:21` / + `src/platform-runtime-operation-host.ts:60` copies it again. Identity survives only where the caller + already separates addressing. +- **`@limrun/api` pins argv at the provider edge.** `LimrunAdbExecutor(args)` is handed to an external + package, so the provider-facing executor stays argv-shaped; only the host port + (`LimrunRuntimeDependencies.host.runAdb`) can be typed. `provider-limrun` declares no + `platform-android` dependency, so the prototype duplicated the invocation structurally and mirrored + argv for failure text — two bridges that a shared addressing home would remove. +- **Released surface is narrower than it looks.** `agent-device/android-adb` publishes + `AndroidAdbExecutor`, `AndroidAdbExecutorOptions`, `AndroidAdbProvider` and nine `*WithAdb` + helpers (`src/sdk/android-adb.ts`), so the executor payload must stay `(args: string[])` and + `detached` must stay additive. `AndroidAdbHost` and `AndroidAdbInvocation` live on private workspace + packages (`@agent-device/platform-android`, `@agent-device/provider-limrun`) and are not in the + published export list, so they carry no external compatibility obligation. + +## Recommendation: continue, with a smaller cut than the prototype + +Continue. The typed representation moves seven independently authored addressing decisions into one +module, costs +41 production lines outside that module, and needs one production signature change to make +the payload read-only across the platform. Stop criteria are not met: bridges did not multiply (2, both +explained by a missing shared home), enforcement got one owner instead of three, and no lifecycle +behaviour needs to change once `detached` is an option. The cut set is: + +1. Decide the addressing home first: a tiny `@agent-device/adb-addressing` package, or an explicit + `provider-limrun` → `platform-android` edge. Do not ship the structural duplicate. +2. Land the addressing module + host port collapse (`execAdb`/`spawnAdb`) with + `lowerAndroidAdbInvocation` exported from the platform package so the real and test hosts share it, + and `detached` as an explicit option. +3. Move the managed refusal set and the grammar into a golden table under `contracts/fixtures/` and + keep the 21-selector loop as the parity oracle. +4. Re-route the eight generic host command port adb calls to the android host port instead of + `commands.run`; this is where the copies and the `-s` stitches actually disappear. +5. Type the provider host port (`host.runAdb`) and keep `LimrunAdbExecutor` argv-shaped. +6. Re-audit after #2611 lands: its `relayDeviceShellArgv` sites (6 in production, including the two + generic-host copies) are the provenance relay this proposal should make unnecessary at steps 2 and 4. + +Validation lanes for the production proposal: `pnpm test:unit packages/platform-android +packages/provider-limrun`, `pnpm test:unit src/platform-runtime-android-adb-host.test.ts +src/managed-device-reachability.test.ts src/sdk/limrun-runtime-dependencies.test.ts +src/__tests__/android-adb-public.test.ts`, `pnpm check:affected --run`, plus a device-backed pass +through `test/integration/provider-scenarios/android-lifecycle.test.ts` for screenshot/`exec-out` +binary results and background `logcat` spawn, which no unit lane covers. + +## Unresolved evidence and pitfalls + +- Device-backed verification was not run: this is a docs-only deliverable and the spike never claimed + runner or on-device parity, only argv/refusal/identity parity against existing lanes. +- `pnpm typecheck` on `2cafab3ad0` fails in `packages/platform-android/src/__tests__/snapshot-helper-install.test.ts:29` + (`installArgs` was dropped from `AndroidSnapshotHelperManifest` by `f07a4eca31` (#2618) while the + fixture still sets it, added by `2cafab3ad0` (#2619)). Because `typecheck` chains with `&&`, that one + package error means the root and `examples/sdk` projects are never checked. All root typecheck claims + here come from running `npx tsc -p tsconfig.json` directly. Fix the fixture separately. +- `src/sdk/limrun-runtime-dependencies.test.ts > Limrun construction defers operation and + opposite-platform helper modules` fails on pristine `main` on this host (real-time budget), so the + eager-closure claim rests on import inspection instead: `adb-addressing.ts` imports only + `@agent-device/kernel/errors`, and `src/__tests__/package-exports.test.ts` stays green. +- `pnpm check:production-exports` fails on `main` with 68 unused-export findings in 39 files + (`packages/maestro`, `packages/capture-kit`, `src/runtime.ts`, …). None of those files is in this + diff, and none of the exports this spike adds is flagged, so the count is unchanged; the gate was + not green before and is not green now. +- Ambient routes keep argv authority: `rawArgv` wins at serialization, so on those routes `target` is + advisory and the later `-s` still wins. Typing ambient addressing means choosing a precedence rule + that today's behaviour does not have; that decision is unresolved and deliberately out of scope here. +- `pnpm stash`-style isolation is unusable for this kind of spike in parallel worktrees: `git stash` + is scoped to the shared common git directory, so a `stash pop` in one worktree can apply another + agent's stash. Use a commit on a spike branch and `git checkout --` to go back. From 2a5b67c1247a0ab9251bf3767b2414b72d8fe0a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 11:12:41 +0200 Subject: [PATCH 2/2] docs(android): answer the adversarial review of the #2617 trace --- docs/adb-transport-addressing-experiment.md | 280 +++++++++++--------- 1 file changed, 159 insertions(+), 121 deletions(-) diff --git a/docs/adb-transport-addressing-experiment.md b/docs/adb-transport-addressing-experiment.md index 078f544863..be662fc662 100644 --- a/docs/adb-transport-addressing-experiment.md +++ b/docs/adb-transport-addressing-experiment.md @@ -1,54 +1,60 @@ # #2617 experiment: typed ADB transport addressing beside the device-command payload Design trace and disposable prototype from 2026-09-15, base `origin/main` `2cafab3ad0`, prototype -branch `spike/2617-typed-adb-addressing` (`72a1467a93`, `b79e96ccd5`, `74c72809a6`). The prototype -is measurement scaffolding: it is not a production migration and must not be merged. +branch `spike/2617-typed-adb-addressing` (`72a1467a93`, `b79e96ccd5`, `74c72809a6`, `5dc8ced575`). +The prototype is measurement scaffolding: it is not a production migration and must not be merged. -Blocking dependency re-audit first: **#2611 is still open** (`headRefOid` `a3a0ea216`, -`mergedAt` null), and `packages/kernel/src/device-shell.ts` does not exist on `main`. Every -device-shell relay claim below therefore describes a branch shape, not merged transport reality, and -the prototype was traced against `main` with the #2611 relay sites read from -`git fetch origin pull/2611/head:pr-2611-audit`. #2026 owns shell quoting; nothing here changes -quoting or the kernel-owned shell fragment representation. +Blocking dependency re-audit first: **#2611 is still open** (`headRefOid` `a3a0ea216`, `mergedAt` +null), and `packages/kernel/src/device-shell.ts` does not exist on `main`. Every device-shell relay +claim below therefore describes a branch shape, not merged transport reality; the relay sites were +read from `git fetch origin pull/2611/head:pr-2611-audit` (6 production `relayDeviceShellArgv` sites, +including the two generic-host copies). #2026 owns shell quoting; nothing here changes quoting or the +kernel-owned shell fragment representation. ## Accepted addressing grammar Read from `adb help` on the measurement host (adb `1.0.41`, version `35.0.2-12147458`), not from the two fields the current code models: -| Grammar form | Modelled today | Modelled in the prototype | +| Grammar form | Modelled on `main` | Modelled in the prototype | | --- | --- | --- | -| `-s SERIAL` | yes (scan + strip) | `target.selector = { kind: 'serial', serial }` | -| `-P PORT` | yes, managed rewrite only | `target.server = { kind: 'port', port }` | -| `-H HOST`, `-L SOCKET`, `-t ID`, `-d`, `-e`, `-a` | refused under a managed lease, invisible otherwise | `target.hostGlobals`, carried verbatim, refused under a managed lease | -| second `-s` (adb lets the later one win) | silently re-parsed positionally | kept as `hostGlobals` so it keeps its winning position | -| `wait-for[-TRANSPORT]-STATE` | prefix-stripped by `assertManagedAdbCommand` only | `target.waitFor`, then the real command is the tail | -| `-P9999` (attached form) | refused under a managed lease | `hostGlobals`, refused under a managed lease | - -`serial` and `serverPort` are two of six owned selections. A proposal that types only those two -reproduces today's blind spot: an ambient `-H foreign.example` is invisible to every router. +| `-s SERIAL` | scanned and stripped (`2cafab3ad0:packages/platform-android/src/adb-provider-scope.ts:252`, `:277`) | `target.selector = { kind: 'serial', serial }` | +| `-P PORT` | only under a lease, by scan-and-replace (`2cafab3ad0:src/platform-runtime-android-adb-host.ts:164`) | `target.server = { kind: 'port', port }`, emitted by one serializer | +| `-P 9999` with no lease | left in argv, no env change | `rawArgv` kept verbatim, no env change (see Findings 6) | +| `-P ` | silently dropped and replaced under a lease | `hostGlobals` → refused under a lease (residual exception) | +| `-H HOST`, `-L SOCKET`, `-a`, `-d`, `-e` | skipped while looking for `-s` (`:252`), refused only under a lease | `target.hostGlobals`, carried verbatim, refused under a lease | +| `-t ID` | aborts the serial scan entirely, so a `-t` selection is neither honoured nor refused | `target.hostGlobals`, refused under a lease | +| second `-s` (adb lets the later one win; verified with `adb -s A -s B get-state`) | both survive; position decides | first becomes `selector`, second stays in `hostGlobals` and keeps its winning position | +| `wait-for[-TRANSPORT]-STATE` | prefix-skipped by `assertManagedAdbCommand` when picking the command | `target.waitFor`, tail becomes `command` | + +`serial` and `serverPort` are two of the selections adb actually offers. Typing only those two keeps +today's blind spot: on an ambient route a `-H foreign.example` is only ever *skipped*, and a `-t 42` +selection stops the scan, so no router can see either one. ## Routes, representation, and payload identity -Measured with a throwaway probe against the real root host binding (`payload = ['shell','input','tap','13','42']`); -"identity" asks whether the array that reaches the exec boundary is the caller's array. +Measured with throwaway probes against the real root host binding, with +`payload = ['shell','input','tap','13','42']`. "Payload identity" asks whether `invocation.command` at +the host port is the caller's array; the emitted argv is always a fresh array on every route, on +`main` and here alike. No committed test asserts reference identity today — that assertion belongs in +the production cut, not in a spike. | Route | Entry | Addressing on `main` | Addressing in the prototype | Payload identity | | --- | --- | --- | --- | --- | -| Local device exec | `runAndroidAdb` → `createDeviceAdbExecutor` | `-s` stitched in the host binding, `-P`/`-s` re-scanned by `withServerPort` (`src/platform-runtime-android-adb-host.ts:164`) | `target` built once, host serialises | held (`command === payload`) | -| Ambient device, smuggled `-s` | same | emits `-s -s `; adb obeys the later one | `rawArgv` kept verbatim, same emitted argv | held | +| Device exec, no provider in scope | `runAndroidAdb` → `resolveAndroidAdbExecutor` → `createDeviceAdbExecutor` (`packages/platform-android/src/adb-provider-scope.ts:124`) | `-s` stitched by `execSerialAdb`, then `-P`/`-s` re-scanned by `withServerPort` (`2cafab3ad0:src/platform-runtime-android-adb-host.ts:86`, `:164`) | one `target`, one serializer | held (`command === payload`) | +| Same route, caller smuggled `-s` | same | emits `-s -s `; adb obeys the later one | `rawArgv` `['-s', device, …args]`, so the emitted argv is identical | held | | Managed device (lease port) | same, `serverPort` set | argv rebuilt twice (`adbInvocation` + `withServerPort`) | `applyManagedAndroidAdbServer` rewrites addressing only | held | -| Managed device, smuggled matching `-s` | same | slice + rebuild | re-slice at parse | **lost** (unavoidable: addressing arrives inside the payload) | -| Provider forwarding | `runCmd('adb', …)` inside `withAndroidAdbProvider` (`adb-provider-scope.ts:201` at HEAD, `:188`/`:204` at main) | `readAdbSerial` + strip, provider gets a rebuilt array | provider gets `invocation.command` | elementwise equal, **not** the caller's array | -| Managed host-level adb | host transport (`adb-provider-scope.ts:228` at HEAD) | `execHostAdb(['-s', scope.serial, …args])` | serial adopted into `target` | held at the host port | -| Managed server-port precedence | request `-P 9999` + lease `15038` | scan-and-replace, refusal on non `-P`/`-s` leading options | `applyManagedAndroidAdbServer` + `requireManagedAndroidAdbSerial` | argv identical (`-P 15038 -s …`), asserted in `src/platform-runtime-android-adb-host.test.ts` | -| Background spawn | `spawnSerialAdb` | `runCmdBackground` (always detaches internally) | `spawnAdb` | see Findings, detach policy | -| Limrun provider | `packages/provider-limrun/src/android.ts:193` | `['-s', serial, …args]` prefix | structural `LimrunAdbInvocation`, tunnel serial as addressing | provider payload kept argv-shaped for `@limrun/api` | -| Generic host command port | `host.commands.run({ executable: 'adb', args: ['-s', …] })` | flat argv, `[...request.args]` copies at `src/platform-runtime-host.ts:21` and `src/platform-runtime-operation-host.ts:60` | untouched by this spike | not measurable: still flat | +| Managed device, smuggled matching `-s` | same | slice + rebuild | re-sliced by the parser | **lost** (addressing arrived inside the payload) | +| Provider forwarding | `runCmd('adb', …)` inside `withAndroidAdbProvider`, arm at `2cafab3ad0:packages/platform-android/src/adb-provider-scope.ts:175` | serial read, then `stripAdbSerialArgs` rebuild | provider receives a copy of `invocation.command` (`adb-provider-scope.ts:214`) | elementwise equal, not the caller's array | +| Managed host-level adb | host transport arm, `2cafab3ad0:…:204` | `execHostAdb(['-s', scope.serial, …args])` | serial adopted into `target` by `withScopedSerialTarget` | held at the host port | +| Managed server-port precedence | request `-P 9999` + lease `15038` | scan-and-replace, refusal on any other leading option | `applyManagedAndroidAdbServer` | argv `['-P','15038','devices']`, asserted at `src/platform-runtime-android-adb-host.test.ts:144`; the device-scoped form with `-s` is asserted at `:131` | +| Background spawn | `spawnSerialAdb` (`2cafab3ad0:src/platform-runtime-android-adb-host.ts:95`) | `runCmdBackground` passes `options.detached` through (`packages/host-kit/src/internal/exec.ts:391`) and `main` never sets it, so background adb is not detached | `spawnAdb`, still not detached | unchanged; see Findings 1 | +| Limrun provider | `2cafab3ad0:packages/provider-limrun/src/android.ts:193` | `['-s', serial, …args]` prefix | structural `LimrunAdbInvocation`, tunnel serial as addressing | provider-facing executor stays argv-shaped for `@limrun/api` | +| Generic host command port | `host.commands.run({ executable: 'adb', args: ['-s', …] })`, copied again at `src/platform-runtime-host.ts:21` and `src/platform-runtime-operation-host.ts:60` | flat argv | untouched by this spike | not measurable: still flat | ## Proposed API -`packages/platform-android/src/adb-addressing.ts` (241 lines at HEAD) owns grammar and managed policy: +`packages/platform-android/src/adb-addressing.ts` (241 lines) owns grammar and managed policy: ```ts type AndroidAdbSelector = { kind: 'unspecified' } | { kind: 'serial'; serial: string }; @@ -67,122 +73,154 @@ type AndroidAdbInvocation = Readonly<{ ``` Two directions only: `parseAndroidAdbArgv` (the single place that re-slices a payload, and only when -addressing was actually present — `command` is the caller's array otherwise) and -`serializeAndroidAdbInvocation` (the single emitter; returns `rawArgv` verbatim when addressing was -never rewritten). Managed rules: `applyManagedAndroidAdbServer`, `requireManagedAndroidAdbSerial`, -`requireManagedAndroidAdbCommand`, `androidManagedAdbEnvironment`. The host port collapses -`execSerialAdb`/`spawnSerialAdb`/`execHostAdb` into `execAdb`/`spawnAdb` taking an invocation, so -`src/platform-runtime-android-adb-host.ts` nets **-53 lines**. The port-side test host still holds -its own copy of the lowering, but now calls the shared `applyManagedAndroidAdbServer` and serializer -instead of rebuilding `-P`/`-s` by hand. +addressing was present — otherwise `command` is the caller's array) and +`serializeAndroidAdbInvocation` (the single emitter; returns `rawArgv` verbatim when nothing was +rewritten). Managed rules: `applyManagedAndroidAdbServer`, `requireManagedAndroidAdbSerial`, +`requireManagedAndroidAdbCommand`, `androidManagedAdbEnvironment`, and +`androidAdbOwnedServerPort`, which separates a lease-owned port from a `-P` the caller typed. The host +port collapses `execSerialAdb`/`spawnSerialAdb`/`execHostAdb` into `execAdb`/`spawnAdb`, so +`src/platform-runtime-android-adb-host.ts` nets **-53 lines** (`+33 -86`). The in-package test host +still holds its own copy of the lowering, but now calls the shared server-rewrite and serializer +instead of rebuilding `-P`/`-s` by hand; it still does not model the env carrier. ## Numbers | Measure | `main` | prototype | | --- | --- | --- | -| Independently authored addressing/refusal owners | 11: serial scan + strip, scope-identity refusal, port precedence refusal, two host-level `-s` stitches, `withServerPort` scan + refusal, `assertManagedAdbCommand`, managed env lowering, detach-by-method-name, Limrun device prefix, Limrun host prefix, eight route-level `-s` stitches | 7 of them answer to `adb-addressing.ts`; scope-identity and port precedence stay in provider scope (they are scope questions, not grammar), detach became an explicit option on the device executor, and the eight route-level stitches wait for cut-set step 4 | -| Literal `'-s'` producers/consumers in production | 17 | 13 (3 in the owning module, 1 ambient escape, 8 untouched route sites) | -| New parsers/serializers | — | 1 parser, 1 serializer, 1 env builder | -| New bridges | — | 2, both in `provider-limrun` (structural `LimrunAdbInvocation`, and `limrunAdbArgv` to keep failure text identical) | -| Production LOC | — | `+481 -199` (net **+282**); outside the new 241-line module, net **+41** | +| Independently authored addressing/refusal owners (11 named) | serial scan + strip, scope-identity refusal, port-precedence refusal, two host-level `-s` stitches, `withServerPort` scan + refusal, `assertManagedAdbCommand`, managed env lowering, detach-by-method, Limrun device prefix, Limrun host prefix, eight route-level `-s` stitches | 5 answer to `adb-addressing.ts` (scan + strip, the two stitches, `withServerPort` + refusal, `assertManagedAdbCommand`, env lowering); the two Limrun ones moved to a structural duplicate inside `provider-limrun`, which cut-set step 1 says to remove; detach became an explicit option; scope-identity, port precedence and the eight route-level stitches stayed put | +| Literal `'-s'` producers/consumers in production (excluding tests, fixtures and non-adb uses) | 17 | 13: 3 in the owning module, 1 ambient `rawArgv` escape, 1 Limrun serializer, 8 untouched route sites | +| New parsers/serializers | — | 1 parser, 1 serializer, 1 env builder, 1 owned-port predicate | +| New bridges | — | 2, both in `provider-limrun`: structural `LimrunAdbInvocation` (`runtime-dependencies.ts:103`) and `limrunAdbArgv` (`android.ts:227`) to keep failure text identical | +| Extra payload copies | — | +1 at the provider hand-off (`adb-provider-scope.ts:214`, `readonly` → mutable); the two generic-host `[...request.args]` copies are untouched | +| Production LOC | — | `+481 -199` (net **+282**); outside the 241-line module, net **+41** | | Test LOC | — | `+255 -108`, all of it migrated assertions on the same argv expectations, no new synthetic executor | -| argv rebuild sites in the five transport files | 3 (`stripAdbSerialArgs`, `withServerPort`, app-log copy) | 2 (app-log copy, plus one `readonly`→mutable copy at the provider hand-off) | -| `readonly` payload propagation cost (executor payload → `readonly string[]`) | n/a | 96 type errors in 24 files, **1 in production source** (`adb-provider-scope.ts:57`, one `string[]` → `readonly string[]` widening) | - -Gate status on the prototype: 118 files / 846 tests green (`packages/platform-android`, -`packages/provider-limrun`, `src/platform-runtime-android-adb-host.test.ts`, -`src/managed-device-reachability.test.ts`), plus 12 files / 59 tests green across the released-surface, -SDK, provider-scope, dialog-readiness and routing-parity lanes. Root `tsc -p tsconfig.json` reports 0 -errors; `pnpm check:layering` and `pnpm lint` pass. +| `readonly` payload cost | n/a | throwaway mutation of `AndroidAdbExecutor`'s payload to `readonly string[]` (not committed): 96 type errors in 24 files, **1 in production source** (`adb-provider-scope.ts:57`, one signature widening) | + +Gate status on the prototype: 118 files / 846 tests green for +`pnpm test:unit packages/platform-android packages/provider-limrun src/platform-runtime-android-adb-host.test.ts src/managed-device-reachability.test.ts`, +and 12 files / 59 tests green for the released-surface, SDK, provider-scope, dialog-readiness, +readiness-spawn and routing-parity lanes, i.e. `pnpm test:unit src/__tests__/android-adb-public.test.ts +src/sdk/android-adb.test.ts src/__tests__/limrun-runtime.test.ts +src/daemon/__tests__/request-handler-chain-provider-scope.test.ts +src/daemon/__tests__/android-owner-seam.test.ts src/daemon/__tests__/android-dialog-readiness.test.ts +src/daemon/interaction/internal/__tests__/interaction-touch-android-readiness-spawns.test.ts +src/daemon/interaction/internal/__tests__/interaction-type-android-readiness.test.ts +src/__tests__/platform-runtime-android-application-tools.test.ts +src/__tests__/platform-runtime-runtime-hints.test.ts src/__tests__/package-exports.test.ts +src/daemon/__tests__/providers-plugin-routing-parity.test.ts`. Root `npx tsc -p tsconfig.json` reports 0 errors; `pnpm lint` and +`pnpm check:layering` pass. ## Findings a migration must carry -- **The detach policy rode on the method name.** `execSerialAdb` detached, `execHostAdb` and - `spawnSerialAdb` did not (`2cafab3ad0:src/platform-runtime-android-adb-host.ts:85`, `:95`, `:105`). - Collapsing to one `execAdb` keyed on "target has a serial" silently detached host-level managed - calls, which changes timeout/`killProcessTree` behaviour. The prototype carries the decision as an - additive `AndroidAdbExecutorOptions.detached` set by the device-scoped executor only. -- **A hand-written typed guard disagreed with the shared rule on first contact.** The first version of - `requireManagedAndroidAdbCommand` checked `command[0]`, while `assertManagedAdbCommand` finds the - first token that is not `wait-for-*` (`2cafab3ad0:src/platform-runtime-android-adb-host.ts:189`). - Three refusal cases (`wait-for-device kill-server`, `wait-for-device disconnect`, - `wait-for-any-device pair …`) and every host-global selector (`-H`, `-L`, `-t`, `-d`, `-e`, `-P9999`) - passed silently. The existing 21-selector loop in - `src/platform-runtime-android-adb-host.test.ts` is what caught it; it is the oracle for this grammar - and belongs in a golden table under `contracts/fixtures/` if the rules go cross-language. -- **The port carrier differed between the real host and the unit-test host.** The real lowering strips - `serverPort` and emits `-P` plus `ANDROID_ADB_SERVER_PORT`/`ANDROID_ADB_SERVER_ADDRESS`; the - in-package test host passed `serverPort` through untouched, so - `packages/platform-android/src/__tests__/adb-executor.test.ts` asserted the stub's carrier, not the - production one. Sharing `lowerAndroidAdbInvocation` moved that assertion to argv, where it is now the - same contract. That shared lowering is a cut-set item, not a free win. -- **Flat ingress cannot be fixed downstream.** Provider forwarding and the eight generic host command - port sites (`logs/runtime.ts` ×2, `network/runtime.ts` ×2, `readiness/runtime.ts`, - `shutdown/runtime.ts`, `inventory.ts` via `host.commands.run`, and - `src/platform-runtime-app-log-process.ts:187`) deliver one argv. There the payload is by definition - re-sliced, and `[...request.args]` at `src/platform-runtime-host.ts:21` / - `src/platform-runtime-operation-host.ts:60` copies it again. Identity survives only where the caller - already separates addressing. -- **`@limrun/api` pins argv at the provider edge.** `LimrunAdbExecutor(args)` is handed to an external - package, so the provider-facing executor stays argv-shaped; only the host port - (`LimrunRuntimeDependencies.host.runAdb`) can be typed. `provider-limrun` declares no - `platform-android` dependency, so the prototype duplicated the invocation structurally and mirrored - argv for failure text — two bridges that a shared addressing home would remove. -- **Released surface is narrower than it looks.** `agent-device/android-adb` publishes - `AndroidAdbExecutor`, `AndroidAdbExecutorOptions`, `AndroidAdbProvider` and nine `*WithAdb` - helpers (`src/sdk/android-adb.ts`), so the executor payload must stay `(args: string[])` and - `detached` must stay additive. `AndroidAdbHost` and `AndroidAdbInvocation` live on private workspace - packages (`@agent-device/platform-android`, `@agent-device/provider-limrun`) and are not in the - published export list, so they carry no external compatibility obligation. +1. **The detach policy rode on which host method you called.** `execSerialAdb` detached on non-Windows + (`2cafab3ad0:src/platform-runtime-android-adb-host.ts:85`, `:91`); `spawnSerialAdb` (`:95`) and + `execHostAdb` (`:105`) did not, and `runCmdBackground` forwards `options.detached` + (`packages/host-kit/src/internal/exec.ts:391`) rather than forcing it. Collapsing to one `execAdb` + and keying the flag on "target has a serial" silently detached managed host-level calls, which + changes timeout and `killProcessTree` behaviour. The spike carries the decision as an additive + `AndroidAdbExecutorOptions.detached` set only by the device-scoped executor. +2. **A hand-written typed guard disagreed with the shared rule on first contact** (`72a1467a93`). Two + separate escapes: the device route rebuilt the target from the serial alone, so parsed + `hostGlobals` never reached any refusal (`-H`, `-L`, `-t`, `-d`, `-e`, `-P9999` all ran), and + `requireManagedAndroidAdbCommand` tested `command[0]` where + `assertManagedAdbCommand` picks the first token that does not start with `wait-for-` + (`2cafab3ad0:src/platform-runtime-android-adb-host.ts:189`), so `wait-for-device kill-server`, + `wait-for-device disconnect` and `wait-for-any-device pair …` ran too. The existing 21-selector loop + in `src/platform-runtime-android-adb-host.test.ts:100` caught all nine; it is the oracle for this + grammar and belongs in a golden table under `contracts/fixtures/` if the rules go cross-language. +3. **The port carrier differed between the real host and the in-package test host.** The real lowering + strips `serverPort` and emits `-P` plus `ANDROID_ADB_SERVER_PORT`/`ANDROID_ADB_SERVER_ADDRESS`; the + unit host passed `serverPort` through untouched, so + `packages/platform-android/src/__tests__/adb-executor.test.ts` asserted the stub's carrier. Sharing + the rewrite moved that assertion to argv, where it now tests the production contract. Deleting the + test-host copy of the lowering, env carrier included, is a cut-set item, not a free win. +4. **Flat ingress cannot be fixed downstream.** Nine sites hand `-s` to a flat argv: + `logs/runtime.ts:71`, `:105`, `network/runtime.ts:123`, `:156`, `readiness/runtime.ts:119`, + `shutdown/runtime.ts:39`, `inventory.ts:246` (all through `host.commands.run`), plus + `src/platform-runtime-app-log-process.ts:187` (through `runCmdBackground`). There the payload is + re-sliced by definition, and `[...request.args]` copies it again at `src/platform-runtime-host.ts:21` + and `src/platform-runtime-operation-host.ts:60`. Identity survives only where the producer already + separates addressing. +5. **`@limrun/api` pins argv at the provider edge.** `LimrunAdbExecutor(args)` is handed to that + external package, so the provider-facing executor has to stay argv-shaped; only the host port + (`LimrunRuntimeDependencies.host.runAdb`) can be typed. `provider-limrun` declares no + `platform-android` dependency, so the spike duplicated the invocation structurally and mirrored argv + for failure text — the two bridges a shared addressing home removes. +6. **Two ambient divergences surfaced in adversarial review and were closed** (`5dc8ced575`). Treating + any `target.server` port as lease-owned meant a caller-typed `-P 9999` on a host-level call started + lowering `ANDROID_ADB_SERVER_*` and applying managed command refusal, which `main` never did; and a + non-numeric `-P` became a refusal where `main` dropped it silently under a lease. The fix keeps + caller-typed addressing in `rawArgv` and reads the lease port through `androidAdbOwnedServerPort`. + Verified: `['-P','9999','devices']` now emits `["-P","9999","devices"]` with no env change and an + inherited `ADB_SERVER_SOCKET`, and `['-P','9999','kill-server']` still runs. The non-numeric case + stays a deliberate residual exception: refusing is safer than silently rewriting, but it is a + behavior change. +7. **`detached` already existed on `AndroidAdbSpawnOptions`**, so the finding is not that the flag was + invented here; it is that the exec-side policy had no carrier and lived in the method name. +8. **Released surface is narrower than it looks.** `agent-device/android-adb` publishes + `AndroidAdbExecutor`, `AndroidAdbExecutorOptions`, `AndroidAdbProvider`, `AndroidPortReverseEndpoint` + and nine `*WithAdb` helpers (`src/sdk/android-adb.ts:2`-`:34`), so the executor payload must stay + `(args: string[])` and `detached` must stay additive. `AndroidAdbHost` and `AndroidAdbInvocation` + live on private workspace packages (`@agent-device/platform-android`, + `@agent-device/provider-limrun`) and appear in no published export list, so they carry no external + compatibility obligation. ## Recommendation: continue, with a smaller cut than the prototype -Continue. The typed representation moves seven independently authored addressing decisions into one -module, costs +41 production lines outside that module, and needs one production signature change to make -the payload read-only across the platform. Stop criteria are not met: bridges did not multiply (2, both -explained by a missing shared home), enforcement got one owner instead of three, and no lifecycle -behaviour needs to change once `detached` is an option. The cut set is: +Continue. Five independently authored addressing decisions now answer to one module, a sixth and +seventh are ready to follow once addressing has a shared home, the change costs +41 production lines +outside that module, and one signature widening makes the payload read-only across the platform. +Enforcement did not weaken: the four refusal throw sites on `main` became two owners — the addressing +module for grammar and lease rules, provider scope for scope-identity and port precedence, which answer +scope questions rather than grammar. Stop criteria are not met: no lifecycle behaviour had to change +once `detached` was an option, and the two new bridges are both explained by one missing home. Cut set: 1. Decide the addressing home first: a tiny `@agent-device/adb-addressing` package, or an explicit `provider-limrun` → `platform-android` edge. Do not ship the structural duplicate. -2. Land the addressing module + host port collapse (`execAdb`/`spawnAdb`) with - `lowerAndroidAdbInvocation` exported from the platform package so the real and test hosts share it, - and `detached` as an explicit option. -3. Move the managed refusal set and the grammar into a golden table under `contracts/fixtures/` and - keep the 21-selector loop as the parity oracle. -4. Re-route the eight generic host command port adb calls to the android host port instead of - `commands.run`; this is where the copies and the `-s` stitches actually disappear. +2. Land the addressing module and the host port collapse (`execAdb`/`spawnAdb`) with the lowering and + the env carrier exported from the platform package, so the real host and the test host cannot + diverge, and with `detached` as an explicit option. +3. Move the managed refusal set and the grammar into a golden table under `contracts/fixtures/`, + keeping the 21-selector loop as the parity oracle. +4. Re-route the nine generic host command port adb calls onto the android host port instead of + `commands.run`; this is where the `-s` stitches and the argv copies actually disappear. 5. Type the provider host port (`host.runAdb`) and keep `LimrunAdbExecutor` argv-shaped. -6. Re-audit after #2611 lands: its `relayDeviceShellArgv` sites (6 in production, including the two - generic-host copies) are the provenance relay this proposal should make unnecessary at steps 2 and 4. +6. Add a committed payload-identity assertion at the host port; the spike measured identity with + throwaway probes only. +7. Re-audit after #2611 lands: its six `relayDeviceShellArgv` sites are the provenance relays that + steps 2 and 4 should make unnecessary. Validation lanes for the production proposal: `pnpm test:unit packages/platform-android packages/provider-limrun`, `pnpm test:unit src/platform-runtime-android-adb-host.test.ts src/managed-device-reachability.test.ts src/sdk/limrun-runtime-dependencies.test.ts src/__tests__/android-adb-public.test.ts`, `pnpm check:affected --run`, plus a device-backed pass -through `test/integration/provider-scenarios/android-lifecycle.test.ts` for screenshot/`exec-out` -binary results and background `logcat` spawn, which no unit lane covers. +through `test/integration/provider-scenarios/android-lifecycle.test.ts` for `exec-out` binary results +and background `logcat` spawn, which no unit lane covers. ## Unresolved evidence and pitfalls -- Device-backed verification was not run: this is a docs-only deliverable and the spike never claimed - runner or on-device parity, only argv/refusal/identity parity against existing lanes. +- Device-backed verification was not run: this is a docs-only deliverable, and the spike claims only + argv, refusal and identity parity against existing lanes. +- Ambient routes keep argv authority: `rawArgv` wins at serialization, so on those routes `target` is + advisory and a later `-s` still wins. `rawArgv` is a heuristic, not the design: a production shape + needs explicit provenance for "caller requested" versus "lease owns" instead of inferring it from + which fields are populated. That decision is unresolved and deliberately out of scope here. +- `-P ` under a lease now refuses where `main` silently replaced it (Findings 6). - `pnpm typecheck` on `2cafab3ad0` fails in `packages/platform-android/src/__tests__/snapshot-helper-install.test.ts:29` (`installArgs` was dropped from `AndroidSnapshotHelperManifest` by `f07a4eca31` (#2618) while the fixture still sets it, added by `2cafab3ad0` (#2619)). Because `typecheck` chains with `&&`, that one - package error means the root and `examples/sdk` projects are never checked. All root typecheck claims - here come from running `npx tsc -p tsconfig.json` directly. Fix the fixture separately. + package error means `tsc -p tsconfig.json` and the `examples/sdk` project never run. All root + typecheck claims here come from running `npx tsc -p tsconfig.json` directly. Fix the fixture + separately. - `src/sdk/limrun-runtime-dependencies.test.ts > Limrun construction defers operation and - opposite-platform helper modules` fails on pristine `main` on this host (real-time budget), so the - eager-closure claim rests on import inspection instead: `adb-addressing.ts` imports only - `@agent-device/kernel/errors`, and `src/__tests__/package-exports.test.ts` stays green. -- `pnpm check:production-exports` fails on `main` with 68 unused-export findings in 39 files - (`packages/maestro`, `packages/capture-kit`, `src/runtime.ts`, …). None of those files is in this - diff, and none of the exports this spike adds is flagged, so the count is unchanged; the gate was - not green before and is not green now. -- Ambient routes keep argv authority: `rawArgv` wins at serialization, so on those routes `target` is - advisory and the later `-s` still wins. Typing ambient addressing means choosing a precedence rule - that today's behaviour does not have; that decision is unresolved and deliberately out of scope here. -- `pnpm stash`-style isolation is unusable for this kind of spike in parallel worktrees: `git stash` - is scoped to the shared common git directory, so a `stash pop` in one worktree can apply another - agent's stash. Use a commit on a spike branch and `git checkout --` to go back. + opposite-platform helper modules` is timing-dependent on this host: seen failing against its budget on + `2cafab3ad0` and passing at 4.3 s at HEAD. Treat it as flaky, not as evidence. The eager-closure claim + therefore rests on import inspection: `adb-addressing.ts` imports only `@agent-device/kernel/errors`, + and `src/__tests__/package-exports.test.ts` stays green. +- `pnpm check:production-exports` exits 0 because `fallow-production-exports.json` sets + `unused-exports` to warn, while reporting 68 unused exports across 40 files. None of those files is in + this diff and none of the exports this spike adds is flagged, so the count is unchanged — but do not + read a green exit here as an unused-export clean bill. +- `git stash` is unusable for isolation in parallel worktrees: the stash ref lives in the shared common + git directory, so a `stash pop` in one worktree can apply another agent's stash and drop the entry. + Commit on a spike branch and use `git checkout --` to go back.