Conversation
Closes #1538. ## The defect Python decodes a `text=True` subprocess pipe with the locale encoding when the call names none. On Linux that is UTF-8 and the default is invisible. On Windows, where UTF-8 mode is off by default, it is the ANSI code page, commonly cp1252, so output carrying a byte sequence that code page cannot map raises `UnicodeDecodeError`. #1538 reports this against `prose_lint.py`, whose git reads are the ones a downstream repository's docs send a maintainer through before a push. Two of them scope a `--diff` run, and the handler around one catches `CalledProcessError` and `FileNotFoundError`, neither of which reaches a decode failure, so that run ends in a traceback. It is one of the children of #1143, which groups the defects where fleet tooling assumes a POSIX host. ## What this changes **The encoding sweep.** Every `subprocess` call in the repository that asks for decoded output now pins `encoding="utf-8"`, rather than the one file #1538 names. The change is inert on Linux, where the locale already resolved to UTF-8, so CI measures no difference and the fix lands unverified on Windows by construction. **Errors handling where strict still fails.** Pinning the encoding fixed the locale half and left the strict half. A tracked latin-1 file modified in the working tree still ended the prose gate in a traceback, on every platform rather than only on Windows. `prose_lint.py` decodes with `surrogateescape` so such a byte round-trips to the same name on disk, and `host_gate.py` decodes a probe with `replace`, since a probe answering in another encoding is `unreadable` rather than fatal and the added strictness had made it a `ValueError` that neither handler there catches. **Escaping a name on output.** Surrogateescape leaves the lone surrogate in a name to reach the gate's own output, where encoding it strictly raises at the line printing that name, part way through the scan. The output streams escape it instead. The gate now scans and reports such a file, where before this change it silently skipped every untracked file beside it. **A false clean in the diff scope.** Found by a review pass on the above, in the one path this work's own subject area runs through. Git quotes a path holding any byte at or above 0x80, and `changed_lines` read the `+++` header as a literal path, so the header of an ordinary non-ASCII filename matched nothing and the file left scope. An empty scope is falsy, so the no-match refusal never fired either, and the run printed a clean gate on a file it never read. The action always passes `--diff`, so that is the production path. Measured before the fix, on a repository whose only change adds a duplicated word to a file named with one non-ASCII character: ```text --diff HEAD scope: 0 of 2 file(s) read exit 0 <- false clean whole tree Shoko.md:4: dupword ... exit 1 ``` The header is decoded rather than the quoting turned off, since `core.quotePath=false` suppresses only the first of the three routes by which git quotes a name and leaves a name holding a quote or a backslash to miss exactly as before. The invocation pins `core.quotePath=true`, which is what makes a quoted field ASCII and so what the decode assumes. ## Verification `scripts/tests/test_tooling_encoding.py` asserts the invariant over every Python file the checkout holds, tracked or newly added, so a call arriving later cannot reintroduce it. Nothing in this repository's toolchain checks it otherwise: ruff has no rule reaching a subprocess call at all, and its rule for the same omission on `open` and `read_text` is neither selected here nor out of preview. Every behavioral case was checked against a reverted fix rather than trusted. The cp1252 cases patch the locale accessors and drive the real git reads through a character cp1252 cannot map, skipping rather than asserting where UTF-8 mode means no patch can reach the decoder. The scope cases run under both `core.quotePath` settings and cover each route by which git quotes a header, plus two names taking two routes at once. Five local review passes ran against this branch, raising 25 findings between them, every one disposed of. The last pass raised nothing against the head commit, having attacked it with filenames covering every byte value from 0x01 to 0xFF, astral-plane characters, combining marks, invalid UTF-8, a symlink and a submodule. ## Known and not addressed here Two defects of the same false-clean class, both confirmed present at the merge base and so not regressions, are left for issues of their own rather than grown into this change: - A `+++` header whose path contains a space carries a trailing tab that the header parse does not strip, so such a file leaves diff scope. - The `b/` prefix is read from host config rather than pinned, so `diff.noprefix`, `diff.mnemonicPrefix`, `diff.dstPrefix` and `diff.external` each empty the scope. `diff.mnemonicPrefix` is an ordinary developer setting, so this is reachable on a local pre-push run. A third, that a call pins the parent's decoder to UTF-8 while the child interpreter it spawns still encodes with the locale, is likewise filed rather than fixed here. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved handling of Git and command output across different system locales by consistently using UTF-8 decoding. * Prevented non-UTF-8 output and filenames from causing failures. * Improved support for non-ASCII and quoted filenames in diff processing. * Ensured unreadable command output is handled safely rather than terminating operations. * **Tests** * Added coverage for locale-independent decoding, unusual filename encoding, and quoted Git paths. * Added checks ensuring text-based subprocess calls consistently specify UTF-8 encoding. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently enforce UTF-8 decoding for text-mode subprocess output and add automated coverage to prevent regressions without introducing functional inconsistencies in the reviewed call sites.
Pull request overview
Promotes develop to main with a repository-wide hardening against locale-dependent decoding in subprocess text-mode calls by explicitly pinning UTF-8 (and selecting errors= behavior where needed), plus adding regression tests to prevent reintroducing the omission.
Changes:
- Pin
encoding="utf-8"on text-modesubprocesscalls across specs, scripts, actions, and tests. - Make git-output decoding resilient where strict UTF-8 is not safe (notably via
errors="surrogateescape"for git reads anderrors="replace"for probes). - Add
scripts/tests/test_tooling_encoding.pyto enforce the UTF-8 encoding invariant across the Python tree.
File summaries
| File | Description |
|---|---|
spec/workflow_reuse.py |
Pin UTF-8 decoding for git rev-parse output used in workflow reuse logic. |
spec/audit.py |
Pin UTF-8 decoding on subprocess reads (git + gh) and use surrogateescape where git output can contain undecodable bytes. |
scripts/tests/test_tooling_encoding.py |
New AST-based test that enforces encoding="utf-8" on all text-mode subprocess spawns in the repo. |
scripts/tests/test_spec_validate.py |
Pin UTF-8 decoding for subprocess-driven spec validation tests (Python + Node probes). |
scripts/tests/test_skills_install.py |
Pin UTF-8 decoding in subprocess calls used by skills installer tests. |
scripts/tests/test_release_guards.py |
Pin UTF-8 decoding for subprocess calls used by release guard tests. |
scripts/tests/test_publish_plan.py |
Pin UTF-8 decoding for publish plan subprocess output capture. |
scripts/tests/test_prose_lint.py |
Expand encoding coverage and add targeted fixtures around locale decoding and quoted diff headers. |
scripts/tests/test_local_review.py |
Pin UTF-8 decoding for git subprocess calls used by local review tests. |
scripts/tests/test_configure_environments.py |
Pin UTF-8 decoding for bash subprocess test helper. |
scripts/tests/test_canonical_review.py |
Pin UTF-8 decoding for git subprocess calls used by canonical review tests. |
scripts/tests/test_build_dist.py |
Pin UTF-8 decoding for subprocess output used in dist build tests. |
scripts/tests/test_bootstrap.py |
Pin UTF-8 decoding for installer and git subprocess reads in bootstrap tests; use surrogateescape where -z paths can be raw bytes. |
scripts/skills_install.py |
Pin UTF-8 decoding for git/claude subprocess output in the installer. |
scripts/host_gate.py |
Pin UTF-8 decoding for tool probes and treat undecodable output as unreadable (errors="replace"). |
scripts/docker_lint.py |
Pin UTF-8 decoding for text-mode subprocess calls that capture output during docker linting. |
scripts/carry.py |
Pin UTF-8 decoding and use surrogateescape for git reads that can emit raw paths. |
host-setup/agent-safety/claude/test_install.py |
Pin UTF-8 decoding for installer test subprocess reads. |
host-setup/agent-safety/claude/install.py |
Pin UTF-8 decoding for git and selftest subprocess reads during install. |
host-setup/agent-safety/claude/gh-write-guard.py |
Pin UTF-8 decoding for git/gh subprocess reads; use surrogateescape where bytes-like refs/paths can be non-UTF-8. |
.github/actions/repo-gate/repo_gate.py |
Pin UTF-8 decoding for subprocess reads in the repo gate; use surrogateescape for -z git output that can echo raw paths. |
.github/actions/prose-gate/prose_lint.py |
Pin UTF-8 decoding for git reads, decode quoted diff headers into real paths, and make stdout/stderr tolerant of surrogateescaped names. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Promotes one commit from
developtomain.09a82e6Name the Encoding on Every Text-Mode Subprocess Call (Name the Encoding on Every Text-Mode Subprocess Call #1574)Python decodes a
text=Truesubprocess pipe with the locale encoding when the call names none. On Linux that resolves to UTF-8 and the default is invisible; on Windows, where UTF-8 mode is off by default, it is the ANSI code page, so output carrying a byte that code page cannot map raisesUnicodeDecodeError. #1538 reports this againstprose_lint.py, whose git reads are the ones a downstream repository's docs send a maintainer through before a push, and whose handler catchesCalledProcessErrorandFileNotFoundError, neither of which a decode failure reaches.Every
subprocesscall in the repository that asks for decoded output now pinsencoding="utf-8", rather than only the file #1538 names. Pinning the encoding left the strict half, soprose_lint.pydecodes withsurrogateescapeandhost_gate.pydecodes its probe withreplace, and the gate's output streams escape a lone surrogate rather than raising part way through a scan. A review pass on that work found a false clean in the production path: git quotes a path holding any byte at or above0x80,changed_linesread the+++header as a literal path, so an ordinary non-ASCII filename left scope, and an empty scope is falsy, so the no-match refusal never fired either. The header is now decoded andcore.quotePathpinned rather than inherited, sincecore.quotePath=falsesuppresses only one of the three routes by which git quotes a name.scripts/tests/test_tooling_encoding.pyasserts the invariant over every Python file the checkout holds, so a call arriving later cannot reintroduce it; nothing else in the toolchain checks it. Every behavioral case was proved against a reverted fix rather than trusted.Three defects of the same class, all confirmed present at the merge base rather than regressions, were filed rather than grown into the change: #1575, #1576 and #1577. #1578 carries the rule into
CODESTYLE.mdfor the fleet, and #1580 records the same omission inrepo_gate.py.Copilot reviewed the feature pull request at its merged head, full coverage (22 of 22 files), approval recommended, no findings and none suppressed. CodeRabbit's incremental pass over the same range raised none.
Closes #1538