From 81b94cebec40624287c125fee4dd70bcb9f08c59 Mon Sep 17 00:00:00 2001 From: Syed Musa Raza Naqvi <94714949+musaraza004@users.noreply.github.com> Date: Fri, 23 Oct 2026 02:56:19 +0500 Subject: [PATCH] fix(scripts): remove the ADB verifier's pushed probe file from the device (#5256) * fix(scripts): remove the ADB verifier's pushed probe file from the device The verifier pushed `/data/local/tmp/rea-verify-.txt` for its transfer roundtrip but never deleted it, so every successful run left a probe behind while reporting that all checks passed. After a successful no-overwrite push, the verifier now removes that exact destination with the provider-reported adb client and confirms its absence through `list_adb_directory`. Removal runs even when the pull fails, and a removal failure is reported as a failed check instead of a pass. `pull_adb_file` still retains device originals. Fixes #5190 Co-Authored-By: Claude Opus 5.5 * test(scripts): cover ADB probe cleanup after pull failure and leftover probes Add verifier cases where the pull returns an error or throws after a successful push, and where `adb shell rm` exits 0 but the listing still shows the probe. Pull failures must still remove the probe, and a leftover probe must fail the run. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- scripts/verify-adb.mjs | 107 ++++++++++--- .../process/adbVerifierCleanup.test.ts | 145 ++++++++++++++++-- 2 files changed, 216 insertions(+), 36 deletions(-) diff --git a/scripts/verify-adb.mjs b/scripts/verify-adb.mjs index 916a9383ac..1c214e3479 100644 --- a/scripts/verify-adb.mjs +++ b/scripts/verify-adb.mjs @@ -1,12 +1,15 @@ +import { execFile } from "node:child_process"; import { createHash } from "node:crypto"; import { mkdtemp, readFile, rm, stat } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { join, posix } from "node:path"; +import { promisify } from "node:util"; import { AdbDeviceAnalysisService } from "../dist/application/adb/AdbDeviceAnalysisService.js"; import { createAdbDeviceAnalysisProvider } from "../dist/composition/adb.js"; import { closeVerifierResources } from "./lib/verifier-cleanup.mjs"; +const execFileAsync = promisify(execFile); const options = parseArguments(process.argv.slice(2)); if (options.help) { process.stdout.write( @@ -40,6 +43,80 @@ const run = async (operation, input) => { return result.value.normalized_result; }; +// Remove the verifier's own transfer probe with the provider-reported adb +// client, then confirm through the directory listing that it is gone. +const removeDeviceProbe = async (binary, serial, devicePath) => { + const name = "verifier transfer probe removed"; + if (typeof binary !== "string" || binary === "") { + check(name, false, `${devicePath}: adb client path unavailable`); + return; + } + try { + await execFileAsync( + binary, + ["-s", serial, "shell", "rm", "--", devicePath], + { + timeout: 30_000, + }, + ); + } catch (cause) { + check(name, false, `${devicePath}: ${formatFailure(cause)}`); + return; + } + const listing = await run("list_adb_directory", { + serial, + device_path: posix.dirname(devicePath), + }); + if (listing !== null) + check( + name, + listing.coverage === "complete" && + !listing.entries.some( + (entry) => entry.name === posix.basename(devicePath), + ), + devicePath, + ); +}; + +const verifyTransferRoundtrip = async ({ + adbBinary, + serial, + payload, + devicePath, + outputDirectory, +}) => { + const pushed = await run("push_adb_file", { + serial, + local_path: payload, + device_path: devicePath, + overwrite: false, + }); + if (pushed === null) return; + // A successful no-overwrite push proves the verifier created this exact + // destination; pull_adb_file deliberately retains device originals. + try { + check( + "push reports matching device-side digest", + pushed.device_digest_source === "device_sha256sum" && + pushed.device_sha256 === pushed.sha256, + `${pushed.bytes} bytes`, + ); + const pulled = await run("pull_adb_file", { + serial, + device_path: devicePath, + output_directory: outputDirectory, + }); + if (pulled !== null) + check( + "pulled file digest matches the pushed digest", + pulled.sha256 === pushed.sha256, + `${pulled.bytes} bytes`, + ); + } finally { + await removeDeviceProbe(adbBinary, serial, devicePath); + } +}; + let verifierFailed = false; let verifierFailure; try { @@ -245,31 +322,13 @@ try { fs.writeFile(payload, payloadBytes), ); const deviceTarget = `/data/local/tmp/rea-verify-${Date.now()}.txt`; - const pushed = await run("push_adb_file", { + await verifyTransferRoundtrip({ + adbBinary: client?.client.binary_path, serial, - local_path: payload, - device_path: deviceTarget, - overwrite: false, + payload, + devicePath: deviceTarget, + outputDirectory: transferRoot, }); - if (pushed !== null) { - check( - "push reports matching device-side digest", - pushed.device_digest_source === "device_sha256sum" && - pushed.device_sha256 === pushed.sha256, - `${pushed.bytes} bytes`, - ); - const pulled = await run("pull_adb_file", { - serial, - device_path: deviceTarget, - output_directory: transferRoot, - }); - if (pulled !== null) - check( - "pulled file digest matches the pushed digest", - pulled.sha256 === pushed.sha256, - `${pulled.bytes} bytes`, - ); - } const screenRoot = await mkdtemp(join(tmpdir(), "rea-adb-screen-")); try { const screen = await run("capture_adb_screen", { diff --git a/tests/boundary/process/adbVerifierCleanup.test.ts b/tests/boundary/process/adbVerifierCleanup.test.ts index 42e53b85b5..0a8db02b3b 100644 --- a/tests/boundary/process/adbVerifierCleanup.test.ts +++ b/tests/boundary/process/adbVerifierCleanup.test.ts @@ -1,5 +1,13 @@ import { spawnSync } from "node:child_process"; -import { mkdtemp, mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { + chmod, + mkdtemp, + mkdir, + readFile, + readdir, + rm, + writeFile, +} from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, expect, it } from "vitest"; @@ -12,7 +20,12 @@ afterEach(async () => { }); const runVerifier = async ( - settings: { closeFails?: boolean; thrown?: "undefined" | "null" } = {}, + settings: { + closeFails?: boolean; + thrown?: "undefined" | "null"; + removal?: "fails" | "leaves_probe"; + pull?: "error" | "throw"; + } = {}, ) => { const root = await mkdtemp(join(tmpdir(), "rea-adb-verifier-test-")); roots.push(root); @@ -29,6 +42,24 @@ const runVerifier = async ( new URL("../../../scripts/lib/verifier-cleanup.mjs", import.meta.url), ), ); + // The simulated device directory stands in for /data/local/tmp; the fake + // adb client records its argv and removes only the named probe. + const device = join(root, "device"); + await mkdir(device); + const adb = join(root, "adb"); + await writeFile( + adb, + [ + "#!/bin/sh", + `printf '%s\\n' "$*" >> "$REA_FAKE_ADB_LOG"`, + `[ "$REA_FAKE_ADB_REMOVAL" = "fails" ] && exit 1`, + `[ "$REA_FAKE_ADB_REMOVAL" = "leaves_probe" ] && exit 0`, + `[ "$1 $3 $4 $5" = "-s shell rm --" ] || exit 2`, + `rm -- "$REA_FAKE_DEVICE/$(basename -- "$6")"`, + "", + ].join("\n"), + ); + await chmod(adb, 0o755); await writeFile( join(root, "dist", "composition", "adb.js"), `import { writeFile } from "node:fs/promises"; @@ -46,12 +77,21 @@ export const createAdbDeviceAnalysisProvider = () => ({ ); await writeFile( join(root, "dist", "application", "adb", "AdbDeviceAnalysisService.js"), - `export class AdbDeviceAnalysisService { + `import { readdir, writeFile } from "node:fs/promises"; +import { basename, join } from "node:path"; +export class AdbDeviceAnalysisService { async execute(operation, input) { if (process.env.REA_ADB_THROW === "undefined") throw undefined; if (process.env.REA_ADB_THROW === "null") throw null; + const device = process.env.REA_FAKE_DEVICE; + if (operation === "push_adb_file") + await writeFile(join(device, basename(input.device_path)), "probe"); + if (operation === "pull_adb_file" && process.env.REA_ADB_PULL === "throw") + throw new Error("pull transport crashed"); + if (operation === "pull_adb_file" && process.env.REA_ADB_PULL === "error") + return { ok: false, error: { message: "pull refused" } }; const result = { - inspect_adb_client: { client: { version: "1", binary_path: "/tmp/adb" } }, + inspect_adb_client: { client: { version: "1", binary_path: process.env.REA_FAKE_ADB } }, list_adb_devices: { devices: [{ serial: "test", state: "device", kind: "emulator", kind_basis: "stub" }] }, inspect_adb_device: { properties: [{ name: "ro.build.version.sdk", value: "35" }], emulator_observed: true }, list_adb_processes: { processes: [{ name: "probe" }], columns: "pid name" }, @@ -61,7 +101,7 @@ export const createAdbDeviceAnalysisProvider = () => ({ read_adb_logcat: { lines: [], coverage: "complete" }, list_adb_features: { features: ["feature"], coverage: "complete" }, list_adb_services: { services: ["service"], coverage: "complete" }, - list_adb_directory: { entries: [], coverage: "complete" }, + list_adb_directory: { entries: (await readdir(device)).map((name) => ({ name })), coverage: "complete" }, list_adb_packages: { packages: input.scope === "all" ? [{ package_name: "com.test" }] : [], coverage: "complete" }, push_adb_file: { bytes: 3, sha256: "${"a".repeat(64)}", device_sha256: "${"a".repeat(64)}", device_digest_source: "device_sha256sum" }, pull_adb_file: { bytes: 3, sha256: "${"a".repeat(64)}" }, @@ -72,6 +112,7 @@ export const createAdbDeviceAnalysisProvider = () => ({ }\n`, ); const marker = join(root, "provider-closed"); + const adbLog = join(root, "adb.log"); const run = spawnSync( process.execPath, [join(root, "scripts", "verify-adb.mjs")], @@ -82,6 +123,11 @@ export const createAdbDeviceAnalysisProvider = () => ({ ...process.env, REA_ADB_CLOSE_MARKER: marker, REA_ADB_CLOSE_FAIL: settings.closeFails ? "1" : "0", + REA_FAKE_ADB: adb, + REA_FAKE_ADB_LOG: adbLog, + REA_FAKE_ADB_REMOVAL: settings.removal ?? "removes", + REA_ADB_PULL: settings.pull ?? "succeeds", + REA_FAKE_DEVICE: device, ...(settings.thrown === undefined ? {} : { REA_ADB_THROW: settings.thrown }), @@ -89,16 +135,91 @@ export const createAdbDeviceAnalysisProvider = () => ({ }, ); const closed = await readFile(marker, "utf8").catch(() => null); - return { ...run, closed }; + const adbCalls = await readFile(adbLog, "utf8").then( + (text) => text.trim().split("\n"), + () => [], + ); + return { ...run, closed, adbCalls, deviceFiles: await readdir(device) }; }; -it("runs the production verifier successfully and closes its provider", async () => { - const result = await runVerifier(); +// The fake adb client is a POSIX shell script. +const posix = it.skipIf(process.platform === "win32"); - expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); - expect(result.stdout).toContain("all adb device checks passed"); - expect(result.closed).toBe("closed"); -}); +posix( + "runs the production verifier successfully and closes its provider", + async () => { + const result = await runVerifier(); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout).toContain("all adb device checks passed"); + expect(result.closed).toBe("closed"); + }, +); + +posix( + "removes exactly its own pushed probe before reporting success", + async () => { + const result = await runVerifier(); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.adbCalls).toHaveLength(1); + expect(result.adbCalls[0]).toMatch( + /^-s test shell rm -- \/data\/local\/tmp\/rea-verify-\d+\.txt$/u, + ); + expect(result.stdout).toContain("[ok] verifier transfer probe removed"); + expect(result.deviceFiles).toEqual([]); + }, +); + +posix( + "fails visibly and keeps the probe when device removal fails", + async () => { + const result = await runVerifier({ removal: "fails" }); + + expect(result.status).toBe(1); + expect(result.stdout).toMatch( + /\[FAIL\] verifier transfer probe removed — \/data\/local\/tmp\/rea-verify-\d+\.txt: /u, + ); + expect(result.stdout).not.toContain("all adb device checks passed"); + expect(result.deviceFiles).toHaveLength(1); + expect(result.closed).toBe("closed"); + }, +); + +posix( + "fails when removal reports success but the listing still shows the probe", + async () => { + const result = await runVerifier({ removal: "leaves_probe" }); + + expect(result.status).toBe(1); + expect(result.adbCalls).toHaveLength(1); + expect(result.stdout).toMatch( + /\[FAIL\] verifier transfer probe removed — \/data\/local\/tmp\/rea-verify-\d+\.txt\n/u, + ); + expect(result.stdout).not.toContain("all adb device checks passed"); + expect(result.deviceFiles).toHaveLength(1); + }, +); + +posix.each([ + ["returns an error", "error", "[FAIL] pull_adb_file — pull refused"], + ["throws", "throw", "verifier aborted — Error: pull transport crashed"], +] as const)( + "still removes the probe when the pull %s", + async (_label, pull, failure) => { + const result = await runVerifier({ pull }); + + expect(result.status).toBe(1); + expect(`${result.stdout}${result.stderr}`).toContain(failure); + expect(result.adbCalls).toHaveLength(1); + expect(result.adbCalls[0]).toMatch( + /^-s test shell rm -- \/data\/local\/tmp\/rea-verify-\d+\.txt$/u, + ); + expect(result.stdout).toContain("[ok] verifier transfer probe removed"); + expect(result.deviceFiles).toEqual([]); + expect(result.closed).toBe("closed"); + }, +); it("makes provider cleanup failure fail the production verifier with resource details", async () => { const result = await runVerifier({ closeFails: true });