From 7c672283aedb7d1026725de5145f91fe5a228c27 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 05:17:08 -0400 Subject: [PATCH 1/6] feat(server): provider credentials can live in 1Password Running several accounts of one provider means several long-lived tokens, and each had to be copied out of the password manager into a second store to be usable. That copy is the one nobody rotates. Pointing at the secret keeps a single copy under the policy chosen for it, and makes rotation a vault edit rather than a hunt for everywhere the value was pasted. Signed-off-by: Yordis Prieto --- .../src/provider/Drivers/ClaudeDriver.ts | 7 +- .../src/provider/Drivers/CodexDriver.ts | 7 +- .../src/provider/Drivers/CursorDriver.ts | 7 +- .../server/src/provider/Drivers/GrokDriver.ts | 7 +- .../src/provider/Drivers/OpenCodeDriver.ts | 7 +- .../Layers/ProviderAdapterRegistry.test.ts | 1 + .../ProviderInstanceRegistryLive.test.ts | 91 +++++++++++ .../Layers/ProviderInstanceRegistryLive.ts | 44 ++++++ .../provider/Layers/ProviderRegistry.test.ts | 117 ++++++++++++++ .../src/provider/Layers/ProviderRegistry.ts | 43 +++++ .../Layers/ProviderSecretResolverLive.test.ts | 149 ++++++++++++++++++ .../Layers/ProviderSecretResolverLive.ts | 125 +++++++++++++++ .../provider/ProviderSecretReference.test.ts | 58 +++++++ .../src/provider/ProviderSecretReference.ts | 46 ++++++ .../Services/ProviderInstanceRegistry.ts | 27 +++- .../Services/ProviderSecretResolver.ts | 61 +++++++ apps/server/src/server.ts | 5 + .../src/textGeneration/TextGeneration.test.ts | 1 + docs/README.md | 1 + ...0016-provider-secrets-live-in-1password.md | 54 +++++++ docs/fork/README.md | 2 + docs/internals/glossary.md | 8 + docs/internals/providers.md | 48 ++++++ docs/user/provider-secrets.md | 93 +++++++++++ 24 files changed, 1003 insertions(+), 6 deletions(-) create mode 100644 apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts create mode 100644 apps/server/src/provider/Layers/ProviderSecretResolverLive.ts create mode 100644 apps/server/src/provider/ProviderSecretReference.test.ts create mode 100644 apps/server/src/provider/ProviderSecretReference.ts create mode 100644 apps/server/src/provider/Services/ProviderSecretResolver.ts create mode 100644 docs/fork/0016-provider-secrets-live-in-1password.md create mode 100644 docs/user/provider-secrets.md diff --git a/apps/server/src/provider/Drivers/ClaudeDriver.ts b/apps/server/src/provider/Drivers/ClaudeDriver.ts index e099d52e5189..223624963f5e 100644 --- a/apps/server/src/provider/Drivers/ClaudeDriver.ts +++ b/apps/server/src/provider/Drivers/ClaudeDriver.ts @@ -43,6 +43,7 @@ import { } from "../ProviderDriver.ts"; import type { ServerProviderDraft } from "../providerSnapshot.ts"; import { mergeProviderInstanceEnvironment } from "../ProviderInstanceEnvironment.ts"; +import { ProviderSecretResolver } from "../Services/ProviderSecretResolver.ts"; import { enrichProviderSnapshotWithVersionAdvisory, makePackageManagedProviderMaintenanceResolver, @@ -89,6 +90,7 @@ export type ClaudeDriverEnv = | HttpClient.HttpClient | Path.Path | ProviderEventLoggers + | ProviderSecretResolver | ServerConfig | ServerSettingsService; @@ -125,7 +127,10 @@ export const ClaudeDriver: ProviderDriver = { const httpClient = yield* HttpClient.HttpClient; const serverSettings = yield* ServerSettingsService; const eventLoggers = yield* ProviderEventLoggers; - const processEnv = mergeProviderInstanceEnvironment(environment); + const secretResolver = yield* ProviderSecretResolver; + const processEnv = mergeProviderInstanceEnvironment( + yield* secretResolver.resolve(environment), + ); const fallbackContinuationIdentity = defaultProviderContinuationIdentity({ driverKind: DRIVER_KIND, instanceId, diff --git a/apps/server/src/provider/Drivers/CodexDriver.ts b/apps/server/src/provider/Drivers/CodexDriver.ts index 15d7a1ff0216..5766c45c89fe 100644 --- a/apps/server/src/provider/Drivers/CodexDriver.ts +++ b/apps/server/src/provider/Drivers/CodexDriver.ts @@ -42,6 +42,7 @@ import { makeManagedServerProvider } from "../makeManagedServerProvider.ts"; import type { ProviderDriver, ProviderInstance } from "../ProviderDriver.ts"; import type { ServerProviderDraft } from "../providerSnapshot.ts"; import { mergeProviderInstanceEnvironment } from "../ProviderInstanceEnvironment.ts"; +import { ProviderSecretResolver } from "../Services/ProviderSecretResolver.ts"; import { enrichProviderSnapshotWithVersionAdvisory, makePackageManagedProviderMaintenanceResolver, @@ -80,6 +81,7 @@ export type CodexDriverEnv = | HttpClient.HttpClient | Path.Path | ProviderEventLoggers + | ProviderSecretResolver | ServerConfig | ServerSettingsService; @@ -119,7 +121,10 @@ export const CodexDriver: ProviderDriver = { const httpClient = yield* HttpClient.HttpClient; const serverSettings = yield* ServerSettingsService; const eventLoggers = yield* ProviderEventLoggers; - const processEnv = mergeProviderInstanceEnvironment(environment); + const secretResolver = yield* ProviderSecretResolver; + const processEnv = mergeProviderInstanceEnvironment( + yield* secretResolver.resolve(environment), + ); const homeLayout = yield* resolveCodexHomeLayout(config); const continuationIdentity = codexContinuationIdentity(homeLayout); const stampIdentity = withInstanceIdentity({ diff --git a/apps/server/src/provider/Drivers/CursorDriver.ts b/apps/server/src/provider/Drivers/CursorDriver.ts index 2101664d5cb1..b140983aea84 100644 --- a/apps/server/src/provider/Drivers/CursorDriver.ts +++ b/apps/server/src/provider/Drivers/CursorDriver.ts @@ -40,6 +40,7 @@ import { } from "../ProviderDriver.ts"; import type { ServerProviderDraft } from "../providerSnapshot.ts"; import { mergeProviderInstanceEnvironment } from "../ProviderInstanceEnvironment.ts"; +import { ProviderSecretResolver } from "../Services/ProviderSecretResolver.ts"; import { makeProviderMaintenanceCapabilities, type ProviderMaintenanceCapabilitiesResolver, @@ -72,6 +73,7 @@ export type CursorDriverEnv = | HttpClient.HttpClient | Path.Path | ProviderEventLoggers + | ProviderSecretResolver | ServerConfig | ServerSettingsService; @@ -108,7 +110,10 @@ export const CursorDriver: ProviderDriver = { const httpClient = yield* HttpClient.HttpClient; const serverSettings = yield* ServerSettingsService; const eventLoggers = yield* ProviderEventLoggers; - const processEnv = mergeProviderInstanceEnvironment(environment); + const secretResolver = yield* ProviderSecretResolver; + const processEnv = mergeProviderInstanceEnvironment( + yield* secretResolver.resolve(environment), + ); const continuationIdentity = defaultProviderContinuationIdentity({ driverKind: DRIVER_KIND, instanceId, diff --git a/apps/server/src/provider/Drivers/GrokDriver.ts b/apps/server/src/provider/Drivers/GrokDriver.ts index 112f11013161..e218a0590610 100644 --- a/apps/server/src/provider/Drivers/GrokDriver.ts +++ b/apps/server/src/provider/Drivers/GrokDriver.ts @@ -27,6 +27,7 @@ import { } from "../ProviderDriver.ts"; import type { ServerProviderDraft } from "../providerSnapshot.ts"; import { mergeProviderInstanceEnvironment } from "../ProviderInstanceEnvironment.ts"; +import { ProviderSecretResolver } from "../Services/ProviderSecretResolver.ts"; import { makeManualOnlyProviderMaintenanceCapabilities, makeStaticProviderMaintenanceResolver, @@ -55,6 +56,7 @@ export type GrokDriverEnv = | HttpClient.HttpClient | Path.Path | ProviderEventLoggers + | ProviderSecretResolver | ServerConfig | ServerSettingsService; @@ -89,7 +91,10 @@ export const GrokDriver: ProviderDriver = { const httpClient = yield* HttpClient.HttpClient; const serverSettings = yield* ServerSettingsService; const eventLoggers = yield* ProviderEventLoggers; - const processEnv = mergeProviderInstanceEnvironment(environment); + const secretResolver = yield* ProviderSecretResolver; + const processEnv = mergeProviderInstanceEnvironment( + yield* secretResolver.resolve(environment), + ); const continuationIdentity = defaultProviderContinuationIdentity({ driverKind: DRIVER_KIND, instanceId, diff --git a/apps/server/src/provider/Drivers/OpenCodeDriver.ts b/apps/server/src/provider/Drivers/OpenCodeDriver.ts index a01e414f8116..552d77905349 100644 --- a/apps/server/src/provider/Drivers/OpenCodeDriver.ts +++ b/apps/server/src/provider/Drivers/OpenCodeDriver.ts @@ -41,6 +41,7 @@ import { } from "../ProviderDriver.ts"; import type { ServerProviderDraft } from "../providerSnapshot.ts"; import { mergeProviderInstanceEnvironment } from "../ProviderInstanceEnvironment.ts"; +import { ProviderSecretResolver } from "../Services/ProviderSecretResolver.ts"; import { enrichProviderSnapshotWithVersionAdvisory, makePackageManagedProviderMaintenanceResolver, @@ -85,6 +86,7 @@ export type OpenCodeDriverEnv = | OpenCodeRuntime | Path.Path | ProviderEventLoggers + | ProviderSecretResolver | ServerConfig | ServerSettingsService; @@ -119,7 +121,10 @@ export const OpenCodeDriver: ProviderDriver const httpClient = yield* HttpClient.HttpClient; const serverSettings = yield* ServerSettingsService; const eventLoggers = yield* ProviderEventLoggers; - const processEnv = mergeProviderInstanceEnvironment(environment); + const secretResolver = yield* ProviderSecretResolver; + const processEnv = mergeProviderInstanceEnvironment( + yield* secretResolver.resolve(environment), + ); const continuationIdentity = defaultProviderContinuationIdentity({ driverKind: DRIVER_KIND, instanceId, diff --git a/apps/server/src/provider/Layers/ProviderAdapterRegistry.test.ts b/apps/server/src/provider/Layers/ProviderAdapterRegistry.test.ts index c4145ecf1a0e..e5309ff72a6c 100644 --- a/apps/server/src/provider/Layers/ProviderAdapterRegistry.test.ts +++ b/apps/server/src/provider/Layers/ProviderAdapterRegistry.test.ts @@ -140,6 +140,7 @@ const fakeInstanceRegistryLayer = Layer.succeed(ProviderInstanceRegistry.Provide Effect.succeed(fakeInstances.find((instance) => instance.instanceId === instanceId)), listInstances: Effect.succeed(fakeInstances), listUnavailable: Effect.succeed([]), + rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.empty, // Tests never drive changes through this fake; acquire a throwaway // subscription on an unused PubSub so the shape is satisfied. diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts index dea384104964..9ec58cd0b029 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts @@ -44,6 +44,7 @@ import * as BackgroundPolicy from "../../background/BackgroundPolicy.ts"; import { ServerConfig } from "../../config.ts"; import { ServerSettingsService } from "../../serverSettings.ts"; import { ClaudeDriver } from "../Drivers/ClaudeDriver.ts"; +import { ProviderSecretResolverPassthroughLayer } from "../Services/ProviderSecretResolver.ts"; import { CodexDriver } from "../Drivers/CodexDriver.ts"; import { CursorDriver } from "../Drivers/CursorDriver.ts"; import { GrokDriver } from "../Drivers/GrokDriver.ts"; @@ -148,6 +149,7 @@ describe("ProviderInstanceRegistryLive — multi-instance codex slice", () => { Layer.provideMerge(ServerSettingsService.layerTest()), Layer.provideMerge(TestHttpClientLive), Layer.provideMerge(Layer.succeed(ProviderEventLoggers, NoOpProviderEventLoggers)), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ); it.live("boots two independent codex instances from a ProviderInstanceConfigMap", () => @@ -313,6 +315,7 @@ describe("ProviderInstanceRegistryLive — all drivers slice", () => { Layer.provideMerge(ServerSettingsService.layerTest()), Layer.provideMerge(TestHttpClientLive), Layer.provideMerge(Layer.succeed(ProviderEventLoggers, NoOpProviderEventLoggers)), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ); it.live("boots one instance of every shipped driver from a single config map", () => @@ -472,3 +475,91 @@ describe("ProviderInstanceRegistryLive — all drivers slice", () => { }).pipe(Effect.provide(testLayer)), ); }); + +describe("ProviderInstanceRegistryLive: rebuildInstanceWhen", () => { + const testLayer = ServerConfig.layerTest(process.cwd(), { + prefix: "provider-instance-registry-rebuild-test", + }).pipe( + Layer.provideMerge(NodeServices.layer), + Layer.provideMerge(BackgroundPolicyAlwaysRunLayer), + Layer.provideMerge(ServerSettingsService.layerTest()), + Layer.provideMerge(TestHttpClientLive), + Layer.provideMerge(Layer.succeed(ProviderEventLoggers, NoOpProviderEventLoggers)), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), + ); + + const codexDriverKind = ProviderDriverKind.make("codex"); + const firstId = ProviderInstanceId.make("codex_first"); + const secondId = ProviderInstanceId.make("codex_second"); + const configMap: ProviderInstanceConfigMap = { + [firstId]: { + driver: codexDriverKind, + displayName: "Codex (first)", + enabled: false, + environment: [{ name: "OP_TOKEN", value: "op://Vault/Item/token", sensitive: true }], + config: makeCodexConfig({ homePath: "/home/julius/.codex_first" }), + }, + [secondId]: { + driver: codexDriverKind, + displayName: "Codex (second)", + enabled: false, + config: makeCodexConfig({ homePath: "/home/julius/.codex_second" }), + }, + }; + + it.live("replaces only the instance the predicate accepts, in place", () => + Effect.gen(function* () { + const { registry } = yield* makeProviderInstanceRegistry({ + drivers: [CodexDriver], + configMap, + }); + const before = yield* registry.listInstances; + + const rebuilt = yield* registry.rebuildInstanceWhen( + firstId, + (entry) => entry.environment !== undefined, + ); + + expect(rebuilt).toBe(true); + const after = yield* registry.listInstances; + // Order is settings-author order, not "rebuilt last". + expect(after.map((instance) => instance.instanceId)).toEqual([firstId, secondId]); + // The accepted instance is a genuinely new bundle; its neighbour is + // untouched, which is what keeps a refresh from restarting every + // provider on the machine. + expect(after[0]).not.toBe(before[0]); + expect(after[1]).toBe(before[1]); + }).pipe(Effect.provide(testLayer)), + ); + + it.live("leaves the instance alone when the predicate declines", () => + Effect.gen(function* () { + const { registry } = yield* makeProviderInstanceRegistry({ + drivers: [CodexDriver], + configMap, + }); + const before = yield* registry.listInstances; + + const rebuilt = yield* registry.rebuildInstanceWhen(secondId, () => false); + + expect(rebuilt).toBe(false); + expect(yield* registry.listInstances).toEqual(before); + }).pipe(Effect.provide(testLayer)), + ); + + it.live("treats an unknown instance id as a no-op", () => + Effect.gen(function* () { + const { registry } = yield* makeProviderInstanceRegistry({ + drivers: [CodexDriver], + configMap, + }); + + const rebuilt = yield* registry.rebuildInstanceWhen( + ProviderInstanceId.make("codex_missing"), + () => true, + ); + + expect(rebuilt).toBe(false); + }).pipe(Effect.provide(testLayer)), + ); +}); diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts index fb75652e3856..9b3c19d16566 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts @@ -378,6 +378,49 @@ export const makeProviderInstanceRegistry = (input: { // `listInstances` immediately after this effect completes. yield* reconcile(input.configMap); + // Rebuild one instance from the config it already has. Unlike + // `reconcile`, which is driven by a settings diff, this is for inputs the + // envelope cannot show, see the shape docs. Order is preserved by + // rewriting the map in place rather than appending the replacement. + const rebuildInstanceWhen: ProviderInstanceRegistryShape["rebuildInstanceWhen"] = ( + instanceId, + shouldRebuild, + ) => + Effect.gen(function* () { + const previousEntries = yield* Ref.get(entries); + const live = previousEntries.get(instanceId); + if (live === undefined || !shouldRebuild(live.entry)) { + return false; + } + + yield* Scope.close(live.scope, Exit.void).pipe(Effect.ignore); + const result = yield* buildEntry({ + driversById, + parentScope, + instanceId, + rawInstanceId: instanceId, + entry: live.entry, + }); + + const nextEntries = new Map(); + for (const [id, existing] of previousEntries) { + if (id !== instanceId) { + nextEntries.set(id, existing); + } else if (result.kind === "live") { + nextEntries.set(id, result.live); + } + } + yield* Ref.set(entries, nextEntries); + if (result.kind === "unavailable") { + yield* Ref.update(unavailable, (previous) => + new Map(previous).set(instanceId, result.snapshot), + ); + } + + yield* PubSub.publish(changes, undefined); + return true; + }).pipe(Effect.provideContext(driverContext)); + const registry: ProviderInstanceRegistryShape = { getInstance: (id) => Ref.get(entries).pipe(Effect.map((map) => map.get(id)?.instance)), listInstances: Ref.get(entries).pipe( @@ -389,6 +432,7 @@ export const makeProviderInstanceRegistry = (input: { listUnavailable: Ref.get(unavailable).pipe( Effect.map((map) => Array.from(map.values()) as ReadonlyArray), ), + rebuildInstanceWhen, // Getters: each read constructs a fresh Stream / Effect descriptor // so multiple consumers don't share a single already-started // Channel or subscription. Matches the pattern `ProviderRegistry` diff --git a/apps/server/src/provider/Layers/ProviderRegistry.test.ts b/apps/server/src/provider/Layers/ProviderRegistry.test.ts index 1ceb1c00c662..144ea34e00e1 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.test.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.test.ts @@ -37,6 +37,10 @@ import * as BackgroundPolicy from "../../background/BackgroundPolicy.ts"; import * as OpenCodeRuntime from "../opencodeRuntime.ts"; import * as ProviderEventLoggers from "./ProviderEventLoggers.ts"; import { ProviderInstanceRegistryHydrationLive } from "./ProviderInstanceRegistryHydration.ts"; +import { + ProviderSecretResolver, + ProviderSecretResolverPassthroughLayer, +} from "../Services/ProviderSecretResolver.ts"; import { haveProvidersChanged, mergeProviderSnapshot, @@ -868,6 +872,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te Effect.succeed(instanceId === codexInstanceId ? instance : undefined), listInstances: Effect.succeed([instance]), listUnavailable: Effect.succeed([]), + rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.empty, subscribeChanges: Effect.flatMap(PubSub.unbounded(), PubSub.subscribe), }, @@ -883,6 +888,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te }), ), Layer.provideMerge(NodeServices.layer), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ), ).pipe(Scope.provide(scope)); yield* Effect.gen(function* () { @@ -893,6 +899,105 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te }), ); + // Refresh is the only moment a rotated 1Password secret can reach a + // provider: the environment is resolved once, when the driver builds + // the instance, and the process it spawns keeps that copy. So refresh + // has to drop the cached secret AND rebuild the instances that read + // one - a re-probe of the existing process would report the old + // credential right back. + it.effect("drops cached secrets and rebuilds the instances that read them", () => + Effect.gen(function* () { + const codexDriver = ProviderDriverKind.make("codex"); + const codexInstanceId = ProviderInstanceId.make("codex"); + const initialProvider = { + instanceId: codexInstanceId, + driver: codexDriver, + status: "warning", + enabled: true, + installed: false, + auth: { status: "unknown" }, + checkedAt: "2026-06-10T00:00:00.000Z", + version: null, + models: [], + slashCommands: [], + skills: [], + } as const satisfies ServerProvider; + const instance = { + instanceId: codexInstanceId, + driverKind: codexDriver, + continuationIdentity: { + driverKind: codexDriver, + continuationKey: "codex:instance:codex", + }, + displayName: undefined, + enabled: true, + snapshot: { + maintenanceCapabilities: makeManualOnlyProviderMaintenanceCapabilities({ + provider: codexDriver, + packageName: null, + }), + getSnapshot: Effect.succeed(initialProvider), + refresh: Effect.succeed(initialProvider), + streamChanges: Stream.empty, + }, + adapter: {} as ProviderInstance["adapter"], + textGeneration: {} as ProviderInstance["textGeneration"], + } satisfies ProviderInstance; + + const invalidations = yield* Ref.make(0); + const rebuiltIds = yield* Ref.make>([]); + const secretResolverLayer = Layer.succeed(ProviderSecretResolver, { + resolve: Effect.succeed, + invalidate: Ref.update(invalidations, (count) => count + 1), + }); + const instanceRegistryLayer = Layer.succeed( + ProviderInstanceRegistry.ProviderInstanceRegistry, + { + getInstance: (instanceId) => + Effect.succeed(instanceId === codexInstanceId ? instance : undefined), + listInstances: Effect.succeed([instance]), + listUnavailable: Effect.succeed([]), + rebuildInstanceWhen: (instanceId, shouldRebuild) => + shouldRebuild({ + driver: codexDriver, + environment: [ + { name: "CODEX_TOKEN", value: "op://Vault/Item/token", sensitive: true }, + ], + }) + ? 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-refresh-", + }), + ), + Layer.provideMerge(NodeServices.layer), + Layer.provideMerge(secretResolverLayer), + ), + ).pipe(Scope.provide(scope)); + + yield* Effect.gen(function* () { + const registry = yield* ProviderRegistry.ProviderRegistry; + yield* registry.refreshInstance(codexInstanceId); + + assert.strictEqual(yield* Ref.get(invalidations), 1); + assert.deepStrictEqual(yield* Ref.get(rebuiltIds), [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"); @@ -957,6 +1062,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te Effect.succeed(instanceId === cursorInstanceId ? instance : undefined), listInstances: Effect.succeed([instance]), listUnavailable: Effect.succeed([]), + rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.empty, subscribeChanges: Effect.flatMap(PubSub.unbounded(), (pubsub) => PubSub.subscribe(pubsub), @@ -975,6 +1081,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te ), Layer.provideMerge(BackgroundPolicyAlwaysRunLayer), Layer.provideMerge(NodeServices.layer), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ), ).pipe(Scope.provide(scope)); @@ -1086,6 +1193,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te Effect.succeed(instanceId === openCodeInstanceId ? instance : undefined), listInstances: Effect.succeed([instance]), listUnavailable: Effect.succeed([]), + rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.empty, subscribeChanges: Effect.flatMap(PubSub.unbounded(), (pubsub) => PubSub.subscribe(pubsub), @@ -1103,6 +1211,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te }), ), Layer.provideMerge(NodeServices.layer), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ), ).pipe(Scope.provide(scope)); @@ -1193,6 +1302,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te Effect.succeed(instanceId === codexInstanceId ? instance : undefined), listInstances: Effect.succeed([instance]), listUnavailable: Effect.succeed([]), + rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.empty, subscribeChanges: Effect.flatMap(PubSub.unbounded(), (pubsub) => PubSub.subscribe(pubsub), @@ -1211,6 +1321,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te ), Layer.provideMerge(BackgroundPolicyAlwaysRunLayer), Layer.provideMerge(NodeServices.layer), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ), ).pipe(Scope.provide(scope)); @@ -1303,6 +1414,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te return yield* Ref.get(instancesRef); }), listUnavailable: Effect.succeed([]), + rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.fromPubSub(changes), subscribeChanges: PubSub.subscribe(changes), }, @@ -1319,6 +1431,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te ), Layer.provideMerge(BackgroundPolicyAlwaysRunLayer), Layer.provideMerge(NodeServices.layer), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ), ).pipe(Scope.provide(scope)); @@ -1428,6 +1541,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te // genuinely spawn a subprocess. The missing-binary ENOENT is // what exercises the same failure mode as a misconfigured // production `binaryPath`. + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ); const runtimeServices = yield* Layer.build(providerRegistryLayer).pipe( Scope.provide(scope), @@ -1523,6 +1637,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te ), Layer.provideMerge(NodeServices.layer), Layer.provideMerge(BackgroundPolicyAlwaysRunLayer), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ); const runtimeServices = yield* Layer.build(providerRegistryLayer).pipe( Scope.provide(scope), @@ -1639,6 +1754,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te Layer.provideMerge(OpenCodeRuntime.OpenCodeRuntimeLive), Layer.provideMerge(NodeServices.layer), Layer.provideMerge(BackgroundPolicyAlwaysRunLayer), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ); const runtimeServices = yield* Layer.build(providerRegistryLayer).pipe( Scope.provide(scope), @@ -1723,6 +1839,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te throw new Error(`Unexpected args: ${command} ${joined}`); }), ), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ); const runtimeServices = yield* Layer.build( Layer.mergeAll( diff --git a/apps/server/src/provider/Layers/ProviderRegistry.ts b/apps/server/src/provider/Layers/ProviderRegistry.ts index 760c8e1c59e8..dfa40d6fa2cd 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.ts @@ -37,11 +37,14 @@ import * as Layer from "effect/Layer"; import * as Path from "effect/Path"; import * as PubSub from "effect/PubSub"; import * as Ref from "effect/Ref"; +import * as Scope from "effect/Scope"; import * as Stream from "effect/Stream"; 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 { ProviderRegistry, type ProviderRegistryShape } from "../Services/ProviderRegistry.ts"; import { hydrateCachedProvider, @@ -210,6 +213,11 @@ export const ProviderRegistryLive = Layer.effect( ProviderRegistry, Effect.gen(function* () { const instanceRegistry = yield* ProviderInstanceRegistry; + const secretResolver = yield* ProviderSecretResolver; + // The layer's own scope. `syncLiveSources` forks per-instance + // subscription fibres into it; when a refresh drives that sync, the + // fibres have to land here rather than in the caller's request scope. + const layerScope = yield* Scope.Scope; const config = yield* ServerConfig; const fileSystem = yield* FileSystem.FileSystem; const path = yield* Path.Path; @@ -462,6 +470,7 @@ export const ProviderRegistryLive = Layer.effect( }); const refreshAll = Effect.fn("refreshAll")(function* () { + yield* reloadSecretBackedInstances(); const sources = yield* getLiveSources; return yield* Effect.forEach(sources, (source) => refreshOneSource(source), { concurrency: "unbounded", @@ -475,6 +484,7 @@ export const ProviderRegistryLive = Layer.effect( } // Kind-scoped refreshes target the default instance for that driver. const defaultInstanceId = defaultInstanceIdForDriver(provider); + yield* reloadSecretBackedInstances([defaultInstanceId]); const sources = yield* getLiveSources; const providerSource = sources.find( (candidate) => candidate.instanceId === defaultInstanceId, @@ -488,6 +498,7 @@ export const ProviderRegistryLive = Layer.effect( const refreshInstance = Effect.fn("refreshInstance")(function* ( instanceId: ProviderInstanceId, ) { + yield* reloadSecretBackedInstances([instanceId]); const sources = yield* getLiveSources; const providerSource = sources.find((candidate) => candidate.instanceId === instanceId); if (!providerSource) { @@ -629,6 +640,38 @@ export const ProviderRegistryLive = Layer.effect( }); }), ); + /** + * Re-read every credential that lives in an external secret store and + * rebuild the instances that consume one. + * + * A refresh is the user's way of saying "go look again", so it is also + * where a rotated secret should take effect. Dropping the cached value + * alone would change nothing: a driver resolves its environment once, at + * create time, and the provider process it spawns inherits that copy. The + * instance has to be rebuilt for the new value to reach the next process. + * + * Instances whose environment is all literals are left alone, so the + * common refresh stays a plain re-probe. Threads already running keep the + * process they were started with. + */ + const reloadSecretBackedInstances = Effect.fn("reloadSecretBackedInstances")(function* ( + instanceIds?: ReadonlyArray, + ) { + yield* secretResolver.invalidate; + const targets = instanceIds ?? [...(yield* Ref.get(liveSubsRef)).keys()]; + const rebuilt = yield* Effect.forEach(targets, (instanceId) => + instanceRegistry.rebuildInstanceWhen(instanceId, (entry) => + hasProviderSecretReference(entry.environment), + ), + ); + if (rebuilt.some(Boolean)) { + // Adopt the replacement instances now rather than waiting on the + // registry's change tick, so the refresh that triggered the rebuild + // probes the new process instead of the one it just closed. + yield* syncLiveSources.pipe(Effect.provideService(Scope.Scope, layerScope)); + } + }); + const syncLiveSourcesAndContinue = syncLiveSources.pipe( Effect.catchCause((cause) => { if (Cause.hasInterruptsOnly(cause)) { diff --git a/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts b/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts new file mode 100644 index 000000000000..fab183166e2e --- /dev/null +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts @@ -0,0 +1,149 @@ +import { describe, it, assert } from "@effect/vitest"; +import { ProviderInstanceEnvironment } from "@t3tools/contracts"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; +import * as Schema from "effect/Schema"; +import * as Sink from "effect/Sink"; +import * as Stream from "effect/Stream"; +import { ChildProcessSpawner } from "effect/unstable/process"; + +import { ProviderSecretResolverLive } from "./ProviderSecretResolverLive.ts"; +import { ProviderSecretResolver } from "../Services/ProviderSecretResolver.ts"; + +const encoder = new TextEncoder(); +const decodeEnvironment = Schema.decodeSync(ProviderInstanceEnvironment); + +const TOKEN_REFERENCE = "op://Private/claude-code/credential"; + +/** + * Spawner that answers every `op read` with `result` and records the argv it + * was handed, so tests can assert both the substituted value and how many + * times 1Password was actually consulted. + */ +function recordingOpSpawner(result: { stdout: string; stderr: string; code: number }) { + const invocations: Array> = []; + const layer = Layer.succeed( + ChildProcessSpawner.ChildProcessSpawner, + ChildProcessSpawner.make((command) => { + const cmd = command as unknown as { args: ReadonlyArray }; + invocations.push(cmd.args); + return Effect.succeed( + 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 }; +} + +describe("ProviderSecretResolverLive", () => { + it.effect("leaves an environment of literal values alone", () => { + const spawner = recordingOpSpawner({ stdout: "", stderr: "", code: 0 }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + const environment = decodeEnvironment([ + { name: "CLAUDE_SECURESTORAGE_CONFIG_DIR", value: "/home/u/.claude/work" }, + ]); + + const resolved = yield* resolver.resolve(environment); + + assert.deepStrictEqual(resolved, environment); + assert.strictEqual(spawner.invocations.length, 0); + }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }); + + it.effect("swaps a secret reference for the value 1Password returns", () => { + const spawner = recordingOpSpawner({ stdout: "sk-live-token\n", stderr: "", code: 0 }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + + const resolved = yield* resolver.resolve( + decodeEnvironment([ + { name: "CLAUDE_SECURESTORAGE_CONFIG_DIR", value: "/home/u/.claude/work" }, + { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + ]), + ); + + assert.deepStrictEqual( + resolved?.map((variable) => [variable.name, variable.value]), + [ + ["CLAUDE_SECURESTORAGE_CONFIG_DIR", "/home/u/.claude/work"], + ["CLAUDE_CODE_OAUTH_TOKEN", "sk-live-token"], + ], + ); + assert.deepStrictEqual(spawner.invocations, [["read", "--no-newline", TOKEN_REFERENCE]]); + }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }); + + it.effect("reads a reference once and holds it until the caller invalidates", () => { + const spawner = recordingOpSpawner({ stdout: "sk-live-token", stderr: "", code: 0 }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + const environment = decodeEnvironment([ + { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + ]); + + // Every thread start and every instance rebuild resolves again; none of + // them should reach 1Password while the value is already in memory. + yield* resolver.resolve(environment); + yield* resolver.resolve(environment); + assert.strictEqual(spawner.invocations.length, 1); + + yield* resolver.invalidate; + yield* resolver.resolve(environment); + assert.strictEqual(spawner.invocations.length, 2); + }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }); + + it.effect("drops a variable whose reference cannot be read", () => { + const spawner = recordingOpSpawner({ + stdout: "", + stderr: "[ERROR] could not read secret: not signed in", + code: 1, + }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + + // An empty string here would look like a present credential and send + // the provider off to fail mid-turn. Absence is the honest answer, and + // the provider reports itself unauthenticated instead. + const resolved = yield* resolver.resolve( + decodeEnvironment([ + { name: "CLAUDE_SECURESTORAGE_CONFIG_DIR", value: "/home/u/.claude/work" }, + { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + ]), + ); + + assert.deepStrictEqual( + resolved?.map((variable) => variable.name), + ["CLAUDE_SECURESTORAGE_CONFIG_DIR"], + ); + }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); + }); + + it.effect("holds a failed read too, so a locked vault prompts once", () => { + const spawner = recordingOpSpawner({ stdout: "", stderr: "not signed in", code: 1 }); + return Effect.gen(function* () { + const resolver = yield* ProviderSecretResolver; + const environment = decodeEnvironment([ + { name: "CLAUDE_CODE_OAUTH_TOKEN", value: TOKEN_REFERENCE, sensitive: true }, + ]); + + yield* resolver.resolve(environment); + yield* resolver.resolve(environment); + + assert.strictEqual(spawner.invocations.length, 1); + }).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 new file mode 100644 index 000000000000..fbd137f7bdcb --- /dev/null +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts @@ -0,0 +1,125 @@ +/** + * ProviderSecretResolverLive: 1Password-backed implementation of + * `ProviderSecretResolver`. + * + * Resolution is `op read `, which is the same command the user + * would run by hand and inherits their existing `op` session, so there is no + * second place to configure credentials. Reads run one at a time: two + * concurrent reads against a locked vault stack up two biometric prompts. + * + * 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 + * the refresh button, which is where the user already looks when a provider + * shows as logged out. + * + * @module provider/Layers/ProviderSecretResolverLive + */ +import type { + ProviderInstanceEnvironment, + ProviderInstanceEnvironmentVariable, +} from "@t3tools/contracts"; +import * as Cache from "effect/Cache"; +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 { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"; +import { resolveSpawnCommand } from "@t3tools/shared/shell"; + +import { hasProviderSecretReference, providerSecretReference } from "../ProviderSecretReference.ts"; +import { spawnAndCollect } from "../providerSnapshot.ts"; +import { + ProviderSecretResolver, + type ProviderSecretResolverShape, +} from "../Services/ProviderSecretResolver.ts"; + +const ONE_PASSWORD_BINARY = "op"; + +/** + * Bound on a single `op read`. Long enough for a user to reach for the + * fingerprint reader, short enough that a vault that will never answer does + * not wedge the instance rebuild that is waiting on it. + */ +const SECRET_READ_TIMEOUT = Duration.seconds(45); + +/** + * References are small and few - one per provider credential. The cap only + * exists so a settings file that somehow accumulates hundreds cannot pin + * every secret it ever mentioned in memory. + */ +const SECRET_CACHE_CAPACITY = 64; + +const readSecret = Effect.fn("readSecret")(function* (reference: string) { + const spawnCommand = yield* resolveSpawnCommand(ONE_PASSWORD_BINARY, [ + "read", + "--no-newline", + reference, + ]); + const result = yield* spawnAndCollect( + ONE_PASSWORD_BINARY, + ChildProcess.make(spawnCommand.command, spawnCommand.args, { shell: spawnCommand.shell }), + ); + if (result.code !== 0) { + // `op` reports "not signed in", "item not found", and friends on stderr + // and never echoes the secret itself, so this is safe to log verbatim. + yield* Effect.logWarning("Could not read provider secret from 1Password", { + reference, + exitCode: result.code, + detail: result.stderr.trim(), + }); + return undefined; + } + const secret = result.stdout.trim(); + return secret.length > 0 ? secret : undefined; +}); + +export const ProviderSecretResolverLive: Layer.Layer< + ProviderSecretResolver, + never, + ChildProcessSpawner.ChildProcessSpawner +> = Layer.effect( + ProviderSecretResolver, + Effect.gen(function* () { + const cache = yield* Cache.make({ + capacity: SECRET_CACHE_CAPACITY, + // No time to live: a resolved secret is held until the user asks for a + // provider refresh. Expiring on a timer would reintroduce the surprise + // biometric prompt mid-session that the cache exists to remove. + lookup: (reference: string) => + readSecret(reference).pipe( + Effect.timeoutOption(SECRET_READ_TIMEOUT), + Effect.map(Option.getOrUndefined), + Effect.catch((error) => + Effect.logWarning("Could not run 1Password to read a provider secret", { + reference, + detail: String(error), + }).pipe(Effect.as(undefined)), + ), + ), + }); + + const resolve: ProviderSecretResolverShape["resolve"] = (environment) => + Effect.gen(function* () { + if (!hasProviderSecretReference(environment)) { + return environment; + } + const resolved: Array = []; + for (const variable of environment ?? []) { + const reference = providerSecretReference(variable.value); + if (reference === undefined) { + resolved.push(variable); + continue; + } + const secret = yield* Cache.get(cache, reference); + if (secret === undefined) { + continue; + } + resolved.push({ ...variable, value: secret }); + } + return resolved as ProviderInstanceEnvironment; + }); + + return { resolve, invalidate: Cache.invalidateAll(cache) }; + }), +); diff --git a/apps/server/src/provider/ProviderSecretReference.test.ts b/apps/server/src/provider/ProviderSecretReference.test.ts new file mode 100644 index 000000000000..abbcf48b64ea --- /dev/null +++ b/apps/server/src/provider/ProviderSecretReference.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from "@effect/vitest"; +import { ProviderInstanceEnvironment } from "@t3tools/contracts"; +import * as Schema from "effect/Schema"; + +import { hasProviderSecretReference, providerSecretReference } from "./ProviderSecretReference.ts"; + +const decodeEnvironment = Schema.decodeSync(ProviderInstanceEnvironment); + +describe("providerSecretReference", () => { + it("reads a 1Password reference", () => { + expect(providerSecretReference("op://Private/claude-code/credential")).toBe( + "op://Private/claude-code/credential", + ); + }); + + it("trims a reference pasted with surrounding whitespace", () => { + expect(providerSecretReference(" op://Private/claude-code/credential\n")).toBe( + "op://Private/claude-code/credential", + ); + }); + + it("treats a literal value as a literal", () => { + expect(providerSecretReference("sk-live-token")).toBeUndefined(); + expect(providerSecretReference("/home/u/.claude/work")).toBeUndefined(); + }); + + it("ignores a bare scheme with nothing behind it", () => { + expect(providerSecretReference("op://")).toBeUndefined(); + }); +}); + +describe("hasProviderSecretReference", () => { + it("is false for an absent or empty environment", () => { + expect(hasProviderSecretReference(undefined)).toBe(false); + expect(hasProviderSecretReference([])).toBe(false); + }); + + it("is true when any single variable reads from the store", () => { + expect( + hasProviderSecretReference( + decodeEnvironment([ + { name: "CLAUDE_SECURESTORAGE_CONFIG_DIR", value: "/home/u/.claude/work" }, + { name: "CLAUDE_CODE_OAUTH_TOKEN", value: "op://Private/claude-code/credential" }, + ]), + ), + ).toBe(true); + }); + + it("is false when every variable is a literal", () => { + expect( + hasProviderSecretReference( + decodeEnvironment([ + { name: "CLAUDE_SECURESTORAGE_CONFIG_DIR", value: "/home/u/.claude/work" }, + ]), + ), + ).toBe(false); + }); +}); diff --git a/apps/server/src/provider/ProviderSecretReference.ts b/apps/server/src/provider/ProviderSecretReference.ts new file mode 100644 index 000000000000..6900c1b6dc45 --- /dev/null +++ b/apps/server/src/provider/ProviderSecretReference.ts @@ -0,0 +1,46 @@ +/** + * Provider environment values that name a secret instead of carrying one. + * + * A user who keeps a provider credential in 1Password can paste the item's + * secret reference (`op://Vault/Item/field`) as an environment variable's + * value instead of the secret itself. `ProviderSecretResolver` swaps the + * reference for the real value on the way into the provider process, so the + * credential never lands in `settings.json` or the on-disk secret store, and + * rotating it in 1Password rotates it here. + * + * These helpers are pure so the instance registry can ask "does this + * environment read from a secret store?" without depending on the resolver. + * + * @module provider/ProviderSecretReference + */ +import type { ProviderInstanceEnvironment } from "@t3tools/contracts"; + +/** URI scheme 1Password uses for secret references; `op read` consumes these. */ +export const PROVIDER_SECRET_REFERENCE_PREFIX = "op://"; + +/** + * The secret reference an environment value names, or `undefined` when the + * value is a literal. The value is trimmed first: a reference copied out of a + * password manager routinely arrives with surrounding whitespace, which `op` + * rejects. + */ +export function providerSecretReference(value: string): string | undefined { + const trimmed = value.trim(); + if (!trimmed.startsWith(PROVIDER_SECRET_REFERENCE_PREFIX)) { + return undefined; + } + return trimmed.length > PROVIDER_SECRET_REFERENCE_PREFIX.length ? trimmed : undefined; +} + +/** + * Whether any variable in the environment reads from a secret store. Drives + * the "rebuild this instance on refresh" decision, so instances configured + * entirely with literals keep the process they already have. + */ +export function hasProviderSecretReference( + environment: ProviderInstanceEnvironment | undefined, +): boolean { + return ( + environment?.some((variable) => providerSecretReference(variable.value) !== undefined) ?? false + ); +} diff --git a/apps/server/src/provider/Services/ProviderInstanceRegistry.ts b/apps/server/src/provider/Services/ProviderInstanceRegistry.ts index cfea11426664..e57fc5e89a40 100644 --- a/apps/server/src/provider/Services/ProviderInstanceRegistry.ts +++ b/apps/server/src/provider/Services/ProviderInstanceRegistry.ts @@ -17,7 +17,11 @@ * * @module provider/Services/ProviderInstanceRegistry */ -import type { ProviderInstanceId, ServerProvider } from "@t3tools/contracts"; +import type { + ProviderInstanceConfig, + ProviderInstanceId, + ServerProvider, +} from "@t3tools/contracts"; import * as Context from "effect/Context"; import type * as Effect from "effect/Effect"; import type * as PubSub from "effect/PubSub"; @@ -46,6 +50,27 @@ export interface ProviderInstanceRegistryShape { * directly into `ProviderRegistry` output. */ readonly listUnavailable: Effect.Effect>; + /** + * Tear one instance down and build it again from the configuration it + * already has, but only when `shouldRebuild` accepts that configuration. + * Resolves to whether a rebuild happened. + * + * This exists for inputs a driver reads at create time that can change + * without the settings file changing - a credential held in an external + * secret store, say. `reconcile` cannot see those, because the config + * envelope it diffs is identical before and after. The predicate keeps the + * decision with the caller that understands the input, and keeps every + * other instance out of the blast radius. + * + * The rebuilt instance keeps its position in settings-author order, and an + * unknown id is a no-op rather than an error. Sessions already talking to + * the old instance keep the process they were given; only work started + * after the rebuild sees the new one. + */ + readonly rebuildInstanceWhen: ( + instanceId: ProviderInstanceId, + shouldRebuild: (entry: ProviderInstanceConfig) => boolean, + ) => Effect.Effect; /** * Push notification stream emitted whenever the registry's contents * change — instance added, removed, or rebuilt. The payload is `void` diff --git a/apps/server/src/provider/Services/ProviderSecretResolver.ts b/apps/server/src/provider/Services/ProviderSecretResolver.ts new file mode 100644 index 000000000000..d7f663aefee5 --- /dev/null +++ b/apps/server/src/provider/Services/ProviderSecretResolver.ts @@ -0,0 +1,61 @@ +/** + * ProviderSecretResolver: turns `op://` environment values into the secrets + * they name, once, and holds them in memory. + * + * Every provider instance resolves its environment when the driver builds it, + * and a single instance can rebuild several times per session. Shelling out + * to `op` on each of those is slow (seconds) and, worse, can put a biometric + * prompt in front of a user who only started a thread. The resolver therefore + * caches by reference for the lifetime of the process; `invalidate` is wired + * to the Settings refresh button, which is the user's way of saying "go read + * it again" after rotating a credential. + * + * @module provider/Services/ProviderSecretResolver + */ +import type { ProviderInstanceEnvironment } from "@t3tools/contracts"; +import * as Context from "effect/Context"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; + +export interface ProviderSecretResolverShape { + /** + * Replace every secret reference in the environment with its value. + * Literal values pass through untouched, and an environment with no + * references is returned as-is. + * + * Never fails. A reference that cannot be read (1Password locked, `op` not + * installed, item deleted) drops its variable from the result rather than + * substituting an empty string, so the provider reports the honest + * "unauthenticated" instead of failing later with a credential that looks + * present and is not. + */ + readonly resolve: ( + environment: ProviderInstanceEnvironment | undefined, + ) => 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 + * rebuild the instance: a provider process keeps the environment it was + * spawned with. + */ + readonly invalidate: Effect.Effect; +} + +export class ProviderSecretResolver extends Context.Service< + ProviderSecretResolver, + ProviderSecretResolverShape +>()("t3/provider/Services/ProviderSecretResolver") {} + +/** + * Resolver that hands every environment back untouched. This is what a build + * without secret-store integration behaves like, and what tests want unless + * they are testing resolution itself: an `op://` value stays an `op://` + * value, and the provider reports whatever the CLI makes of it. + */ +export const passthroughProviderSecretResolver: ProviderSecretResolverShape = { + resolve: Effect.succeed, + invalidate: Effect.void, +}; + +export const ProviderSecretResolverPassthroughLayer: Layer.Layer = + Layer.succeed(ProviderSecretResolver, passthroughProviderSecretResolver); diff --git a/apps/server/src/server.ts b/apps/server/src/server.ts index d2b72fb62bf9..e2412fab6db9 100644 --- a/apps/server/src/server.ts +++ b/apps/server/src/server.ts @@ -43,6 +43,7 @@ import * as GitHubCli from "./sourceControl/GitHubCli.ts"; import * as GitLabCli from "./sourceControl/GitLabCli.ts"; import * as TextGeneration from "./textGeneration/TextGeneration.ts"; import { ProviderInstanceRegistryHydrationLive } from "./provider/Layers/ProviderInstanceRegistryHydration.ts"; +import { ProviderSecretResolverLive } from "./provider/Layers/ProviderSecretResolverLive.ts"; import * as TerminalManager from "./terminal/Manager.ts"; import * as McpHttpServer from "./mcp/McpHttpServer.ts"; import * as McpSessionRegistry from "./mcp/McpSessionRegistry.ts"; @@ -393,6 +394,10 @@ const RuntimeCoreDependenciesWithoutThreadBootstrapLive = ReactorLayerLive.pipe( // `providerInstances` hydration merges `settings.providers.` // with explicit `providerInstances` entries on boot. Layer.provideMerge(ProviderInstanceRegistryHydrationLive), + // Resolves `op://` environment values for both halves above: the drivers + // read secrets while building an instance, and `ProviderRegistryLive` + // drops the cached values when the user refreshes. + Layer.provideMerge(ProviderSecretResolverLive), // Shared native/canonical NDJSON writers used by both the per-instance // drivers (native stream, written from inside each `Adapter`) and // `ProviderService` (canonical stream, written after event normalization). diff --git a/apps/server/src/textGeneration/TextGeneration.test.ts b/apps/server/src/textGeneration/TextGeneration.test.ts index 9bccb9c1fc5b..dbde7b8f2c14 100644 --- a/apps/server/src/textGeneration/TextGeneration.test.ts +++ b/apps/server/src/textGeneration/TextGeneration.test.ts @@ -50,6 +50,7 @@ const makeStubRegistry = ( getInstance: (id) => Effect.succeed(byId.get(id)), listInstances: Effect.succeed(instances), listUnavailable: Effect.succeed([]), + rebuildInstanceWhen: () => Effect.succeed(false), streamChanges: Stream.empty, // Tests never drive changes through this stub; acquire a throwaway // subscription on an unused PubSub so the shape is satisfied. diff --git a/docs/README.md b/docs/README.md index f1698a66e179..2adb984134fd 100644 --- a/docs/README.md +++ b/docs/README.md @@ -14,6 +14,7 @@ - [Source control integrations](./user/source-control.md) - [Background service (Linux)](./user/background-service.md) - Providers: [Codex](./user/providers-codex.md) · [Claude](./user/providers-claude.md) +- [Provider secrets from 1Password](./user/provider-secrets.md) Mobile app: [apps/mobile/README.md](../apps/mobile/README.md) diff --git a/docs/fork/0016-provider-secrets-live-in-1password.md b/docs/fork/0016-provider-secrets-live-in-1password.md new file mode 100644 index 000000000000..daedc445dfcc --- /dev/null +++ b/docs/fork/0016-provider-secrets-live-in-1password.md @@ -0,0 +1,54 @@ +# 0016: Provider secrets can live in 1Password + +- PR: pending +- Status: active + +## What you can do now + +- Give a provider its credential without giving T3 Code the credential. Paste a + 1Password `op://` secret reference as an environment variable value on any + provider instance, and T3 Code reads the value from the 1Password CLI when it + starts the agent. What gets saved is the reference; the secret itself is + 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. +- 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. +- Tell when a vault is locked. A reference T3 Code cannot read leaves the + variable unset, so the provider reports as not authenticated instead of + starting with a blank credential and failing on your first message. + +## Why + +Running several accounts of the same provider side by side means several +long-lived tokens, and every one of them had to be copied out of the password +manager and pasted into a second store to be usable. That is the copy nobody +rotates: it outlives the original, it has no expiry anyone tracks, and it sits +in a file that gets synced, backed up, and occasionally shared with a support +thread. + +Pointing at the secret instead of duplicating it keeps one copy under the +policy that was chosen for it, and makes rotation a vault edit plus a refresh +rather than a hunt for everywhere the value was pasted. + +The in-memory hold is what makes it usable rather than merely correct. Reading +the vault on every thread start turns a background biometric prompt into a +constant interruption, which is the kind of friction that ends with the token +pasted in plaintext again to make it stop. + +## Upstream considerations + +Worth proposing upstream, though it is a product decision rather than a bug +fix, so it may not be wanted in that shape. The 1Password-specific piece is +deliberately confined to one layer behind a resolver service, which is the +seam another secret store would plug into, so the argument upstream is about +whether to carry any secret-store integration at all rather than about +1Password specifically. + +Rebase burden is moderate. It adds a call in each of the five drivers' `create` +and one method on the provider instance registry, so a sync that reshapes +driver creation or instance rebuilding will conflict. The conflicts are shallow +and repetitive: the driver change is the same two lines in all five files. diff --git a/docs/fork/README.md b/docs/fork/README.md index 760d71ed627a..1827b7edcc26 100644 --- a/docs/fork/README.md +++ b/docs/fork/README.md @@ -47,3 +47,5 @@ Each entry uses these sections: active, [#23](https://github.com/TrogonStack/t3code/pull/23) - **0015** [A logged-out Claude install reads as logged out](./0015-a-logged-out-claude-install-reads-as-logged-out.md) active, [#26](https://github.com/TrogonStack/t3code/pull/26) +- **0016** [Provider secrets can live in 1Password](./0016-provider-secrets-live-in-1password.md) + active diff --git a/docs/internals/glossary.md b/docs/internals/glossary.md index da16f74d339f..976e635ba788 100644 --- a/docs/internals/glossary.md +++ b/docs/internals/glossary.md @@ -112,6 +112,13 @@ The agent interaction style for a thread. In [the contracts][1], the values are Controls how assistant text reaches the thread timeline. In [the contracts][1], `streaming` updates incrementally and `buffered` accumulates text. Buffered delivery is not held until the turn completes: it spills once accumulated text would exceed 24,000 characters, and flushes at approval and user-input boundaries. See [ProviderRuntimeIngestion.ts][5]. +#### Secret reference + +A provider environment value that names a credential instead of carrying it. Today that means a +1Password `op://` reference, resolved by [ProviderSecretResolver.ts][25] just before the driver +merges the environment into the agent's process env. Resolved values are held for the life of the +server and dropped when the user refreshes provider status. See [providers.md][16]. + #### Snapshot A point-in-time view of state. The word is used in multiple layers, including orchestration, provider, and checkpointing. See [ProjectionSnapshotQuery.ts][10], [ProviderAdapter.ts][15], and [CheckpointStore.ts][19]. @@ -179,3 +186,4 @@ The file patch and changed-file summary for one turn. It is usually computed in [22]: ../../apps/server/src/checkpointing/Utils.ts [23]: ../../apps/server/src/checkpointing/Diffs.ts [24]: ./overview.md +[25]: ../../apps/server/src/provider/Services/ProviderSecretResolver.ts diff --git a/docs/internals/providers.md b/docs/internals/providers.md index f36e28b2c15e..dbda3b5453c5 100644 --- a/docs/internals/providers.md +++ b/docs/internals/providers.md @@ -39,6 +39,51 @@ directory to route session and turn operations for a thread, so callers name a t Adding a driver means writing the driver plus adapter and adding it to `BUILT_IN_DRIVERS`. No orchestration, contract, or client change is required for the common case. +## Secret references in provider environments + +A provider instance's `environment` is merged into the child process env by +`mergeProviderInstanceEnvironment`, once per driver inside `create`. A value that starts with `op://` +is not passed through: it is a secret reference, and [`ProviderSecretResolver`][secretresolver] +swaps it for the value the 1Password CLI returns before the merge happens. + +The parsing half lives in [`ProviderSecretReference.ts`][secretref] and knows nothing about how a +secret is fetched, so the registry can ask "does this instance read from a secret store?" without +depending on the resolver. [`ProviderSecretResolverLive`][secretlive] is the half that shells out to +`op read --no-newline`. + +Three decisions are load-bearing: + +- **A failed read drops the variable.** It never substitutes an empty string, because an empty + `ANTHROPIC_API_KEY` reads to the provider as a configured-but-broken credential rather than an + absent one, and the status badge would go back to lying about it. +- **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. +- **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`. + +### Why a refresh has to rebuild the instance + +A driver resolves its environment once, at `create` time, and `makeManagedServerProvider` re-probes +using that captured `processEnv`. Dropping the cached secret therefore changes nothing on its own, +because the running instance still holds the value it was built with. + +So `ProviderRegistry.reloadSecretBackedInstances` invalidates the cache and then calls +[`rebuildInstanceWhen`][instances] on each instance, passing `hasProviderSecretReference` as the +predicate. The registry owns the child scopes and already stores each instance's config, so it can +close and rebuild one entry in place; passing a predicate rather than exposing the entries keeps +secret-store policy in `ProviderRegistry` and keeps the instance registry ignorant of 1Password. +`reconcile` cannot do this job, because it diffs the config envelope and a rotated secret leaves +that envelope byte-identical. + +This hangs off the three refresh entry points (`refreshAll`, the kind-scoped `refresh`, and +`refreshInstance`), all of which are reached only by a user action: the Settings refresh button, and +the post-update verification in `providerMaintenanceRunner`. The periodic provider health loop is +not one of them. It lives inside `makeManagedServerProvider` and calls `refreshSnapshot` directly, +which is what keeps a resolved secret alive between refreshes instead of re-reading it every few +minutes. + ## How provider work is requested Clients never call a provider directly. They dispatch orchestration commands over the RPC method @@ -100,6 +145,9 @@ usable, then fails with an `AcpTransportError`. `ProviderCommandReactor` turns t session error with a `provider.turn.start.failed` activity and clears `activeTurnId`, so the working indicator stops and the reason is visible in the timeline. +[secretref]: ../../apps/server/src/provider/ProviderSecretReference.ts +[secretresolver]: ../../apps/server/src/provider/Services/ProviderSecretResolver.ts +[secretlive]: ../../apps/server/src/provider/Layers/ProviderSecretResolverLive.ts [drivers]: ../../apps/server/src/provider/builtInDrivers.ts [codex]: ../../apps/server/src/provider/Drivers/CodexDriver.ts [claude]: ../../apps/server/src/provider/Drivers/ClaudeDriver.ts diff --git a/docs/user/provider-secrets.md b/docs/user/provider-secrets.md new file mode 100644 index 000000000000..8a142c980511 --- /dev/null +++ b/docs/user/provider-secrets.md @@ -0,0 +1,93 @@ +# Provider Secrets From 1Password + +This guide is for people who keep provider credentials in 1Password and would rather not copy them +into a second place. It applies to every provider: Codex, Claude, Cursor, Grok, and OpenCode. + +For provider setup itself, see [Codex](./providers-codex.md) and [Claude](./providers-claude.md). + +## I Do Not Want To Paste My Token Into T3 Code + +Paste the 1Password reference instead of the value. + +In the provider's Environment variables section in Settings, use the `op://` secret reference as the +value: + +```text +Name: CLAUDE_CODE_OAUTH_TOKEN +Value: op://Private/claude-code/credential +``` + +T3 Code reads the value with the 1Password CLI right before it starts the agent, and hands the +resolved value to the agent process only. The reference is what T3 Code stores; the secret itself +never lands in your settings file or in T3 Code's secret store. + +Any value beginning with `op://` is treated this way. Everything else is used exactly as typed, so +mixing literal variables and references on the same provider is fine. + +To copy a reference in 1Password, open the item, use the field's overflow menu, and choose +**Copy Secret Reference**. + +## What Do I Need Installed + +The [1Password CLI](https://developer.1password.com/docs/cli/get-started/), signed in on the machine +running the T3 Code server. + +Confirm it works from a normal shell first: + +```bash +op read --no-newline "op://Private/claude-code/credential" +``` + +If that command prints your secret, T3 Code can read it too. If it asks you to sign in, sign in +first, otherwise providers using references will start unauthenticated. + +Remote and tunnelled setups resolve references on the server, not on the device you are looking at. +The vault has to be reachable from wherever `npx t3` or the desktop app is actually running. + +## Do I Still Mark It Sensitive + +You do not need to. A reference is not a secret, so there is nothing to protect by storing it as one. + +Marking it sensitive still works if you prefer the redacted field in the UI, and the reference is +resolved the same way either way. + +## How Often Does It Ask Me To Unlock + +Once, and then not again until you ask for it. + +Each reference is read one time and held in memory for the life of the server. Starting a thread, +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. + +## I Rotated The Secret, How Do I Pick Up The New One + +Refresh provider status in Settings. + +Refresh drops everything that was held in memory and rebuilds the providers that use references, so +the next read goes back to 1Password. Providers with only literal variables are left alone. + +Threads that are already running keep the process they were given. Work started after the refresh +uses the new value. + +## The Provider Says It Is Not Authenticated + +That is what a failed read looks like, and it is deliberate: when T3 Code cannot read a reference it +leaves the variable unset rather than passing an empty value the agent would misread as a real one. + +Work through it in this order: + +1. Run the `op read` command above by hand. Most failures are a locked vault or a typo in the + reference. +2. Confirm `op` is on the `PATH` of whoever launched the T3 Code server. A CLI installed only for + your interactive shell is not always visible to a background service. +3. Refresh provider status once the underlying problem is fixed. A failed read is remembered exactly + like a successful one, so nothing retries on its own. + +The server log records which reference failed and what the 1Password CLI said about it. + +## Can I Use A Different Password Manager + +Not yet. `op://` references are the only form T3 Code resolves today. From f391884ec4adef21c8c58c74225b54988678d219 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 05:17:29 -0400 Subject: [PATCH 2/6] docs(fork): link the 1Password entry to its pull request Signed-off-by: Yordis Prieto --- docs/fork/0016-provider-secrets-live-in-1password.md | 2 +- docs/fork/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/fork/0016-provider-secrets-live-in-1password.md b/docs/fork/0016-provider-secrets-live-in-1password.md index daedc445dfcc..ff7b1e104d44 100644 --- a/docs/fork/0016-provider-secrets-live-in-1password.md +++ b/docs/fork/0016-provider-secrets-live-in-1password.md @@ -1,6 +1,6 @@ # 0016: Provider secrets can live in 1Password -- PR: pending +- PR: [TrogonStack/t3code#27](https://github.com/TrogonStack/t3code/pull/27) - Status: active ## What you can do now diff --git a/docs/fork/README.md b/docs/fork/README.md index 1827b7edcc26..6ad790c74414 100644 --- a/docs/fork/README.md +++ b/docs/fork/README.md @@ -48,4 +48,4 @@ Each entry uses these sections: - **0015** [A logged-out Claude install reads as logged out](./0015-a-logged-out-claude-install-reads-as-logged-out.md) active, [#26](https://github.com/TrogonStack/t3code/pull/26) - **0016** [Provider secrets can live in 1Password](./0016-provider-secrets-live-in-1password.md) - active + active, [#27](https://github.com/TrogonStack/t3code/pull/27) From 5ddf5ae3d823d34de01f1491c9acc260daf10cbe Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 15:10:27 -0400 Subject: [PATCH 3/6] fix(server): a secret refresh no longer costs the user an instance Resolving a secret can park on a person at a biometric prompt, and for that whole window a rebuild was observable: lookups handed back a torn-down instance, a settings change landing mid-rebuild was silently reverted, and an instance a locked vault had failed to rebuild stayed gone until settings changed. Signed-off-by: Yordis Prieto --- .../ProviderInstanceRegistryLive.test.ts | 175 ++++++++++++++++-- .../Layers/ProviderInstanceRegistryLive.ts | 125 +++++++++---- docs/internals/providers.md | 13 ++ 3 files changed, 268 insertions(+), 45 deletions(-) diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts index 9ec58cd0b029..904fd5964603 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts @@ -31,16 +31,21 @@ import { type GrokSettings, type OpenCodeSettings, ProviderDriverKind, + type ProviderInstanceConfig, type ProviderInstanceConfigMap, ProviderInstanceId, } from "@t3tools/contracts"; import * as DateTime from "effect/DateTime"; +import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; +import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; +import * as Ref from "effect/Ref"; import * as Stream from "effect/Stream"; import { HttpClient, HttpClientResponse } from "effect/unstable/http"; import * as BackgroundPolicy from "../../background/BackgroundPolicy.ts"; +import { ProviderDriverError } from "../Errors.ts"; import { ServerConfig } from "../../config.ts"; import { ServerSettingsService } from "../../serverSettings.ts"; import { ClaudeDriver } from "../Drivers/ClaudeDriver.ts"; @@ -491,21 +496,71 @@ describe("ProviderInstanceRegistryLive: rebuildInstanceWhen", () => { const codexDriverKind = ProviderDriverKind.make("codex"); const firstId = ProviderInstanceId.make("codex_first"); const secondId = ProviderInstanceId.make("codex_second"); - const configMap: ProviderInstanceConfigMap = { - [firstId]: { - driver: codexDriverKind, - displayName: "Codex (first)", - enabled: false, - environment: [{ name: "OP_TOKEN", value: "op://Vault/Item/token", sensitive: true }], - config: makeCodexConfig({ homePath: "/home/julius/.codex_first" }), - }, - [secondId]: { - driver: codexDriverKind, - displayName: "Codex (second)", - enabled: false, - config: makeCodexConfig({ homePath: "/home/julius/.codex_second" }), - }, + const firstEntry: ProviderInstanceConfig = { + driver: codexDriverKind, + displayName: "Codex (first)", + enabled: false, + environment: [{ name: "OP_TOKEN", value: "op://Vault/Item/token", sensitive: true }], + config: makeCodexConfig({ homePath: "/home/julius/.codex_first" }), }; + const secondEntry: ProviderInstanceConfig = { + driver: codexDriverKind, + displayName: "Codex (second)", + enabled: false, + config: makeCodexConfig({ homePath: "/home/julius/.codex_second" }), + }; + const configMap: ProviderInstanceConfigMap = { [firstId]: firstEntry, [secondId]: secondEntry }; + + /** + * A codex driver whose `create` parks until the test opens the gate, so a + * rebuild can be held exactly where a real one waits: inside the driver, + * with the old instance already gone. Arming is explicit so the registry's + * initial hydration runs unblocked. + */ + const makeCreateGate = Effect.gen(function* () { + const armed = yield* Ref.make(false); + const entered = yield* Deferred.make(); + const released = yield* Deferred.make(); + const gatedDriver: typeof CodexDriver = { + ...CodexDriver, + create: (input) => + Effect.gen(function* () { + if (yield* Ref.get(armed)) { + yield* Deferred.succeed(entered, undefined); + yield* Deferred.await(released); + } + return yield* CodexDriver.create(input); + }), + }; + return { + gatedDriver, + arm: Ref.set(armed, true), + awaitEntered: Deferred.await(entered), + release: Deferred.succeed(released, undefined), + }; + }); + + /** A codex driver that fails `create` while the returned ref says so. */ + const makeFailingCreate = Effect.gen(function* () { + const failing = yield* Ref.make(false); + const failingDriver: typeof CodexDriver = { + ...CodexDriver, + create: (input) => + Effect.gen(function* () { + if (yield* Ref.get(failing)) { + return yield* Effect.fail( + new ProviderDriverError({ + driver: codexDriverKind, + instanceId: input.instanceId, + detail: "secret store is locked", + }), + ); + } + return yield* CodexDriver.create(input); + }), + }; + return { failingDriver, setFailing: (value: boolean) => Ref.set(failing, value) }; + }); it.live("replaces only the instance the predicate accepts, in place", () => Effect.gen(function* () { @@ -562,4 +617,96 @@ describe("ProviderInstanceRegistryLive: rebuildInstanceWhen", () => { expect(rebuilt).toBe(false); }).pipe(Effect.provide(testLayer)), ); + + it.live("never serves the instance it is in the middle of replacing", () => + Effect.gen(function* () { + const gate = yield* makeCreateGate; + const { registry } = yield* makeProviderInstanceRegistry({ + drivers: [gate.gatedDriver], + configMap, + }); + yield* gate.arm; + + const rebuilding = yield* registry + .rebuildInstanceWhen(firstId, () => true) + .pipe(Effect.forkScoped); + yield* gate.awaitEntered; + + // The window this covers is a real one: resolving a secret can park on + // a person at a biometric prompt. Handing out the old bundle here means + // handing out a closed scope. + expect(yield* registry.getInstance(firstId)).toBeUndefined(); + expect((yield* registry.listInstances).map((instance) => instance.instanceId)).toEqual([ + secondId, + ]); + + yield* gate.release; + expect(yield* Fiber.join(rebuilding)).toBe(true); + expect((yield* registry.listInstances).map((instance) => instance.instanceId)).toEqual([ + firstId, + secondId, + ]); + }).pipe(Effect.provide(testLayer)), + ); + + it.live("does not undo a settings change that lands during a rebuild", () => + Effect.gen(function* () { + const gate = yield* makeCreateGate; + const { registry, mutator } = yield* makeProviderInstanceRegistry({ + drivers: [gate.gatedDriver], + configMap, + }); + yield* gate.arm; + + const rebuilding = yield* registry + .rebuildInstanceWhen(firstId, () => true) + .pipe(Effect.forkScoped); + yield* gate.awaitEntered; + const reconciling = yield* mutator + .reconcile({ [firstId]: firstEntry }) + .pipe(Effect.forkScoped); + + yield* gate.release; + expect(yield* Fiber.join(rebuilding)).toBe(true); + yield* Fiber.join(reconciling); + + // The removal sticks. A rebuild that wrote back the map it read before + // the removal would put the deleted instance back, pointing at a scope + // reconcile already closed. + expect((yield* registry.listInstances).map((instance) => instance.instanceId)).toEqual([ + firstId, + ]); + }).pipe(Effect.provide(testLayer)), + ); + + it.live("retries an instance a previous rebuild could not bring back", () => + Effect.gen(function* () { + const driver = yield* makeFailingCreate; + const { registry } = yield* makeProviderInstanceRegistry({ + drivers: [driver.failingDriver], + configMap, + }); + + yield* driver.setFailing(true); + expect(yield* registry.rebuildInstanceWhen(firstId, () => true)).toBe(true); + expect((yield* registry.listInstances).map((instance) => instance.instanceId)).toEqual([ + secondId, + ]); + expect((yield* registry.listUnavailable).map((provider) => provider.instanceId)).toEqual([ + firstId, + ]); + + // The next refresh is a retry, not a lookup that finds nothing: a locked + // vault must not cost the user their instance until settings change. + yield* driver.setFailing(false); + expect(yield* registry.rebuildInstanceWhen(firstId, () => true)).toBe(true); + expect(yield* registry.listUnavailable).toEqual([]); + // Recovered last, where it already sat while unavailable; the next + // settings change restores settings-author order. + expect((yield* registry.listInstances).map((instance) => instance.instanceId)).toEqual([ + secondId, + firstId, + ]); + }).pipe(Effect.provide(testLayer)), + ); }); diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts index 9b3c19d16566..918b9e84c8fd 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts @@ -50,6 +50,7 @@ import * as PubSub from "effect/PubSub"; import * as Ref from "effect/Ref"; import * as Schema from "effect/Schema"; import * as Scope from "effect/Scope"; +import * as Semaphore from "effect/Semaphore"; import * as Stream from "effect/Stream"; import { buildUnavailableProviderSnapshot } from "../unavailableProviderSnapshot.ts"; @@ -74,6 +75,40 @@ interface LiveEntry { readonly entry: ProviderInstanceConfig; } +function withoutKey( + map: ReadonlyMap, + instanceId: ProviderInstanceId, +): ReadonlyMap { + if (!map.has(instanceId)) { + return map; + } + const next = new Map(map); + next.delete(instanceId); + return next; +} + +/** + * Reinstate a rebuilt instance where it used to sit. Map iteration order is + * settings-author order all the way out to the provider list, so appending a + * rebuilt instance would shuffle the UI every time a secret is refreshed. + * + * An instance that is not in `previousEntries` was never live to begin with + * (a rebuild that recovers a previously unavailable instance), so it has no + * position to return to and goes last until the next settings change. + */ +function withInstanceAt( + previousEntries: ReadonlyMap, + instanceId: ProviderInstanceId, + rebuilt: LiveEntry, +): ReadonlyMap { + const next = new Map(); + for (const [id, existing] of previousEntries) { + next.set(id, id === instanceId ? rebuilt : existing); + } + next.set(instanceId, rebuilt); + return next; +} + /** * Internal state shared between the public registry service and the * mutator service. Both services are thin shells around these refs. @@ -369,10 +404,28 @@ export const makeProviderInstanceRegistry = (input: { const changes = yield* PubSub.unbounded(); yield* Effect.addFinalizer(() => PubSub.shutdown(changes)); + // Config envelopes for instances a rebuild could not bring back. Without + // this a failed rebuild is a one-way door: the instance leaves `entries`, + // so the next refresh finds nothing live to rebuild and only a settings + // change can restore it. + const rebuildable = yield* Ref.make>( + new Map(), + ); + // Both mutators read the map, do slow work, then write it back. A secret + // read can park on a person at a biometric prompt, which is more than long + // enough for a settings change to land in the middle, so they take turns. + // Reads stay outside: `getInstance` and `listInstances` never wait. + const mutations = yield* Semaphore.make(1); + const state: RegistryState = { entries, unavailable, changes }; const reconcileWithR = makeReconcile({ state, driversById, parentScope }); const reconcile: ProviderInstanceRegistryMutatorShape["reconcile"] = (configMap) => - reconcileWithR(configMap).pipe(Effect.provideContext(driverContext)); + mutations.withPermits(1)( + reconcileWithR(configMap).pipe( + Effect.tap(() => Ref.set(rebuildable, new Map())), + Effect.provideContext(driverContext), + ), + ); // Hydrate the initial configMap synchronously so callers can read // `listInstances` immediately after this effect completes. @@ -386,40 +439,50 @@ export const makeProviderInstanceRegistry = (input: { instanceId, shouldRebuild, ) => - Effect.gen(function* () { - const previousEntries = yield* Ref.get(entries); - const live = previousEntries.get(instanceId); - if (live === undefined || !shouldRebuild(live.entry)) { - return false; - } + mutations.withPermits(1)( + Effect.gen(function* () { + const previousEntries = yield* Ref.get(entries); + const live = previousEntries.get(instanceId); + // An instance a previous rebuild could not restore has no live entry + // to read the config from, so its envelope comes from `rebuildable`. + const entry = live?.entry ?? (yield* Ref.get(rebuildable)).get(instanceId); + if (entry === undefined || !shouldRebuild(entry)) { + return false; + } - yield* Scope.close(live.scope, Exit.void).pipe(Effect.ignore); - const result = yield* buildEntry({ - driversById, - parentScope, - instanceId, - rawInstanceId: instanceId, - entry: live.entry, - }); + // Drop the instance before closing it. Building the replacement can + // wait on a person at a biometric prompt, and for that whole window + // the map would otherwise hand callers a bundle whose scope is gone. + if (live !== undefined) { + yield* Ref.set(entries, withoutKey(previousEntries, instanceId)); + yield* Scope.close(live.scope, Exit.void).pipe(Effect.ignore); + } - const nextEntries = new Map(); - for (const [id, existing] of previousEntries) { - if (id !== instanceId) { - nextEntries.set(id, existing); - } else if (result.kind === "live") { - nextEntries.set(id, result.live); + const result = yield* buildEntry({ + driversById, + parentScope, + instanceId, + rawInstanceId: instanceId, + entry, + }); + + if (result.kind === "live") { + yield* Ref.set(entries, withInstanceAt(previousEntries, instanceId, result.live)); + yield* Ref.update(unavailable, (previous) => withoutKey(previous, instanceId)); + yield* Ref.update(rebuildable, (previous) => withoutKey(previous, instanceId)); + } else { + // Keep the envelope so the next refresh is a real retry rather than + // a lookup that finds nothing. + yield* Ref.update(rebuildable, (previous) => new Map(previous).set(instanceId, entry)); + yield* Ref.update(unavailable, (previous) => + new Map(previous).set(instanceId, result.snapshot), + ); } - } - yield* Ref.set(entries, nextEntries); - if (result.kind === "unavailable") { - yield* Ref.update(unavailable, (previous) => - new Map(previous).set(instanceId, result.snapshot), - ); - } - yield* PubSub.publish(changes, undefined); - return true; - }).pipe(Effect.provideContext(driverContext)); + yield* PubSub.publish(changes, undefined); + return true; + }).pipe(Effect.provideContext(driverContext)), + ); const registry: ProviderInstanceRegistryShape = { getInstance: (id) => Ref.get(entries).pipe(Effect.map((map) => map.get(id)?.instance)), diff --git a/docs/internals/providers.md b/docs/internals/providers.md index dbda3b5453c5..60bf921bb71c 100644 --- a/docs/internals/providers.md +++ b/docs/internals/providers.md @@ -77,6 +77,19 @@ secret-store policy in `ProviderRegistry` and keeps the instance registry ignora `reconcile` cannot do this job, because it diffs the config envelope and a rotated secret leaves that envelope byte-identical. +A rebuild takes as long as the secret read does, and a locked vault can park it on a person at a +biometric prompt. Three things follow from that window being long: + +- **The instance leaves the map before its scope closes.** Otherwise every lookup for the whole + window hands back a bundle whose scope is already gone. +- **Rebuilds and `reconcile` take turns.** Both read the instance map, do slow work, then write it + back, so a settings change landing mid-rebuild would be overwritten by the map the rebuild read + before it. Reads stay outside the lock: `getInstance` and `listInstances` never wait on a + 1Password prompt. +- **A rebuild that fails is retryable.** The registry keeps the config envelope of an instance it + could not bring back, so the next refresh retries it. Without that, a vault that happened to be + locked would cost the user the instance until settings changed. + This hangs off the three refresh entry points (`refreshAll`, the kind-scoped `refresh`, and `refreshInstance`), all of which are reached only by a user action: the Settings refresh button, and the post-update verification in `providerMaintenanceRunner`. The periodic provider health loop is From e299917ff7d41c2189ebbb376d026f0c6ddb679e Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 15:20:04 -0400 Subject: [PATCH 4/6] fix(server): a provider card survives its own credential refresh An instance being rebuilt is briefly neither live nor unavailable, and the aggregator treats an id it finds in neither list as gone, so a provider could vanish from Settings for as long as the secret store took to answer. A refresh with no explicit target also only reached live instances, which left the one instance that most needed retrying, the one a locked vault had just failed to rebuild, waiting on a settings edit instead. Signed-off-by: Yordis Prieto --- .../ProviderInstanceRegistryLive.test.ts | 7 ++ .../Layers/ProviderInstanceRegistryLive.ts | 7 ++ .../provider/Layers/ProviderRegistry.test.ts | 74 +++++++++++++++++++ .../src/provider/Layers/ProviderRegistry.ts | 13 +++- docs/internals/providers.md | 9 ++- 5 files changed, 106 insertions(+), 4 deletions(-) diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts index 904fd5964603..7872435342a0 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts @@ -639,6 +639,12 @@ describe("ProviderInstanceRegistryLive: rebuildInstanceWhen", () => { expect((yield* registry.listInstances).map((instance) => instance.instanceId)).toEqual([ secondId, ]); + // Not routable, but not gone either: consumers prune ids they find in + // neither list, and the provider's card must not blink out of the UI for + // as long as a secret store takes to answer. + expect((yield* registry.listUnavailable).map((provider) => provider.instanceId)).toEqual([ + firstId, + ]); yield* gate.release; expect(yield* Fiber.join(rebuilding)).toBe(true); @@ -646,6 +652,7 @@ describe("ProviderInstanceRegistryLive: rebuildInstanceWhen", () => { firstId, secondId, ]); + expect(yield* registry.listUnavailable).toEqual([]); }).pipe(Effect.provide(testLayer)), ); diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts index 918b9e84c8fd..1c6e69bacec3 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts @@ -453,8 +453,15 @@ export const makeProviderInstanceRegistry = (input: { // Drop the instance before closing it. Building the replacement can // wait on a person at a biometric prompt, and for that whole window // the map would otherwise hand callers a bundle whose scope is gone. + // + // The last snapshot stands in for it meanwhile. Aggregators treat an + // id that is in neither list as gone and prune it, so leaving the id + // nowhere would blank the provider's card for as long as the secret + // store takes to answer, then bring it back. if (live !== undefined) { + const parked = yield* live.instance.snapshot.getSnapshot; yield* Ref.set(entries, withoutKey(previousEntries, instanceId)); + yield* Ref.update(unavailable, (previous) => new Map(previous).set(instanceId, parked)); yield* Scope.close(live.scope, Exit.void).pipe(Effect.ignore); } diff --git a/apps/server/src/provider/Layers/ProviderRegistry.test.ts b/apps/server/src/provider/Layers/ProviderRegistry.test.ts index 144ea34e00e1..133cca76d681 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.test.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.test.ts @@ -998,6 +998,80 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te }), ); + // A rebuild that could not finish - a locked vault, a secret store that + // was not running - leaves the instance out of the live set. Refresh is + // the user saying "try again", so it has to reach the instances that + // need trying again, not just the ones that are already working. + it.effect("retries instances that are not live when nothing is targeted", () => + Effect.gen(function* () { + const codexDriver = ProviderDriverKind.make("codex"); + const codexInstanceId = ProviderInstanceId.make("codex"); + const unavailableProvider = { + instanceId: codexInstanceId, + 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 rebuiltIds = yield* Ref.make>([]); + const secretResolverLayer = Layer.succeed(ProviderSecretResolver, { + resolve: Effect.succeed, + invalidate: Effect.void, + }); + const instanceRegistryLayer = Layer.succeed( + ProviderInstanceRegistry.ProviderInstanceRegistry, + { + getInstance: () => Effect.succeed(undefined), + listInstances: Effect.succeed([]), + listUnavailable: Effect.succeed([unavailableProvider]), + rebuildInstanceWhen: (instanceId, shouldRebuild) => + shouldRebuild({ + driver: codexDriver, + environment: [ + { name: "CODEX_TOKEN", value: "op://Vault/Item/token", sensitive: true }, + ], + }) + ? 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-retry-", + }), + ), + Layer.provideMerge(NodeServices.layer), + Layer.provideMerge(secretResolverLayer), + ), + ).pipe(Scope.provide(scope)); + + yield* Effect.gen(function* () { + const registry = yield* ProviderRegistry.ProviderRegistry; + yield* registry.refresh(); + + assert.deepStrictEqual(yield* Ref.get(rebuiltIds), [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"); diff --git a/apps/server/src/provider/Layers/ProviderRegistry.ts b/apps/server/src/provider/Layers/ProviderRegistry.ts index dfa40d6fa2cd..fa65677ca2ff 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.ts @@ -658,7 +658,18 @@ export const ProviderRegistryLive = Layer.effect( instanceIds?: ReadonlyArray, ) { yield* secretResolver.invalidate; - const targets = instanceIds ?? [...(yield* Ref.get(liveSubsRef)).keys()]; + // Unavailable instances are targets too. An instance whose last rebuild + // failed because the vault was locked is not live, so live subscriptions + // alone would never reach it again, and a refresh is exactly the moment + // the user is saying the vault is open now. + const targets = + instanceIds ?? + Array.from( + new Set([ + ...(yield* Ref.get(liveSubsRef)).keys(), + ...(yield* instanceRegistry.listUnavailable).map(snapshotInstanceKey), + ]), + ); const rebuilt = yield* Effect.forEach(targets, (instanceId) => instanceRegistry.rebuildInstanceWhen(instanceId, (entry) => hasProviderSecretReference(entry.environment), diff --git a/docs/internals/providers.md b/docs/internals/providers.md index 60bf921bb71c..a3159e5d7198 100644 --- a/docs/internals/providers.md +++ b/docs/internals/providers.md @@ -81,14 +81,17 @@ A rebuild takes as long as the secret read does, and a locked vault can park it biometric prompt. Three things follow from that window being long: - **The instance leaves the map before its scope closes.** Otherwise every lookup for the whole - window hands back a bundle whose scope is already gone. + window hands back a bundle whose scope is already gone. Its last snapshot stands in for it + meanwhile, because `ProviderRegistry` prunes ids it finds in neither list, and the card must not + blink out of Settings while 1Password waits on a fingerprint. - **Rebuilds and `reconcile` take turns.** Both read the instance map, do slow work, then write it back, so a settings change landing mid-rebuild would be overwritten by the map the rebuild read before it. Reads stay outside the lock: `getInstance` and `listInstances` never wait on a 1Password prompt. - **A rebuild that fails is retryable.** The registry keeps the config envelope of an instance it - could not bring back, so the next refresh retries it. Without that, a vault that happened to be - locked would cost the user the instance until settings changed. + could not bring back, so the next refresh retries it, and a refresh with no explicit target + covers the unavailable instances as well as the live ones. Without both halves, a vault that + happened to be locked would cost the user the instance until settings changed. This hangs off the three refresh entry points (`refreshAll`, the kind-scoped `refresh`, and `refreshInstance`), all of which are reached only by a user action: the Settings refresh button, and From 1f948692cf2433b60e305c47998e40c60db0257a Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 15:30:47 -0400 Subject: [PATCH 5/6] fix(server): an unreadable secret cannot fall back to the server's own credential A provider process starts from the server's environment, so leaving an unreadable reference out of the resolved variables was not the same as leaving it unset: a machine that already exported the same name handed the agent that credential instead, under the account the instance did not name, and reported it as authenticated. This is the behavior the secrets guide already describes. Signed-off-by: Yordis Prieto --- .../provider/Layers/ProviderRegistry.test.ts | 4 +- .../Layers/ProviderSecretResolverLive.test.ts | 10 +++-- .../Layers/ProviderSecretResolverLive.ts | 6 ++- .../ProviderInstanceEnvironment.test.ts | 37 +++++++++++++++++-- .../provider/ProviderInstanceEnvironment.ts | 25 +++++++++++-- .../Services/ProviderSecretResolver.ts | 14 ++++--- docs/internals/providers.md | 7 +++- 7 files changed, 82 insertions(+), 21 deletions(-) diff --git a/apps/server/src/provider/Layers/ProviderRegistry.test.ts b/apps/server/src/provider/Layers/ProviderRegistry.test.ts index 133cca76d681..6d4f8318f86f 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.test.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.test.ts @@ -947,7 +947,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te const invalidations = yield* Ref.make(0); const rebuiltIds = yield* Ref.make>([]); const secretResolverLayer = Layer.succeed(ProviderSecretResolver, { - resolve: Effect.succeed, + resolve: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), invalidate: Ref.update(invalidations, (count) => count + 1), }); const instanceRegistryLayer = Layer.succeed( @@ -1023,7 +1023,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te const rebuiltIds = yield* Ref.make>([]); const secretResolverLayer = Layer.succeed(ProviderSecretResolver, { - resolve: Effect.succeed, + resolve: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), invalidate: Effect.void, }); const instanceRegistryLayer = Layer.succeed( diff --git a/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts b/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts index fab183166e2e..8179d03aa311 100644 --- a/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts @@ -58,7 +58,7 @@ describe("ProviderSecretResolverLive", () => { const resolved = yield* resolver.resolve(environment); - assert.deepStrictEqual(resolved, environment); + assert.deepStrictEqual(resolved, { variables: environment, unresolved: [] }); assert.strictEqual(spawner.invocations.length, 0); }).pipe(Effect.provide(ProviderSecretResolverLive.pipe(Layer.provide(spawner.layer)))); }); @@ -76,7 +76,7 @@ describe("ProviderSecretResolverLive", () => { ); assert.deepStrictEqual( - resolved?.map((variable) => [variable.name, variable.value]), + resolved.variables?.map((variable) => [variable.name, variable.value]), [ ["CLAUDE_SECURESTORAGE_CONFIG_DIR", "/home/u/.claude/work"], ["CLAUDE_CODE_OAUTH_TOKEN", "sk-live-token"], @@ -126,9 +126,13 @@ describe("ProviderSecretResolverLive", () => { ); assert.deepStrictEqual( - resolved?.map((variable) => variable.name), + resolved.variables?.map((variable) => variable.name), ["CLAUDE_SECURESTORAGE_CONFIG_DIR"], ); + // Naming it is what gets it unset in the child environment. Merely + // leaving it out would hand the provider whatever the server itself was + // started with under that name. + assert.deepStrictEqual(resolved.unresolved, ["CLAUDE_CODE_OAUTH_TOKEN"]); }).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 fbd137f7bdcb..f3cb3b9a42c9 100644 --- a/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts @@ -102,9 +102,10 @@ export const ProviderSecretResolverLive: Layer.Layer< const resolve: ProviderSecretResolverShape["resolve"] = (environment) => Effect.gen(function* () { if (!hasProviderSecretReference(environment)) { - return environment; + return { variables: environment, unresolved: [] }; } const resolved: Array = []; + const unresolved: Array = []; for (const variable of environment ?? []) { const reference = providerSecretReference(variable.value); if (reference === undefined) { @@ -113,11 +114,12 @@ export const ProviderSecretResolverLive: Layer.Layer< } const secret = yield* Cache.get(cache, reference); if (secret === undefined) { + unresolved.push(variable.name); continue; } resolved.push({ ...variable, value: secret }); } - return resolved as ProviderInstanceEnvironment; + return { variables: resolved as ProviderInstanceEnvironment, unresolved }; }); return { resolve, invalidate: Cache.invalidateAll(cache) }; diff --git a/apps/server/src/provider/ProviderInstanceEnvironment.test.ts b/apps/server/src/provider/ProviderInstanceEnvironment.test.ts index 7ac3f2f2837d..ad3de6d1a5cc 100644 --- a/apps/server/src/provider/ProviderInstanceEnvironment.test.ts +++ b/apps/server/src/provider/ProviderInstanceEnvironment.test.ts @@ -6,10 +6,13 @@ describe("mergeProviderInstanceEnvironment", () => { it("overrides inherited environment values and preserves empty strings", () => { expect( mergeProviderInstanceEnvironment( - [ - { name: "OPENROUTER_API_KEY", value: "sk-or-test", sensitive: true }, - { name: "ANTHROPIC_API_KEY", value: "", sensitive: false }, - ], + { + variables: [ + { name: "OPENROUTER_API_KEY", value: "sk-or-test", sensitive: true }, + { name: "ANTHROPIC_API_KEY", value: "", sensitive: false }, + ], + unresolved: [], + }, { ANTHROPIC_API_KEY: "inherited", PATH: "/bin" }, ), ).toMatchObject({ @@ -18,4 +21,30 @@ describe("mergeProviderInstanceEnvironment", () => { PATH: "/bin", }); }); + + // The instance says this credential comes from the vault. If the vault could + // not answer, the honest child environment has no credential at all: falling + // back to the one the server was started with runs the agent as a different + // account than the one the user configured, and reports it as authenticated. + it("unsets a variable the secret store could not answer for", () => { + const merged = mergeProviderInstanceEnvironment( + { + variables: [ + { + name: "CLAUDE_SECURESTORAGE_CONFIG_DIR", + value: "/home/u/.claude/work", + sensitive: false, + }, + ], + unresolved: ["ANTHROPIC_API_KEY"], + }, + { ANTHROPIC_API_KEY: "inherited", PATH: "/bin" }, + ); + + expect("ANTHROPIC_API_KEY" in merged).toBe(false); + expect(merged).toMatchObject({ + CLAUDE_SECURESTORAGE_CONFIG_DIR: "/home/u/.claude/work", + PATH: "/bin", + }); + }); }); diff --git a/apps/server/src/provider/ProviderInstanceEnvironment.ts b/apps/server/src/provider/ProviderInstanceEnvironment.ts index e469253604e6..fafd662b26e5 100644 --- a/apps/server/src/provider/ProviderInstanceEnvironment.ts +++ b/apps/server/src/provider/ProviderInstanceEnvironment.ts @@ -1,15 +1,34 @@ import type { ProviderInstanceEnvironment } from "@t3tools/contracts"; +/** + * An instance environment after its secret references have been read. + * + * `unresolved` names the variables the instance configures but whose value + * could not be read. They are tracked separately because being absent from + * `variables` is not enough: the child environment starts from the server's + * own, so a name left alone keeps whatever the server inherited under it. + * That is the wrong credential precisely when the user asked for a specific + * one, and it hides as "authenticated". + */ +export interface ResolvedProviderInstanceEnvironment { + readonly variables: ProviderInstanceEnvironment | undefined; + readonly unresolved: ReadonlyArray; +} + export function mergeProviderInstanceEnvironment( - environment: ProviderInstanceEnvironment | undefined, + resolved: ResolvedProviderInstanceEnvironment, baseEnv: NodeJS.ProcessEnv = process.env, ): NodeJS.ProcessEnv { - if (!environment || environment.length === 0) { + const { variables, unresolved } = resolved; + if ((!variables || variables.length === 0) && unresolved.length === 0) { return baseEnv; } const next: NodeJS.ProcessEnv = { ...baseEnv }; - for (const variable of environment) { + for (const name of unresolved) { + delete next[name]; + } + for (const variable of variables ?? []) { next[variable.name] = variable.value; } return next; diff --git a/apps/server/src/provider/Services/ProviderSecretResolver.ts b/apps/server/src/provider/Services/ProviderSecretResolver.ts index d7f663aefee5..644ffa6828d2 100644 --- a/apps/server/src/provider/Services/ProviderSecretResolver.ts +++ b/apps/server/src/provider/Services/ProviderSecretResolver.ts @@ -17,6 +17,8 @@ import * as Context from "effect/Context"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; +import type { ResolvedProviderInstanceEnvironment } from "../ProviderInstanceEnvironment.ts"; + export interface ProviderSecretResolverShape { /** * Replace every secret reference in the environment with its value. @@ -24,14 +26,16 @@ export interface ProviderSecretResolverShape { * references is returned as-is. * * Never fails. A reference that cannot be read (1Password locked, `op` not - * installed, item deleted) drops its variable from the result rather than - * substituting an empty string, so the provider reports the honest + * installed, item deleted) is reported as unresolved rather than + * substituted with an empty string, so the provider reports the honest * "unauthenticated" instead of failing later with a credential that looks - * present and is not. + * present and is not. `mergeProviderInstanceEnvironment` unsets those names, + * which is what keeps the provider off a same-named credential the server + * happens to have inherited. */ readonly resolve: ( environment: ProviderInstanceEnvironment | undefined, - ) => Effect.Effect; + ) => 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 @@ -53,7 +57,7 @@ export class ProviderSecretResolver extends Context.Service< * value, and the provider reports whatever the CLI makes of it. */ export const passthroughProviderSecretResolver: ProviderSecretResolverShape = { - resolve: Effect.succeed, + resolve: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), invalidate: Effect.void, }; diff --git a/docs/internals/providers.md b/docs/internals/providers.md index a3159e5d7198..56b9b0c791f7 100644 --- a/docs/internals/providers.md +++ b/docs/internals/providers.md @@ -53,9 +53,12 @@ depending on the resolver. [`ProviderSecretResolverLive`][secretlive] is the hal Three decisions are load-bearing: -- **A failed read drops the variable.** It never substitutes an empty string, because an empty +- **A failed read unsets the variable.** It never substitutes an empty string, because an empty `ANTHROPIC_API_KEY` reads to the provider as a configured-but-broken credential rather than an - absent one, and the status badge would go back to lying about it. + absent one, and the status badge would go back to lying about it. Unsetting is stronger than + 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. From b5d7b9b2336c7aeaf5a03b0de9217c509e47954d Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 20 Aug 2026 15:37:40 -0400 Subject: [PATCH 6/6] fix(server): an abandoned refresh does not strand the instance it was rebuilding The request that asks for a refresh can go away while 1Password is still waiting on a fingerprint. The registry only recorded what it needed to retry after the build returned, so an interrupt in that window left the instance with no live entry and no envelope, and the refresh button, its only recovery path, had nothing to act on. Signed-off-by: Yordis Prieto --- .../ProviderInstanceRegistryLive.test.ts | 30 ++++++++ .../Layers/ProviderInstanceRegistryLive.ts | 72 ++++++++++++------- docs/internals/providers.md | 4 +- 3 files changed, 78 insertions(+), 28 deletions(-) diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts index 7872435342a0..a7c4243a1701 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts @@ -535,6 +535,7 @@ describe("ProviderInstanceRegistryLive: rebuildInstanceWhen", () => { return { gatedDriver, arm: Ref.set(armed, true), + disarm: Ref.set(armed, false), awaitEntered: Deferred.await(entered), release: Deferred.succeed(released, undefined), }; @@ -686,6 +687,35 @@ describe("ProviderInstanceRegistryLive: rebuildInstanceWhen", () => { }).pipe(Effect.provide(testLayer)), ); + // Interruption is not exotic here: the request that asked for the refresh can + // go away while `op` is still waiting on a fingerprint. + it.live("keeps an interrupted rebuild retryable", () => + Effect.gen(function* () { + const gate = yield* makeCreateGate; + const { registry } = yield* makeProviderInstanceRegistry({ + drivers: [gate.gatedDriver], + configMap, + }); + yield* gate.arm; + + const rebuilding = yield* registry + .rebuildInstanceWhen(firstId, () => true) + .pipe(Effect.forkScoped); + yield* gate.awaitEntered; + yield* Fiber.interrupt(rebuilding); + + // The instance is not live and its envelope only ever existed in the + // registry, so a refresh that cannot find it here has nowhere else to + // look and the user waits on a settings edit to get it back. + yield* gate.disarm; + expect(yield* registry.rebuildInstanceWhen(firstId, () => true)).toBe(true); + expect((yield* registry.listInstances).map((instance) => instance.instanceId)).toContain( + firstId, + ); + expect(yield* registry.listUnavailable).toEqual([]); + }).pipe(Effect.provide(testLayer)), + ); + it.live("retries an instance a previous rebuild could not bring back", () => Effect.gen(function* () { const driver = yield* makeFailingCreate; diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts index 1c6e69bacec3..6cd1f9ae3125 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts @@ -450,20 +450,35 @@ export const makeProviderInstanceRegistry = (input: { return false; } - // Drop the instance before closing it. Building the replacement can - // wait on a person at a biometric prompt, and for that whole window - // the map would otherwise hand callers a bundle whose scope is gone. + // Everything except the build itself is bookkeeping over refs, and it + // is uninterruptible so the instance is never half-moved. Only the + // build is interruptible, because it is the part that can take + // minutes waiting on a secret store. // - // The last snapshot stands in for it meanwhile. Aggregators treat an - // id that is in neither list as gone and prune it, so leaving the id - // nowhere would blank the provider's card for as long as the secret - // store takes to answer, then bring it back. - if (live !== undefined) { - const parked = yield* live.instance.snapshot.getSnapshot; - yield* Ref.set(entries, withoutKey(previousEntries, instanceId)); - yield* Ref.update(unavailable, (previous) => new Map(previous).set(instanceId, parked)); - yield* Scope.close(live.scope, Exit.void).pipe(Effect.ignore); - } + // Remembering the envelope comes first: an interrupt anywhere after + // this point still leaves the instance retryable on the next + // refresh, which is the only recovery path it has left. + yield* Effect.uninterruptible( + Effect.gen(function* () { + yield* Ref.update(rebuildable, (previous) => + new Map(previous).set(instanceId, entry), + ); + if (live === undefined) { + return; + } + // Drop the instance before closing it, or the map hands callers + // a bundle whose scope is gone for the whole build. Its last + // snapshot stands in meanwhile: aggregators treat an id that is + // in neither list as gone and prune it, so leaving the id + // nowhere would blank the provider's card until the build lands. + const parked = yield* live.instance.snapshot.getSnapshot; + yield* Ref.set(entries, withoutKey(previousEntries, instanceId)); + yield* Ref.update(unavailable, (previous) => + new Map(previous).set(instanceId, parked), + ); + yield* Scope.close(live.scope, Exit.void).pipe(Effect.ignore); + }), + ); const result = yield* buildEntry({ driversById, @@ -473,20 +488,23 @@ export const makeProviderInstanceRegistry = (input: { entry, }); - if (result.kind === "live") { - yield* Ref.set(entries, withInstanceAt(previousEntries, instanceId, result.live)); - yield* Ref.update(unavailable, (previous) => withoutKey(previous, instanceId)); - yield* Ref.update(rebuildable, (previous) => withoutKey(previous, instanceId)); - } else { - // Keep the envelope so the next refresh is a real retry rather than - // a lookup that finds nothing. - yield* Ref.update(rebuildable, (previous) => new Map(previous).set(instanceId, entry)); - yield* Ref.update(unavailable, (previous) => - new Map(previous).set(instanceId, result.snapshot), - ); - } - - yield* PubSub.publish(changes, undefined); + // Uninterruptible as well: the replacement is already running by + // now, and an interrupt that dropped it on the floor would leave a + // provider process nothing can reach or stop. + yield* Effect.uninterruptible( + Effect.gen(function* () { + if (result.kind === "live") { + yield* Ref.set(entries, withInstanceAt(previousEntries, instanceId, result.live)); + yield* Ref.update(unavailable, (previous) => withoutKey(previous, instanceId)); + yield* Ref.update(rebuildable, (previous) => withoutKey(previous, instanceId)); + } else { + yield* Ref.update(unavailable, (previous) => + new Map(previous).set(instanceId, result.snapshot), + ); + } + yield* PubSub.publish(changes, undefined); + }), + ); return true; }).pipe(Effect.provideContext(driverContext)), ); diff --git a/docs/internals/providers.md b/docs/internals/providers.md index 56b9b0c791f7..910de3d695fa 100644 --- a/docs/internals/providers.md +++ b/docs/internals/providers.md @@ -94,7 +94,9 @@ biometric prompt. Three things follow from that window being long: - **A rebuild that fails is retryable.** The registry keeps the config envelope of an instance it could not bring back, so the next refresh retries it, and a refresh with no explicit target covers the unavailable instances as well as the live ones. Without both halves, a vault that - happened to be locked would cost the user the instance until settings changed. + happened to be locked would cost the user the instance until settings changed. The envelope is + recorded before the build starts and the map writes around the build are uninterruptible, so a + refresh whose caller walked away mid-read is retryable on the same terms as one that failed. This hangs off the three refresh entry points (`refreshAll`, the kind-scoped `refresh`, and `refreshInstance`), all of which are reached only by a user action: the Settings refresh button, and