Cache 1Password resolutions and unblock the op spawn (#1860)

Author: RhysSullivan

.changeset/tidy-onepassword-latency.md

@@ -0,0 +1,13 @@
+---
+"@executor-js/plugin-onepassword": patch
+---
+
+**1Password-backed connections no longer pay a 1Password read on every tool call**
+
+Each tool call resolves its connection's credential, and for 1Password-backed connections every resolution shelled out to the `op` CLI — roughly a second per call under desktop-app auth, multiplying the latency of every call several times over. The spawn was also synchronous, so one slow resolution (for example `op` waiting on a 1Password approval prompt) blocked the whole local server for every other request, with no timeout on that path.
+
+Three changes:
+
+- Successful resolutions are now served from memory for a short TTL (default 60s, `secretCacheTtlMs`). The cache keys by a fingerprint of the provider config, so editing or removing an account drops all cached secrets at once; not-found, ambiguity, and failure outcomes are never retained. Concurrent resolutions of the same ref share one backend read even with the TTL set to `0`.
+- The `op` CLI now runs as an asynchronous spawn with a hard deadline (the plugin's existing `timeoutMs`), so a stuck `op` fails with the troubleshooting message instead of freezing the server. Auth reaches the child per spawn (service-account token via the environment, desktop account via `--account`) instead of through the previous backend's process-global token state.
+- Services are memoized per auth identity, so the SDK fallback reuses one authenticated client instead of re-authenticating per resolution.

bun.lock

@@ -53,7 +53,6 @@
     "typecheck:slow": "bunx tsc --noEmit -p tsconfig.json"
   },
   "dependencies": {
-    "@1password/op-js": "^0.1.13",
     "@1password/sdk": "^0.4.1-beta.1",
     "@effect/atom-react": "catalog:",
     "@executor-js/sdk": "workspace:*"

packages/plugins/onepassword/package.json

@@ -0,0 +1,77 @@
+import { execFile } from "node:child_process";
+
+// Raw `op` CLI spawn boundary. Kept as its own module so the service can be
+// tested against a fake without mocking node builtins. The spawn is
+// asynchronous on purpose: the previous backend (`@1password/op-js`) ran `op`
+// with execFileSync, so an `op` stuck on a 1Password approval prompt blocked
+// the host's entire event loop — on the single-threaded local daemon that
+// froze every in-flight request until the prompt was answered.
+
+/** Spawn outcome as plain data. The promise always resolves; the service
+ *  layer owns failure typing and message shaping (redaction, truncation). */
+export type OpCliResult =
+  | { readonly ok: true; readonly stdout: string }
+  | {
+      readonly ok: false;
+      /** True when the child was killed by the spawn timeout. */
+      readonly timedOut: boolean;
+      readonly message: string;
+    };
+
+export interface OpCliInvocation {
+  readonly args: readonly string[];
+  readonly env: Readonly<Record<string, string | undefined>>;
+  /** Hard deadline for the child; on expiry it is killed and the result
+   *  carries `timedOut: true`. */
+  readonly timeoutMs: number;
+  /** Fiber interruption reaches the child through this signal. */
+  readonly signal: AbortSignal;
+}
+
+const isExecFileError = (
+  error: unknown,
+): error is NodeJS.ErrnoException & { readonly killed?: boolean } =>
+  typeof error === "object" && error !== null && "message" in error;
+
+const describeSpawnError = (error: unknown): string => {
+  if (isExecFileError(error)) {
+    // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: normalizing the untyped execFile callback error into plain result data
+    return error.message;
+  }
+  // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: last-resort stringification of a non-Error spawn failure
+  return String(error);
+};
+
+/** Run `op` once. stdout carries the successful payload; stderr carries the
+ *  CLI's human-readable diagnostics, so a non-zero exit reports stderr when
+ *  present (falling back to the spawn error, e.g. `spawn op ENOENT`). */
+export const opCliExec = ({
+  args,
+  env,
+  timeoutMs,
+  signal,
+}: OpCliInvocation): Promise<OpCliResult> =>
+  new Promise((resolve) => {
+    execFile(
+      "op",
+      args,
+      {
+        env: env as NodeJS.ProcessEnv,
+        timeout: timeoutMs,
+        signal,
+        maxBuffer: 16 * 1024 * 1024,
+      },
+      (error, stdout, stderr) => {
+        if (error === null) {
+          resolve({ ok: true, stdout });
+          return;
+        }
+        const stderrText = stderr.trim();
+        resolve({
+          ok: false,
+          timedOut: isExecFileError(error) && error.killed === true,
+          message: stderrText.length > 0 ? stderrText : describeSpawnError(error),
+        });
+      },
+    );
+  });

packages/plugins/onepassword/src/sdk/op-cli.ts

@@ -1,11 +1,17 @@
 import { describe, it, expect } from "@effect/vitest";
 import { Effect } from "effect";
+import { TestClock } from "effect/testing";
 
 import { ProviderKey, ToolAddress, createExecutor } from "@executor-js/sdk";
 import { makeInMemoryBlobStore, pluginBlobStore } from "@executor-js/sdk/core";
 import { makeTestConfig } from "@executor-js/sdk/testing";
 
-import { makeOnePasswordStore, onepasswordPlugin, resolveConfiguredRef } from "./plugin";
+import {
+  makeCachedRefResolver,
+  makeOnePasswordStore,
+  onepasswordPlugin,
+  resolveConfiguredRef,
+} from "./plugin";
 import type { OnePasswordService } from "./service";
 import { OnePasswordError } from "./errors";
 import { OnePasswordAccount, OnePasswordConfig, DesktopAppAuth } from "./types";
@@ -524,3 +530,121 @@ describe("resolveConfiguredRef", () => {
     }),
   );
 });
+
+// ---------------------------------------------------------------------------
+// Cached ref resolution — the executor resolves a connection's credential on
+// every tool call, so successful resolutions are served from memory for a
+// short TTL instead of paying a 1Password round trip per call.
+// ---------------------------------------------------------------------------
+
+describe("makeCachedRefResolver", () => {
+  const countingBackend = () => {
+    let resolves = 0;
+    const serviceFor = (account: OnePasswordAccount) =>
+      Effect.succeed<OnePasswordService>({
+        resolveSecret: (uri) =>
+          Effect.sync(() => {
+            resolves += 1;
+            return `secret:${account.id}:${uri}`;
+          }),
+        listVaults: () => Effect.succeed([]),
+        listItems: () => Effect.succeed([]),
+      });
+    return { serviceFor, resolveCount: () => resolves };
+  };
+
+  it.effect("serves a repeated resolution from memory within the TTL", () =>
+    Effect.gen(function* () {
+      const backend = countingBackend();
+      const resolve = makeCachedRefResolver(backend.serviceFor, 60_000);
+
+      const first = yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential");
+      const second = yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential");
+
+      expect(first).toEqual({
+        kind: "resolved",
+        value: "secret:acct-default:op://vault-123/item-1/credential",
+      });
+      expect(second).toEqual(first);
+      expect(backend.resolveCount()).toBe(1);
+    }),
+  );
+
+  it.effect("asks the backend again once the TTL has passed", () =>
+    Effect.gen(function* () {
+      const backend = countingBackend();
+      const resolve = makeCachedRefResolver(backend.serviceFor, 60_000);
+
+      yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential");
+      yield* TestClock.adjust("61 seconds");
+      yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential");
+
+      expect(backend.resolveCount()).toBe(2);
+    }),
+  );
+
+  it.effect("never retains a not-found outcome", () =>
+    Effect.gen(function* () {
+      // A bare ref against empty vault listings resolves to not-found; the
+      // item may be created a moment later, so the miss must not stick.
+      const backend = countingBackend();
+      let listings = 0;
+      const serviceFor = (account: OnePasswordAccount) =>
+        backend.serviceFor(account).pipe(
+          Effect.map((service) => ({
+            ...service,
+            listItems: () =>
+              Effect.sync(() => {
+                listings += 1;
+                return [];
+              }),
+          })),
+        );
+      const resolve = makeCachedRefResolver(serviceFor, 60_000);
+
+      const first = yield* resolve(oneAccountConfig, "missing-item");
+      const second = yield* resolve(oneAccountConfig, "missing-item");
+
+      expect(first).toEqual({ kind: "not-found" });
+      expect(second).toEqual({ kind: "not-found" });
+      // Two vaults in the config, listed once per resolution.
+      expect(listings).toBe(4);
+    }),
+  );
+
+  it.effect("drops every cached secret the moment the config changes", () =>
+    Effect.gen(function* () {
+      const backend = countingBackend();
+      const resolve = makeCachedRefResolver(backend.serviceFor, 60_000);
+
+      yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential");
+      // Same ref, edited config (one vault removed): a removed account or
+      // vault must not keep serving secrets it used to grant.
+      const edited = OnePasswordConfig.make({
+        accounts: [
+          OnePasswordAccount.make({
+            id: "acct-default",
+            name: "1Password",
+            auth: desktopAuth,
+            vaults: [{ id: "vault-123", name: "Personal" }],
+          }),
+        ],
+      });
+      yield* resolve(edited, "op://vault-123/item-1/credential");
+
+      expect(backend.resolveCount()).toBe(2);
+    }),
+  );
+
+  it.effect("with a zero TTL every sequential resolution reaches the backend", () =>
+    Effect.gen(function* () {
+      const backend = countingBackend();
+      const resolve = makeCachedRefResolver(backend.serviceFor, 0);
+
+      yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential");
+      yield* resolve(oneAccountConfig, "op://vault-123/item-1/credential");
+
+      expect(backend.resolveCount()).toBe(2);
+    }),
+  );
+});