diff --git a/README.md b/README.md index 9828ceaa40..beb23e0e20 100644 --- a/README.md +++ b/README.md @@ -245,7 +245,9 @@ Ghidra and IDA use your existing installations. See [provider setup](docs/instal Do I need to start Hopper first? REA starts Hopper when an operation needs it. On macOS, a first-run dialog may -ask you to choose demo mode or activate your license. See +ask you to choose demo mode or activate your license. Complete that prompt in the +same user session before your first CLI/MCP operation or unattended job. Open +Hopper manually if needed. See [Hopper startup and troubleshooting](docs/installation.md#launcher-paths-and-troubleshooting). diff --git a/docs/installation.md b/docs/installation.md index e39e2b6e19..914dcc4b44 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -567,8 +567,13 @@ need Xvfb, Python 3, X11 and XTEST; approved setup installs those dependencies on supported distributions. Add the curl installer's reported executable directory, commonly `~/.local/bin`, to your shell `PATH` if needed. -REA starts Hopper when needed. On macOS, its launcher can bring a window or -first-run dialog forward; choose demo mode or activate your existing license. +REA starts Hopper when needed. On macOS, complete its demo or license prompt in +the same user session before the first CLI/MCP operation or unattended job. Open +Hopper manually if needed; its launcher can also bring the dialog forward. +If startup reports `Cannot launch Hopper! Connection is invalid.` or waits for +bridge readiness, check Hopper for a pending demo, license, or loader dialog. +Complete the dialog and retry. If REA reports `cleanup_incomplete`, follow the +retained-resource guidance below before reopening the target. Hopper serializes analysis requests. Cancelling a wait can leave provider work running, which the session reports. Successful decompilation is cached until a relevant rename or comment changes it. @@ -597,6 +602,11 @@ if document closure is unconfirmed, REA retains the backing image and reports MCP-exit behavior. Startup deadlines report missing bridge readiness and preserve the launcher outcome; a successful helper exit does not prove that a loader dialog completed. +If an owned provider process exits, request failures preserve its observed exit +code or signal and bounded output in `details.launcher`. An exit can precede +output-stream closure; `output_closed: false` keeps that uncertainty explicit. +Later requests retain the known exit instead of reducing it to socket loss. +An external GUI's helper exit does not establish that the GUI itself exited. Cursor navigation returns the observed object start when Hopper snaps an interior address; adjacent-object navigation rejects unmapped inputs and document ends. Native API text rejects NUL characters and unpaired Unicode surrogates before diff --git a/src/domain/analysisErrorBase.ts b/src/domain/analysisErrorBase.ts index 71dad30c3a..389d4870ac 100644 --- a/src/domain/analysisErrorBase.ts +++ b/src/domain/analysisErrorBase.ts @@ -51,9 +51,9 @@ export interface AnalysisCleanupObservation { /** Optional provider-neutral context that must survive typed error projection. */ export interface AnalysisErrorOptions extends ErrorOptions { - readonly capturedOutput?: AnalysisCapturedOutput; - readonly cleanup?: AnalysisCleanupObservation; - readonly partialObservation?: AnalysisPartialObservation; + readonly capturedOutput?: AnalysisCapturedOutput | undefined; + readonly cleanup?: AnalysisCleanupObservation | undefined; + readonly partialObservation?: AnalysisPartialObservation | undefined; } /** Base class for expected analysis, provider, and session failures. */ diff --git a/src/domain/analysisErrorPresentation.ts b/src/domain/analysisErrorPresentation.ts index b0e81e54ee..fa159394d3 100644 --- a/src/domain/analysisErrorPresentation.ts +++ b/src/domain/analysisErrorPresentation.ts @@ -265,9 +265,12 @@ const hopperProcessMessage = (error: HopperProcessError): string => { const startup = error.userMessage === undefined ? "" : ` ${error.userMessage}`; if (error.providerState === "exited") { + const signal = error.launcherFailure?.signal; const exit = error.exitCode === null - ? "no exit code was observed" + ? signal === undefined || signal === null + ? "no exit code was observed" + : `signal ${signal}` : `exit code ${String(error.exitCode)}`; const recovery = error.failureCode === undefined && stage !== "launch" diff --git a/src/domain/analysisErrorProjection.test.ts b/src/domain/analysisErrorProjection.test.ts index 635d5c0219..eb1de87005 100644 --- a/src/domain/analysisErrorProjection.test.ts +++ b/src/domain/analysisErrorProjection.test.ts @@ -572,7 +572,7 @@ describe("analysis error projection: caller contract", () => { describe("analysis error projection: operational diagnostics", () => { it("distinguishes an observed Hopper exit from an undetermined provider failure", () => { const exited = projectAnalysisError( - new HopperProcessError(7, undefined, "search_strings", 4), + new HopperProcessError(7, { operation: "search_strings", requestId: 4 }), ); expect(exited).toMatchObject({ code: "provider_unavailable", @@ -590,7 +590,10 @@ describe("analysis error projection: operational diagnostics", () => { expect(exited.remediation.action).toContain("Restart the owned provider"); const unknown = projectAnalysisError( - new HopperProcessError(null, undefined, "search_strings", 5), + new HopperProcessError(null, { + operation: "search_strings", + requestId: 5, + }), ); expect(unknown.details).toMatchObject({ stage: "analysis", diff --git a/src/domain/hopperErrors.ts b/src/domain/hopperErrors.ts index f2b37a8a5a..2ec3135c70 100644 --- a/src/domain/hopperErrors.ts +++ b/src/domain/hopperErrors.ts @@ -1,4 +1,7 @@ -import { AnalysisError } from "./analysisErrorBase.js"; +import { + AnalysisError, + type AnalysisErrorOptions, +} from "./analysisErrorBase.js"; import type { HopperProcessProviderState, ProviderFailureStage, @@ -73,10 +76,10 @@ export type HopperDiagnosticType = | "bridge_exception"; /** Hopper's JSON-RPC endpoint returned an expected remote error response. */ -export interface HopperRemoteErrorContext { - readonly diagnosticType?: HopperDiagnosticType; - readonly operation?: string; - readonly requestId?: number; +export interface HopperRemoteErrorContext extends AnalysisErrorOptions { + readonly diagnosticType?: HopperDiagnosticType | undefined; + readonly operation?: string | undefined; + readonly requestId?: number | undefined; } export class HopperRemoteError extends HopperError { @@ -90,7 +93,7 @@ export class HopperRemoteError extends HopperError { readonly safeMessage: string, context: HopperRemoteErrorContext = {}, ) { - super(`Hopper request failed (${String(code)}): ${safeMessage}`); + super(`Hopper request failed (${String(code)}): ${safeMessage}`, context); this.diagnosticType = context.diagnosticType ?? "remote"; this.operation = context.operation; this.requestId = context.requestId; @@ -98,35 +101,50 @@ export class HopperRemoteError extends HopperError { } /** The owned Hopper bridge stopped before its client was closed. */ +export interface HopperProcessErrorContext extends AnalysisErrorOptions { + readonly diagnostic?: HopperStartupFailureDiagnostic | undefined; + readonly operation?: string | undefined; + readonly requestId?: number | undefined; + readonly providerState?: HopperProcessProviderState | undefined; + readonly stage?: ProviderFailureStage | undefined; + readonly launcherFailure?: HopperLauncherOutcome | undefined; +} + export class HopperProcessError extends HopperError { readonly _tag = "HopperProcessError"; readonly failureCode: HopperStartupFailureCode | undefined; override readonly userMessage: string | undefined; readonly providerState: HopperProcessProviderState; readonly stage: ProviderFailureStage; + readonly diagnostic: HopperStartupFailureDiagnostic | undefined; + readonly operation: string | undefined; + readonly requestId: number | undefined; + readonly launcherFailure: HopperLauncherOutcome | undefined; constructor( readonly exitCode: number | null, - readonly diagnostic?: HopperStartupFailureDiagnostic, - readonly operation?: string, - readonly requestId?: number, - providerState?: HopperProcessProviderState, - stage?: ProviderFailureStage, - readonly launcherFailure?: HopperLauncherOutcome, + context: HopperProcessErrorContext = {}, ) { - super(`Hopper bridge stopped unexpectedly with code ${String(exitCode)}`); + super( + `Hopper bridge stopped unexpectedly with code ${String(exitCode)}`, + context, + ); + this.diagnostic = context.diagnostic; + this.operation = context.operation; + this.requestId = context.requestId; + this.launcherFailure = context.launcherFailure; const failure = hopperStartupFailure(exitCode); this.stage = - stage ?? + context.stage ?? (failure === undefined - ? operation === undefined + ? context.operation === undefined ? "connection" : "analysis" : "launch"); this.failureCode = failure?.code; this.userMessage = failure?.message; this.providerState = - providerState ?? (exitCode === null ? "unknown" : "exited"); + context.providerState ?? (exitCode === null ? "unknown" : "exited"); } } diff --git a/src/hopper/BridgeLauncher.ts b/src/hopper/BridgeLauncher.ts index a7e8b33afe..8201ee2dfb 100644 --- a/src/hopper/BridgeLauncher.ts +++ b/src/hopper/BridgeLauncher.ts @@ -362,10 +362,9 @@ export class HopperApplicationLauncher implements BridgeLauncher { ? { ok: true, strategy: selection.strategy } : { ok: false, - error: new HopperProcessError( - selection.exitCode, - selection.diagnostic, - ), + error: new HopperProcessError(selection.exitCode, { + diagnostic: selection.diagnostic, + }), }; } } diff --git a/src/hopper/HopperCleanup.ts b/src/hopper/HopperCleanup.ts index 346e0b8949..eb155a9fb5 100644 --- a/src/hopper/HopperCleanup.ts +++ b/src/hopper/HopperCleanup.ts @@ -202,9 +202,7 @@ const shutdownRequestFailure = ( operation: "shutdown" | "shutdown_document", cause: unknown, ): HopperProcessError => { - const failure = new HopperProcessError(null, undefined, operation); - failure.cause = cause; - return failure; + return new HopperProcessError(null, { operation, cause }); }; const shutdownFailureDetails = ( diff --git a/src/hopper/HopperClient.ts b/src/hopper/HopperClient.ts index 99881422e0..2e903c8d2b 100644 --- a/src/hopper/HopperClient.ts +++ b/src/hopper/HopperClient.ts @@ -129,13 +129,9 @@ export class HopperClient { this.#responses.push(chunk); }; readonly #onSocketError = (): void => { - const error = new HopperProcessError( - null, - undefined, - undefined, - undefined, - "unreachable", - ); + const error = new HopperProcessError(null, { + providerState: "unreachable", + }); this.#failAll(error); this.#rememberProcessFailure(error, []); }; @@ -151,25 +147,18 @@ export class HopperClient { setImmediate(() => { if (this.#closing || this.#launcherExitCode !== undefined) return; const exitCode = launch.process.exitCode ?? null; - const error = new HopperProcessError( - exitCode, - this.#launcherFailureDiagnostic, - undefined, - undefined, - exitCode !== null ? "exited" : "unreachable", - ); + const error = new HopperProcessError(exitCode, { + diagnostic: this.#launcherFailureDiagnostic, + providerState: exitCode !== null ? "exited" : "unreachable", + }); this.#failAll(error); this.#rememberProcessFailure(error, []); }); return; } - const error = new HopperProcessError( - null, - undefined, - undefined, - undefined, - "unreachable", - ); + const error = new HopperProcessError(null, { + providerState: "unreachable", + }); this.#failAll(error); this.#rememberProcessFailure(error, []); }; @@ -729,15 +718,12 @@ export class HopperClient { )) ?? false; const launcherFailure = this.#launcherOutcome(outputClosed); if (ownsProviderLifetime) - return new HopperProcessError( - exitCode, - this.#launcherFailureDiagnostic, - undefined, - undefined, - "exited", - "launch", + return new HopperProcessError(exitCode, { + diagnostic: this.#launcherFailureDiagnostic, + providerState: "exited", + stage: "launch", launcherFailure, - ); + }); if ( this.#resources.launch?.authorizationDenialBeforeHandoff === true && nativeHopperLaunchWasDenied(launcherFailure) @@ -766,18 +752,34 @@ export class HopperClient { }, ): Promise> { const id = this.#nextId++; + if ( + this.#resources.launch?.providerLifetime === "launcher-process" && + this.#launcherExitCode !== undefined + ) { + return this.#unavailableRequest( + new HopperProcessError(this.#launcherExitCode, { + diagnostic: this.#launcherFailureDiagnostic, + operation: method, + requestId: id, + providerState: "exited", + stage: hopperOperationStage(method), + launcherFailure: this.#launcherOutcome( + (await this.#resources.processSupervisor?.waitForOutputClose(0)) ?? + false, + ), + }), + ); + } const socket = this.#socket; const token = this.#token; if (socket === undefined || socket.destroyed || token === undefined) { return this.#unavailableRequest( - new HopperProcessError( - null, - undefined, - method, - id, - "unreachable", - hopperOperationStage(method), - ), + new HopperProcessError(null, { + operation: method, + requestId: id, + providerState: "unreachable", + stage: hopperOperationStage(method), + }), ); } if (options.signal?.aborted === true) @@ -888,13 +890,11 @@ export class HopperClient { if (launch.providerLifetime === "launcher-process" && !this.#closing) { this.#rememberExit(event.code); this.#failAll( - new HopperProcessError( - event.code, - this.#launcherFailureDiagnostic, - undefined, - undefined, - "exited", - ), + new HopperProcessError(event.code, { + diagnostic: this.#launcherFailureDiagnostic, + providerState: "exited", + launcherFailure: this.#launcherOutcome(false), + }), ); } } diff --git a/src/hopper/HopperRequestQueue.ts b/src/hopper/HopperRequestQueue.ts index 6ae162e06f..43f4f2821d 100644 --- a/src/hopper/HopperRequestQueue.ts +++ b/src/hopper/HopperRequestQueue.ts @@ -273,14 +273,12 @@ export class HopperRequestQueue { this.accept( entry.id, err( - new HopperProcessError( - null, - undefined, - entry.method, - entry.id, - "unreachable", - hopperOperationStage(entry.method), - ), + new HopperProcessError(null, { + operation: entry.method, + requestId: entry.id, + providerState: "unreachable", + stage: hopperOperationStage(entry.method), + }), ), ), ); @@ -386,19 +384,20 @@ const withRequestContext = ( entry: QueuedRequest, ): HopperError => { if (error instanceof HopperProcessError) - return new HopperProcessError( - error.exitCode, - error.diagnostic, - error.operation ?? entry.method, - error.requestId ?? entry.id, - error.providerState, - error.operation === undefined && error.stage === "connection" - ? hopperOperationStage(entry.method) - : error.stage, - ); + return new HopperProcessError(error.exitCode, { + ...error, + cause: error.cause, + operation: error.operation ?? entry.method, + requestId: error.requestId ?? entry.id, + stage: + error.operation === undefined && error.stage === "connection" + ? hopperOperationStage(entry.method) + : error.stage, + }); if (error instanceof HopperRemoteError) return new HopperRemoteError(error.code, error.safeMessage, { - diagnosticType: error.diagnosticType, + ...error, + cause: error.cause, operation: error.operation ?? entry.method, requestId: error.requestId ?? entry.id, }); diff --git a/tests/boundary/mcp/toolResult.test.ts b/tests/boundary/mcp/toolResult.test.ts index 90e1778f45..c82edd0797 100644 --- a/tests/boundary/mcp/toolResult.test.ts +++ b/tests/boundary/mcp/toolResult.test.ts @@ -338,20 +338,22 @@ describe("tool result projection", () => { const result = delivery.toCallToolResult( err( new HopperProcessError(80, { - component: "hopper_private_display", - operation: "launch", - status: "error", - failure_code: "x11_socket_directory_unusable", - reason: "socket_directory_read_only", - socket_directory: "/tmp/.X11-unix", - socket_directory_mode: "0777", - mount_read_only: true, - effective_socket_directory_mode: "1777", - effective_mount_read_only: false, - wsl: true, - strategy: "user-mount-namespace", - fallback_reason: null, - xvfb_stderr_bytes: 512, + diagnostic: { + component: "hopper_private_display", + operation: "launch", + status: "error", + failure_code: "x11_socket_directory_unusable", + reason: "socket_directory_read_only", + socket_directory: "/tmp/.X11-unix", + socket_directory_mode: "0777", + mount_read_only: true, + effective_socket_directory_mode: "1777", + effective_mount_read_only: false, + wsl: true, + strategy: "user-mount-namespace", + fallback_reason: null, + xvfb_stderr_bytes: 512, + }, }), ), contract, diff --git a/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts b/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts index abfce5834a..cc4a8d1ff6 100644 --- a/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts @@ -3,7 +3,6 @@ import { access } from "node:fs/promises"; import { afterEach, describe, expect, it } from "vitest"; -import { projectAnalysisError } from "../../../../src/domain/analysisErrorProjection.js"; import { ok } from "../../../../src/domain/result.js"; import type { BridgeLauncher, @@ -53,31 +52,6 @@ class OwnedFixtureLauncher implements BridgeLauncher { } } -class UnconfirmedFixtureLauncher implements BridgeLauncher { - constructor(readonly ownsProcessLifetime = false) {} - - launch(session: BridgeSession) { - return Promise.resolve( - ok({ - process: spawn( - process.execPath, - [ - fixturePath, - session.socketPath, - session.token, - session.runId, - "unconfirmed", - ], - { stdio: ["ignore", "ignore", "pipe"] }, - ), - ownsProcessLifetime: this.ownsProcessLifetime, - providerLifetime: "external-application" as const, - shutdownMode: "bridge-request" as const, - }), - ); - } -} - const clients: HopperClient[] = []; afterEach(async () => { @@ -137,42 +111,6 @@ describe("HopperClient cleanup", () => { }); }); - it.each([ - ["unowned", false], - ["owned helper", true], - ] as const)( - "reports unconfirmed external document cleanup for an %s process", - async (_description, ownsProcessLifetime) => { - const client = new HopperClient({ - launcher: new UnconfirmedFixtureLauncher(ownsProcessLifetime), - startupTimeoutMs: 1_000, - }); - clients.push(client); - expect((await client.start()).ok).toBe(true); - - const closed = await client.close(); - - expect(closed).toMatchObject({ - ok: false, - error: { - _tag: "ProviderAdapterError", - cleanupIncomplete: true, - cleanupResources: expect.arrayContaining(["hopper-document"]), - }, - }); - if (!closed.ok) - expect(projectAnalysisError(closed.error)).toMatchObject({ - code: "cleanup_incomplete", - details: { - provider_id: "hopper", - operation: "close_binary", - cleanup: "incomplete", - resources: expect.arrayContaining(["hopper-document"]), - }, - }); - }, - ); - it("kills only its owned Hopper group and emits sanitized shutdown coordinates", async () => { const unrelated = spawn( process.execPath, diff --git a/tests/boundary/providers/hopper/hopperClient.cleanupRetry.test.ts b/tests/boundary/providers/hopper/hopperClient.cleanupRetry.test.ts index eac881e588..e6f8215a03 100644 --- a/tests/boundary/providers/hopper/hopperClient.cleanupRetry.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.cleanupRetry.test.ts @@ -160,50 +160,71 @@ it("retains failed startup cleanup until an owned process and its backing image }); }); -it("keeps an unconfirmed external document visible across sequential closes", async () => { - const launcher = new RetryCleanupLauncher(true); - const runtimeRoot = await PrivateRuntimeRoot.create(); - onTestFinished(() => runtimeRoot.close()); - const launched = await launcher.launch({ - directory: runtimeRoot.path, - socketPath: join(runtimeRoot.path, "bridge.sock"), - token: "fixture-token", - runId: "fixture-run", - }); - if (!launched.ok) throw launched.error; - const child = launched.value.process; - const supervisor = new ProviderProcessSupervisor(launched.value); - onTestFinished(() => supervisor.dispose()); - const resources: HopperOwnedResources = { - launch: launched.value, - processSupervisor: supervisor, - runtimeRoot, - shutdownConfirmed: false, - }; - for (let attempt = 0; attempt < 2; attempt += 1) { - await expect( - cleanupHopperSession({ - socket: undefined, - resources, - activeRequest: null, - retainDocument: false, - progress: undefined, - logger: silentLogger, - onDiagnostic: undefined, - request: () => Promise.resolve(ok(null)), - releaseTransport: (socket) => socket?.destroy(), - }), - ).resolves.toMatchObject({ - ok: false, - error: { cleanupResources: ["hopper-document", runtimeRoot.path] }, +it.each([false, true])( + "keeps an unconfirmed external document across closes when helper ownership is %s", + async (ownsHelper) => { + const launcher = new RetryCleanupLauncher(true); + const runtimeRoot = await PrivateRuntimeRoot.create(); + onTestFinished(() => runtimeRoot.close()); + const launched = await launcher.launch({ + directory: runtimeRoot.path, + socketPath: join(runtimeRoot.path, "bridge.sock"), + token: "fixture-token", + runId: "fixture-run", }); - } - expect(child.exitCode).toBeNull(); - expect(child.signalCode).toBeNull(); - await expect( - access(join(runtimeRoot.path, "image.macho")), - ).resolves.toBeUndefined(); -}); + if (!launched.ok) throw launched.error; + const child = launched.value.process; + const launch: BridgeLaunch = ownsHelper + ? { ...launched.value, ownsProcessLifetime: true } + : launched.value; + const supervisor = new ProviderProcessSupervisor(launch); + onTestFinished(() => supervisor.dispose()); + const resources: HopperOwnedResources = { + launch, + processSupervisor: supervisor, + runtimeRoot, + shutdownConfirmed: false, + }; + const socket = new Socket(); + onTestFinished(() => { + socket.destroy(); + }); + for (let attempt = 0; attempt < 2; attempt += 1) { + await expect( + cleanupHopperSession({ + socket: attempt === 0 ? socket : undefined, + resources, + activeRequest: null, + retainDocument: false, + progress: undefined, + logger: silentLogger, + onDiagnostic: undefined, + request: () => + Promise.resolve( + ok({ + shutdown: true, + analysis_stopped: true, + document_closed: false, + }), + ), + releaseTransport: (socket) => socket?.destroy(), + }), + ).resolves.toMatchObject({ + ok: false, + error: { cleanupResources: ["hopper-document", runtimeRoot.path] }, + }); + } + if (ownsHelper) + expect(child.exitCode !== null || child.signalCode !== null).toBe(true); + else { + expect(child.exitCode).toBeNull(); + expect(child.signalCode).toBeNull(); + } + await expect( + access(join(runtimeRoot.path, "image.macho")), + ).resolves.toBeUndefined(); + }, +); it("blocks a fresh launch while cleanup remains unverified", async () => { const launcher = new RetryCleanupLauncher(); diff --git a/tests/boundary/providers/hopper/hopperClient.health.test.ts b/tests/boundary/providers/hopper/hopperClient.health.test.ts index 76b9be52f5..66748051ef 100644 --- a/tests/boundary/providers/hopper/hopperClient.health.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.health.test.ts @@ -1,4 +1,5 @@ import { EventEmitter } from "node:events"; +import { PassThrough } from "node:stream"; import { describe, expect, it, onTestFinished, vi } from "vitest"; @@ -12,15 +13,18 @@ import { HopperClient } from "../../../../src/hopper/HopperClient.js"; import { HopperFixtureLauncher } from "./hopperClient.fixture.js"; class ObservedProcess extends EventEmitter { - readonly stdout = null; - readonly stderr = null; + readonly stdout = new PassThrough(); + readonly stderr = new PassThrough(); exitCode: number | null = null; - readonly signalCode = null; + signalCode: NodeJS.Signals | null = null; - exit(code: number): void { + exit(code: number | null, signal: NodeJS.Signals | null = null): void { this.exitCode = code; - this.emit("exit", code, null); - this.emit("close", code, null); + this.signalCode = signal; + this.emit("exit", code, signal); + this.stdout.end(); + this.stderr.end(); + this.emit("close", code, signal); } kill(): boolean { @@ -66,7 +70,7 @@ describe("Hopper provider lifecycle health", () => { launcher.observed.exit(0); const pending = client.callTool("hang"); await launcher.fixture.waitForRequest("hang"); - launcher.fixture.processes.at(-1)?.kill("SIGKILL"); + launcher.fixture.processes.at(-1)?.send({ disconnect: true }); await vi.waitFor(() => expect(client.operationHealth().state).toBe("unreachable"), ); @@ -92,6 +96,7 @@ describe("Hopper provider lifecycle health", () => { operation: "procedure_pseudo_code", }, }); + expect((await client.close()).ok).toBe(true); }); it("projects the exit code when the owned process exit is observed before disconnection", async () => { @@ -120,6 +125,7 @@ describe("Hopper provider lifecycle health", () => { stage: "analysis", operation: "hang", request_id: request.id, + launcher: { exit_code: 7, signal: null, output_closed: false }, }, }); }); @@ -166,3 +172,52 @@ describe("Hopper provider lifecycle health", () => { expect(client.operationHealth().state).toBe("idle"); }); }); + +it("preserves an owned signal exit and diagnostics for active, queued and later callers", async () => { + const { client, launcher } = await start(true); + const active = client.callTool("hang"); + const request = await launcher.fixture.waitForRequest("hang"); + const queued = client.callTool("echo"); + await vi.waitFor(() => + expect(client.requestActivity()?.queuedRequests).toBe(1), + ); + const token = launcher.fixture.tokens[0]; + launcher.observed.stderr.write(`API_KEY=local-evidence-kept ${token}\n`); + launcher.observed.exit(null, "SIGTERM"); + const results = await Promise.all([active, queued]); + for (const [index, result] of results.entries()) { + if (result.ok) throw new Error("Expected an observed provider exit"); + const projected = projectAnalysisError(result.error); + expect(projected).toMatchObject({ + code: "provider_unavailable", + message: expect.stringContaining("signal SIGTERM"), + details: { + provider_state: "exited", + operation: index === 0 ? "hang" : "echo", + request_id: request.id + index, + launcher: { + exit_code: null, + signal: "SIGTERM", + output_closed: false, + stderr: { + text: "API_KEY=local-evidence-kept [redacted transport credential]\n", + }, + }, + }, + }); + expect(JSON.stringify(projected)).not.toContain(token); + } + const later = await client.callTool("procedure_pseudo_code", { + procedure: "main", + }); + if (later.ok) throw new Error("An exited provider must reject later queries"); + expect(projectAnalysisError(later.error)).toMatchObject({ + code: "provider_unavailable", + details: { + provider_state: "exited", + stage: "decompilation", + operation: "procedure_pseudo_code", + launcher: { signal: "SIGTERM", output_closed: true }, + }, + }); +}); diff --git a/tests/boundary/providers/hopper/hopperClient.helperFailure.test.ts b/tests/boundary/providers/hopper/hopperClient.helperFailure.test.ts index 928ec76a22..13601e172b 100644 --- a/tests/boundary/providers/hopper/hopperClient.helperFailure.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.helperFailure.test.ts @@ -1,13 +1,68 @@ import { spawn } from "node:child_process"; +import { access } from "node:fs/promises"; import { expect, it, onTestFinished } from "vitest"; import { AnalysisError } from "../../../../src/domain/analysisErrorBase.js"; import { projectAnalysisError } from "../../../../src/domain/analysisErrorProjection.js"; import { ok } from "../../../../src/domain/result.js"; -import type { BridgeLauncher } from "../../../../src/hopper/BridgeLauncher.js"; +import type { + BridgeLauncher, + BridgeSession, +} from "../../../../src/hopper/BridgeLauncher.js"; import { HopperApplicationLauncher } from "../../../../src/hopper/BridgeLauncher.js"; import { HopperClient } from "../../../../src/hopper/HopperClient.js"; +import { HopperFixtureLauncher } from "./hopperClient.fixture.js"; + +// Failure assertions keep the external document unknown. Teardown then publishes +// this run's authenticated bridge so normal cleanup can release its runtime/lease. +const trackFailedHelperClient = ( + launcher: BridgeLauncher, + startupTimeoutMs = 10_000, +): HopperClient => { + let session: BridgeSession | undefined; + let external = false; + const client = new HopperClient({ + launcher: { + async launch(input, options) { + session = input; + const launched = await launcher.launch(input, options); + external = + launched.ok && + launched.value.providerLifetime === "external-application"; + return launched; + }, + }, + startupTimeoutMs, + }); + onTestFinished(async () => { + const fixture = new HopperFixtureLauncher(); + const published = + external && session !== undefined + ? await fixture.launch(session) + : undefined; + try { + const closed = await client.close(); + if (!closed.ok) throw closed.error; + if (published !== undefined) { + expect(fixture.requests.map((request) => request.method)).toEqual([ + "health", + "shutdown_document", + ]); + } + if (session !== undefined) + await expect(access(session.directory)).rejects.toMatchObject({ + code: "ENOENT", + }); + } finally { + if (published?.ok) { + const cleaned = await published.value.cleanup(); + expect(cleaned.cleaned).toBe(true); + } + } + }); + return client; +}; const projectPrimaryStartupFailure = (error: AnalysisError) => { if (!error.cleanupIncomplete) return projectAnalysisError(error); @@ -32,10 +87,7 @@ it("does not equate an owned native helper exit with an external GUI exit", asyn bridgeScriptPath: "/rea/hopper_bridge.py", launchMode: "native", }); - const client = new HopperClient({ launcher, startupTimeoutMs: 10_000 }); - onTestFinished(async () => { - await client.close(); - }, 45_000); + const client = trackFailedHelperClient(launcher); const result = await client.start(); expect(result).toMatchObject({ ok: false, @@ -84,10 +136,7 @@ it.each(["exit", "signal", "exit75"] as const)( ); }, }; - const client = new HopperClient({ launcher, startupTimeoutMs: 10_000 }); - onTestFinished(async () => { - await client.close(); - }, 45_000); + const client = trackFailedHelperClient(launcher); const result = await client.start(); expect(result).toMatchObject({ ok: false, @@ -136,10 +185,7 @@ it.each([0, 1])( ); }, }; - const client = new HopperClient({ launcher, startupTimeoutMs: 10_000 }); - onTestFinished(async () => { - await client.close(); - }, 45_000); + const client = trackFailedHelperClient(launcher); const result = await client.start(); expect(result).toMatchObject({ ok: false, @@ -174,10 +220,7 @@ it("preserves a successful helper's diagnostics when bridge readiness is never o ); }, }; - const client = new HopperClient({ launcher, startupTimeoutMs: 1500 }); - onTestFinished(async () => { - await client.close(); - }, 45_000); + const client = trackFailedHelperClient(launcher, 1500); const result = await client.start(); if (result.ok) throw new Error("Expected missing bridge readiness to time out"); diff --git a/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts b/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts index 88db157995..0388346b92 100644 --- a/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts @@ -53,10 +53,7 @@ class CancelThenFixtureLauncher implements BridgeLauncher { } class ExitingLauncher implements BridgeLauncher { - constructor( - readonly code: number, - readonly ownsProcessLifetime = true, - ) {} + constructor(readonly code: number) {} launch() { return Promise.resolve( @@ -64,10 +61,8 @@ class ExitingLauncher implements BridgeLauncher { process: spawn(process.execPath, ["-e", `process.exit(${this.code})`], { stdio: ["ignore", "ignore", "pipe"], }), - ownsProcessLifetime: this.ownsProcessLifetime, - providerLifetime: this.ownsProcessLifetime - ? ("launcher-process" as const) - : ("external-application" as const), + ownsProcessLifetime: true as const, + providerLifetime: "launcher-process" as const, shutdownMode: "bridge-request" as const, }), ); @@ -286,21 +281,4 @@ describe("HopperClient startup failures", () => { }, }); }); - - it("allows a short-lived launcher to hand off bridge startup", async () => { - const client = new HopperClient({ - launcher: new ExitingLauncher(0, false), - startupTimeoutMs: 100, - }); - clients.push(client); - const result = await client.start(); - expect(result.ok).toBe(false); - if (!result.ok) { - expect(result.error).toMatchObject({ - cleanupIncomplete: true, - cause: { _tag: "HopperTimeoutError" }, - diagnostics: { primary_error: { code: "provider_timeout" } }, - }); - } - }); }); diff --git a/tests/fixtures/fakeHopper.mjs b/tests/fixtures/fakeHopper.mjs index 89886f76d1..192d69851a 100644 --- a/tests/fixtures/fakeHopper.mjs +++ b/tests/fixtures/fakeHopper.mjs @@ -10,8 +10,13 @@ const [ ] = process.argv.slice(2); let diagnosticOutputEmitted = false; const heldReplies = new Map(); +const sockets = new Set(); if (process.send !== undefined) process.on("message", (message) => { + if (message?.disconnect === true) { + for (const socket of sockets) socket.destroy(); + return; + } const reply = heldReplies.get(message?.release); if (reply === undefined) return; heldReplies.delete(message.release); @@ -173,6 +178,8 @@ const sendEchoWithProgress = (send, request) => { }; const server = createServer((socket) => { + sockets.add(socket); + socket.once("close", () => sockets.delete(socket)); socket.on("error", () => undefined); socket.setEncoding("utf8"); let buffer = "";