From 92fddf3176f348e509ea664db0d4a4de76bb0093 Mon Sep 17 00:00:00 2001 From: Kaoru0822 <208281322+kaoru0822-kitauji@users.noreply.github.com> Date: Sun, 11 Oct 2026 20:05:33 +0000 Subject: [PATCH] fix(adb): share host configuration refusals with apktool The ADB provider, which the apktool provider mirrors, had the same two defects: a relative REA_ADB_PATH escaped as UNKNOWN because selection ran before the try that maps configuration failures, and a missing adb binary projected generic target advice instead of its reason. Move the configuration-failure-to-refusal mapping into one providerConfigurationRefusal helper used by both providers, resolve the ADB selection inside the mapped try, and share the CLI refusal assertion. Co-Authored-By: Claude Opus 5.5 --- src/adb/AdbProvider.ts | 14 +++++------ src/apktool/ApktoolProvider.ts | 14 +++-------- src/application/AnalysisProvider.ts | 26 ++++++++++++++++++++ tests/boundary/cli/adbCli.test.ts | 35 +++++++++++++++++++++------ tests/boundary/cli/apktoolCli.test.ts | 15 ++++-------- tests/boundary/cli/cliServeHarness.ts | 17 +++++++++++++ 6 files changed, 85 insertions(+), 36 deletions(-) diff --git a/src/adb/AdbProvider.ts b/src/adb/AdbProvider.ts index 9e33404b99..e88f8d8073 100644 --- a/src/adb/AdbProvider.ts +++ b/src/adb/AdbProvider.ts @@ -10,12 +10,12 @@ import type { } from "../application/AnalysisProvider.js"; import { createAnalysisExecution, + providerConfigurationRefusal, type AnalysisExecution, } from "../application/AnalysisProvider.js"; import type { AdbRequest } from "../domain/adb/adbDeviceAnalysis.js"; import { AnalysisArtifactChangedError, - AnalysisCapabilityUnavailableError, AnalysisCancelledError, AnalysisInputError, AnalysisOutputError, @@ -417,8 +417,8 @@ export class AdbProvider { async inspectAvailability( signal?: AbortSignal, ): Promise { - const selection = this.selection(); try { + const selection = this.selection(); await inspectAdbClient(selection, signal); return { status: "available", @@ -555,21 +555,19 @@ export class AdbProvider { const signal = options?.signal; if (signal?.aborted === true) return err(new AnalysisCancelledError(request.operation)); - const selection = this.selection(); + let selection: AdbBinarySelection; let identity: AdbClientIdentity; try { + selection = this.selection(); identity = await inspectAdbClient(selection, signal); } catch (cause) { if (cause instanceof AnalysisCancelledError) return err(cause); if (cause instanceof AdbConfigurationFailure) return err( - new AnalysisCapabilityUnavailableError( + providerConfigurationRefusal( ADB_PROVIDER_IDENTITY.id, request.operation, - typeof cause.diagnostics.remediation === "string" - ? `${cause.message} ${cause.diagnostics.remediation}` - : cause.message, - { cause }, + cause, ), ); return err( diff --git a/src/apktool/ApktoolProvider.ts b/src/apktool/ApktoolProvider.ts index eba734883b..e61cf7186c 100644 --- a/src/apktool/ApktoolProvider.ts +++ b/src/apktool/ApktoolProvider.ts @@ -22,6 +22,7 @@ import type { } from "../application/AnalysisProvider.js"; import { createAnalysisExecution, + providerConfigurationRefusal, type AnalysisExecution, } from "../application/AnalysisProvider.js"; import type { ApktoolRequest } from "../domain/apktool/apktoolResourceAnalysis.js"; @@ -409,21 +410,14 @@ export class ApktoolProvider { ); } catch (cause) { if (cause instanceof AnalysisCancelledError) return err(cause); - if (cause instanceof ApktoolConfigurationFailure) { - // A host configuration refusal; the target is not the problem. - const reason = - typeof cause.diagnostics.remediation === "string" - ? `${cause.message}. ${cause.diagnostics.remediation}.` - : `${cause.message}.`; + if (cause instanceof ApktoolConfigurationFailure) return err( - new AnalysisCapabilityUnavailableError( + providerConfigurationRefusal( APKTOOL_PROVIDER_IDENTITY.id, request.operation, - reason, - { cause, userMessage: reason }, + cause, ), ); - } return err( new AnalysisProtocolError( `apktool version probe failed for ${request.operation}`, diff --git a/src/application/AnalysisProvider.ts b/src/application/AnalysisProvider.ts index d88da6df73..598dd79ff8 100644 --- a/src/application/AnalysisProvider.ts +++ b/src/application/AnalysisProvider.ts @@ -5,6 +5,7 @@ import type { NativeToolName } from "../contracts/native/nativeToolContracts.js" import type { ArtifactAnalysisOperation } from "../contracts/artifactToolContracts.js"; import type { ManagedToolName } from "../contracts/managed/managedToolContracts.js"; import type { AnalysisError } from "../domain/analysisErrorBase.js"; +import { AnalysisCapabilityUnavailableError } from "../domain/analysisErrorCore.js"; import type { AnalysisProfileCommitment } from "../domain/analysisProfile.js"; import type { JsonValue } from "../domain/jsonValue.js"; import type { Result } from "../domain/result.js"; @@ -210,6 +211,31 @@ export type ProviderAvailability = readonly diagnostics: Readonly>; }; +/** A host tool configuration failure observed before any target work. */ +export interface ProviderConfigurationFailure extends Error { + readonly diagnostics: Readonly>; +} + +/** + * Refuse an operation because the host tool is misconfigured. The reason, + * not the generic target advice, tells the caller what to change. + */ +export const providerConfigurationRefusal = ( + providerId: string, + operation: string, + failure: ProviderConfigurationFailure, +): AnalysisCapabilityUnavailableError => { + const remediation = failure.diagnostics.remediation; + const reason = + typeof remediation === "string" + ? `${failure.message}. ${remediation}.` + : `${failure.message}.`; + return new AnalysisCapabilityUnavailableError(providerId, operation, reason, { + cause: failure, + userMessage: reason, + }); +}; + /** Provider-owned target support observation before profile resolution. */ export type ProviderTargetSupport = | { diff --git a/tests/boundary/cli/adbCli.test.ts b/tests/boundary/cli/adbCli.test.ts index ac5e96fa09..f361667804 100644 --- a/tests/boundary/cli/adbCli.test.ts +++ b/tests/boundary/cli/adbCli.test.ts @@ -1,6 +1,9 @@ import { expect, it } from "vitest"; -import { serveCli as serve } from "./cliServeHarness.js"; +import { + expectConfigurationRefusal, + serveCli as serve, +} from "./cliServeHarness.js"; it.each([ ["inspect-adb-client"], @@ -34,13 +37,29 @@ it.each([ expect(schema).toBeTruthy(); }); -it("reports a missing adb binary with recovery guidance", async () => { - const result = await serve(["inspect-adb-client", "--json"], { - REA_ADB_PATH: "/nonexistent/adb", - }); - expect(result.stdout).toContain("capability_unavailable"); - expect(result.stdout).toContain("REA_ADB_PATH"); -}); +it.each([ + [ + ["inspect-adb-client", "--json"], + "/nonexistent/adb", + "The selected adb binary was not found: /nonexistent/adb. Set REA_ADB_PATH", + ], + [ + ["list-adb-devices", "--json"], + "/nonexistent/adb", + "The selected adb binary was not found: /nonexistent/adb. Set REA_ADB_PATH", + ], + [ + ["list-adb-devices", "--json"], + "relative/adb", + "REA_ADB_PATH must be an absolute path to the adb binary.", + ], +])( + "%j with REA_ADB_PATH=%s names the host requirement", + async (argv, binary, reason) => { + const result = await serve(argv, { REA_ADB_PATH: binary }); + expectConfigurationRefusal(result, reason); + }, +); it("validates device serials before contacting any device", async () => { const result = await serve(["inspect-adb-device", "bad serial", "--json"]); diff --git a/tests/boundary/cli/apktoolCli.test.ts b/tests/boundary/cli/apktoolCli.test.ts index e3415e6f0b..f52855b788 100644 --- a/tests/boundary/cli/apktoolCli.test.ts +++ b/tests/boundary/cli/apktoolCli.test.ts @@ -1,6 +1,9 @@ import { expect, it } from "vitest"; -import { serveCli as serve } from "./cliServeHarness.js"; +import { + expectConfigurationRefusal, + serveCli as serve, +} from "./cliServeHarness.js"; it.each(["inspect-apktool-client", "decode-android-resources"])( "%s advertises a schema through the public CLI", @@ -32,15 +35,7 @@ it.each([ "%j with REA_APKTOOL_COMMAND=%s names the host requirement", async (argv, command, reason) => { const result = await serve(argv, { REA_APKTOOL_COMMAND: command }); - const output = JSON.parse(result.stdout) as { - code?: unknown; - message?: unknown; - remediation?: { action?: unknown }; - }; - expect(output.code).toBe("capability_unavailable"); - // The target is not the problem; the refusal must say what to configure. - expect(output.message).toEqual(expect.stringContaining(reason)); - expect(output.remediation?.action).toEqual(expect.stringContaining(reason)); + expectConfigurationRefusal(result, reason); }, ); diff --git a/tests/boundary/cli/cliServeHarness.ts b/tests/boundary/cli/cliServeHarness.ts index 35676e842b..827d1c3e32 100644 --- a/tests/boundary/cli/cliServeHarness.ts +++ b/tests/boundary/cli/cliServeHarness.ts @@ -1,3 +1,5 @@ +import { expect } from "vitest"; + import { createCli } from "../../../src/cli.js"; /** Captured CLI invocation for boundary tests. */ @@ -25,3 +27,18 @@ export const serveCli = async ( }); return { stdout, exitCode }; }; + +/** A host tool configuration refusal names what to configure, not the target. */ +export const expectConfigurationRefusal = ( + result: ServedCli, + reason: string, +): void => { + const output = JSON.parse(result.stdout) as { + code?: unknown; + message?: unknown; + remediation?: { action?: unknown }; + }; + expect(output.code).toBe("capability_unavailable"); + expect(output.message).toEqual(expect.stringContaining(reason)); + expect(output.remediation?.action).toEqual(expect.stringContaining(reason)); +};