From 1e83833d6a8eb812d3a69dd445de5d42be07b0e6 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 16:20:15 -0400 Subject: [PATCH 1/2] fix(server): one vault unlock covers every provider that reads from it Signed-off-by: Yordis Prieto --- .../Layers/ProviderAdapterRegistry.test.ts | 1 + .../ProviderInstanceRegistryHydration.ts | 23 ++- .../Layers/ProviderInstanceRegistryLive.ts | 13 ++ .../provider/Layers/ProviderRegistry.test.ts | 104 +++++++++++++ .../src/provider/Layers/ProviderRegistry.ts | 13 +- .../Layers/ProviderSecretResolverLive.test.ts | 143 ++++++++++++++++++ .../Layers/ProviderSecretResolverLive.ts | 102 ++++++++++++- .../provider/ProviderSecretReference.test.ts | 40 ++++- .../src/provider/ProviderSecretReference.ts | 24 +++ .../Services/ProviderInstanceRegistry.ts | 14 ++ .../Services/ProviderSecretResolver.ts | 16 ++ .../src/textGeneration/TextGeneration.test.ts | 1 + ...0016-provider-secrets-live-in-1password.md | 6 +- docs/internals/providers.md | 16 +- docs/user/provider-secrets.md | 5 +- 15 files changed, 506 insertions(+), 15 deletions(-) diff --git a/apps/server/src/provider/Layers/ProviderAdapterRegistry.test.ts b/apps/server/src/provider/Layers/ProviderAdapterRegistry.test.ts index e5309ff72a6c..316e9462a43b 100644 --- a/apps/server/src/provider/Layers/ProviderAdapterRegistry.test.ts +++ b/apps/server/src/provider/Layers/ProviderAdapterRegistry.test.ts @@ -144,6 +144,7 @@ const fakeInstanceRegistryLayer = Layer.succeed(ProviderInstanceRegistry.Provide streamChanges: Stream.empty, // Tests never drive changes through this fake; acquire a throwaway // subscription on an unused PubSub so the shape is satisfied. + listEnvironments: Effect.succeed(new Map()), subscribeChanges: Effect.flatMap(PubSub.unbounded(), (pubsub) => PubSub.subscribe(pubsub)), }); diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryHydration.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryHydration.ts index 0fd88b4262a6..8bb673c4598f 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryHydration.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryHydration.ts @@ -53,8 +53,10 @@ import * as Stream from "effect/Stream"; import { ServerSettingsService } from "../../serverSettings.ts"; import { BUILT_IN_DRIVERS, type BuiltInDriversEnv } from "../builtInDrivers.ts"; +import { collectProviderSecretReferences } from "../ProviderSecretReference.ts"; import { ProviderInstanceRegistry } from "../Services/ProviderInstanceRegistry.ts"; import { ProviderInstanceRegistryMutator } from "../Services/ProviderInstanceRegistryMutator.ts"; +import { ProviderSecretResolver } from "../Services/ProviderSecretResolver.ts"; import { ProviderInstanceRegistryMutableLayer } from "./ProviderInstanceRegistryLive.ts"; /** @@ -118,16 +120,27 @@ const SettingsWatcherLive = Layer.effectDiscard( Effect.gen(function* () { const mutator = yield* ProviderInstanceRegistryMutator; const serverSettings = yield* ServerSettingsService; + const secretResolver = yield* ProviderSecretResolver; yield* serverSettings.streamChanges.pipe( - Stream.runForEach((next) => - mutator - .reconcile(deriveProviderInstanceConfigMap(next)) + Stream.runForEach((next) => { + const configMap = deriveProviderInstanceConfigMap(next); + // Every instance about to be built resolves its own environment, and + // the secret store charges an unlock per read. Resolving the whole + // settings file's references first turns a fleet's worth of prompts + // into one, which matters most at boot when nothing is cached yet. + return secretResolver + .prime( + collectProviderSecretReferences( + Object.values(configMap).map((entry) => entry.environment), + ), + ) + .pipe(Effect.andThen(mutator.reconcile(configMap))) .pipe( Effect.catchCause((cause) => Effect.logError("ProviderInstanceRegistry reconcile failed", cause), ), - ), - ), + ); + }), Effect.forkScoped, ); }), diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts index 6cd1f9ae3125..a55d7235ff1e 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts @@ -38,6 +38,7 @@ import { ProviderInstanceId, type ProviderInstanceConfig, type ProviderInstanceConfigMap, + type ProviderInstanceEnvironment, type ProviderDriverKind, type ServerProvider, } from "@t3tools/contracts"; @@ -520,6 +521,18 @@ export const makeProviderInstanceRegistry = (input: { listUnavailable: Ref.get(unavailable).pipe( Effect.map((map) => Array.from(map.values()) as ReadonlyArray), ), + listEnvironments: Effect.gen(function* () { + const environments = new Map(); + for (const [instanceId, entry] of yield* Ref.get(rebuildable)) { + environments.set(instanceId, entry.environment); + } + // Live entries are written second so an instance that is both live and + // pending a retry reports the configuration it is actually running. + for (const [instanceId, live] of yield* Ref.get(entries)) { + environments.set(instanceId, live.entry.environment); + } + return environments; + }), rebuildInstanceWhen, // Getters: each read constructs a fresh Stream / Effect descriptor // so multiple consumers don't share a single already-started diff --git a/apps/server/src/provider/Layers/ProviderRegistry.test.ts b/apps/server/src/provider/Layers/ProviderRegistry.test.ts index b404f1d29254..cce76cddbecc 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.test.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.test.ts @@ -881,6 +881,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te listUnavailable: Effect.succeed([]), rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.empty, + listEnvironments: Effect.succeed(new Map()), subscribeChanges: Effect.flatMap(PubSub.unbounded(), PubSub.subscribe), }, ); @@ -955,6 +956,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te const rebuiltIds = yield* Ref.make>([]); const secretResolverLayer = Layer.succeed(ProviderSecretResolver, { resolve: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), + prime: () => Effect.void, invalidate: Ref.update(invalidations, (count) => count + 1), }); const instanceRegistryLayer = Layer.succeed( @@ -976,6 +978,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te ) : Effect.succeed(false), streamChanges: Stream.empty, + listEnvironments: Effect.succeed(new Map()), subscribeChanges: Effect.flatMap(PubSub.unbounded(), PubSub.subscribe), }, ); @@ -1031,6 +1034,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te const rebuiltIds = yield* Ref.make>([]); const secretResolverLayer = Layer.succeed(ProviderSecretResolver, { resolve: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), + prime: () => Effect.void, invalidate: Effect.void, }); const instanceRegistryLayer = Layer.succeed( @@ -1051,6 +1055,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te ) : Effect.succeed(false), streamChanges: Stream.empty, + listEnvironments: Effect.succeed(new Map()), subscribeChanges: Effect.flatMap(PubSub.unbounded(), PubSub.subscribe), }, ); @@ -1079,6 +1084,101 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te }), ); + it.effect("reads every instance's secret in one go before rebuilding any of them", () => + Effect.gen(function* () { + const codexDriver = ProviderDriverKind.make("codex"); + const claudeInstanceId = ProviderInstanceId.make("claude"); + const codexInstanceId = ProviderInstanceId.make("codex"); + const claudeReference = "op://Vault/claude/token"; + const codexReference = "op://Vault/codex/token"; + const unavailableProvider = (instanceId: ProviderInstanceId) => + ({ + instanceId, + driver: codexDriver, + status: "error", + enabled: true, + installed: false, + auth: { status: "unknown" }, + checkedAt: "2026-06-10T00:00:00.000Z", + version: null, + models: [], + slashCommands: [], + skills: [], + message: "Driver 'codex' failed to create instance: secret store is locked", + }) as const satisfies ServerProvider; + + const primed = yield* Ref.make>>([]); + const rebuiltIds = yield* Ref.make>([]); + const secretResolverLayer = Layer.succeed(ProviderSecretResolver, { + resolve: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), + prime: (references) => + Ref.update(primed, (previous) => [...previous, references]).pipe(Effect.asVoid), + invalidate: Effect.void, + }); + const environmentFor = (reference: string) => [ + { name: "TOKEN", value: reference, sensitive: true }, + ]; + const instanceRegistryLayer = Layer.succeed( + ProviderInstanceRegistry.ProviderInstanceRegistry, + { + getInstance: () => Effect.succeed(undefined), + listInstances: Effect.succeed([]), + listUnavailable: Effect.succeed([ + unavailableProvider(claudeInstanceId), + unavailableProvider(codexInstanceId), + ]), + listEnvironments: Effect.succeed( + new Map([ + [claudeInstanceId, environmentFor(claudeReference)], + [codexInstanceId, environmentFor(codexReference)], + ]), + ), + rebuildInstanceWhen: (instanceId, shouldRebuild) => + shouldRebuild({ + driver: codexDriver, + environment: environmentFor( + instanceId === claudeInstanceId ? claudeReference : codexReference, + ), + }) + ? Ref.update(rebuiltIds, (previous) => [...previous, instanceId]).pipe( + Effect.as(true), + ) + : Effect.succeed(false), + streamChanges: Stream.empty, + subscribeChanges: Effect.flatMap(PubSub.unbounded(), PubSub.subscribe), + }, + ); + + const scope = yield* Scope.make(); + yield* Effect.addFinalizer(() => Scope.close(scope, Exit.void)); + const runtimeServices = yield* Layer.build( + ProviderRegistryLive.pipe( + Layer.provideMerge(instanceRegistryLayer), + Layer.provideMerge( + ServerConfig.layerTest(process.cwd(), { + prefix: "t3-provider-registry-secret-prime-", + }), + ), + Layer.provideMerge(NodeServices.layer), + Layer.provideMerge(secretResolverLayer), + ), + ).pipe(Scope.provide(scope)); + + yield* Effect.gen(function* () { + const registry = yield* ProviderRegistry.ProviderRegistry; + yield* registry.refresh(); + + // One call carrying both references. Priming per instance would be + // one unlock prompt per provider, which is the thing this exists to + // avoid, so the count matters as much as the contents. + const calls = yield* Ref.get(primed); + assert.strictEqual(calls.length, 1); + assert.deepStrictEqual(Array.from(calls[0] ?? []), [claudeReference, codexReference]); + assert.deepStrictEqual(yield* Ref.get(rebuiltIds), [claudeInstanceId, codexInstanceId]); + }).pipe(Effect.provide(runtimeServices)); + }), + ); + it.effect("persists the merged snapshot when a live update has empty models", () => Effect.gen(function* () { const cursorDriver = ProviderDriverKind.make("cursor"); @@ -1145,6 +1245,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te listUnavailable: Effect.succeed([]), rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.empty, + listEnvironments: Effect.succeed(new Map()), subscribeChanges: Effect.flatMap(PubSub.unbounded(), (pubsub) => PubSub.subscribe(pubsub), ), @@ -1276,6 +1377,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te listUnavailable: Effect.succeed([]), rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.empty, + listEnvironments: Effect.succeed(new Map()), subscribeChanges: Effect.flatMap(PubSub.unbounded(), (pubsub) => PubSub.subscribe(pubsub), ), @@ -1385,6 +1487,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te listUnavailable: Effect.succeed([]), rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.empty, + listEnvironments: Effect.succeed(new Map()), subscribeChanges: Effect.flatMap(PubSub.unbounded(), (pubsub) => PubSub.subscribe(pubsub), ), @@ -1497,6 +1600,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te listUnavailable: Effect.succeed([]), rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.fromPubSub(changes), + listEnvironments: Effect.succeed(new Map()), subscribeChanges: PubSub.subscribe(changes), }, ); diff --git a/apps/server/src/provider/Layers/ProviderRegistry.ts b/apps/server/src/provider/Layers/ProviderRegistry.ts index fa65677ca2ff..a794d333d509 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.ts @@ -44,7 +44,10 @@ import * as Semaphore from "effect/Semaphore"; import { ServerConfig } from "../../config.ts"; import { ProviderInstanceRegistry } from "../Services/ProviderInstanceRegistry.ts"; import { ProviderSecretResolver } from "../Services/ProviderSecretResolver.ts"; -import { hasProviderSecretReference } from "../ProviderSecretReference.ts"; +import { + collectProviderSecretReferences, + hasProviderSecretReference, +} from "../ProviderSecretReference.ts"; import { ProviderRegistry, type ProviderRegistryShape } from "../Services/ProviderRegistry.ts"; import { hydrateCachedProvider, @@ -670,6 +673,14 @@ export const ProviderRegistryLive = Layer.effect( ...(yield* instanceRegistry.listUnavailable).map(snapshotInstanceKey), ]), ); + // Resolve every reference the rebuilds are about to need, in one read. + // Each rebuild resolves its own environment, and the secret store + // charges an unlock per read rather than per secret, so without this a + // fleet of five instances is five authorizations for one refresh. + const environments = yield* instanceRegistry.listEnvironments; + yield* secretResolver.prime( + collectProviderSecretReferences(targets.map((instanceId) => environments.get(instanceId))), + ); const rebuilt = yield* Effect.forEach(targets, (instanceId) => instanceRegistry.rebuildInstanceWhen(instanceId, (entry) => hasProviderSecretReference(entry.environment), diff --git a/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts b/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts index 8179d03aa311..7f818a492fe3 100644 --- a/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts @@ -151,3 +151,146 @@ describe("ProviderSecretResolverLive", () => { }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); }); }); + +const SECOND_REFERENCE = "op://Private/codex/credential"; + +/** + * Spawner that answers each `op` invocation from `handler`, which is handed the + * argv and, for `op inject`, the template that was piped to stdin. + * + * The template matters: `prime` picks a random separator per call, so a test + * cannot hard-code the output. Recovering the separator from the template is + * also what proves the batch and the split agree on a format. + */ +function scriptedOpSpawner( + handler: ( + args: ReadonlyArray, + template: string, + ) => { stdout: string; stderr: string; code: number }, +) { + const invocations: Array> = []; + const layer = Layer.succeed( + ChildProcessSpawner.ChildProcessSpawner, + ChildProcessSpawner.make((command) => + Effect.gen(function* () { + const cmd = command as unknown as { + args: ReadonlyArray; + options?: { stdin?: Stream.Stream }; + }; + invocations.push(cmd.args); + const stdin = cmd.options?.stdin; + const chunks = stdin === undefined ? [] : yield* Stream.runCollect(stdin); + const template = Array.from(chunks) + .map((chunk) => new TextDecoder().decode(chunk)) + .join(""); + const result = handler(cmd.args, template); + return ChildProcessSpawner.makeHandle({ + pid: ChildProcessSpawner.ProcessId(1), + exitCode: Effect.succeed(ChildProcessSpawner.ExitCode(result.code)), + isRunning: Effect.succeed(false), + kill: () => Effect.void, + unref: Effect.succeed(Effect.void), + stdin: Sink.drain, + stdout: Stream.make(encoder.encode(result.stdout)), + stderr: Stream.make(encoder.encode(result.stderr)), + all: Stream.empty, + getInputFd: () => Sink.drain, + getOutputFd: () => Stream.empty, + }); + }), + ), + ); + return { layer, invocations }; +} + +/** The separator `prime` chose, read back out of the template it built. */ +function separatorOf(template: string): string { + const parts = template.split(/\{\{[^}]*\}\}/); + return parts[1] ?? ""; +} + +describe("ProviderSecretResolverLive.prime", () => { + it.effect("reads every reference in a single 1Password call", () => { + const spawner = scriptedOpSpawner((args, template) => { + if (args.includes("inject")) { + return { + stdout: ["sk-claude-token", "sk-codex-token"].join(separatorOf(template)), + stderr: "", + code: 0, + }; + } + return { stdout: "should-not-be-read-one-at-a-time", stderr: "", code: 0 }; + }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + + yield* resolver.prime([TOKEN_REFERENCE, SECOND_REFERENCE]); + + assert.strictEqual(spawner.invocations.length, 1); + assert.deepStrictEqual(Array.from(spawner.invocations[0] ?? []), ["inject"]); + + // Both instances resolve out of the primed cache, so the fleet costs the + // one authorization the batch already paid for. + const claude = yield* resolver.resolve( + decodeEnvironment([ + { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + ]), + ); + const codex = yield* resolver.resolve( + decodeEnvironment([{ name: "CODEX_TOKEN", value: SECOND_REFERENCE, sensitive: true }]), + ); + + assert.strictEqual(claude.variables?.[0]?.value, "sk-claude-token"); + assert.strictEqual(codex.variables?.[0]?.value, "sk-codex-token"); + assert.strictEqual(spawner.invocations.length, 1); + }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }); + + it.effect("falls back to one read at a time when the batch fails", () => { + const spawner = scriptedOpSpawner((args) => { + if (args.includes("inject")) { + return { stdout: "", stderr: 'could not resolve item "codex"', code: 1 }; + } + return args.includes(SECOND_REFERENCE) + ? { stdout: "", stderr: 'could not resolve item "codex"', code: 1 } + : { stdout: "sk-claude-token", stderr: "", code: 0 }; + }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + + yield* resolver.prime([TOKEN_REFERENCE, SECOND_REFERENCE]); + + // The batch is still attempted; it is the recovery that is per reference. + assert.deepStrictEqual(Array.from(spawner.invocations[0] ?? []), ["inject"]); + + // A batch that cannot be trusted leaves the cache cold rather than + // caching a failure for every reference in it, so the good reference + // still resolves and only the bad one is reported unresolved. + const claude = yield* resolver.resolve( + decodeEnvironment([ + { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + ]), + ); + const codex = yield* resolver.resolve( + decodeEnvironment([{ name: "CODEX_TOKEN", value: SECOND_REFERENCE, sensitive: true }]), + ); + + assert.strictEqual(claude.variables?.[0]?.value, "sk-claude-token"); + assert.deepStrictEqual(Array.from(claude.unresolved), []); + assert.deepStrictEqual(Array.from(codex.unresolved), ["CODEX_TOKEN"]); + }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }); + + it.effect("does not spawn a batch for a single reference", () => { + const spawner = scriptedOpSpawner(() => ({ stdout: "sk-claude-token", stderr: "", code: 0 })); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + + yield* resolver.prime([TOKEN_REFERENCE]); + + // One reference is one prompt either way, and `op read` names the + // reference it could not resolve. + assert.strictEqual(spawner.invocations.length, 0); + }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }); +}); diff --git a/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts b/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts index f3cb3b9a42c9..548ee6658a67 100644 --- a/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts @@ -7,6 +7,12 @@ * second place to configure credentials. Reads run one at a time: two * concurrent reads against a locked vault stack up two biometric prompts. * + * The prompt is charged per `op` invocation rather than per secret, so reading + * one reference at a time makes the whole fleet cost one authorization each. + * `prime` exists for that: `op inject` substitutes any number of references in + * a single process, so the caller that is about to build every instance pays + * one prompt for all of them. + * * Failures are cached alongside successes. If the vault is locked when the * first thread starts, every later thread in that session would otherwise * re-prompt; caching the miss keeps the failure quiet and puts recovery on @@ -24,6 +30,8 @@ import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as Option from "effect/Option"; +import * as Stream from "effect/Stream"; +import * as NodeCrypto from "node:crypto"; import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"; import { resolveSpawnCommand } from "@t3tools/shared/shell"; @@ -50,6 +58,58 @@ const SECRET_READ_TIMEOUT = Duration.seconds(45); */ const SECRET_CACHE_CAPACITY = 64; +/** + * Read many references in one `op inject`. + * + * `op inject` substitutes references inside a template, so the template is the + * references themselves joined by a separator, and the output is the secrets + * in the same order. The separator is random per call because a secret can + * contain anything at all, newlines included: splitting on a fixed marker + * would let a secret that happened to contain it shift every later value. + * + * Returns `undefined` when the batch cannot be trusted as a whole, which + * includes a single bad reference, since `op` resolves the template or fails + * it. The caller treats that as "not primed" and reads one at a time, which is + * both the per-variable failure isolation and the way the user finds out which + * reference is the broken one. + */ +const readSecretsTogether = Effect.fn("readSecretsTogether")(function* ( + references: ReadonlyArray, +) { + const separator = `__t3-secret-${NodeCrypto.randomUUID()}__`; + const template = references.map((reference) => `{{ ${reference} }}`).join(separator); + const spawnCommand = yield* resolveSpawnCommand(ONE_PASSWORD_BINARY, ["inject"]); + const result = yield* spawnAndCollect( + ONE_PASSWORD_BINARY, + ChildProcess.make(spawnCommand.command, spawnCommand.args, { + shell: spawnCommand.shell, + stdin: Stream.make(new TextEncoder().encode(template)), + }), + ); + if (result.code !== 0) { + // `op` names the reference it could not resolve on stderr and never echoes + // a secret, so this is safe to log verbatim. + yield* Effect.logWarning("Could not batch-read provider secrets from 1Password", { + references: references.length, + exitCode: result.code, + detail: result.stderr.trim(), + }); + return undefined; + } + const values = result.stdout.split(separator); + if (values.length !== references.length) { + yield* Effect.logWarning("1Password returned an unexpected number of provider secrets", { + expected: references.length, + received: values.length, + }); + return undefined; + } + return values.map((value) => { + const secret = value.trim(); + return secret.length > 0 ? secret : undefined; + }); +}); + const readSecret = Effect.fn("readSecret")(function* (reference: string) { const spawnCommand = yield* resolveSpawnCommand(ONE_PASSWORD_BINARY, [ "read", @@ -81,6 +141,11 @@ export const ProviderSecretResolverLive: Layer.Layer< > = Layer.effect( ProviderSecretResolver, Effect.gen(function* () { + // The service tag declares `prime` as `Effect`, so the spawner it + // needs is captured here rather than asked of the caller, the same way + // the cache's own lookup captures it. + const spawnerContext = yield* Effect.context(); + const cache = yield* Cache.make({ capacity: SECRET_CACHE_CAPACITY, // No time to live: a resolved secret is held until the user asks for a @@ -122,6 +187,41 @@ export const ProviderSecretResolverLive: Layer.Layer< return { variables: resolved as ProviderInstanceEnvironment, unresolved }; }); - return { resolve, invalidate: Cache.invalidateAll(cache) }; + const prime: ProviderSecretResolverShape["prime"] = (references) => + Effect.gen(function* () { + const wanted: Array = []; + for (const reference of new Set(references)) { + if (!(yield* Cache.has(cache, reference))) { + wanted.push(reference); + } + } + // One reference costs one prompt whichever command reads it, so there + // is nothing to save and `op read` gives the better error. + if (wanted.length < 2) { + return; + } + const values = yield* readSecretsTogether(wanted).pipe( + Effect.timeoutOption(SECRET_READ_TIMEOUT), + Effect.map(Option.getOrUndefined), + Effect.catch((error) => + Effect.logWarning("Could not run 1Password to batch-read provider secrets", { + references: wanted.length, + detail: String(error), + }).pipe(Effect.as(undefined)), + ), + ); + if (values === undefined) { + return; + } + yield* Effect.forEach( + wanted, + (reference, index) => Cache.set(cache, reference, values[index]), + { + discard: true, + }, + ); + }).pipe(Effect.provideContext(spawnerContext)); + + return { resolve, prime, invalidate: Cache.invalidateAll(cache) }; }), ); diff --git a/apps/server/src/provider/ProviderSecretReference.test.ts b/apps/server/src/provider/ProviderSecretReference.test.ts index abbcf48b64ea..a288451d1ae0 100644 --- a/apps/server/src/provider/ProviderSecretReference.test.ts +++ b/apps/server/src/provider/ProviderSecretReference.test.ts @@ -2,7 +2,11 @@ import { describe, expect, it } from "@effect/vitest"; import { ProviderInstanceEnvironment } from "@t3tools/contracts"; import * as Schema from "effect/Schema"; -import { hasProviderSecretReference, providerSecretReference } from "./ProviderSecretReference.ts"; +import { + collectProviderSecretReferences, + hasProviderSecretReference, + providerSecretReference, +} from "./ProviderSecretReference.ts"; const decodeEnvironment = Schema.decodeSync(ProviderInstanceEnvironment); @@ -56,3 +60,37 @@ describe("hasProviderSecretReference", () => { ).toBe(false); }); }); + +describe("collectProviderSecretReferences", () => { + it("returns each distinct reference once, in first-seen order", () => { + const shared = "op://Private/shared/credential"; + const environments = [ + decodeEnvironment([ + { name: "CLAUDE_CODE_OAUTH_TOKEN", value: shared, sensitive: true }, + { name: "HOME", value: "/home/u" }, + ]), + undefined, + decodeEnvironment([ + { name: "CODEX_TOKEN", value: "op://Private/codex/credential", sensitive: true }, + // The same item behind two providers is one read, not two. + { name: "OTHER_TOKEN", value: shared, sensitive: true }, + ]), + ]; + + expect(Array.from(collectProviderSecretReferences(environments))).toEqual([ + shared, + "op://Private/codex/credential", + ]); + }); + + it("is empty when nothing reads from a secret store", () => { + expect( + Array.from( + collectProviderSecretReferences([ + decodeEnvironment([{ name: "HOME", value: "/home/u" }]), + undefined, + ]), + ), + ).toEqual([]); + }); +}); diff --git a/apps/server/src/provider/ProviderSecretReference.ts b/apps/server/src/provider/ProviderSecretReference.ts index 6900c1b6dc45..4b45e843517e 100644 --- a/apps/server/src/provider/ProviderSecretReference.ts +++ b/apps/server/src/provider/ProviderSecretReference.ts @@ -44,3 +44,27 @@ export function hasProviderSecretReference( environment?.some((variable) => providerSecretReference(variable.value) !== undefined) ?? false ); } + +/** + * Every distinct secret reference across a set of environments, in the order + * they were first seen. + * + * Resolution is per reference but unlocking is per `op` invocation, so the + * caller that is about to build many instances wants the whole list up front: + * one call covering every reference costs one authorization, where one call + * per instance costs one each. + */ +export function collectProviderSecretReferences( + environments: Iterable, +): ReadonlyArray { + const references = new Set(); + for (const environment of environments) { + for (const variable of environment ?? []) { + const reference = providerSecretReference(variable.value); + if (reference !== undefined) { + references.add(reference); + } + } + } + return Array.from(references); +} diff --git a/apps/server/src/provider/Services/ProviderInstanceRegistry.ts b/apps/server/src/provider/Services/ProviderInstanceRegistry.ts index e57fc5e89a40..063e4429af72 100644 --- a/apps/server/src/provider/Services/ProviderInstanceRegistry.ts +++ b/apps/server/src/provider/Services/ProviderInstanceRegistry.ts @@ -19,6 +19,7 @@ */ import type { ProviderInstanceConfig, + ProviderInstanceEnvironment, ProviderInstanceId, ServerProvider, } from "@t3tools/contracts"; @@ -50,6 +51,19 @@ export interface ProviderInstanceRegistryShape { * directly into `ProviderRegistry` output. */ readonly listUnavailable: Effect.Effect>; + /** + * The configured environment of every instance the registry could rebuild, + * keyed by id. Covers the live instances and the ones whose last build + * failed, which is exactly the set `rebuildInstanceWhen` can act on. + * + * Exists so a caller that is about to rebuild several instances can inspect + * what they are configured with first. `ProviderRegistry` uses it to resolve + * every external secret in one go instead of once per rebuild; the registry + * itself stays ignorant of what any particular value means. + */ + readonly listEnvironments: Effect.Effect< + ReadonlyMap + >; /** * Tear one instance down and build it again from the configuration it * already has, but only when `shouldRebuild` accepts that configuration. diff --git a/apps/server/src/provider/Services/ProviderSecretResolver.ts b/apps/server/src/provider/Services/ProviderSecretResolver.ts index 644ffa6828d2..2e045769c749 100644 --- a/apps/server/src/provider/Services/ProviderSecretResolver.ts +++ b/apps/server/src/provider/Services/ProviderSecretResolver.ts @@ -36,6 +36,21 @@ export interface ProviderSecretResolverShape { readonly resolve: ( environment: ProviderInstanceEnvironment | undefined, ) => Effect.Effect; + /** + * Resolve `references` ahead of the callers that will ask for them, in as + * few reads as the secret store allows. + * + * Unlocking is charged per invocation of the store's CLI, not per secret, so + * a caller about to build ten instances that each resolve their own + * environment would otherwise cost ten authorizations. Priming turns that + * into one. + * + * Purely an optimization, and deliberately best effort: it never fails, and + * a batch that does not come back leaves the cache exactly as cold as it + * found it, so `resolve` falls back to reading one reference at a time with + * the same per-variable failure isolation it has always had. + */ + readonly prime: (references: ReadonlyArray) => Effect.Effect; /** * Drop every cached secret. The next `resolve` re-reads from the store. * Callers that need the new value to reach a running provider must also @@ -58,6 +73,7 @@ export class ProviderSecretResolver extends Context.Service< */ export const passthroughProviderSecretResolver: ProviderSecretResolverShape = { resolve: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), + prime: () => Effect.void, invalidate: Effect.void, }; diff --git a/apps/server/src/textGeneration/TextGeneration.test.ts b/apps/server/src/textGeneration/TextGeneration.test.ts index dbde7b8f2c14..22a7a43fdf9f 100644 --- a/apps/server/src/textGeneration/TextGeneration.test.ts +++ b/apps/server/src/textGeneration/TextGeneration.test.ts @@ -54,6 +54,7 @@ const makeStubRegistry = ( streamChanges: Stream.empty, // Tests never drive changes through this stub; acquire a throwaway // subscription on an unused PubSub so the shape is satisfied. + listEnvironments: Effect.succeed(new Map()), subscribeChanges: Effect.flatMap(PubSub.unbounded(), (pubsub) => PubSub.subscribe(pubsub), ), diff --git a/docs/fork/0016-provider-secrets-live-in-1password.md b/docs/fork/0016-provider-secrets-live-in-1password.md index ff7b1e104d44..00d2cf991f93 100644 --- a/docs/fork/0016-provider-secrets-live-in-1password.md +++ b/docs/fork/0016-provider-secrets-live-in-1password.md @@ -12,8 +12,10 @@ never written to the settings file or to T3 Code's secret store. - Unlock your vault once instead of all day. Each reference is read a single time and held in memory for the life of the server, so starting a thread, - sending a message, and the background status check all reuse it. A provider - carrying several references still asks once. + sending a message, and the background status check all reuse it. One approval + covers every reference across every provider, so a machine running five + providers on references costs the same single unlock that one provider does, + both at startup and on a refresh. - Rotate a secret and pick it up without restarting anything. Refreshing provider status in Settings goes back to 1Password and rebuilds only the providers that read from it. Threads already running keep working. diff --git a/docs/internals/providers.md b/docs/internals/providers.md index 249ef4476381..98cbde97e102 100644 --- a/docs/internals/providers.md +++ b/docs/internals/providers.md @@ -59,9 +59,19 @@ Three decisions are load-bearing: leaving the name out of the resolved list: the child environment starts from the server's own, so a name left alone keeps whatever the server inherited under it, and the agent would quietly run as a different account than the one the instance names. -- **Reads are sequential.** `resolve` walks the environment with a plain loop instead of - `Effect.forEach` with concurrency, so an instance carrying four references produces one biometric - prompt rather than four simultaneous ones. +- **Reads are batched across instances, and sequential within one.** The store charges an unlock per + `op` invocation, not per secret, and every instance resolves its own environment inside `create`, + so a fleet would otherwise cost one prompt per provider. `prime` reads the whole set in a single + `op inject` before the builds start, called from the settings watcher (which covers boot) and from + `reloadSecretBackedInstances` (which covers the refresh button). Whatever `prime` misses, + `resolve` still walks with a plain loop rather than `Effect.forEach` with concurrency, so it + produces one prompt rather than several simultaneous ones. +- **Priming can only ever help.** It is best effort and never fails: `op inject` resolves the whole + template or fails it, so one bad reference would take the batch down with it. A batch that does + not come back leaves the cache exactly as cold as it found it and `resolve` falls back to reading + one reference at a time, which is both where the per-variable failure isolation lives and how the + user learns which reference is the broken one. A separator is generated per call because a secret + can contain anything, newlines included. - **Failures are cached alongside successes.** The `Cache` holds Exits, so a locked vault costs one prompt per refresh cycle instead of one per thread start. Recovery is the refresh button, not a timeout: the cache is built without a `timeToLive`. diff --git a/docs/user/provider-secrets.md b/docs/user/provider-secrets.md index 8a142c980511..7156e8f3b028 100644 --- a/docs/user/provider-secrets.md +++ b/docs/user/provider-secrets.md @@ -59,8 +59,9 @@ Each reference is read one time and held in memory for the life of the server. S sending a message, and the background provider status check all reuse the value that was already read, so a locked vault prompts you once rather than every few minutes. -Providers with more than one reference are read one after another, so a single unlock covers all of -them. +One unlock covers every reference T3 Code needs, across every provider. Starting the server and +refreshing provider status both read the whole set in a single request to 1Password, so five +providers backed by references cost the same one approval that one provider does. ## I Rotated The Secret, How Do I Pick Up The New One From 0ced035c24d4351b1983638dd7106a718353bc33 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 16:41:13 -0400 Subject: [PATCH 2/2] fix(server): boot pays one unlock too, not one per instance The settings stream carries later writes only, so the initial fleet was built without priming and every instance read its own reference. Boot is the run where nothing is cached yet, which made it the most expensive path rather than the one the change was aimed at. Signed-off-by: Yordis Prieto --- .../ProviderInstanceRegistryHydration.ts | 39 +++++--- .../provider/Layers/ProviderRegistry.test.ts | 89 +++++++++++++++++++ 2 files changed, 116 insertions(+), 12 deletions(-) diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryHydration.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryHydration.ts index 8bb673c4598f..bc8f48bfbc67 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryHydration.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryHydration.ts @@ -29,7 +29,9 @@ * ---------- * On layer build we: * 1. Read the current `ServerSettings` once and use it to seed the - * registry's initial state via `ProviderInstanceRegistryMutableLayer`. + * registry's initial state via `ProviderInstanceRegistryMutableLayer`, + * priming that snapshot's secret references first so the whole boot + * fleet costs one unlock rather than one per instance. * 2. Fork a daemon fiber (lifetime tied to the layer's scope) that * subscribes to `ServerSettingsService.streamChanges` and calls * `ProviderInstanceRegistryMutator.reconcile` on every emission. @@ -56,7 +58,10 @@ import { BUILT_IN_DRIVERS, type BuiltInDriversEnv } from "../builtInDrivers.ts"; import { collectProviderSecretReferences } from "../ProviderSecretReference.ts"; import { ProviderInstanceRegistry } from "../Services/ProviderInstanceRegistry.ts"; import { ProviderInstanceRegistryMutator } from "../Services/ProviderInstanceRegistryMutator.ts"; -import { ProviderSecretResolver } from "../Services/ProviderSecretResolver.ts"; +import { + ProviderSecretResolver, + type ProviderSecretResolverShape, +} from "../Services/ProviderSecretResolver.ts"; import { ProviderInstanceRegistryMutableLayer } from "./ProviderInstanceRegistryLive.ts"; /** @@ -105,6 +110,20 @@ export const deriveProviderInstanceConfigMap = ( return merged as ProviderInstanceConfigMap; }; +/** + * Read every secret the config map's instances are about to need, before any + * of them is built. Each instance resolves its own environment, and the secret + * store charges an unlock per read rather than per secret, so without this a + * fleet of five reference-backed providers is five authorizations. + */ +const primeConfigMapSecrets = ( + secretResolver: ProviderSecretResolverShape, + configMap: ProviderInstanceConfigMap, +) => + secretResolver.prime( + collectProviderSecretReferences(Object.values(configMap).map((entry) => entry.environment)), + ); + /** * Layer that consumes `ProviderInstanceRegistryMutator` and forks a * settings-watcher fiber. The fiber's lifetime is tied to the enclosing @@ -124,16 +143,7 @@ const SettingsWatcherLive = Layer.effectDiscard( yield* serverSettings.streamChanges.pipe( Stream.runForEach((next) => { const configMap = deriveProviderInstanceConfigMap(next); - // Every instance about to be built resolves its own environment, and - // the secret store charges an unlock per read. Resolving the whole - // settings file's references first turns a fleet's worth of prompts - // into one, which matters most at boot when nothing is cached yet. - return secretResolver - .prime( - collectProviderSecretReferences( - Object.values(configMap).map((entry) => entry.environment), - ), - ) + return primeConfigMapSecrets(secretResolver, configMap) .pipe(Effect.andThen(mutator.reconcile(configMap))) .pipe( Effect.catchCause((cause) => @@ -169,6 +179,7 @@ export const ProviderInstanceRegistryHydrationLive: Layer.Layer< > = Layer.unwrap( Effect.gen(function* () { const serverSettings = yield* ServerSettingsService; + const secretResolver = yield* ProviderSecretResolver; const initialSettings: ServerSettings | undefined = yield* serverSettings.getSettings.pipe( Effect.orElseSucceed(() => undefined), ); @@ -177,6 +188,10 @@ export const ProviderInstanceRegistryHydrationLive: Layer.Layer< ? ({} as ProviderInstanceConfigMap) : deriveProviderInstanceConfigMap(initialSettings); + // The watcher only sees later writes, so this snapshot is the whole boot + // fleet and the one place where nothing is cached yet. + yield* primeConfigMapSecrets(secretResolver, initialConfigMap); + const mutableLayer = ProviderInstanceRegistryMutableLayer({ drivers: BUILT_IN_DRIVERS, configMap: initialConfigMap, diff --git a/apps/server/src/provider/Layers/ProviderRegistry.test.ts b/apps/server/src/provider/Layers/ProviderRegistry.test.ts index cce76cddbecc..31f06e505f3a 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.test.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.test.ts @@ -1769,6 +1769,95 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te }), ); + it.effect("reads every boot instance's secret before building any of them", () => + Effect.gen(function* () { + const claudeReference = "op://Vault/claude/token"; + const codexReference = "op://Vault/codex/token"; + // `streamChanges` carries later writes only, so the watcher never + // sees this snapshot. Boot is the run where nothing is cached yet + // and therefore the one that pays the most prompts without priming. + const serverSettings = yield* makeMutableServerSettingsService( + decodeServerSettings( + deepMerge(encodedDefaultServerSettings, { + providers: { + codex: { enabled: false }, + claudeAgent: { enabled: false }, + cursor: { enabled: false }, + grok: { enabled: false }, + opencode: { enabled: false }, + }, + providerInstances: { + claude_secret: { + driver: "claudeAgent", + displayName: "Claude Secret", + enabled: false, + environment: [ + { name: "CLAUDE_CODE_OAUTH_TOKEN", value: claudeReference, sensitive: true }, + ], + }, + codex_secret: { + driver: "codex", + displayName: "Codex Secret", + enabled: false, + environment: [{ name: "TOKEN", value: codexReference, sensitive: true }], + }, + } as unknown as ContractServerSettings["providerInstances"], + }), + ), + ); + + const calls = yield* Ref.make>([]); + const recordingSecretResolverLayer = Layer.succeed(ProviderSecretResolver, { + resolve: (environment) => + Ref.update(calls, (previous) => [...previous, "resolve"]).pipe( + Effect.as({ variables: environment, unresolved: [] }), + ), + prime: (references) => + Ref.update(calls, (previous) => [ + ...previous, + `prime:${Array.from(references).join(",")}`, + ]).pipe(Effect.asVoid), + invalidate: Effect.void, + }); + + const scope = yield* Scope.make(); + yield* Effect.addFinalizer(() => Scope.close(scope, Exit.void)); + yield* Layer.build( + ProviderInstanceRegistryHydrationLive.pipe( + Layer.provideMerge( + Layer.succeed(ServerSettingsModule.ServerSettingsService, serverSettings), + ), + Layer.provideMerge( + ServerConfig.layerTest(process.cwd(), { + prefix: "t3-provider-registry-boot-prime-", + }), + ), + Layer.provideMerge(TestHttpClientLive), + Layer.provideMerge( + Layer.succeed( + ProviderEventLoggers.ProviderEventLoggers, + ProviderEventLoggers.NoOpProviderEventLoggers, + ), + ), + Layer.provideMerge(OpenCodeRuntime.OpenCodeRuntimeLive), + Layer.provideMerge(NodeServices.layer), + Layer.provideMerge(BackgroundPolicyAlwaysRunLayer), + Layer.provideMerge(recordingSecretResolverLayer), + ), + ).pipe(Scope.provide(scope)); + + // Both references in one call, and that call ahead of the first + // instance that would have read one on its own. + const recorded = yield* Ref.get(calls); + assert.strictEqual( + recorded[0], + `prime:${claudeReference},${codexReference}`, + `Expected boot to prime both references first; instead saw: ${recorded.join(" | ")}`, + ); + assert.strictEqual(recorded.filter((entry) => entry.startsWith("prime")).length, 1); + }), + ); + // Guards the second half of the reported bug: changing // `providers.codex.binaryPath` in settings must tear down the live // instance and rebuild it so a fresh probe runs with the new binary.