Skip to content

Commit

Permalink
feat: improve error handling (#367)
Browse files Browse the repository at this point in the history
### What
* Add `ShortestError` base class with specialized error types
* Implement `asShortestError()` utility for error conversion
* Standardize error messages and formatting
* Use `ConfigError` for invalid `config` for `Log`

### Why
* Improves error reporting with consistent formatting
* Makes error handling more predictable
* Provides clearer error types for debugging
* Prevent catching non-Shortest errors
  • Loading branch information
rmarescu authored Feb 26, 2025
1 parent b420e26 commit 1dba67e
Show file tree
Hide file tree
Showing 22 changed files with 182 additions and 99 deletions.
11 changes: 8 additions & 3 deletions packages/shortest/src/ai/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ import { getConfig } from "@/index";
import { getLogger, Log } from "@/log";
import { ToolResult } from "@/types";
import { TokenUsage, TokenUsageSchema } from "@/types/ai";
import { getErrorDetails, AIError, AIErrorType } from "@/utils/errors";
import {
getErrorDetails,
AIError,
AIErrorType,
asShortestError,
} from "@/utils/errors";
import { sleep } from "@/utils/sleep";

/**
Expand Down Expand Up @@ -139,7 +144,7 @@ export class AIClient {
} catch (error: any) {
this.log.error("Action failed", getErrorDetails(error));
if (this.isNonRetryableError(error)) {
throw error;
throw asShortestError(error);
}
retries++;
this.log.trace("Retry attempt", { retries, maxRetries: MAX_RETRIES });
Expand Down Expand Up @@ -231,7 +236,7 @@ export class AIClient {
if (NoSuchToolError.isInstance(error)) {
this.log.error("Tool is not supported");
}
throw error;
throw asShortestError(error);
}

this.log.trace("Request completed", {
Expand Down
3 changes: 1 addition & 2 deletions packages/shortest/src/browser/actions/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Page } from "playwright";

import { ToolError } from "@/browser/core";
import { ToolError } from "@/utils/errors";

export const keyboardShortcuts: Record<string, string | string[]> = {
"ctrl+l": ["Control", "l"],
Expand Down
3 changes: 2 additions & 1 deletion packages/shortest/src/browser/core/bash-tool.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { spawn } from "child_process";
import { getLogger, Log } from "@/log";
import { asShortestError } from "@/utils/errors";

// eslint-disable-next-line zod/require-zod-schema-types
type BashToolError = "timeout" | "network" | "unknown" | "unauthorized";
Expand Down Expand Up @@ -43,7 +44,7 @@ export class BashTool {

child.on("error", (err) => {
reject(`Error spawning process: ${err.message}`);
throw err;
throw asShortestError(err);
});
});
}
Expand Down
25 changes: 13 additions & 12 deletions packages/shortest/src/browser/core/browser-tool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
import { join } from "path";
import { Page } from "playwright";
import * as actions from "@/browser/actions";
import { BaseBrowserTool, ToolError } from "@/browser/core";
import { BaseBrowserTool } from "@/browser/core";
import { GitHubTool } from "@/browser/integrations/github";
import { MailosaurTool } from "@/browser/integrations/mailosaur";
import { BrowserManager } from "@/browser/manager";
Expand All @@ -31,9 +31,7 @@ import {
ShortestConfig,
} from "@/types";
import { ActionInput, ToolResult, BetaToolType } from "@/types/browser";
import { CallbackError } from "@/types/test";
import { AssertionCallbackError } from "@/types/test";
import { getErrorDetails } from "@/utils/errors";
import { getErrorDetails, ToolError, TestError } from "@/utils/errors";

export class BrowserTool extends BaseBrowserTool {
private page: Page;
Expand Down Expand Up @@ -383,13 +381,13 @@ export class BrowserTool extends BaseBrowserTool {
// Check if it's an assertion error from jest/expect
if (error && (error as any).matcherResult) {
const assertionError = error as any;
throw new AssertionCallbackError(
assertionError.message,
assertionError.matcherResult.actual,
assertionError.matcherResult.expected,
);
throw new TestError("assertion-failed", assertionError.message, {
actual: assertionError.matcherResult.actual,
expected: assertionError.matcherResult.expected,
});
}
throw new CallbackError(
throw new TestError(
"callback-execution-failed",
error instanceof Error ? error.message : String(error),
);
}
Expand Down Expand Up @@ -593,7 +591,7 @@ export class BrowserTool extends BaseBrowserTool {
} catch (error) {
this.log.error("Browser action failed", getErrorDetails(error));

if (error instanceof AssertionCallbackError) {
if (error instanceof TestError && error.type === "assertion-failed") {
return {
output: `Assertion failed: ${error.message}${
error.actual !== undefined
Expand All @@ -602,7 +600,10 @@ export class BrowserTool extends BaseBrowserTool {
}`,
};
}
if (error instanceof CallbackError) {
if (
error instanceof TestError &&
error.type === "callback-execution-failed"
) {
return {
output: `Callback execution failed: ${error.message}`,
};
Expand Down
8 changes: 1 addition & 7 deletions packages/shortest/src/browser/core/index.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
import { BrowserToolOptions, ActionInput, ToolResult } from "@/types/browser";
import { BetaToolType } from "@/types/browser";

export class ToolError extends Error {
constructor(message: string) {
super(message);
this.name = "ToolError";
}
}
import { ToolError } from "@/utils/errors";

export abstract class BaseBrowserTool {
protected width: number;
Expand Down
9 changes: 6 additions & 3 deletions packages/shortest/src/browser/integrations/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import dotenv from "dotenv";
import { authenticator } from "otplib";
import { ENV_LOCAL_FILENAME } from "@/constants";
import { BrowserToolInterface } from "@/types/browser";
import { ConfigError, ShortestError } from "@/utils/errors";

export class GitHubTool {
private totpSecret: string;
Expand All @@ -22,15 +23,17 @@ export class GitHubTool {
this.totpSecret = secret || process.env.GITHUB_TOTP_SECRET || "";

if (!this.totpSecret) {
throw new Error(
throw new ConfigError(
"invalid-config",
"GITHUB_TOTP_SECRET is required in .env file or via --secret flag",
);
}
}

private validateSecret() {
if (!this.totpSecret) {
throw new Error(
throw new ConfigError(
"invalid-config",
"GITHUB_TOTP_SECRET is required in .env file or via --secret flag",
);
}
Expand All @@ -43,7 +46,7 @@ export class GitHubTool {
const timeRemaining = authenticator.timeRemaining();
return { code, timeRemaining };
} catch (error) {
throw new Error(`Failed to generate TOTP code: ${error}`);
throw new ShortestError(`Failed to generate TOTP code: ${error}`);
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/shortest/src/browser/integrations/mailosaur.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import Mailosaur from "mailosaur";
import { ToolError } from "@/browser/core";
import { ToolError } from "@/utils/errors";

export class MailosaurTool {
private client: Mailosaur;
Expand Down
3 changes: 2 additions & 1 deletion packages/shortest/src/browser/manager/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import pc from "picocolors";
import { Browser, BrowserContext, chromium } from "playwright";
import { getLogger, Log } from "@/log/index";
import { ShortestConfig } from "@/types/config";
import { ShortestError } from "@/utils/errors";
import { getInstallationCommand } from "@/utils/platform";

export class BrowserManager {
Expand Down Expand Up @@ -67,7 +68,7 @@ export class BrowserManager {

async clearContext(): Promise<BrowserContext> {
if (!this.context) {
throw new Error("No context available");
throw new ShortestError("No context available");
}

// Clear all browser state
Expand Down
4 changes: 2 additions & 2 deletions packages/shortest/src/cache/test-cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { getLogger, Log } from "@/log";
import { CacheEntry, CacheStep } from "@/types/cache";
import type { TestFunction } from "@/types/test";
import { createHash } from "@/utils/create-hash";
import { getErrorDetails } from "@/utils/errors";
import { getErrorDetails, ShortestError } from "@/utils/errors";

// Shared process handlers registration
let handlersRegistered = false;
Expand Down Expand Up @@ -55,7 +55,7 @@ export class TestCache {
private currentCacheFileName: string | undefined = undefined; // e.g. "2025-02-22T12-34-56-a1b2c3d4.json"
private get currentCacheFilePath(): string {
if (!this.currentCacheFileName) {
throw new Error("currentCacheFilePath is not set");
throw new ShortestError("currentCacheFilePath is not set");
}
return path.join(this.cacheDir, this.currentCacheFileName);
}
Expand Down
15 changes: 11 additions & 4 deletions packages/shortest/src/cli/bin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { getConfig, initializeConfig } from "@/index";
import { LogLevel } from "@/log/config";
import { getLogger } from "@/log/index";
import { CLIOptions } from "@/types";
import { getErrorDetails } from "@/utils/errors";
import { getErrorDetails, ShortestError } from "@/utils/errors";

process.removeAllListeners("warning");
process.on("warning", (warning) => {
Expand Down Expand Up @@ -197,8 +197,11 @@ const main = async () => {
const success = await runner.execute(config.testPattern);
process.exitCode = success ? 0 : 1;
} catch (error: any) {
console.error(pc.red(error));
console.error(pc.red(error.name), error.message, getErrorDetails(error));
log.trace("Handling error for TestRunner");
if (!(error instanceof ShortestError)) throw error;

console.error(pc.red(error.name));
console.error(error.message, getErrorDetails(error));
process.exitCode = 1;
} finally {
await cleanUpCache();
Expand All @@ -207,6 +210,10 @@ const main = async () => {
};

main().catch(async (error) => {
console.error(error, getErrorDetails(error));
const log = getLogger();
log.trace("Handling error on main()");
if (!(error instanceof ShortestError)) throw error;

console.error(pc.red(error.name), error.message);
process.exit(1);
});
5 changes: 4 additions & 1 deletion packages/shortest/src/cli/init/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { DOT_SHORTEST_DIR_NAME } from "@/cache";
import { CONFIG_FILENAME, ENV_LOCAL_FILENAME } from "@/constants";
import { addToEnv } from "@/utils/add-to-env";
import { addToGitignore } from "@/utils/add-to-gitignore";
import { ShortestError } from "@/utils/errors";

export default async function main() {
console.log(pc.blue("Setting up Shortest..."));
Expand Down Expand Up @@ -115,7 +116,9 @@ export const getInstallCmd = async () => {
);

if (!command) {
throw new Error(`Unsupported package manager: ${packageManager.agent}`);
throw new ShortestError(
`Unsupported package manager: ${packageManager.agent}`,
);
}

const cmdString = `${command.command} ${command.args.join(" ")}`;
Expand Down
28 changes: 16 additions & 12 deletions packages/shortest/src/core/runner/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ import {
ShortestStrictConfig,
} from "@/types";
import { TokenUsageSchema } from "@/types/ai";
import { CacheError, getErrorDetails } from "@/utils/errors";
import {
CacheError,
getErrorDetails,
ShortestError,
asShortestError,
} from "@/utils/errors";

const TestStatusSchema = z.enum(["pending", "running", "passed", "failed"]);
export type TestStatus = z.infer<typeof TestStatusSchema>;
Expand Down Expand Up @@ -292,8 +297,8 @@ export class TestRunner {
this.log.trace("Launching browser");
context = await this.browserManager.launch();
} catch (error) {
this.log.error("Browser initialization failed", getErrorDetails(error));
throw error;
this.log.error("Browser launching failed", getErrorDetails(error));
throw asShortestError(error);
}
this.log.trace("Creating test context");
const testContext = await this.createTestContext(context);
Expand Down Expand Up @@ -346,16 +351,15 @@ export class TestRunner {
this.reporter.onFileEnd(fileResult);
}
} catch (error) {
this.log.trace("Handling error for executeTestFile");
if (!(error instanceof ShortestError)) throw error;
this.testContext = null; // Reset on error
if (error instanceof Error) {
const fileResult: FileResult = {
filePath: file,
status: "failed",
reason: error.message,
};

this.reporter.onFileEnd(fileResult);
}
const fileResult: FileResult = {
filePath: file,
status: "failed",
reason: error.message,
};
this.reporter.onFileEnd(fileResult);
}
}

Expand Down
24 changes: 17 additions & 7 deletions packages/shortest/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
CLIOptions,
} from "@/types";
import { parseConfig } from "@/utils/config";
import { ConfigError } from "@/utils/errors";
import { ConfigError, ShortestError } from "@/utils/errors";
// to include the global expect in the generated d.ts file
import "./globals";

Expand Down Expand Up @@ -97,7 +97,8 @@ export const initializeConfig = async ({
}

if (configs.length > 1) {
throw new Error(
throw new ConfigError(
"multiple-config",
`Multiple config files found: ${configs.map((c) => c.file).join(", ")}. Please keep only one.`,
);
}
Expand All @@ -109,7 +110,10 @@ export const initializeConfig = async ({

export const getConfig = (): ShortestStrictConfig => {
if (!globalConfig) {
throw new Error("Config not initialized. Call initializeConfig() first");
throw new ConfigError(
"no-config",
"Config not initialized. Call initializeConfig() first",
);
}
return globalConfig;
};
Expand Down Expand Up @@ -139,7 +143,7 @@ const createTestChain = (
// Return chain for the last test
const lastTest = tests[tests.length - 1];
if (!lastTest.name) {
throw new Error("Test name is required");
throw new ShortestError("Test name is required");
}
return createTestChain(lastTest.name, payloadOrFn, fn);
}
Expand All @@ -156,13 +160,19 @@ const createTestChain = (
registry.currentFileTests.push(test);
return {
expect: () => {
throw new Error("expect() cannot be called on direct execution test");
throw new ShortestError(
"expect() cannot be called on direct execution test",
);
},
after: () => {
throw new Error("after() cannot be called on direct execution test");
throw new ShortestError(
"after() cannot be called on direct execution test",
);
},
before: () => {
throw new Error("before() cannot be called on direct execution test");
throw new ShortestError(
"before() cannot be called on direct execution test",
);
},
};
}
Expand Down
1 change: 1 addition & 0 deletions packages/shortest/src/log/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import { LogConfig } from "@/log/config";
import { Log } from "@/log/log";

export { Log };
export { LogGroup } from "@/log/group";

Expand Down
Loading

0 comments on commit 1dba67e

Please sign in to comment.