Fix piped workflow JSON output truncation - #2389
Conversation
Large workflow JSON payloads could be truncated when Bun hit pipe backpressure while the CLI still exited successfully. Changes: - Preserve CLI exit codes without forcing process termination - Await stdout completion for large status and verbose get payloads - Add lifecycle and stdout backpressure regression coverage Fixes #2384
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe CLI adds callback-aware stdout utilities for machine-readable output. Workflow, AI, and validation commands use asynchronous JSON-line writes. Tests capture stdout writes and verify complete piped output, JSON payloads, and exit statuses. ChangesCLI JSON stdout delivery
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Self-Fix Report (Aggressive)Status: COMPLETE Fixes Applied (0 total)The review reported no actionable findings. Tests Added(none) Skipped (0)(none — all findings addressed) Suggested Follow-up Issues(none) Validation✅ Type check | ✅ Lint | ✅ Tests Self-fix by Archon · aggressive mode · no changes required on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/cli/src/commands/workflow.test.ts (1)
2342-2355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover backpressure in the verbose
workflowGetCommandpath.The get-command mock invokes the
process.stdout.writecallback immediately. The assertion at Line [2524] checks only the captured JSON. Add a test that holds the callback, verifiesworkflowGetCommand('run-v', true, true)remains pending, then releases the callback. The existing delayed-callback test covers onlyworkflowStatusCommand.Also applies to: 2524-2524
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/workflow.test.ts` around lines 2342 - 2355, Extend the tests around workflowGetCommand to cover stdout backpressure: make the process.stdout.write mock retain its callback, assert workflowGetCommand('run-v', true, true) remains pending before releasing it, then invoke the callback and verify completion and output. Keep the existing workflowStatusCommand delayed-callback coverage unchanged.packages/cli/src/cli.test.ts (1)
253-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTest the runtime lifecycle instead of source text.
This test reads
cli.tsand checks string presence. It does not executemain()or verify that pending stdout completes before exit. Add a behavior-level test that controls stdout completion and checks the normal and fatal exit codes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli.test.ts` around lines 253 - 263, Replace the source-text assertions in the “CLI process lifecycle” test with a runtime test that invokes the CLI entrypoint, controls or mocks stdout completion, and verifies output is allowed to flush before termination. Cover both the normal exit-code path and the fatal-error path, asserting the resulting exit codes without relying on string searches of cli.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/cli/src/cli.test.ts`:
- Around line 253-263: Replace the source-text assertions in the “CLI process
lifecycle” test with a runtime test that invokes the CLI entrypoint, controls or
mocks stdout completion, and verifies output is allowed to flush before
termination. Cover both the normal exit-code path and the fatal-error path,
asserting the resulting exit codes without relying on string searches of cli.ts.
In `@packages/cli/src/commands/workflow.test.ts`:
- Around line 2342-2355: Extend the tests around workflowGetCommand to cover
stdout backpressure: make the process.stdout.write mock retain its callback,
assert workflowGetCommand('run-v', true, true) remains pending before releasing
it, then invoke the callback and verify completion and output. Keep the existing
workflowStatusCommand delayed-callback coverage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a1c15f77-6a45-4d6e-bf8e-5bda41500c93
📒 Files selected for processing (4)
packages/cli/src/cli.test.tspackages/cli/src/cli.tspackages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.ts
The previous attempt did not fix the bug. `workflow runs --json --all
--limit 200 | cat` still truncated at exactly 98,304 bytes with exit 0 in
5/5 runs, because that command was never one of the two call sites it
patched — and its tests mocked `process.stdout.write`, the very thing that
was broken, so they passed regardless.
Root cause: `@archon/paths` builds the pino root logger at module load,
whose default destination puts fd 1 into non-blocking mode. Every CLI
command imports it transitively. On a non-blocking pipe a write larger than
the pipe capacity returns a SHORT count, and `console.log` discards the
unwritten remainder without error. Verified directly: `fs.writeSync(1, ...)`
inside the CLI returns 65536-byte short writes and intermittently throws
EAGAIN. A regular-file fd stays blocking, which is why redirecting to a file
always looked fine.
The loss happens inside `console.log` at call time, not at process exit, so
no exit-path flush can recover it — confirmed by measuring an awaited
trailing `process.stdout.write('')`, which still truncated 8/8. That makes
the previous `process.exit()` -> `process.exitCode` change unnecessary, and
it carried a real hang risk if any command leaves a handle open, so it is
reverted.
Changes:
- Add src/utils/stdout.ts: writeStdout()/writeJsonLine() resolve only once
every byte has reached the OS, retrying short writes and EAGAIN through
the stream rather than busy-waiting.
- Route every CLI --json emitter through it (workflow list/status/get/runs/
search/reset-sessions, the approve/reject/abandon/resume envelopes, the
--detach ack, validate workflows/commands, ai tier/alias list). No
machine-readable payload goes through console.log any more. Output bytes
are unchanged, including the compact-vs-pretty formatting of each command.
- Restore explicit process.exit() so a lingering handle cannot hang the CLI.
- Replace the mock-based and source-grep tests with a real-pipe regression
test that spawns the CLI through a genuine `| cat` shell pipeline and
compares the result byte-for-byte against a file redirect, 10 runs per
assertion, plus exit-code propagation. Verified to fail on the pre-fix
code (98,304 vs 162,979 bytes) — Bun.spawn's own pipe does not reproduce
the truncation, so a test built on it would have proved nothing.
- Wire src/cli.test.ts into the package test script; it was never run.
Measured on the reported repro (`workflow runs --json --all --limit 200`):
before 1/6 piped runs valid, after 12/12 valid and byte-identical to the
file redirect.
Fixes #2384
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/utils/stdout.test.ts`:
- Around line 78-99: Change the stdout test fixture lifecycle from
beforeAll/afterAll to beforeEach/afterEach so each test receives a fresh scratch
directory and generated workflows, and cleanup runs after every test. Preserve
the existing fixture creation in beforeAll and removal logic in afterAll while
moving them to the per-test hooks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c68bdecf-3670-4f2a-9433-d084e5cfbb92
📒 Files selected for processing (9)
packages/cli/package.jsonpackages/cli/src/cli.tspackages/cli/src/commands/ai.test.tspackages/cli/src/commands/ai.tspackages/cli/src/commands/validate.tspackages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.tspackages/cli/src/utils/stdout.test.tspackages/cli/src/utils/stdout.ts
| beforeAll(() => { | ||
| scratch = mkdtempSync(join(tmpdir(), 'archon-pipe-test-')); | ||
| archonHome = join(scratch, 'home'); | ||
| repoDir = join(scratch, 'repo'); | ||
| mkdirSync(archonHome, { recursive: true }); | ||
| const workflowsDir = join(repoDir, '.archon', 'workflows'); | ||
| mkdirSync(workflowsDir, { recursive: true }); | ||
| spawnSync('git', ['init', '-q', '.'], { cwd: repoDir }); | ||
|
|
||
| const padding = 'padding '.repeat(DESCRIPTION_WORDS); | ||
| for (let i = 0; i < WORKFLOW_COUNT; i++) { | ||
| const name = `probe-${String(i).padStart(3, '0')}`; | ||
| writeFileSync( | ||
| join(workflowsDir, `${name}.yaml`), | ||
| `name: ${name}\ndescription: ${padding}${i}\nnodes:\n - id: only\n prompt: hello\n` | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| if (scratch) rmSync(scratch, { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Clean up the temporary fixture after each test.
beforeAll creates workflow files and pipeline output files that remain available to later tests. Use beforeEach and afterEach, or remove all generated files in afterEach, to isolate the tests.
As per coding guidelines, “clean up test data after each test.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/utils/stdout.test.ts` around lines 78 - 99, Change the
stdout test fixture lifecycle from beforeAll/afterAll to beforeEach/afterEach so
each test receives a fresh scratch directory and generated workflows, and
cleanup runs after every test. Preserve the existing fixture creation in
beforeAll and removal logic in afterAll while moving them to the per-test hooks.
Source: Coding guidelines
Two assertions in cli.test.ts hardcode '/tmp', which does not exist on Windows —
findRepoRoot('/tmp') and existsSync('/tmp') both fail there.
Not a Windows defect in the CLI. This file was absent from the package's test
script until it was wired in alongside the stdout work, so the POSIX assumption
had never run in CI and the failure only appeared once the file started
executing. os.tmpdir() carries the same intent on every platform: a directory
that exists and is not inside a git repo.
|
Pushed Two assertions hardcoded const result = await git.findRepoRoot('/tmp');
expect(existsSync('/tmp')).toBe(true);Neither holds on Windows, where that path does not exist. Both now use Worth noting why this surfaced now — Verified: 50 pass / 0 fail locally, type-check and format clean. |
|
On the The guideline's concern ("clean up test data after each test") is already satisfied: The isolation risk worth checking would be tests sharing an output filename, where a truncated-to-zero run could silently read a previous test's good file — which for a truncation test would be the worst possible failure mode. They don't share:
Every write target is distinct, so a stale file cannot mask a failure. Moving to |
1 participant
Summary
workflow status --json --verboseandworkflow get --json --verbosepayloads could be silently truncated when stdout was piped, while the CLI exited successfully.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
commands/workflow.tsprocess.stdoutcli.tsLabel Snapshot
risk: lowsize: Scli,testscli:workflow-outputChange Metadata
bugcliLinked Issue
Validation Evidence (required)
Commands and result summary:
bun run type-check bun run lint bun run format:check bun run test bun run buildProcess+Pipe()regression captured parseable output in 16/16 runs forworkflow status --json --verbose(363,722 bytes each) and 16/16 runs forworkflow get --json --verbose(84,459–87,307 bytes); the not-found JSON envelope also parsed fully with exit status 1.Security Impact (required)
Yes, describe risk and mitigation: Not applicable.Compatibility / Migration
Human Verification (required)
What was personally validated beyond CI:
Side Effects / Blast Radius (required)
Rollback Plan (required)
634f1ba7.Risks and Mitigations
Summary by CodeRabbit
Bug Fixes
Tests