Skip to content

Commit e49c2bd

Browse files
thymikeethymikee
committed
refactor(android): separate adb transport addressing from the command 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.
1 parent 2ad819c commit e49c2bd

27 files changed

Lines changed: 1588 additions & 376 deletions

‎packages/platform-android/src/__tests__/adb-executor.test.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -245,15 +245,17 @@ test('createLocalAndroidAdbProvider carries a private server port through every
245245
await provider.pull?.('/sdcard/video.mp4', '/tmp/video.mp4');
246246
await provider.install?.('/tmp/app.apk');
247247

248-
assert.equal(readServerPort(mockRunCmdBackground.mock.calls[0]?.[2]), 15_037);
248+
assert.equal(readServerPortArgv(mockRunCmdBackground.mock.calls[0]?.[1]), 15_037);
249249
assert.equal(mockRunCmd.mock.calls.length, 4);
250-
for (const call of mockRunCmd.mock.calls) assert.equal(readServerPort(call[2]), 15_037);
250+
for (const call of mockRunCmd.mock.calls) assert.equal(readServerPortArgv(call[1]), 15_037);
251251
});
252252

253-
function readServerPort(options: unknown): number | undefined {
254-
if (options === null || typeof options !== 'object') return undefined;
255-
const value = (options as { serverPort?: unknown }).serverPort;
256-
return typeof value === 'number' ? value : undefined;
253+
function readServerPortArgv(args: unknown): number | undefined {
254+
if (!Array.isArray(args)) return undefined;
255+
const index = args.indexOf('-P');
256+
if (index === -1) return undefined;
257+
const value = args[index + 1];
258+
return typeof value === 'string' ? Number(value) : undefined;
257259
}
258260

259261
test('createAndroidPortReverseManager makes duplicate setup idempotent and cleans owner mappings', async () => {

‎packages/platform-android/src/__tests__/test-utils/android-host-test-setup.ts‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,32 +7,34 @@ import {
77
withoutCommandExecutorOverride,
88
} from '@agent-device/host-kit/command';
99
import { emitDiagnostic } from '@agent-device/host-kit/diagnostics';
10+
import { lowerAndroidAdbInvocation } from '../../adb-transport.ts';
1011
import { bindAndroidAdbHostStub } from '../../adb-host.fixtures.ts';
1112
import { createAndroidFileHost } from './android-file-host.ts';
1213

1314
export function bindAndroidAdbTestHost() {
1415
return bindAndroidAdbHostStub({
1516
environment: process.env,
1617
files: createAndroidFileHost(),
17-
execSerialAdb: async (serial, args, options) =>
18-
await withoutCommandExecutorOverride(
18+
execAdb: async (invocation, options) => {
19+
const lowered = lowerAndroidAdbInvocation(invocation, options, process.env);
20+
return await withoutCommandExecutorOverride(
1921
async () =>
20-
await runCmd('adb', ['-s', serial, ...args], {
21-
...options,
22+
await runCmd('adb', lowered.args, {
23+
...lowered.options,
2224
detached: process.platform !== 'win32',
2325
}),
24-
),
25-
spawnSerialAdb: (serial, args, options) => {
26-
const background = runCmdBackground('adb', ['-s', serial, ...args], {
27-
...options,
26+
);
27+
},
28+
spawnAdb: (invocation, options) => {
29+
const lowered = lowerAndroidAdbInvocation(invocation, options, process.env);
30+
const background = runCmdBackground('adb', lowered.args, {
31+
...lowered.options,
2832
allowFailure: true,
2933
captureOutput: false,
3034
});
3135
void background.wait.catch(() => {});
3236
return background.child;
3337
},
34-
execHostAdb: async (args, options) =>
35-
await runCmd('adb', args, { ...options, detached: process.platform !== 'win32' }),
3638
withAdbCommandExecutorOverride: withCommandExecutorOverride,
3739
withoutAdbCommandExecutorOverride: withoutCommandExecutorOverride,
3840
coerceAdbResult: coerceExecResult,

‎packages/platform-android/src/adb-executor-host.test.ts‎

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,26 @@
11
import assert from 'node:assert/strict';
22
import { test } from 'vitest';
33
import { AppError } from '@agent-device/kernel/errors';
4+
import { parseAndroidAdbArgv } from './adb-transport.ts';
45
import { bindAndroidAdbHostStub } from './adb-host.fixtures.ts';
56
import { runAndroidHostAdb, withAndroidHostAdbTransport } from './adb-executor.ts';
67

78
test('a scoped transport intercepts host adb without reaching the injected host', async () => {
89
let hostCalls = 0;
910
bindAndroidAdbHostStub({
10-
execHostAdb: async () => {
11+
execAdb: async () => {
1112
hostCalls += 1;
1213
return { stdout: 'host', stderr: '', exitCode: 0 };
1314
},
1415
});
1516

1617
const result = await withAndroidHostAdbTransport(
17-
async (args, options) => {
18-
assert.deepEqual(args, ['devices']);
18+
async (invocation, options) => {
19+
assert.deepEqual(invocation.command, ['devices']);
1920
assert.deepEqual(options, { timeoutMs: 1_234 });
2021
return { stdout: 'transport', stderr: '', exitCode: 0 };
2122
},
22-
async () => await runAndroidHostAdb(['devices'], { timeoutMs: 1_234 }),
23+
async () => await runAndroidHostAdb(parseAndroidAdbArgv(['devices']), { timeoutMs: 1_234 }),
2324
);
2425

2526
assert.equal(result.stdout, 'transport');
@@ -29,13 +30,13 @@ test('a scoped transport intercepts host adb without reaching the injected host'
2930
test('the local host arm always obtains a result before applying the shared failure contract', async () => {
3031
let receivedOptions: Record<string, unknown> | undefined;
3132
bindAndroidAdbHostStub({
32-
execHostAdb: async (_args, options) => {
33+
execAdb: async (_invocation, options) => {
3334
receivedOptions = options;
3435
return { stdout: '', stderr: 'error: device offline', exitCode: 1 };
3536
},
3637
});
3738

38-
const error = await runAndroidHostAdb(['devices']).then(
39+
const error = await runAndroidHostAdb(parseAndroidAdbArgv(['devices'])).then(
3940
() => assert.fail('expected the host adb call to reject'),
4041
(error: unknown) => error,
4142
);
@@ -49,10 +50,13 @@ test('the local host arm always obtains a result before applying the shared fail
4950

5051
test('allowFailure returns a nonzero local result unchanged', async () => {
5152
const scripted = { stdout: '', stderr: 'offline', exitCode: 7 };
52-
bindAndroidAdbHostStub({ execHostAdb: async () => scripted });
53+
bindAndroidAdbHostStub({ execAdb: async () => scripted });
5354

5455
assert.deepEqual(
55-
await runAndroidHostAdb(['devices'], { allowFailure: true, timeoutMs: 5_000 }),
56+
await runAndroidHostAdb(parseAndroidAdbArgv(['devices']), {
57+
allowFailure: true,
58+
timeoutMs: 5_000,
59+
}),
5660
scripted,
5761
);
5862
});
@@ -70,15 +74,15 @@ test('unchecked transport results are normalized at the package boundary', async
7074

7175
const result = await withAndroidHostAdbTransport(
7276
async () => sloppy,
73-
async () => await runAndroidHostAdb(['devices'], { allowFailure: true }),
77+
async () => await runAndroidHostAdb(parseAndroidAdbArgv(['devices']), { allowFailure: true }),
7478
);
7579

7680
assert.deepEqual(result, { stdout: '', stderr: '', exitCode: 1 });
7781
});
7882

7983
test('nested transport scopes are innermost-first and restore on scope exit', async () => {
8084
bindAndroidAdbHostStub({
81-
execHostAdb: async () => ({ stdout: 'host', stderr: '', exitCode: 0 }),
85+
execAdb: async () => ({ stdout: 'host', stderr: '', exitCode: 0 }),
8286
});
8387
const transportFor = (name: string) => async () => ({
8488
stdout: name,
@@ -87,11 +91,11 @@ test('nested transport scopes are innermost-first and restore on scope exit', as
8791
});
8892

8993
await withAndroidHostAdbTransport(transportFor('outer'), async () => {
90-
assert.equal((await runAndroidHostAdb(['devices'])).stdout, 'outer');
94+
assert.equal((await runAndroidHostAdb(parseAndroidAdbArgv(['devices']))).stdout, 'outer');
9195
await withAndroidHostAdbTransport(transportFor('inner'), async () => {
92-
assert.equal((await runAndroidHostAdb(['devices'])).stdout, 'inner');
96+
assert.equal((await runAndroidHostAdb(parseAndroidAdbArgv(['devices']))).stdout, 'inner');
9397
});
94-
assert.equal((await runAndroidHostAdb(['devices'])).stdout, 'outer');
98+
assert.equal((await runAndroidHostAdb(parseAndroidAdbArgv(['devices']))).stdout, 'outer');
9599
});
96-
assert.equal((await runAndroidHostAdb(['devices'])).stdout, 'host');
100+
assert.equal((await runAndroidHostAdb(parseAndroidAdbArgv(['devices']))).stdout, 'host');
97101
});

‎packages/platform-android/src/adb-host.fixtures.ts‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,11 @@ export function bindAndroidAdbHostStub(
5757
throw new Error('adb-host stub: writeBytes not stubbed');
5858
},
5959
},
60-
execSerialAdb: async () => {
61-
throw new Error('adb-host stub: execSerialAdb not stubbed');
60+
execAdb: async () => {
61+
throw new Error('adb-host stub: execAdb not stubbed');
6262
},
63-
spawnSerialAdb: () => {
64-
throw new Error('adb-host stub: spawnSerialAdb not stubbed');
65-
},
66-
execHostAdb: async () => {
67-
throw new Error('adb-host stub: execHostAdb not stubbed');
63+
spawnAdb: () => {
64+
throw new Error('adb-host stub: spawnAdb not stubbed');
6865
},
6966
withAdbCommandExecutorOverride: async (_override, fn) => await fn(),
7067
withoutAdbCommandExecutorOverride: async (fn) => await fn(),

‎packages/platform-android/src/adb-host.ts‎

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { AsyncLocalStorage } from 'node:async_hooks';
22
import type { AndroidHelperInstallDecision, AndroidImeHelperArtifact } from './helper-artifacts.ts';
33
import type {
4+
AndroidAdbInvocation,
45
AndroidAdbExecutor,
56
AndroidAdbExecutorOptions,
67
AndroidAdbExecutorResult,
@@ -45,26 +46,18 @@ export type AndroidAdbHost = Readonly<{
4546
/** Narrow filesystem authority used by Android helper, SDK, and artifact mechanics. */
4647
files: AndroidAdbFileHost;
4748
/**
48-
* Device-scoped local adb execution for `serial`, escaping any active command-executor
49-
* override (a tunnel-backed provider shelling out to adb must not route back into itself)
50-
* and owning the host-side process-group/teardown semantics.
49+
* Local adb execution for one addressing decision, visible to an installed command-executor
50+
* override so a scoped transport can answer for it. A caller that must not be captured — the
51+
* device-scoped executor a provider would otherwise re-enter — wraps this in
52+
* `withoutAdbCommandExecutorOverride`. The invocation's command is appended verbatim; only
53+
* `target` is lowered into adb global options.
5154
*/
52-
execSerialAdb(
53-
serial: string,
54-
args: string[],
55-
options?: AndroidAdbExecutorOptions,
56-
): Promise<AndroidAdbExecutorResult>;
57-
/** Device-scoped local adb background spawn for `serial`; the host owns stream wiring. */
58-
spawnSerialAdb(
59-
serial: string,
60-
args: string[],
61-
options?: AndroidAdbSpawnOptions,
62-
): AndroidAdbProcess;
63-
/** Host-global adb execution (no serial), e.g. `adb devices`. */
64-
execHostAdb(
65-
args: string[],
55+
execAdb(
56+
invocation: AndroidAdbInvocation,
6657
options?: AndroidAdbExecutorOptions,
6758
): Promise<AndroidAdbExecutorResult>;
59+
/** Local adb background spawn for one addressing decision; the host owns stream wiring. */
60+
spawnAdb(invocation: AndroidAdbInvocation, options?: AndroidAdbSpawnOptions): AndroidAdbProcess;
6861
/** Installs `override` as the host command-executor override for the duration of `fn`. */
6962
withAdbCommandExecutorOverride<T>(
7063
override: AndroidAdbCommandExecutorOverride,
@@ -109,9 +102,9 @@ export type AndroidAdbHost = Readonly<{
109102

110103
let boundHost: AndroidAdbHost | undefined;
111104

112-
/** Scoped override for host-global and explicitly serial-qualified adb argv. */
105+
/** Scoped override for host-global and explicitly serial-qualified adb invocations. */
113106
export type AndroidAdbHostTransport = (
114-
args: string[],
107+
invocation: AndroidAdbInvocation,
115108
options?: AndroidAdbExecutorOptions,
116109
) => Promise<AndroidAdbExecutorResult>;
117110

@@ -138,20 +131,21 @@ export function requireAndroidAdbHost(): AndroidAdbHost {
138131
* innermost-first and restore automatically.
139132
*/
140133
export async function runAndroidHostAdb(
141-
args: string[],
134+
invocation: AndroidAdbInvocation,
142135
options?: AndroidAdbExecutorOptions,
143136
): Promise<AndroidAdbExecutorResult> {
144137
const host = requireAndroidAdbHost();
145138
const transport = androidAdbHostTransportScope.getStore();
146139
const result = host.coerceAdbResult(
147140
transport
148-
? await transport(args, options)
149-
: await host.execHostAdb(args, { ...options, allowFailure: true }),
141+
? await transport(invocation, options)
142+
: await host.execAdb(invocation, { ...options, allowFailure: true }),
150143
);
151144
if (!options?.allowFailure && result.exitCode !== 0) {
152145
const { androidAdbResultError } = await import('./adb-failure.ts');
146+
const { serializeAndroidAdbInvocation } = await import('./adb-transport.ts');
153147
throw androidAdbResultError(
154-
`adb ${args.join(' ')} exited with code ${result.exitCode}`,
148+
`adb ${serializeAndroidAdbInvocation(invocation).join(' ')} exited with code ${result.exitCode}`,
155149
result,
156150
);
157151
}

0 commit comments

Comments
 (0)