Skip to content

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

Open
user-github-me wants to merge 10 commits into
heygen-com:mainfrom
user-github-me:fix/process-output-utf8
Open

user-github-me wants to merge 10 commits into
heygen-com:mainfrom
user-github-me:fix/process-output-utf8

Conversation

@user-github-me

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

Copy link
Copy Markdown
Contributor

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 jrusso1020 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.

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 with setTimeout(() => {}, 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 original terminationReason when 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 confusing taskkill exited with status 128 instead of the real reason.

I'll approve once the Windows lane is green at the new head.

— Rames

@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

This branch has not been deployed

No deployments
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.

2 participants