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..a7c4243a1701 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts @@ -31,19 +31,25 @@ 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"; +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 +154,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 +320,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 +480,270 @@ 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 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), + disarm: Ref.set(armed, false), + 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* () { + 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)), + ); + + 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, + ]); + // 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); + expect((yield* registry.listInstances).map((instance) => instance.instanceId)).toEqual([ + firstId, + secondId, + ]); + expect(yield* registry.listUnavailable).toEqual([]); + }).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)), + ); + + // 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; + 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 fb75652e3856..6cd1f9ae3125 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,15 +404,111 @@ 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. 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, + ) => + 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; + } + + // 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. + // + // 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, + parentScope, + instanceId, + rawInstanceId: instanceId, + entry, + }); + + // 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)), + ); + const registry: ProviderInstanceRegistryShape = { getInstance: (id) => Ref.get(entries).pipe(Effect.map((map) => map.get(id)?.instance)), listInstances: Ref.get(entries).pipe( @@ -389,6 +520,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..6d4f8318f86f 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,179 @@ 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: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), + 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)); + }), + ); + + // 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: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), + 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"); @@ -957,6 +1136,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 +1155,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 +1267,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 +1285,7 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsModule.layerTest(), Te }), ), Layer.provideMerge(NodeServices.layer), + Layer.provideMerge(ProviderSecretResolverPassthroughLayer), ), ).pipe(Scope.provide(scope)); @@ -1193,6 +1376,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 +1395,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 +1488,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 +1505,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 +1615,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 +1711,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 +1828,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 +1913,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..fa65677ca2ff 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,49 @@ 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; + // 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), + ), + ); + 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..8179d03aa311 --- /dev/null +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts @@ -0,0 +1,153 @@ +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, { variables: environment, unresolved: [] }); + 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.variables?.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.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)))); + }); + + 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..f3cb3b9a42c9 --- /dev/null +++ b/apps/server/src/provider/Layers/ProviderSecretResolverLive.ts @@ -0,0 +1,127 @@ +/** + * 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 { variables: environment, unresolved: [] }; + } + const resolved: Array = []; + const unresolved: 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) { + unresolved.push(variable.name); + continue; + } + resolved.push({ ...variable, value: secret }); + } + 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/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..644ffa6828d2 --- /dev/null +++ b/apps/server/src/provider/Services/ProviderSecretResolver.ts @@ -0,0 +1,65 @@ +/** + * 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"; + +import type { ResolvedProviderInstanceEnvironment } from "../ProviderInstanceEnvironment.ts"; + +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) 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. `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; + /** + * 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: (environment) => Effect.succeed({ variables: environment, unresolved: [] }), + 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..ff7b1e104d44 --- /dev/null +++ b/docs/fork/0016-provider-secrets-live-in-1password.md @@ -0,0 +1,54 @@ +# 0016: Provider secrets can live in 1Password + +- PR: [TrogonStack/t3code#27](https://github.com/TrogonStack/t3code/pull/27) +- 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..6ad790c74414 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, [#27](https://github.com/TrogonStack/t3code/pull/27) 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..910de3d695fa 100644 --- a/docs/internals/providers.md +++ b/docs/internals/providers.md @@ -39,6 +39,72 @@ 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 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. 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. +- **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. + +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. 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, 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. 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 +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 +166,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.