Skip to content

Commit fa3d70d

Browse files
thymikeethymikee
committed
feat: classify tolerated adb failures and semantic provider methods
Review follow-up closing two classifier coverage gaps: 1. allowFailure paths bypassed the classifier: the executor wrapper only enriches THROWN errors, but many Android flows run adb with allowFailure, inspect the nonzero result, and throw a fresh AppError built from its stdout/stderr — so e.g. "device offline" during uninstall still surfaced the generic hint. There was no common construction point for these errors, so androidAdbResultError in adb-executor.ts now IS that point: it builds the COMMAND_FAILED error from a tolerated result and runs it through the classifier, keeping hint strings central. The result-inspecting throw sites (app launch/uninstall, appearance read, keyboard/clipboard queries, package listing, logcat capture, helper APK installs, heap-dump pull, uiautomator dump) route through it; a site hint (heap-dump pull) still wins because attachAdbFailureHint never overwrites. 2. Provider-scoped enrichment only wrapped exec, but the transfer helpers prefer the semantic provider methods, so a provider whose install rejects with INSTALL_FAILED output kept the generic hint. Enrichment now lives in normalizeAndroidAdbProvider — the single funnel every provider passes through (scope installation and explicitly passed providers alike) — wrapping exec plus pull/install/installBundle; a WeakSet keeps repeated normalization from stacking wrappers. The local provider needs no wrap since its methods delegate to the already enriched serial executor. Tests: androidAdbResultError classification + site-hint precedence and provider install/pull classification in adb-executor.test.ts; an allowFailure regression on a real production path (keyboard state query with a nonzero device-offline result) in device-input-state.test.ts.
1 parent 5438520 commit fa3d70d

12 files changed

Lines changed: 206 additions & 78 deletions

‎src/platforms/android/__tests__/adb-executor.test.ts‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ vi.mock('../../../utils/exec.ts', async (importOriginal) => {
1818
});
1919

2020
import {
21+
androidAdbResultError,
2122
attachAdbFailureHint,
2223
classifyAdbFailure,
2324
createAndroidPortReverseManager,
@@ -467,6 +468,81 @@ test('port reverse removal failures surface as classified AppErrors, not bare Er
467468
assert.match(String(error.details?.hint), /reconnect/i);
468469
});
469470

471+
test('androidAdbResultError classifies tolerated nonzero results like thrown executor failures', () => {
472+
const error = androidAdbResultError(
473+
'adb uninstall failed for com.example.app',
474+
{ exitCode: 1, stdout: '', stderr: 'error: device offline' },
475+
{ package: 'com.example.app' },
476+
);
477+
478+
assert.equal(error.code, 'COMMAND_FAILED');
479+
assert.equal(error.details?.stderr, 'error: device offline');
480+
assert.equal(error.details?.exitCode, 1);
481+
assert.equal(error.details?.package, 'com.example.app');
482+
assert.equal(error.details?.adbFailure, 'device_offline');
483+
assert.equal(error.details?.retriable, true);
484+
assert.match(String(error.details?.hint), /adb reconnect/i);
485+
});
486+
487+
test('androidAdbResultError keeps a site hint over the classified one', () => {
488+
const error = androidAdbResultError(
489+
'Failed to pull Android heap dump',
490+
{ exitCode: 1, stdout: '', stderr: 'error: device offline' },
491+
{ hint: 'site-specific hint' },
492+
);
493+
494+
assert.equal(error.details?.hint, 'site-specific hint');
495+
assert.equal(error.details?.adbFailure, 'device_offline');
496+
});
497+
498+
test('semantic provider install failures carry classified hints', async () => {
499+
const error = await withAndroidAdbProvider(
500+
{
501+
exec: async () => ({ stdout: '', stderr: '', exitCode: 0 }),
502+
install: async () => {
503+
throw new AppError('COMMAND_FAILED', 'remote install failed', {
504+
exitCode: 1,
505+
stdout: 'Failure [INSTALL_FAILED_UPDATE_INCOMPATIBLE: signatures do not match]',
506+
stderr: '',
507+
});
508+
},
509+
},
510+
{ serial: 'emulator-5554' },
511+
async () =>
512+
await installAndroidAdbPackage('/app.apk', { replace: true }).then(
513+
() => assert.fail('expected the provider install to reject'),
514+
(err: unknown) => err,
515+
),
516+
);
517+
518+
assert.ok(error instanceof AppError);
519+
assert.equal(error.details?.adbFailure, 'install_update_incompatible');
520+
assert.match(String(error.details?.hint), /incompatible signature/i);
521+
});
522+
523+
test('explicitly passed provider pull failures carry classified hints', async () => {
524+
const error = await pullAndroidAdbFile('/remote.mp4', '/local.mp4', {
525+
provider: {
526+
exec: async () => ({ stdout: '', stderr: '', exitCode: 0 }),
527+
pull: async () => {
528+
throw new AppError('COMMAND_FAILED', 'remote pull failed', {
529+
exitCode: 1,
530+
stdout: '',
531+
stderr: 'error: device offline',
532+
});
533+
},
534+
},
535+
}).then(
536+
() => assert.fail('expected the provider pull to reject'),
537+
(err: unknown) => err,
538+
);
539+
540+
assert.ok(error instanceof AppError);
541+
assert.equal(error.details?.adbFailure, 'device_offline');
542+
assert.equal(error.details?.retriable, true);
543+
assert.match(String(error.details?.hint), /adb reconnect/i);
544+
});
545+
470546
test('provider-scoped adb failures get the same classified hints as local execution', async () => {
471547
const device = {
472548
platform: 'android',

‎src/platforms/android/__tests__/device-input-state.test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import assert from 'node:assert/strict';
22
import { test } from 'vitest';
33
import { getAndroidKeyboardStatusWithAdb } from '../device-input-state.ts';
44
import type { AndroidAdbExecutor } from '../adb-executor.ts';
5+
import { AppError } from '../../../kernel/errors.ts';
56

67
test('getAndroidKeyboardStatusWithAdb exposes active input method package', async () => {
78
const adb: AndroidAdbExecutor = async (args) => {
@@ -27,3 +28,24 @@ test('getAndroidKeyboardStatusWithAdb exposes active input method package', asyn
2728
});
2829
});
2930
});
31+
32+
test('getAndroidKeyboardStatusWithAdb classifies tolerated adb failures with actionable hints', async () => {
33+
// allowFailure regression: the executor returns the nonzero result instead of
34+
// throwing, so the classified hint must come from the result-to-error path.
35+
const adb: AndroidAdbExecutor = async () => ({
36+
stdout: '',
37+
stderr: 'error: device offline',
38+
exitCode: 1,
39+
});
40+
41+
const error = await getAndroidKeyboardStatusWithAdb(adb).then(
42+
() => assert.fail('expected the keyboard query to reject'),
43+
(err: unknown) => err,
44+
);
45+
46+
assert.ok(error instanceof AppError);
47+
assert.equal(error.code, 'COMMAND_FAILED');
48+
assert.equal(error.details?.adbFailure, 'device_offline');
49+
assert.equal(error.details?.retriable, true);
50+
assert.match(String(error.details?.hint), /adb reconnect/i);
51+
});

‎src/platforms/android/adb-executor.ts‎

Lines changed: 62 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -325,22 +325,75 @@ export function attachAdbFailureHint<T>(error: T): T {
325325
return error;
326326
}
327327

328-
function withAdbFailureHintExecutor(exec: AndroidAdbExecutor): AndroidAdbExecutor {
329-
return async (args, options) => {
328+
/**
329+
* Builds the COMMAND_FAILED AppError for a nonzero `allowFailure` adb result and
330+
* runs it through the failure classifier. This is the shared construction point
331+
* for call sites that tolerate a failure to inspect it and then throw — those
332+
* errors never cross the executor throw path, so they classify here instead.
333+
* Site-provided `details` win on key collisions, and a site `hint` is preserved
334+
* over the classified one.
335+
*/
336+
export function androidAdbResultError(
337+
message: string,
338+
result: Pick<AndroidAdbExecutorResult, 'exitCode' | 'stdout' | 'stderr'>,
339+
details?: Record<string, unknown>,
340+
): AppError {
341+
return attachAdbFailureHint(
342+
new AppError('COMMAND_FAILED', message, {
343+
stdout: result.stdout,
344+
stderr: result.stderr,
345+
exitCode: result.exitCode,
346+
...details,
347+
}),
348+
);
349+
}
350+
351+
function withAdbFailureHints<Args extends unknown[], Result>(
352+
call: (...args: Args) => Promise<Result>,
353+
): (...args: Args) => Promise<Result> {
354+
return async (...args) => {
330355
try {
331-
return await exec(args, options);
356+
return await call(...args);
332357
} catch (error) {
333358
throw attachAdbFailureHint(error);
334359
}
335360
};
336361
}
337362

363+
// Providers already enriched by withAdbFailureHintProvider, so repeated
364+
// normalization resolves to the same object instead of stacking wrappers.
365+
const adbFailureHintProviders = new WeakSet<AndroidAdbProvider>();
366+
367+
/**
368+
* Wraps every promise-returning adb funnel a provider exposes — `exec` plus the
369+
* semantic `pull`/`install`/`installBundle` methods that bypass it — so a
370+
* provider failure (e.g. an INSTALL_FAILED verdict from `provider.install`)
371+
* carries the same classified hint as local execution. Applied once inside
372+
* {@link normalizeAndroidAdbProvider}, the single funnel every provider passes
373+
* through; the local provider needs no wrap because its methods delegate to the
374+
* already-enriched serial executor.
375+
*/
376+
function withAdbFailureHintProvider(provider: AndroidAdbProvider): AndroidAdbProvider {
377+
if (adbFailureHintProviders.has(provider)) return provider;
378+
const enriched: AndroidAdbProvider = {
379+
...provider,
380+
exec: withAdbFailureHints(provider.exec),
381+
...(provider.pull ? { pull: withAdbFailureHints(provider.pull) } : {}),
382+
...(provider.install ? { install: withAdbFailureHints(provider.install) } : {}),
383+
...(provider.installBundle
384+
? { installBundle: withAdbFailureHints(provider.installBundle) }
385+
: {}),
386+
};
387+
adbFailureHintProviders.add(enriched);
388+
return enriched;
389+
}
390+
338391
export function createDeviceAdbExecutor(device: DeviceInfo): AndroidAdbExecutor {
339392
return createSerialAdbExecutor(device.id);
340393
}
341394

342395
function createSerialAdbExecutor(serial: string): AndroidAdbExecutor {
343-
return withAdbFailureHintExecutor(
396+
return withAdbFailureHints(
344397
async (args, options) =>
345398
// Local adb execution must escape any active provider scope to avoid routing
346399
// tunnel-backed providers back into themselves when they shell out to adb.
@@ -467,10 +520,7 @@ export function createAndroidPortReverseManager(
467520
function normalizeAndroidAdbProvider(
468521
provider: AndroidAdbProvider | AndroidAdbExecutor,
469522
): AndroidAdbProvider {
470-
if (typeof provider === 'function') {
471-
return { exec: provider };
472-
}
473-
return provider;
523+
return withAdbFailureHintProvider(typeof provider === 'function' ? { exec: provider } : provider);
474524
}
475525

476526
type AndroidAdbTransferProviderOptions = {
@@ -537,13 +587,10 @@ export async function withAndroidAdbProvider<T>(
537587
fn: () => Promise<T>,
538588
): Promise<T> {
539589
if (!provider) return await fn();
540-
const normalized = typeof provider === 'function' ? { exec: provider } : provider;
541-
// Wrap once at scope installation so every consumer — the command-executor
542-
// override and direct resolveAndroidAdb* lookups — gets classified failure hints.
543-
const enriched: AndroidAdbProvider = {
544-
...normalized,
545-
exec: withAdbFailureHintExecutor(normalized.exec),
546-
};
590+
// Normalization wraps once at scope installation, so every consumer — the
591+
// command-executor override and direct resolveAndroidAdb* lookups — gets
592+
// classified failure hints on exec and the semantic provider methods alike.
593+
const enriched = normalizeAndroidAdbProvider(provider);
547594
const scope = { provider: enriched, serial: options.serial };
548595
const override = createAndroidCommandExecutorOverride(scope);
549596
return await androidAdbProviderScope.run(

‎src/platforms/android/app-helpers.ts‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
1-
import { AppError } from '../../kernel/errors.ts';
21
import { resolveAppsFilter, type AppsFilter } from '../../contracts/app-inventory.ts';
3-
import type { AndroidAdbExecutor } from './adb-executor.ts';
2+
import { androidAdbResultError, type AndroidAdbExecutor } from './adb-executor.ts';
43
import {
54
parseAndroidForegroundApp,
65
parseAndroidLaunchablePackages,
@@ -101,11 +100,7 @@ function parseAndroidLaunchablePackageOutput(stdout: string): string[] {
101100
async function listAndroidUserInstalledPackagesWithAdb(adb: AndroidAdbExecutor): Promise<string[]> {
102101
const result = await adb(['shell', 'pm', 'list', 'packages', '-3'], { allowFailure: true });
103102
if (result.exitCode !== 0) {
104-
throw new AppError('COMMAND_FAILED', 'Failed to list Android user-installed apps', {
105-
stdout: result.stdout,
106-
stderr: result.stderr,
107-
exitCode: result.exitCode,
108-
});
103+
throw androidAdbResultError('Failed to list Android user-installed apps', result);
109104
}
110105
return parseAndroidUserInstalledPackages(result.stdout);
111106
}

‎src/platforms/android/app-lifecycle.ts‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { createAppResolutionCache, type AppResolutionCacheScope } from '../app-r
1111
import { waitForAndroidBoot } from './devices.ts';
1212
import { runAndroidAdb } from './adb.ts';
1313
import {
14+
androidAdbResultError,
1415
createAndroidPortReverseManager,
1516
installAndroidAdbPackage,
1617
resolveAndroidAdbProvider,
@@ -479,10 +480,7 @@ async function openAndroidPackage(
479480
if (!(await isAndroidPackageInstalled(device, packageName))) {
480481
throw buildAndroidPackageNotInstalledError(packageName);
481482
}
482-
throw new AppError('COMMAND_FAILED', `Failed to launch ${packageName}`, {
483-
stdout: primaryResult.stdout,
484-
stderr: primaryResult.stderr,
485-
});
483+
throw androidAdbResultError(`Failed to launch ${packageName}`, primaryResult);
486484
}
487485
await runAndroidAdb(device, buildAndroidActivityLaunchArgs(component, launchCategory, options));
488486
}
@@ -726,11 +724,7 @@ async function uninstallAndroidApp(device: DeviceInfo, app: string): Promise<{ p
726724
if (result.exitCode !== 0) {
727725
const output = `${result.stdout}\n${result.stderr}`.toLowerCase();
728726
if (!output.includes('unknown package') && !output.includes('not installed')) {
729-
throw new AppError('COMMAND_FAILED', `adb uninstall failed for ${resolved.value}`, {
730-
stdout: result.stdout,
731-
stderr: result.stderr,
732-
exitCode: result.exitCode,
733-
});
727+
throw androidAdbResultError(`adb uninstall failed for ${resolved.value}`, result);
734728
}
735729
}
736730
return { package: resolved.value };

‎src/platforms/android/device-input-state.ts‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@ import { emitDiagnostic } from '../../utils/diagnostics.ts';
22
import type { DeviceInfo } from '../../kernel/device.ts';
33
import { AppError } from '../../kernel/errors.ts';
44
import { isClipboardShellUnsupported, sleep } from './adb.ts';
5-
import { resolveAndroidAdbExecutor, type AndroidAdbExecutor } from './adb-executor.ts';
5+
import {
6+
androidAdbResultError,
7+
resolveAndroidAdbExecutor,
8+
type AndroidAdbExecutor,
9+
} from './adb-executor.ts';
610
import {
711
classifyAndroidInputOwner,
812
isFallbackAndroidInputMethodPackage,
@@ -84,11 +88,7 @@ export async function getAndroidKeyboardStatusWithAdb(
8488
allowFailure: true,
8589
});
8690
if (result.exitCode !== 0) {
87-
throw new AppError('COMMAND_FAILED', 'Failed to query Android keyboard state', {
88-
stdout: result.stdout,
89-
stderr: result.stderr,
90-
exitCode: result.exitCode,
91-
});
91+
throw androidAdbResultError('Failed to query Android keyboard state', result);
9292
}
9393
return parseAndroidKeyboardState(result.stdout);
9494
}
@@ -326,11 +326,7 @@ async function runAndroidClipboardShellCommand(
326326
);
327327
}
328328
if (result.exitCode !== 0) {
329-
throw new AppError('COMMAND_FAILED', `Failed to ${operation} Android clipboard text`, {
330-
stdout: result.stdout,
331-
stderr: result.stderr,
332-
exitCode: result.exitCode,
333-
});
329+
throw androidAdbResultError(`Failed to ${operation} Android clipboard text`, result);
334330
}
335331
return result.stdout;
336332
}

‎src/platforms/android/logcat.ts‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
import fs from 'node:fs';
22
import { AppError } from '../../kernel/errors.ts';
3-
import type { AndroidAdbExecutor, AndroidAdbProcess, AndroidAdbProvider } from './adb-executor.ts';
3+
import {
4+
androidAdbResultError,
5+
type AndroidAdbExecutor,
6+
type AndroidAdbProcess,
7+
type AndroidAdbProvider,
8+
} from './adb-executor.ts';
49

510
export type AndroidLogcatCaptureOptions = {
611
lines?: number;
@@ -28,11 +33,7 @@ export async function captureAndroidLogcatWithAdb(
2833
signal: options.signal,
2934
});
3035
if (result.exitCode !== 0) {
31-
throw new AppError('COMMAND_FAILED', 'Failed to capture Android logcat', {
32-
stdout: result.stdout,
33-
stderr: result.stderr,
34-
exitCode: result.exitCode,
35-
});
36+
throw androidAdbResultError('Failed to capture Android logcat', result);
3637
}
3738
return result.stdout;
3839
}

‎src/platforms/android/multitouch-helper.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { findProjectRoot, readVersion } from '../../utils/version.ts';
77
import type { DeviceInfo } from '../../kernel/device.ts';
88
import type { TransformGestureParams } from '../../core/scroll-gesture.ts';
99
import {
10+
androidAdbResultError,
1011
installAndroidAdbPackage,
1112
resolveAndroidAdbExecutor,
1213
resolveAndroidAdbProvider,
@@ -528,12 +529,9 @@ export async function ensureAndroidMultiTouchHelper(options: {
528529
timeoutMs: ANDROID_MULTITOUCH_HELPER_INSTALL_TIMEOUT_MS,
529530
});
530531
if (result.exitCode !== 0) {
531-
throw new AppError('COMMAND_FAILED', 'Failed to install Android multi-touch helper', {
532+
throw androidAdbResultError('Failed to install Android multi-touch helper', result, {
532533
packageName,
533534
versionCode,
534-
stdout: result.stdout,
535-
stderr: result.stderr,
536-
exitCode: result.exitCode,
537535
});
538536
}
539537
installedMultiTouchHelpers.add(cacheKey);

0 commit comments

Comments
 (0)