Repository navigation
fix(cli): preserve UTF-8 across subprocess output chunks - #5296
user-github-me wants to merge 10 commits into
Conversation
|
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 |
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for keeping this up to date with main. The change in runCancellableProcess is right: each stream gets its own StringDecoder, the output limit is counted on raw bytes across both streams before decoding, and the decoders are flushed on close so a trailing partial character becomes a replacement character instead of disappearing. I also checked the two onStdout callers (installInChild.ts line reader and the whisper transcriber). The callback can now receive an empty string when a chunk holds only part of a character, and both handle that fine. The tests call runCancellableProcess directly, which is the function the CLI's setup workers go through, and CI is green on this head.
One thing to fix before merge: the new sentence in docs/developers/cli.mdx (lines 51-53) says diagnostics from child tools keep Unicode intact across chunks, but several other places still decode each chunk on its own:
packages/cli/src/capture/captureCompositionFrame.ts:868-870(ffmpeg stderr)packages/cli/src/background-removal/pipeline.ts:425-427(ffmpeg stderr)packages/cli/src/commands/render.ts:868-871(docker stderr tail)packages/cli/src/utils/optionalPackages.ts:293-294(runNpmerror output)
For example, an ffmpeg error during frame capture that names 项目/音声.wav can still show a replacement character if the name crosses a chunk boundary. Either narrow the docs sentence to the processes this PR covers, or give those sites the same decoder treatment (collecting Buffers and decoding once at the end, as stoppableExecFile.ts already does, also works). Either is fine with me.
Small optional tidy-up: skipping the onStdout call when decoded is empty would keep the callback from firing with nothing.
#5273 is open at this same commit, so please keep one of the two so review and the merge queue happen in one place.
Verdict: REQUEST_CHANGES
Reasoning: The decoding fix is correct and tested through the real function, but the added docs promise the same behavior for child tools that still decode per chunk.
Closed as a duplicate of #5273, which has the same tested commit and retains the earlier review history. Review and merging continue there, as requested in the review.
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.