From 38e2807f6cb2c96caf81e97bcae5ca8e12933bc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Fri, 24 Apr 2026 03:31:39 -0400 Subject: [PATCH] refactor(errors): route fatal app errors --- src/main/browser-action-service.ts | 22 +-------------- src/main/credentials/credential-resolver.ts | 22 ++++++++++++--- src/main/errors/handlers.ts | 19 +++++++++++++ src/main/errors/index.ts | 2 +- tests/unit/app-error-fatal-route.test.ts | 30 +++++++++++++++++++++ tests/unit/credential-resolver.test.ts | 16 ++++++++++- 6 files changed, 84 insertions(+), 27 deletions(-) create mode 100644 tests/unit/app-error-fatal-route.test.ts diff --git a/src/main/browser-action-service.ts b/src/main/browser-action-service.ts index 38312a0b..6342f597 100644 --- a/src/main/browser-action-service.ts +++ b/src/main/browser-action-service.ts @@ -78,26 +78,6 @@ export function createBrowserActionService({ }); } - /** - * Returns the approval record for `approvalId`, or null if the record is - * missing. Isolates the "approval row disappeared" case so the caller does - * not need to nest a try/catch around the state machine. - */ - function getApprovalOrNull(approvalId: string): ReturnType | null { - try { - return db.core.approvals.getApprovalRecord(approvalId); - } catch (err) { - const classified = classifyUnknownError(err, { - kind: "recoverable", - code: "browser_action.approval.lookup_failed", - message: `getApprovalRecord failed for ${approvalId}`, - context: { approvalId }, - }); - log.warn(`[${classified.code}]`, classified.message, classified.context); - return null; - } - } - async function dispatchBrowserAction(actionId: string, companyId: string) { const action = db.core.social.getBrowserAction(actionId, companyId); if (!action) { @@ -107,7 +87,7 @@ export function createBrowserActionService({ throw new Error(`Browser action ${actionId} is ${action.status} and cannot be dispatched.`); } if (action.approvalId) { - const approval = getApprovalOrNull(action.approvalId); + const approval = db.core.approvals.getApprovalRecord(action.approvalId); if (!approval || approval.companyId !== companyId) { throw new Error(`Browser action approval ${action.approvalId} does not belong to company ${companyId}.`); } diff --git a/src/main/credentials/credential-resolver.ts b/src/main/credentials/credential-resolver.ts index 483480c1..2c719c64 100644 --- a/src/main/credentials/credential-resolver.ts +++ b/src/main/credentials/credential-resolver.ts @@ -1,19 +1,33 @@ import { getOAuthProvider } from "@mariozechner/pi-ai/oauth"; import { getEnvApiKey } from "@mariozechner/pi-ai"; import { getConnectorDefinition, isPiKnownProvider } from "@main/connectors"; +import { UpstreamError } from "../errors/index.js"; import { CredentialStore } from "./credential-store"; import type { OAuthCredentialPayload, StoredCredentialPayload } from "./types"; const REFRESH_WINDOW_MS = 60_000; -export class CredentialError extends Error { +export type CredentialErrorCode = + | "CREDENTIAL_MISSING" + | "CREDENTIAL_EXPIRED" + | "CREDENTIAL_INVALID" + | "CREDENTIAL_REFRESH_FAILED"; + +export class CredentialError extends UpstreamError { + readonly connectorId: string; + constructor( - public readonly code: "CREDENTIAL_MISSING" | "CREDENTIAL_EXPIRED" | "CREDENTIAL_INVALID" | "CREDENTIAL_REFRESH_FAILED", - public readonly connectorId: string, + code: CredentialErrorCode, + connectorId: string, message?: string, ) { - super(message ?? code); + super({ + code, + message: message ?? code, + context: { connectorId }, + }); this.name = "CredentialError"; + this.connectorId = connectorId; } } diff --git a/src/main/errors/handlers.ts b/src/main/errors/handlers.ts index d0c4b668..763deefb 100644 --- a/src/main/errors/handlers.ts +++ b/src/main/errors/handlers.ts @@ -13,6 +13,22 @@ export interface Logger { warn?: (...args: unknown[]) => void; } +type FatalErrorRoute = (error: AppError) => never; + +const defaultFatalErrorRoute: FatalErrorRoute = (error) => { + if (process.env.VITEST === "true" || process.env.NODE_ENV === "test") { + throw error; + } + process.stderr.write(`[fatal:${error.code}] ${error.message}\n`); + process.abort(); +}; + +let fatalErrorRoute: FatalErrorRoute = defaultFatalErrorRoute; + +export function setFatalErrorRouteForTesting(route: FatalErrorRoute | null): void { + fatalErrorRoute = route ?? defaultFatalErrorRoute; +} + /** * Classify an unknown thrown value and emit a single structured log line. * Returns the `AppError` so the caller can still branch on `err.kind` @@ -46,5 +62,8 @@ export function logAppError( } else if (typeof logger.warn === "function") { logger.warn(...parts); } + if (err.kind === "fatal") { + fatalErrorRoute(err); + } return err; } diff --git a/src/main/errors/index.ts b/src/main/errors/index.ts index 71983ae6..048e2a5d 100644 --- a/src/main/errors/index.ts +++ b/src/main/errors/index.ts @@ -18,4 +18,4 @@ export { type ErrorKind, } from "./taxonomy.js"; -export { logAppError, type Logger } from "./handlers.js"; +export { logAppError, setFatalErrorRouteForTesting, type Logger } from "./handlers.js"; diff --git a/tests/unit/app-error-fatal-route.test.ts b/tests/unit/app-error-fatal-route.test.ts new file mode 100644 index 00000000..c4fd792c --- /dev/null +++ b/tests/unit/app-error-fatal-route.test.ts @@ -0,0 +1,30 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { logAppError, setFatalErrorRouteForTesting } from "../../src/main/errors"; +import type { AppError } from "../../src/main/errors"; + +describe("AppError fatal route", () => { + afterEach(() => { + setFatalErrorRouteForTesting(null); + }); + + it("routes fatal errors through the process-level handler", () => { + const logger = { error: vi.fn(), warn: vi.fn() }; + const routed: AppError[] = []; + const sentinel = new Error("fatal route called"); + setFatalErrorRouteForTesting((error) => { + routed.push(error); + throw sentinel; + }); + + expect(() => logAppError(logger, new Error("sqlite corrupt"), { + kind: "fatal", + code: "db.sqlite.corrupt", + message: "SQLite profile is corrupt", + context: { profile: "test" }, + })).toThrow(sentinel); + + expect(routed[0]?.kind).toBe("fatal"); + expect(routed[0]?.code).toBe("db.sqlite.corrupt"); + expect(logger.error).toHaveBeenCalled(); + }); +}); diff --git a/tests/unit/credential-resolver.test.ts b/tests/unit/credential-resolver.test.ts index a8162a9f..c5f1f1b5 100644 --- a/tests/unit/credential-resolver.test.ts +++ b/tests/unit/credential-resolver.test.ts @@ -5,6 +5,7 @@ import { afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest"; import { AppDatabase } from "../../src/main/database"; import { CredentialStore } from "../../src/main/credentials/credential-store"; import { CredentialResolver, CredentialError } from "../../src/main/credentials/credential-resolver"; +import { AppError } from "../../src/main/errors"; beforeAll(() => { process.env.AGENTCOMPANY_ALLOW_INSECURE_VAULT = "true"; @@ -56,7 +57,20 @@ describe("CredentialResolver", () => { it("throws CREDENTIAL_MISSING when the env-backed key is unset", async () => { delete process.env.XAI_API_KEY; store.save("xai", { kind: "api_key", value: { source: "env", value: "XAI_API_KEY" } }); - await expect(resolver.resolve("xai")).rejects.toBeInstanceOf(CredentialError); + let thrown: unknown; + try { + await resolver.resolve("xai"); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(CredentialError); + expect(thrown).toBeInstanceOf(AppError); + expect(thrown).toMatchObject({ + kind: "upstream", + code: "CREDENTIAL_MISSING", + connectorId: "xai", + context: { connectorId: "xai" }, + }); }); it("assembles AWS extraEnv for IAM auth mode", async () => {