Two engines install where one should: the conformance checker, built and wired - #161
Conversation
…ey raise Shaping work for the conformance checker, brought back with rulings needed before implementation. The finding that shapes the slice: two of the three checks already exist, in the two repos S6 was meant to wire them into, and prisma-cli has none of them. composer shipped four such checks during S3 (engine pin agreement within its own manifests, packed-output import purity, per-entrypoint imports in a fresh node, and a real npm install that counts duplicate copies of effect). prisma/prisma has five more. prisma-cli, which owns both the engine and the shell, runs build and script tests and then publishes. What exists nowhere is the check that catches a live defect: nothing compares the shell's engine pin against the pin of a family it mounts. @prisma/cli ships @prisma/cli-engine at 8.0.0-rc.1 and pins @prisma/composer@0.6.0-dev.16, which declares the engine at 0.0.9, so installing the shell resolves two engines. cli.ts already records this and names the fix composer's under the tandem release order. prisma/prisma contains no reference to the engine and no config sections, so all three checks would be no-ops in its publish path today. That, plus how a second repo consumes the tool without touching S7's release automation, is why the contract ends in questions rather than a plan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 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: 17 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 ignored due to path filters (1)
📒 Files selected for processing (34)
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: |
…rt that is not Four dispatches build the three checks and wire them into prisma-cli's own publish path. That work does not change with any of the open questions: prisma-cli is in scope under every reading of "both products", it has no conformance checks today, and a check's implementation does not depend on which other repo runs it. A fifth dispatch, the cross-repo wiring, is specified but not dispatched. It is the part the rulings decide. Two design notes worth the reader's attention. The unit tests inject the pack, install and bin-start seams so the suite stays fast; the real packing and the real out-of-workspace install happen when the check itself runs, which is the place whose job is to be slow. And the shell already exports its two command families, so naming the array they go into lets the checker consume the shell's own mount list instead of a second list that drifts from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…pies The first draft argued that two engine copies break type identity across the shell-family boundary. That is wrong, and deferred.md says so. The engine's cross-copy markers are Symbol.for, and two tests prove a structured error raised by one copy is recognised by the other. The real argument is stronger. deferred.md:37-51 already rules that the pins must match, that this is a release requirement for the tandem release, and that the two-copy install is a preview-only state to end rather than a configuration to support. It declines to test execution and signal behaviour across two copies precisely because the pins are required to match. So the requirement exists, has been ruled, and nothing anywhere enforces it. That is what the check is for. Also records that the bot has push access to all three repos, so the blocked cross-repo dispatch is waiting on rulings and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…rocess error Fifteen findings, all adopted. Three matter enough to name. I claimed the pin check could not ride on the tarball install, because npm's overrides erase the version divergence it looks for. That was wrong. I had tested a blanket override and one scoped to the shell, but npm also accepts a version-qualified key, which replaces only the matching request. Verified: composer's 0.0.9 survives alongside the local 8.0.0-rc.1. So "exactly one engine resolves in the installed tree" is available, and it is the stronger statement, because it also catches a mismatch arriving through a family's own dependencies. I planned to close the definition-of-done line by rewording it to describe what shipped. That turns an unmet requirement into a met one by editing the requirement. The line stays as written and unchecked, with its dependencies recorded against it. And starting the packed bin proves nothing about the boundary this slice protects. The only declared bin is the legacy commander shell, and of the packed output only dist/v8/cli.js imports composer's family. So the recommendation flips: start the v8 entry by path too, and accept naming a file the manifest does not. Also: check 2's subject set was narrower than the engine's own, which derives sections from families and from standalone mounted commands alike; every check now reports a finding on an empty subject set rather than passing; the exception is keyed on the observed pin triple so a third wrong version reopens it; and the scope split is restated honestly, since only two of the four dispatches are ruling-independent. Fixes a stale comment while here: it named composer dev.15 and engine 0.0.7; the real versions are dev.16 and 0.0.9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…oday Import purity and validator no-throw, the two dispatches that are correct under every open ruling. Thirty-five tests, written before the code they check. The import check reads a real module lexer, not a substring search, and the shipped bundle is why. packages/cli/dist/v8/cli.js contains the string "@repo/cli-telemetry/sender" inside an import.meta.resolve call wrapped in try/catch, with a documented fallback for the published case. It is deliberate and correct. A substring check fails it on day one; the lexer does not see it, because it is not an import. Measured against the real output: 109 files, 16 bare roots, all declared, none unimported. The validator check hammers each shipped config-section validator with 21 hostile inputs. The case that earns its keep is a Proxy whose get and ownKeys traps throw — composer's validator spreads its input inside a try/catch precisely because of it, so a corpus that never got there would pass a validator that crashes in the field. Composer's real validator survives all 21. Both checks report a finding when their subject set is empty, and the import check can be told which specifiers must be present. A check that swept the wrong directory now fails instead of looking like a pass. Subjects come from the union the engine itself uses — command families and standalone mounted commands alike — not from families alone, which would state a narrower rule than the engine enforces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…eck reads Twenty findings, all adopted. Four change what gets built. Packing rebuilds the directory check 1 measures. Both published packages run a build on prepack and tsdown cleans, so pnpm pack wipes and recreates packages/cli/dist. Verified with a sentinel: appended to dist/cli.js, gone after packing. No flag suppresses it. So the checks are strictly ordered, never concurrent, and the tarball check reads only the extracted tarball. Left undiscovered, this would have produced intermittent ENOENT that looked like flakiness. The sandbox install was going to run third-party postinstall scripts — esbuild, workerd, msgpackr-extract — two of which this repo deliberately disables, on a runner holding publish credentials. It now passes --ignore-scripts, verified not to break either bin. My reason for putting the sandbox outside the repo was also wrong. The reviewer ran it inside the worktree successfully; pnpm resolution defeats nothing. The real blocker is corepack's npm shim, which walks up to the repo root, sees pnpm, and refuses. That is a different problem with a different fix, so the question changed. And the checker now runs as a turbo task rather than a root script. tsx removes the build-ordering requirement for the checker itself, not for the two dist directories it reads — and neither workflow builds them, because turbo's test task excludes the shell's own build. Also: a step I had copied from an earlier brief moved wip/ to /tmp before linting. Biome already honours .gitignore, so it did nothing except break a standing rule. Gone. One new question: whether a registry outage should block publishing, since the tarball check cannot reach a verdict without the network. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…checks The first arrangement had the conformance package depend on @prisma/cli so it could reach the shell's command families. That put the checker downstream of the thing it checks, and the consequences were immediate: its `tsc --noEmit` followed the relative source import into the shell's entire command tree, so its typecheck duplicated the shell's and needed the engine's built declarations to be present and settled. Now @repo/cli-conformance depends on nothing it checks. It defines the one shape it needs — a section with a name and a validate function — structurally, which the engine's ConfigSection satisfies. The two consumers depend on it and run it on themselves: the engine's suite checks its own built output, and the shell's suite checks its own output and its own mounted sections. Both are the packages that own the subjects, and they already typecheck their own trees. It exports subpaths rather than a barrel, because noBarrelFile is on. Recorded in deferred.md while here, both out of scope: `pnpm test` fails on a pre-existing race — the engine's test script rebuilds its own dist with a cleaning build while the shell's tests resolve through it, and turbo's test task never waits on a dependency's test. Confirmed against the base commit, which fails three runs out of three; sequential runs pass on both. And the packed shell manifest names two private packages at versions no registry has, which none of the three checks looks at by design. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Both documents described the arrangement that failed: the checker depending on the shell so it could reach its families. They now describe what shipped, and say why the first shape was wrong, so the next reader does not reintroduce it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ngines
All nine questions closed by the operator; the contract records the
rulings. Revisions 1-3 also carried a factual error worth naming: they
claimed the ORM adoption had not started in prisma/prisma, because the
shaping read a stale local checkout. It has landed on that repo's
origin/main — the engine pinned in three manifests, the orm config
section, the ported commands — so both products have real subjects, and
the plan had already answered the question I asked ("wired into both
products' publish CI as S3/S5 land").
What ships here:
The tarball check. Packs both published packages with pnpm, verifies
the packed output imports only what the packed manifest declares,
installs the shell's tarball into a clean sandbox with real npm
(--ignore-scripts; version-qualified overrides supply unpublished
workspace siblings), starts every declared bin on plain node under a
timeout, and measures engine-pin agreement twice: declared manifests
compared pairwise, and copies counted in the installed tree. Thirteen
new tests, all through an injected seam; the suite never touches pnpm,
npm or the network.
The known composer mismatch is recorded as one exception, keyed on the
observed pin triple per the operator's "ignore for now" ruling. The run
prints it with its reason and removal condition and exits 0; composer
arriving at any third version fails.
The wiring. A conformance turbo task depending on build, run by both
publish paths' guards: publish.yml before the dry-run and real publish
steps, pr-quality.yml after the test job. Run end to end here: 0
failing, 2 allowed, 5 subjects.
One placement lesson, learned from a real failure: the sandbox first
lived inside packages/cli, and its ~440-package node_modules slowed
vitest's file crawl enough to time out five unrelated test files. It
now lives under .conformance/ at the repo root, outside every package,
and the full sequential suite passes 7 of 7 alongside it.
prisma/prisma's publish path gets the same checks in its own repo, as
its own scripts, per the per-repo precedent both sibling repos already
follow — that lands as a separate PR from the bot.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…S7 planned S7 landed while this branch was open and shipped scripts/tarball-smoke.mjs in the publish path — written, per its own header, "to S6's check-3b design so the conformance slice absorbs it as a move, not a rewrite". This merge is that move. The conformance check now packs into artifacts/tarballs, so the tarballs it verifies are the exact files the workflow uploads and attaches to the GitHub Release. The smoke's two behaviours mine lacked are adopted with tests: overrides computed transitively (a sibling reached only through another sibling still resolves locally), and the string-shorthand bin form. Bin starts also inherit the smoke's near-empty environment, so a bin must start for a user whose shell carries none of this repo's variables. The three smoke files are deleted and the publish workflow runs one verification step instead of two installs. S7 also mounted the ORM family, and the subject-set pin test failed on merge exactly as designed: the orm section was added to the checked subjects (its validator survives the corpus) and @prisma/orm-toolchain to the pin check, where it carries the same known engine mismatch as composer, under the same recorded ruling. A real install of the shell now resolves THREE engine copies; the check reports all of it, allowed for now, and fails the moment any pin moves anywhere else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ready does The shell's conformance test reads packages/cli/dist and reports a finding when nothing was swept. In the PR quality Test job that guard fired on its first run: the job never runs pnpm build, and turbo's test task depends only on a package's dependencies' builds, never its own. The engine's identical test passed because the engine's test script starts with a build. The shell's now does the same. Reproduced locally by deleting dist before pnpm --filter @prisma/cli test: 85 files pass. The lint job's failure in the same run was a 503 downloading Biome — infrastructure, retried by this push. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
… follow it S2d deleted the commander shell and promoted the engine tree to packages/cli/src, so the module the conformance tests read moved from src/v8/cli.ts to src/cli.ts; the imports follow it, and the stale composer-pin comment corrected on this branch is re-corrected in the file's new home. The tarball-smoke scripts this branch had absorbed into the conformance check came back through main with a small test edit; the absorption stands and the three files stay deleted. All gates green on the merged tree: lint, typecheck, script tests, the full sequential suite 7 of 7, and the conformance run itself — 0 failing, 3 allowed, with the verified tarballs landing in artifacts/tarballs for the release pipeline to attach. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…lidator that never throws, and a tarball that installs (prisma#29998) ``` $ pnpm check:conformance Packing 17 publishable packages → …/.conformance/tarballs OK — packed output imports only declared packages, the shipped orm validator survives the hostile corpus, the @prisma/orm-toolchain tarball installs clean and its bins start, and every @prisma/cli-engine pin agrees. $ echo $? 0 ``` That run packed all 17 publishable packages, parsed every `.js`/`.mjs` inside each tarball, ran the built `ormConfigSection.validate` over 21 hostile inputs, installed the `@prisma/orm-toolchain` tarball into a clean npm sandbox at `.conformance/`, started its bin with plain `node … --version`, and compared every `@prisma/cli-engine` pin. It found nothing — the repo passes today. ## Decision Add a conformance step to the publish workflow, immediately after `Check publish dependency specifiers`, running three checks over what actually ships: 1. **Import purity over built JavaScript.** Every bare specifier the packed JavaScript imports (static imports, re-exports, dynamic `import()`) must be declared in the packed manifest's `dependencies`/`peerDependencies`/`optionalDependencies` or be a Node builtin. `@internal/*` and `@repo/*` imports are always findings — those names never exist on the registry. Parsing uses es-module-lexer, so a package name in a string or an `import.meta.resolve` argument never counts as an import. This complements `check-publish-deps.mjs`, which reads only declaration files. 2. **The shipped orm config-section validator never throws.** `ormConfigSection.validate` is loaded from the built `@prisma/orm-toolchain` dist (`dist/cli.mjs`) — the code that ships, not source — and run over a fixed hostile corpus (`undefined`, `null`, bigints, symbols, frozen and null-prototype objects, throwing proxies and getters, …). It must return a well-formed `SectionValidation` for every input. 3. **Published-tarball verification.** (a) is check 1 running on packed tarballs. (b) The `@prisma/orm-toolchain` tarball installs into a clean sandbox with `npm install --no-audit --no-fund --ignore-scripts`; unpublished workspace siblings are supplied as npm `overrides` with version-qualified keys and absolute `file:` tarball paths, computed recursively from the packed manifests, and every packed `bin` entry must start under plain `node <path> --version` within 30 seconds. (c) The `@prisma/cli-engine` pin must be a single exact version, identical across every packed publishable manifest that declares it and the source `@internal/cli` manifest (all `0.0.9` today). Every check reports a finding when its subject set is empty, so a run that checked nothing cannot pass. `--json` emits a machine-readable report. ## How it is built The script follows this repo's publish-gate conventions: `scripts/check-conformance.mjs` is ESM with an injected `io` seam (`runCheck({ argv, io })` merged over a `DEFAULT_IO`), modelled on `check-publish-deps.mjs`. The unit tests in `scripts/check-conformance.test.mjs` exercise the finding logic through the seam with plain values — no packing, no installs, no network — and the file is added to the hand-maintained `test:scripts` list. The real pack/install runs only when the script itself runs. The sandbox lives at `.conformance/` at the repo root (gitignored). It is deleted at the start of each run, not the end, so a failed run leaves its sandbox and tarballs behind as evidence. The install runs with `COREPACK_ENABLE_STRICT=0` because corepack's npm shim walks up to the repo root, sees pnpm in `packageManager`, and refuses otherwise. npm is deliberate here — real consumers install with npm. `es-module-lexer` becomes a root devDependency (it was previously only declared in `packages/0-config/tsdown`, so it did not resolve from `scripts/`). This is the prisma/prisma half of prisma-cli's conformance slice (prisma/prisma-cli#161); the same three checks run in that repo against its own packages. ## Verification - `node --test scripts/check-conformance.test.mjs` — 37 tests, exit 0 - `pnpm test:scripts` — 417 tests, exit 0 - `pnpm check:conformance` — exit 0 against the real repo (output above) - `biome check` on both new files — exit 0 ## Alternatives considered - **A regex sweep instead of a lexer** — rejected; the shipped output legitimately contains package names in strings and `import.meta.resolve` arguments, and a regex cannot tell those from imports. - **Importing the validator from source** — rejected; the built `@prisma/orm-toolchain/dist/cli.mjs` loads and exports `ormConfigSection` directly, so the check exercises exactly what ships. - **Reusing `check-publish-deps.mjs`'s tarball pass** — rejected; that script checks declaration files and manifests, and folding a JavaScript sweep, a validator harness, and a sandbox install into it would blur two gates with different failure modes. The two scripts share the pack-and-read pattern instead. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Quality Improvements** * Added automated conformance checks to publishing workflows. * Validates package imports, configuration validation behavior, dependency consistency, and clean installation from published packages. * Added checks for executable availability and reporting of packaging or installation issues. * **Testing** * Added comprehensive coverage for successful and failure scenarios, including malformed inputs, packaging errors, dependency mismatches, and command failures. * **Maintenance** * Added temporary conformance output to ignored files. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What is wrong today
Install the shell and you get two copies of the CLI engine.
deferred.md:37-51already rules that these pins must match and calls it a release requirement for the tandem release. Nothing enforced it: composer's own engine-pin check compares composer's two manifests against each other and never looks at the shell's. Now the publish path measures it — twice, from the declared manifests and by counting engine copies in a real install — and the one known mismatch is suppressed by a recorded exception that names its removal condition, so any new mismatch fails the publish.The decision
The slice mandate (project plan §S6): a small three-check tool — import purity, validator no-throw on hostile input, published-tarball verification — wired into both products' publish CI. All shaping questions were closed by the operator on 2026-08-12; the record is in
.drive/projects/prisma-cli-v8/specs/s6-conformance.md§5.The checker is a private workspace package,
@repo/cli-conformance, that depends on nothing it checks. Each checked package runs the checks on itself: the engine's test suite checks the engine's built output, the shell's suite checks its output and its mounted config sections, and aconformanceturbo task runs all three checks — including the real pack, sandbox install and bin start — in both CI paths that publish.What the checks are
Import purity. Every bare specifier the built output imports must belong to a package a consumer will have installed; every declared runtime dependency must be reached. Measured with
es-module-lexer, not substring search — the shipped bundle contains@repo/cli-telemetry/senderinside animport.meta.resolvecall with a documented published-case fallback, which is deliberate and correct, and which a substring check would fail on day one.Validator no-throw. Every config-section validator the shell mounts survives a fixed 21-case hostile corpus (R10: a throwing validator turns a user's config mistake into a CLI crash at exit 1). The corpus includes a
ProxywhosegetandownKeystraps throw — composer's validator guards against exactly that, so a corpus that never got there would pass a validator that crashes in the field. The subject set is pinned: when the ORM slice's section reaches this shell, the pin test fails and the new validator gets checked rather than silently skipped.Tarball verification. Pack both published packages with pnpm; verify the packed output's imports against the packed manifest; install the shell's tarball into a clean sandbox with real npm (
--ignore-scripts, version-qualifiedoverridessupplying unpublished workspace siblings); start every declared bin on plain Node under a timeout; and require engine-pin agreement between the shell and every mounted family, on the manifests and in the installed tree.Wiring
publish.yml: a conformance step before both the dry-run and real publish steps.pr-quality.yml: the same in the Test job, so a broken tarball is found on the pull request.ormsection validator, theprisma-nextbin, the engine pin in three manifests.Numbers
48 tests written before the code they check (44 in the checker's suite, 3 in the shell's, 1 in the engine's). Full end-to-end run: 0 failing, 2 allowed, 5 subjects, ~40 s warm.
pnpm lint,pnpm typecheck, andturbo run test --concurrency=1all green.Found on the way, recorded in
deferred.md, not this PR's to fixpnpm testfails on main (three runs out of three): the engine'stestscript rebuilds its owndistwith a cleaning build while the shell's tests resolve through that directory, and turbo'stesttask never waits on a dependency'stest. Sequential runs pass. CI has been passing by timing luck.devDependenciesname two private packages at versions no registry has — harmless for tarball installs, fatal for installing the unpacked directory, and deliberately outside these checks' scope.Alternatives considered
Substring-match the built output for forbidden names, as the engine's
no-child-process-in-disttest does. Correct there; wrong here, because the shell legitimately mentions a private package name in a string. A lexer distinguishes an import from a string.The checker depending on the packages it checks. Built that way first; its typecheck then traversed the shell's whole command tree and failed against unbuilt declarations. Inverted: the checker declares the one shape it needs structurally, and the owners run the checks on themselves.
Asserting "exactly one engine resolves" only, without the manifest comparison. The install-tree count is the stronger claim but needs the network and an install; the manifest comparison is offline and names both sides in the finding. Both run.
The sandbox inside
packages/cli. Tried; its ~440-packagenode_modulesslowed vitest's file crawl enough to time out five unrelated test files. It lives at the repo root now, outside every package.🤖 Generated with Claude Code