From c89972c8777d0114566fe76a060849a18bdd2659 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Tue, 13 Oct 2026 04:12:20 +0800 Subject: [PATCH] fix(adb): evaluate the full activity start report Evaluate stdout and stderr together, giving hard errors and non-ok wait statuses precedence over an attempted Starting line. Retain raw streams and typed partial launch facts when Activity Manager refuses or times out; retain successful raw output too. Validation: parser/provider tests and public MCP app/activity starts through a fake executable at the real process boundary. Real Android device behavior was not verified; device-reported timeout remains distinct from a host deadline. Fixes #2225 --- src/adb/AdbProvider.ts | 55 +++++-- src/adb/AdbShellOutput.test.ts | 75 ++++++++++ src/adb/AdbShellOutput.ts | 78 ++++++++-- src/contracts/adb/adbToolContracts.ts | 4 +- src/domain/adb/adbDeviceAnalysis.ts | 17 +++ src/domain/analysisErrorBase.ts | 2 + tests/boundary/mcp/adbStartMcp.test.ts | 189 +++++++++++++++++++++++++ 7 files changed, 390 insertions(+), 30 deletions(-) create mode 100644 src/adb/AdbShellOutput.test.ts create mode 100644 tests/boundary/mcp/adbStartMcp.test.ts diff --git a/src/adb/AdbProvider.ts b/src/adb/AdbProvider.ts index 7113d49bfa..b9af6d14fc 100644 --- a/src/adb/AdbProvider.ts +++ b/src/adb/AdbProvider.ts @@ -23,9 +23,11 @@ import { type AnalysisExecution, } from "../application/AnalysisProvider.js"; import type { + AdbActivityStartPartialObservation, AdbAcquiredFilePartialObservation, AdbRequest, } from "../domain/adb/adbDeviceAnalysis.js"; +import { adbResultSchemas } from "../domain/adb/adbDeviceAnalysis.js"; import { AnalysisArtifactChangedError, AnalysisCancelledError, @@ -1985,28 +1987,55 @@ export class AdbProvider { signal, ); if (!started.ok) return started; - const verdict = parseAmStartOutput(started.value.stdout); - if (verdict.status === "refused") + const verdict = parseAmStartOutput( + started.value.stdout, + started.value.stderr, + ); + const result = { + ...resultFields, + started_activity: verdict.startedActivity, + reported_activity: verdict.activity, + reported_status: verdict.reportedStatus, + }; + if (verdict.status === "refused") { + const partialObservation: AdbActivityStartPartialObservation = + operation === "start_adb_app" + ? { + provider_id: "adb", + operation, + result: adbResultSchemas.start_adb_app.parse(result), + } + : { + provider_id: "adb", + operation, + result: adbResultSchemas.start_adb_activity.parse(result), + }; return err( new AnalysisUnsupportedTargetError( operation, serial, verdict.reason ?? "am start refused the intent", + { + capturedOutput: { + stdout: started.value.stdout, + stderr: started.value.stderr, + truncated: false, + }, + partialObservation, + }, ), ); + } return ok( - createAnalysisExecution( - { - ...resultFields, - started_activity: verdict.startedActivity, + createAnalysisExecution(result, provider, { + rawResult: { + stdout: started.value.stdout, + stderr: started.value.stderr, }, - provider, - { - limitations: [ - "Starting an activity changes device state; am start -W waits for launch completion but no lifecycle observation is captured", - ], - }, - ) as AnalysisExecution, + limitations: [ + "Starting an activity changes device state; am start -W waits for launch completion but no lifecycle observation is captured", + ], + }) as AnalysisExecution, ); } diff --git a/src/adb/AdbShellOutput.test.ts b/src/adb/AdbShellOutput.test.ts new file mode 100644 index 0000000000..1b9ad1f0b4 --- /dev/null +++ b/src/adb/AdbShellOutput.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it } from "vitest"; + +import { parseAmStartOutput } from "./AdbShellOutput.js"; + +const starting = + "Starting: Intent { act=android.intent.action.MAIN cmp=com.example/.Main }"; + +describe("parseAmStartOutput", () => { + it("recognizes a CRLF wait-timeout report", () => { + expect( + parseAmStartOutput( + `${starting}\r\nStatus: timeout\r\nActivity: com.example/.Main\r\nComplete\r\n`, + ), + ).toMatchObject({ + status: "refused", + activity: "com.example/.Main", + reportedStatus: "timeout", + reason: "am start reported status: timeout", + }); + }); + + it("treats every non-ok status as refusal, including a later status after ok", () => { + expect( + parseAmStartOutput(`${starting}\nStatus: ok\nStatus: timeout\n`), + ).toMatchObject({ + status: "refused", + reportedStatus: "timeout", + reason: "am start reported status: timeout", + }); + expect( + parseAmStartOutput(`${starting}\nStatus: future-value\n`), + ).toMatchObject({ + status: "refused", + reportedStatus: "future-value", + }); + }); + + it("gives hard errors in either stream priority over status and attempt", () => { + expect( + parseAmStartOutput( + `${starting}\nStatus: timeout\nActivity: com.example/.Main\n`, + "Exception: Activity class does not exist\n", + ), + ).toEqual({ + status: "refused", + startedActivity: "act=android.intent.action.MAIN cmp=com.example/.Main", + activity: "com.example/.Main", + reportedStatus: "timeout", + reason: "Activity class does not exist", + }); + expect( + parseAmStartOutput( + `${starting}\nStatus: ok\nError: Activity class does not exist\n`, + ), + ).toMatchObject({ + status: "refused", + reason: "Activity class does not exist", + }); + }); + + it("keeps Starting-only output compatible and refuses no-attempt output", () => { + expect(parseAmStartOutput(starting)).toMatchObject({ + status: "started", + reportedStatus: null, + activity: null, + }); + expect( + parseAmStartOutput("Status: ok\nWarning: Activity not started\n"), + ).toMatchObject({ + status: "refused", + startedActivity: null, + reportedStatus: "ok", + }); + }); +}); diff --git a/src/adb/AdbShellOutput.ts b/src/adb/AdbShellOutput.ts index 97720366f4..f3323e7b25 100644 --- a/src/adb/AdbShellOutput.ts +++ b/src/adb/AdbShellOutput.ts @@ -320,32 +320,80 @@ export const parseResolveActivityOutput = (stdout: string): string | null => { return lines.length === 0 ? null : lines[lines.length - 1]!; }; +/** Parsed facts and verdict from the full `am start -W` report. */ +export interface AmStartOutput { + readonly status: "started" | "refused"; + readonly startedActivity: string | null; + readonly activity: string | null; + readonly reportedStatus: string | null; + readonly reason: string | null; +} + /** - * Parse `am start` output. Success prints `Starting: Intent { ... }`; - * refusals print lines starting with `Error:` or `Warning:` and activity - * classes that do not exist name the component. + * Parse the complete `am start -W` report. `Starting:` records an attempt; + * hard errors and any non-ok Activity Manager status determine the verdict. */ export const parseAmStartOutput = ( stdout: string, -): { - readonly status: "started" | "refused"; - readonly startedActivity: string | null; - readonly reason: string | null; -} => { - const text = stdout.trim(); - const starting = /^Starting:\s+Intent\s*\{(.*)\}\s*$/mu.exec(text); - if (starting !== null) + stderr = "", +): AmStartOutput => { + const normalizedStdout = stdout.replaceAll("\r\n", "\n"); + const starting = /^Starting:[ \t]+Intent[ \t]*\{(.*)\}[ \t]*$/mu.exec( + normalizedStdout, + ); + const startedActivity = starting?.[1]?.trim() ?? null; + const activityMatch = /^Activity:[ \t]*(.*)$/mu.exec(normalizedStdout); + const activity = activityMatch?.[1]?.trim() || null; + const statuses = [...normalizedStdout.matchAll(/^Status:[ \t]*(.*)$/gmu)].map( + (match) => match[1]!.trim(), + ); + const nonOkStatus = statuses.find((status) => status.toLowerCase() !== "ok"); + const reportedStatus = nonOkStatus ?? statuses[0] ?? null; + const error = [normalizedStdout, stderr.replaceAll("\r\n", "\n")] + .flatMap((stream) => stream.split("\n")) + .map((line) => line.trim()) + .map((line) => ({ + line, + match: /^(?:Error|Exception)(?::|\b)(.*)$/iu.exec(line), + })) + .find(({ match }) => match !== null); + const errorReason = error?.match?.[1]?.trim() || error?.line || null; + const statusReason = + nonOkStatus === "" + ? "am start reported an empty status" + : nonOkStatus === undefined + ? null + : `am start reported status: ${nonOkStatus}`; + const reason = + errorReason ?? + statusReason ?? + (/^Warning:/mu.test(normalizedStdout.trim()) + ? normalizedStdout.trim().slice(0, 512) + : null); + if (errorReason !== null || nonOkStatus !== undefined) + return { + status: "refused", + startedActivity, + activity, + reportedStatus, + reason: reason ?? "am start did not report a successful launch", + }; + if (startedActivity !== null) return { status: "started", - startedActivity: starting[1]!.trim(), + startedActivity, + activity, + reportedStatus, reason: null, }; - const error = /^(?:Error|Warning|Exception)(?::|\b)(.*)$/mu.exec(text); + const text = `${normalizedStdout}\n${stderr.replaceAll("\r\n", "\n")}`.trim(); return { status: "refused", - startedActivity: null, + startedActivity, + activity, + reportedStatus, reason: - error?.[1]?.trim() ?? + reason ?? (text === "" ? "am start returned no activity" : text.slice(0, 512)), }; }; diff --git a/src/contracts/adb/adbToolContracts.ts b/src/contracts/adb/adbToolContracts.ts index 0ebd39790f..b121c8846e 100644 --- a/src/contracts/adb/adbToolContracts.ts +++ b/src/contracts/adb/adbToolContracts.ts @@ -326,7 +326,7 @@ export const ADB_TOOL_CONTRACTS = [ name: "start_adb_app", ...toolContractMetadata("start_adb_app"), description: - "Start one installed app by launching its declared launcher activity: the component is resolved through `cmd package resolve-activity --brief`, then started with `am start -n`. A device-mutating tool for dynamic-analysis workflows; pair with `read_adb_logcat` (pid filter) and `list_adb_processes` to observe the started app, and `stop_adb_app` to end the run.", + "Start one installed app's resolved launcher activity with `am start -W -n`. A device-mutating tool for dynamic analysis. The full wait report determines success; failures retain launch-attempt facts and raw output. Pair with `read_adb_logcat` (pid filter) and `list_adb_processes` to observe the app, and `stop_adb_app` to end the run.", kind: "adb-provider", inputSchema: adbInputSchemas.start_adb_app, outputSchema: evidenceResultOf(adbResultSchemas.start_adb_app), @@ -341,7 +341,7 @@ export const ADB_TOOL_CONTRACTS = [ name: "start_adb_activity", ...toolContractMetadata("start_adb_activity"), description: - "Start an intent with `am start`: one action, optional data URI, optional explicit component, and up to 16 typed extras (string, boolean, int, long, float). A device-mutating tool; the activity manager's Starting/Error verdict is preserved verbatim.", + "Start an intent with `am start -W`: one action, optional data URI, optional explicit component, and up to 16 typed extras (string, boolean, int, long, float). A device-mutating tool; the full wait report determines success, with failed-attempt facts and raw output retained.", kind: "adb-provider", inputSchema: adbInputSchemas.start_adb_activity, outputSchema: evidenceResultOf(adbResultSchemas.start_adb_activity), diff --git a/src/domain/adb/adbDeviceAnalysis.ts b/src/domain/adb/adbDeviceAnalysis.ts index 11198df5e7..17cdc223ea 100644 --- a/src/domain/adb/adbDeviceAnalysis.ts +++ b/src/domain/adb/adbDeviceAnalysis.ts @@ -679,6 +679,8 @@ export const adbResultSchemas = { package_name: packageNameSchema, component: z.string().min(1), started_activity: z.string().nullable(), + reported_activity: z.string().nullable().optional(), + reported_status: z.string().nullable().optional(), }), start_adb_activity: z.strictObject({ client, @@ -694,6 +696,8 @@ export const adbResultSchemas = { }), ), started_activity: z.string().nullable(), + reported_activity: z.string().nullable().optional(), + reported_status: z.string().nullable().optional(), }), stop_adb_app: z.strictObject({ client, @@ -702,6 +706,19 @@ export const adbResultSchemas = { }), } as const; +/** Launch-attempt facts retained when Activity Manager does not report success. */ +export type AdbActivityStartPartialObservation = + | { + readonly provider_id: "adb"; + readonly operation: "start_adb_app"; + readonly result: z.infer; + } + | { + readonly provider_id: "adb"; + readonly operation: "start_adb_activity"; + readonly result: z.infer; + }; + /** Acquired package facts retained when cancellation leaves caller-owned output. */ export type AdbPackagePullPartialObservation = { readonly provider_id: "adb"; diff --git a/src/domain/analysisErrorBase.ts b/src/domain/analysisErrorBase.ts index 43f39f6279..60c3c04726 100644 --- a/src/domain/analysisErrorBase.ts +++ b/src/domain/analysisErrorBase.ts @@ -114,6 +114,7 @@ import type { Evidence } from "./evidence.js"; import type { AndroidPartialObservation } from "./android/androidPartialObservation.js"; import type { FirmwarePartialObservation } from "./firmware/firmwareAnalysis.js"; import type { + AdbActivityStartPartialObservation, AdbAcquiredFilePartialObservation, AdbPackagePullPartialObservation, } from "./adb/adbDeviceAnalysis.js"; @@ -143,5 +144,6 @@ export type AnalysisPartialObservation = | AndroidPartialObservation | FirmwarePartialObservation | NativeUiObservationResult + | AdbActivityStartPartialObservation | AdbAcquiredFilePartialObservation | AdbPackagePullPartialObservation; diff --git a/tests/boundary/mcp/adbStartMcp.test.ts b/tests/boundary/mcp/adbStartMcp.test.ts new file mode 100644 index 0000000000..702a6360d6 --- /dev/null +++ b/tests/boundary/mcp/adbStartMcp.test.ts @@ -0,0 +1,189 @@ +import { Client, InMemoryTransport } from "@modelcontextprotocol/client"; +import { chmod, readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { expect, it, onTestFinished } from "vitest"; + +import { AdbProvider } from "../../../src/adb/AdbProvider.js"; +import { analysisErrorProjectionSchema } from "../../../src/contracts/errorSchemas.js"; +import { toolContract } from "../../../src/contracts/toolContracts.js"; +import { parseEvidence } from "../../../src/domain/evidence.js"; +import { createServer } from "../../../src/server/createServer.js"; +import { createTestBinarySession } from "../../fixtures/binarySession.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +const responseText = (response: { readonly content: readonly unknown[] }) => + response.content + .flatMap((part) => + typeof part === "object" && + part !== null && + "text" in part && + typeof part.text === "string" + ? [part.text] + : [], + ) + .join(""); + +const createFakeAdb = async () => { + const directory = await createTestTempDirectory("rea-adb-start-mcp-"); + const binary = join(directory, "adb"); + const stdoutFile = join(directory, "stdout"); + const stderrFile = join(directory, "stderr"); + const callsFile = join(directory, "calls"); + const fakeAdbSource = `#!/usr/bin/env node +const fs = require("node:fs"); +const [stdoutPath, stderrPath, callsPath] = ${JSON.stringify([stdoutFile, stderrFile, callsFile])}; +const args = process.argv.slice(2); +fs.appendFileSync(callsPath, args.join(" ") + "\\n"); +if (args[0] === "version") { + process.stdout.write("Android Debug Bridge version 1.0.41\\nVersion 34.0.5-stub\\n"); +} else if (args.includes("resolve-activity")) { + process.stdout.write("com.example/.Main\\n"); +} else { + process.stdout.write(fs.readFileSync(stdoutPath, "utf8")); + process.stderr.write(fs.readFileSync(stderrPath, "utf8")); +} +`; + await writeFile(binary, fakeAdbSource); + await chmod(binary, 0o755); + return { + binary, + calls: async () => (await readFile(callsFile, "utf8")).trim().split("\n"), + setReport: async (stdout: string, stderr: string) => { + await writeFile(stdoutFile, stdout); + await writeFile(stderrFile, stderr); + }, + }; +}; + +it + .skipIf(process.platform === "win32") + .each(["start_adb_app", "start_adb_activity"] as const)( + "%s preserves successful reports and failed launch attempts through MCP (ADB process output simulated)", + async (operation) => { + const fakeAdb = await createFakeAdb(); + const { binary } = fakeAdb; + const starting = + "Starting: Intent { act=android.intent.action.MAIN cmp=com.example/.Main }\n"; + let stdout = `${starting}Warning: Activity not started, intent has been delivered to currently running top-most instance.\nStatus: ok\nActivity: com.example/.Main\nWaitTime: 315\nComplete\n`; + let stderr = ""; + await fakeAdb.setReport(stdout, stderr); + + const server = createServer( + { + kind: "session", + session: createTestBinarySession(() => { + throw new Error("ADB start must not open a deep-analysis provider"); + }), + }, + { + adbDeviceAnalysis: new AdbProvider({ + environment: { REA_ADB_PATH: binary }, + }), + }, + ); + onTestFinished(() => server.close()); + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + const client = new Client({ name: "adb-start-boundary", version: "0.0.0" }); + onTestFinished(() => client.close()); + await client.connect(clientTransport); + + const input = + operation === "start_adb_app" + ? { serial: "emulator-5554", package: "com.example" } + : { + serial: "emulator-5554", + action: "android.intent.action.MAIN", + data_uri: "https://example.invalid/launch", + component: "com.example/.Main", + extras: [{ key: "mode", type: "string" as const, value: "test" }], + }; + const success = await client.callTool({ + name: operation, + arguments: input, + }); + expect(success.isError).not.toBe(true); + const evidence = toolContract(operation).outputSchema.parse( + parseEvidence(JSON.parse(responseText(success))), + ); + expect(evidence.confidence).toBe("observed"); + expect(evidence.normalized_result).toMatchObject({ + started_activity: "act=android.intent.action.MAIN cmp=com.example/.Main", + reported_activity: "com.example/.Main", + reported_status: "ok", + }); + expect(evidence.raw_result).toEqual({ stdout, stderr }); + + stdout = `${starting}Status: timeout\nActivity: com.example/.Main\nWaitTime: 10004\nComplete\n`; + await fakeAdb.setReport(stdout, stderr); + const timeout = await client.callTool({ + name: operation, + arguments: input, + }); + expect(timeout.isError).toBe(true); + const timeoutError = analysisErrorProjectionSchema.parse( + JSON.parse(responseText(timeout)).error, + ); + expect(timeoutError).toMatchObject({ + code: "unsupported_target", + details: { + path: "emulator-5554", + reason: "am start reported status: timeout", + captured_output: { stdout, stderr, truncated: false }, + partial_observation: { + provider_id: "adb", + operation, + result: { + started_activity: + "act=android.intent.action.MAIN cmp=com.example/.Main", + reported_activity: "com.example/.Main", + reported_status: "timeout", + }, + }, + }, + }); + expect(timeoutError).not.toHaveProperty("timeout_ms"); + + stdout = `${starting}Status: ok\nActivity: com.example/.Main\nComplete\n`; + stderr = "Exception: Activity class does not exist\n"; + await fakeAdb.setReport(stdout, stderr); + const hardError = await client.callTool({ + name: operation, + arguments: input, + }); + expect(hardError.isError).toBe(true); + const hardErrorProjection = analysisErrorProjectionSchema.parse( + JSON.parse(responseText(hardError)).error, + ); + expect(hardErrorProjection.message).toContain( + "Activity class does not exist", + ); + expect(hardErrorProjection.details).toMatchObject({ + captured_output: { stdout, stderr, truncated: false }, + partial_observation: { + operation, + result: { reported_status: "ok" }, + }, + }); + + const calls = await fakeAdb.calls(); + expect( + calls.filter((arguments_) => arguments_.includes("start -W")), + ).toEqual( + Array.from({ length: 3 }, () => + [ + "-s emulator-5554 shell am start -W", + ...(operation === "start_adb_app" + ? ["-n com.example/.Main"] + : [ + "-a android.intent.action.MAIN", + "-d https://example.invalid/launch", + "-n com.example/.Main", + "--es mode test", + ]), + ].join(" "), + ), + ); + }, +);