Skip to content

fix(install): read native child logs with an explicit UTF-8 codec - #90510

Open
yoggydev wants to merge 1 commit into
NousResearch:mainfrom
yoggydev:fix/install-native-log-encoding
Open

fix(install): read native child logs with an explicit UTF-8 codec#90510
yoggydev wants to merge 1 commit into
NousResearch:mainfrom
yoggydev:fix/install-native-log-encoding

Conversation

@yoggydev

Copy link
Copy Markdown

What does this PR do?

_Invoke-NativeWithTimeout has cmd.exe redirect a child's stdout+stderr into a log file:

$cmdLine = "/d /s /c "" ""$exePath"" $argLine > ""$logPath"" 2>&1 """
$proc = Start-Process -FilePath $env:ComSpec -ArgumentList $cmdLine ...

No PowerShell decode happens on the way in, so that file holds the child's own bytes and carries no BOM. The children here are Node-based — npm, npx, playwright — and Node writes UTF-8.

Four reads of those logs do not state a codec:

$lines   = @(Get-Content $path -ErrorAction SilentlyContinue)                  # live tail
$errText = (Get-Content $logPath -Raw -ErrorAction SilentlyContinue)           # failure output
$pwErr   = Get-Content $pwLog -Raw -ErrorAction SilentlyContinue               # playwright
$tail    = Get-Content -LiteralPath $logPath -Tail $TailLines -ErrorAction Stop  # npm debug log

Windows PowerShell 5.1's Get-Content sniffs a BOM, and falls back to the machine ANSI code page when there isn't one.

Measured on ja-JP Windows 11 (ACP=932), Windows PowerShell 5.1.26100.9168. The same string written both ways, then read back:

no BOM  + bare Get-Content            error: 綢輔ぃ 經、綢ォ縺瑚九九▽縺九 j 縺セ縺帙 s
no BOM  + Get-Content -Encoding UTF8  error: ファイルが見つかりません
with BOM + bare Get-Content           error: ファイルが見つかりません

The BOM is the whole difference. Logs this script writes itself go through Out-File -Encoding utf8 and carry one, so they read back correctly. Logs a child writes do not.

Where it lands. _Drain-NewLines is the live-tail helper — it reads the log and prints it:

$lines = @(Get-Content $path -ErrorAction SilentlyContinue)
...
Write-Host "    $_" -ForegroundColor DarkGray

Near the top of the script there is [Console]::OutputEncoding = [System.Text.UTF8Encoding]::new(), whose own comment calls it "a DISPLAY-only fix ... only what the user sees in their terminal". Those two are on the same path and the read runs first, so on a CJK host that display fix never receives intact text from this stream — the one _Invoke-NativeWithTimeout exists to preserve, because "on a fresh VM the install is 1-3 minutes; total silence is indistinguishable from a hang".

$errText at the failure branch is the same file. That is the text a user is shown, and told to copy, at the moment an install fails.

Scope — two reads deliberately left alone. $npmLog and $buildLog are written through Tee-Object -FilePath, which on 5.1 writes UTF-16LE with a BOM. Measured: a single "あ" teed to a file gives FF FE 42 30 0D 00 0A 00. Get-Content detects that BOM, so those reads are already correct and adding -Encoding UTF8 to them would break them. The four changed here are exactly the ones whose file was written by a native child.

Note on the linter. scripts/check-windows-footguns.py never sees this file: should_scan_file() returns True only for .py, .pyw, .pyi. No .ps1 in the repository is scanned, including the Windows installer itself.

Related Issue

No separate issue — reporting it here with the measurement. Same class as #89442 / #89468 ("Affected Windows users lose diagnostic text from native commands"), one layer out: this is the installer, before the agent exists to lose anything.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • scripts/install.ps1 — the four Get-Content reads of a natively-written log pass -Encoding UTF8: the live tail and the failure output in _Invoke-NativeWithTimeout's consumers, the playwright log, and the npm debug-log tail.
  • scripts/install.ps1 — a comment on the cmd.exe redirect in _Invoke-NativeWithTimeout recording why any reader of that file has to state a codec, and that [Console]::OutputEncoding does not cover it.
  • tests/test_install_ps1_native_log_encoding.py — four tests: every natively-written log read states a codec; the redirect helper documents the constraint; the console fix is still display-only (so this is revisited rather than silently kept passing if that changes); and the Tee-Object sites stay out of scope so nobody "finishes the job" onto a UTF-16 file.

How to Test

  1. On a CP932 host, reproduce the decode directly:
[IO.File]::WriteAllBytes("$env:TEMP\nobom.log",[Text.Encoding]::UTF8.GetBytes("error: ファイルが見つかりません`r`n"))
"error: ファイルが見つかりません" | Out-File "$env:TEMP\bom.log" -Encoding utf8

Get-Content "$env:TEMP\nobom.log" -Raw                    # mojibake
Get-Content "$env:TEMP\nobom.log" -Raw -Encoding UTF8     # correct
Get-Content "$env:TEMP\bom.log"   -Raw                    # correct (BOM sniffed)
$PSVersionTable.PSVersion.ToString()

Output above is from that run.

  1. The scope check, on the same host:
"" | Tee-Object -FilePath "$env:TEMP\tee.log" | Out-Null
Format-Hex "$env:TEMP\tee.log" | Select-Object -First 1

FF FE 42 30 0D 00 0A 00 — UTF-16LE with a BOM, which is why the two Tee-Object reads are not touched.

  1. python -m pytest tests/test_install_ps1_*.py -q across all fourteen install.ps1 test files, including the four added here:
ja-JP Windows host   1 failed, 45 passed
Linux checkout       45 passed, 1 skipped

The one Windows failure is test_install_ps1_venv_process_tree.py::test_venv_sweep_stops_managed_runtime_children_but_not_unrelated_processes, which runs install.ps1 -Stage venv for real and stops on the installer's own precondition — {"ok":false,"reason":"uv is not installed. Run install.ps1 -Stage uv first."}. That host has no uv; the same test is skipped off-Windows. Nothing in this change touches that path.

  1. Against the pre-patch install.ps1 with the new tests kept: 2 failed / 2 passed. The two that pass either way are the scope statements — they assert facts about [Console]::OutputEncoding and Tee-Object that this PR does not change, and exist so a later edit cannot quietly invalidate the premise.

Note on the checklist below: I ran the install.ps1 test files and the comparison above, not the full pytest tests/ -q, so I left that box unchecked rather than claiming it.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11, ja-JP (ACP=932), Windows PowerShell 5.1.26100.9168

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

I have a ja-JP (ACP=932) Windows host and can measure anything else on that locale.

(Measured on my host. Drafted with Claude.)

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage area/install-update Installer, updater, packaging, wheels, doctor area/i18n Localization, locales, translations sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Aug 20, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Reviewed the diff. Thoroughly done: all four native-child log reads now state `-Encoding UTF8`, the reasoning lives as a comment at the redirect site where the BOM-less bytes are created, and the test module is unusually good — measured PS 5.1 behavior on ja-JP recorded in the docstring, an explicit out-of-scope statement for `Tee-Object`'s BOM'd UTF-16LE logs so nobody "finishes the job" into a misdecode, and a premise pin that `[Console]::OutputEncoding` remains display-only.

Nit: the regression test matches four specific `Get-Content` shapes by regex, so a future read written in a different parameter order (`Get-Content -Raw -Path $logPath`) slips through uncovered. A stronger and simpler invariant: assert every `Get-Content` line in install.ps1 contains `-Encoding`, with a tiny named allowlist for the Tee-Object-fed reads — that fails loudly on any new unannotated read regardless of argument order.

No blocking issues found.

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

Labels

area/i18n Localization, locales, translations area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants