From 3e8236639abffa39deb5d5e939ec8baf6aaf3ca3 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sat, 17 Oct 2026 09:04:09 +0800 Subject: [PATCH] fix(errors): restore typed primary after confirmed cleanup Retain the primary error through cleanup composition and resolve its presentation only after live ownership confirms release. Preserve historical diagnostics separately from producer facts. Validate domain primary types, nested owners, denied retry and late observations; exercise public CLI and MCP projection plus firmware descriptor retry. Focused tests, source controls, static checks and docs pass. Native providers were not verified. Refs: #4628 --- docs/mcp-contracts.md | 3 + src/domain/analysisErrorBase.ts | 15 ++ src/domain/analysisErrorCleanup.test.ts | 172 ++++++++++++++++++ src/domain/analysisErrorCleanup.ts | 1 + src/domain/analysisErrorPresentation.ts | 9 +- src/domain/analysisErrorProjection.ts | 35 +++- .../mcp/cleanupPrimaryProjection.test.ts | 80 ++++++++ .../binaryDiagnosticsCliCleanup.test.ts | 43 +++-- .../firmwareFileLifetime.test.ts | 1 + 9 files changed, 332 insertions(+), 27 deletions(-) create mode 100644 src/domain/analysisErrorCleanup.test.ts create mode 100644 tests/boundary/mcp/cleanupPrimaryProjection.test.ts diff --git a/docs/mcp-contracts.md b/docs/mcp-contracts.md index 677cd44371..c541c46977 100644 --- a/docs/mcp-contracts.md +++ b/docs/mcp-contracts.md @@ -435,6 +435,9 @@ Provider calls receive the request cancellation signal. Artifact traversal, hashing, version comparisons, Hopper requests, and process capture check the same signal. Cancellation is distinct from timeout. A cleanup failure uses `cleanup_incomplete` and lists only the owned resource kinds that remain. +Once those owners confirm cleanup, composed failures recover their original +typed primary outcome and recovery guidance. Historical composition diagnostics +remain in `details.cleanup_history`, separate from the primary diagnostics. An atomic Evidence bundle or snapshot write can publish its complete output before staging cleanup fails. In that case, `details.published: true`, `details.published_path`, and `details.bytes` retain the publication receipt. diff --git a/src/domain/analysisErrorBase.ts b/src/domain/analysisErrorBase.ts index c62ba74117..593c943a60 100644 --- a/src/domain/analysisErrorBase.ts +++ b/src/domain/analysisErrorBase.ts @@ -53,6 +53,8 @@ export interface AnalysisCleanupObservation { /** Optional provider-neutral context that must survive typed error projection. */ export interface AnalysisErrorOptions extends ErrorOptions { + /** Typed primary retained when a later lifecycle failure composes this error. */ + readonly cleanupPrimary?: AnalysisError | undefined; /** Producer-owned, caller-safe facts retained through every typed projection. */ readonly diagnostics?: Readonly> | undefined; readonly capturedOutput?: AnalysisCapturedOutput | undefined; @@ -67,6 +69,7 @@ export interface AnalysisCleanupResourceOwner { /** Base class for expected analysis, provider, and session failures. */ export abstract class AnalysisError extends Error { + readonly #cleanupPrimary: AnalysisError | undefined; readonly #cleanupDependencies = new Set(); #cleanupDependents: WeakRef[] = []; readonly diagnostics: Readonly> | undefined; @@ -77,6 +80,7 @@ export abstract class AnalysisError extends Error { #cleanupResourceOwner: AnalysisCleanupResourceOwner | undefined; constructor(message: string, options?: AnalysisErrorOptions) { super(message, options); + this.#cleanupPrimary = options?.cleanupPrimary; this.diagnostics = options?.diagnostics === undefined ? undefined @@ -95,6 +99,10 @@ export abstract class AnalysisError extends Error { readonly userCategory: "cancelled" | undefined = undefined; readonly executionFailure: string | undefined = undefined; partialObservation: AnalysisPartialObservation | undefined; + + get cleanupPrimary(): AnalysisError | undefined { + return this.#cleanupPrimary; + } readonly cleanupReport: ProcessCaptureCleanupReport | undefined = undefined; /** Live dependencies own current resource attribution; cleanup keeps history. */ @@ -184,6 +192,13 @@ export abstract class AnalysisError extends Error { return this; } } + +/** Confirmed cleanup reveals the typed primary; unresolved ownership stays visible. */ +export const resolvedAnalysisError = (error: AnalysisError): AnalysisError => { + while (!error.cleanupIncomplete && error.cleanupPrimary !== undefined) + error = error.cleanupPrimary; + return error; +}; import type { PartialProcessCaptureObservation, ProcessCaptureCleanupReport, diff --git a/src/domain/analysisErrorCleanup.test.ts b/src/domain/analysisErrorCleanup.test.ts new file mode 100644 index 0000000000..481fc77df1 --- /dev/null +++ b/src/domain/analysisErrorCleanup.test.ts @@ -0,0 +1,172 @@ +import { expect, it } from "vitest"; +import { + AnalysisCancelledError, + AnalysisInputError, + AnalysisOutputError, + AnalysisResourceConstraintError, + AnalysisTimeoutError, +} from "./analysisErrorCore.js"; +import { analysisErrorWithCleanupFailure } from "./analysisErrorCleanup.js"; +import { projectAnalysisError } from "./analysisErrorProjection.js"; +import { + analysisErrorCategory, + analysisErrorRemediationAction, + analysisErrorUserMessage, +} from "./analysisErrorPresentation.js"; +import { ProviderCleanupError } from "./providerCleanupError.js"; +import { createEvidence } from "./evidence.js"; + +it("keeps primary diagnostics separate from historical cleanup diagnostics with the same keys", () => { + const primary = new AnalysisInputError("inspect", { + diagnostics: { + primary_error: "source observation", + cleanup_error: "another source observation", + }, + }); + const cleanup = new ProviderCleanupError("fixture", ["owned-input"], {}); + const composed = analysisErrorWithCleanupFailure( + primary, + cleanup, + ).bindCleanupDependencies([primary, cleanup]); + cleanup.resolveCleanup(); + const projected = projectAnalysisError(composed); + expect(projected.details?.diagnostics).toEqual(primary.diagnostics); + expect(projected.details?.cleanup_history).toMatchObject({ + primary_error: projectAnalysisError(primary), + cleanup_error: { code: "cleanup_incomplete" }, + }); + expect(composed.cleanupPrimary).toBe(primary); + expect(Object.hasOwn(composed, "cleanupPrimary")).toBe(false); +}); + +it.each([ + new AnalysisInputError("inspect", { diagnostics: { input: "original" } }, [ + { + path: ["path"], + reason: "out_of_range", + message: "fixed input allowance", + }, + ]), + new AnalysisCancelledError("inspect", { cause: "caller reason" }), + new AnalysisTimeoutError("inspect", 1234), + new AnalysisResourceConstraintError( + "inspect", + "file-size", + "fixed input allowance", + { maximum_input_bytes: 123 }, + { retryable: false, remediationAction: "Select a smaller suitable input." }, + ), + new AnalysisOutputError("inspect", "invalid output", { + capturedOutput: { + stdout: "original output", + stderr: "original warning", + truncated: true, + }, + partialObservation: createEvidence( + undefined, + { id: "fixture", name: "Fixture", version: null }, + { + operation: "inspect", + parameters: {}, + result: { original_fact: true }, + confidence: "observed", + locations: [], + limitations: [], + }, + ), + }), +])("restores the typed $_tag primary after confirmed cleanup", (primary) => { + const original = projectAnalysisError(primary); + const cleanup = new ProviderCleanupError( + "fixture", + ["owned-input"], + { reason: "close denied" }, + { operation: "inspect" }, + ); + const composed = analysisErrorWithCleanupFailure( + primary, + cleanup, + "inspect", + ).bindCleanupDependencies([primary, cleanup]); + expect(projectAnalysisError(composed).code).toBe("cleanup_incomplete"); + cleanup.resolveCleanup(); + const projected = projectAnalysisError(composed); + expect(projected).toMatchObject(original); + expect(projected.details?.cleanup_history).toMatchObject({ + primary_error: original, + cleanup_error: { code: "cleanup_incomplete" }, + }); + expect(projected.details).not.toHaveProperty("cleanup", "incomplete"); + expect(projected.details).not.toHaveProperty("resources"); + expect(analysisErrorCategory(composed)).toBe(original.category); + expect(analysisErrorUserMessage(composed)).toBe(original.message); + expect(analysisErrorRemediationAction(composed)).toBe( + original.remediation.action, + ); + expect(composed.cause).toBeInstanceOf(AggregateError); + expect((composed.cause as AggregateError).errors[0]).toBe(primary); +}); + +it.each(["inner", "outer"] as const)( + "waits for both nested cleanup owners when %s resolves first", + (first) => { + const primary = new AnalysisInputError("inspect"); + const innerCleanup = new ProviderCleanupError("fixture", ["input"], {}); + const outerCleanup = new ProviderCleanupError("fixture", ["workspace"], {}); + const inner = analysisErrorWithCleanupFailure( + primary, + innerCleanup, + ).bindCleanupDependencies([primary, innerCleanup]); + const outer = analysisErrorWithCleanupFailure( + inner, + outerCleanup, + ).bindCleanupDependencies([inner, outerCleanup]); + const released = first === "inner" ? innerCleanup : outerCleanup; + const pending = first === "inner" ? outerCleanup : innerCleanup; + released.resolveCleanup(); + expect(outer.cleanupIncomplete).toBe(true); + expect(projectAnalysisError(outer).code).toBe("cleanup_incomplete"); + pending.resolveCleanup(); + expect(outer.cleanupIncomplete).toBe(false); + expect(projectAnalysisError(outer)).toMatchObject( + projectAnalysisError(primary), + ); + }, +); + +it("does not infer cleanup success when a composition has no live owner binding", () => { + const primary = new AnalysisInputError("inspect"); + const cleanup = new ProviderCleanupError("fixture", ["input"], {}); + const composed = analysisErrorWithCleanupFailure(primary, cleanup); + cleanup.resolveCleanup(); + expect(projectAnalysisError(composed).code).toBe("cleanup_incomplete"); +}); + +it("retains an observation attached to the composite after the primary failed", () => { + const primary = new AnalysisInputError("inspect"); + const cleanup = new ProviderCleanupError("fixture", ["input"], {}); + const composed = analysisErrorWithCleanupFailure( + primary, + cleanup, + ).bindCleanupDependencies([primary, cleanup]); + const observation = createEvidence( + undefined, + { id: "fixture", name: "Fixture", version: null }, + { + operation: "inspect", + parameters: {}, + result: { late_fact: true }, + confidence: "observed", + locations: [], + limitations: [], + }, + ); + composed.retainPartialObservation(observation); + composed.resolveCleanup(); + expect(projectAnalysisError(composed).code).toBe("cleanup_incomplete"); + cleanup.resolveCleanup(); + expect(projectAnalysisError(composed)).toMatchObject({ + code: "invalid_request", + details: { partial_observation: observation }, + }); +}); diff --git a/src/domain/analysisErrorCleanup.ts b/src/domain/analysisErrorCleanup.ts index 3dc8ed6f76..ab7b6d43ea 100644 --- a/src/domain/analysisErrorCleanup.ts +++ b/src/domain/analysisErrorCleanup.ts @@ -13,6 +13,7 @@ export const analysisErrorWithCleanupFailure = ( cleanup instanceof ProviderAdapterError ? cleanup.providerId : "rea", operation, { + cleanupPrimary: primary, cause: new AggregateError( [primary, cleanup], "Analysis failed and cleanup remains incomplete.", diff --git a/src/domain/analysisErrorPresentation.ts b/src/domain/analysisErrorPresentation.ts index 410dfae2a4..75a2d0c285 100644 --- a/src/domain/analysisErrorPresentation.ts +++ b/src/domain/analysisErrorPresentation.ts @@ -31,12 +31,17 @@ import { ProviderSelectionError } from "./providerSelectionError.js"; import { ProviderAdapterError } from "./providerAdapterError.js"; import { UnknownRegistryError } from "./unknownRegistryError.js"; import { providerRetryAction } from "./providerOperationHealth.js"; -import type { AnalysisError, AnalysisErrorTag } from "./analysisErrorBase.js"; +import { + resolvedAnalysisError, + type AnalysisError, + type AnalysisErrorTag, +} from "./analysisErrorBase.js"; import type { AnalysisErrorProjection } from "./analysisErrorProjection.js"; export const analysisErrorRemediationAction = ( error: AnalysisError, ): string => { + error = resolvedAnalysisError(error); if ( error instanceof ProviderAdapterError && error.remediationAction !== undefined @@ -138,6 +143,7 @@ export const analysisErrorRemediationAction = ( export const analysisErrorCategory = ( error: AnalysisError, ): AnalysisErrorProjection["category"] => { + error = resolvedAnalysisError(error); if (error instanceof AnalysisCapabilityUnavailableError && error.retryable) return "unavailable"; if ( @@ -234,6 +240,7 @@ const STATIC_ERROR_CATEGORIES: Readonly< }; export const analysisErrorUserMessage = (error: AnalysisError): string => { + error = resolvedAnalysisError(error); if (error instanceof AnalysisSnapshotMismatchError) return error.message; if (error instanceof AnalysisUnsupportedTargetError) return error.message; if (error instanceof AnalysisResourceConstraintError) return error.reason; diff --git a/src/domain/analysisErrorProjection.ts b/src/domain/analysisErrorProjection.ts index a89f1b4138..f0d94b8815 100644 --- a/src/domain/analysisErrorProjection.ts +++ b/src/domain/analysisErrorProjection.ts @@ -40,21 +40,32 @@ import { ProviderAdapterError } from "./providerAdapterError.js"; import { ProviderSelectionError } from "./providerSelectionError.js"; import { UnknownRegistryError } from "./unknownRegistryError.js"; import { providerRetryAction } from "./providerOperationHealth.js"; -import type { AnalysisError, AnalysisErrorTag } from "./analysisErrorBase.js"; +import { + resolvedAnalysisError, + type AnalysisError, + type AnalysisErrorTag, +} from "./analysisErrorBase.js"; /** Project expected failures into exhaustive, secret-safe caller fields. */ export const projectAnalysisError = ( error: AnalysisError, ): AnalysisErrorProjection => { + const composed = error; + error = resolvedAnalysisError(error); + const diagnostics = error.diagnostics; + const capturedOutput = composed.capturedOutput ?? error.capturedOutput; + const partialObservation = + composed.partialObservation ?? error.partialObservation; assertKnownAnalysisErrorTag(error._tag); const underlyingCode = underlyingErrorCode(error); const code = error.cleanupIncomplete ? "cleanup_incomplete" : underlyingCode; const primaryDetails = errorDetails(error); const details = { ...primaryDetails, - ...(error.diagnostics === undefined + ...(diagnostics === undefined ? {} : { diagnostics }), + ...(composed === error || composed.diagnostics === undefined ? {} - : { diagnostics: error.diagnostics }), + : { cleanup_history: composed.diagnostics }), ...(error.cleanupIncomplete ? { cleanup: "incomplete", @@ -66,15 +77,19 @@ export const projectAnalysisError = ( primaryDetails?.execution_failure ?? underlyingCode, } : {}), - ...(error.capturedOutput === undefined - ? {} - : { captured_output: { ...error.capturedOutput } }), - ...(error.partialObservation === undefined + ...(capturedOutput === undefined ? {} : { - partial_observation: isImmutableJsonSnapshot(error.partialObservation) - ? error.partialObservation - : jsonValueSchema.parse(error.partialObservation), + captured_output: { + ...capturedOutput, + }, + }), + ...(partialObservation === undefined + ? {} + : { + partial_observation: isImmutableJsonSnapshot(partialObservation) + ? partialObservation + : jsonValueSchema.parse(partialObservation), }), }; return { diff --git a/tests/boundary/mcp/cleanupPrimaryProjection.test.ts b/tests/boundary/mcp/cleanupPrimaryProjection.test.ts new file mode 100644 index 0000000000..a6113bdc09 --- /dev/null +++ b/tests/boundary/mcp/cleanupPrimaryProjection.test.ts @@ -0,0 +1,80 @@ +import { expect, it, onTestFinished } from "vitest"; +import { AnalysisCleanupLease } from "../../../src/artifacts/AnalysisCleanupLease.js"; +import { AnalysisInputError } from "../../../src/domain/analysisErrorCore.js"; +import { analysisErrorWithCleanupFailure } from "../../../src/domain/analysisErrorCleanup.js"; +import { ProviderCleanupError } from "../../../src/domain/providerCleanupError.js"; +import { err } from "../../../src/domain/result.js"; +import { createApplicationMcpHarness } from "../../fixtures/applicationMcpHarness.js"; +import { advertisedMcpToolSchemas } from "../../fixtures/advertisedMcpTool.js"; +import { parseMcpToolError } from "../../fixtures/mcpToolError.js"; + +it("restores the primary error after exact cleanup retry through the same MCP connection", async () => { + const primary = new AnalysisInputError("inspect_adb_window", undefined, [ + { + path: ["serial"], + reason: "invalid_value", + message: "fixture input refused", + }, + ]); + const cleanup = new ProviderCleanupError( + "fixture", + ["selected-input"], + { reason: "close denied" }, + { operation: "inspect_adb_window" }, + ); + let permitRelease = false; + const lease = new AnalysisCleanupLease(async () => { + if (!permitRelease) throw new Error("close denied"); + }, ["selected-input"]); + lease.bindCleanupError(cleanup); + const composed = analysisErrorWithCleanupFailure( + primary, + cleanup, + "inspect_adb_window", + ).bindCleanupDependencies([primary, cleanup]); + const harness = await createApplicationMcpHarness({ + serverOptions: { + adbDeviceAnalysis: { + inspectAvailability: async () => ({ + status: "available", + code: null, + reason: null, + diagnostics: {}, + }), + execute: async () => err(composed), + close: async () => { + permitRelease = true; + await lease.release(); + }, + }, + }, + }); + onTestFinished(() => harness.close()); + const { name, ajv, inputSchema } = await advertisedMcpToolSchemas( + harness.client, + "inspect_adb_window", + ); + const args = { serial: "fixture" }; + expect(ajv.validate(inputSchema, args)).toBe(true); + const call = async () => + parseMcpToolError(await harness.client.callTool({ name, arguments: args })) + .error; + expect((await call()).code).toBe("cleanup_incomplete"); + await expect(lease.release()).rejects.toThrow("close denied"); + expect((await call()).code).toBe("cleanup_incomplete"); + permitRelease = true; + await lease.release(); + expect(await call()).toMatchObject({ + code: "invalid_request", + category: "invalid_input", + retryable: true, + details: { + issues: [{ path: ["serial"], reason: "invalid_value" }], + cleanup_history: { + primary_error: { code: "invalid_request" }, + cleanup_error: { code: "cleanup_incomplete" }, + }, + }, + }); + await harness.client.ping(); +}); diff --git a/tests/process-global/binaryDiagnosticsCliCleanup.test.ts b/tests/process-global/binaryDiagnosticsCliCleanup.test.ts index d23ef2a493..29a5c1d23d 100644 --- a/tests/process-global/binaryDiagnosticsCliCleanup.test.ts +++ b/tests/process-global/binaryDiagnosticsCliCleanup.test.ts @@ -16,9 +16,14 @@ afterEach(() => { process.exitCode = exitCode; }); -it.each(["layout", "recorded"] as const)( - "projects the final owned outcome through the public CLI: %s", - async (scenario) => { +it.each( + (["layout", "recorded"] as const).flatMap((scenario) => [ + { scenario, recovered: false }, + { scenario, recovered: true }, + ]), +)( + "projects the final owned outcome through the public CLI: $scenario recovered=$recovered", + async ({ scenario, recovered }) => { const primary = new AnalysisOutputError( "inspect_binary_layout", "fixture decoder output overflow", @@ -35,15 +40,13 @@ it.each(["layout", "recorded"] as const)( ["owned-process", "/retained/runtime"], { reason: "fixture process inspection still denied" }, ); - const inspectAndClose = vi.fn(async () => - err( - analysisErrorWithCleanupFailure( - primary, - cleanup, - "inspect_binary_layout", - ), - ), - ); + const composed = analysisErrorWithCleanupFailure( + primary, + cleanup, + "inspect_binary_layout", + ).bindCleanupDependencies([primary, cleanup]); + if (recovered) cleanup.resolveCleanup(); + const inspectAndClose = vi.fn(async () => err(composed)); const identity = { id: "fixture-decoder", name: "Fixture decoder", @@ -90,21 +93,29 @@ it.each(["layout", "recorded"] as const)( ); const output: unknown = JSON.parse(stdout); expect(output).toMatchObject({ - code: "cleanup_incomplete", + code: recovered ? "unreadable_output" : "cleanup_incomplete", details: { - cleanup: "incomplete", - resources: ["owned-process", "/retained/runtime"], + ...(recovered + ? {} + : { + cleanup: "incomplete", + resources: ["owned-process", "/retained/runtime"], + }), captured_output: { stdout: "partial original output", stderr: "warning", truncated: true, }, - diagnostics: { + [recovered ? "cleanup_history" : "diagnostics"]: { primary_error: { code: "unreadable_output" }, cleanup_error: { code: "cleanup_incomplete" }, }, }, }); + if (recovered) { + expect(output).not.toHaveProperty("details.cleanup"); + expect(output).not.toHaveProperty("details.resources"); + } expect(inspectAndClose).toHaveBeenCalledOnce(); expect(process.exitCode).toBe(1); }, diff --git a/tests/process-global/firmwareFileLifetime.test.ts b/tests/process-global/firmwareFileLifetime.test.ts index 6e4f783bc9..00c3e53cdb 100644 --- a/tests/process-global/firmwareFileLifetime.test.ts +++ b/tests/process-global/firmwareFileLifetime.test.ts @@ -310,6 +310,7 @@ it.each( expect(error.diagnostics?.primary_error).toMatchObject({ code: "invalid_request", }); + expect(projectAnalysisError(error).code).toBe("invalid_request"); } expect(fault.handles[0]?.fd).toBe(-1); } finally {