From 0b24aecba8d8a66eaae15d062d15dc9d328d395a Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Fri, 14 Aug 2026 02:17:25 +0200 Subject: [PATCH 1/3] fix(sdk): bound a credential provider call so an unreachable store fails, not hangs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A credential provider is frequently remote — the 1Password backend talks to a service over HTTP, and any custom provider may be a network store — so "stopped answering" is one of its ordinary failure modes rather than an exotic one. Nothing bounded the call, so a store that went away did not fail a tool invocation, it hung it, and nothing in the resulting silence named the provider. Measured before changing anything: with a provider whose `get` never returns, seeding and connection creation both succeed and the resolution never comes back. A control provider resolves normally, so the hang is the provider call and not the harness. `CredentialProvider` documents nothing about timing — no expectation that `get` returns promptly, no note that the caller will not bound it — so neither side owned this. Executor already bounds its other remote calls the same way, in OAuth discovery and in the MCP plugin's probes; credential resolution was the one that did not. Bounded once at the registration funnel rather than at each call site, so a method added later is bounded by default instead of by whoever remembers. Optional methods stay optional: a provider that cannot enumerate must not appear to. The failure names the provider and the operation, so the diagnostic points at the store rather than at whatever the caller happened to be doing. Thirty seconds is a backstop against a dead dependency, not a latency budget. The tests advance a virtual clock past it rather than waiting. --- packages/core/sdk/src/executor.ts | 66 ++++++++++- .../sdk/src/provider-call-timeout.test.ts | 108 ++++++++++++++++++ 2 files changed, 172 insertions(+), 2 deletions(-) create mode 100644 packages/core/sdk/src/provider-call-timeout.test.ts diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 4a962c3ab..07a8b2e30 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -1,4 +1,4 @@ -import { Effect, Inspectable, Layer, Option, Predicate, Schema } from "effect"; +import { Duration, Effect, Inspectable, Layer, Option, Predicate, Schema } from "effect"; import { FetchHttpClient, type HttpClient } from "effect/unstable/http"; import { fumadb } from "@executor-js/fumadb"; import { memoryAdapter } from "@executor-js/fumadb/adapters/memory"; @@ -1672,6 +1672,68 @@ export const createExecutor = ( + effect: Effect.Effect, + key: string, + operation: string, + ): Effect.Effect => + effect.pipe( + Effect.timeoutOrElse({ + duration: Duration.millis(CREDENTIAL_PROVIDER_TIMEOUT_MS), + orElse: () => + Effect.fail( + new StorageError({ + message: + `Credential provider "${key}" did not answer ${operation} within ` + + `${CREDENTIAL_PROVIDER_TIMEOUT_MS}ms. The store is unreachable or not responding; ` + + `the credential was not resolved.`, + cause: undefined, + }), + ), + }), + ); + + /** Wrap a provider so every call it exposes is bounded. + * + * Done once at the registration funnel rather than at each call site: every + * provider passes through here, so a method added later is bounded by default + * instead of by whoever remembers. Optional methods stay optional — a provider + * that cannot enumerate must not appear to. */ + const boundedProvider = (provider: CredentialProvider, key: string): CredentialProvider => { + const { has, set, delete: remove, list } = provider; + return { + ...provider, + get: (id) => boundedCall(provider.get(id), key, "get"), + ...(has ? { has: (id: ProviderItemId) => boundedCall(has(id), key, "has") } : {}), + ...(set + ? { set: (id: ProviderItemId, value: string) => boundedCall(set(id, value), key, "set") } + : {}), + ...(remove + ? { delete: (id: ProviderItemId) => boundedCall(remove(id), key, "delete") } + : {}), + ...(list ? { list: () => boundedCall(list(), key, "list") } : {}), + }; + }; + const registerCredentialProvider = ( provider: CredentialProvider, sourceLabel: string, @@ -1685,7 +1747,7 @@ export const createExecutor = ({ + key: STORE, + writable: true, + get, + set: () => Effect.void, +}); + +const plugin = (provider: CredentialProvider) => + definePlugin(() => ({ + id: "acme" as const, + credentialProviders: [provider], + storage: () => ({}), + extension: (ctx) => ({ + seed: () => ctx.core.integrations.register({ slug: INTEG, description: "Acme", config: {} }), + read: () => ctx.connections.resolveValue({ owner: "org", integration: INTEG, name: CONN }), + }), + }))(); + +const executorWithConnection = (provider: CredentialProvider) => + Effect.gen(function* () { + const executor = yield* createExecutor( + makeTestConfig({ plugins: [plugin(provider)] as const }), + ); + yield* executor.acme.seed(); + yield* executor.connections.create({ + owner: "org", + name: CONN, + integration: INTEG, + template: AuthTemplateSlug.make("api_key"), + from: { provider: STORE, id: ProviderItemId.make("item-1") }, + }); + return executor; + }); + +describe("a credential provider that stops answering", () => { + it.effect("fails the resolution instead of hanging it", () => + Effect.gen(function* () { + const executor = yield* executorWithConnection(providerWith(() => Effect.never)); + + const fiber = yield* Effect.forkChild(Effect.exit(executor.acme.read())); + yield* TestClock.adjust(Duration.minutes(5)); + const exit = yield* Fiber.join(fiber); + + expect(exit._tag).toBe("Failure"); + }), + ); + + it.effect("names the provider and the operation, not just a failure", () => + Effect.gen(function* () { + // A bare timeout would leave an operator looking at whatever the caller was + // doing rather than at the store that stopped answering. + const executor = yield* executorWithConnection(providerWith(() => Effect.never)); + + const fiber = yield* Effect.forkChild(Effect.exit(executor.acme.read())); + yield* TestClock.adjust(Duration.minutes(5)); + const exit = yield* Fiber.join(fiber); + + expect(String(exit)).toContain("remote-store"); + expect(String(exit)).toContain("did not answer"); + }), + ); + + it.effect("still resolves normally when the provider answers", () => + Effect.gen(function* () { + // The control. A bound that refused everything would satisfy both assertions + // above while breaking every working deployment. + const executor = yield* executorWithConnection(providerWith(() => Effect.succeed("tok"))); + + expect(yield* executor.acme.read()).toBe("tok"); + }), + ); +}); From 7cc550f9df0b324d0870775e2d22cf98c2bcb548 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Fri, 14 Aug 2026 18:24:20 +0200 Subject: [PATCH 2/3] fix(sdk): keep the provider's receiver when bounding its calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wrapper destructured the four optional methods and called the bindings bare, which drops `this`. `get` was already called on the provider, so the two disagreed. Every provider in the tree is an object literal and cannot notice; a provider written as a class — which is exactly what "wrap any provider" invites — threw TypeError on its first optional call. Covered by a class-based provider test, with an object-literal control that is identical except for that one difference, so a red result can only mean the receiver. Also pins the operation in the message-shape test, which asserted the provider and the phrasing but not the operation it is named for, and uses Exit.isFailure rather than inspecting _tag, which the repo's own no-manual-tag-check rule rejects. --- packages/core/sdk/src/executor.ts | 31 ++++++--- .../sdk/src/provider-call-timeout.test.ts | 67 ++++++++++++++++++- 2 files changed, 86 insertions(+), 12 deletions(-) diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 07a8b2e30..93407a4bb 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -1715,22 +1715,33 @@ export const createExecutor = { - const { has, set, delete: remove, list } = provider; + // Every method is invoked ON the provider. Destructuring them and calling the + // bindings bare drops `this`, which every in-tree provider survives only because + // it happens to be an object literal — a provider written as a class throws + // TypeError on its first call. Wrapping arbitrary providers is the point of this + // funnel, so it must not change how their own methods are called. return { ...provider, get: (id) => boundedCall(provider.get(id), key, "get"), - ...(has ? { has: (id: ProviderItemId) => boundedCall(has(id), key, "has") } : {}), - ...(set - ? { set: (id: ProviderItemId, value: string) => boundedCall(set(id, value), key, "set") } + ...(provider.has + ? { has: (id: ProviderItemId) => boundedCall(provider.has!(id), key, "has") } : {}), - ...(remove - ? { delete: (id: ProviderItemId) => boundedCall(remove(id), key, "delete") } + ...(provider.set + ? { + set: (id: ProviderItemId, value: string) => + boundedCall(provider.set!(id, value), key, "set"), + } + : {}), + ...(provider.delete + ? { delete: (id: ProviderItemId) => boundedCall(provider.delete!(id), key, "delete") } : {}), - ...(list ? { list: () => boundedCall(list(), key, "list") } : {}), + ...(provider.list ? { list: () => boundedCall(provider.list!(), key, "list") } : {}), }; }; diff --git a/packages/core/sdk/src/provider-call-timeout.test.ts b/packages/core/sdk/src/provider-call-timeout.test.ts index f61c38a93..2f2e262b6 100644 --- a/packages/core/sdk/src/provider-call-timeout.test.ts +++ b/packages/core/sdk/src/provider-call-timeout.test.ts @@ -15,7 +15,7 @@ // --------------------------------------------------------------------------- import { describe, expect, it } from "@effect/vitest"; -import { Duration, Effect, Fiber } from "effect"; +import { Duration, Effect, Exit, Fiber } from "effect"; import { TestClock } from "effect/testing"; import { createExecutor } from "./executor"; @@ -77,7 +77,7 @@ describe("a credential provider that stops answering", () => { yield* TestClock.adjust(Duration.minutes(5)); const exit = yield* Fiber.join(fiber); - expect(exit._tag).toBe("Failure"); + expect(Exit.isFailure(exit)).toBe(true); }), ); @@ -93,6 +93,69 @@ describe("a credential provider that stops answering", () => { expect(String(exit)).toContain("remote-store"); expect(String(exit)).toContain("did not answer"); + // The operation, too — without this the test passes its own name by accident: + // the operation could drop out of the message entirely and nothing would notice. + expect(String(exit)).toContain("get"); + }), + ); + + it.effect("an object-literal provider stores a pasted value — the control", () => + Effect.gen(function* () { + // The control for the class case below: identical in every respect except that the + // provider is an object literal. Without it, a red class test could mean anything. + const items = new Map(); + const lit: CredentialProvider = { + key: STORE, + writable: true, + get: (id) => Effect.sync(() => items.get(String(id)) ?? null), + set: (id, value) => Effect.sync(() => void items.set(String(id), value)), + }; + const executor = yield* createExecutor(makeTestConfig({ plugins: [plugin(lit)] as const })); + yield* executor.acme.seed(); + yield* executor.connections.create({ + owner: "org", + name: CONN, + integration: INTEG, + template: AuthTemplateSlug.make("api_key"), + value: "tok", + }); + expect(yield* executor.acme.read()).toBe("tok"); + }), + ); + + it.effect("wraps a CLASS-based provider without breaking its methods", () => + Effect.gen(function* () { + // The wrapper must not change HOW a provider's own methods are called. `get` was + // invoked with its receiver (`provider.get(id)`) but the optional methods were + // destructured and called bare, which silently drops `this`. Every in-tree provider + // is an object literal and cannot notice; a provider written as a class — exactly + // what "wrap any provider" invites — throws TypeError on the first optional call. + class ClassProvider { + readonly key = STORE; + readonly writable = true; + private readonly items = new Map(); + get(id: ProviderItemId) { + return Effect.sync(() => this.items.get(String(id)) ?? null); + } + set(id: ProviderItemId, value: string) { + // `this` is the whole point: bare invocation makes this line throw. + return Effect.sync(() => void this.items.set(String(id), value)); + } + } + + const executor = yield* createExecutor( + makeTestConfig({ plugins: [plugin(new ClassProvider() as CredentialProvider)] as const }), + ); + yield* executor.acme.seed(); + yield* executor.connections.create({ + owner: "org", + name: CONN, + integration: INTEG, + template: AuthTemplateSlug.make("api_key"), + value: "tok", + }); + + expect(yield* executor.acme.read()).toBe("tok"); }), ); From f9c799910f45e55c1f52ecf0d892d60dee575dc3 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Fri, 14 Aug 2026 22:51:38 +0200 Subject: [PATCH 3/3] fix(sdk): stop the bounded wrapper dropping a provider's prototype members MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wrapper spread the provider. A spread copies only own ENUMERABLE properties, so everything on a class's prototype — its methods, and accessors like `writable` — was dropped silently. Nothing raised: the wrapper simply appeared not to have the capability, and the caller took a path the provider meant to own. A class-based provider whose `writable` is an accessor stops being seen as a writable store at all, so creating a connection from a pasted value fails with "provider not registered: default". It now inherits through Object.create and shadows only the five methods it bounds, which also means a capability added to CredentialProvider later survives the wrapper without anyone remembering to list it here. Covered by a class-based provider whose `writable` lives on the prototype, verified failing before the change with exactly that error. --- packages/core/sdk/src/executor.ts | 50 +++++++++++-------- .../sdk/src/provider-call-timeout.test.ts | 41 +++++++++++++++ 2 files changed, 70 insertions(+), 21 deletions(-) diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 93407a4bb..4b099bb57 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -1721,28 +1721,36 @@ export const createExecutor = { - // Every method is invoked ON the provider. Destructuring them and calling the - // bindings bare drops `this`, which every in-tree provider survives only because - // it happens to be an object literal — a provider written as a class throws - // TypeError on its first call. Wrapping arbitrary providers is the point of this - // funnel, so it must not change how their own methods are called. - return { - ...provider, - get: (id) => boundedCall(provider.get(id), key, "get"), - ...(provider.has - ? { has: (id: ProviderItemId) => boundedCall(provider.has!(id), key, "has") } - : {}), - ...(provider.set - ? { - set: (id: ProviderItemId, value: string) => - boundedCall(provider.set!(id, value), key, "set"), - } - : {}), - ...(provider.delete - ? { delete: (id: ProviderItemId) => boundedCall(provider.delete!(id), key, "delete") } - : {}), - ...(provider.list ? { list: () => boundedCall(provider.list!(), key, "list") } : {}), + // Wrapping must change neither how the provider's methods are CALLED nor what the + // object LOOKS like. + // + // Spreading would break the second: a spread copies only own ENUMERABLE properties, so + // everything on a class's prototype — its methods, and accessors like `writable` — is + // dropped silently. Nothing raises; the wrapper simply appears not to have the capability + // and the caller takes a path the provider meant to own. `Object.create` keeps the whole + // object reachable, including anything added to the interface later. + // + // Each bounded method is invoked ON the provider, which is the first half: a destructured + // binding called bare loses `this`, and a class-based provider throws TypeError on its + // first call. Every provider in this repo is an object literal and cannot notice either + // problem, but "wrap any provider" is the whole point of this funnel. + const bounded: Record = { + get: (id: ProviderItemId) => boundedCall(provider.get(id), key, "get"), }; + if (provider.has) { + bounded.has = (id: ProviderItemId) => boundedCall(provider.has!(id), key, "has"); + } + if (provider.set) { + bounded.set = (id: ProviderItemId, value: string) => + boundedCall(provider.set!(id, value), key, "set"); + } + if (provider.delete) { + bounded.delete = (id: ProviderItemId) => boundedCall(provider.delete!(id), key, "delete"); + } + if (provider.list) { + bounded.list = () => boundedCall(provider.list!(), key, "list"); + } + return Object.assign(Object.create(provider) as CredentialProvider, bounded); }; const registerCredentialProvider = ( diff --git a/packages/core/sdk/src/provider-call-timeout.test.ts b/packages/core/sdk/src/provider-call-timeout.test.ts index 2f2e262b6..81039d9fc 100644 --- a/packages/core/sdk/src/provider-call-timeout.test.ts +++ b/packages/core/sdk/src/provider-call-timeout.test.ts @@ -159,6 +159,47 @@ describe("a credential provider that stops answering", () => { }), ); + it.effect("keeps a capability the provider defines on its PROTOTYPE", () => + Effect.gen(function* () { + // The wrapper must not change the provider's SHAPE either. A spread copies only own + // ENUMERABLE properties, so anything on a class's prototype — every method, and any + // accessor like the `writable` below — is dropped silently. Nothing raises; the wrapper + // simply appears not to have it, and the caller takes a path the provider meant to own. + // Here that means `defaultWritableProvider` no longer sees a writable store, so creating + // a connection from a pasted value fails with no provider at all. + class PrototypeProvider { + readonly key = STORE; + private readonly items = new Map(); + // On the PROTOTYPE, not the instance — this is the property a spread loses. + get writable() { + return true; + } + get(id: ProviderItemId) { + return Effect.sync(() => this.items.get(String(id)) ?? null); + } + set(id: ProviderItemId, value: string) { + return Effect.sync(() => void this.items.set(String(id), value)); + } + } + + const executor = yield* createExecutor( + makeTestConfig({ + plugins: [plugin(new PrototypeProvider() as CredentialProvider)] as const, + }), + ); + yield* executor.acme.seed(); + yield* executor.connections.create({ + owner: "org", + name: CONN, + integration: INTEG, + template: AuthTemplateSlug.make("api_key"), + value: "tok", + }); + + expect(yield* executor.acme.read()).toBe("tok"); + }), + ); + it.effect("still resolves normally when the provider answers", () => Effect.gen(function* () { // The control. A bound that refused everything would satisfy both assertions