Skip to content

Commit ea01590

Browse files
fix(effect): withTempDirectory reproduces the try/finally rm contract exactly
A scope finalizer cannot fail typed, so a cleanup error (EACCES, EBUSY) surfaced as the PlatformError wrapper after orDie, and when the operation had failed too Cause.squash preferred the operation's failure where the former throwing finally reported the cleanup error. withTempDirectory is a bracket: makeTempDirectory, Effect.exit(use), rm({ recursive, force }) on the typed error channel, then the operation's exit; uninterruptible around the cleanup. Tests cover both cleanup-failure orders over FileSystem.layerNoop and cleanup on interruption.
1 parent 42305e6 commit ea01590

5 files changed

Lines changed: 204 additions & 132 deletions

File tree

‎docs/effect-conventions.md‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -237,13 +237,16 @@ contract).
237237
modules. `fromFileUrl` fails with `BadArgument`; `Effect.orDie` it when the
238238
URL is built from `import.meta.url`.
239239
- Temporary directories whose lifetime ends with the enclosing operation:
240-
a scoped temp directory inside `Effect.scoped`, replacing `mkdtemp` +
241-
`try`/`finally` `rm`. In `agent-bundle` use `scopedTempDirectory` from
242-
`src/effect/platform.ts`, not `fs.makeTempDirectoryScoped` directly: the
243-
rc.112 finalizer removes without `force`, so an operation that deletes its
244-
own staging directory would die with ENOENT at scope close, where the old
245-
`rm(dir, { recursive: true, force: true })` succeeded. Tests may use
246-
`makeTempDirectoryScoped` (nothing removes the fixture underneath them).
240+
in `agent-bundle`, `withTempDirectory(options, use)` from
241+
`src/effect/platform.ts`, the bracket that reproduces `mkdtemp` +
242+
`try`/`finally` `rm(dir, { recursive: true, force: true })` exactly —
243+
`force`, cleanup failure as a typed `PlatformError` that wins over the
244+
operation's failure, cleanup on interruption. Not
245+
`fs.makeTempDirectoryScoped` in library code: the rc.112 finalizer removes
246+
without `force` and `orDie`s, so an operation that deleted its own staging
247+
directory would fail an already successful call, and a real cleanup error
248+
would surface as the `PlatformError` wrapper (scope finalizers cannot fail
249+
typed). Tests may use `makeTempDirectoryScoped` for fixtures.
247250
**Not** when ownership of the directory is
248251
transferred to a longer-lived object (the MCP session plugin-data dir in
249252
`dev/mcp-session/mcp-session-service.ts`): a scoped temp is removed when
@@ -307,16 +310,16 @@ provides `NodeServices.layer` once. Measured on rc.112 (bundled by Rslib,
307310
the bundle.
308311

309312
`packages/agent-bundle/src/effect/platform.ts` owns the framework's platform
310-
layer: `platformLayer` (= `NodeServices.layer`), `scopedTempDirectory`,
313+
layer: `platformLayer` (= `NodeServices.layer`), `withTempDirectory`,
311314
`unwrapPlatformError`, and `runWithPlatform`, which provides the layer and unwraps `PlatformError`
312315
before handing off to `boundary.ts`'s `runPromise`. It is the only module
313316
that imports `effect/PlatformError`: `boundary.ts` is bundled into every
314317
emitted hook wrapper, and the error class would drag `Data.TaggedError` into
315318
each one (measured: +12 kB per hook). Phase-1 callers are the throwaway
316319
artifact in `api.ts` (`listMcp` / `invokeMcp` / `runMcp` / `listHooks` /
317320
`simulateHook` without `artifact`) and the Codex validator's
318-
schema-generation directory, both `scopedTempDirectory` (the `force: true`
319-
finalizer) inside `Effect.scoped`. Emitted artifacts, hook wrappers, and compiler hot paths
321+
schema-generation directory, both through `withTempDirectory`. Emitted
322+
artifacts, hook wrappers, and compiler hot paths
320323
never import this module; the dev server picks it up in phase 2 through
321324
`makeScopedEffectRuntime(platformLayer)`.
322325

‎packages/agent-bundle/src/api.ts‎

Lines changed: 19 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ import {
166166
// Imported after the service modules on purpose: the position of
167167
// `effect/lift.ts` in the module graph fixes its position in the emitted
168168
// hook bundles, and this order keeps those bundles byte-identical.
169-
import { runWithPlatform, scopedTempDirectory } from './effect/platform.ts';
169+
import { runWithPlatform, withTempDirectory } from './effect/platform.ts';
170170
import { liftPromise } from './effect/lift.ts';
171171

172172
export {
@@ -575,25 +575,24 @@ const temporaryArtifact = async <Result>(
575575
): Promise<Result> => {
576576
if (options.artifact !== undefined) return operation(resolve(options.artifact));
577577

578-
// The staging directory lives exactly as long as the scope: created next
579-
// to the project (same filesystem as a real `artifact/`), removed when the
580-
// build or the operation settles, success or failure.
581-
return runWithPlatform(Effect.scoped(Effect.gen(function* () {
582-
const artifact = yield* scopedTempDirectory({
583-
directory: resolve(options.root),
584-
prefix: '.agent-bundle-artifact-',
585-
});
586-
yield* liftPromise(() => build({
587-
configPath: options.configPath,
588-
logger: options.logger,
589-
mode: options.mode,
590-
output: artifact,
591-
registry: options.registry,
592-
root: options.root,
593-
targets: options.targets,
594-
}));
595-
return yield* liftPromise(() => operation(artifact));
596-
})));
578+
// The staging directory lives exactly as long as the operation: created
579+
// next to the project (same filesystem as a real `artifact/`), removed
580+
// when the build or the operation settles, success, failure or interrupt.
581+
return runWithPlatform(withTempDirectory(
582+
{ directory: resolve(options.root), prefix: '.agent-bundle-artifact-' },
583+
(artifact) => Effect.gen(function* () {
584+
yield* liftPromise(() => build({
585+
configPath: options.configPath,
586+
logger: options.logger,
587+
mode: options.mode,
588+
output: artifact,
589+
registry: options.registry,
590+
root: options.root,
591+
targets: options.targets,
592+
}));
593+
return yield* liftPromise(() => operation(artifact));
594+
}),
595+
));
597596
};
598597

599598
type HostValidatedTarget = 'claude' | 'codex' | 'cursor' | 'plugin' | 'portable';

‎packages/agent-bundle/src/effect/platform.ts‎

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import * as NodeServices from '@effect/platform-node/NodeServices';
2-
import { Effect, FileSystem, type Layer, type Scope } from 'effect';
2+
import { Effect, FileSystem, type Layer } from 'effect';
33
import { PlatformError } from 'effect/PlatformError';
44

55
import { runPromise, type RunPromiseOptions } from './boundary.ts';
@@ -37,25 +37,35 @@ export const unwrapPlatformError = <E>(error: E): Exclude<E, PlatformError> | Er
3737
: (error as Exclude<E, PlatformError>);
3838

3939
/**
40-
* A temporary directory that lives exactly as long as the enclosing scope,
41-
* with the `rm(dir, { recursive: true, force: true })` finalizer the
42-
* `try`/`finally` sites had before they moved onto Effect. rc.112's own
43-
* `makeTempDirectoryScoped` finalizes without `force`, so an operation that
44-
* removes (or renames away) its own staging directory would die with ENOENT
45-
* at scope close and reject a call that had already succeeded. A finalizer
46-
* failure other than "already gone" is still a defect, as the `finally`
47-
* throw was.
40+
* `const dir = await mkdtemp(...); try { return await use(dir) } finally
41+
* { await rm(dir, { recursive: true, force: true }) }` as an Effect, with
42+
* the same contract the two `try`/`finally` sites had before they moved
43+
* onto Effect:
44+
*
45+
* - `force: true` — an operation that removed (or renamed away) its own
46+
* staging directory does not fail the call;
47+
* - the cleanup failure is a typed `PlatformError` on the error channel
48+
* (unwrapped to its Node cause by `runWithPlatform`), and when both the
49+
* operation and the cleanup fail the cleanup error wins, as a throwing
50+
* `finally` did;
51+
* - cleanup runs on interruption as well, uninterruptibly.
52+
*
53+
* Not `fs.makeTempDirectoryScoped`: in rc.112 its finalizer removes without
54+
* `force` and `orDie`s, so a missing directory would reject an already
55+
* successful call, and an `EACCES` would surface as the `PlatformError`
56+
* wrapper (scope finalizers cannot fail typed).
4857
*/
49-
export const scopedTempDirectory = (
50-
options?: { readonly directory?: string; readonly prefix?: string },
51-
): Effect.Effect<string, PlatformError, FileSystem.FileSystem | Scope.Scope> =>
52-
Effect.gen(function* () {
58+
export const withTempDirectory = <A, E, R>(
59+
options: { readonly directory?: string; readonly prefix?: string } | undefined,
60+
use: (directory: string) => Effect.Effect<A, E, R>,
61+
): Effect.Effect<A, E | PlatformError, R | FileSystem.FileSystem> =>
62+
Effect.uninterruptibleMask((restore) => Effect.gen(function* () {
5363
const fs = yield* FileSystem.FileSystem;
54-
return yield* Effect.acquireRelease(
55-
fs.makeTempDirectory(options),
56-
(directory) => Effect.orDie(fs.remove(directory, { force: true, recursive: true })),
57-
);
58-
});
64+
const directory = yield* fs.makeTempDirectory(options);
65+
const exit = yield* Effect.exit(restore(use(directory)));
66+
yield* fs.remove(directory, { force: true, recursive: true });
67+
return yield* exit;
68+
}));
5969

6070
/**
6171
* Run a platform-dependent Effect program at a Promise edge. Same failure

‎packages/agent-bundle/src/host-contracts/codex-plugin-validation.ts‎

Lines changed: 58 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import {
2626
// Imported last on purpose: see the matching note in `src/api.ts` — the
2727
// position of `effect/lift.ts` in the module graph keeps the emitted hook
2828
// bundles byte-identical.
29-
import { runWithPlatform, scopedTempDirectory } from '../effect/platform.ts';
29+
import { runWithPlatform, withTempDirectory } from '../effect/platform.ts';
3030
import { liftPromise } from '../effect/lift.ts';
3131

3232
const maximumOutputBytes = 1024 * 1024;
@@ -261,10 +261,10 @@ const schemaVerbUnavailable = (output: string): boolean =>
261261
/(?:unrecognized|unknown|invalid) (?:subcommand|command)|no such (?:subcommand|command)/iu.test(output);
262262

263263
/**
264-
* Runs the live schema generator into a scoped temp directory and compares
264+
* Runs the live schema generator into a temporary directory and compares
265265
* its output with the pinned revision. Every generator failure is reported
266266
* as a diagnostic (AB6033 / AB6031), never thrown; the directory is removed
267-
* when the scope closes, whichever way the program settles.
267+
* whichever way the program settles.
268268
*/
269269
const schemaGenerationDiagnostics = (
270270
options: Readonly<{
@@ -275,67 +275,69 @@ const schemaGenerationDiagnostics = (
275275
readonly target: string;
276276
readonly version: string | undefined;
277277
}>,
278-
): Effect.Effect<readonly Diagnostic[], PlatformError, FileSystem.FileSystem> => Effect.scoped(Effect.gen(function* () {
279-
const outputDirectory = yield* scopedTempDirectory({ prefix: 'agent-bundle-codex-schema-' });
280-
const started = yield* liftPromise(() => options.run(Object.freeze({
281-
args: Object.freeze(['app-server', 'generate-json-schema', '--out', outputDirectory]),
282-
cwd: options.cwd,
283-
executable: options.executable,
284-
}))).pipe(Effect.option);
285-
if (Option.isNone(started)) {
286-
return freezeDiagnostics([diagnostic(
287-
'AB6033',
288-
'Codex CLI schema generation could not be started.',
289-
'error',
290-
options.target,
291-
'Verify the Codex CLI starts and supports app-server schema generation, then rerun artifact validation.',
292-
)]);
293-
}
294-
const result: CodexPluginCommandResult = started.value;
278+
): Effect.Effect<readonly Diagnostic[], PlatformError, FileSystem.FileSystem> => withTempDirectory(
279+
{ prefix: 'agent-bundle-codex-schema-' },
280+
(outputDirectory) => Effect.gen(function* () {
281+
const started = yield* liftPromise(() => options.run(Object.freeze({
282+
args: Object.freeze(['app-server', 'generate-json-schema', '--out', outputDirectory]),
283+
cwd: options.cwd,
284+
executable: options.executable,
285+
}))).pipe(Effect.option);
286+
if (Option.isNone(started)) {
287+
return freezeDiagnostics([diagnostic(
288+
'AB6033',
289+
'Codex CLI schema generation could not be started.',
290+
'error',
291+
options.target,
292+
'Verify the Codex CLI starts and supports app-server schema generation, then rerun artifact validation.',
293+
)]);
294+
}
295+
const result: CodexPluginCommandResult = started.value;
295296

296-
if (result.termination !== undefined) {
297-
return freezeDiagnostics([diagnostic(
298-
'AB6033',
299-
commandFailureMessage('schema generation', result),
300-
'error',
301-
options.target,
302-
'Rerun Codex schema generation within the configured time and output bounds.',
303-
)]);
304-
}
305-
if (result.exitCode !== 0) {
306-
const output = `${result.stdout}\n${result.stderr}`;
307-
if (schemaVerbUnavailable(output)) {
297+
if (result.termination !== undefined) {
308298
return freezeDiagnostics([diagnostic(
309-
'AB6031',
310-
`The Codex ${options.version ?? 'unknown'} app-server generate-json-schema verb is unavailable; ` +
311-
`live schema drift could not be checked against pinned Codex ${pinnedRevision}.`,
312-
'info',
299+
'AB6033',
300+
commandFailureMessage('schema generation', result),
301+
'error',
313302
options.target,
314-
'Use a Codex release that publishes app-server schema generation, or retain validation against the vendored pinned schemas.',
303+
'Rerun Codex schema generation within the configured time and output bounds.',
315304
)]);
316305
}
317-
return freezeDiagnostics([diagnostic(
306+
if (result.exitCode !== 0) {
307+
const output = `${result.stdout}\n${result.stderr}`;
308+
if (schemaVerbUnavailable(output)) {
309+
return freezeDiagnostics([diagnostic(
310+
'AB6031',
311+
`The Codex ${options.version ?? 'unknown'} app-server generate-json-schema verb is unavailable; ` +
312+
`live schema drift could not be checked against pinned Codex ${pinnedRevision}.`,
313+
'info',
314+
options.target,
315+
'Use a Codex release that publishes app-server schema generation, or retain validation against the vendored pinned schemas.',
316+
)]);
317+
}
318+
return freezeDiagnostics([diagnostic(
319+
'AB6033',
320+
commandFailureMessage('schema generation', result),
321+
'error',
322+
options.target,
323+
'Run `codex app-server generate-json-schema --out <dir>` successfully, then rerun artifact validation.',
324+
)]);
325+
}
326+
327+
return yield* liftPromise(() => compareGeneratedSchemas(
328+
outputDirectory,
329+
options.version,
330+
options.strict,
331+
options.target,
332+
)).pipe(Effect.catch(() => Effect.succeed(freezeDiagnostics([diagnostic(
318333
'AB6033',
319-
commandFailureMessage('schema generation', result),
334+
'Codex CLI generated schema output could not be inspected.',
320335
'error',
321336
options.target,
322-
'Run `codex app-server generate-json-schema --out <dir>` successfully, then rerun artifact validation.',
323-
)]);
324-
}
325-
326-
return yield* liftPromise(() => compareGeneratedSchemas(
327-
outputDirectory,
328-
options.version,
329-
options.strict,
330-
options.target,
331-
)).pipe(Effect.catch(() => Effect.succeed(freezeDiagnostics([diagnostic(
332-
'AB6033',
333-
'Codex CLI generated schema output could not be inspected.',
334-
'error',
335-
options.target,
336-
'Ensure the generated schema directory is readable, then rerun artifact validation.',
337-
)]))));
338-
}));
337+
'Ensure the generated schema directory is readable, then rerun artifact validation.',
338+
)]))));
339+
}),
340+
);
339341

340342
export const validateCodexPlugin = async (
341343
options: ValidateCodexPluginOptions,

0 commit comments

Comments
 (0)