Skip to content

Commit 6cfa4e9

Browse files
committed
fix(android): let only a clean adb exit prove clipboard support
Third review P1 on #2021. The typed verdict landed one layer too high. The adapter probe runs `adb shell cmd clipboard get text` with `allowFailure`, so a non-zero exit comes back as an ordinary result rather than a throw — and the only thing standing between that result and `supported` was the missing-shell prose check. A device that had gone offline, was unauthorized, timed out, or failed for any other reason produced none of that prose, so it fell through to `supported` and was then cached by device id for the runtime owner's lifetime. The `catch` I added guarded the one path adb almost never takes. Each adb outcome now proves only what it can: - `exitCode === 0` is the sole evidence of support, because it is the only result that shows the command ran. - The recognized missing-shell prose is the sole evidence of absence, and is read before the exit code — adb reports that condition non-zero, so checking the code first would turn every honest `unsupported` into a refusal. - Everything else — non-zero without that prose, and the transport throw — is `probe-failed`, which admission refuses and the cache does not remember. The package tests mocked the typed verdict, so they sat downstream of the bug and could not see it. The regression is therefore at the adapter, over the raw adb result: four planted reds (offline, unauthorized, device-not-found, generic failure) that all returned `supported` before this change, plus the two definitive verdicts and the ordering case that keeps `unsupported` reachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RpfS12XApXqasuAWZJaEX
1 parent df5e5bd commit 6cfa4e9

2 files changed

Lines changed: 83 additions & 6 deletions

File tree

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
import { describe, expect, test, vi } from 'vitest';
2+
import type { DeviceInfo } from '@agent-device/kernel/device';
3+
import { createAndroidToolHost } from '../platform-runtime-android-tool-host.ts';
4+
5+
const runAndroidAdb = vi.hoisted(() => vi.fn());
6+
7+
vi.mock('../platforms/android/adb.ts', async (importOriginal) => ({
8+
...(await importOriginal<typeof import('../platforms/android/adb.ts')>()),
9+
runAndroidAdb,
10+
}));
11+
12+
const device: DeviceInfo = {
13+
platform: 'android',
14+
id: 'emulator-5554',
15+
name: 'Pixel 9 Pro XL',
16+
kind: 'emulator',
17+
booted: true,
18+
};
19+
20+
function adbResult(exitCode: number, stdout = '', stderr = '') {
21+
return { exitCode, stdout, stderr, stdoutBuffer: Buffer.from(stdout) };
22+
}
23+
24+
async function probe() {
25+
const host = createAndroidToolHost();
26+
return await host.probeClipboardShellSupport?.(device);
27+
}
28+
29+
// The probe runs with `allowFailure`, so every adb outcome short of a transport throw arrives as an
30+
// ordinary result. Anything the adapter reports as `supported` is cached for the runtime owner's
31+
// lifetime, so a wrong admission here is not a single bad answer -- it advertises a clipboard the
32+
// build may not have until the daemon restarts.
33+
describe('android clipboard shell probe: what each adb result is allowed to prove', () => {
34+
test('a clean exit is the only thing that proves support', async () => {
35+
runAndroidAdb.mockResolvedValueOnce(adbResult(0, 'clipboard contents'));
36+
await expect(probe()).resolves.toBe('supported');
37+
});
38+
39+
test('an empty clipboard on a clean exit still proves support', async () => {
40+
runAndroidAdb.mockResolvedValueOnce(adbResult(0, ''));
41+
await expect(probe()).resolves.toBe('supported');
42+
});
43+
44+
test('the missing-shell prose proves the build ships no clipboard command', async () => {
45+
runAndroidAdb.mockResolvedValueOnce(
46+
adbResult(255, '', 'Error: no shell command implementation.'),
47+
);
48+
await expect(probe()).resolves.toBe('unsupported');
49+
});
50+
51+
// The planted red: before the fix every non-zero result that lacked the missing-shell prose fell
52+
// through to `supported`, so an offline device advertised a working clipboard.
53+
test.each([
54+
['a device that dropped off the bridge', 'error: device offline'],
55+
['an unauthorized device', 'error: device unauthorized.'],
56+
['a bridge that never found the device', "error: device '(null)' not found"],
57+
['a generic adb failure', 'error: closed'],
58+
])('%s refuses rather than admitting support', async (_case, stderr) => {
59+
runAndroidAdb.mockResolvedValueOnce(adbResult(1, '', stderr));
60+
await expect(probe()).resolves.toBe('probe-failed');
61+
});
62+
63+
test('a transport throw refuses rather than admitting support', async () => {
64+
runAndroidAdb.mockRejectedValueOnce(new Error('spawn adb ENOENT'));
65+
await expect(probe()).resolves.toBe('probe-failed');
66+
});
67+
68+
// Recognized absence outranks the exit code: adb reports the missing shell command non-zero, so
69+
// reading the code first would turn every honest `unsupported` into a refusal.
70+
test('missing-shell prose on a non-zero exit still reads as unsupported', async () => {
71+
runAndroidAdb.mockResolvedValueOnce(adbResult(1, '', 'Unknown command: clipboard'));
72+
await expect(probe()).resolves.toBe('unsupported');
73+
});
74+
});

src/platform-runtime-android-tool-host.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,13 @@ import type { AndroidToolHost } from '@agent-device/contracts/platform';
44
export function createAndroidToolHost(): AndroidToolHost {
55
return Object.freeze({
66
/**
7-
* Definitive in both directions where adb answers, and honest when it does not: a transport
8-
* failure reports `probe-failed` rather than a guess, so admission can refuse instead of
9-
* fabricating availability the operation would then reject.
7+
* Definitive in both directions only where adb actually answers, and honest everywhere else.
8+
* The probe runs with `allowFailure`, so a device that is offline, unauthorized, timed out or
9+
* otherwise broken comes back as an ordinary non-zero result rather than a throw: only a clean
10+
* exit proves the clipboard shell command exists, and only the recognized missing-shell prose
11+
* proves it does not. Every other result -- non-zero without that prose, or a transport throw
12+
* -- is `probe-failed`, so admission refuses instead of caching availability the operation
13+
* would then reject.
1014
*/
1115
probeClipboardShellSupport: async (device, signal) => {
1216
try {
@@ -16,9 +20,8 @@ export function createAndroidToolHost(): AndroidToolHost {
1620
allowFailure: true,
1721
signal,
1822
});
19-
return isClipboardShellUnsupported(result.stdout, result.stderr)
20-
? 'unsupported'
21-
: 'supported';
23+
if (isClipboardShellUnsupported(result.stdout, result.stderr)) return 'unsupported';
24+
return result.exitCode === 0 ? 'supported' : 'probe-failed';
2225
} catch {
2326
return 'probe-failed';
2427
}

0 commit comments

Comments
 (0)