Fix/windows utf8 io#86
Open
Le-Anh-Duy wants to merge 4 commits into
Open
Conversation
subprocess.Popen(preexec_fn=..., start_new_session=True) raised "preexec_fn is not supported on Windows platforms" for every LLM call and test-runner invocation, causing /cmind.encode and friends to fail outright on Windows. Branch on platform: POSIX keeps start_new_session + preexec_fn (killpg-based tree kill); Windows uses CREATE_NEW_PROCESS_GROUP and a new _kill_process_tree() helper that shells out to `taskkill /T /F` to reap the process tree instead. Verified end-to-end: reinstalled the patched package as the local cmind-cli tool and ran a full /cmind.encode against an external repo on Windows; it now completes successfully (previously failed on every LLM call).
On Windows, _kill_process_tree ignores taskkill's return code. If taskkill fails (e.g. access denied / process already exited / PID reuse edge), this function returns without killing the process, but callers then do proc.wait(), which can hang indefinitely. Please fall back to proc.kill() when taskkill returns non-zero (or use check=True and catch CalledProcessError). Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
On Windows, a bundled script's stdout/stderr is almost never a real
console (cmind script pipes it, hooks capture it, the initial-encode
kickoff pipes it), so CPython falls back to locale.getpreferredencoding()
for stdio instead of UTF-8. That's a legacy code page (cp1252, cp936,
...) on most Windows installs, so a bare print() of any non-ASCII
character raises UnicodeEncodeError and kills the script outright.
Confirmed live: update_graphs.py's `status` command prints an MCP-tools
guidance line containing "->" (functional areas -> groups -> features)
unconditionally whenever an RPG exists and is readable -- not an edge
case, it fires on the ordinary "status looks fine" path, which is why
the SessionStart/post-commit hooks were silently failing
("[CoderMind] RPG status unavailable"). Reproduced end-to-end against
a minimal sample project: `cmind script update_graphs.py status`
crashes with the unfixed build and succeeds with the fixed one.
Whether the same class of crash is what broke /cmind.encode specifically
for a Windows reviewer is not confirmed (that command's own
print(json.dumps(...)) calls are ASCII-safe by default) -- see
tests/repro_windows_utf8/README.md for the full writeup of what's
verified vs. still a hypothesis, and a from-scratch repro that needs no
LLM calls.
- common/paths.py: reconfigure sys.stdout/stderr to UTF-8 at import
time. Every bundled script imports this module near the top, making
it a single choke point that protects all of them uniformly
regardless of how they end up being invoked (cmind script wrapper,
a hook, a test, direct invocation, ...).
- cmind_cli/__init__.py: also set PYTHONIOENCODING=utf-8:replace /
PYTHONUTF8=1 on the child env in script() and _run_initial_encode(),
and encoding="utf-8"/errors="replace" on the encoder's Popen, as
defense in depth for non-Python or third-party children.
- llm_client.py: same encoding/errors on the LLM CLI subprocess's
Popen, so decoding its stdout/stderr never raises UnicodeDecodeError
either.
- test_hooks_install.py: regression test pinning that the
diverged-branch status guidance (which also contains the arrow
character) no longer crashes update_graphs.py on a piped stdout.
- tests/repro_windows_utf8/: standalone, LLM-free repro (two two-line
scripts) demonstrating the crash and the fix in under a second.
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a reproducible Windows UnicodeEncodeError caused by non-UTF-8 stdio when bundled scripts run with piped/captured output (common for hooks and cmind script). The fix forces UTF-8 stdio consistently across script entrypoints and subprocesses, and adds regression coverage plus a fast local repro.
Changes:
- Force UTF-8 stdio (with
errors="replace") for bundled scripts and key subprocess launches to prevent Windows code-page encoding crashes. - Add a regression test covering non-ASCII status output and a minimal, LLM-free Windows repro harness.
- Refactor process-tree termination logic to be cross-platform (POSIX process groups vs. Windows
taskkill /T).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| CoderMind/tests/test_hooks_install.py | Adds regression test to ensure non-ASCII status output doesn’t crash under captured stdout. |
| CoderMind/tests/repro_windows_utf8/repro_runner.py | Adds a fast repro driver that mimics CoderMind’s subprocess capture pattern. |
| CoderMind/tests/repro_windows_utf8/repro_child.py | Adds a minimal child script that prints a Unicode arrow to reproduce the failure mode. |
| CoderMind/tests/repro_windows_utf8/README.md | Documents the Windows failure mode, evidence, and repro steps. |
| CoderMind/src/cmind_cli/init.py | Forces UTF-8 environment/decoding for script and encoder subprocesses. |
| CoderMind/scripts/common/paths.py | Reconfigures sys.stdout/sys.stderr to UTF-8 at a shared import “choke point”. |
| CoderMind/scripts/common/llm_client.py | Adds cross-platform process-tree killing and Windows-safe subprocess configuration for the AI CLI. |
| CoderMind/scripts/code_gen/test_runner.py | Switches pytest subprocess management to use the shared cross-platform kill logic. |
| .gitignore | Updates ignore rules for .cmind and related generated directories/files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
@Le-Anh-Duy Thank you for your further efforts! I tried this new PR, and find another small patch that can make it work on my Windows device. The patch is as follows: |
Reviewer-suggested patch (@QingtaoLi1): on Windows the configured AI CLI command is often a quoted, backslash-separated path with an executable suffix (e.g. "C:\tools\claude.cmd") rather than the bare name _CLI_TO_AGENT is keyed on, so detection always fell through to "unknown". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@QingtaoLi1 Thanks for checking! I dug into this and can confirm there's a real, reproducible Windows bug — just not exactly where I first assumed.
Confirmed: cmind script update_graphs.py status (used by the SessionStart/post-commit hooks) crashes with UnicodeEncodeError on Windows whenever an RPG already exists:
UnicodeEncodeError: 'charmap' codec can't encode character '→' in position ...
Root cause: on Windows, a script's stdout is almost never a real console (it's piped by cmind script, captured by a hook, etc.), so CPython falls back to locale.getpreferredencoding() for stdio instead of UTF-8 — a legacy code page (cp1252/cp936/...) on most Windows installs. _format_status_for_agent() unconditionally prints an MCP-tools guidance line containing "→" (functional areas → groups → features) whenever the RPG is readable, so this fires on the ordinary "status looks fine" path, not just some rare edge case.
I reproduced it end-to-end on a fresh 1-file sample repo: cmind script update_graphs.py status crashes on the unfixed build and succeeds on the fixed one, no LLM calls needed to trigger it.
Not confirmed: whether this exact code path is what broke /cmind.encode itself for you — run_encode.py/check_encode.py's own print(json.dumps(...)) calls are ASCII-safe by default (ensure_ascii=True), so they shouldn't hit this. If some other print()/log call deeper in the encode pipeline emitted a non-ASCII character (LLMs often produce "invisible" typographic characters — smart quotes, em-dashes, arrows, bullets — even for plain English content), it would crash the same way. Could you share the exact traceback/error text you saw from /cmind.encode? That'll tell us for sure whether it's the same bug or a second one.
Pushed a fix to fix/windows-utf8-io: forces UTF-8 stdio at the single import choke point every bundled script shares (common/paths.py), plus PYTHONIOENCODING/errors="replace" as defense in depth around the cmind script subprocess launch and the LLM CLI subprocess. Added a regression test and a from-scratch, LLM-free repro under tests/repro_windows_utf8/ if you want to verify on your machine.