diff --git a/.changeset/effect-filesystem-typegen.md b/.changeset/effect-filesystem-typegen.md new file mode 100644 index 000000000..693e60c96 --- /dev/null +++ b/.changeset/effect-filesystem-typegen.md @@ -0,0 +1,5 @@ +--- +"agent-bundle": patch +--- + +Publish `.agent-bundle/routes.d.ts` during project preparation (`agent-bundle build`, `agent-bundle dev`, and every API call that prepares a project) through Effect `FileSystem`: the staging file is removed on every exit path, including interruption, while the generated declarations, the atomic rename, and thrown Node errors are unchanged. (#520) diff --git a/docs/effect-conventions.md b/docs/effect-conventions.md index 7eafad7ca..e779cd086 100644 --- a/docs/effect-conventions.md +++ b/docs/effect-conventions.md @@ -256,7 +256,11 @@ contract). without `force` and `orDie`s, so an operation that deleted its own staging directory would fail an already successful call, and a real cleanup error would surface as the `PlatformError` wrapper (scope finalizers cannot fail - typed). Tests may use `makeTempDirectoryScoped` for fixtures. + typed). Tests may use `makeTempDirectoryScoped` for fixtures. + Same shape for a staging *file*: `ensuringRemoved(path, use)` is the + `try`/`finally` `rm(path, { force: true })` bracket behind + `withTempDirectory`; `routes/typegen.ts` uses it around its + write-then-rename of `.agent-bundle/routes.d.ts`. **Not** when ownership of the directory is transferred to a longer-lived object (the MCP session plugin-data dir in `dev/mcp-session/mcp-session-service.ts`): a scoped temp is removed when @@ -322,18 +326,23 @@ the bundle. `packages/agent-bundle/src/effect/platform.ts` owns the framework's platform layer: `platformLayer` (the `NodeServices` union composed from -`@effect/platform-node-shared`), `withTempDirectory`, -`unwrapPlatformError`, and `runWithPlatform`, which provides the layer and unwraps `PlatformError` -before handing off to `boundary.ts`'s `runPromise`. It is the only module -that imports `effect/PlatformError`: `boundary.ts` is bundled into every -emitted hook wrapper, and the error class would drag `Data.TaggedError` into -each one (measured: +12 kB per hook). Phase-1 callers are the throwaway -artifact in `api.ts` (`listMcp` / `invokeMcp` / `runMcp` / `listHooks` / -`simulateHook` without `artifact`) and the Codex validator's -schema-generation directory, both through `withTempDirectory`. Emitted -artifacts, hook wrappers, and compiler hot paths -never import this module; the dev server picks it up in phase 2 through -`makeScopedEffectRuntime(platformLayer)`. +`@effect/platform-node-shared`), `withTempDirectory`, `ensuringRemoved`, +`unwrapPlatformError`, and `runWithPlatform`, which provides the layer and +unwraps `PlatformError` before handing off to `boundary.ts`'s `runPromise`. +It is the only module that imports `effect/PlatformError`: `boundary.ts` is +bundled into every emitted hook wrapper, and the error class would drag +`Data.TaggedError` into each one (measured: +12 kB per hook). Phase-1 +callers are the throwaway artifact in `api.ts` (`listMcp` / `invokeMcp` / +`runMcp` / `listHooks` / `simulateHook` without `artifact`) and the Codex +validator's schema-generation directory, both through `withTempDirectory`, +and `routes/typegen.ts`'s `writeRouteTypesProgram` (the atomic +`routes.d.ts` publish, through `ensuringRemoved`; `writeRouteTypes` keeps +its Promise signature via `runWithPlatform`). The sibling `routes/graph.ts` +reads stay raw: `compileRouteGraph` is the compiler/cold-start discovery +path, and lifting only its two async reads would add an Effect runtime per +compile for nothing. Emitted artifacts, hook wrappers, and compiler hot +paths never import this module; the dev server picks it up in phase 2 +through `makeScopedEffectRuntime(platformLayer)`. ## Effect Schema wire contracts (Schema projections) diff --git a/packages/agent-bundle/src/effect/platform.ts b/packages/agent-bundle/src/effect/platform.ts index cc3aaffde..56f99aa74 100644 --- a/packages/agent-bundle/src/effect/platform.ts +++ b/packages/agent-bundle/src/effect/platform.ts @@ -60,23 +60,42 @@ export const unwrapPlatformError = (error: E): Exclude | Er : (error as Exclude); /** - * `const dir = await mkdtemp(...); try { return await use(dir) } finally - * { await rm(dir, { recursive: true, force: true }) }` as an Effect, with - * the same contract the two `try`/`finally` sites had before they moved - * onto Effect: + * `try { return await use } finally { await rm(path, { recursive: true, + * force: true }) }` as an Effect, with the contract the `try`/`finally` + * sites had before they moved onto Effect: * * - `force: true` — an operation that removed (or renamed away) its own - * staging directory does not fail the call; + * staging path does not fail the call; * - the cleanup failure is a typed `PlatformError` on the error channel * (unwrapped to its Node cause by `runWithPlatform`), and when both the * operation and the cleanup fail the cleanup error wins, as a throwing * `finally` did; * - cleanup runs on interruption as well, uninterruptibly. * - * Not `fs.makeTempDirectoryScoped`: in rc.112 its finalizer removes without + * Not a scope finalizer: those cannot fail typed, so an `EACCES` from the + * cleanup would surface as the `PlatformError` wrapper after `orDie`. + */ +export const ensuringRemoved = ( + path: string, + use: Effect.Effect, +): Effect.Effect => + Effect.uninterruptibleMask((restore) => Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const exit = yield* Effect.exit(restore(use)); + yield* fs.remove(path, { force: true, recursive: true }); + return yield* exit; + })); + +/** + * `const dir = await mkdtemp(...); try { return await use(dir) } finally + * { await rm(dir, { recursive: true, force: true }) }` — the + * `ensuringRemoved` bracket with the directory created inside the mask, so + * an interrupt cannot land between its creation and its cleanup. The + * operation is restored to the caller's interruptibility before it enters + * the bracket (the bracket's own mask is a no-op inside this one). Not + * `fs.makeTempDirectoryScoped`: in rc.112 its finalizer removes without * `force` and `orDie`s, so a missing directory would reject an already - * successful call, and an `EACCES` would surface as the `PlatformError` - * wrapper (scope finalizers cannot fail typed). + * successful call and a real cleanup error would lose its Node cause. */ export const withTempDirectory = ( options: { readonly directory?: string; readonly prefix?: string } | undefined, @@ -85,9 +104,7 @@ export const withTempDirectory = ( Effect.uninterruptibleMask((restore) => Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; const directory = yield* fs.makeTempDirectory(options); - const exit = yield* Effect.exit(restore(use(directory))); - yield* fs.remove(directory, { force: true, recursive: true }); - return yield* exit; + return yield* ensuringRemoved(directory, restore(use(directory))); })); /** diff --git a/packages/agent-bundle/src/routes/typegen.ts b/packages/agent-bundle/src/routes/typegen.ts index 0a1a97cc7..9fa0f001d 100644 --- a/packages/agent-bundle/src/routes/typegen.ts +++ b/packages/agent-bundle/src/routes/typegen.ts @@ -1,7 +1,10 @@ import { randomUUID } from 'node:crypto'; -import { mkdir, rename, rm, writeFile } from 'node:fs/promises'; import { dirname, extname, join, relative } from 'node:path'; +import { Effect, FileSystem } from 'effect'; +import type { PlatformError } from 'effect/PlatformError'; + +import { ensuringRemoved, runWithPlatform } from '../effect/platform.ts'; import { providerKeyFromName } from './providers.ts'; import type { CompiledAgentRoute, CompiledProvider, CompiledRouteGraph } from './types.ts'; @@ -159,20 +162,29 @@ export const generateRouteTypes = (graph: CompiledRouteGraph): string => { /** * Publishes typegen with a same-directory rename so dev readers observe the * previous complete file or the next complete file, never a partial write. + * Runs on Effect `FileSystem` (the path arithmetic stays `node:path`: it is + * string work shared with the pure generator above); a filesystem failure + * still rejects with the same Node `ErrnoException`. */ -export const writeRouteTypes = async (root: string, graph: CompiledRouteGraph): Promise => { +export const writeRouteTypesProgram = ( + root: string, + graph: CompiledRouteGraph, +): Effect.Effect => Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; const output = join(root, routeTypesRelativePath); + const published = relative(root, output).replaceAll('\\', '/'); if (executableRoutes(graph).length === 0 && graph.providers.length === 0) { - await rm(output, { force: true }); - return relative(root, output).replaceAll('\\', '/'); + yield* fs.remove(output, { force: true }); + return published; } - await mkdir(dirname(output), { recursive: true }); + yield* fs.makeDirectory(dirname(output), { recursive: true }); const temporary = `${output}.${String(process.pid)}.${randomUUID()}.tmp`; - try { - await writeFile(temporary, generateRouteTypes(graph), 'utf8'); - await rename(temporary, output); - } finally { - await rm(temporary, { force: true }); - } - return relative(root, output).replaceAll('\\', '/'); -}; + yield* ensuringRemoved(temporary, Effect.gen(function* () { + yield* fs.writeFileString(temporary, generateRouteTypes(graph)); + yield* fs.rename(temporary, output); + })); + return published; +}); + +export const writeRouteTypes = (root: string, graph: CompiledRouteGraph): Promise => + runWithPlatform(writeRouteTypesProgram(root, graph)); diff --git a/packages/agent-bundle/tests/effect-platform.test.ts b/packages/agent-bundle/tests/effect-platform.test.ts index 9d4c8e30e..9c6e5c73c 100644 --- a/packages/agent-bundle/tests/effect-platform.test.ts +++ b/packages/agent-bundle/tests/effect-platform.test.ts @@ -2,7 +2,7 @@ import { access, mkdtemp, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { Cause, Effect, Exit, FileSystem, Path, PlatformError } from 'effect'; +import { Cause, Deferred, Effect, Exit, Fiber, FileSystem, Option, Path, PlatformError } from 'effect'; import { describe, expect, it } from '@rstest/core'; import { DiagnosticError } from '../src/core/diagnostics.ts'; @@ -134,6 +134,33 @@ describe('effect platform layer (agent-bundle)', () => { } }); + it('lets an external interrupt reach the operation, then removes the temp directory', async () => { + // `withTempDirectory` delegates to `ensuringRemoved` from inside its own + // mask; the operation must still run at the caller's interruptibility, + // or a fiber parked in it could never be interrupted. + const parent = await mkdtemp(join(tmpdir(), 'agent-bundle-platform-')); + try { + const outcome = await runWithPlatform(Effect.gen(function* () { + const created = yield* Deferred.make(); + const fiber = yield* Effect.forkChild(withTempDirectory( + { directory: parent, prefix: '.staging-' }, + (directory) => Deferred.succeed(created, directory).pipe(Effect.andThen(Effect.never)), + )); + const directory = yield* Deferred.await(created); + const exit = yield* Fiber.interrupt(fiber).pipe( + Effect.andThen(Fiber.await(fiber)), + Effect.timeoutOption('2 seconds'), + ); + return { directory, exit }; + })); + expect(Option.isSome(outcome.exit)).toBe(true); + expect(Option.isSome(outcome.exit) && Exit.isFailure(outcome.exit.value) && Cause.hasInterrupts(outcome.exit.value.cause)).toBe(true); + await expect(access(outcome.directory)).rejects.toMatchObject({ code: 'ENOENT' }); + } finally { + await rm(parent, { force: true, recursive: true }); + } + }); + it('throws the Node error when the temp directory cannot be created', async () => { const missingParent = join(tmpdir(), 'agent-bundle-platform-missing', String(process.pid)); await expect(runWithPlatform(withTempDirectory( diff --git a/packages/agent-bundle/tests/route-typegen-write.test.ts b/packages/agent-bundle/tests/route-typegen-write.test.ts new file mode 100644 index 000000000..132e54113 --- /dev/null +++ b/packages/agent-bundle/tests/route-typegen-write.test.ts @@ -0,0 +1,131 @@ +import { access, mkdtemp, readdir, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { Effect, FileSystem, PlatformError } from 'effect'; +import { afterEach, describe, expect, it } from '@rstest/core'; + +import { runWithPlatform } from '../src/effect/platform.ts'; +import { emptyCompiledRouteGraph } from '../src/routes/graph.ts'; +import { generateRouteTypes, routeTypesRelativePath, writeRouteTypes, writeRouteTypesProgram } from '../src/routes/typegen.ts'; +import type { CompiledRouteGraph } from '../src/routes/types.ts'; + +/** + * `writeRouteTypes` publishes `.agent-bundle/routes.d.ts` with a + * same-directory rename and never leaves its temporary file behind. The + * real-filesystem cases pin the published bytes and the cleanup; the + * `FileSystem.layerNoop` cases pin the call protocol around a failing + * rename, where the temporary must still be removed and the caller must + * still see the Node error. + */ +const roots: string[] = []; +afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => rm(root, { force: true, recursive: true }))); +}); + +const scratchRoot = async (): Promise => { + const root = await mkdtemp(join(tmpdir(), 'agent-bundle-typegen-')); + roots.push(root); + return root; +}; + +const oneRouteGraph = (root: string): CompiledRouteGraph => ({ + ...emptyCompiledRouteGraph, + servers: [{ + id: 'mcp:curator', + mode: 'generated', + name: 'curator', + routes: [{ + config: {}, + id: 'tool:curator/inspect', + kind: 'tool', + provenance: { kind: 'conventional', relativePath: 'src/mcp/curator/tools/inspect.tsx' }, + serverId: 'mcp:curator', + source: join(root, 'src/mcp/curator/tools/inspect.tsx'), + }], + }], +}); + +describe('writeRouteTypes', () => { + it('publishes the generated declarations and leaves no temporary file', async () => { + const root = await scratchRoot(); + const graph = oneRouteGraph(root); + await expect(writeRouteTypes(root, graph)).resolves.toBe(routeTypesRelativePath); + expect(await readFile(join(root, routeTypesRelativePath), 'utf8')).toBe(generateRouteTypes(graph)); + expect(await readdir(join(root, '.agent-bundle'))).toEqual(['routes.d.ts']); + }); + + it('removes a stale declaration file when the graph has nothing to type', async () => { + const root = await scratchRoot(); + await writeRouteTypes(root, oneRouteGraph(root)); + await expect(writeRouteTypes(root, emptyCompiledRouteGraph)).resolves.toBe(routeTypesRelativePath); + await expect(access(join(root, routeTypesRelativePath))).rejects.toMatchObject({ code: 'ENOENT' }); + // And again when there is nothing to remove. + await expect(writeRouteTypes(root, emptyCompiledRouteGraph)).resolves.toBe(routeTypesRelativePath); + }); + + it('rejects with the Node error when the declarations directory cannot be created', async () => { + const root = await scratchRoot(); + await writeFile(join(root, '.agent-bundle'), 'not a directory'); + await expect(writeRouteTypes(root, oneRouteGraph(root))).rejects.toMatchObject({ + code: expect.stringMatching(/^(EEXIST|ENOTDIR)$/u), + syscall: 'mkdir', + }); + }); + + describe('over FileSystem.layerNoop', () => { + const exdev: NodeJS.ErrnoException = new Error('EXDEV: cross-device link not permitted, rename'); + exdev.code = 'EXDEV'; + + const recordingFileSystem = () => { + const calls: string[] = []; + const written = new Map(); + const layer = FileSystem.layerNoop({ + makeDirectory: (path) => Effect.sync(() => { calls.push(`makeDirectory ${path}`); }), + remove: (path, options) => Effect.sync(() => { + calls.push(`remove ${path} force=${String(options?.force ?? false)} recursive=${String(options?.recursive ?? false)}`); + }), + rename: (from, to) => Effect.suspend(() => { + calls.push(`rename ${from} -> ${to}`); + return Effect.fail(PlatformError.systemError({ + _tag: 'Unknown', + cause: exdev, + method: 'rename', + module: 'FileSystem', + pathOrDescriptor: from, + })); + }), + writeFileString: (path, data) => Effect.sync(() => { + calls.push(`writeFile ${path}`); + written.set(path, data); + }), + }); + return { calls, layer, written }; + }; + + it('removes the temporary file and rethrows the Node error when the rename fails', async () => { + const root = '/virtual/project'; + const graph = oneRouteGraph(root); + const { calls, layer, written } = recordingFileSystem(); + await expect(runWithPlatform(writeRouteTypesProgram(root, graph).pipe(Effect.provide(layer)))).rejects.toBe(exdev); + + const output = join(root, routeTypesRelativePath); + expect(calls[0]).toBe(`makeDirectory ${join(root, '.agent-bundle')}`); + const temporary = calls[1]?.replace(/^writeFile /u, ''); + expect(temporary).toMatch(new RegExp(`^${output.replaceAll('.', '\\.')}\\.${String(process.pid)}\\.[0-9a-f-]{36}\\.tmp$`, 'u')); + expect(calls.slice(2)).toEqual([ + `rename ${temporary} -> ${output}`, + `remove ${temporary} force=true recursive=true`, + ]); + expect(written.get(temporary!)).toBe(generateRouteTypes(graph)); + }); + + it('removes the declaration file, and nothing else, for an empty graph', async () => { + const root = '/virtual/project'; + const { calls, layer } = recordingFileSystem(); + await expect(runWithPlatform(writeRouteTypesProgram(root, emptyCompiledRouteGraph).pipe(Effect.provide(layer)))) + .resolves.toBe(routeTypesRelativePath); + expect(calls).toEqual([`remove ${join(root, routeTypesRelativePath)} force=true recursive=false`]); + }); + }); +});