Repository navigation
fix(cli): preserve UTF-8 across subprocess output chunks - #5273
user-github-me wants to merge 10 commits into
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
The decoder change is right, but the required Tests on windows-latest check is red at this head. One of the tests this PR adds fails on Windows, and it failed again on rerun:
FAIL src/utils/cancellableProcess.test.ts > cancellable process output > still rejects output that exceeds the combined raw byte limit
AssertionError: expected Error: taskkill exited with status 128 to match object { code: 'ENOBUFS', killed: true }
Why. In splitOutputSource, the second write overflows the 3-byte limit and the child exits right after it. terminate() starts terminateProcessTree. On Windows that spawns taskkill, which exits 128 (process not found) because the child has already gone. processTree.ts turns any non-zero taskkill status into a rejection. The close handler awaits terminationTask, catches that rejection and passes it to finishReject, so the taskkill error replaces the ENOBUFS CancellableProcessError. On POSIX, signalling a dead group is swallowed, which is why Linux and macOS pass.
Fix, either of:
- Test-only: keep the child alive after the overflow write, for example by ending
splitOutputSource's timeout callback withsetTimeout(() => {}, 10_000)in the overflow case. The kill then hits a live process on every platform. - Behavioral (arguably the real bug): treat taskkill "process not found" (status 128) as success in
terminateProcessTree. Alternatively, keep the originalterminationReasonwhen the tree teardown fails after the child has already closed. Otherwise any Windows caller whose child exits during an ENOBUFS, timeout or abort gets a confusingtaskkill exited with status 128instead of the real reason.
I'll approve once the Windows lane is green at the new head.
— Rames
275ced7 to
81e2252
Compare
|
Thanks for this, @user-github-me. This PR now has merge conflicts with |
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
Decode stdout and stderr incrementally so Unicode filenames and diagnostics remain intact when a subprocess splits a UTF-8 character across chunks. Count raw bytes across both streams for the existing output limit and flush unfinished trailing bytes when the streams close.
The overflow fixture keeps its child alive until termination, retaining the prior Windows review fix. This refresh also retains current
main's process-group behavior and preservation of the original termination reason if tree teardown fails. The CLI guide documents Unicode diagnostic handling.Validation:
main.Windows CI must confirm the refreshed head before merge; the local checks above ran on macOS.
Current-main compatibility
Merged main (
fcec26ed5) and resolved the conflict with its new browser-download progress callback. Progress callbacks receive the same decoded UTF-8 text as captured stdout, including incomplete trailing bytes flushed when the process closes. The browser-install mode, download stall detection, combined raw-byte limit, and process-tree cleanup remain covered.Three callback assertions reproduce corrupted non-ASCII text in a naive merge. The corrected merge passes all 99 tests across seven process, browser, and worker suites, including both download-stall cases. Full build, CLI rebuild/typecheck, root lint, normal commit hooks, both comment checks, and Mintlify validation/link checks also pass. Updated GitHub CI will run on the published commit.