Make Runtime.outputStreamsShareDevice required - #198
Open
wmadden-electric wants to merge 1 commit into
Open
Conversation
This field decides whether a command stdout payload is written at all. Optional, it defaulted to "one screen" and silently dropped that payload for any host that did not know to set it — which is every host except this repo bin. The ORM toolchain bin in prisma/prisma hit exactly that: adopting engine 0.1.1 typechecked clean, because the required Runtime.host errored and this one did not, and `migration graph --dot` stopped printing DOT. Required, the compiler asks each host the question. Behaviour is unchanged everywhere: absent previously meant "same device" via `!== false`, and each call site now says true explicitly. The bin helper answers true when fstat cannot inspect the fds, which is the same answer the default gave it. The test harness gains an outputStreamsShareDevice option. It had no way to express separate sinks, so a test could not assert a stdout payload as a caller with two destinations receives it. 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: 51 minutes Limit details: You’ve used all 1 included review currently available under your plan. 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 (14)
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: |
wmadden
pushed a commit
to prisma/prisma
that referenced
this pull request
Aug 18, 2026
…dding main (#30057) ## What broke 8.0.0-rc.2 shipped with `migration graph --dot` printing no DOT on a terminal, and main has been red on `test/integration/test/cli-journeys/migration-graph-dot.e2e.test.ts` since the release merge (#30049) — silently, because integration tests were not a required check at the time. @prisma/cli-engine 0.1.1 treats a command's stdout payload as a mirror of the human output and suppresses it when stdout and stderr are one screen (`isTty.stdout && isTty.stderr && outputStreamsShareDevice !== false`). For `--dot` the payload was the only copy of the DOT text, so a terminal showed nothing, and piping switches the engine to json mode, so there was no stream shape that produced raw DOT. ## The fix `graphPresentations` now also emits the DOT as a human `drawing` block. That makes the stdout payload a genuine mirror, so the engine's suppression rule is correct in every shape: - one shared screen: the human block shows the DOT on stderr, the stdout mirror is suppressed - split sinks (stdout terminal, stderr elsewhere): stdout carries the raw DOT and stderr the human copy - json mode: unchanged — the result envelope carries `dot` alongside the graph document ## Tests - `packages/1-framework/3-tooling/cli/test/orm/migration-graph.test.ts`: new case pinning the one-shared-screen shape (human block present, stderr shows DOT, stdout empty, payload still declared); the split-shape case now also asserts the raw DOT actually reaches stdout. - `test/integration/test/cli-journeys/migration-graph-dot.e2e.test.ts`: the red journey now asserts the new contract and passes. Verified locally: cli package suite (1421 tests), the DOT journey (3 tests), `pnpm typecheck`, `pnpm lint` — all green. ## Follow-up not in this PR `runtimeFromProcess` still never sets `Runtime.outputStreamsShareDevice`, so two genuinely separate terminals are treated as one screen. Upstream prisma/prisma-cli#198 makes the field required, which will force that decision here when the next engine version is adopted. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Enhancements** * Improved migration graph `--dot` output in human-readable mode. * DOT diagrams now appear in the human presentation while remaining available through the existing standard output format. * Clarified command documentation describing how DOT output is presented. <!-- 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>
wmadden-electric
added a commit
to prisma/prisma
that referenced
this pull request
Aug 18, 2026
…nals keep the stdout mirror The engine suppresses the stdout payload when both output streams are TTYs unless the host says they are separate devices. The bin never answered, so stdout and stderr routed to two different terminals were still treated as one screen and the mirror dropped. runtimeFromProcess now compares the device and inode behind fds 1 and 2 and sets the field; where a stream exposes no fd or cannot be stat-ed the answer stays absent and the engine keeps its one-screen default. Upstream prisma/prisma-cli#198 makes the field required, so this also pre-empts the next engine adoption. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
wmadden-electric
added a commit
to WhyAsh5114/prisma
that referenced
this pull request
Aug 18, 2026
…nals keep the stdout mirror (prisma#30060) ## What this closes The last piece of the cli-engine 0.1.1 stream-suppression adoption. The engine drops a command's stdout payload when both output streams are TTYs, unless the host says they are separate devices via `Runtime.outputStreamsShareDevice`. Our bin adapter never set the field, so stdout and stderr routed to two different terminals were still treated as one screen and the machine-readable stdout copy was dropped. (prisma#30057 fixed the data-loss half — every payload now has a human mirror — this fixes the over-suppression half.) ## The fix `runtimeFromProcess` now compares the device and inode behind the two streams' fds (`fstatSync`) and answers the field: - same file identity → `true`: one screen, mirror suppressed, human copy shown — unchanged behavior - different identities → `false`: two sinks, both copies delivered - no fd on a stream, or fstat fails → field stays absent and the engine keeps its one-screen default, which is the safe common case This also pre-empts prisma/prisma-cli#198, which makes the field required — when the next engine version lands, this adapter already answers. ## Tests Four new cases in `packages/1-framework/3-tooling/cli/test/orm/cli.test.ts` driving `runtimeFromProcess` with real fds: same file twice → `true`, two files → `false`, fd-less streams → absent, unstat-able fd → absent. Verified locally: cli package suite (1425 tests), `pnpm typecheck`, `pnpm lint` — all green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Runtime detection now reports whether standard output and error streams share the same device when this can be determined. * **Bug Fixes** * Improved handling of missing or invalid output stream descriptors by returning an indeterminate result instead of failing. * Improved configuration loading across environments with more reliable module resolution. * **Tests** * Added coverage for shared devices, separate devices, unavailable or invalid stream descriptors, and configuration loading. <!-- 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes
Runtime.outputStreamsShareDevicea required field.Why
This field decides whether a command's stdout payload is written at all. As an optional field it defaulted to "one screen", so a host that never set it silently dropped every stdout payload it produced on a terminal.
Every host except this repo's own bin was in that position.
packages/cli/src/runtime.tsalready answers honestly by comparingfstaton fd 1 and fd 2, so the gap was invisible from inside this repo.It surfaced downstream. The ORM toolchain bin in prisma/prisma adopted engine
0.1.1and typechecked clean —Runtime.hostis required, so the compiler caught that one and it was wired up; this field is optional, so nothing was said and its default took over. The result was a user-visible regression:prisma migration graph --dotstopped printing DOT on a terminal, and piping doesn't recover it because a non-TTY stdout selects JSON mode. Their integration suite caught it; the type system had every opportunity to and didn't.A default that changes what a command prints is not a convenience. It's a decision made on the host's behalf, out of sight of the person who has to live with it.
What changes
Runtime.outputStreamsShareDeviceis nowboolean, notboolean | undefined.renderCompletedHumanreads it directly instead of!== false.packages/cli/src/runtime.tsreturnstruewhenfstatcan't inspect the fds — the same answer the engine's default was already applying, now stated where a reader can see it.createTestCligains anoutputStreamsShareDeviceoption, defaulting totrue. The harness previously had no way to express separate sinks, so no test could assert a command's stdout payload the way a caller with two destinations receives it. That gap is why the downstream regression was awkward to pin down.Runtimeliterals in tests each statetrue.Behaviour
Unchanged, everywhere. Absent previously meant "same device" through
!== false; every call site now saystrueexplicitly and takes the identical branch.The breaking part is the type, and it is the point: any host constructing a
Runtimemust now answer the question. For consumers outside this repo that is a compile error with an obvious fix, which is strictly better than the silent output loss it replaces.Verified
pnpm typecheck— 9/9 tasks.@prisma/cli-engine— 35 files, 819 tests, all pass.@prisma/cli— 62 files, 978 passed, 1 skipped.A full-workspace
pnpm testis red on my machine, but it is equally red on a clean checkout ofmainwith none of these changes —@prisma/compute,@repo/cli-telemetryand@repo/cli-conformancefail there too (@prisma/computereports "No projects were found"). I confirmed by stashing the change and rerunning. Treating CI as the arbiter for those.🤖 Generated with Claude Code