fix(cli): fail at start-up on a mismatched effect version instead of crashing inside alchemy - #202
Conversation
…d effect
TML-3158 reopened on 0.6.0: the exact pins are not sufficient in real
trees. A consumer app that also depends on a floating @effect/* range
(the operator hit @effect/platform-node-shared@^4.0.0-beta.93, which
npm floats to beta.102) drags a newer effect peer to the root of
node_modules, over our pins, with only an npm warning. alchemy then
imports the newer copy and crashes mid-deploy
(`TypeError: Schedule.both is not a function`).
No dependency declaration can prevent this. Empirically verified in
scratch trees on npm 11.6.2: an exact effect peerDependency on our
packages does NOT make npm fail or keep the pinned version at root —
transitive peer conflicts are downgraded to warnings and the newer
copy is hoisted anyway. So the enforcement point moves to the CLI:
- check-effect-resolution.ts resolves effect from alchemy's installed
position (walk up to node_modules/alchemy, realpath, createRequire)
and compares it to the version @prisma/composer's own installed
package.json pins (single source of truth — never hardcoded). On
mismatch it fails with a short error naming found vs required and
the consumer fix: `"overrides": { "effect": "<required>" }`. When
either side cannot be determined (no alchemy, no composer, odd
layout) the check skips instead of misfiring — later steps already
report those states with their own errors. Verified not to misfire
in this monorepo (hoisted pnpm layout resolves beta.93) or in
healthy npm trees.
- bin.ts runs the check BEFORE loading the rest of the CLI, for the
alchemy-driving commands only (deploy/destroy/dev): the command
modules transitively import alchemy's provider tree, which crashes
at import time in exactly the broken trees the check exists to
explain — so cli.ts is now imported dynamically after the check.
--help and log keep working in a broken tree.
Unit tests cover resolution, the healthy/unknown/mismatch rule, and
the operator's exact broken shape (root effect@beta.102, composer's
pin nested). Docs: the failure and its fix are documented in
docs/guides/deploying.md and skills/prisma-composer/SKILL.md.
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
Extends the standalone-install regression check with the tree that
broke 0.6.0 in the field: the scratch app also depends on
@effect/platform-node-shared@^4.0.0-beta.93, which npm floats to the
newest beta and hoists its newer effect peer over our exact pins —
install still exits 0 (verified; a peerDependency changes nothing).
Acceptance for that shape: npm refuses the install outright, OR the
install lands broken (alchemy resolves effect != the pin) and running
the built prisma-composer bin there must exit non-zero with the
start-up check's error ("alchemy resolves effect@..."), never the
Schedule TypeError. The two healthy shapes additionally assert the
inverse: the CLI check must NOT trip on a good tree.
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
…ing ones The argv sniff assumed only deploy/destroy/dev load alchemy. The adversarial npm shape disproves it: `prisma-composer --help` in a tree where alchemy resolves a mismatched effect dies with the raw `Schedule.both is not a function` from alchemy/lib/AWS/IAM/SAMLProvider.js, because the CLI module graph reaches the provider tree whatever the argv says. Run the check for every invocation, so no command can meet that TypeError. The npm shape now asserts it: `--help` in the broken tree must report the check, and the install-refused path is only accepted when npm actually failed on the dependency conflict, so a registry outage cannot turn the one end-to-end proof green. The healthy shapes assert the bin still reaches its usage output — proof it ran at all, which "the marker is absent" alone does not give. That assertion tests the banner rather than the exit code: a bare `--help` exits 1 on main too, since clipanion reports it as a missing command. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
|
Warning Review limit reached
Next review available in: 55 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThe CLI now checks the Effect version resolved from Alchemy against Composer’s required version before loading commands. Conflicts produce a 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/guides/deploying.md`:
- Around line 192-197: Add the `text` or `console` language tag to the fenced
diagnostic code block containing the dependency conflict output, preserving its
contents unchanged.
In
`@packages/0-framework/3-tooling/cli/src/__tests__/check-effect-resolution.test.ts`:
- Around line 158-160: Update both conflict-path assertions for
checkEffectResolution to verify that the thrown value is a CliError, not only
that its message matches the expected text. Preserve the existing message
patterns while adding the type assertion to both relevant expect blocks.
In `@scripts/check-npm-effect-resolution.mjs`:
- Around line 228-233: Update the adversarial resolution check around
resolvedVersion, the --help invocation, and the final success path so the script
fails when npm keeps pinnedEffect instead of producing the expected mismatch.
Require help.status to be non-zero regardless of CLI_CHECK_MARKER output, while
preserving marker validation for the failure output, and ensure the final
success message is reachable only after both assertions pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b07b8c26-2691-407f-8df4-b53acf33ed78
📒 Files selected for processing (6)
docs/guides/deploying.mdpackages/0-framework/3-tooling/cli/src/__tests__/check-effect-resolution.test.tspackages/0-framework/3-tooling/cli/src/bin.tspackages/0-framework/3-tooling/cli/src/check-effect-resolution.tsscripts/check-npm-effect-resolution.mjsskills/prisma-composer/SKILL.md
wmadden-electric
changed the title
…ror type Review follow-ups. The adversarial fixture named a floating range, so the day npm stopped producing a mismatch the shape would have passed while proving nothing. It now names the exact @effect/platform-node-shared release whose effect peer is newer than our pin, and treats "no mismatch" as a failure that says which constant to move — reachable only when the pin itself moves past that release, which is fixture maintenance rather than registry weather. `--help` in the broken tree must also exit non-zero, not merely print the error: a preflight that explains itself and exits 0 would let a script carry on. The unit tests assert CliError rather than message text alone, since bin.ts branches on that type to choose the exit status. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/guides/deploying.md (1)
205-210: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the Yarn and pnpm fix syntax for
effectversion conflicts.Lines 205-210 provide copyable npm syntax only, then mention
resolutionsandpnpm.overridesfor the other package managers without showing where those settings go. Add the Yarn and pnpmpackage.jsonentries and restate that dependencies must be reinstalled after adding the setting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/deploying.md` around lines 205 - 210, Update the dependency-override guidance in the deployment documentation to include copyable Yarn `resolutions` and pnpm `pnpm.overrides` package.json entries for resolving `effect` version conflicts, alongside the existing npm example. Explicitly remind readers to reinstall dependencies after adding any of these settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/guides/deploying.md`:
- Around line 205-210: Update the dependency-override guidance in the deployment
documentation to include copyable Yarn `resolutions` and pnpm `pnpm.overrides`
package.json entries for resolving `effect` version conflicts, alongside the
existing npm example. Explicitly remind readers to reinstall dependencies after
adding any of these settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c9c6abe7-b19c-4651-98b6-65a9e30c95be
📒 Files selected for processing (3)
docs/guides/deploying.mdpackages/0-framework/3-tooling/cli/src/__tests__/check-effect-resolution.test.tsscripts/check-npm-effect-resolution.mjs
Naming `resolutions` and `pnpm.overrides` without showing them left readers on those package managers to guess the nesting, which pnpm gets wrong in a way that fails silently. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
|
Addressed the outside-diff-range comment on |
2 participants
A user with a slightly unlucky dependency tree sees this on 0.6.0, having done nothing wrong:
They never asked for AWS IAM, and nothing in the message points at the real cause.
The decision
Check the installed dependency tree when the CLI starts, and refuse to run when it cannot work — so the same situation produces this instead:
Detecting the problem is as far as we can go — we cannot prevent it. That is the uncomfortable part, so it is worth explaining before the code.
What is actually going wrong
Composer deploys through alchemy, which is built on the
effectlibrary. alchemy 2.0.0-beta.59 works only witheffect@4.0.0-beta.93; newer betas removed functions it calls (Schedule.both,Schedule.either). Composer pins that exact version.A pin only constrains the edges we declare. In the reported tree the app itself depended on
@effect/platform-node-shared@^4.0.0-beta.93. npm floated that to a newer beta, which requires a newereffectas a peer — so npm installed one, and placed it at the root ofnode_modules, where alchemy finds it first. Composer's pinned copy sat nested and unused. #196 pinned every package in that family exactly, which does fix the trees our packages fully own; this is not one of them, which is why TML-3158 is reopened.We cannot close the hole from our side. alchemy declares
effect >=4.0.0-beta.84, so a newer beta genuinely satisfies alchemy's own requirement and npm is behaving correctly. Only the app's ownoverridescan force which copy alchemy resolves — which is exactly what the new error asks for.Changes
check-effect-resolution.ts(new): resolveseffectfrom alchemy's installed location the way Node will at runtime, and compares it against Composer's own pin, read from the installed manifest so the version is never written down twice. When alchemy, Composer oreffectcannot be resolved it skips rather than guesses, leaving unfamiliar layouts (pnpm-isolated, Yarn PnP, no alchemy installed) unaffected. Costs 1.7 ms cold, 0.2 ms warm, and touches no network.bin.ts: runs the check before the CLI's command graph loads, behind a dynamic import. The ordering is forced — in a broken tree the import itself is what throws, so a check that runs any later never gets the chance.scripts/check-npm-effect-resolution.mjs: a third install shape reproducing the reported tree, asserting the built CLI reports our error rather than theTypeError. "npm refused the install" counts as a pass only when npm actually failed on the dependency conflict — otherwise a registry outage would quietly turn the one end-to-end proof green. The two healthy shapes now also prove the published bin still starts.docs/guides/deploying.mdandskills/prisma-composer/SKILL.md.Why every command, not just the deploying ones
The check first ran only for
deploy,destroyanddev, on the assumption that only those load alchemy. The new CI shape disproved it:prisma-composer --helpcrashed with the sameTypeError, because the command graph reaches alchemy's provider tree whatever the arguments say. The check now guards every command, which also settles any question about aliases or flag spellings slipping past it.Alternatives considered
effectas an exactpeerDependency— the obvious candidate, on the theory that npm must then honour our version at the root or fail loudly. Tested against the reported tree:npm installexits 0, hoists the newereffectanyway, marks our peer edgeinvalid, and prints only warnings. Transitive peer conflicts do not hard-fail without--strict-peer-deps, which we cannot impose on consumers, and yarn classic does not auto-install peers at all. It would buy silent breakage plus extra scrollback.overridesblock in a template orinit— helps new apps only, and does nothing for a tree that already exists, like this one. Worth doing separately.effect— a newer pair floats the same way. The durable fix is upstream, alchemy tightening its own ranges; tracked as a follow-up.🤖 Generated with Claude Code