effects/node: add memoryRun, dropping the ToAsyncOperationMap casts - #1713

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e93e265380

The diagnosis is right and the code is clean. ToAsyncOperationMap<O> is keyed on the indexed access O[0] — neither keyof T nor a naked type parameter — so it is non-homomorphic and not reversible, and deleting the casts at the merge-base does name (...payload: never) => Promise<never> exactly as the body says. memoryOperationMap() really does already return ToAsyncOperationMap<MemOp>: the value was fine, the inference wasn't, and the old todo's proposal genuinely could not be carried out as written. No cast moved — neither changed file has an inline @type cast, as, @ts-ignore or any, and both annotations are load-bearing.

The problem is the evidence, not the change.

"Under the cast it compiled" is false. memoryOperationMap already carries /** @type {(uuid?: Uuid) => MemoryOperationMap} */, so its object literal was contextually typed at the merge-base too. The cast you removed wrapped a call result, not the literal, so it never disabled per-handler checking. Five drifts, one at a time, base vs head — wrong return on memWrite, wrong payload on memRead, missing handler, extra handler, wrong arity — all five behave identically at both commits. Your quoted TS2322 does reproduce, but it is line 4 of a cascade rooted at module.mjs(33,14), the factory annotation, not at the asyncRun call, and it appears verbatim at the merge-base. The extra-key drift is silent at both.

There is a real delta, and it's worth stating in its own terms: declare MemoryOperationMap as ToAsyncOperationMap<MemCreate | MemRead> and drop the memWrite handler, and head gives TS2741: Property 'memWrite' is missing in type 'MemoryOperationMap' but required in type 'ToAsyncOperationMap<MemOp>' at the runner. Base is silent there — it only catches it incidentally over in node/module.mjs(359,32), because runNodeEffect spreads the map. So what you gained is agreement between the declared map type and MemOp, checked at the runner. Narrower than "the check is live again", but genuine.

The same overstatement is now in fjs/AGENTS.md, and that's the part I'd most want fixed, since every future agent reads it. The constraint statement and the annotate-the-result cure are both correct and correctly scoped. The closing generalisation isn't: "Both Node runners are written that way … leaving the map checked" — only runNodeEffect's annotation checks a map, because it passes a fresh object literal. memoryRun passes an already-annotated call result, so its annotation isn't what keeps the map checked. Same conflation in memory/module.mjs's new doc comment.

One factual error in the new todo. types.ts:235 has CreateServer = ['createServer', (listener: RequestListener<Operation>) => OpResult<Server>] — not generic. The <O extends Operation> signature the todo attributes to it exists only on the effect constructor at module.f.mjs:346, which is itself a cast. So the Erl<NodeOp> cast isn't Pr erasing a type parameter; Operation is written into the operation declaration. The sandbox case is genuine erasure, as are MemCreate/MemRead/MemWrite — two of three examples are right, but this one changes what the fix would be.

Both kept casts are correctly kept, verified: annotate-the-result cures neither, since both handlers already sit in result-annotated maps, and the todo's create claim reproduces exactly.

One surviving mutant, cheap to close. Making all runners share a single module-level store passes the whole suite — 3378/0. Nothing pins "a new runner starts empty", nor run's "a store per call, so nothing written by one run is visible to the next", both of which this PR's own doc comments promise. Rebuilding the store per call is caught (reusedRunnerPersists). A two-runner isolation proof closes it. Note assert sites are 5 → 5 and the total is unchanged at 3378, so reusedRunnerPersists is the renamed reusedOperationMapPersists, not new coverage.

Gates: npm test 3378/0 at head — and identical at the merge-base, so that number isn't evidence of anything added. tsc --noEmit exit 0 both. The declaration-emit diff claim is accurate: only MemoryRun and memoryRun appear, run's type unchanged after alias expansion. changelog/unreleased/1713.md conforms and no **BREAKING CHANGES:** is needed — the additions are additive and run is unchanged.

Body nit: the Changelog: section itself is correct and byte-identical to the file, but there are two trailer blocks — a 🤖 Generated with… block plus session URL, then a --- rule, then a second _Generated by…_ line pointing at the same session.

Three of the four are properly fixed.

The restated body claim is accurate — I re-ran all five drifts at the merge-base and at head and they're byte-identical, all rooted at the factory annotation, and the TS2741 probe fires at the memory runner on head while the base only catches it incidentally over in node/module.mjs(359,32). The AGENTS.md correction is right and doesn't over-correct; I checked the substantive claim rather than the wording by narrowing a handler inside runNodeEffect's literal — as shipped it's caught, and moving the annotation onto the literal absorbs it, so "the annotation is the only thing checking any handler there" holds. Trailers fixed, Changelog: still byte-identical.

The two-runner mutant is dead: a module-level shared store now fails runnersDoNotShareStore, and reusedRunnerPersists still catches the rebuild-per-call case rather than being displaced. Both new assertions and the fixed uuid are load-bearing.

Two residuals, both small:

run's promise is still unpinned. memory/module.mjs's run documents "a store per call, so nothing written by one run is visible to the next", but const sharedRun = memoryRun(); run = effect => sharedRun(effect) passes the whole suite — 3379/0. Only the memoryRun two-runner case is pinned, so "Store isolation is now pinned" is still a shade broader than what's proven. This passes at head (3380/0) and fails under that mutant:

runIsPerCall: async () => {
    const key = unwrap(await run(unwrapStep(create(1), errorSummary)))
    const result = await run(read(key)).then(() => undefined, error => error)
    assert(result instanceof Error, result)
},

One factual error survives in the todo, and it's the one that matters to its own argument. generic-operation-payload-erasure.md:9-10 gives Sandbox as <T>(f: () => SandboxResult<T>) => …. types.ts:325 is readonly['sandbox', <T>(f: () => T) => OpResult<SandboxResult<T>>] — SandboxResult is on the other side. Under the quoted signature f() at T = unknown would already be SandboxResult<unknown> and the cast the file exists to explain wouldn't be needed, which contradicts its own causal claim five lines down. Dropping the cast confirms the real shape: virtual/module.f.mjs(627,5): TS2322 … 'Ok<unknown>' is not assignable to 'OpResult<SandboxResult<unknown>>'. Pre-existing, but this delta reflowed that line in a commit titled "correct the review claims".

The CreateServer correction itself is right and every cited line lands at head. Nit: the body's table still says memory/module.mjs(73,58); it's (83,58) now — stale by exactly the ten lines this delta added to the doc comment.

Gates: npm test 3379/0 at head (was 3378, +1 = runnersDoNotShareStore), tsc --noEmit exit 0, purge matched nothing, no casts introduced.

Approving. Both residuals are closed.

run's per-call store is pinned, and the proof is stronger than what I suggested — it asserts the exact memory key not found: <key> message for the key the first run minted, so it fails for the right reason rather than on any incidental throw. I confirmed that by flipping the expected message and reading the observed value: a real randomUUID key from call 1, genuinely absent from call 2's store.

The shared-runner mutant now dies naming runIsPerCall, with every other row still green — which matches the proof's own comment. Neither neighbour was displaced: hoisting store to module level fails both runnersDoNotShareStore and runIsPerCall, and rebuilding the map per effect fails reusedRunnerPersists alone. Worth noting the naive rebuild-per-call spelling doesn't even compile — TS2345 … 'MemoryOperationMap' is not assignable to 'ToAsyncOperationMap<Operation>' — which is the same inference machinery this PR is about.

The Sandbox quote now matches types.ts:325 character for character, and the prose around it was reworked rather than patched: the new paragraph states the asymmetry explicitly and its counterfactual is exactly what dropping the cast produces — the gap is unknown vs SandboxResult<unknown> on the output side. The other examples survived the edit; every cited line lands and every link resolves. Body line fixed to (83,58), verified by running its own mutation.

Gates: npm test 3380/0 (+1 = runIsPerCall), tsc --noEmit exit 0, purge matched nothing, additive only — all four prior rows present and unrenamed, the -1 is just the import line, no casts introduced. No changelog item needed for a proof plus a todo doc.