Every presentation a command declares is required - #171
Conversation
`Presentations` made `stdout`, `json` and `next` optional, so a command that omitted `json` got its `--json` contract by accident: the engine published the handler's internal `data` object as the envelope's `result`. The surface machine consumers depend on was set by omission rather than by choice. All four are now required, and every command states what it publishes on each surface. Behaviour is preserved exactly: where a command had no `json` it now returns what the handler passes as `data`, because that is what the envelope carried; where it had no `stdout` or `next` it returns an empty array, because that is what it wrote. The engine's settlement fallback is gone — `result` is the json presentation, not a substitution. One fallback stays, and it is commented where it lives: `materializePresentation` still calls `next` optionally, because @prisma/orm-toolchain@8.0.0-rc.1-dev.40 is compiled against an earlier engine and declares no `next` for `migration list`. Requiring it makes that command exit 2. `stdout` and `json` are required at runtime, which both published families already satisfy. 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 package test scripts now begin with pnpm run build, so turbo runs the engine's build while the CLI's vitest imports the dist it is rewriting. The failure is "Cannot find package '@prisma/cli-engine/testing'" at file level, and the count varies: 13, 34 and 43 across three runs of one branch. Serial turbo and a direct vitest run both pass all 60. The existing entry said the CLI script was a bare vitest run and that turbo was therefore honest. The build step has since been added to both scripts, which is what created the race. 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>
commit: |
Summary by CodeRabbit
WalkthroughThe presentation contract now requires 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.drive/projects/prisma-cli-v8/deferred.md:
- Line 168: Consolidate the duplicate shared-dist build-race documentation by
keeping one canonical entry and removing or merging the redundant entry around
the documented race. Align the canonical entry’s reproduction command, observed
failure message, and remediation with the corresponding details in the other
entry, preserving the recommendation to eliminate concurrent builds of the
shared dist directory.
In `@packages/cli-engine/src/execution/command-context.ts`:
- Around line 58-59: Complete the comment describing the next field’s
compatibility behavior: state that next is the only field that may be absent at
runtime while the other fields remain required, and preserve the surrounding
Presentations requirement.
- Around line 77-83: Update the presentation materialization flow to pass
outcome.data into materializePresentation, and in the returned presentation
object invoke json and stdout presenters optionally to support legacy
Presentations objects. Preserve the existing outputs while defaulting missing
stdout to [] and avoiding runtime errors when json or stdout is absent.
In `@packages/cli-engine/src/execution/settlement.ts`:
- Line 74: Add or update a regression test for the settlement flow around the
JSON presentation handler, using distinct `data` and `json` fixture values and
asserting that the completed JSON envelope’s `result` matches the JSON handler’s
returned value. Ensure the test would fail if the implementation regresses to
using presented.data.
🪄 Autofix
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: c65e6df2-870f-4a44-8ca7-a9a7777b109b
📒 Files selected for processing (69)
.drive/projects/prisma-cli-v8/deferred.mdpackages/cli-engine/src/execution/command-context.tspackages/cli-engine/src/execution/settlement.tspackages/cli-engine/src/presentation.tspackages/cli-engine/src/telemetry/commands.tspackages/cli-engine/tests/blocks.test.tspackages/cli-engine/tests/clack-isolation.test.tspackages/cli-engine/tests/clack-prompts.test.tspackages/cli-engine/tests/command-capabilities.test-d.tspackages/cli-engine/tests/command-families.test.tspackages/cli-engine/tests/config.test.tspackages/cli-engine/tests/credential-manager.test.tspackages/cli-engine/tests/engine.type-test.tspackages/cli-engine/tests/events.test.tspackages/cli-engine/tests/execution.test.tspackages/cli-engine/tests/interaction-affordances.test.tspackages/cli-engine/tests/lifetimes.test.tspackages/cli-engine/tests/management-api.test.tspackages/cli-engine/tests/package-install-matrix.test.tspackages/cli-engine/tests/package-operations.test.tspackages/cli-engine/tests/palette.test.tspackages/cli-engine/tests/prompts.test.tspackages/cli-engine/tests/redirects.test.tspackages/cli-engine/tests/run-hooks.test.tspackages/cli-engine/tests/spawn.test.tspackages/cli-engine/tests/telemetry-payload.test.tspackages/cli-engine/tests/telemetry-run.test.tspackages/cli/src/commands/agent/presentation.tspackages/cli/src/commands/auth/login.tspackages/cli/src/commands/auth/logout.tspackages/cli/src/commands/auth/whoami.tspackages/cli/src/commands/auth/workspace-logout.tspackages/cli/src/commands/auth/workspace-use.tspackages/cli/src/commands/branch/list.tspackages/cli/src/commands/bucket/create.tspackages/cli/src/commands/bucket/delete.tspackages/cli/src/commands/bucket/key-create.tspackages/cli/src/commands/bucket/key-delete.tspackages/cli/src/commands/bucket/key-list.tspackages/cli/src/commands/bucket/list.tspackages/cli/src/commands/feedback.tspackages/cli/src/commands/git/connect.tspackages/cli/src/commands/git/disconnect.tspackages/cli/src/commands/init/presentation.tspackages/cli/src/commands/postgres/backup-list.tspackages/cli/src/commands/postgres/connection-create.tspackages/cli/src/commands/postgres/connection-list.tspackages/cli/src/commands/postgres/connection-remove.tspackages/cli/src/commands/postgres/connection-rotate.tspackages/cli/src/commands/postgres/create.tspackages/cli/src/commands/postgres/list.tspackages/cli/src/commands/postgres/remove.tspackages/cli/src/commands/postgres/restore.tspackages/cli/src/commands/postgres/show.tspackages/cli/src/commands/postgres/usage.tspackages/cli/src/commands/project/env-add.tspackages/cli/src/commands/project/env-remove.tspackages/cli/src/commands/project/env-shared.tspackages/cli/src/commands/project/env-update.tspackages/cli/src/commands/project/presentation.tspackages/cli/src/commands/project/remove.tspackages/cli/src/commands/project/rename.tspackages/cli/src/commands/project/show.tspackages/cli/src/commands/project/transfer.tspackages/cli/src/commands/service/presentation.tspackages/cli/tests/auth.test.tspackages/cli/tests/golden-rendering.test.tspackages/cli/tests/mount-coverage.test.tspackages/cli/tests/whoami.test.ts
Every fixture passed the same value as `data` and as the json presentation, so none of them could tell which one the envelope published. A new fixture makes them differ, and the test fails if settlement goes back to publishing `data`. Verified by making settlement publish `data` and watching it fail, after rebuilding — the engine's own tests resolve the engine through `dist`, so a source edit alone proves nothing. Also restores the deferred entry about a single filter reporting green against a stale engine build, which the previous commit deleted by mistake. The concurrency race it was replaced with was already recorded under S6, in more detail; that entry gains the second error message and the varying failure count seen here. 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>
…answers to Spec R7 requires product-repo e2e in prisma/prisma and composer, and says nothing about the grammar this binary ships. The binary answers to 89 commands, counted by walking --help from the root through every group. 40 have an e2e test. 49 do not. 26 of the 49 come from other packages — 22 from orm-toolchain, 4 from composer — and none of them is run by this repo's e2e suite at all, though it is this binary that breaks when a mount breaks. #171 found that the hard way: requiring next at runtime made migration list exit 2, caught only by an in-process mount test. The other 23 are this repo's own, listed with what each needs. 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>
… human mode I reported that requiring `stdout` at runtime was safe because the ORM mount test passed. That test runs in json mode, and json mode never calls `stdout` — so it could not have detected the failure it was cited as ruling out. orm-toolchain's `migration list` declares `human` and `json` and neither of the other two. In human mode, the default for a user at a terminal, the strict call made it exit 2. `stdout` now uses the same optional call as `next`. `json` stays strict: every ORM command declares one, and a missing json presentation is the defect this change removes. The mount test gains a human-mode run of the same command, because the two formats call different presentation functions and a run in one proves nothing about the other. The real fix is not this shim. orm-toolchain is our code; declaring the missing presentations there and building it against this engine makes the type refuse to compile without them. Both `?.()` come out when that version is pinned here. 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>
Presentationsmadestdout,jsonandnextoptional, so a command that omittedjsongot its--jsoncontract by accident: the engine published the handler's internaldataobject as the envelope'sresult. The surface machine consumers script against was set by omission rather than by choice.All four are now required in the type. Every command states what it publishes on each surface.
Behaviour is preserved exactly
Where a command had no
json, it now returns what the handler passes asdata, because that is what the envelope carried. Where it had nostdoutornext, it returns an empty array, because that is what it wrote. The point is to make each surface a stated choice, so a wrong one can then be corrected one command at a time — not to change any output here.The engine's settlement fallback is gone:
resultis the json presentation, not a substitution for a missing one, and a new test pins that by making a command'sdataandjsondiffer.Two runtime shims, and a correction
materializePresentationstill callsstdoutandnextoptionally.@prisma/orm-toolchainis built against engine0.0.9, where three of the four were optional, and its commands took that up —migration listdeclareshumanandjsonand neither of the others. Calling them unconditionally makes it exit 2:stdoutin human mode,nextin both.An earlier revision of this PR claimed
stdoutwas safe to require. That was wrong, and the way it was wrong matters: I checked it againsttests/orm-mount.test.ts, which runs in json mode — and json mode never callsstdout. The test could not have detected the failure it was cited as ruling out. Human mode, the default for anyone at a terminal, would have exited 2 across 18 ORM commands.The mount test now runs the same command in both formats, with a comment saying why: the two formats call different presentation functions, so a run in one proves nothing about the other.
jsonstays strict. Every ORM command declares one, and a missing json presentation is the defect this change exists to remove.The shims are temporary, and the real fix is ours
This is version skew in our own code, not a foreign contract.
@prisma/orm-toolchainlives inprisma/prismaand pins@prisma/cli-engine@0.0.9. Declaring the missing presentations there — 18 files lackstdout, 16 lacknext— and building it against this engine makes the type refuse to compile without them, which is where the enforcement belongs. Both?.()come out when that version is pinned here.Scope
156 call sites: 95 in the engine (93 of them test fixtures that declared only
human) and 61 in the CLI, concentrated insrc/commands/service/presentation.ts— the service group was written after this change was first attempted on a branch that never merged.Four auth commands and the shared env-file presentation needed care rather than a mechanical edit: their builders take a derived object, not the handler's
data, so returning the builder's own parameter would have changed the--jsonoutput. They now take the result explicitly.A hazard for whoever runs the tests
npm testfails at random on this repo, and it is not this change. Both packagetestscripts begin withpnpm run build, so turbo runs the engine's build while the CLI's vitest imports thedistit is rewriting.npx turbo run test --concurrency=1passes all 94 files every time. Already recorded indeferred.mdunder S6.Verified: build, lint, typecheck clean; 60 CLI test files and 34 engine test files pass serially.