fix(test): adopt effect-rstest PR #4 canary and drop the pnpm patch - #871
Conversation
npm effect-rstest@0.1.4 predates upstream PR #4 and lacks its fixes, which is why main still carried patches/effect-rstest@0.1.4.patch. Pin the immutable merged-commit canary f29b3f49 that contains Effect equality, scoped test lifetimes and timed-out settlement upstream, remove the patch and its patchedDependencies entry, and regenerate the lockfile. Closes #507 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Review changes with Changed Files
|
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. |
|
Warning Review limit reachedNext included review available in 55 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (14)
🧰 Additional context used📓 Path-based instructions (1)Before changing files under `app/`, read [the application coding guide](./README.md).📄 CodeRabbit inference engine (app/AGENTS.md) Files:
🔇 Additional comments (11)
WalkthroughThe change updates all Changeseffect-rstest adoption
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy the main coding requirements in issue ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/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: 45a0c95e0a
ℹ️ 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".
| }, | ||
| "devDependencies": { | ||
| "effect-rstest": "0.1.4", | ||
| "effect-rstest": "https://pkg.pr.new/ScriptedAlchemy/effect-rstest@f29b3f495d45a11037334620bf5f693414fb5578", |
There was a problem hiding this comment.
When maintainers consult app/tools/oxlint/effect-native/README.md:84, it still states that the current canary uses a temporary pnpm patch and that issue #507 tracks deleting it. This commit pins the unpatched canary and closes that issue, so the authoritative guidance now contradicts the dependency configuration and can prompt someone to restore an obsolete patch; update that paragraph as part of this migration.
AGENTS.md reference: app/AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3160860: the paragraph now describes the unpatched canary pin, points at the installed-package guard test, and warns that npm effect-rstest releases are a separate publishing line without these fixes.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
Closes TechsioCZ/ontos#507 — Remove temporary effect-rstest patch after upstream fixes merge.
Why the patch was still needed on main
effect-rstest@0.1.4(pinned since 3b9f684) is published fromgithub.com/Nsttt/effect-rstest, not ScriptedAlchemy, on 2026-09-08 13:34Z — before ScriptedAlchemy/effect-rstest PR #4 merged (20:29Z,f29b3f49). Itsdiststill hasisEqual(a) || isEqual(b), noonTestFinishedsettlement and no setup-fiber interruption, so main carried a 68-line re-application of the fixes aspatches/effect-rstest@0.1.4.patch.https://pkg.pr.new/ScriptedAlchemy/effect-rstest@f29b3f495d45a11037334620bf5f693414fb5578(verified: all three fix markers present,it.propschema support included, exports././utilsonly — the repo imports only'effect-rstest'; peers@rstest/core ^0.11.11,effect ^4.0.0-rc.108).Change
f29b3f49canary in all 11 consuming manifests.effect-rstest@0.1.4frompatchedDependenciesand deletepatches/effect-rstest@0.1.4.patch.pnpm-lock.yaml;pnpm install --frozen-lockfilesucceeds without the patch. No@app/effect-rstestalias or vendored copy is restored.Verification (local, against the installed package)
scripts/tests/effect-rstest-package.test.mts(equality tester, tuple/record schema props, discarded success values, timed-out finalizer settlement): 7/7 passpnpm test:scripts335/335 ·pnpm test:unitall workspace projects pass ·pnpm test:generation143/143 ·pnpm test:deployment-impact43/43pnpm typecheck✅ ·pnpm lint && pnpm format:check✅ ·pnpm typecheck:lint-rules && pnpm test:lint-rules217/217 ✅pnpm test:integration, Postgres-backed) not run locally — no local database; relies on CI.🤖 Generated with Claude Code
Summary by CodeRabbit