fix(cli): flush NDJSON before forced exit#95
Merged
Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| return state.queue | ||
| } | ||
|
|
||
| export async function flush() { |
There was a problem hiding this comment.
🟠 Signal exits bypass flush(); NDJSON still truncates.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #95, packages/cli/src/cli/stdout.ts:39-42):
Problem: Signal exits bypass flush(); NDJSON still truncates
Detail: `Stdout.flush()` is invoked only from the `try/finally` blocks of `index.ts` and `headless.ts`. SIGINT / SIGTERM / SIGHUP terminate the Node process abruptly — `finally` does not run for signal-induced exits — so any writes still chained in `state.queue` when a signal arrives are dropped. The PR diff contains no `SIGINT`/`SIGTERM`/`SIGHUP`/`beforeExit` handler that would drain the queue, so for the common case of `aictrl run --format=json | jq …` killed via Ctrl+C, or a supervisor sending SIGTERM, the lifecycle/error NDJSON events still sitting in `state.queue` are lost — the same truncation issue #91 was opened for. The PR statement ("fully flushed before process.exit() runs, even under pipe backpressure or EPIPE") is satisfied only for the happy path; forced exits via signal regress to pre-fix behavior. If signal handlers exist elsewhere in the codebase, this is moot, but the diff itself does not register any.
Suggested fix: In the Stdout module, register `process.once(sig, async () => { try { await flush() } finally { process.exit(sig === 'SIGINT' ? 130 : 143) } })` for SIGINT/SIGTERM/SIGHUP, and/or drain in a `process.on('beforeExit')` handler. Confirm whether the rest of the codebase already registers signal handlers that route through `process.exit()` (in which case finally covers them).
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Stdout.flush() is invoked only from the try/finally blocks of index.ts and headless.ts. SIGINT / SIGTERM / SIGHUP terminate the Node process abruptly — finally does not run for signal-induced exits — so any writes still chained in state.queue when a signal arrives are dropped. The PR diff contains no SIGINT/SIGTERM/SIGHUP/beforeExit handler that would drain the queue, so for the common case of aictrl run --format=json | jq … killed via Ctrl+C, or a supervisor sending SIGTERM, the lifecycle/error NDJSON events still sitting in state.queue are lost — the same truncation issue #91 was opened for. The PR statement ("fully flushed before process.exit() runs, even under pipe backpressure or EPIPE") is satisfied only for the happy path; forced exits via signal regress to pre-fix behavior. If signal handlers exist elsewhere in the codebase, this is moot, but the diff itself does not register any.
export namespace Stdout {
export function write(chunk: string) {
state.queue = state.queue.then(() => output(chunk))
return state.queue
}
export async function flush() {
await state.queue
await output("", true)
}
}| }) | ||
| } | ||
|
|
||
| process.stdout.on("error", (error) => { |
There was a problem hiding this comment.
🟠 Global stdout error handler installed at import.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #95, packages/cli/src/cli/stdout.ts:25-31):
Problem: Global stdout error handler installed at import
Detail: `process.stdout.on("error", ...)` is registered at module load as an import side-effect. Because `Stdout` is imported by `run.ts`, `headless.ts`, AND `index.ts`, the listener attaches once but governs ALL stdout writes process-wide — including interactive / text / non-JSON modes that never go through `Stdout`. This silently changes error semantics for unrelated code paths: EPIPE on stdout is silently swallowed everywhere (state.closed = true), and non-EPIPE errors are re-thrown from inside an EventEmitter error handler, which surfaces as an uncaughtException from an unpredictable call site rather than the default stream-error path. Neighbouring utility modules in this codebase (e.g. `Locale`, `Database`) are pure exports without global hook registration at import. Recommend scoping the handler to the JSON/NDJSON path — either via an explicit `Stdout.init()` called only from the JSON-format code path, or by attaching lazily on first `write()` guarded by a `state.bound` flag.
Suggested fix: Remove the module-level `process.stdout.on("error", ...)`. Attach it lazily on first `write()` (guarded by a `state.bound` flag) so only processes that actually use `Stdout` get the global handler, leaving interactive / text mode untouched.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
process.stdout.on("error", ...) is registered at module load as an import side-effect. Because Stdout is imported by run.ts, headless.ts, AND index.ts, the listener attaches once but governs ALL stdout writes process-wide — including interactive / text / non-JSON modes that never go through Stdout. This silently changes error semantics for unrelated code paths: EPIPE on stdout is silently swallowed everywhere (state.closed = true), and non-EPIPE errors are re-thrown from inside an EventEmitter error handler, which surfaces as an uncaughtException from an unpredictable call site rather than the default stream-error path. Neighbouring utility modules in this codebase (e.g. Locale, Database) are pure exports without global hook registration at import. Recommend scoping the handler to the JSON/NDJSON path — either via an explicit Stdout.init() called only from the JSON-format code path, or by attaching lazily on first write() guarded by a state.bound flag.
process.stdout.on("error", (error) => {
if (pipe(error)) {
state.closed = true
return
}
throw error
})| return new Promise<void>((resolve) => { | ||
| const drain = () => resolve() | ||
| const wait = { ready: true, done: false } | ||
| wait.ready = process.stdout.write(chunk, (error) => { |
There was a problem hiding this comment.
🟡 Inconsistent error handling within stdout.ts.
Suggested change
| wait.ready = process.stdout.write(chunk, (error) => { | |
| In the write callback, reject the Promise on non-EPIPE errors (`if (error && !pipe(error)) reject(error); else { state.closed = true; resolve() }`) so the failure style matches the global handler's throw-on-non-EPIPE intent, and so callers of `flush()` can distinguish success from silent close. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #95, packages/cli/src/cli/stdout.ts:15-31):
Problem: Inconsistent error handling within stdout.ts
Detail: Within the same module, errors on the same stream are handled two different ways. The `process.stdout.write` callback inside `output()` swallows ALL errors (`if (error) state.closed = true; resolve()`) — never rejecting the Promise. The module-level `error` listener, by contrast, re-throws non-EPIPE errors. Both code paths field the same class of failures on the same stream, so callers cannot tell a successful flush from a silently-closed pipe; the only path that surfaces a failure is the global listener's `throw`, and only for non-EPIPE. Pick one style: either propagate via Promise rejection (so callers can react), or swallow uniformly.
Suggested fix: In the write callback, reject the Promise on non-EPIPE errors (`if (error && !pipe(error)) reject(error); else { state.closed = true; resolve() }`) so the failure style matches the global handler's throw-on-non-EPIPE intent, and so callers of `flush()` can distinguish success from silent close.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Within the same module, errors on the same stream are handled two different ways. The process.stdout.write callback inside output() swallows ALL errors (if (error) state.closed = true; resolve()) — never rejecting the Promise. The module-level error listener, by contrast, re-throws non-EPIPE errors. Both code paths field the same class of failures on the same stream, so callers cannot tell a successful flush from a silently-closed pipe; the only path that surfaces a failure is the global listener's throw, and only for non-EPIPE. Pick one style: either propagate via Promise rejection (so callers can react), or swallow uniformly.
wait.ready = process.stdout.write(chunk, (error) => {
wait.done = true
if (error) state.closed = true
if (!wait.ready) process.stdout.off("drain", drain)
resolve()
})| return error instanceof Error && "code" in error && error.code === "EPIPE" | ||
| } | ||
|
|
||
| function output(chunk: string, flush = false) { |
There was a problem hiding this comment.
🟡 flush flag is dead code; trailing empty write is a no-op.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #95, packages/cli/src/cli/stdout.ts:10-42):
Problem: flush flag is dead code; trailing empty write is a no-op
Detail: Two related dead-code issues in `flush()`/`output()`: (1) The `flush` parameter on `output(chunk, flush = false)` exists solely to skip registering a `drain` listener under backpressure, but the only call site that passes `flush=true` is `flush()` → `output("", true)`, and an empty-string chunk never triggers backpressure on `process.stdout.write`, so `wait.ready` is always `true` and the `!flush` branch is never reached differently. The flag is effectively dead at its current call site. (2) The trailing `await output("", true)` in `flush()` adds no drain guarantee beyond what `await state.queue` already provides: per Node's Writable contract, an empty-string write is a no-op at the OS level and its callback fires regardless of buffer state. The construct is misleading — future readers will assume the trailing empty write is what guarantees OS-level flush and "fix" it incorrectly. `await state.queue` already awaits every prior write's per-chunk callback (which fires after libuv hands the bytes to the OS), so the kernel pipe buffer already holds everything before `flush()` returns.
Suggested fix: Drop the `flush` parameter (`function output(chunk: string) { ... }`) and drop the trailing `await output("", true)`. Rely solely on `await state.queue`. If a final-yield semantic is desired, replace it with an explicit `await new Promise<void>(r => process.stdout.write("", () => r()))` with a comment noting it only yields one tick.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Two related dead-code issues in flush()/output(): (1) The flush parameter on output(chunk, flush = false) exists solely to skip registering a drain listener under backpressure, but the only call site that passes flush=true is flush() → output("", true), and an empty-string chunk never triggers backpressure on process.stdout.write, so wait.ready is always true and the !flush branch is never reached differently. The flag is effectively dead at its current call site. (2) The trailing await output("", true) in flush() adds no drain guarantee beyond what await state.queue already provides: per Node's Writable contract, an empty-string write is a no-op at the OS level and its callback fires regardless of buffer state. The construct is misleading — future readers will assume the trailing empty write is what guarantees OS-level flush and "fix" it incorrectly. await state.queue already awaits every prior write's per-chunk callback (which fires after libuv hands the bytes to the OS), so the kernel pipe buffer already holds everything before flush() returns.
function output(chunk: string, flush = false) {
if (state.closed || process.stdout.destroyed || process.stdout.writableEnded) return Promise.resolve()
return new Promise<void>((resolve) => {
const drain = () => resolve()
const wait = { ready: true, done: false }
wait.ready = process.stdout.write(chunk, (error) => {
wait.done = true
if (error) state.closed = true
if (!wait.ready) process.stdout.off("drain", drain)
resolve()
})
if (!wait.ready && !wait.done && !flush) process.stdout.once("drain", drain)
})
}| }) | ||
|
|
||
| export namespace Stdout { | ||
| export function write(chunk: string) { |
There was a problem hiding this comment.
🟡 state.queue grows unbounded under backpressure.
Suggested change
| export function write(chunk: string) { | |
| Bound the queue: track pending bytes/chunks; when above a high watermark, either await a drain slot before queueing more, or expose `write()` as async and have `emit()`/the agent loop await it so backpressure propagates to the producer. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #95, packages/cli/src/cli/stdout.ts:34-36):
Problem: state.queue grows unbounded under backpressure
Detail: `Stdout.write()` reassigns `state.queue = state.queue.then(() => output(chunk))` on every call; each `.then` closure captures `chunk`. `output()` is fully serialized (one in-flight write at a time), so when the downstream pipe applies backpressure (slow consumer, e.g. `aictrl run --format=json | pv -L 1k`), writes accumulate as pending closures in `state.queue`, each retaining its full chunk string. There is no high-watermark, no drop policy, and no backpressure signal back to `emit()` callers — `emit()` returns `true` synchronously and the producer keeps calling. Long runs against a slow pipe can grow memory without bound before `flush()` runs. The previous direct `process.stdout.write` had the same fundamental issue but resided in libuv's buffer; this PR duplicates the retention in a JS Promise chain on top.
Suggested fix: Bound the queue: track pending bytes/chunks; when above a high watermark, either await a drain slot before queueing more, or expose `write()` as async and have `emit()`/the agent loop await it so backpressure propagates to the producer.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Stdout.write() reassigns state.queue = state.queue.then(() => output(chunk)) on every call; each .then closure captures chunk. output() is fully serialized (one in-flight write at a time), so when the downstream pipe applies backpressure (slow consumer, e.g. aictrl run --format=json | pv -L 1k), writes accumulate as pending closures in state.queue, each retaining its full chunk string. There is no high-watermark, no drop policy, and no backpressure signal back to emit() callers — emit() returns true synchronously and the producer keeps calling. Long runs against a slow pipe can grow memory without bound before flush() runs. The previous direct process.stdout.write had the same fundamental issue but resided in libuv's buffer; this PR duplicates the retention in a JS Promise chain on top.
export function write(chunk: string) {
state.queue = state.queue.then(() => output(chunk))
return state.queue
}
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 2 · 🟡 4 · ⚪ 1 · 0/7 resolved
🤖 Fix all 7 open findings with your agent📋 Out-of-diff findings (7)
Reviewed 6 files · 0 inline · view all 7 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Contributor
Author
|
Addressed the current-head review against
Verification on this head:
Requesting one bounded re-review of the updated head. No merge requested. |
| return pending | ||
| } | ||
|
|
||
| export async function flush() { |
There was a problem hiding this comment.
🟡 flush() throwing makes it unsafe for finally blocks.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #95, packages/cli/src/cli/stdout.ts:41-44):
Problem: flush() throwing makes it unsafe for finally blocks
Detail: flush() is intended to be awaited in the finally of both entrypoints before process.exit(). By throwing `state.error` it forces every caller in a shutdown path to remember a try/catch, and the current callers (index.ts, headless.ts) do NOT wrap it — so the throw propagates and skips process.exit() (see the two MAJOR findings). Either swallow inside flush and expose the error via a getter (e.g. `Stdout.error()`) for callers that care, or document on flush() that it must be wrapped on shutdown paths. As-is, the API is a foot-gun the only two callers in the repo have already stepped on.
Suggested fix: Option A (consolidate at source): `export async function flush() { while (state.pending.size) await Promise.all(state.pending) }` and add `export function error() { return state.error }`. Option B (keep throw but document): add a JSDoc note that flush() throws on prior write errors and must be wrapped when used before process.exit().
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
flush() is intended to be awaited in the finally of both entrypoints before process.exit(). By throwing state.error it forces every caller in a shutdown path to remember a try/catch, and the current callers (index.ts, headless.ts) do NOT wrap it — so the throw propagates and skips process.exit() (see the two MAJOR findings). Either swallow inside flush and expose the error via a getter (e.g. Stdout.error()) for callers that care, or document on flush() that it must be wrapped on shutdown paths. As-is, the API is a foot-gun the only two callers in the repo have already stepped on.
export async function flush() {
while (state.pending.size) await Promise.all(state.pending)
if (state.error) throw state.error
}| } | ||
|
|
||
| export namespace Stdout { | ||
| export function write(chunk: string) { |
There was a problem hiding this comment.
🟡 write() never rejects while flush() throws.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #95, packages/cli/src/cli/stdout.ts:27-39):
Problem: write() never rejects while flush() throws
Detail: Stdout.write returns a Promise<void> that ALWAYS resolves: the write callback stores any error into state.error via fail() and then resolves unconditionally. Errors only surface when flush() is later called and throws. This is asymmetric with flush()'s throw-on-error contract and breaks the usual Promise<void> intuition — callers who `await Stdout.write(...)` expecting it to reject on I/O failure will silently miss errors. The one caller in run.ts (emit()) does not await the returned promise, so the contract is currently load-bearing on remembering to call flush().
Suggested fix: Either document the contract on write() (e.g. `// Resolves regardless; errors are surfaced by flush()`), or have write() reject when state.error is already set so individual awaits can observe prior failures.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Stdout.write returns a Promise that ALWAYS resolves: the write callback stores any error into state.error via fail() and then resolves unconditionally. Errors only surface when flush() is later called and throws. This is asymmetric with flush()'s throw-on-error contract and breaks the usual Promise intuition — callers who await Stdout.write(...) expecting it to reject on I/O failure will silently miss errors. The one caller in run.ts (emit()) does not await the returned promise, so the contract is currently load-bearing on remembering to call flush().
export function write(chunk: string) {
bind()
if (state.closed || process.stdout.destroyed || process.stdout.writableEnded) return Promise.resolve()
const pending = new Promise<void>((resolve) => {
process.stdout.write(chunk, (error) => {
if (error) fail(error)
resolve()
})
})
state.pending.add(pending)
pending.then(() => state.pending.delete(pending))
return pending
}| const lines = (await output) | ||
| .trim() | ||
| .split("\n") | ||
| .map((line) => JSON.parse(line)) |
There was a problem hiding this comment.
🟡 EPIPE test shells out to bash+head, unlike sibling tests.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #95, packages/cli/test/cli/stdout.test.ts:20-35):
Problem: EPIPE test shells out to bash+head, unlike sibling tests
Detail: The "treats a closed output pipe as controlled" test wraps the fixture in `bash -c '... | head -c 1'`, while the three other tests in the same file spawn the fixture directly via `Bun.spawn` with no shell. This is an internal style inconsistency and adds a POSIX-only dependency (bash, head) that will fail on Windows CI or minimal containers. The same scenario (read one byte, then close the consumer) can be expressed with `Bun.spawn({ stdout: "pipe" })` + `cli.stdout.getReader()` + `reader.read()` + `reader.cancel()`, keeping the file shell-free.
Suggested fix: Replace the bash wrapper with a direct Bun spawn whose stdout is read for 1 byte then cancelled:
```ts
const cli = Bun.spawn([process.execPath, fixturePath, "--epipe"], { stdout: "pipe", stderr: "pipe" });
const reader = cli.stdout.getReader();
await reader.read();
await reader.cancel();
expect(await cli.exited).toBe(0);
expect(await new Response(cli.stderr).text()).toBe("");
```
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The "treats a closed output pipe as controlled" test wraps the fixture in bash -c '... | head -c 1', while the three other tests in the same file spawn the fixture directly via Bun.spawn with no shell. This is an internal style inconsistency and adds a POSIX-only dependency (bash, head) that will fail on Windows CI or minimal containers. The same scenario (read one byte, then close the consumer) can be expressed with Bun.spawn({ stdout: "pipe" }) + cli.stdout.getReader() + reader.read() + reader.cancel(), keeping the file shell-free.
test("treats a closed output pipe as controlled", async () => {
const child = Bun.spawn(
[
"bash",
"-c",
'set -o pipefail; "$1" "$2" --epipe | head -c 1 >/dev/null',
"--",
process.execPath,
path.join(import.meta.dir, "fixture", "stdout.ts"),
],
{
cwd: path.join(import.meta.dir, "../.."),
stdout: "ignore",
stderr: "pipe",
},
)
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 2 · 🟡 3 · ⚪ 2 · 0/7 resolved
🤖 Fix all 7 open findings with your agent📋 Out-of-diff findings (7)
Reviewed 6 files · 0 inline · view all 7 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Contributor
Author
|
Final bounded review round addressed at
Verification:
This is the second and final remediation round. No merge requested. |
| @@ -212,5 +213,11 @@ try { | |||
| // Most notably, some docker-container-based MCP servers don't handle such signals unless | |||
| // run using `docker run --init`. | |||
| // Explicitly exit to avoid any hanging subprocesses. | |||
There was a problem hiding this comment.
🟡 Flush-catch block duplicated across both entrypoints.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #95, packages/cli/src/index.ts:215-220):
Problem: Flush-catch block duplicated across both entrypoints
Detail: The exact 6-line `await Stdout.flush().catch(...)` block (flush, log via Log.Default.error, set process.exitCode=1) is copy-pasted verbatim in index.ts (lines 215-220) and headless.ts (lines 158-163). The consistency convention calls for logic used across files to live in a shared module. Extracting a helper (e.g. `Stdout.flushSafely()` on the same module, optionally taking a logger to keep stdout.ts log-agnostic) would remove the duplication and also relieve the structural test in stdout.test.ts that currently string-matches this exact block.
Suggested fix: Add a helper to packages/cli/src/cli/stdout.ts, e.g. `export async function flushSafely() { try { await Stdout.flush() } catch (error) { Log.Default.error("stdout flush failed", { error: error instanceof Error ? error.message : error }); process.exitCode = 1 } }`, then call `await Stdout.flushSafely()` from the `finally` block of both index.ts and headless.ts.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The exact 6-line await Stdout.flush().catch(...) block (flush, log via Log.Default.error, set process.exitCode=1) is copy-pasted verbatim in index.ts (lines 215-220) and headless.ts (lines 158-163). The consistency convention calls for logic used across files to live in a shared module. Extracting a helper (e.g. Stdout.flushSafely() on the same module, optionally taking a logger to keep stdout.ts log-agnostic) would remove the duplication and also relieve the structural test in stdout.test.ts that currently string-matches this exact block.
// run using `docker run --init`.
// Explicitly exit to avoid any hanging subprocesses.
await Stdout.flush().catch((error) => {
Log.Default.error("stdout flush failed", {
error: error instanceof Error ? error.message : error,
})
process.exitCode = 1
})
process.exit()
}| }) | ||
|
|
||
| test("entrypoints handle flush failures before forced exit", async () => { | ||
| for (const entry of ["index.ts", "headless.ts"]) { |
There was a problem hiding this comment.
🟡 Entrypoint flush test is string-based, not behavioral.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #95, packages/cli/test/cli/stdout.test.ts:67-73):
Problem: Entrypoint flush test is string-based, not behavioral
Detail: The test "entrypoints handle flush failures before forced exit" reads the entrypoint source files as text and only checks for the substrings `await Stdout.flush().catch` and a subsequent `process.exit()`. It never spawns the entrypoints and never observes runtime behavior — e.g. that a flush rejection sets `process.exitCode=1` and the process exits non-zero. Every other test in this file spawns a subprocess and asserts observable behavior, so this structural test breaks the file's convention. It also gives false confidence: it would still pass if the `await` were removed (strings left in a comment), and it would break on a harmless refactor such as extracting the flush-catch into a helper or changing `process.exit()` to `process.exit(process.exitCode)`.
Suggested fix: Replace with a behavioral assertion: spawn index.ts / headless.ts in a configuration where Stdout.flush() rejects (e.g. pipe stdout to a reader that closes early and injects a non-EPIPE error, or wrap with a fixture that forces EIO) and assert `child.exited === 1` and that stderr contains the `stdout flush failed` log line.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The test "entrypoints handle flush failures before forced exit" reads the entrypoint source files as text and only checks for the substrings await Stdout.flush().catch and a subsequent process.exit(). It never spawns the entrypoints and never observes runtime behavior — e.g. that a flush rejection sets process.exitCode=1 and the process exits non-zero. Every other test in this file spawns a subprocess and asserts observable behavior, so this structural test breaks the file's convention. It also gives false confidence: it would still pass if the await were removed (strings left in a comment), and it would break on a harmless refactor such as extracting the flush-catch into a helper or changing process.exit() to process.exit(process.exitCode).
test("entrypoints handle flush failures before forced exit", async () => {
for (const entry of ["index.ts", "headless.ts"]) {
const source = await Bun.file(path.join(import.meta.dir, "../../src", entry)).text()
const flush = source.lastIndexOf("await Stdout.flush().catch")
expect(flush).toBeGreaterThan(-1)
expect(source.indexOf("process.exit()", flush)).toBeGreaterThan(flush)
}
})
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 2 · ⚪ 0 · 0/2 resolved
🤖 Fix all 2 open findings with your agent📋 Out-of-diff findings (2)
Reviewed 6 files · 0 inline · view all 2 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Contributor
Author
|
Addressed the user-selected findings from the current-head review at
Verification:
This user-triggered round is bounded; no further automated review loop is requested. No merge requested. |
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.
Closes #91
Intent
Make machine-readable NDJSON output durable through the CLI's forced-exit path without changing event meaning or ordering.
Expected outcomes
Expected impact
Automation and CI consumers can parse complete streams reliably instead of receiving partial JSON or losing the final status. Interactive and non-NDJSON behavior remains unchanged, and this establishes the stdout-delivery foundation used by the invocation and signal workstreams.
Summary
Reproduction
A Bun 1.3.10 child that immediately forced-exited after writing ~2.1 MiB to a pipe delivered 621,248 bytes and lost its terminal record. The regression test uses a real child process and verifies all 1,025 NDJSON records, including the terminal record, arrive before forced exit.
This change only addresses stdout durability. It does not claim to fix independent event-delivery ordering races.
Verification
cd packages/cli && bun test test/cli/stdout.test.ts— 2 passedcd packages/cli && bun run typecheck— passedcd packages/cli && bun test— 1,326 passed, 7 skippedgit diff --check— passed