Fix 1Password vault listing: dead CLI→SDK fallback and swallowed error causes

packages/plugins/onepassword/src/react/OnePasswordSettings.tsx

@@ -41,6 +41,14 @@ import type { RedactedOnePasswordConfig } from "../sdk/types";
 // Vault picker
 // ---------------------------------------------------------------------------
 
+const VAULT_LIST_ERROR_FALLBACK = "Failed to list vaults";
+
+const formatVaultListError = (error: Error): string => {
+  // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: OnePasswordError carries a typed `message`
+  const message = error.message.trim();
+  return message ? `${VAULT_LIST_ERROR_FALLBACK}: ${message}` : VAULT_LIST_ERROR_FALLBACK;
+};
+
 function VaultPicker(props: {
   authKind: "desktop-app" | "service-account";
   accountName: string;
@@ -61,15 +69,15 @@ function VaultPicker(props: {
         isLoading: true,
         error: null,
       }),
-      onError: () => ({
+      onError: (queryError) => ({
         vaults: [] as { id: string; name: string }[],
         isLoading: false,
-        error: "Failed to list vaults",
+        error: formatVaultListError(queryError),
       }),
       onDefect: () => ({
         vaults: [] as { id: string; name: string }[],
         isLoading: false,
-        error: "Failed to list vaults",
+        error: VAULT_LIST_ERROR_FALLBACK,
       }),
       onSuccess: ({ value }) => {
         const v = value.vaults;

packages/plugins/onepassword/src/sdk/service.test.ts

@@ -0,0 +1,106 @@
+import { beforeEach, describe, expect, it } from "@effect/vitest";
+import { Effect } from "effect";
+// oxlint-disable-next-line executor/no-vitest-import -- boundary: vi.mock/vi.hoisted must come from vitest itself for mock hoisting to resolve
+import { vi } from "vitest";
+
+import { OnePasswordError } from "./errors";
+import { makeOnePasswordService } from "./service";
+
+const opMocks = vi.hoisted(() => ({
+  setGlobalFlags: vi.fn(),
+  setServiceAccount: vi.fn(),
+  vaultList: vi.fn(),
+  itemList: vi.fn(),
+  readParse: vi.fn(),
+}));
+
+const sdkMocks = vi.hoisted(() => ({
+  createClient: vi.fn(),
+  DesktopAuth: vi.fn((accountName: string) => ({ accountName })),
+}));
+
+vi.mock("@1password/op-js", () => ({
+  setGlobalFlags: opMocks.setGlobalFlags,
+  setServiceAccount: opMocks.setServiceAccount,
+  vault: { list: opMocks.vaultList },
+  item: { list: opMocks.itemList },
+  read: { parse: opMocks.readParse },
+}));
+
+vi.mock("@1password/sdk", () => ({
+  createClient: sdkMocks.createClient,
+  DesktopAuth: sdkMocks.DesktopAuth,
+}));
+
+describe("makeOnePasswordService", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    opMocks.vaultList.mockReturnValue([]);
+    opMocks.itemList.mockReturnValue([]);
+    opMocks.readParse.mockReturnValue("secret");
+    sdkMocks.createClient.mockResolvedValue({
+      secrets: { resolve: vi.fn(async () => "secret") },
+      vaults: { list: vi.fn(async () => []) },
+      items: { list: vi.fn(async () => []) },
+    });
+  });
+
+  it.effect("falls back to the SDK when the CLI throws while listing vaults", () =>
+    Effect.gen(function* () {
+      const sdkVaultsList = vi.fn(async () => [{ id: "sdk-vault", title: "SDK Vault" }]);
+      opMocks.vaultList.mockImplementation(() => {
+        // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: simulates the untyped op-js CLI wrapper throwing
+        throw new Error("spawn op ENOENT");
+      });
+      sdkMocks.createClient.mockResolvedValue({
+        secrets: { resolve: vi.fn(async () => "secret") },
+        vaults: { list: sdkVaultsList },
+        items: { list: vi.fn(async () => []) },
+      });
+
+      const service = yield* makeOnePasswordService(
+        { kind: "service-account", token: "ops_test_token" },
+        { timeoutMs: 1_000 },
+      );
+      const vaults = yield* service.listVaults();
+
+      expect(vaults).toEqual([{ id: "sdk-vault", title: "SDK Vault" }]);
+      expect(sdkMocks.createClient).toHaveBeenCalledTimes(1);
+      expect(sdkVaultsList).toHaveBeenCalledTimes(1);
+    }),
+  );
+
+  it.effect("includes the backend cause when both vault listing backends fail", () =>
+    Effect.gen(function* () {
+      opMocks.vaultList.mockImplementation(() => {
+        // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: simulates the untyped op-js CLI wrapper throwing
+        throw new Error("spawn op ENOENT");
+      });
+      sdkMocks.createClient.mockResolvedValue({
+        secrets: { resolve: vi.fn(async () => "secret") },
+        vaults: {
+          list: vi.fn(async () => {
+            // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: simulates the untyped 1Password SDK rejecting
+            throw new Error("desktop approval refused for account");
+          }),
+        },
+        items: { list: vi.fn(async () => []) },
+      });
+
+      const error = yield* makeOnePasswordService(
+        { kind: "service-account", token: "ops_test_token" },
+        { timeoutMs: 1_000 },
+      ).pipe(
+        Effect.flatMap((service) => service.listVaults()),
+        Effect.flip,
+      );
+
+      expect(error).toBeInstanceOf(OnePasswordError);
+      // oxlint-disable executor/no-unknown-error-message -- boundary: OnePasswordError carries a typed message; asserting its contents
+      expect(error.message).toContain("1Password SDK vault listing failed:");
+      expect(error.message).toContain("desktop approval refused for account");
+      expect(error.message).not.toBe("1Password CLI vault listing failed");
+      // oxlint-enable executor/no-unknown-error-message
+    }),
+  );
+});

packages/plugins/onepassword/src/sdk/service.ts

@@ -48,15 +48,37 @@ export type ResolvedAuth =
 // ---------------------------------------------------------------------------
 
 const DEFAULT_TIMEOUT_MS = 15_000;
+const MAX_ERROR_MESSAGE_LENGTH = 300;
+const SERVICE_ACCOUNT_TOKEN_RE = /ops_[A-Za-z0-9_-]+/g;
 type OnePasswordSdkModule = typeof import("@1password/sdk");
 
+const formatCause = (cause: unknown): string => {
+  // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: normalizing untyped op-js/SDK throwables into OnePasswordError.message
+  const maybeMessage = (cause as { readonly message?: unknown } | null | undefined)?.message;
+  const raw =
+    // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: last-resort stringification of a non-Error throwable
+    typeof maybeMessage === "string" && maybeMessage.length > 0 ? maybeMessage : String(cause);
+  return raw
+    .replace(SERVICE_ACCOUNT_TOKEN_RE, "[redacted 1Password token]")
+    .replace(/\s+/g, " ")
+    .trim();
+};
+
+const messageWithCause = (prefix: string, cause: unknown): string => {
+  const causeMessage = formatCause(cause);
+  const message = causeMessage ? `${prefix}: ${causeMessage}` : prefix;
+  return message.length > MAX_ERROR_MESSAGE_LENGTH
+    ? `${message.slice(0, MAX_ERROR_MESSAGE_LENGTH - 3)}...`
+    : message;
+};
+
 const loadOnePasswordSdk = (): Effect.Effect<OnePasswordSdkModule, OnePasswordError> =>
   Effect.tryPromise({
     try: () => import("@1password/sdk"),
-    catch: () =>
+    catch: (cause) =>
       new OnePasswordError({
         operation: "sdk module load",
-        message: "Failed to load 1Password SDK",
+        message: messageWithCause("Failed to load 1Password SDK", cause),
       }),
   });
 
@@ -99,20 +121,20 @@ export const makeNativeSdkService = (
           integrationName: "Executor",
           integrationVersion: "0.0.0",
         }),
-      catch: () =>
+      catch: (cause) =>
         new OnePasswordError({
           operation: "client setup",
-          message: "Failed to set up 1Password client",
+          message: messageWithCause("Failed to set up 1Password client", cause),
         }),
     }).pipe(timeoutWithOnePasswordError("client setup", timeoutMs));
 
     const wrap = <A>(fn: () => Promise<A>, operation: string): Effect.Effect<A, OnePasswordError> =>
       Effect.tryPromise({
         try: fn,
-        catch: () =>
+        catch: (cause) =>
           new OnePasswordError({
             operation,
-            message: `1Password SDK ${operation} failed`,
+            message: messageWithCause(`1Password SDK ${operation} failed`, cause),
           }),
       }).pipe(
         timeoutWithOnePasswordError(operation, timeoutMs),
@@ -158,10 +180,10 @@ export const makeCliService = (
               }
               return fn();
             },
-            catch: () =>
+            catch: (cause) =>
               new OnePasswordError({
                 operation,
-                message: `1Password CLI ${operation} failed`,
+                message: messageWithCause(`1Password CLI ${operation} failed`, cause),
               }),
           }),
         )
@@ -186,6 +208,24 @@ export const makeCliService = (
 // Smart factory — tries CLI first (avoids IPC hang), falls back to SDK
 // ---------------------------------------------------------------------------
 
+const isCliUnavailable = (error: OnePasswordError): boolean => {
+  // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: OnePasswordError carries a typed `message`
+  const message = error.message.toLowerCase();
+  return (
+    message.includes("enoent") ||
+    message.includes("not found") ||
+    message.includes("command not found") ||
+    message.includes("not installed") ||
+    message.includes("no such file") ||
+    message.includes("spawn op")
+  );
+};
+
+const chooseFallbackError = (
+  cliError: OnePasswordError,
+  sdkError: OnePasswordError,
+): OnePasswordError => (isCliUnavailable(cliError) ? sdkError : cliError);
+
 export const makeOnePasswordService = (
   auth: ResolvedAuth,
   options?: { readonly preferSdk?: boolean; readonly timeoutMs?: number },
@@ -196,11 +236,33 @@ export const makeOnePasswordService = (
     return makeNativeSdkService(auth, timeoutMs);
   }
 
-  // Default: prefer CLI to avoid the IPC hang bug
-  return makeCliService(auth).pipe(
-    Effect.catch((cliError: OnePasswordError) =>
-      // CLI unavailable (e.g. `op` not installed) — fall back to SDK
-      makeNativeSdkService(auth, timeoutMs).pipe(Effect.mapError(() => cliError)),
-    ),
-  );
+  return Effect.gen(function* () {
+    const cliService = yield* makeCliService(auth);
+    const sdkService = yield* Effect.cached(makeNativeSdkService(auth, timeoutMs));
+
+    const withSdkFallback = <A>(
+      cliEffect: Effect.Effect<A, OnePasswordError>,
+      sdkEffect: (service: OnePasswordService) => Effect.Effect<A, OnePasswordError>,
+    ): Effect.Effect<A, OnePasswordError> =>
+      cliEffect.pipe(
+        Effect.catch((cliError: OnePasswordError) =>
+          sdkService.pipe(
+            Effect.flatMap(sdkEffect),
+            Effect.mapError((sdkError: OnePasswordError) =>
+              chooseFallbackError(cliError, sdkError),
+            ),
+          ),
+        ),
+      );
+
+    return OnePasswordServiceTag.of({
+      resolveSecret: (uri) =>
+        withSdkFallback(cliService.resolveSecret(uri), (service) => service.resolveSecret(uri)),
+
+      listVaults: () => withSdkFallback(cliService.listVaults(), (service) => service.listVaults()),
+
+      listItems: (vaultId) =>
+        withSdkFallback(cliService.listItems(vaultId), (service) => service.listItems(vaultId)),
+    });
+  }).pipe(Effect.withSpan("onepassword.make_service"));
 };