Skip to content

fix(cli): preserve UTF-8 across subprocess output chunks - #5296

Closed
user-github-me wants to merge 10 commits into
heygen-com:mainfrom
user-github-me:review/5273-windows-output-fixture
Closed

user-github-me wants to merge 10 commits into
heygen-com:mainfrom
user-github-me:review/5273-windows-output-fixture

Conversation

@user-github-me

@user-github-me user-github-me commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

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:

  • CLI rebuild and typecheck passed after updating against current main.
  • All 18 focused tests passed: split Unicode on both streams, error diagnostics, combined byte boundaries, trailing bytes, process-group cancellation and kill-failure handling.
  • Lint, formatting, test reachability, normal commit hooks and both comment checks passed.
  • Mint documentation validation and broken-link checks passed.

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.

@jrusso1020

Copy link
Copy Markdown
Collaborator

Thanks for this, @user-github-me. This PR now has merge conflicts with main. Could you rebase it onto the latest main? Once it's conflict-free we'll review it and get it into the merge queue.

@mintlify

mintlify Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Preview approval required

This pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit 18da975.

Approve preview

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (runNpm error 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants