effects: the operation vocabulary is not node's - #1753
Conversation
`OpResult`, `IoError`, `IoErrorInfo`, `IoChannel`, `IoResult` and the `ioError` / `toIoError` / `isNotFound` constructors were declared in `effects/node`, but nothing in them names a host: "the runner cannot dispatch this" and "the host tried and failed" are the two ways any operation goes wrong, on any host. They move to `effects/` beside `NotImplemented`, which `OpResult` is defined in terms of. The misfiling already had a victim: `effects/memory/types.ts` — which has no host at all — imported `OpResult` from `../node/types.ts`. It now takes it from the layer it belongs to. `effects/node` re-exports every moved name, so the several dozen modules that reach for them through it are unchanged, and an operation's declaration still reads as one vocabulary. Groundwork for step 4 of emergent_testing/todo/share-browser-console-runner.md: a second host's operations cannot be typed while the types they are written in live in the first host's module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016PyLwDNkPQM1uD1Tg5ApBg
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | d91518d | Commit Preview URL Branch Preview URL |
Aug 28 2026, 04:00 PM |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dffe551e3
ℹ️ 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".
…on-runners-7wvd1q
Review found `effects/todo/node-module-layering.md`, an open design that
had already decided two of these questions. It was right about one and
outdated about the other, so this reconciles both rather than leaving
contradictory guidance for whoever implements it.
`isNotFound` moves back to `effects/node`, with its proofs. It reads
`ENOENT`, a POSIX filesystem code a host without a filesystem never
reports, so it is a node predicate — "none of it is node's" was
overreach for that one function. Being about a host failure does not
make a thing host-agnostic; being about no host in particular does.
The issue's other ruling — that `IoResult` must not move to the core —
is overturned in the file with the reason: it was reasoned without a
second host, and a browser interpreter cannot declare `fetch` or
`import` without that alias existing somewhere shared. Its stated test
("not an effect constructor or combinator") also did not describe the
file it was applied to, where `NotImplemented` already lives. The
one-site `fjs/media/type` cleanup it proposed survives and stays open.
Also records why a re-export here is not the "no shims behind" case
that issue rules out: node's own operations are declared in these
types, so it re-exports what it genuinely uses.
Changelog entry added — the core module gains public exports even
though no old import path changed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016PyLwDNkPQM1uD1Tg5ApBg
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e67bb52b73
ℹ️ 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".
The step-4 note was written before review established that isNotFound belongs in effects/node, and still listed it among the moved names under the claim that none of them names a host. It now records the opposite, and uses it as the test to apply to each operation the remaining move covers rather than moving the list wholesale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016PyLwDNkPQM1uD1Tg5ApBg
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d66ae39eb5
ℹ️ 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".
The cross-reference added in the last commit made node-module-layering.md read as the source of truth, but the two files disagree: it keeps Now, Fetch and Import in effects/node on a reader-benefit argument, while share-browser-console-runner's step 4 lists all three as moving. Whichever a later reader opened first would have looked authoritative. Neither was written knowing the fact that decides it — which operations the step-5 browser interpreter actually implements — so both now record the disagreement, name that as what settles it, and require step 5 to update both in one change. The expectation, not a ruling: now and import move (a browser proof run needs a clock and dynamic import), fetch stays (nothing in the shared runner performs one). all, await and sandbox were never in dispute; both files move them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016PyLwDNkPQM1uD1Tg5ApBg
…on-runners-7wvd1q # Conflicts: # fjs/effects/proof.f.mjs
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approved.
The move is genuinely non-breaking, and I checked that rather than reading it: deleting the type re-export line from node/types.ts fails npm test at the tsc stage with 33 TS2459s across real consumers (cas, media, mcp, web). The compatibility rests on an exercised mechanism, not an assertion. isNotFound correctly stayed behind in the node layer. Coverage moves 1:1 — the six toIoError.* leaves leave node/proof and the same six appear in effects/proof, confirmed running rather than merely present. Gates re-run after the merge that brought #1750 in: npm test 3522/3522, exit 0.
One line worth a follow-up, pre-existing rather than introduced here: fjs/effects/types.ts:154 still describes IoChannel as "the node standard of NotImplemented | IoError". In the file this PR promotes to the host-agnostic layer, under a title saying the vocabulary is not node's, that sentence now reads as the opposite of what the change means. The two doc corrections listed in the body do not cover it.
Minor: the changelog entry is 280 characters against the ~250 guideline.
Review found the leftover: `Effect`'s doc still called `IoChannel` "the node standard of NotImplemented | IoError" — in the file this change promotes to the host-agnostic layer, under a paragraph saying the vocabulary is not node's. The two corrections the PR body listed did not cover it. Also trims the changelog entry to the ~250-character guideline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016PyLwDNkPQM1uD1Tg5ApBg
|
Thank you — and the Both follow-ups are fixed in 61d8d47:
Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61d8d47425
ℹ️ 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".
io-effect-migration.md is done but deliberately kept, and two live documents cite it as the design record — so its statement that IoError sits "in fjs/effects/node/types.ts beside the operations it belongs to" is guidance, not just history, and this change made it false. It now records both: that the migration put them there, which was true while node's were the only operations there were, and what overturned it — effects/memory importing OpResult from the node module, and a second host unable to declare an operation without doing the same. Nothing it says about their shape or use changed, and isNotFound stayed behind. Swept the rest of the markdown for the same claim: the remaining mentions are past-tense history, statements about operations (still in node), or a released changelog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016PyLwDNkPQM1uD1Tg5ApBg
Groundwork for step 4 of
emergent_testing/todo/share-browser-console-runner.md, found by looking at what step 4 actually requires: before a host-independent operation can move out ofeffects/node, the types it is declared in need a home that is not node's either.What moved
OpResult,IoError,IoErrorInfo,IoChannel,IoResult, and theioError/toIoErrorconstructors, fromeffects/nodetoeffects/— besideNotImplemented, whichOpResultis defined in terms of.Nothing in any of them names a host. "The runner cannot dispatch this" and "the host tried and failed" are the two ways any operation goes wrong, on any host; normalizing a thrown value into serializable effect data is what any interpreter's
catchdoes.isNotFoundstayed ineffects/node, and the line between it and the two constructors is the useful part: it readsENOENT, a POSIX filesystem code that a host without a filesystem never reports. Being about a host failure does not make a thing host-agnostic — being about no host in particular does.It already had a victim
effects/memory/types.ts— typed key-value memory, no host at all — importedOpResultfrom../node/types.ts. It now takes it from the layer it belongs to. That is what makes this separation of concerns with a consumer today (DESIGN.md §4), rather than an extraction on the promise of one.The operations themselves (
all,await,sandbox, and the unsettledfetch/import/now) deliberately did not move: until step 5's browser interpreter is a second implementer, moving them makes nothing shorter or clearer, and §4 says to extract at the second real consumer. The todo now records that split and why.Reconciling the design records
Review found three documents that had already ruled on these questions, or that this change made stale. All three are settled in-place rather than left contradicting the code:
effects/todo/node-module-layering.mdonisNotFound— the issue's reasoning holds, and the change reverted to it.IoResult— it said the move must not happen, calling the core "the wrong destination". That was reasoned without a second host: a browser interpreter cannot declarefetchorimportwithout the alias existing somewhere shared, so "exactly a Node-layer contract" is what a second host falsifies. Its stated test — "not an effect constructor or combinator" — also does not describe the file it was applied to, whereNotImplementedalready lives and is neither. It never mentionedOpResultat all. The one-sitefjs/media/typecleanup it proposed survives untouched as an open task: a pure consumer should not name an IO alias whichever module it lives in.fjs/text/sgrshould stop namingeffects/nodeat all). Node's own operations are declared in these types, so re-exporting what it genuinely uses is not that.Now/Fetch/Importare unsettled, and both this issue and step 4 now say so instead of asserting opposite answers. Neither was written knowing what decides it — which operations the step-5 interpreter implements — so step 5 rules and updates both files in one change.effects/todo/io-effect-migration.mdisdonebut deliberately kept and cited by two live documents, and saidIoErrorlives ineffects/node/types.ts"beside the operations it belongs to". It now records both what the migration did (true while node's were the only operations there were) and what has since overturned it.Nothing else changes
effects/nodere-exports every moved name, so the several dozen modules that reach for them through it are untouched, and an operation's declaration still reads as one vocabulary next to its failure constructor. Three doc corrections came with the move:IoChannelsaid an effect "fails the way node IO fails" (now host IO);OperationcalledOpResult/IoResult"node conveniences" (now: they read that way because of where they were declared); andEffectcalledIoChannel"the node standard ofNotImplemented | IoError" (now the standard channel any host's IO answers in).toIoError's doc notes that a browserDOMExceptionhas no stringcodeand so normalizes through the message branch — correctly, since there is no OS code to report.The six proofs for
toIoErrormoved with it intoeffects/proof.f.mjs;isNotFound's three stayed with it ineffects/node/proof.f.mjs.Verification
With
origin/mainmerged in (which brought #1750):npx tscclean,fjs t3522/3522,npm run covexit 0 (100% line/branch/function thresholds). The commits after that merge are documentation and changelog only, re-checked withtsc.Changelog: the core module gains public exports even though no old import path changed, so this is an addition to record rather than a silent refactor.
Changelog:
effects:OpResult,IoChannel,IoError,IoErrorInfo,IoResultandthe
ioError/toIoErrorconstructors are now importable from the coremodule, not only through
effects/node, which re-exports them unchanged🤖 Generated with Claude Code
https://claude.ai/code/session_016PyLwDNkPQM1uD1Tg5ApBg