fixups after PR review

The comments for the PR review were taking from the previous PR (#12055)

Author: petebacondarwin

packages/wrangler/src/core/CommandHandledError.ts

@@ -1,14 +1,9 @@
 /**
- * A wrapper that indicates the original error was thrown from a command handler
- * that has already sent telemetry (started + errored events).
- *
- * When this error is caught in index.ts, the outer error handler should:
- * 1. NOT send fallback telemetry (it's already been sent)
- * 2. Unwrap and rethrow the original error for proper error handling/display
+ * A wrapper Error that indicates the original error was thrown during Command handler execution.
  *
  * This is used to distinguish between:
- * - Errors from command handlers (telemetry sent by handler)
- * - Yargs validation errors (telemetry needs to be sent by fallback handler)
+ * - Errors from within command handlers: telemetry and error reporting already sent by handler.
+ * - Yargs validation errors: telemetry and error reporting needs to be sent in the yargs middleware.
  */
 export class CommandHandledError {
 	constructor(public readonly originalError: unknown) {}

packages/wrangler/src/core/handle-errors.ts

@@ -249,7 +249,6 @@ function isNetworkFetchFailedError(e: unknown): boolean {
 
 /**
  * Determines the error type for telemetry purposes.
- * This is a pure function that doesn't log or have side effects.
  */
 export function getErrorType(e: unknown): string | undefined {
 	if (isCloudflareAPIDNSError(e)) {
@@ -264,12 +263,7 @@ export function getErrorType(e: unknown): string | undefined {
 	if (isCloudflareAPIConnectionTimeoutError(e)) {
 		return "ConnectionTimeout";
 	}
-	if (
-		isAuthenticationError(e) ||
-		(e instanceof UserError &&
-			e.cause instanceof ApiError &&
-			e.cause.status === 403)
-	) {
+	if (isAuthenticationError(e) || isContainersAuthenticationError(e)) {
 		return "AuthenticationError";
 	}
 	if (isBuildFailure(e) || isBuildFailureFromCause(e)) {
@@ -475,14 +469,7 @@ export async function handleError(
 		}
 
 		await wrangler.parse();
-	} else if (
-		isAuthenticationError(e) ||
-		// Is this a Containers/Cloudchamber-based auth error?
-		// This is different because it uses a custom OpenAPI-based generated client
-		(e instanceof UserError &&
-			e.cause instanceof ApiError &&
-			e.cause.status === 403)
-	) {
+	} else if (isAuthenticationError(e) || isContainersAuthenticationError(e)) {
 		mayReport = false;
 		if (e.cause instanceof ApiError) {
 			logger.error(e.cause);
@@ -600,3 +587,16 @@ export async function handleError(
 		await captureGlobalException(loggableException);
 	}
 }
+
+/**
+ * Is this a Containers/Cloudchamber-based auth error?
+ *
+ * This is different because it uses a custom OpenAPI-based generated client
+ */
+function isContainersAuthenticationError(e: unknown): e is UserError {
+	return (
+		e instanceof UserError &&
+		e.cause instanceof ApiError &&
+		e.cause.status === 403
+	);
+}

packages/wrangler/src/core/register-yargs-command.ts

@@ -18,7 +18,7 @@ import { dedent } from "../utils/dedent";
 import { isLocal, printResourceLocation } from "../utils/is-local";
 import { printWranglerBanner } from "../wrangler-banner";
 import { CommandHandledError } from "./CommandHandledError";
-import { getErrorType } from "./handle-errors";
+import { getErrorType, handleError } from "./handle-errors";
 import { demandSingleValue } from "./helpers";
 import type { CommonYargsArgv, SubHelp } from "../yargs-types";
 import type {
@@ -186,7 +186,6 @@ function createHandler(
 							})
 						: defaultWranglerConfig;
 
-				// Create metrics dispatcher with config info
 				const dispatcher = getMetricsDispatcher({
 					sendMetrics: config.send_metrics,
 					hasAssets: !!config.assets?.directory,
@@ -215,7 +214,6 @@ function createHandler(
 					}
 				}
 
-				// Send "started" event
 				dispatcher.sendCommandEvent("wrangler command started", {
 					command: commandName,
 					args,
@@ -230,7 +228,6 @@ function createHandler(
 						fetchResult,
 					});
 
-					// Send "completed" event
 					const durationMs = Date.now() - startTime;
 					dispatcher.sendCommandEvent("wrangler command completed", {
 						command: commandName,
@@ -243,12 +240,11 @@ function createHandler(
 					return result;
 				} catch (err) {
 					// If the error is already a CommandHandledError (e.g., from a nested wrangler.parse() call),
-					// don't wrap it again - just rethrow it
+					// don't wrap it again; just rethrow.
 					if (err instanceof CommandHandledError) {
 						throw err;
 					}
 
-					// Send "errored" event
 					const durationMs = Date.now() - startTime;
 					dispatcher.sendCommandEvent("wrangler command errored", {
 						command: commandName,
@@ -262,7 +258,10 @@ function createHandler(
 								? err.telemetryMessage
 								: undefined,
 					});
-					// Wrap the error to signal that telemetry has been sent
+
+					await handleError(err, args, argv);
+
+					// Wrap the error to signal that the telemetry has already been sent and the error reporting handled.
 					throw new CommandHandledError(err);
 				}
 			});
@@ -274,13 +273,11 @@ function createHandler(
 			if (outputErr instanceof Error) {
 				const code =
 					"code" in outputErr ? (outputErr.code as number) : undefined;
-				const message =
-					"message" in outputErr ? (outputErr.message as string) : undefined;
 				writeOutput({
 					type: "command-failed",
 					version: 1,
 					code,
-					message,
+					message: outputErr.message,
 				});
 			}
 			throw err;

packages/wrangler/src/index.ts

@@ -1814,41 +1814,27 @@ export async function main(argv: string[]): Promise<void> {
 	} catch (e) {
 		cliHandlerThrew = true;
 
-		// Check if this is a CommandHandledError (telemetry already sent by handler)
 		if (e instanceof CommandHandledError) {
-			// Unwrap and handle the original error
-			await handleError(e.originalError, configArgs, argv);
+			// This error occurred during Command handler execution,
+			// and has already sent metrics and reported to the user.
+			// So we can just re-throw the original error.
 			throw e.originalError;
+		} else {
+			// The error occurred before Command handler ran
+			// (e.g., yargs validation errors like unknown commands or invalid arguments).
+			// So we need to handle telemetry and error reporting here.
+			if (dispatcher && command) {
+				dispatchGenericCommandErrorEvent(
+					dispatcher,
+					command,
+					configArgs,
+					startTime,
+					e
+				);
+			}
+			await handleError(e, configArgs, argv);
+			throw e;
 		}
-
-		// Fallback telemetry for errors that occurred before handler ran
-		// (e.g., yargs validation errors like unknown commands or invalid arguments)
-		if (dispatcher && command) {
-			const durationMs = Date.now() - startTime;
-
-			// Send "started" event (since handler never got to send it)
-			dispatcher.sendCommandEvent("wrangler command started", {
-				command,
-				args: configArgs,
-			});
-
-			// Send "errored" event
-			dispatcher.sendCommandEvent("wrangler command errored", {
-				command,
-				args: configArgs,
-				durationMs,
-				durationSeconds: durationMs / 1000,
-				durationMinutes: durationMs / 1000 / 60,
-				errorType: getErrorType(e),
-				errorMessage:
-					e instanceof UserError || e instanceof ContainersUserError
-						? e.telemetryMessage
-						: undefined,
-			});
-		}
-
-		await handleError(e, configArgs, argv);
-		throw e;
 	} finally {
 		try {
 			// In the bootstrapper script `bin/wrangler.js`, we open an IPC channel,
@@ -1881,3 +1867,36 @@ export async function main(argv: string[]): Promise<void> {
 		}
 	}
 }
+
+/**
+ * Dispatches generic metrics events to indicate that a wrangler command errored
+ * when we don't know the CommandDefinition and cannot be sure what is safe to send.
+ */
+function dispatchGenericCommandErrorEvent(
+	dispatcher: ReturnType<typeof getMetricsDispatcher>,
+	command: string,
+	configArgs: ReadConfigCommandArgs,
+	startTime: number,
+	error: unknown
+) {
+	const durationMs = Date.now() - startTime;
+
+	// Send "started" event since handler never got to send it.
+	dispatcher.sendCommandEvent("wrangler command started", {
+		command,
+		args: configArgs,
+	});
+
+	dispatcher.sendCommandEvent("wrangler command errored", {
+		command,
+		args: configArgs,
+		durationMs,
+		durationSeconds: durationMs / 1000,
+		durationMinutes: durationMs / 1000 / 60,
+		errorType: getErrorType(error),
+		errorMessage:
+			error instanceof UserError || error instanceof ContainersUserError
+				? error.telemetryMessage
+				: undefined,
+	});
+}