History

refactor(core,plugins): complete Captured migration — core handlers + secret providers

Wave 1 of the cleanup sweep: **Core API handlers** (wrap ExecutorService, drop orDie): - `ExecutorService` service tag flipped from `Executor` to `Captured<Executor>`. - 13 `.pipe(Effect.orDie)` sites across tools/sources/secrets handlers removed; handlers now just `yield* executor.X.foo(...)`. - Added `.addError(InternalError)` at the core tools/sources/secrets group level (was missing). - Promoted `SourceRemovalNotAllowedError` to a typed 409 on the `sources.remove` endpoint; previously orDie'd into a 500. - Host wiring in cloud `protected.ts`, local `main.ts`, and cloud test-harness flipped to pass `wrapped` (the `withCapture` proxy) to `ExecutorService` instead of the raw executor. **SecretProvider impls now emit StorageError with cause preserved:** - `keychain/src/provider.ts`: replaced `Effect.orElseSucceed(...)` (silent swallow) with `Effect.mapError(err → new StorageError({ message, cause }))`. Legitimate absences still return `null`/`false`; real keychain errors now surface as InternalError(traceId) with the original cause in Sentry. - `file-secrets/src/index.ts`: replaced `Effect.sync` + blanket try/catch in `readFullFile` with `Effect.try` + explicit ENOENT handling. JSON parse errors, schema decode failures, FS permission errors all propagate as StorageError instead of becoming defects or being swallowed. **Subtle `Captured<T>` type fix:** - Added a `CapturedOpaque` leaf (primitives + Date/Promise/Error/Array) before the recursive `extends object` branch in the `Captured<T>` conditional type. Previously, branded strings (`ScopeId = string & Brand<"ScopeId">`) satisfied `extends object` via the Brand marker's shape, so `Captured<>` expanded them into the whole string-prototype shape — breaking any handler that returned a branded id. Mirrors the runtime `isPlainObject` short-circuit. No casts needed. 34/34 typecheck, 35+7+27+23 tests green across sdk/api/cloud.

Rhys Sullivan committed 7 months ago

Commit e36b8361 parent 3d3d39f13 files · +156 −61Browse files

Unified · Split

13 files
  1. apps/cloud/src/api/protected.ts +1 −1
  2. apps/cloud/src/services/__test-harness__/api-harness.ts +1 −1
  3. apps/local/src/server/main.ts +1 −1
  4. packages/core/api/src/handlers/secrets.ts +10 −12
  5. packages/core/api/src/handlers/sources.ts +5 −7
  6. packages/core/api/src/handlers/tools.ts +2 −2
  7. packages/core/api/src/observability.ts +32 −3
  8. packages/core/api/src/secrets/api.ts +4 −1
  9. packages/core/api/src/services.ts +5 −1
  10. packages/core/api/src/sources/api.ts +16 −5
  11. packages/core/api/src/tools/api.ts +4 −1
  12. packages/plugins/file-secrets/src/index.ts +57 −22
  13. packages/plugins/keychain/src/provider.ts +18 −4
apps/cloud/src/api/protected.ts+1−1
@@ -55,7 +55,7 @@ const createProtectedApp = (organizationId: string, organizationName: string) => · 54 unmodified lines
5555 // already-captured shape. See notes/error-handling.md.
5656 const wrapped = withCapture(executor);
5757 const requestServices = Layer.mergeAll(
58− Layer.succeed(ExecutorService, executor),
58+ Layer.succeed(ExecutorService, wrapped),
5959 Layer.succeed(ExecutionEngineService, engine),
6060 Layer.succeed(OpenApiExtensionService, wrapped.openapi),
6161 Layer.succeed(McpExtensionService, wrapped.mcp),
apps/cloud/src/services/__test-harness__/api-harness.ts+1−1
@@ -219,7 +219,7 @@ const buildAppForScope = (scopeId: string, scopeName: string) => · 218 unmodified lines
219219 const engine = createExecutionEngine({ executor });
220220 const wrapped = withCapture(executor);
221221 const services = Layer.mergeAll(
222− Layer.succeed(ExecutorService, executor),
222+ Layer.succeed(ExecutorService, wrapped),
223223 Layer.succeed(ExecutionEngineService, engine),
224224 Layer.succeed(OpenApiExtensionService, wrapped.openapi),
225225 Layer.succeed(McpExtensionService, wrapped.mcp),
apps/local/src/server/main.ts+1−1
@@ -113,7 +113,7 @@ export const createServerHandlers = async (): Promise<ServerHandlers> => { · 112 unmodified lines
113113 Layer.provideMerge(HttpApiBuilder.middlewareOpenApi()),
114114 Layer.provideMerge(LocalApiBase),
115115 Layer.provideMerge(pluginExtensions),
116− Layer.provideMerge(Layer.succeed(ExecutorService, executor)),
116+ Layer.provideMerge(Layer.succeed(ExecutorService, wrapped)),
117117 Layer.provideMerge(Layer.succeed(ExecutionEngineService, engine)),
118118 Layer.provideMerge(HttpServer.layerContext),
119119 Layer.provideMerge(HttpRouter.setRouterConfig({ maxParamLength: 1000 })),
packages/core/api/src/handlers/secrets.ts+10−12
@@ -18,35 +18,33 @@ export const SecretsHandlers = HttpApiBuilder.group(ExecutorApi, "secrets", (han · 17 unmodified lines
1818 .handle("list", () =>
1919 Effect.gen(function* () {
2020 const executor = yield* ExecutorService;
21− const refs = yield* executor.secrets.list().pipe(Effect.orDie);
21+ const refs = yield* executor.secrets.list();
2222 return refs.map(refToResponse);
2323 }),
2424 )
2525 .handle("status", ({ path }) =>
2626 Effect.gen(function* () {
2727 const executor = yield* ExecutorService;
28− const status = yield* executor.secrets.status(path.secretId).pipe(Effect.orDie);
28+ const status = yield* executor.secrets.status(path.secretId);
2929 return { secretId: path.secretId, status };
3030 }),
3131 )
3232 .handle("set", ({ payload }) =>
3333 Effect.gen(function* () {
3434 const executor = yield* ExecutorService;
35− const ref = yield* executor.secrets
36− .set({
37− id: payload.id,
38− name: payload.name,
39− value: payload.value,
40− provider: payload.provider,
41− })
42− .pipe(Effect.orDie);
35+ const ref = yield* executor.secrets.set({
36+ id: payload.id,
37+ name: payload.name,
38+ value: payload.value,
39+ provider: payload.provider,
40+ });
4341 return refToResponse(ref);
4442 }),
4543 )
4644 .handle("resolve", ({ path }) =>
4745 Effect.gen(function* () {
4846 const executor = yield* ExecutorService;
49− const value = yield* executor.secrets.get(path.secretId).pipe(Effect.orDie);
47+ const value = yield* executor.secrets.get(path.secretId);
5048 if (value === null) {
5149 return yield* Effect.fail(new SecretNotFoundError({ secretId: path.secretId }));
5250 }
@@ -56,7 +54,7 @@ export const SecretsHandlers = HttpApiBuilder.group(ExecutorApi, "secrets", (han · 3 unmodified lines
5654 .handle("remove", ({ path }) =>
5755 Effect.gen(function* () {
5856 const executor = yield* ExecutorService;
59− yield* executor.secrets.remove(path.secretId).pipe(Effect.orDie);
57+ yield* executor.secrets.remove(path.secretId);
6058 return { removed: true };
6159 }),
6260 ),
packages/core/api/src/handlers/sources.ts+5−7
@@ -10,7 +10,7 @@ export const SourcesHandlers = HttpApiBuilder.group(ExecutorApi, "sources", (han · 9 unmodified lines
1010 .handle("list", () =>
1111 Effect.gen(function* () {
1212 const executor = yield* ExecutorService;
13− const sources = yield* executor.sources.list().pipe(Effect.orDie);
13+ const sources = yield* executor.sources.list();
1414 return sources.map((s) => ({
1515 id: s.id,
1616 name: s.name,
@@ -26,23 +26,21 @@ export const SourcesHandlers = HttpApiBuilder.group(ExecutorApi, "sources", (han · 9 unmodified lines
2626 .handle("remove", ({ path }) =>
2727 Effect.gen(function* () {
2828 const executor = yield* ExecutorService;
29− yield* executor.sources.remove(path.sourceId).pipe(Effect.orDie);
29+ yield* executor.sources.remove(path.sourceId);
3030 return { removed: true };
3131 }),
3232 )
3333 .handle("refresh", ({ path }) =>
3434 Effect.gen(function* () {
3535 const executor = yield* ExecutorService;
36− yield* executor.sources.refresh(path.sourceId).pipe(Effect.orDie);
36+ yield* executor.sources.refresh(path.sourceId);
3737 return { refreshed: true };
3838 }),
3939 )
4040 .handle("tools", ({ path }) =>
4141 Effect.gen(function* () {
4242 const executor = yield* ExecutorService;
43− const tools = yield* executor.tools
44− .list({ sourceId: path.sourceId })
45− .pipe(Effect.orDie);
43+ const tools = yield* executor.tools.list({ sourceId: path.sourceId });
4644 return tools.map((t) => ({
4745 id: ToolId.make(t.id),
4846 pluginId: t.pluginId,
@@ -56,7 +54,7 @@ export const SourcesHandlers = HttpApiBuilder.group(ExecutorApi, "sources", (han · 7 unmodified lines
5654 .handle("detect", ({ payload }) =>
5755 Effect.gen(function* () {
5856 const executor = yield* ExecutorService;
59− const results = yield* executor.sources.detect(payload.url).pipe(Effect.orDie);
57+ const results = yield* executor.sources.detect(payload.url);
6058 return results.map((r) => ({
6159 kind: r.kind,
6260 confidence: r.confidence,
packages/core/api/src/handlers/tools.ts+2−2
@@ -10,7 +10,7 @@ export const ToolsHandlers = HttpApiBuilder.group(ExecutorApi, "tools", (handler · 9 unmodified lines
1010 .handle("list", () =>
1111 Effect.gen(function* () {
1212 const executor = yield* ExecutorService;
13− const tools = yield* executor.tools.list().pipe(Effect.orDie);
13+ const tools = yield* executor.tools.list();
1414 return tools.map((t) => ({
1515 id: ToolId.make(t.id),
1616 pluginId: t.pluginId,
@@ -24,7 +24,7 @@ export const ToolsHandlers = HttpApiBuilder.group(ExecutorApi, "tools", (handler · 7 unmodified lines
2424 .handle("schema", ({ path }) =>
2525 Effect.gen(function* () {
2626 const executor = yield* ExecutorService;
27− const schema = yield* executor.tools.schema(path.toolId).pipe(Effect.orDie);
27+ const schema = yield* executor.tools.schema(path.toolId);
2828 if (schema === null) {
2929 return yield* Effect.fail(new ToolNotFoundError({ toolId: path.toolId }));
3030 }
packages/core/api/src/observability.ts+32−3
@@ -146,6 +146,33 @@ const isPlainObject = (v: unknown): v is Record<string | symbol, unknown> => { · 145 unmodified lines
146146 * Captured<McpPluginExtension>
147147 * >() {}
148148 */
149+// Opaque leaves that we DO NOT want `Captured<T>` to descend into at the
150+// type level — the runtime Proxy also leaves these alone (via
151+// `isPlainObject` returning false for class instances). Includes:
152+//
153+// - primitives (branded strings/numbers show up as intersections with
154+// `{ [BrandTypeId]: ... }`, which TS reads as `extends object` — we
155+// have to exclude them explicitly),
156+// - common stdlib boxed types (Date, Promise, Error, Array),
157+// - Schema-class instances and tagged errors. These all carry a
158+// non-Object prototype at runtime so the Proxy short-circuits; the
159+// type mirror stops with the same shape.
160+//
161+// Anything else that's a plain object (the executor surface, plugin
162+// extensions, their namespaced sub-objects) walks recursively.
163+type CapturedOpaque =
164+ | string
165+ | number
166+ | boolean
167+ | bigint
168+ | symbol
169+ | null
170+ | undefined
171+ | Date
172+ | Promise<unknown>
173+ | Error
174+ | readonly unknown[];
175+
149176 export type Captured<T> = T extends (
150177 ...args: infer A
151178 ) => Effect.Effect<infer X, infer E, infer R>
@@ -154,9 +181,11 @@ export type Captured<T> = T extends ( · 2 unmodified lines
154181 ) => Effect.Effect<X, Exclude<E, StorageFailure> | InternalError, R>
155182 : T extends (...args: infer A) => infer U
156183 ? (...args: A) => U
157− : T extends object
158− ? { readonly [K in keyof T]: Captured<T[K]> }
159− : T;
184+ : T extends CapturedOpaque
185+ ? T
186+ : T extends object
187+ ? { readonly [K in keyof T]: Captured<T[K]> }
188+ : T;
160189
161190 export const withCapture = <T extends object>(value: T): Captured<T> => {
162191 return new Proxy(value, {
packages/core/api/src/secrets/api.ts+4−1
@@ -2,6 +2,8 @@ import { HttpApiEndpoint, HttpApiGroup, HttpApiSchema } from "@effect/platform"; · 1 unmodified line
22 import { Schema } from "effect";
33 import { ScopeId, SecretId, SecretNotFoundError, SecretResolutionError } from "@executor/sdk";
44
5+import { InternalError } from "../observability";
6+
57 // ---------------------------------------------------------------------------
68 // Params
79 // ---------------------------------------------------------------------------
@@ -78,4 +80,5 @@ export class SecretsApi extends HttpApiGroup.make("secrets") · 70 unmodified lines
7880 HttpApiEndpoint.del("remove")`/scopes/${scopeIdParam}/secrets/${secretIdParam}`
7981 .addSuccess(Schema.Struct({ removed: Schema.Boolean }))
8082 .addError(SecretNotFound),
81− ) {}
83+ )
84+ .addError(InternalError) {}
packages/core/api/src/services.ts+5−1
@@ -1,8 +1,12 @@
11 import { Context } from "effect";
22 import type { Executor } from "@executor/sdk";
33 import type { ExecutionEngine } from "@executor/execution";
4+import type { Captured } from "./observability";
45
5−export class ExecutorService extends Context.Tag("ExecutorService")<ExecutorService, Executor>() {}
6+export class ExecutorService extends Context.Tag("ExecutorService")<
7+ ExecutorService,
8+ Captured<Executor>
9+>() {}
610
711 export class ExecutionEngineService extends Context.Tag("ExecutionEngineService")<
812 ExecutionEngineService,
packages/core/api/src/sources/api.ts+16−5
@@ -1,6 +1,8 @@
11 import { HttpApiEndpoint, HttpApiGroup, HttpApiSchema } from "@effect/platform";
22 import { Schema } from "effect";
3−import { ScopeId, ToolId } from "@executor/sdk";
3+import { ScopeId, SourceRemovalNotAllowedError, ToolId } from "@executor/sdk";
4+
5+import { InternalError } from "../observability";
46
57 // ---------------------------------------------------------------------------
68 // Params
@@ -53,6 +55,14 @@ const DetectResultResponse = Schema.Struct({ · 46 unmodified lines
5355 namespace: Schema.String,
5456 });
5557
58+// ---------------------------------------------------------------------------
59+// Error schemas with HTTP status annotations
60+// ---------------------------------------------------------------------------
61+
62+const SourceRemovalNotAllowed = SourceRemovalNotAllowedError.annotations(
63+ HttpApiSchema.annotations({ status: 409 }),
64+);
65+
5666 // ---------------------------------------------------------------------------
5767 // Group
5868 // ---------------------------------------------------------------------------
@@ -64,9 +74,9 @@ export class SourcesApi extends HttpApiGroup.make("sources") · 5 unmodified lines
6474 ),
6575 )
6676 .add(
67− HttpApiEndpoint.del("remove")`/scopes/${scopeIdParam}/sources/${sourceIdParam}`.addSuccess(
68− SourceRemoveResponse,
69− ),
77+ HttpApiEndpoint.del("remove")`/scopes/${scopeIdParam}/sources/${sourceIdParam}`
78+ .addSuccess(SourceRemoveResponse)
79+ .addError(SourceRemovalNotAllowed),
7080 )
7181 .add(
7282 HttpApiEndpoint.post(
@@ -82,4 +92,5 @@ export class SourcesApi extends HttpApiGroup.make("sources") · 9 unmodified lines
8292 HttpApiEndpoint.post("detect")`/scopes/${scopeIdParam}/sources/detect`
8393 .setPayload(DetectRequest)
8494 .addSuccess(Schema.Array(DetectResultResponse)),
85− ) {}
95+ )
96+ .addError(InternalError) {}
packages/core/api/src/tools/api.ts+4−1
@@ -2,6 +2,8 @@ import { HttpApiEndpoint, HttpApiGroup, HttpApiSchema } from "@effect/platform"; · 1 unmodified line
22 import { Schema } from "effect";
33 import { ScopeId, ToolId, ToolNotFoundError } from "@executor/sdk";
44
5+import { InternalError } from "../observability";
6+
57 // ---------------------------------------------------------------------------
68 // Params
79 // ---------------------------------------------------------------------------
@@ -53,4 +55,5 @@ export class ToolsApi extends HttpApiGroup.make("tools") · 45 unmodified lines
5355 HttpApiEndpoint.get("schema")`/scopes/${scopeIdParam}/tools/${toolIdParam}/schema`
5456 .addSuccess(ToolSchemaResponse)
5557 .addError(ToolNotFound),
56− ) {}
58+ )
59+ .addError(InternalError) {}
packages/plugins/file-secrets/src/index.ts+57−22
@@ -2,7 +2,12 @@ import { Effect, Schema } from "effect"; · 1 unmodified line
22 import * as fs from "node:fs";
33 import * as path from "node:path";
44
5−import { definePlugin, type PluginCtx, type SecretProvider } from "@executor/sdk";
5+import {
6+ definePlugin,
7+ StorageError,
8+ type PluginCtx,
9+ type SecretProvider,
10+} from "@executor/sdk";
611
712 // ---------------------------------------------------------------------------
813 // XDG data dir resolution
@@ -43,16 +48,28 @@ const decodeScopedAuthFile = Schema.decodeUnknownSync(ScopedAuthFile); · 34 unmodified lines
4348
4449 // ---------------------------------------------------------------------------
4550 // File I/O with restricted permissions
51+//
52+// These helpers throw on real I/O or decode failures — the provider wraps
53+// every call in `Effect.try` so those throws surface as typed
54+// `StorageError` on the Effect error channel. Previously `readFullFile`
55+// used a blanket `try { ... } catch { return {}; }` which masked JSON
56+// parse errors, schema decode failures, and permission errors as
57+// "empty file", making misconfigured installs silently return null from
58+// every `get`.
4659 // ---------------------------------------------------------------------------
4760
4861 const readFullFile = (filePath: string): Record<string, Record<string, string>> => {
62+ if (!fs.existsSync(filePath)) return {};
63+ let raw: string;
4964 try {
50− if (!fs.existsSync(filePath)) return {};
51− const raw = fs.readFileSync(filePath, "utf-8");
52− return decodeScopedAuthFile(JSON.parse(raw));
53− } catch {
54− return {};
65+ raw = fs.readFileSync(filePath, "utf-8");
66+ } catch (cause) {
67+ // Treat "file disappeared between existsSync and readFileSync" as
68+ // absence — anything else (EACCES, EISDIR, …) propagates.
69+ if ((cause as NodeJS.ErrnoException).code === "ENOENT") return {};
70+ throw cause;
5571 }
72+ return decodeScopedAuthFile(JSON.parse(raw));
5673 };
5774
5875 const readScopeSecrets = (filePath: string, scopeId: string): Record<string, string> =>
@@ -100,36 +117,54 @@ export interface FileSecretsExtension { · 41 unmodified lines
100117 // Provider factory (internal)
101118 // ---------------------------------------------------------------------------
102119
120+const toStorageError = (cause: unknown) =>
121+ new StorageError({
122+ message: cause instanceof Error ? cause.message : String(cause),
123+ cause,
124+ });
125+
103126 const makeScopedProvider = (filePath: string, scopeId: string): SecretProvider => ({
104127 key: "file",
105128 writable: true,
106129
107130 get: (secretId) =>
108− Effect.sync(() => {
109− const data = readScopeSecrets(filePath, scopeId);
110− return data[secretId] ?? null;
131+ Effect.try({
132+ try: () => {
133+ const data = readScopeSecrets(filePath, scopeId);
134+ return data[secretId] ?? null;
135+ },
136+ catch: toStorageError,
111137 }),
112138
113139 set: (secretId, value) =>
114− Effect.sync(() => {
115− const data = readScopeSecrets(filePath, scopeId);
116− data[secretId] = value;
117− writeScopeSecrets(filePath, scopeId, data);
140+ Effect.try({
141+ try: () => {
142+ const data = readScopeSecrets(filePath, scopeId);
143+ data[secretId] = value;
144+ writeScopeSecrets(filePath, scopeId, data);
145+ },
146+ catch: toStorageError,
118147 }),
119148
120149 delete: (secretId) =>
121− Effect.sync(() => {
122− const data = readScopeSecrets(filePath, scopeId);
123− const had = secretId in data;
124− delete data[secretId];
125− if (had) writeScopeSecrets(filePath, scopeId, data);
126− return had;
150+ Effect.try({
151+ try: () => {
152+ const data = readScopeSecrets(filePath, scopeId);
153+ const had = secretId in data;
154+ delete data[secretId];
155+ if (had) writeScopeSecrets(filePath, scopeId, data);
156+ return had;
157+ },
158+ catch: toStorageError,
127159 }),
128160
129161 list: () =>
130− Effect.sync(() => {
131− const data = readScopeSecrets(filePath, scopeId);
132− return Object.keys(data).map((k) => ({ id: k, name: k }));
162+ Effect.try({
163+ try: () => {
164+ const data = readScopeSecrets(filePath, scopeId);
165+ return Object.keys(data).map((k) => ({ id: k, name: k }));
166+ },
167+ catch: toStorageError,
133168 }),
134169 });
135170
packages/plugins/keychain/src/provider.ts+18−4
@@ -1,21 +1,35 @@
11 import { Effect } from "effect";
22
3−import type { SecretProvider } from "@executor/sdk";
3+import { StorageError, type SecretProvider } from "@executor/sdk";
44
55 import { getPassword, setPassword, deletePassword } from "./keyring";
66
77 // ---------------------------------------------------------------------------
88 // SecretProvider adapter — bridges keyring into SDK resolution chain
9+//
10+// The underlying `@napi-rs/keyring` sync API encodes "no entry" as an
11+// ordinary return value (`getPassword()` → `null`, `deletePassword()` →
12+// `false`), and only throws on real failures (keychain locked, permission
13+// denied, platform init failure, etc.). `keyring.ts` wraps those thrown
14+// failures as `KeychainError`. We translate `KeychainError` →
15+// `StorageError` so the HTTP edge can capture it to telemetry and surface
16+// an opaque `InternalError({ traceId })` — previously `orElseSucceed`
17+// silently converted every failure into "nothing found", which made it
18+// impossible to debug why secrets weren't resolving.
919 // ---------------------------------------------------------------------------
1020
21+const toStorageError = (cause: { readonly message: string; readonly cause?: unknown }) =>
22+ new StorageError({ message: cause.message, cause: cause.cause ?? cause });
23+
1124 export const makeKeychainProvider = (serviceName: string): SecretProvider => ({
1225 key: "keychain",
1326 writable: true,
14− get: (secretId) => getPassword(serviceName, secretId).pipe(Effect.orElseSucceed(() => null)),
27+ get: (secretId) =>
28+ getPassword(serviceName, secretId).pipe(Effect.mapError(toStorageError)),
1529 set: (secretId, value) =>
16− setPassword(serviceName, secretId, value).pipe(Effect.orElseSucceed(() => undefined)),
30+ setPassword(serviceName, secretId, value).pipe(Effect.mapError(toStorageError)),
1731 delete: (secretId) =>
18− deletePassword(serviceName, secretId).pipe(Effect.orElseSucceed(() => false)),
32+ deletePassword(serviceName, secretId).pipe(Effect.mapError(toStorageError)),
1933 // Keychain doesn't support enumerating — you need to know the account name
2034 list: undefined,
2135 });