diff --git a/src/errors/errors.test.tsx b/src/errors/errors.test.tsx index 090bcd4fe..1cf7a8717 100644 --- a/src/errors/errors.test.tsx +++ b/src/errors/errors.test.tsx @@ -4,7 +4,13 @@ import { ValidationException, InternalServerException, } from "@aws-sdk/client-bedrock-agentcore-control"; -import { AgentCoreCLIError, InputValidationError } from "./errors"; +import { CommanderError } from "commander"; +import { + AgentCoreCLIError, + InputValidationError, + SilentCLIError, + UserCancellationError, +} from "./errors"; describe("AgentCoreCLIError", () => { test("fromError preserves existing AgentCoreCLIError instances", () => { @@ -17,6 +23,31 @@ describe("AgentCoreCLIError", () => { expect(AgentCoreCLIError.fromError(err)).toBe(err); }); + test.each([ + ["parse failures", new CommanderError(1, "commander.invalidArgument", "invalid option"), 2], + ["help", new CommanderError(0, "commander.helpDisplayed", "help displayed"), 0], + ])("fromError classifies Commander %s", (_label, err, exitCode) => { + const result = AgentCoreCLIError.fromError(err); + expect(result).toBeInstanceOf(SilentCLIError); + expect(result.json()).toMatchObject({ + name: "CommanderError", + source: "user", + exitCode, + meta: { code: err.code }, + }); + }); + + test("UserCancellationError is a silent user interruption", () => { + const error = new UserCancellationError(); + expect(error).toBeInstanceOf(SilentCLIError); + expect(error.json()).toMatchObject({ + name: "UserCancellationError", + message: "Operation cancelled by user", + source: "user", + exitCode: 130, + }); + }); + test.each([ [ "AccessDeniedException (403)", diff --git a/src/errors/errors.tsx b/src/errors/errors.tsx index 27c6edc69..7cadbd738 100644 --- a/src/errors/errors.tsx +++ b/src/errors/errors.tsx @@ -1,4 +1,5 @@ import { ServiceException } from "@smithy/core/client"; +import { CommanderError } from "commander"; import { join } from "node:path"; import { ERROR_SOURCE, type ErrorSource } from "./types"; @@ -41,6 +42,16 @@ export class AgentCoreCLIError extends Error { static fromError(error: unknown): AgentCoreCLIError { if (error instanceof AgentCoreCLIError) return error; + if (error instanceof CommanderError) { + return new SilentCLIError(error.message, { + cause: error, + source: ERROR_SOURCE.USER, + name: error.name, + meta: { code: error.code }, + exitCode: error.exitCode === 0 ? 0 : 2, + }); + } + if (ServiceException.isInstance(error)) { const httpStatusCode = error.$metadata.httpStatusCode; const source = @@ -62,6 +73,9 @@ export class AgentCoreCLIError extends Error { } } +/** Base for CLI errors intentionally omitted from root stderr output. */ +export class SilentCLIError extends AgentCoreCLIError {} + /** Error raised for invalid user input. */ export class InputValidationError extends AgentCoreCLIError { constructor(message?: string, options?: Omit) { @@ -135,19 +149,17 @@ export class EmbeddedAssetNotFoundError extends AgentCoreCLIError { } } -export class RuntimeInvokeInterruptedError extends AgentCoreCLIError { - readonly reported: boolean; - - constructor(cause?: unknown, reported = false) { - super("The operation was aborted", { cause, exitCode: 130 }); - this.name = "AbortError"; - this.reported = reported; +/** Raised when a user intentionally cancels a headless CLI operation. */ +export class UserCancellationError extends SilentCLIError { + constructor() { + super("Operation cancelled by user", { + source: ERROR_SOURCE.USER, + exitCode: 130, + }); } } -export class RuntimeInvokeResponseError extends AgentCoreCLIError { - readonly reported = true; - +export class RuntimeInvokeResponseError extends SilentCLIError { constructor(message: string, cause?: unknown) { super(message, { cause }); } diff --git a/src/errors/index.tsx b/src/errors/index.tsx index ed44888ab..33459172d 100644 --- a/src/errors/index.tsx +++ b/src/errors/index.tsx @@ -10,9 +10,10 @@ export { NotImplementedError, ProjectFileExistsError, ResultTruncationError, - RuntimeInvokeInterruptedError, RuntimeInvokeResponseError, + SilentCLIError, SourceResolutionError, + UserCancellationError, type AgentCoreCLIErrorOptions, } from "./errors"; export { ERROR_SOURCE } from "./types"; diff --git a/src/handlers/eval/dataset/dataset.test.tsx b/src/handlers/eval/dataset/dataset.test.tsx index ba174ec61..838ef088e 100644 --- a/src/handlers/eval/dataset/dataset.test.tsx +++ b/src/handlers/eval/dataset/dataset.test.tsx @@ -8,7 +8,9 @@ import { TestCoreClient, TestGlobalConfigAccessor, testIO, + waitFor, } from "../../../testing"; +import { UserCancellationError } from "../../../errors"; import { createRootHandler } from "../../index"; import type { CreateDatasetInput } from "../types"; @@ -421,6 +423,41 @@ describe("dataset get", () => { expect(call?.args.slice(0, 3)).toEqual(["dataset-orders-abc123", "2", "/tmp/v2.jsonl"]); }); + test("SIGINT cancels a download with the shared user cancellation error", async () => { + const { core, route } = testDatasetCommand(); + core.eval.downloadDataset = async (id, version, filePath, options, signal) => { + core.eval.calls.push({ + method: "downloadDataset", + args: [id, version, filePath, options, signal], + }); + return new Promise((_, reject) => { + const abort = () => reject(signal?.reason); + if (signal?.aborted) abort(); + else signal?.addEventListener("abort", abort, { once: true }); + }); + }; + const pending = route([ + "eval", + "dataset", + "get", + "--id", + "dataset-orders-abc123", + "--file-path", + "/tmp/out.jsonl", + ]); + + try { + await waitFor(() => core.eval.calls.some((call) => call.method === "downloadDataset")); + process.emit("SIGINT", "SIGINT"); + + const signal = core.eval.calls[0]!.args[4] as AbortSignal; + expect(signal.reason).toBeInstanceOf(UserCancellationError); + await expect(pending).rejects.toBe(signal.reason); + } finally { + await pending.catch(() => undefined); + } + }); + test("requires --id", async () => { const { core, route } = testDatasetCommand(); diff --git a/src/handlers/eval/dataset/get/index.tsx b/src/handlers/eval/dataset/get/index.tsx index 4c2e11c81..8b88a95d9 100644 --- a/src/handlers/eval/dataset/get/index.tsx +++ b/src/handlers/eval/dataset/get/index.tsx @@ -1,6 +1,6 @@ import z from "zod"; import { createHandler, flag } from "../../../../router"; -import { InputValidationError } from "../../../../errors"; +import { InputValidationError, UserCancellationError } from "../../../../errors"; import { JsonRendererKey } from "../../../../tui"; import type { Core } from "../../../types"; import { coreOptsFromCtx } from "../../../utils"; @@ -37,7 +37,7 @@ export const createGetDatasetHandler = (core: Core) => // --file-path downloads the contents via the presigned download URL in metadata const controller = new AbortController(); - const interrupt = () => controller.abort(); + const interrupt = () => controller.abort(new UserCancellationError()); process.once("SIGINT", interrupt); try { const response = await core.eval.downloadDataset( diff --git a/src/handlers/runtime/invoke/index.tsx b/src/handlers/runtime/invoke/index.tsx index 0110273a3..463403208 100644 --- a/src/handlers/runtime/invoke/index.tsx +++ b/src/handlers/runtime/invoke/index.tsx @@ -2,7 +2,7 @@ import z from "zod"; import { InputValidationError, InvalidEnvironmentError, - RuntimeInvokeInterruptedError, + UserCancellationError, } from "../../../errors"; import { createHandler, flag, PathKey } from "../../../router"; import type { AppIO } from "../../../io"; @@ -119,7 +119,7 @@ export const createInvokeRuntimeHandler = (core: Core, io: AppIO) => throw new InputValidationError("--json cannot be used with --output-file"); } const controller = new AbortController(); - const interrupt = () => controller.abort(); + const interrupt = () => controller.abort(new UserCancellationError()); process.once("SIGINT", interrupt); try { const applicationHeaders = parseRuntimeInvokeHeaders(flags.header); @@ -158,10 +158,7 @@ export const createInvokeRuntimeHandler = (core: Core, io: AppIO) => signal: controller.signal, }); } catch (error) { - if (controller.signal.aborted && (error as Error)?.name === "AbortError") { - if (error instanceof RuntimeInvokeInterruptedError) throw error; - throw new RuntimeInvokeInterruptedError(error); - } + controller.signal.throwIfAborted(); throw error; } finally { controller.abort(); diff --git a/src/handlers/runtime/invoke/invoke.test.tsx b/src/handlers/runtime/invoke/invoke.test.tsx index fa4d4859d..d9007e06a 100644 --- a/src/handlers/runtime/invoke/invoke.test.tsx +++ b/src/handlers/runtime/invoke/invoke.test.tsx @@ -12,7 +12,7 @@ import { waitFor, } from "../../../testing"; import { ExitCode, runWithExitCode } from "../../../runnable"; -import { InvalidEnvironmentError } from "../../../errors"; +import { InvalidEnvironmentError, UserCancellationError } from "../../../errors"; import { createRootHandler } from "../../index"; import * as tui from "../../../tui"; import { RuntimeInvokeLaunchContextKey } from "./launchContext"; @@ -247,6 +247,64 @@ describe("runtime invoke", () => { expect((invoke.args[0] as RuntimeInvokeRequest).payload).toEqual(new Uint8Array()); }); + test("SIGINT cancels payload stdin resolution with the typed reason", async () => { + const core = new TestCoreClient(); + const output = captureIO(); + const initialListeners = process.listenerCount("SIGINT"); + const pending = runCommand(core, output.io, [ + "runtime", + "invoke", + "--id", + RUNTIME_ID, + "--payload", + "-", + ]); + + try { + await waitFor(() => process.listenerCount("SIGINT") > initialListeners); + process.emit("SIGINT", "SIGINT"); + + await expect(pending).rejects.toBeInstanceOf(UserCancellationError); + expect(core.runtime.calls).toEqual([]); + } finally { + await pending.catch(() => undefined); + } + }); + + test("SIGINT replaces a raw Runtime lookup abort with the typed reason", async () => { + const core = new TestCoreClient(); + const output = captureIO(); + core.runtime.getRuntime = async (id, options, signal) => { + core.runtime.calls.push({ method: "getRuntime", args: [id, options, signal] }); + return new Promise((_, reject) => { + const abort = () => + reject(Object.assign(new Error("lookup aborted"), { name: "AbortError" })); + if (signal?.aborted) abort(); + else signal?.addEventListener("abort", abort, { once: true }); + }); + }; + const pending = runCommand(core, output.io, [ + "runtime", + "invoke", + "--id", + RUNTIME_ID, + "--payload", + "{}", + ]); + + try { + await waitFor(() => core.runtime.calls.some((call) => call.method === "getRuntime")); + process.emit("SIGINT", "SIGINT"); + + const signal = core.runtime.calls[0]!.args[2] as AbortSignal; + expect(signal.reason).toBeInstanceOf(UserCancellationError); + await expect(pending).rejects.toBe(signal.reason); + expect(core.runtime.calls.map((call) => call.method)).toEqual(["getRuntime"]); + } finally { + await pending.catch(() => undefined); + } + }); + test("SIGINT aborts an active headless invocation after preserving emitted bytes", async () => { const core = new TestCoreClient(); const output = captureIO(); @@ -285,14 +343,14 @@ describe("runtime invoke", () => { process.emit("SIGINT", "SIGINT"); expect(signal!.aborted).toBe(true); - await expect(pending).rejects.toMatchObject({ name: "AbortError" }); + await expect(pending).rejects.toBeInstanceOf(UserCancellationError); expect(output.bytes().toString()).toBe("partial"); } finally { await pending.catch(() => undefined); } }); - test("wraps a raw Core abort after SIGINT", async () => { + test("replaces a raw Core abort with the typed SIGINT reason", async () => { const core = new TestCoreClient(); const output = captureIO(); const rawAbort = Object.assign(new Error("transport aborted"), { name: "AbortError" }); @@ -318,11 +376,10 @@ describe("runtime invoke", () => { await waitFor(() => core.runtime.calls.some((call) => call.method === "invokeRuntime")); process.emit("SIGINT", "SIGINT"); - await expect(pending).rejects.toMatchObject({ - name: "AbortError", - cause: rawAbort, - reported: false, - }); + const signal = core.runtime.calls.find((call) => call.method === "invokeRuntime")! + .args[2] as AbortSignal; + expect(signal.reason).toBeInstanceOf(UserCancellationError); + await expect(pending).rejects.toBe(signal.reason); } finally { await pending.catch(() => undefined); } diff --git a/src/handlers/runtime/invoke/response.test.ts b/src/handlers/runtime/invoke/response.test.ts index d918b2350..8b2850ca5 100644 --- a/src/handlers/runtime/invoke/response.test.ts +++ b/src/handlers/runtime/invoke/response.test.ts @@ -3,6 +3,7 @@ import { rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { PassThrough, Writable } from "node:stream"; +import { SilentCLIError, UserCancellationError } from "../../../errors"; import { waitFor } from "../../../testing"; import type { RuntimeInvokeResponse } from "../types"; import { writeRuntimeInvokeResponse } from "./response"; @@ -121,15 +122,13 @@ describe("Runtime invoke response output", () => { }, }) as unknown as NodeJS.WriteStream; - await expect( - writeRuntimeInvokeResponse(response(), { - stdout: stdout.stream, - stderr, - }), - ).rejects.toMatchObject({ - message: "response stream failed", - reported: true, + const pending = writeRuntimeInvokeResponse(response(), { + stdout: stdout.stream, + stderr, }); + + await expect(pending).rejects.toBeInstanceOf(SilentCLIError); + await expect(pending).rejects.toThrow("response stream failed"); }); test("streams exact bytes to a file and leaves stdout empty", async () => { @@ -266,11 +265,12 @@ describe("Runtime invoke response output", () => { test("writes an interruption summary after the output signal is aborted", async () => { const controller = new AbortController(); + const cancellation = new UserCancellationError(); const stdout = capture(); const stderr = capture(); const source = (async function* () { yield Buffer.from("partial"); - controller.abort(); + controller.abort(cancellation); throw Object.assign(new Error("The operation was aborted"), { name: "AbortError" }); })(); @@ -280,7 +280,7 @@ describe("Runtime invoke response output", () => { stderr: stderr.stream, signal: controller.signal, }), - ).rejects.toMatchObject({ name: "AbortError" }); + ).rejects.toBe(cancellation); expect(stdout.bytes().toString()).toBe("partial"); expect(stderr.bytes().toString()).toBe( "status=200 content-type=text/event-stream runtime-session-id=- mcp-session-id=- " + @@ -289,6 +289,33 @@ describe("Runtime invoke response output", () => { ); }); + test("JSON cancellation emits no partial envelope and preserves the typed reason", async () => { + const controller = new AbortController(); + const cancellation = new UserCancellationError(); + const stdout = capture(); + const stderr = capture(); + const source = (async function* () { + yield Buffer.from("partial"); + controller.abort(cancellation); + throw Object.assign(new Error("The operation was aborted"), { name: "AbortError" }); + })(); + + await expect( + writeRuntimeInvokeResponse(response({ body: source }), { + stdout: stdout.stream, + stderr: stderr.stream, + json: true, + signal: controller.signal, + }), + ).rejects.toBe(cancellation); + expect(stdout.bytes()).toHaveLength(0); + expect(stderr.bytes().toString()).toBe( + "status=200 content-type=text/plain runtime-session-id=- mcp-session-id=- " + + "mcp-protocol-version=- trace-id=- trace-parent=- trace-state=- baggage=- " + + "complete=false bytes=7 error=interrupted\n", + ); + }); + test("preserves partial raw output regardless of response media type", async () => { const stdout = capture(); const stderr = capture(); diff --git a/src/handlers/runtime/invoke/response.ts b/src/handlers/runtime/invoke/response.ts index d28af16f6..70ff548ac 100644 --- a/src/handlers/runtime/invoke/response.ts +++ b/src/handlers/runtime/invoke/response.ts @@ -1,6 +1,6 @@ import { createWriteStream } from "node:fs"; import { pipeline } from "node:stream/promises"; -import { RuntimeInvokeInterruptedError, RuntimeInvokeResponseError } from "../../../errors"; +import { RuntimeInvokeResponseError, UserCancellationError } from "../../../errors"; import type { RuntimeInvokeResponse } from "../types"; interface RuntimeInvokeOutput { @@ -49,7 +49,7 @@ async function writeChunk( try { await pipeline([chunk], stream, { end: false, signal }); } catch (error) { - failure(error); + failure(error, signal); } } @@ -68,9 +68,14 @@ export async function writeRuntimeInvokeFile( ); } -function failure(error: unknown): never { - const interrupted = (error as Error)?.name === "AbortError"; - if (interrupted) throw new RuntimeInvokeInterruptedError(error, true); +function userCancellation(error: unknown, signal?: AbortSignal): UserCancellationError | undefined { + if (error instanceof UserCancellationError) return error; + return signal?.reason instanceof UserCancellationError ? signal.reason : undefined; +} + +function failure(error: unknown, signal?: AbortSignal): never { + const cancellation = userCancellation(error, signal); + if (cancellation) throw cancellation; throw new RuntimeInvokeResponseError(RESPONSE_STREAM_FAILED, error); } @@ -172,10 +177,10 @@ export async function writeRuntimeInvokeResponse( response, byteCount, false, - (error as Error)?.name === "AbortError" ? "interrupted" : "response-stream-failed", + userCancellation(error, output.signal) ? "interrupted" : "response-stream-failed", ), ); - failure(error); + failure(error, output.signal); } if (!output.json) { await writeChunk(output.stderr, summary(response, byteCount, true)); diff --git a/src/index.ts b/src/index.ts index aa2834b1b..f626f066d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -91,12 +91,14 @@ process.exit( await rootHandler.route(argv, context); } catch (e) { const error = AgentCoreCLIError.fromError(e); - rootLogger.child({ error: error.json() }).error(); - commandRunMetricEvent.setAttributes({ - exit_reason: "failure", - error_name: error.name, - error_source: error.source, - }); + if (error.exitCode !== 0) { + rootLogger.child({ error: error.json() }).error(); + commandRunMetricEvent.setAttributes({ + exit_reason: "failure", + error_name: error.name, + error_source: error.source, + }); + } throw error; } finally { try { diff --git a/src/runnable/index.test.ts b/src/runnable/index.test.ts index acf43b090..d8c781e0a 100644 --- a/src/runnable/index.test.ts +++ b/src/runnable/index.test.ts @@ -1,7 +1,12 @@ import { expect, spyOn, test } from "bun:test"; import { CommanderError } from "commander"; -import { AgentCoreCLIError, InputValidationError } from "../errors"; +import { + AgentCoreCLIError, + InputValidationError, + SilentCLIError, + UserCancellationError, +} from "../errors"; import { ExitCode, runRunnable, runWithExitCode, type Runnable } from "./index.tsx"; async function captureErrors(run: () => Promise) { @@ -75,19 +80,12 @@ test.each([ ExitCode.USAGE, ["Error: bad request"], ], + ["user cancellation", new UserCancellationError(), ExitCode.INTERRUPTED, []], [ - "interruption", + "raw AbortError", Object.assign(new Error("The operation was aborted"), { name: "AbortError" }), - ExitCode.INTERRUPTED, - ["AbortError: The operation was aborted"], - ], - [ - "classified interruption", - AgentCoreCLIError.fromError( - Object.assign(new Error("The operation was aborted"), { name: "AbortError" }), - ), - ExitCode.INTERRUPTED, - ["AbortError: The operation was aborted"], + ExitCode.FAILURE, + ["Error: The operation was aborted"], ], [ "Commander parse failure", @@ -95,43 +93,18 @@ test.each([ ExitCode.USAGE, [], ], - [ - "classified Commander parse failure", - AgentCoreCLIError.fromError( - new CommanderError(1, "commander.invalidArgument", "invalid option"), - ), - ExitCode.USAGE, - [], - ], [ "Commander help", new CommanderError(0, "commander.helpDisplayed", "help displayed"), ExitCode.SUCCESS, [], ], - [ - "classified Commander help", - AgentCoreCLIError.fromError(new CommanderError(0, "commander.helpDisplayed", "help displayed")), - ExitCode.SUCCESS, - [], - ], - [ - "classified reported failure", - AgentCoreCLIError.fromError(Object.assign(new Error("already reported"), { reported: true })), - ExitCode.FAILURE, - [], - ], - [ - "reported failure", - Object.assign(new Error("already reported"), { reported: true }), - ExitCode.FAILURE, - [], - ], + ["hidden failure", new SilentCLIError("already displayed"), ExitCode.FAILURE, []], [ "arbitrary TypeError", new TypeError("transport failed"), ExitCode.FAILURE, - ["TypeError: transport failed"], + ["Error: transport failed"], ], ])("runWithExitCode maps %s", async (_name, error, expected, expectedErrors) => { const result = await captureErrors(() => runWithExitCode(async () => Promise.reject(error))); diff --git a/src/runnable/index.tsx b/src/runnable/index.tsx index 5cfc399f2..c2df8aaf3 100644 --- a/src/runnable/index.tsx +++ b/src/runnable/index.tsx @@ -1,5 +1,4 @@ -import { CommanderError } from "commander"; -import { AgentCoreCLIError } from "../errors"; +import { AgentCoreCLIError, SilentCLIError } from "../errors"; // ExitCode provides names for default Unix exit codes. export enum ExitCode { @@ -9,21 +8,6 @@ export enum ExitCode { INTERRUPTED = 130, } -function externallyHandledError(error: unknown): unknown { - if ((error as { reported?: boolean } | null)?.reported === true) return error; - if (!(error instanceof AgentCoreCLIError)) return error; - - const cause = error.cause; - if ( - cause instanceof CommanderError || - (cause as { reported?: boolean } | null)?.reported === true || - (cause as Error)?.name === "AbortError" - ) { - return cause; - } - return error; -} - // Runnable can be implemented by any application's main entrypoint. export interface Runnable { run(argv: string[]): Promise; @@ -48,20 +32,8 @@ export async function runWithExitCode( await fn(argv); return ExitCode.SUCCESS; } catch (caught) { - const error = externallyHandledError(caught); - if ( - !(error instanceof CommanderError) && - (error as { reported?: boolean } | null)?.reported !== true - ) { - const reported = error instanceof Error ? error : new Error(String(error)); - const name = reported instanceof AgentCoreCLIError ? "Error" : reported.name; - console.error(`${name}: ${reported.message}`); - } - if (error instanceof CommanderError) { - return error.exitCode === 0 ? ExitCode.SUCCESS : ExitCode.USAGE; - } - if ((error as Error)?.name === "AbortError") return ExitCode.INTERRUPTED; - if (caught instanceof AgentCoreCLIError) return caught.exitCode; - return ExitCode.FAILURE; + const error = AgentCoreCLIError.fromError(caught); + if (!(error instanceof SilentCLIError)) console.error(`Error: ${error.message}`); + return error.exitCode; } }