Skip to content

Commit 991e723

Browse files
authored
fix(android): actionable error when the snapshot helper is unavailable (#1284) (#1285)
* fix(android): actionable error when the snapshot helper is unavailable (#1284) #1284 kept the hard-fail from #1217 (a silent stock-UIAutomator fallback produced a materially different, app-window-only capture) but asked for actionable hints on both failure modes: - Artifact missing on disk: hint now names the exact `pnpm build:android` command, the full dist file set it produces, and notes packaged installs ship it via prepack. - Artifact present but the device rejects the install (adb/OEM policy): the underlying adb error already surfaces in the message; the hint now explicitly frames this as a device-side failure distinct from a missing build artifact, tagged via a new androidSnapshotHelperInstallFailure detail set at the adb install call site. * fix(android): correct helper-missing hint to the two runtime-required files Review correction on #1285: resolveAndroidSnapshotHelperArtifact only fs.access'es the versioned .manifest.json and the .apk it references — the sha256 is a manifest field, and *.idsig is excluded from the npm package by design. The hint no longer claims the full sidecar set is required, and the test now rejects any future idsig claim. * fix(android): cover install rejections and preserve diagnostic identity (#1285 review) P1: the device-side install marker now covers the whole install phase. ensureAndroidSnapshotHelper previously tagged only a resolved nonzero install result; an AndroidAdbProvider.install rejection (enriched INSTALL_FAILED_* AppError from the provider funnel) bypassed the marker and fell back to the generic retry/doctor hint. Both paths now flow through markAndroidSnapshotHelperInstallFailure, which mutates details in place so the original code, message, hint, details, and cause all survive. Regression covers the public daemon snapshot route with a real request handler and an injected provider whose install rejects. P2: androidSnapshotHelperCaptureError rewrapped through normalizeError, which lifts diagnosticId/logPath out of details — the rewrap dropped them (ADR 0010 violation). They are now reinstated into the rewrapped error's details. Hint selection moved to a helper to keep the function under the complexity gate. * fix(android): restore all lifted wire fields through the capture rewrap (#1285 review) normalizeError hoists hint, diagnosticId, logPath, retriable, and supportedOn out of details (ADR 0010); the capture rewrap restored only diagnosticId/logPath, so a transient-classified install rejection (e.g. connection_dropped) lost its structured retriable signal on the public daemon error. liftedDiagnosticIdentity is generalized to liftedWireFields covering the complete hoisted set (hint stays owned by the capture hint selector). Public route regression: a retriable provider install rejection keeps error.retriable === true on the daemon snapshot response.
1 parent bd62502 commit 991e723

5 files changed

Lines changed: 353 additions & 24 deletions

File tree

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
import { beforeEach, expect, test } from 'vitest';
2+
import { createRequestHandler } from '../request-router.ts';
3+
import { LeaseRegistry } from '../lease-registry.ts';
4+
import { SessionStore } from '../session-store.ts';
5+
import { AppError } from '../../kernel/errors.ts';
6+
import { resetAndroidSnapshotHelperInstallCache } from '../../platforms/android/snapshot-helper-install.ts';
7+
import { ANDROID_SNAPSHOT_HELPER_FIXTURE_ARTIFACT } from '../../__tests__/test-utils/index.ts';
8+
import type { AndroidAdbProvider } from '../../platforms/android/adb-executor.ts';
9+
10+
function makeAndroidSessionStore(name: string): SessionStore {
11+
const sessionStore = new SessionStore(`/tmp/${name}`);
12+
sessionStore.set('default', {
13+
name: 'default',
14+
createdAt: Date.now(),
15+
device: {
16+
platform: 'android',
17+
id: 'remote-android-1',
18+
name: 'Remote Android',
19+
kind: 'device',
20+
booted: true,
21+
},
22+
appBundleId: 'com.example.app',
23+
actions: [],
24+
});
25+
return sessionStore;
26+
}
27+
28+
function makeHandler(sessionStore: SessionStore, androidAdbProvider: () => AndroidAdbProvider) {
29+
return createRequestHandler({
30+
logPath: '/tmp/daemon.log',
31+
token: 'token',
32+
sessionStore,
33+
leaseRegistry: new LeaseRegistry(),
34+
androidAdbProvider,
35+
trackDownloadableArtifact: () => 'artifact-id',
36+
});
37+
}
38+
39+
beforeEach(() => {
40+
resetAndroidSnapshotHelperInstallCache();
41+
});
42+
43+
// Regression for #1284/#1285 P1: a provider whose `install` REJECTS with an
44+
// enriched INSTALL_FAILED_* error (instead of resolving with a nonzero result)
45+
// must still surface the device-side install classification on the public
46+
// daemon snapshot error, not the generic retry/doctor hint.
47+
test('snapshot reports a device-side install failure when the provider install rejects', async () => {
48+
const sessionStore = makeAndroidSessionStore(
49+
'agent-device-request-router-snapshot-helper-install-reject-test',
50+
);
51+
const provider: AndroidAdbProvider = {
52+
// Version probe reports no installed helper; every other device query is inert.
53+
exec: async () => ({ exitCode: 0, stdout: '', stderr: '' }),
54+
install: async () => {
55+
throw new AppError('COMMAND_FAILED', 'Failed to install Android snapshot helper', {
56+
stderr: 'adb: failed to install helper.apk: Failure [INSTALL_FAILED_TEST_ONLY]',
57+
stdout: '',
58+
exitCode: 1,
59+
processExitError: true,
60+
});
61+
},
62+
snapshotHelperArtifact: ANDROID_SNAPSHOT_HELPER_FIXTURE_ARTIFACT,
63+
};
64+
const handler = makeHandler(sessionStore, () => provider);
65+
66+
const response = await handler({
67+
token: 'token',
68+
session: 'default',
69+
command: 'snapshot',
70+
positionals: [],
71+
flags: {},
72+
});
73+
74+
expect(response.ok).toBe(false);
75+
if (response.ok) throw new Error('Expected snapshot to fail');
76+
expect(response.error.message).toMatch(/Android snapshot helper failed/);
77+
expect(response.error.message).toMatch(/INSTALL_FAILED_TEST_ONLY/);
78+
// The provider funnel's classified adb hint is preserved, with the
79+
// device-side framing appended rather than replacing it.
80+
expect(response.error.hint).toMatch(/package installer rejected the APK/);
81+
expect(response.error.hint).toMatch(/device-side install failure/);
82+
expect(response.error.hint).not.toMatch(/pnpm build:android/);
83+
expect(response.error.details?.androidSnapshotHelperInstallFailure).toBe(true);
84+
});
85+
86+
// ADR 0010 wire contract: a transient-classified install rejection (here the
87+
// funnel's `connection_dropped` family) must keep its structured `retriable`
88+
// signal through the capture rewrap onto the public daemon error.
89+
test('snapshot keeps the transient retry signal when the provider install rejection is retriable', async () => {
90+
const sessionStore = makeAndroidSessionStore(
91+
'agent-device-request-router-snapshot-helper-install-transient-test',
92+
);
93+
const provider: AndroidAdbProvider = {
94+
exec: async () => ({ exitCode: 0, stdout: '', stderr: '' }),
95+
install: async () => {
96+
throw new AppError('COMMAND_FAILED', 'Failed to install Android snapshot helper', {
97+
stderr: 'adb: transport error while pushing helper.apk',
98+
stdout: '',
99+
exitCode: 1,
100+
processExitError: true,
101+
});
102+
},
103+
snapshotHelperArtifact: ANDROID_SNAPSHOT_HELPER_FIXTURE_ARTIFACT,
104+
};
105+
const handler = makeHandler(sessionStore, () => provider);
106+
107+
const response = await handler({
108+
token: 'token',
109+
session: 'default',
110+
command: 'snapshot',
111+
positionals: [],
112+
flags: {},
113+
});
114+
115+
expect(response.ok).toBe(false);
116+
if (response.ok) throw new Error('Expected snapshot to fail');
117+
expect(response.error.message).toMatch(/Android snapshot helper failed/);
118+
expect(response.error.hint).toMatch(/connection dropped/);
119+
expect(response.error.hint).toMatch(/device-side install failure/);
120+
expect(response.error.retriable).toBe(true);
121+
expect(response.error.details?.androidSnapshotHelperInstallFailure).toBe(true);
122+
});

src/platforms/android/__tests__/snapshot-helper.test.ts

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
forgetAndroidSnapshotHelperInstall,
1414
resetAndroidSnapshotHelperInstallCache,
1515
} from '../snapshot-helper-install.ts';
16+
import { AppError } from '../../../kernel/errors.ts';
1617
import { parseAndroidSnapshotHelperManifest } from '../snapshot-helper-artifact.ts';
1718
import { verifyAndroidHelperApkChecksum } from '../helper-package-install.ts';
1819
import type {
@@ -204,6 +205,80 @@ test('ensureAndroidSnapshotHelper installs when missing and skips a newer versio
204205
assert.equal(skipped.reason, 'current');
205206
});
206207

208+
test('ensureAndroidSnapshotHelper tags a device-side install rejection distinctly from artifact resolution', async () => {
209+
const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'snapshot-helper-install-reject-'));
210+
const apkPath = path.join(tmpDir, 'helper.apk');
211+
await fs.writeFile(apkPath, 'helper-apk');
212+
const adb: AndroidAdbExecutor = async (args) => {
213+
if (args.includes('--show-versioncode')) {
214+
return { exitCode: 1, stdout: '', stderr: 'not found' };
215+
}
216+
if (args[0] === 'install') {
217+
return { exitCode: 1, stdout: '', stderr: 'Failure [INSTALL_FAILED_TEST_ONLY]' };
218+
}
219+
throw new Error(`unexpected adb call: ${args.join(' ')}`);
220+
};
221+
222+
await assert.rejects(
223+
() =>
224+
ensureAndroidSnapshotHelper({
225+
adb,
226+
artifact: { apkPath, manifest: { ...manifest, sha256: sha256Text('helper-apk') } },
227+
}),
228+
(error) => {
229+
assert.match((error as Error).message, /Failed to install Android snapshot helper/);
230+
assert.equal(
231+
(error as { details?: Record<string, unknown> }).details
232+
?.androidSnapshotHelperInstallFailure,
233+
true,
234+
);
235+
return true;
236+
},
237+
);
238+
});
239+
240+
test('ensureAndroidSnapshotHelper tags a rejected provider install without losing the enriched error', async () => {
241+
const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'snapshot-helper-install-throw-'));
242+
const apkPath = path.join(tmpDir, 'helper.apk');
243+
await fs.writeFile(apkPath, 'helper-apk');
244+
const installError = new AppError('COMMAND_FAILED', 'Failed to install Android snapshot helper', {
245+
stderr: 'adb: failed to install helper.apk: Failure [INSTALL_FAILED_TEST_ONLY]',
246+
stdout: '',
247+
exitCode: 1,
248+
processExitError: true,
249+
hint: 'The Android package installer rejected the APK — see the INSTALL_FAILED code in the error output for the exact cause.',
250+
});
251+
const adb: AndroidAdbExecutor = async (args) => {
252+
if (args.includes('--show-versioncode')) {
253+
return { exitCode: 1, stdout: '', stderr: 'not found' };
254+
}
255+
throw new Error(`unexpected adb call: ${args.join(' ')}`);
256+
};
257+
const adbProvider: AndroidAdbProvider = {
258+
exec: adb,
259+
install: async () => {
260+
throw installError;
261+
},
262+
};
263+
264+
await assert.rejects(
265+
() =>
266+
ensureAndroidSnapshotHelper({
267+
adb,
268+
adbProvider,
269+
artifact: { apkPath, manifest: { ...manifest, sha256: sha256Text('helper-apk') } },
270+
}),
271+
(error) => {
272+
assert.equal(error, installError);
273+
const details = (error as AppError).details;
274+
assert.equal(details?.androidSnapshotHelperInstallFailure, true);
275+
assert.match(String(details?.stderr), /INSTALL_FAILED_TEST_ONLY/);
276+
assert.match(String(details?.hint), /package installer rejected the APK/);
277+
return true;
278+
},
279+
);
280+
});
281+
207282
test('ensureAndroidSnapshotHelper replaces same-version helper when APK bytes differ', async () => {
208283
const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'snapshot-helper-identity-'));
209284
const apkPath = path.join(tmpDir, 'helper.apk');

src/platforms/android/__tests__/snapshot.test.ts

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -892,6 +892,79 @@ test('snapshotAndroid emits unavailable diagnostics when helper artifact is miss
892892
}
893893
});
894894

895+
test('snapshotAndroid gives an actionable hint when the helper artifact is missing on disk', async () => {
896+
const accessSpy = vi.spyOn(fs, 'access').mockRejectedValueOnce(new Error('helper missing'));
897+
898+
try {
899+
await assert.rejects(
900+
() => snapshotAndroid(device),
901+
(error) => {
902+
assert.match((error as Error).message, /the bundled helper artifact was not found/);
903+
const hint = String((error as { details?: Record<string, unknown> }).details?.hint);
904+
assert.match(hint, /pnpm build:android/);
905+
assert.match(hint, /prepack/);
906+
assert.match(hint, /\.manifest\.json/);
907+
assert.match(hint, /\.apk/);
908+
// The npm package excludes *.idsig by design — the hint must never claim it is required.
909+
assert.doesNotMatch(hint, /idsig/);
910+
return true;
911+
},
912+
);
913+
} finally {
914+
accessSpy.mockRestore();
915+
}
916+
});
917+
918+
test('snapshotAndroid distinguishes a device-side install rejection from a missing build artifact', async () => {
919+
const helperAdb: AndroidAdbExecutor = async (args) => {
920+
if (args.includes('--show-versioncode')) {
921+
// No installed package reported, so ensureAndroidSnapshotHelper treats the
922+
// helper as missing and attempts a real install.
923+
return { exitCode: 0, stdout: '', stderr: '' };
924+
}
925+
if (args[0] === 'install') {
926+
return { exitCode: 1, stdout: '', stderr: 'Failure [INSTALL_FAILED_TEST_ONLY]' };
927+
}
928+
throw new Error(`unexpected adb args: ${args.join(' ')}`);
929+
};
930+
931+
await assert.rejects(
932+
() => snapshotAndroidWithHelper(helperAdb),
933+
(error) => {
934+
const message = (error as Error).message;
935+
assert.match(message, /Android snapshot helper failed/);
936+
assert.match(message, /Failed to install Android snapshot helper/);
937+
assert.match(message, /INSTALL_FAILED_TEST_ONLY/);
938+
const hint = String((error as { details?: Record<string, unknown> }).details?.hint);
939+
assert.match(hint, /device-side install failure/);
940+
assert.doesNotMatch(hint, /pnpm build:android/);
941+
return true;
942+
},
943+
);
944+
});
945+
946+
test('snapshotAndroid preserves upstream diagnosticId and logPath through the capture rewrap', async () => {
947+
const helperAdb = createHelperAdb({
948+
instrument: async () => {
949+
throw new AppError('COMMAND_FAILED', 'helper capture exploded', {
950+
diagnosticId: 'diag-upstream',
951+
logPath: '/tmp/upstream.ndjson',
952+
});
953+
},
954+
});
955+
956+
await assert.rejects(
957+
() => snapshotAndroidWithHelper(helperAdb),
958+
(error) => {
959+
assert.match((error as Error).message, /Android snapshot helper failed/);
960+
const details = (error as { details?: Record<string, unknown> }).details;
961+
assert.equal(details?.diagnosticId, 'diag-upstream');
962+
assert.equal(details?.logPath, '/tmp/upstream.ndjson');
963+
return true;
964+
},
965+
);
966+
});
967+
895968
test('snapshotAndroid emits timeout diagnostics when helper capture times out', async () => {
896969
const helperAdb = createHelperAdb({
897970
instrument: async () => {

src/platforms/android/snapshot-helper-install.ts

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { asAppError, type AppError } from '../../kernel/errors.ts';
12
import { readAndroidSnapshotHelperInstallOptions } from './snapshot-helper-artifact.ts';
23
import {
34
inspectInstalledAndroidHelper,
@@ -136,22 +137,30 @@ export async function ensureAndroidSnapshotHelper(options: {
136137
};
137138
}
138139

139-
const result = await installAndroidSnapshotHelper(
140-
adb,
141-
options.adbProvider ?? adb,
142-
artifact.apkPath,
143-
readAndroidSnapshotHelperInstallOptions(artifact.manifest),
144-
{
145-
packageName,
146-
timeoutMs: options.timeoutMs,
147-
},
148-
);
140+
let result: Awaited<ReturnType<AndroidAdbExecutor>>;
141+
try {
142+
result = await installAndroidSnapshotHelper(
143+
adb,
144+
options.adbProvider ?? adb,
145+
artifact.apkPath,
146+
readAndroidSnapshotHelperInstallOptions(artifact.manifest),
147+
{
148+
packageName,
149+
timeoutMs: options.timeoutMs,
150+
},
151+
);
152+
} catch (error) {
153+
forgetInstalledSnapshotHelper(installCacheKey);
154+
throw markAndroidSnapshotHelperInstallFailure(error, { packageName, versionCode });
155+
}
149156
if (result.exitCode !== 0) {
150157
forgetInstalledSnapshotHelper(installCacheKey);
151-
throw androidAdbResultError('Failed to install Android snapshot helper', result, {
152-
packageName,
153-
versionCode,
154-
});
158+
throw markAndroidSnapshotHelperInstallFailure(
159+
androidAdbResultError('Failed to install Android snapshot helper', result, {
160+
packageName,
161+
versionCode,
162+
}),
163+
);
155164
}
156165

157166
rememberInstalledSnapshotHelper(installCacheKey, versionCode);
@@ -165,6 +174,23 @@ export async function ensureAndroidSnapshotHelper(options: {
165174
};
166175
}
167176

177+
// Tags every failure of the helper install phase — nonzero results and provider
178+
// rejections alike — so the capture layer can distinguish a device-side install
179+
// rejection (adb/OEM policy) from a missing build artifact. Mutates details in
180+
// place, preserving the original code, message, hint, and cause.
181+
function markAndroidSnapshotHelperInstallFailure(
182+
error: unknown,
183+
context?: { packageName: string; versionCode: number },
184+
): AppError {
185+
const appError = asAppError(error, 'COMMAND_FAILED');
186+
appError.details = {
187+
...context,
188+
...appError.details,
189+
androidSnapshotHelperInstallFailure: true,
190+
};
191+
return appError;
192+
}
193+
168194
function normalizeAdbProvider(
169195
provider: AndroidAdbProvider | AndroidAdbExecutor | undefined,
170196
adb: AndroidAdbExecutor,

0 commit comments

Comments
 (0)