diff --git a/src/browser/PlaywrightElectronActiveProvider.test.ts b/src/browser/PlaywrightElectronActiveProvider.test.ts index b51e216f73..d74d17a695 100644 --- a/src/browser/PlaywrightElectronActiveProvider.test.ts +++ b/src/browser/PlaywrightElectronActiveProvider.test.ts @@ -9,6 +9,7 @@ import { } from "../../tests/fixtures/electron/fuseImage.js"; import * as processOwnership from "../process/ProcessOwnership.js"; +import * as ownershipObservation from "../process/ProcessOwnershipObservation.js"; import * as electronActions from "./PlaywrightElectronActiveActions.js"; import { electronActiveObservationInputSchema } from "../domain/javascript/electronActiveObservation.js"; import type { OwnedProcessGroup } from "../process/ProcessOwnership.js"; @@ -97,32 +98,90 @@ const emptyHookSnapshot = () => ({ hook_error: false, }); -it("does not signal a Windows Electron PID when lineage is unavailable", async () => { - const terminateTree = vi.fn(async () => ({ - cleaned: true as const, - signaled: true, - })); - const result = await cleanupElectronProcesses( - ownership, - { - status: "unavailable", - observedAt: "2026-10-10T00:00:00.000Z", - runId: ownership.runId, - launcherPid: ownership.leaderPid, - processGroupId: ownership.processGroupId, - reason: "owned launcher is not live", - }, - { platform: "win32", terminateTree }, - ); +it.each([ + "owned launcher is not live", + "process table permission denied", + "process ownership did not match", +])( + "does not signal a Windows Electron PID when lineage is unavailable: %s", + async (reason) => { + const terminateTree = vi.fn(async () => ({ + cleaned: true as const, + signaled: true, + })); + const result = await cleanupElectronProcesses( + ownership, + { + status: "unavailable", + observedAt: "2026-10-10T00:00:00.000Z", + runId: ownership.runId, + launcherPid: ownership.leaderPid, + processGroupId: ownership.processGroupId, + reason, + }, + { platform: "win32", terminateTree }, + ); - expect(terminateTree).not.toHaveBeenCalled(); - expect(result).toEqual({ + expect(terminateTree).not.toHaveBeenCalled(); + expect(result).toEqual({ + cleaned: false, + reason: `${reason}; helper cleanup was not proven`, + }); + }, +); + +it("does not signal an unobserved Windows Electron lineage", async () => { + const terminateTree = vi.fn(); + expect( + await cleanupElectronProcesses(ownership, undefined, { + platform: "win32", + terminateTree, + }), + ).toEqual({ cleaned: false, reason: - "owned Electron lineage was unavailable; helper cleanup was not proven", + "owned Electron lineage was not observed; helper cleanup was not proven", }); + expect(terminateTree).not.toHaveBeenCalled(); }); +it.each([0, 1])( + "preserves a Windows termination failure at tree index %i", + async (failedIndex) => { + const failure = { cleaned: false as const, reason: "termination denied" }; + const terminateTree = vi + .fn() + .mockResolvedValue({ cleaned: true, signaled: true }); + terminateTree.mockImplementationOnce(async () => + failedIndex === 0 ? failure : { cleaned: true, signaled: true }, + ); + if (failedIndex === 1) terminateTree.mockResolvedValueOnce(failure); + const result = await cleanupElectronProcesses( + ownership, + { + status: "verified", + observedAt: "2026-10-10T00:00:00.000Z", + lineage: { + runId: ownership.runId, + launcherPid: ownership.leaderPid, + launcherParentPid: 1, + processGroupId: ownership.processGroupId, + descendants: [ + { + pid: 4343, + parentPid: ownership.leaderPid, + processGroupId: ownership.processGroupId, + }, + ], + }, + }, + { platform: "win32", terminateTree }, + ); + expect(result).toBe(failure); + expect(terminateTree).toHaveBeenCalledTimes(failedIndex + 1); + }, +); + it("signals a Windows Electron tree only after lineage is verified", async () => { const terminateTree = vi.fn(async () => ({ cleaned: true as const, @@ -157,6 +216,18 @@ it("signals a Windows Electron tree only after lineage is verified", async () => it("retains the partial capture without taskkill when Windows lineage is unavailable", async () => { // Exercise the Windows decision branch; this is not native Windows coverage. + // The injected unavailable observation keeps this independent of host PID reuse. + vi.spyOn( + ownershipObservation, + "observeOwnedProcessLineage", + ).mockResolvedValue({ + status: "unavailable", + observedAt: "2026-10-10T00:00:00.000Z", + runId: "fixture-run", + launcherPid: 424242, + processGroupId: 424242, + reason: "process table permission denied", + }); const originalPlatform = Object.getOwnPropertyDescriptor(process, "platform"); Object.defineProperty(process, "platform", { value: "win32" }); const terminateTree = vi @@ -198,7 +269,7 @@ it("retains the partial capture without taskkill when Windows lineage is unavail reason: "cleanup_failed", cleanup: { reason: - "owned Electron lineage was unavailable; helper cleanup was not proven", + "process table permission denied; helper cleanup was not proven", }, partialObservation: { kind: "electron-active-observation", diff --git a/src/browser/PlaywrightElectronActiveProvider.ts b/src/browser/PlaywrightElectronActiveProvider.ts index 606ffadb50..b5d484aa4b 100644 --- a/src/browser/PlaywrightElectronActiveProvider.ts +++ b/src/browser/PlaywrightElectronActiveProvider.ts @@ -292,14 +292,17 @@ export const cleanupElectronProcesses = async ( terminateTree: cleanupWindowsProcessTree, }, ): Promise => { + if (lineage?.status !== "verified") { + if (host.platform !== "win32") { + const root = await cleanupOwnedProcessGroup(ownership); + if (!root.cleaned) return root; + } + return { + cleaned: false, + reason: `${lineage?.reason ?? "owned Electron lineage was not observed"}; helper cleanup was not proven`, + }; + } if (host.platform === "win32") { - // An empty Windows process table is not ownership proof for this PID. - if (lineage?.status !== "verified") - return { - cleaned: false, - reason: - "owned Electron lineage was unavailable; helper cleanup was not proven", - }; const root = await host.terminateTree(ownership.leaderPid); if (!root.cleaned) return root; for (const descendant of lineage.lineage.descendants) { @@ -308,16 +311,6 @@ export const cleanupElectronProcesses = async ( } return root; } - if (lineage?.status !== "verified") { - const root = await cleanupOwnedProcessGroup(ownership); - return root.cleaned - ? { - cleaned: false, - reason: - "owned Electron lineage was unavailable; helper cleanup was not proven", - } - : root; - } const groupIds = selectCapturedProcessGroupIds( ownership.leaderPid, lineage.lineage.descendants.map(({ pid, processGroupId }) => ({ diff --git a/src/domain/analysisErrorProjection.ts b/src/domain/analysisErrorProjection.ts index d916db49df..026886ed5c 100644 --- a/src/domain/analysisErrorProjection.ts +++ b/src/domain/analysisErrorProjection.ts @@ -110,7 +110,7 @@ const underlyingErrorCode = ( if (error instanceof BrowserObservationError) return error.userCategory === "cancelled" ? "cancelled" - : browserErrorCode(error.reason); + : BROWSER_ERROR_CODES[error.reason]; if (error instanceof ArtifactOperationError) return artifactOperationCode(error); if (error instanceof BinaryTargetError) @@ -126,22 +126,25 @@ const underlyingErrorCode = ( return staticErrorCode(error._tag); }; -const browserErrorCode = ( - reason: BrowserObservationError["reason"], -): AnalysisErrorProjection["code"] => { - if (reason === "cancelled") return "cancelled"; - if (reason === "timeout") return "provider_timeout"; - if (reason === "payload_limit") return "truncated"; - if ( - reason === "target_not_found" || - reason === "target_not_allowed" || - reason === "service_worker_blocked" || - reason === "target_changed" - ) - return "target_unavailable"; - if (reason === "endpoint_unreachable" || reason === "disconnected") - return "provider_unavailable"; - return "unreadable_output"; +const BROWSER_ERROR_CODES: Readonly< + Record +> = { + endpoint_unreachable: "provider_unavailable", + invalid_endpoint_response: "unreadable_output", + target_not_found: "target_unavailable", + target_not_allowed: "target_unavailable", + service_worker_blocked: "target_unavailable", + target_changed: "target_unavailable", + protocol_error: "unreadable_output", + disconnected: "provider_unavailable", + payload_limit: "truncated", + window_not_found: "unreadable_output", + window_metadata_missing: "unreadable_output", + process_ownership_unavailable: "unreadable_output", + cleanup_failed: "execution_failure", + cancelled: "cancelled", + timeout: "provider_timeout", + secret_unavailable: "unreadable_output", }; const artifactOperationCode = ( @@ -439,8 +442,9 @@ const providerErrorDetails = ( const primaryBrowserFailureReason = ( error: BrowserObservationError, ): string | undefined => { - if (!(error.cause instanceof AggregateError)) return undefined; - const primary = error.cause.errors[0]; + const aggregate = error.cause instanceof AggregateError; + const primary = + error.cause instanceof AggregateError ? error.cause.errors[0] : error.cause; if (primary instanceof BrowserObservationError) return primary.reason; if (primary instanceof AnalysisCancelledError) return "cancelled"; if (primary instanceof AnalysisTimeoutError) return "timeout"; @@ -448,7 +452,7 @@ const primaryBrowserFailureReason = ( return typeof primary._tag === "string" ? primary._tag : "execution_failure"; - return "execution_failure"; + return aggregate ? "execution_failure" : undefined; }; const lifecycleErrorDetails = ( diff --git a/tests/conformance/contracts/analysisErrorProjection.test.ts b/tests/conformance/contracts/analysisErrorProjection.test.ts index bb7014dde5..3b9ab734d5 100644 --- a/tests/conformance/contracts/analysisErrorProjection.test.ts +++ b/tests/conformance/contracts/analysisErrorProjection.test.ts @@ -7,9 +7,85 @@ import { EvidenceFileError } from "../../../src/domain/evidenceErrors.js"; import { HopperRemoteError } from "../../../src/domain/hopperErrors.js"; import { UnknownRegistryError } from "../../../src/domain/unknownRegistryError.js"; import { projectAnalysisError } from "../../../src/domain/analysisErrorProjection.js"; +import { BrowserObservationError } from "../../../src/domain/browserObservationError.js"; +import { + AnalysisCancelledError, + AnalysisTimeoutError, +} from "../../../src/domain/analysisErrorCore.js"; import type { AnalysisError } from "../../../src/domain/analysisErrorBase.js"; describe("analysis error projection contract", () => { + it("keeps cleanup failure distinct from unreadable collected output", () => { + const projected = projectAnalysisError( + new BrowserObservationError( + "capture_electron_scenario", + "cleanup_failed", + { + cause: new Error("lineage could not be inspected"), + cleanup: { + reason: "process table permission denied", + resources: ["owned_process_group"], + }, + capturedOutput: { + stdout: "completed observation", + stderr: "", + truncated: false, + }, + }, + ), + ); + expect(projected).toMatchObject({ + code: "cleanup_incomplete", + details: { + cleanup_reason: "process table permission denied", + execution_failure: "execution_failure", + captured_output: { stdout: "completed observation", truncated: false }, + }, + }); + expect(projected.details).not.toHaveProperty("primary_reason"); + expect(analysisErrorProjectionSchema.safeParse(projected).success).toBe( + true, + ); + }); + + it.each([ + [new AnalysisCancelledError("capture_electron_scenario"), "cancelled"], + [new AnalysisTimeoutError("capture_electron_scenario", 10), "timeout"], + [ + new BrowserObservationError("capture_electron_scenario", "disconnected"), + "disconnected", + ], + ] as const)( + "preserves direct and aggregated primary failures: %s", + (primary, reason) => { + for (const cause of [ + primary, + new AggregateError([primary, new Error("close failed")]), + ]) { + const projected = projectAnalysisError( + new BrowserObservationError( + "capture_electron_scenario", + "cleanup_failed", + { + cause, + cleanup: { + reason: "close failed", + resources: ["owned_process_group"], + }, + }, + ), + ); + expect(projected).toMatchObject({ + code: "cleanup_incomplete", + details: { primary_reason: reason, cleanup_reason: "close failed" }, + }); + expect(analysisErrorProjectionSchema.safeParse(projected).success).toBe( + true, + ); + } + }, + ); + it("accepts every closed error-reason variant", () => { const variants: AnalysisError[] = [ ...(