From 150b62ed5e6ea23c4374e8452cbda2c04af311bc Mon Sep 17 00:00:00 2001 From: Aman Varshney Date: Fri, 14 Aug 2026 18:57:02 +0530 Subject: [PATCH 1/5] feat(engine): support structured delegated output Signed-off-by: Aman Varshney --- .../assets/engine/engine-interface-draft.ts | 25 ++- .../assets/s2/parity-divergences-s3.md | 30 ++-- .../prisma-cli-v8/specs/s3-composer.md | 5 +- docs/product/output-conventions.md | 17 ++ packages/cli-engine/src/commands.ts | 14 +- packages/cli-engine/src/execution/engine.ts | 41 +---- packages/cli-engine/src/execution/help.ts | 9 +- .../cli-engine/src/execution/settlement.ts | 57 +++++- packages/cli-engine/src/execution/spawn.ts | 1 + .../src/execution/stricli-adapter.ts | 9 +- packages/cli-engine/src/spawn.ts | 12 +- packages/cli-engine/src/testing.ts | 2 + .../cli-engine/tests/fixtures/spawn-host.mjs | 13 +- .../cli-engine/tests/spawn-real-child.test.ts | 17 +- packages/cli-engine/tests/spawn.test.ts | 168 +++++++++++++++--- .../cli-engine/tests/telemetry-run.test.ts | 8 +- packages/cli/src/runtime.ts | 4 +- packages/cli/src/spawn.ts | 53 +++--- packages/cli/tests/spawn-adapter.test.ts | 38 +++- 19 files changed, 352 insertions(+), 171 deletions(-) diff --git a/.drive/projects/prisma-cli-v8/assets/engine/engine-interface-draft.ts b/.drive/projects/prisma-cli-v8/assets/engine/engine-interface-draft.ts index 00bb1971..248c7248 100644 --- a/.drive/projects/prisma-cli-v8/assets/engine/engine-interface-draft.ts +++ b/.drive/projects/prisma-cli-v8/assets/engine/engine-interface-draft.ts @@ -33,11 +33,13 @@ * Amended 2026-08-11 for S3 (the TERMINAL HANDOFF, contract * s3-composer.md): §4 gains ctx.spawn and §4c its shapes + * exitWithChildStatus; §6 gains the SpawnDeclarations (maySpawn) and - * the two kind amendments (a maySpawn command rejects --json as soon - * as the command is known — after routing, before the needs check and - * before anything runs; a session settles non-zero through + * the two kind amendments (a maySpawn command owns structured stdout + * while routing child output to diagnostics in json mode; a session settles non-zero through * exitWithChildStatus and no other way); §10 gains Runtime.spawn. D1 * rulings: abort-ladder grace 5s; near-expiry refusal threshold 5min. + * Amended 2026-08-14: maySpawn commands now support json. Human mode + * still delegates the terminal; json mode routes child output to + * diagnostic stderr and preserves framed stdout through settlement. * Re-amended after the PR-136 review round: handing credentials to the * child is a PRECONDITION, `needs: { credentials: 'child' }` — the * separate credentialsForSpawn declaration is gone, and the entailment @@ -1103,19 +1105,14 @@ export type Handler< * S3: the terminal-handoff declaration, accepted by defineCommand and * defineSessionCommand and normalized onto every definition (server * commands normalize maySpawn to false: they own stdio already). - * `maySpawn` unlocks ctx.spawn and makes the command reject `--json` - * as soon as the command is known — after routing, before the needs - * check and before anything runs (the rule depends on which command - * was selected, so "parse time" was loose wording) — exit 2, stated - * in help (delegated terminal output cannot be framed). Handing + * `maySpawn` unlocks ctx.spawn. Human mode hands over the terminal; + * json mode routes the child's stdout and stderr to diagnostics while + * the engine retains framed stdout and emits a terminal result. Handing * credentials to the child is a PRECONDITION, not a declaration: * `needs: { credentials: 'child' }` (see NeedsSpec). - * Naming note (PR-136 review, considered and rejected): renaming - * maySpawn to delegatesTerminal would state the --json rule's premise - * directly, but the identifier is already woven through the S3 stack - * (D2/D3 handlers, tests, this draft) and the rename's churn was - * judged to outweigh the clarity gain. Do not re-open without new - * evidence. + * Naming note (PR-136 review): `maySpawn` remains the capability name; + * structured mode no longer delegates stdout, so `delegatesTerminal` + * would now be actively misleading. */ export interface SpawnDeclarations { readonly maySpawn?: boolean diff --git a/.drive/projects/prisma-cli-v8/assets/s2/parity-divergences-s3.md b/.drive/projects/prisma-cli-v8/assets/s2/parity-divergences-s3.md index 6b2bde7f..a338d8d5 100644 --- a/.drive/projects/prisma-cli-v8/assets/s2/parity-divergences-s3.md +++ b/.drive/projects/prisma-cli-v8/assets/s2/parity-divergences-s3.md @@ -95,24 +95,18 @@ instead. Three things change for the user: handler asked for. Legacy collapsed a signalled child's status (`run-alchemy.ts:61`) and printed the hint anyway. -## `--json` is refused on `deploy`, `destroy` and `dev`; `log` gains it - -No composer command had a json mode at all. The engine gives every -command one, and these three give it back: they hand the terminal to a -child process whose output cannot be framed, so `--json` (and -`--format json`) is rejected **at parse time**, before the command does -any work — `CLI.JSON_UNSUPPORTED`, exit 2, with the reason stated in the -command's own help ("This command hands the terminal to another program -and does not support --json."). - -`log` never spawns and keeps json: its lines are its payload. So a -composer user gains a machine-readable `log` and gains an explicit, -early refusal on the other three, where before there was no flag to -pass. - -Note the interaction with format auto-selection: a piped -`prisma composer deploy ` does not silently become a json run — -it stays human, because json is refused rather than auto-selected. +## `deploy`, `destroy`, `dev`, and `log` support structured output + +Amended 2026-08-14: `maySpawn` no longer disables JSON. In human mode the +child still inherits the terminal. In JSON mode its stdout and stderr are +routed to diagnostic stderr while the engine retains framed NDJSON stdout and +emits the command family's terminal result. A failed child keeps its verbatim +process exit code and emits `CLI.CHILD_PROCESS_FAILED` with the child status in +`error.meta`. + +This also restores normal format auto-selection: a piped +`prisma composer deploy ` produces structured output, including +Composer's deployment summary, without requiring an explicit flag. ## Usage, parse errors and bare invocation diff --git a/.drive/projects/prisma-cli-v8/specs/s3-composer.md b/.drive/projects/prisma-cli-v8/specs/s3-composer.md index 7c9c69b6..1d0b4d63 100644 --- a/.drive/projects/prisma-cli-v8/specs/s3-composer.md +++ b/.drive/projects/prisma-cli-v8/specs/s3-composer.md @@ -100,8 +100,9 @@ const child = await ctx.spawn({ command, args, cwd, env }); through this path (it used to hard-code 0, and now also settles 130/143 on its own — below), and the session-kind "always supports json" guarantee is amended: - a command that may spawn rejects `--json` at PARSE time - (delegated terminal output cannot be framed; stated in help). + a command that may spawn keeps stdout framed in JSON mode by routing the + child's output to diagnostic stderr; the session then emits its normal + terminal result frame. - **Exit codes are the engine's** (operator ruling, 2026-08-11): a run a delivered signal terminated settles 128+signal from the ENGINE's own record of that signal, for both command kinds and diff --git a/docs/product/output-conventions.md b/docs/product/output-conventions.md index 807f7f94..0ca9d38d 100644 --- a/docs/product/output-conventions.md +++ b/docs/product/output-conventions.md @@ -29,6 +29,23 @@ Rules: - never write decorative or human-only output to stdout - when `--json` is active, stdout must contain only structured output +### Delegated child processes + +Commands that declare `maySpawn` use the same format rules as every other +command: + +- in human mode, the child inherits stdin, stdout, and stderr +- in JSON mode, stdin remains inherited while the child's stdout and stderr + are routed to the CLI's diagnostic stderr stream +- JSON stdout remains an NDJSON event stream with exactly one terminal result + frame +- a non-zero child status is preserved as the process exit code and is + represented by `CLI.CHILD_PROCESS_FAILED`, with `exitCode` and `signal` in + `error.meta` + +This lets automation consume a command family's structured result without +having to parse the delegated tool's human output. + ## TTY and Piped Behavior Interactive TTY behavior: diff --git a/packages/cli-engine/src/commands.ts b/packages/cli-engine/src/commands.ts index f656466f..729a9bca 100644 --- a/packages/cli-engine/src/commands.ts +++ b/packages/cli-engine/src/commands.ts @@ -122,10 +122,9 @@ function normalizeArgs< * The terminal-handoff declaration, normalized onto every definition * (server commands normalize to false: they own stdio already). * - * `maySpawn` unlocks ctx.spawn and makes the command reject `--json` - * as soon as the command is known, before anything runs: output - * delegated to a child process cannot be framed. Handing credentials - * to the child is a precondition, declared as + * `maySpawn` unlocks ctx.spawn. In human mode the child inherits the + * terminal; in json mode its output is routed to diagnostics so stdout + * remains framed. Handing credentials to the child is a precondition, declared as * `needs: { credentials: "child" }`. */ export interface SpawnDeclarations { @@ -296,10 +295,9 @@ export function defineCommand< * speaks entirely through events, returns Result. No * presentation, no exit-code set. * - * A session supports json mode — the event stream is its json surface — - * UNLESS it declares `maySpawn`, in which case it rejects --json as soon - * as the command is known, before anything runs (S3): output delegated - * to a child process cannot be framed. A session that returns + * A session supports json mode — the event stream is its json surface. + * When it declares `maySpawn`, child output is routed to diagnostics while + * the engine retains ownership of structured stdout. A session that returns * ok(undefined) exits 0 — or 130/143 when a signal ended the run, which * the engine settles from its own record of that signal, not from * anything the handler returns; one that returns diff --git a/packages/cli-engine/src/execution/engine.ts b/packages/cli-engine/src/execution/engine.ts index fec829e5..09d1d060 100644 --- a/packages/cli-engine/src/execution/engine.ts +++ b/packages/cli-engine/src/execution/engine.ts @@ -15,7 +15,7 @@ import type { ActiveCredential } from "../credential-manager"; import type { EngineEvent, Severity, StreamEvent } from "../events"; import type { ManagementApiClient } from "../management-api"; import type { Format, PresentedResult } from "../presentation"; -import { CliStructuredError, type Result } from "../protocol"; +import type { CliStructuredError, Result } from "../protocol"; import type { EngineCommandSnapshot, RunSummary } from "../run-summary"; import type { InputStream, Runtime } from "../runtime"; import { @@ -45,11 +45,7 @@ import { renderHelp, } from "./help"; import { checkNeeds, type NeedsOutcome } from "./needs"; -import { - configFlagGivenNoValue, - formatFlagGiven, - versionFlagGiven, -} from "./pre-parse-argv"; +import { configFlagGivenNoValue, versionFlagGiven } from "./pre-parse-argv"; import { commandSegments, settleBug, @@ -520,10 +516,8 @@ export class EngineImpl implements Engine { } } - /** The refusals a maySpawn command can hit before anything runs: a - * child's terminal output cannot be framed as a json stream, and a - * host that mounts the command without a spawn adapter is - * misconfigured. Returns whether the run was settled here. */ + /** A host that mounts a maySpawn command without a spawn adapter is + * misconfigured. Refuse before needs or handler side effects. */ private refuseUnspawnable( invocation: Invocation, entry: CommandTreeEntry, @@ -532,11 +526,6 @@ export class EngineImpl implements Engine { if (!entry.def.maySpawn) { return false; } - if (formatFlagGiven(state.argv) === "json") { - state.format = "human"; - settleErrored(invocation, jsonUnsupportedError(entry.id)); - return true; - } if (invocation.runtime.spawn === undefined) { // The run is doomed, and refuses here, before the needs check, // rather than mid-handler after side effects. @@ -632,9 +621,6 @@ export class EngineImpl implements Engine { let needsOutcome: NeedsOutcome; try { applySharedFlags(state, rawFlags as SharedFlags, invocation.runtime); - if (entry.def.maySpawn) { - state.format = "human"; - } needsOutcome = await checkNeeds(entry.def, invocation); } catch (cause) { settleBug(invocation, cause); @@ -791,25 +777,6 @@ function declaredConfigSections(spec: EngineSpec): readonly string[] { ]; } -/** A command that may hand the terminal to a child cannot frame its - * output, so json is refused as soon as the command is known — before - * the needs check and before anything runs. */ -function jsonUnsupportedError(commandId: string): CliStructuredError { - return new CliStructuredError( - "CLI.JSON_UNSUPPORTED", - `The '${commandId.replaceAll(".", " ")}' command does not support json output.`, - { - why: "It hands the terminal to another program, whose output cannot be framed as a json stream.", - nextActions: [ - { - kind: "user-choice", - label: "Run it without --json or --format json.", - }, - ], - }, - ); -} - function declaredCapabilities(def: AnyCommand): CommandCapabilities { if (def.kind !== "result-command") { return { managesCredentials: false, installsPackages: false }; diff --git a/packages/cli-engine/src/execution/help.ts b/packages/cli-engine/src/execution/help.ts index a5be2275..f42f9d52 100644 --- a/packages/cli-engine/src/execution/help.ts +++ b/packages/cli-engine/src/execution/help.ts @@ -17,7 +17,7 @@ import type { CommandTreeEntry, CommandTreeNode } from "./command-tree"; import type { EngineSpec } from "./engine"; import { makePaint, type Paint, textWidth } from "./palette"; import { SHARED_ALIASES, SHARED_FLAG_PARAMETERS } from "./shared-flags"; -import { NO_JSON_NOTE, resolveExample } from "./stricli-adapter"; +import { resolveExample } from "./stricli-adapter"; const RAIL = "│"; const GAP = " "; @@ -487,13 +487,6 @@ function renderLeafHelp( lines.push(rail(paint)); proseLines(def.help.description, paint, lines); } - if (def.maySpawn) { - lines.push(rail(paint)); - // One line on purpose: the sentence is the contract several tests - // and consumers grep for, so it never wraps. - lines.push(rail(paint, paint("muted", NO_JSON_NOTE))); - } - const positionalEntries = Object.values>( def.args.positionals, ).map((spec) => positionalRuntime(spec)); diff --git a/packages/cli-engine/src/execution/settlement.ts b/packages/cli-engine/src/execution/settlement.ts index bf62febb..e18bbf65 100644 --- a/packages/cli-engine/src/execution/settlement.ts +++ b/packages/cli-engine/src/execution/settlement.ts @@ -211,9 +211,8 @@ export function settleVerbatimExitCode( * The status comes from the engine's own record of the child, never * from the handler, so there is nothing here for a handler to state. * Two conditions fence it, both construction errors: the command must - * hand the terminal to another program — reachable from a - * non-declaring handler this would also end a json stream without its - * terminal result frame — and a child must actually have run. + * declare that it can hand execution to another program, and a child must + * actually have run. * * A signal-killed child overrules whatever the handler asked for. The * user stopped the run: it settles 128 + the signal number, with no @@ -235,7 +234,11 @@ export function settleChildStatus( `@prisma/cli-engine: command '${invocation.state.commandId}' returned exitWithChildStatus without a child having run — that settlement reports the status of a child ctx.spawn started, and this run started none`, ); } - // Only human format is reachable here: maySpawn forces it. + const exitCode = childExitCode(child); + if (invocation.state.format === "json") { + settleStructuredChildStatus(invocation, settlement, child, exitCode); + return; + } if (child.signal === null) { for (const action of settlement.nextActions) { invocation.runtime.stderr.write( @@ -243,7 +246,51 @@ export function settleChildStatus( ); } } - settleVerbatimExitCode(invocation, childExitCode(child)); + settleVerbatimExitCode(invocation, exitCode); +} + +function settleStructuredChildStatus( + invocation: Invocation, + settlement: ChildStatusSettlement, + child: { readonly exitCode: number | null; readonly signal: string | null }, + exitCode: number, +): void { + invocation.state.settledExitCode = exitCode; + const nextActions = child.signal === null ? settlement.nextActions : []; + if (exitCode === 0) { + const envelope: CompletedEnvelope = { + ok: true, + commandId: invocation.state.commandId, + result: null, + exitCode, + diagnostics: [], + nextActions, + }; + emitFrame(invocation, { + kind: "result", + envelope, + commandId: invocation.state.commandId, + timestamp: invocation.now().toISOString(), + }); + return; + } + const how = + child.signal === null + ? `exited with code ${String(child.exitCode)}` + : `was terminated by ${child.signal}`; + emitErrored(invocation, { + ok: false, + commandId: invocation.state.commandId, + error: { + code: "CLI.CHILD_PROCESS_FAILED", + severity: "error", + summary: `The delegated process ${how}.`, + nextActions, + meta: { exitCode: child.exitCode, signal: child.signal }, + }, + diagnostics: [], + nextActions, + }); } /** A session command that returned ok shut down cleanly: no diff --git a/packages/cli-engine/src/execution/spawn.ts b/packages/cli-engine/src/execution/spawn.ts index 3be412e5..666ec6e6 100644 --- a/packages/cli-engine/src/execution/spawn.ts +++ b/packages/cli-engine/src/execution/spawn.ts @@ -138,6 +138,7 @@ async function runDelegated( args: options.args ?? [], cwd: options.cwd ?? invocation.runtime.cwd, env: await composeChildEnv(invocation, def, options.env), + output: state.format === "json" ? "diagnostic" : "inherit", }; debug( `spawn: ${request.command} ${request.args.join(" ")} (cwd ${request.cwd})`, diff --git a/packages/cli-engine/src/execution/stricli-adapter.ts b/packages/cli-engine/src/execution/stricli-adapter.ts index e9334228..17572b07 100644 --- a/packages/cli-engine/src/execution/stricli-adapter.ts +++ b/packages/cli-engine/src/execution/stricli-adapter.ts @@ -181,11 +181,6 @@ export function resolveExample(example: string, cliName: string): string { : `${cliName} ${example}`; } -/** The --json refusal is stated in help, so a machine consumer learns - * it without running the command. */ -export const NO_JSON_NOTE = - "This command hands the terminal to another program and does not support --json."; - function commandDocs( def: AnyCommand, cliName: string, @@ -193,15 +188,13 @@ function commandDocs( const examples = def.help.examples.map((example) => resolveExample(example, cliName), ); - const notes = def.maySpawn ? ["", NO_JSON_NOTE] : []; - if (examples.length === 0 && notes.length === 0) { + if (examples.length === 0) { return { brief: def.help.summary, fullDescription: def.help.description }; } return { brief: def.help.summary, fullDescription: [ def.help.description ?? def.help.summary, - ...notes, ...(examples.length === 0 ? [] : ["", "Examples:", ...examples.map((example) => ` ${example}`)]), diff --git a/packages/cli-engine/src/spawn.ts b/packages/cli-engine/src/spawn.ts index 1948a1a9..2f21b790 100644 --- a/packages/cli-engine/src/spawn.ts +++ b/packages/cli-engine/src/spawn.ts @@ -13,6 +13,9 @@ export interface SpawnRequest { readonly args: readonly string[]; readonly cwd: string; readonly env: Readonly>; + /** Human mode hands the terminal over unchanged. Structured mode keeps + * stdout framed and routes the child's human output to diagnostics. */ + readonly output: "inherit" | "diagnostic"; } /** How a child ended. A signal-killed child carries `signal` and a null @@ -33,10 +36,11 @@ export interface SpawnedChild { } /** - * The Runtime seam. The adapter starts the child with INHERITED stdio, - * in the caller's own process group (POSIX) / console (Windows) — no - * `detached`, no new console — so the terminal delivers Ctrl-C to the - * child natively. + * The Runtime seam. In human mode the adapter starts the child with + * inherited stdio, in the caller's own process group (POSIX) / console + * (Windows). In structured mode stdin remains inherited while stdout and + * stderr are routed to the host's diagnostic stream, preserving framed + * stdout. Neither mode detaches or opens a new console. */ export type SpawnChild = (request: SpawnRequest) => SpawnedChild; diff --git a/packages/cli-engine/src/testing.ts b/packages/cli-engine/src/testing.ts index 99053d6e..5284d9c5 100644 --- a/packages/cli-engine/src/testing.ts +++ b/packages/cli-engine/src/testing.ts @@ -34,6 +34,7 @@ export interface SpawnRecord { readonly command: string; readonly args: readonly string[]; readonly cwd: string; + readonly output: SpawnRequest["output"]; /** Environment KEYS only. Values are never recorded: a fixture file * must not be able to carry token material. */ readonly envKeys: readonly string[]; @@ -83,6 +84,7 @@ function recordingSpawn( command: request.command, args: [...request.args], cwd: request.cwd, + output: request.output, envKeys: Object.keys(request.env), kills: [], }; diff --git a/packages/cli-engine/tests/fixtures/spawn-host.mjs b/packages/cli-engine/tests/fixtures/spawn-host.mjs index 22432d88..fc4ca413 100644 --- a/packages/cli-engine/tests/fixtures/spawn-host.mjs +++ b/packages/cli-engine/tests/fixtures/spawn-host.mjs @@ -16,11 +16,16 @@ const [scenario, dir] = process.argv.slice(2); const childScript = fileURLToPath(new URL("./child.mjs", import.meta.url)); const spawnChild = (request) => { + const structured = request.output === "diagnostic"; const child = spawn(request.command, [...request.args], { cwd: request.cwd, env: request.env, - stdio: "inherit", + stdio: structured ? ["inherit", "pipe", "pipe"] : "inherit", }); + if (structured) { + child.stdout.on("data", (chunk) => process.stderr.write(chunk)); + child.stderr.on("data", (chunk) => process.stderr.write(chunk)); + } writeFileSync(join(dir, "child-pid"), String(child.pid)); return { ended: new Promise((resolve, reject) => { @@ -80,7 +85,11 @@ const runtime = { }, cwd: process.cwd(), env: process.env, - isTty: { stdin: false, stdout: false, stderr: false }, + isTty: { + stdin: false, + stdout: scenario !== "unframed-stdout", + stderr: scenario !== "unframed-stdout", + }, exit: (code) => process.exit(code), onSignal: (subscriber) => { // The marker is written AFTER the engine has handled the press, so diff --git a/packages/cli-engine/tests/spawn-real-child.test.ts b/packages/cli-engine/tests/spawn-real-child.test.ts index 926a20f0..eb0e1301 100644 --- a/packages/cli-engine/tests/spawn-real-child.test.ts +++ b/packages/cli-engine/tests/spawn-real-child.test.ts @@ -140,7 +140,7 @@ describe.skipIf(process.platform === "win32")( spawn: realSpawn, }); - const result = await cli.run(["converge"]); + const result = await cli.run(["converge", "--format", "human"]); expect(result.exitCode).toBe(2); expect(result.stderr).toContain("CLI.SPAWN_FAILED"); @@ -297,19 +297,22 @@ function startHost( describe.skipIf(process.platform === "win32")( "ctx.spawn, real child through a real engine host", () => { - test("child stdio is inherited and unframed; buffered commentary flushes after it", async () => { + test("structured mode routes child output to diagnostics and keeps stdout framed", async () => { const dir = scratch(); const host = startHost("unframed-stdout", dir); const run = await host.done; expect(run.exitCode).toBe(0); - expect(run.stdout).toBe("child-said-hello\n"); + const frames = run.stdout + .trim() + .split("\n") + .map((line) => JSON.parse(line) as { kind: string; text?: string }); + expect(frames.map((frame) => frame.kind)).toEqual(["message", "result"]); + expect(frames[0]?.text).toBe("during-child"); + expect(run.stdout).not.toContain("child-said-hello"); + expect(run.stderr).toContain("child-said-hello"); expect(run.stderr).toContain("child-said-stderr"); - expect(run.stderr).toContain("during-child"); - expect(run.stderr.indexOf("during-child")).toBeGreaterThan( - run.stderr.indexOf("child-said-stderr"), - ); }); test("native Ctrl-C reaches the child through the shared process group", async () => { diff --git a/packages/cli-engine/tests/spawn.test.ts b/packages/cli-engine/tests/spawn.test.ts index e25d5b4a..51045000 100644 --- a/packages/cli-engine/tests/spawn.test.ts +++ b/packages/cli-engine/tests/spawn.test.ts @@ -1,6 +1,6 @@ /** - * ctx.spawn against the harness's scripted fake child: the --json - * refusal, credential injection through both credential origins, the + * ctx.spawn against the harness's scripted fake child: structured output, + * credential injection through both credential origins, the * near-expiry refusal, reentrancy, buffered commentary, the abort * ladder, and signal record-and-replay. */ @@ -49,43 +49,106 @@ const converge = defineCommand({ }, }); -describe("--json is refused for commands that may spawn", () => { - test("--json settles 2 with a structured refusal and no frames", async () => { +describe("structured output for commands that may spawn", () => { + test("--json keeps stdout framed and marks the child diagnostic", async () => { const cli = createTestCli({ commands: { converge }, now: CLOCK }); const result = await cli.run(["converge", "--json"]); - expect(result.exitCode).toBe(2); - expect(result.json).toEqual([]); - expect(result.stderr).toContain("does not support json output"); - expect(result.spawns).toEqual([]); + expect(result.exitCode).toBe(0); + expect(result.json).toHaveLength(1); + expect(result.json[0]).toMatchObject({ + kind: "result", + envelope: { ok: true, commandId: "converge", result: null, exitCode: 0 }, + }); + expect(result.spawns[0]?.output).toBe("diagnostic"); }); - test("--format json is refused the same way", async () => { + test("--format json selects the same structured handoff", async () => { const cli = createTestCli({ commands: { converge }, now: CLOCK }); const result = await cli.run(["converge", "--format", "json"]); - expect(result.exitCode).toBe(2); - expect(result.json).toEqual([]); + expect(result.exitCode).toBe(0); + expect(result.json).toHaveLength(1); + expect(result.spawns[0]?.output).toBe("diagnostic"); }); - test("an auto-selected json format falls back to human instead of failing", async () => { + test("a non-TTY caller auto-selects structured output", async () => { const cli = createTestCli({ commands: { converge }, now: CLOCK }); const result = await cli.run(["converge"]); expect(result.exitCode).toBe(0); - expect(result.json).toEqual([]); + expect(result.json).toHaveLength(1); expect(result.spawns).toHaveLength(1); + expect(result.spawns[0]?.output).toBe("diagnostic"); }); - test("help states the refusal", async () => { + test("a Composer-shaped deployment summary reaches the terminal result", async () => { + const deploy = defineCommand({ + help: { summary: "Deploys through a child" }, + maySpawn: true, + handler: async (_args, ctx) => { + await ctx.spawn({ command: "alchemy", args: ["deploy"] }); + const summary = { + app: "my-app", + nodes: [ + { + address: "https://my-app.prisma.build", + entities: [{ kind: "service", id: "web" }], + }, + ], + }; + return ok( + ctx.present( + { data: { summary } }, + { + human: () => [], + stdout: () => [], + json: () => ({ summary }), + next: () => [], + }, + ), + ); + }, + }); + const cli = createTestCli({ commands: { deploy }, now: CLOCK }); + + const result = await cli.run(["deploy"]); + + expect(result.exitCode).toBe(0); + expect(result.json.at(-1)).toMatchObject({ + kind: "result", + envelope: { + ok: true, + result: { + summary: { + nodes: [{ address: "https://my-app.prisma.build" }], + }, + }, + }, + }); + }); + + test("a TTY caller keeps the inherited terminal handoff", async () => { + const cli = createTestCli({ commands: { converge }, now: CLOCK }); + + const result = await cli.run(["converge"], { + isTty: { stdout: true, stderr: true }, + }); + + expect(result.exitCode).toBe(0); + expect(result.json).toEqual([]); + expect(result.spawns[0]?.output).toBe("inherit"); + }); + + test("help no longer claims json is unsupported", async () => { const cli = createTestCli({ commands: { converge }, now: CLOCK }); const result = await cli.run(["converge", "--help"]); - expect(`${result.stdout}${result.stderr}`).toContain( + expect(`${result.stdout}${result.stderr}`).not.toContain( "does not support --json", ); }); @@ -99,7 +162,7 @@ describe("the child's status", () => { spawnScript: () => ({ exitCode: 3, signal: null }), }); - const result = await cli.run(["converge"]); + const result = await cli.run(["converge", "--format", "human"]); expect(result.exitCode).toBe(3); expect(result.stdout).toBe(""); @@ -113,7 +176,19 @@ describe("the child's status", () => { spawnScript: () => ({ exitCode: null, signal: "SIGINT" }), }); - expect((await cli.run(["converge"])).exitCode).toBe(130); + const result = await cli.run(["converge"]); + + expect(result.exitCode).toBe(130); + expect(result.json.at(-1)).toMatchObject({ + kind: "result", + envelope: { + ok: false, + error: { + code: "CLI.CHILD_PROCESS_FAILED", + meta: { exitCode: null, signal: "SIGINT" }, + }, + }, + }); }); test("the settlement summary carries the child's code", async () => { @@ -146,7 +221,18 @@ describe("the child's status", () => { spawnScript: () => ({ exitCode: 5, signal: null }), }); - expect((await cli.run(["dev"])).exitCode).toBe(5); + const result = await cli.run(["dev"]); + + expect(result.exitCode).toBe(5); + expect(result.json.at(-1)).toMatchObject({ + envelope: { + ok: false, + error: { + code: "CLI.CHILD_PROCESS_FAILED", + meta: { exitCode: 5, signal: null }, + }, + }, + }); }); test("a launch failure is a structured error, not a crash", async () => { @@ -158,7 +244,7 @@ describe("the child's status", () => { }, }); - const result = await cli.run(["converge"]); + const result = await cli.run(["converge", "--format", "human"]); expect(result.exitCode).toBe(2); expect(result.stderr).toContain("CLI.SPAWN_FAILED"); @@ -593,7 +679,9 @@ describe("signals", () => { }, }); - const result = await cli.run(["dev"], { abort: controller.signal }); + const result = await cli.run(["dev", "--format", "human"], { + abort: controller.signal, + }); expect(result.exitCode).toBe(130); expect(result.stderr).toContain("stopped"); @@ -759,7 +847,7 @@ describe("credential injection", () => { }, }); - const result = await cli.run(["converge"]); + const result = await cli.run(["converge", "--format", "human"]); expect(result.exitCode).toBe(0); expect(seen.PRISMA_SERVICE_TOKEN).toBe(token); @@ -777,7 +865,7 @@ describe("credential injection", () => { }, }); - const result = await cli.run(["converge"]); + const result = await cli.run(["converge", "--format", "human"]); expect(result.exitCode).toBe(2); expect(result.stderr).toContain("CLI.CREDENTIALS_REQUIRED"); @@ -800,7 +888,7 @@ describe("credential injection", () => { }, }); - const result = await cli.run(["converge"]); + const result = await cli.run(["converge", "--format", "human"]); expect(result.exitCode).toBe(2); expect(result.stderr).toContain("expires too soon"); @@ -992,7 +1080,7 @@ describe("next actions on a child-status settlement", () => { spawnScript: () => ({ exitCode: 3, signal: null }), }); - const result = await cli.run(["hinting"]); + const result = await cli.run(["hinting"], { isTty: { stdout: true } }); expect(result.exitCode).toBe(3); expect(result.stdout).toBe(""); @@ -1008,7 +1096,7 @@ describe("next actions on a child-status settlement", () => { spawnScript: () => ({ exitCode: null, signal: "SIGINT" }), }); - const result = await cli.run(["hinting"]); + const result = await cli.run(["hinting"], { isTty: { stdout: true } }); // The user stopped the converge, so there is nothing to reproduce: // the abort wins over what the handler asked for. @@ -1016,6 +1104,30 @@ describe("next actions on a child-status settlement", () => { expect(result.stdout).toBe(""); expect(result.stderr).toBe(""); }); + + test("json carries reproduce guidance in the terminal error envelope", async () => { + const cli = createTestCli({ + commands: { hinting }, + now: CLOCK, + spawnScript: () => ({ exitCode: 3, signal: null }), + }); + + const result = await cli.run(["hinting", "--json"]); + + expect(result.exitCode).toBe(3); + expect(result.json.at(-1)).toMatchObject({ + envelope: { + ok: false, + error: { code: "CLI.CHILD_PROCESS_FAILED" }, + nextActions: [ + { + kind: "run-command", + command: "alchemy deploy ./entry.ts", + }, + ], + }, + }); + }); }); describe("unknown terminations are never success", () => { @@ -1068,7 +1180,7 @@ describe("a handler that abandons the spawn promise", () => { spawnScript: childEndingOnlyWhenKilled(order), }); - const result = await cli.run(["abandoning"]); + const result = await cli.run(["abandoning", "--format", "human"]); order.push("run settled"); expect(result.exitCode).toBe(1); @@ -1093,7 +1205,7 @@ describe("a handler that abandons the spawn promise", () => { spawnScript: childEndingOnlyWhenKilled(order), }); - const result = await cli.run(["throwing"]); + const result = await cli.run(["throwing", "--format", "human"]); order.push("run settled"); expect(result.exitCode).toBe(1); @@ -1123,7 +1235,7 @@ describe("a handler that abandons the spawn promise", () => { spawnScript: childEndingOnlyWhenKilled(order), }); - const result = await cli.run(["abandoning"]); + const result = await cli.run(["abandoning", "--format", "human"]); expect(result.stderr).toContain("during-child"); }); diff --git a/packages/cli-engine/tests/telemetry-run.test.ts b/packages/cli-engine/tests/telemetry-run.test.ts index 5d28b7d9..d092439e 100644 --- a/packages/cli-engine/tests/telemetry-run.test.ts +++ b/packages/cli-engine/tests/telemetry-run.test.ts @@ -122,7 +122,7 @@ const signedOut = defineCommand({ }, }); -/** Refused before its handler when the run asks for --json. */ +/** Declares the spawn capability and can still complete in json mode. */ const spawning = defineCommand({ help: { summary: "May hand the terminal to a child" }, maySpawn: true, @@ -235,11 +235,11 @@ describe("the engine reports at command start", () => { ]); }); - it("reports a run refused before its handler for asking --json of a spawning command", async () => { + it("reports a spawning command before its json-capable handler", async () => { const result = await run(makeCli(), ["app", "spawner", "--json"]); - expect(result.exitCode).not.toBe(0); - expect(order).toEqual([]); + expect(result.exitCode).toBe(0); + expect(order).toEqual(["handler"]); expect(result.telemetry.map((payload) => payload.command)).toEqual([ "app spawner", ]); diff --git a/packages/cli/src/runtime.ts b/packages/cli/src/runtime.ts index 48156e7a..00641cf1 100644 --- a/packages/cli/src/runtime.ts +++ b/packages/cli/src/runtime.ts @@ -22,7 +22,7 @@ import { } from "./auth/state-file"; import { fetchWorkspaceName } from "./auth/workspace-name"; import { runPackageManager } from "./package-manager-runner"; -import { spawnChild } from "./spawn"; +import { makeSpawnChild } from "./spawn"; export type SignalProcess = Pick; @@ -142,7 +142,7 @@ export async function assembleRuntime(proc: HostProcess): Promise { apiBaseUrl, authBaseUrl: getAuthBaseUrl(proc.env), }, - spawn: spawnChild, + spawn: makeSpawnChild({ write: (text) => proc.stderr.write(text) }), /** The engine has already decided and composed; the bin only forks * the detached sender and hands the payload over. Every failure is * swallowed inside runTelemetry. */ diff --git a/packages/cli/src/spawn.ts b/packages/cli/src/spawn.ts index fed625aa..01131391 100644 --- a/packages/cli/src/spawn.ts +++ b/packages/cli/src/spawn.ts @@ -1,27 +1,38 @@ import { spawn } from "node:child_process"; -import type { SpawnChild } from "@prisma/cli-engine"; +import type { OutputStream, SpawnChild } from "@prisma/cli-engine"; /** - * The engine's spawn seam, adapted to node:child_process. Inherited - * stdio, no `detached`, no new console: the child stays in this - * process's group (POSIX) or console (Windows), so the terminal - * delivers Ctrl-C to it natively. + * The engine's spawn seam, adapted to node:child_process. Human mode + * inherits stdio; structured mode pipes both child output streams to + * diagnostics. Neither mode detaches or opens a new console, so the child + * stays in this process's group (POSIX) or console (Windows). */ -export const spawnChild: SpawnChild = (request) => { - const child = spawn(request.command, [...request.args], { - cwd: request.cwd, - env: request.env, - stdio: "inherit", - }); - return { - ended: new Promise((resolve, reject) => { - child.on("error", reject); - child.on("close", (exitCode, signal) => { - resolve({ exitCode, signal }); +export function makeSpawnChild(diagnostics: OutputStream): SpawnChild { + return (request) => { + const structured = request.output === "diagnostic"; + const child = spawn(request.command, [...request.args], { + cwd: request.cwd, + env: request.env, + stdio: structured ? ["inherit", "pipe", "pipe"] : "inherit", + }); + if (structured) { + child.stdout?.on("data", (chunk: Buffer) => { + diagnostics.write(chunk.toString("utf8")); }); - }), - kill: (signal) => { - child.kill(signal); - }, + child.stderr?.on("data", (chunk: Buffer) => { + diagnostics.write(chunk.toString("utf8")); + }); + } + return { + ended: new Promise((resolve, reject) => { + child.on("error", reject); + child.on("close", (exitCode, signal) => { + resolve({ exitCode, signal }); + }); + }), + kill: (signal) => { + child.kill(signal); + }, + }; }; -}; +} diff --git a/packages/cli/tests/spawn-adapter.test.ts b/packages/cli/tests/spawn-adapter.test.ts index 2da5cec5..2566f873 100644 --- a/packages/cli/tests/spawn-adapter.test.ts +++ b/packages/cli/tests/spawn-adapter.test.ts @@ -4,13 +4,13 @@ * about test-local copies, so this is the test that pins the adapter * production actually wires — exit passthrough, ENOENT rejection, kill * delivery, and the spawn options the whole design rests on (inherited - * stdio, no `detached`, no new console). Runs on the Windows CI leg + * human stdio, piped structured output, no `detached`, no new console). Runs on the Windows CI leg * too, which is where the options assertion earns its keep. */ import { existsSync, mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { describe, expect, test, vi } from "vitest"; +import { beforeEach, describe, expect, test, vi } from "vitest"; const spawnOptionsSeen = vi.hoisted(() => [] as Array>); @@ -29,9 +29,19 @@ vi.mock("node:child_process", async (importOriginal) => { }; }); -import { spawnChild } from "../src/spawn"; +import { makeSpawnChild } from "../src/spawn"; const NODE = process.execPath; +let diagnosticText = ""; +const spawnChild = makeSpawnChild({ + write: (text) => { + diagnosticText += text; + }, +}); + +beforeEach(() => { + diagnosticText = ""; +}); async function waitForFile(path: string, timeoutMs = 10_000): Promise { const deadline = Date.now() + timeoutMs; @@ -51,6 +61,7 @@ describe("the shipped spawn adapter", () => { args: ["-e", "process.exit(0)"], cwd: process.cwd(), env: process.env, + output: "inherit", }); await child.ended; @@ -68,6 +79,7 @@ describe("the shipped spawn adapter", () => { args: ["-e", "process.exit(3)"], cwd: process.cwd(), env: process.env, + output: "inherit", }); await expect(child.ended).resolves.toEqual({ exitCode: 3, signal: null }); @@ -79,6 +91,7 @@ describe("the shipped spawn adapter", () => { args: [], cwd: process.cwd(), env: process.env, + output: "inherit", }); await expect(child.ended).rejects.toMatchObject({ code: "ENOENT" }); @@ -100,6 +113,7 @@ describe("the shipped spawn adapter", () => { ], cwd: process.cwd(), env: process.env, + output: "inherit", }); await waitForFile(ready); @@ -114,4 +128,22 @@ describe("the shipped spawn adapter", () => { }, 20_000, ); + + test("routes both child streams to diagnostics in structured mode", async () => { + const child = spawnChild({ + command: NODE, + args: [ + "-e", + "process.stdout.write('child-out'); process.stderr.write('child-err')", + ], + cwd: process.cwd(), + env: process.env, + output: "diagnostic", + }); + + await expect(child.ended).resolves.toEqual({ exitCode: 0, signal: null }); + expect(spawnOptionsSeen.at(-1)?.stdio).toEqual(["inherit", "pipe", "pipe"]); + expect(diagnosticText).toContain("child-out"); + expect(diagnosticText).toContain("child-err"); + }); }); From f542dfa4d94d25bdb823d52baf773aa71fbe40cf Mon Sep 17 00:00:00 2001 From: Aman Varshney Date: Fri, 14 Aug 2026 19:27:48 +0530 Subject: [PATCH 2/5] fix(cli): forward delegated output safely --- packages/cli/src/runtime.ts | 2 +- packages/cli/src/spawn.ts | 91 ++++++++++++++++++++---- packages/cli/tests/spawn-adapter.test.ts | 54 ++++++++++++++ 3 files changed, 132 insertions(+), 15 deletions(-) diff --git a/packages/cli/src/runtime.ts b/packages/cli/src/runtime.ts index 00641cf1..35bf4e80 100644 --- a/packages/cli/src/runtime.ts +++ b/packages/cli/src/runtime.ts @@ -142,7 +142,7 @@ export async function assembleRuntime(proc: HostProcess): Promise { apiBaseUrl, authBaseUrl: getAuthBaseUrl(proc.env), }, - spawn: makeSpawnChild({ write: (text) => proc.stderr.write(text) }), + spawn: makeSpawnChild(proc.stderr), /** The engine has already decided and composed; the bin only forks * the detached sender and hands the payload over. Every failure is * swallowed inside runTelemetry. */ diff --git a/packages/cli/src/spawn.ts b/packages/cli/src/spawn.ts index 01131391..6da35153 100644 --- a/packages/cli/src/spawn.ts +++ b/packages/cli/src/spawn.ts @@ -1,5 +1,16 @@ import { spawn } from "node:child_process"; -import type { OutputStream, SpawnChild } from "@prisma/cli-engine"; +import { type Readable, Writable } from "node:stream"; +import { pipeline } from "node:stream/promises"; +import type { ChildResult, SpawnChild } from "@prisma/cli-engine"; + +interface DiagnosticStream { + write(text: string): unknown; + once?(event: "drain", listener: () => void): unknown; +} + +type ForwardingResult = + | { readonly ok: true } + | { readonly ok: false; readonly cause: unknown }; /** * The engine's spawn seam, adapted to node:child_process. Human mode @@ -7,7 +18,7 @@ import type { OutputStream, SpawnChild } from "@prisma/cli-engine"; * diagnostics. Neither mode detaches or opens a new console, so the child * stays in this process's group (POSIX) or console (Windows). */ -export function makeSpawnChild(diagnostics: OutputStream): SpawnChild { +export function makeSpawnChild(diagnostics: DiagnosticStream): SpawnChild { return (request) => { const structured = request.output === "diagnostic"; const child = spawn(request.command, [...request.args], { @@ -15,20 +26,23 @@ export function makeSpawnChild(diagnostics: OutputStream): SpawnChild { env: request.env, stdio: structured ? ["inherit", "pipe", "pipe"] : "inherit", }); - if (structured) { - child.stdout?.on("data", (chunk: Buffer) => { - diagnostics.write(chunk.toString("utf8")); + const forwarding = forwardStructuredOutput( + structured, + child.stdout, + child.stderr, + diagnostics, + ); + const processEnded = new Promise((resolve, reject) => { + child.on("error", reject); + child.on("close", (exitCode, signal) => { + resolve({ exitCode, signal }); }); - child.stderr?.on("data", (chunk: Buffer) => { - diagnostics.write(chunk.toString("utf8")); - }); - } + }); return { - ended: new Promise((resolve, reject) => { - child.on("error", reject); - child.on("close", (exitCode, signal) => { - resolve({ exitCode, signal }); - }); + ended: processEnded.then(async (result) => { + const output = await forwarding; + if (!output.ok) throw output.cause; + return result; }), kill: (signal) => { child.kill(signal); @@ -36,3 +50,52 @@ export function makeSpawnChild(diagnostics: OutputStream): SpawnChild { }; }; } + +function forwardStructuredOutput( + structured: boolean, + stdout: Readable | null, + stderr: Readable | null, + diagnostics: DiagnosticStream, +): Promise { + if (!structured) return Promise.resolve({ ok: true }); + if (stdout === null || stderr === null) { + return Promise.resolve({ + ok: false, + cause: new Error("structured child output streams were not piped"), + }); + } + return Promise.all([ + forwardOutput(stdout, diagnostics), + forwardOutput(stderr, diagnostics), + ]).then( + (): ForwardingResult => ({ ok: true }), + (cause: unknown): ForwardingResult => ({ ok: false, cause }), + ); +} + +/** Decode each child stream continuously and stop reading while the + * diagnostic destination applies backpressure. The child status does not + * settle until both streams have fully drained. */ +function forwardOutput( + source: Readable, + diagnostics: DiagnosticStream, +): Promise { + const destination = new Writable({ + decodeStrings: false, + write: (text: string, _encoding, done) => { + try { + if ( + diagnostics.write(text) === false && + diagnostics.once !== undefined + ) { + diagnostics.once("drain", () => done()); + } else { + done(); + } + } catch (cause) { + done(cause instanceof Error ? cause : new Error(String(cause))); + } + }, + }); + return pipeline(source.setEncoding("utf8"), destination); +} diff --git a/packages/cli/tests/spawn-adapter.test.ts b/packages/cli/tests/spawn-adapter.test.ts index 2566f873..acd77632 100644 --- a/packages/cli/tests/spawn-adapter.test.ts +++ b/packages/cli/tests/spawn-adapter.test.ts @@ -146,4 +146,58 @@ describe("the shipped spawn adapter", () => { expect(diagnosticText).toContain("child-out"); expect(diagnosticText).toContain("child-err"); }); + + test("preserves a UTF-8 character split across child output chunks", async () => { + const child = spawnChild({ + command: NODE, + args: [ + "-e", + "process.stdout.write(Buffer.from([0xf0, 0x9f])); setTimeout(() => process.stdout.write(Buffer.from([0x98, 0x80])), 50)", + ], + cwd: process.cwd(), + env: process.env, + output: "diagnostic", + }); + + await expect(child.ended).resolves.toEqual({ exitCode: 0, signal: null }); + expect(diagnosticText).toBe("😀"); + }); + + test("waits for diagnostic backpressure to drain before settling", async () => { + let drainListener: (() => void) | undefined; + let firstWrite = true; + let forwarded = ""; + const backpressuredSpawn = makeSpawnChild({ + write: (text) => { + forwarded += text; + if (!firstWrite) return true; + firstWrite = false; + return false; + }, + once: (event, listener) => { + expect(event).toBe("drain"); + drainListener = listener; + }, + }); + const child = backpressuredSpawn({ + command: NODE, + args: ["-e", "process.stdout.write('held-output')"], + cwd: process.cwd(), + env: process.env, + output: "diagnostic", + }); + let settled = false; + const ended = child.ended.then((result) => { + settled = true; + return result; + }); + + await vi.waitFor(() => expect(drainListener).toBeDefined()); + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(settled).toBe(false); + drainListener?.(); + + await expect(ended).resolves.toEqual({ exitCode: 0, signal: null }); + expect(forwarded).toBe("held-output"); + }); }); From 83e6c89e1bb88ba859088d4943f374245511e7b6 Mon Sep 17 00:00:00 2001 From: willbot Date: Mon, 17 Aug 2026 12:10:43 +0200 Subject: [PATCH 3/5] fix(engine): harden structured delegated output against review findings Apply the seven code-review findings on structured delegated output: - Stream NDJSON frames live in json mode: the delegated-terminal buffer (and its 1000-event cap) now applies only in human mode, where the child actually owns the terminal. - Never reject the child's status for a relay failure: the adapter's ended promise settles from the process exit event and forwarding is best-effort, so a dead diagnostic sink cannot turn a completed run into CLI.SPAWN_FAILED. - Complete a backpressured relay write on 'error'/'close' as well as 'drain', so an EPIPE'd stderr fails the relay instead of crashing the CLI unsettled or stalling it forever. - Bound the post-exit pipe drain with a grace period and destroy the pipes when it lapses, so a grandchild holding the inherited pipes cannot block settlement. - State the adapter's obligation on SpawnRequest.output: ignoring "diagnostic" silently corrupts framed stdout, and pre-existing adapters must be updated. - Render an unknown child termination as 'exited with code unknown', and pin the summary in the json envelope test. - Settle the structured child status through settleVerbatimExitCode like the sibling human path. Co-Authored-By: Claude Fable 5 Signed-off-by: willbot Signed-off-by: Will Madden --- .../cli-engine/src/execution/reporting.ts | 8 +- .../cli-engine/src/execution/settlement.ts | 4 +- packages/cli-engine/src/spawn.ts | 9 +- packages/cli-engine/tests/spawn.test.ts | 63 +++++++++- packages/cli/src/spawn.ts | 118 +++++++++++++----- packages/cli/tests/spawn-adapter.test.ts | 81 +++++++++++- 6 files changed, 235 insertions(+), 48 deletions(-) diff --git a/packages/cli-engine/src/execution/reporting.ts b/packages/cli-engine/src/execution/reporting.ts index 43d9f562..da68d706 100644 --- a/packages/cli-engine/src/execution/reporting.ts +++ b/packages/cli-engine/src/execution/reporting.ts @@ -44,10 +44,12 @@ export function reportEvent(invocation: Invocation, event: EngineEvent): void { reportAfterResolution(invocation); return; } - // No engine write may interleave with a delegated terminal's child - // output; the buffer is flushed in order when the child ends. + // In human mode the child owns the terminal, so no engine write may + // interleave with its output; the buffer is flushed in order when the + // child ends. In json mode the engine keeps stdout (the child is + // routed to diagnostic stderr), so frames stream live. const terminal = state.delegatedTerminal; - if (terminal !== undefined) { + if (terminal !== undefined && state.format !== "json") { if (terminal.buffered.length >= SPAWN_COMMENTARY_BUFFER_CAP) { terminal.dropped += 1; return; diff --git a/packages/cli-engine/src/execution/settlement.ts b/packages/cli-engine/src/execution/settlement.ts index e18bbf65..76804f8e 100644 --- a/packages/cli-engine/src/execution/settlement.ts +++ b/packages/cli-engine/src/execution/settlement.ts @@ -255,7 +255,7 @@ function settleStructuredChildStatus( child: { readonly exitCode: number | null; readonly signal: string | null }, exitCode: number, ): void { - invocation.state.settledExitCode = exitCode; + settleVerbatimExitCode(invocation, exitCode); const nextActions = child.signal === null ? settlement.nextActions : []; if (exitCode === 0) { const envelope: CompletedEnvelope = { @@ -276,7 +276,7 @@ function settleStructuredChildStatus( } const how = child.signal === null - ? `exited with code ${String(child.exitCode)}` + ? `exited with code ${String(child.exitCode ?? "unknown")}` : `was terminated by ${child.signal}`; emitErrored(invocation, { ok: false, diff --git a/packages/cli-engine/src/spawn.ts b/packages/cli-engine/src/spawn.ts index 2f21b790..bd36f396 100644 --- a/packages/cli-engine/src/spawn.ts +++ b/packages/cli-engine/src/spawn.ts @@ -13,8 +13,13 @@ export interface SpawnRequest { readonly args: readonly string[]; readonly cwd: string; readonly env: Readonly>; - /** Human mode hands the terminal over unchanged. Structured mode keeps - * stdout framed and routes the child's human output to diagnostics. */ + /** "inherit" hands the terminal over unchanged. "diagnostic" MUST + * route the child's stdout and stderr away from this process's + * stdout (to the host's diagnostic stream): the engine emits framed + * NDJSON on stdout in that mode and cannot detect an adapter that + * ignores this field — inherited child stdout would silently corrupt + * the stream. Adapters written before this field existed must be + * updated. */ readonly output: "inherit" | "diagnostic"; } diff --git a/packages/cli-engine/tests/spawn.test.ts b/packages/cli-engine/tests/spawn.test.ts index 51045000..b88b14ba 100644 --- a/packages/cli-engine/tests/spawn.test.ts +++ b/packages/cli-engine/tests/spawn.test.ts @@ -554,7 +554,7 @@ describe("output while a child owns the terminal", () => { }, }); - await cli.run(["chatty"], { + await cli.run(["chatty", "--format", "human"], { onEvent: (event) => { if (event.kind === "message") { events.push(event.text); @@ -566,6 +566,52 @@ describe("output while a child owns the terminal", () => { expect(events).toEqual(["during-1", "during-2", "after"]); }); + test("json frames stream live while the child runs", async () => { + let eventsAtChildExit = -1; + const events: string[] = []; + const chatty = defineCommand({ + help: { summary: "Reports while the child runs" }, + maySpawn: true, + handler: async (_args, ctx) => { + const child = ctx.spawn({ command: "alchemy" }); + ctx.report({ kind: "message", severity: "info", text: "during-1" }); + ctx.report({ kind: "message", severity: "info", text: "during-2" }); + await child; + return ok( + ctx.present( + { data: null }, + { + human: () => [], + stdout: () => [], + json: () => null, + next: () => [], + }, + ), + ); + }, + }); + const cli = createTestCli({ + commands: { chatty }, + now: CLOCK, + spawnScript: () => { + eventsAtChildExit = events.length; + return { exitCode: 0, signal: null }; + }, + }); + + const result = await cli.run(["chatty", "--json"], { + onEvent: (event) => { + if (event.kind === "message") { + events.push(event.text); + } + }, + }); + + expect(result.exitCode).toBe(0); + expect(eventsAtChildExit).toBe(2); + expect(events).toEqual(["during-1", "during-2"]); + }); + test("ctx.present while a child is live is a construction error", async () => { const presenting = defineCommand({ help: { summary: "Presents mid-child" }, @@ -1138,7 +1184,18 @@ describe("unknown terminations are never success", () => { spawnScript: () => ({ exitCode: null, signal: null }), }); - expect((await cli.run(["converge"])).exitCode).toBe(1); + const result = await cli.run(["converge"]); + + expect(result.exitCode).toBe(1); + expect(result.json.at(-1)).toMatchObject({ + envelope: { + ok: false, + error: { + code: "CLI.CHILD_PROCESS_FAILED", + summary: "The delegated process exited with code unknown.", + }, + }, + }); }); test("a signal outside the portable table settles 1, not 128", async () => { @@ -1345,7 +1402,7 @@ describe("the commentary buffer is bounded", () => { const messages: string[] = []; const cli = createTestCli({ commands: { chatty }, now: CLOCK }); - const result = await cli.run(["chatty"], { + const result = await cli.run(["chatty", "--format", "human"], { onEvent: (event) => { if (event.kind === "message") { messages.push(event.text); diff --git a/packages/cli/src/spawn.ts b/packages/cli/src/spawn.ts index 6da35153..297a08eb 100644 --- a/packages/cli/src/spawn.ts +++ b/packages/cli/src/spawn.ts @@ -5,20 +5,37 @@ import type { ChildResult, SpawnChild } from "@prisma/cli-engine"; interface DiagnosticStream { write(text: string): unknown; - once?(event: "drain", listener: () => void): unknown; + once?( + event: "drain" | "error" | "close", + listener: (cause?: unknown) => void, + ): unknown; } -type ForwardingResult = - | { readonly ok: true } - | { readonly ok: false; readonly cause: unknown }; +/** How long after the child exits the relay keeps reading its pipes. A + * grandchild that inherited them can hold EOF back forever; settlement + * must not wait on it, so the pipes are destroyed after this grace. */ +const POST_EXIT_DRAIN_GRACE_MS = 5_000; + +export interface SpawnChildOptions { + readonly drainGraceMs?: number; +} /** * The engine's spawn seam, adapted to node:child_process. Human mode * inherits stdio; structured mode pipes both child output streams to * diagnostics. Neither mode detaches or opens a new console, so the child * stays in this process's group (POSIX) or console (Windows). + * + * The child's own status settles the run: `ended` resolves from the + * process `exit` event, waits for the diagnostic relay only up to the + * drain grace, and never rejects for a relay failure — rejection is + * reserved for a child that could not be launched at all. */ -export function makeSpawnChild(diagnostics: DiagnosticStream): SpawnChild { +export function makeSpawnChild( + diagnostics: DiagnosticStream, + options?: SpawnChildOptions, +): SpawnChild { + const drainGraceMs = options?.drainGraceMs ?? POST_EXIT_DRAIN_GRACE_MS; return (request) => { const structured = request.output === "diagnostic"; const child = spawn(request.command, [...request.args], { @@ -26,22 +43,33 @@ export function makeSpawnChild(diagnostics: DiagnosticStream): SpawnChild { env: request.env, stdio: structured ? ["inherit", "pipe", "pipe"] : "inherit", }); - const forwarding = forwardStructuredOutput( - structured, - child.stdout, - child.stderr, - diagnostics, - ); const processEnded = new Promise((resolve, reject) => { child.on("error", reject); - child.on("close", (exitCode, signal) => { + child.on("exit", (exitCode, signal) => { resolve({ exitCode, signal }); }); }); + if (!structured) { + return { + ended: processEnded, + kill: (signal) => { + child.kill(signal); + }, + }; + } + const forwarding = forwardStructuredOutput( + child.stdout, + child.stderr, + diagnostics, + ); return { ended: processEnded.then(async (result) => { - const output = await forwarding; - if (!output.ok) throw output.cause; + const drainDeadline = setTimeout(() => { + child.stdout?.destroy(); + child.stderr?.destroy(); + }, drainGraceMs); + await forwarding; + clearTimeout(drainDeadline); return result; }), kill: (signal) => { @@ -51,51 +79,75 @@ export function makeSpawnChild(diagnostics: DiagnosticStream): SpawnChild { }; } +/** Best-effort relay: a forwarding failure never rejects, so the child's + * real status still settles the run when the diagnostic sink dies. */ function forwardStructuredOutput( - structured: boolean, stdout: Readable | null, stderr: Readable | null, diagnostics: DiagnosticStream, -): Promise { - if (!structured) return Promise.resolve({ ok: true }); - if (stdout === null || stderr === null) { - return Promise.resolve({ - ok: false, - cause: new Error("structured child output streams were not piped"), - }); - } - return Promise.all([ - forwardOutput(stdout, diagnostics), - forwardOutput(stderr, diagnostics), - ]).then( - (): ForwardingResult => ({ ok: true }), - (cause: unknown): ForwardingResult => ({ ok: false, cause }), +): Promise { + const sources = [stdout, stderr].filter( + (source): source is Readable => source !== null, + ); + return Promise.all( + sources.map((source) => forwardOutput(source, diagnostics)), + ).then( + () => undefined, + () => undefined, ); } /** Decode each child stream continuously and stop reading while the - * diagnostic destination applies backpressure. The child status does not - * settle until both streams have fully drained. */ + * diagnostic destination applies backpressure. A destination that + * errors or closes instead of draining fails the relay rather than + * stalling it. */ function forwardOutput( source: Readable, diagnostics: DiagnosticStream, ): Promise { + let pendingDone: ((cause?: Error) => void) | undefined; + let failure: Error | undefined; + const fail = (cause: Error) => { + failure ??= cause; + const done = pendingDone; + pendingDone = undefined; + done?.(cause); + }; + diagnostics.once?.("error", (cause) => { + fail(toError(cause)); + }); + diagnostics.once?.("close", () => { + fail(new Error("the diagnostic stream closed during child output")); + }); const destination = new Writable({ decodeStrings: false, write: (text: string, _encoding, done) => { + if (failure !== undefined) { + done(failure); + return; + } try { if ( diagnostics.write(text) === false && diagnostics.once !== undefined ) { - diagnostics.once("drain", () => done()); + pendingDone = done; + diagnostics.once("drain", () => { + if (pendingDone !== done) return; + pendingDone = undefined; + done(); + }); } else { done(); } } catch (cause) { - done(cause instanceof Error ? cause : new Error(String(cause))); + done(toError(cause)); } }, }); return pipeline(source.setEncoding("utf8"), destination); } + +function toError(cause: unknown): Error { + return cause instanceof Error ? cause : new Error(String(cause)); +} diff --git a/packages/cli/tests/spawn-adapter.test.ts b/packages/cli/tests/spawn-adapter.test.ts index acd77632..4a6349f0 100644 --- a/packages/cli/tests/spawn-adapter.test.ts +++ b/packages/cli/tests/spawn-adapter.test.ts @@ -164,7 +164,8 @@ describe("the shipped spawn adapter", () => { }); test("waits for diagnostic backpressure to drain before settling", async () => { - let drainListener: (() => void) | undefined; + const listeners: Record void) | undefined> = + {}; let firstWrite = true; let forwarded = ""; const backpressuredSpawn = makeSpawnChild({ @@ -175,8 +176,7 @@ describe("the shipped spawn adapter", () => { return false; }, once: (event, listener) => { - expect(event).toBe("drain"); - drainListener = listener; + listeners[event] = listener; }, }); const child = backpressuredSpawn({ @@ -192,12 +192,83 @@ describe("the shipped spawn adapter", () => { return result; }); - await vi.waitFor(() => expect(drainListener).toBeDefined()); + await vi.waitFor(() => expect(listeners.drain).toBeDefined()); await new Promise((resolve) => setTimeout(resolve, 10)); expect(settled).toBe(false); - drainListener?.(); + listeners.drain?.(); await expect(ended).resolves.toEqual({ exitCode: 0, signal: null }); expect(forwarded).toBe("held-output"); }); + + test("a diagnostic sink that errors instead of draining still settles with the child's status", async () => { + const listeners: Record void>> = {}; + const failingSpawn = makeSpawnChild({ + write: () => false, + once: (event, listener) => { + listeners[event] ??= []; + listeners[event].push(listener); + }, + }); + const child = failingSpawn({ + command: NODE, + args: ["-e", "process.stdout.write('doomed-output'); process.exit(7)"], + cwd: process.cwd(), + env: process.env, + output: "diagnostic", + }); + + await vi.waitFor(() => + expect(listeners.drain?.length ?? 0).toBeGreaterThan(0), + ); + for (const listener of listeners.error ?? []) { + listener(new Error("EPIPE")); + } + + await expect(child.ended).resolves.toEqual({ exitCode: 7, signal: null }); + }); + + test("a throwing diagnostic sink does not reject the child's status", async () => { + const throwingSpawn = makeSpawnChild({ + write: () => { + throw new Error("sink is gone"); + }, + }); + const child = throwingSpawn({ + command: NODE, + args: ["-e", "process.stdout.write('lost'); process.exit(3)"], + cwd: process.cwd(), + env: process.env, + output: "diagnostic", + }); + + await expect(child.ended).resolves.toEqual({ exitCode: 3, signal: null }); + }); + + test.skipIf(process.platform === "win32")( + "a grandchild holding the pipes does not block settlement past the drain grace", + async () => { + const gracedSpawn = makeSpawnChild( + { + write: (text) => { + diagnosticText += text; + }, + }, + { drainGraceMs: 200 }, + ); + const child = gracedSpawn({ + command: NODE, + args: [ + "-e", + `require("node:child_process").spawn(process.execPath, ["-e", "setTimeout(() => {}, 10000)"], { detached: true, stdio: ["ignore", "inherit", "ignore"] }).unref();`, + ], + cwd: process.cwd(), + env: process.env, + output: "diagnostic", + }); + + await expect(child.ended).resolves.toEqual({ exitCode: 0, signal: null }); + }, + 20_000, + ); }); From 469a913fc23febcfe8c742825ff3fe43680785ba Mon Sep 17 00:00:00 2001 From: willbot Date: Mon, 17 Aug 2026 12:22:05 +0200 Subject: [PATCH 4/5] fix(cli): release diagnostic listeners and pin the drain grace Address the two open review threads: - Remove each relay's error/close (and any armed drain) listener from the shared diagnostic stream when its pipeline settles, so sequential structured children do not accumulate listeners on process.stderr past Node's default limit. DiagnosticStream gains an optional off, and a test pins that every registered listener is removed. - Assert the grandchild test settles materially below the 5s default grace, so an adapter that ignored drainGraceMs would fail the test rather than pass on the Vitest timeout. Co-Authored-By: Claude Fable 5 Signed-off-by: willbot Signed-off-by: Will Madden --- packages/cli/src/spawn.ts | 31 +++++++++++++---- packages/cli/tests/spawn-adapter.test.ts | 44 ++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/spawn.ts b/packages/cli/src/spawn.ts index 297a08eb..095f17f7 100644 --- a/packages/cli/src/spawn.ts +++ b/packages/cli/src/spawn.ts @@ -9,6 +9,10 @@ interface DiagnosticStream { event: "drain" | "error" | "close", listener: (cause?: unknown) => void, ): unknown; + off?( + event: "drain" | "error" | "close", + listener: (cause?: unknown) => void, + ): unknown; } /** How long after the child exits the relay keeps reading its pipes. A @@ -106,6 +110,7 @@ function forwardOutput( diagnostics: DiagnosticStream, ): Promise { let pendingDone: ((cause?: Error) => void) | undefined; + let pendingDrain: ((cause?: unknown) => void) | undefined; let failure: Error | undefined; const fail = (cause: Error) => { failure ??= cause; @@ -113,12 +118,14 @@ function forwardOutput( pendingDone = undefined; done?.(cause); }; - diagnostics.once?.("error", (cause) => { + const onSinkError = (cause?: unknown) => { fail(toError(cause)); - }); - diagnostics.once?.("close", () => { + }; + const onSinkClose = () => { fail(new Error("the diagnostic stream closed during child output")); - }); + }; + diagnostics.once?.("error", onSinkError); + diagnostics.once?.("close", onSinkClose); const destination = new Writable({ decodeStrings: false, write: (text: string, _encoding, done) => { @@ -132,11 +139,14 @@ function forwardOutput( diagnostics.once !== undefined ) { pendingDone = done; - diagnostics.once("drain", () => { + const onDrain = () => { + pendingDrain = undefined; if (pendingDone !== done) return; pendingDone = undefined; done(); - }); + }; + pendingDrain = onDrain; + diagnostics.once("drain", onDrain); } else { done(); } @@ -145,7 +155,14 @@ function forwardOutput( } }, }); - return pipeline(source.setEncoding("utf8"), destination); + // The diagnostic stream outlives this relay (it is the process's own + // stderr), so every listener registered here is removed when the + // pipeline settles — sequential children must not accumulate them. + return pipeline(source.setEncoding("utf8"), destination).finally(() => { + diagnostics.off?.("error", onSinkError); + diagnostics.off?.("close", onSinkClose); + if (pendingDrain !== undefined) diagnostics.off?.("drain", pendingDrain); + }); } function toError(cause: unknown): Error { diff --git a/packages/cli/tests/spawn-adapter.test.ts b/packages/cli/tests/spawn-adapter.test.ts index 4a6349f0..a1273d93 100644 --- a/packages/cli/tests/spawn-adapter.test.ts +++ b/packages/cli/tests/spawn-adapter.test.ts @@ -256,6 +256,7 @@ describe("the shipped spawn adapter", () => { }, { drainGraceMs: 200 }, ); + const startedAt = Date.now(); const child = gracedSpawn({ command: NODE, args: [ @@ -268,7 +269,50 @@ describe("the shipped spawn adapter", () => { }); await expect(child.ended).resolves.toEqual({ exitCode: 0, signal: null }); + // Materially below the 5s default grace: proves the configured + // 200ms grace was honored, not merely eventual completion. + expect(Date.now() - startedAt).toBeLessThan(3_000); }, 20_000, ); + + test("removes its diagnostic listeners once forwarding ends", async () => { + const registered: Array<{ + event: string; + listener: (cause?: unknown) => void; + }> = []; + const removed: Array<{ + event: string; + listener: (cause?: unknown) => void; + }> = []; + const trackingSpawn = makeSpawnChild({ + write: () => true, + once: (event, listener) => { + registered.push({ event, listener }); + }, + off: (event, listener) => { + removed.push({ event, listener }); + }, + }); + const child = trackingSpawn({ + command: NODE, + args: ["-e", "process.stdout.write('bye')"], + cwd: process.cwd(), + env: process.env, + output: "diagnostic", + }); + + await expect(child.ended).resolves.toEqual({ exitCode: 0, signal: null }); + expect(registered.length).toBeGreaterThan(0); + expect(removed.length).toBe(registered.length); + for (const entry of registered) { + expect( + removed.some( + (candidate) => + candidate.event === entry.event && + candidate.listener === entry.listener, + ), + ).toBe(true); + } + }); }); From 4cf0b2c279f6337cca986e0aecd9e35d310f411d Mon Sep 17 00:00:00 2001 From: willbot Date: Mon, 17 Aug 2026 12:34:22 +0200 Subject: [PATCH 5/5] test(engine): pin the delegated-auth refusal tests to human mode Merge main (PR 183's delegated credential refresh) and resolve the semantic collision with structured delegated output: the four auth tests that assert refusal text on stderr now run with --format human, since the harness's non-TTY default is now json and structured errors land in the result frame instead. Co-Authored-By: Claude Fable 5 Signed-off-by: willbot Signed-off-by: Will Madden --- packages/cli-engine/tests/spawn.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/cli-engine/tests/spawn.test.ts b/packages/cli-engine/tests/spawn.test.ts index 5bc42750..2430a362 100644 --- a/packages/cli-engine/tests/spawn.test.ts +++ b/packages/cli-engine/tests/spawn.test.ts @@ -900,7 +900,7 @@ describe("credential injection", () => { }, }); - const result = await cli.run(["converge"], { + const result = await cli.run(["converge", "--format", "human"], { onEvent: (event) => { if (event.kind === "status") { cli.credentialManager.overwriteStoredState({ @@ -1031,7 +1031,7 @@ describe("credential injection", () => { }, }); - const result = await cli.run(["converge"]); + const result = await cli.run(["converge", "--format", "human"]); expect(result.exitCode).toBe(2); expect(result.stderr).toContain("expires too soon"); @@ -1134,7 +1134,7 @@ describe("credential injection", () => { refreshCredential: async () => ({ kind: "invalid" }), }); - const result = await cli.run(["converge"]); + const result = await cli.run(["converge", "--format", "human"]); expect(result.exitCode).toBe(2); expect(result.stderr).toContain("Your session has expired"); @@ -1157,7 +1157,7 @@ describe("credential injection", () => { }, }); - const result = await cli.run(["converge"]); + const result = await cli.run(["converge", "--format", "human"]); expect(result.exitCode).toBe(2); expect(result.stderr).toContain("CLI.AUTH_SERVICE_ERROR");