fix(lint): gate unpinned text-mode subprocess decodes, pin git/gh/python children to UTF-8

[chenmingwei23]

Problem / Motivation

A subprocess call in text mode (text=True / universal_newlines=True) with no explicit encoding= decodes the child's output with locale.getpreferredencoding(False) -- UTF-8 on POSIX, but the legacy ANSI code page (cp1252, cp936, cp949...) on Windows. Any non-ASCII byte the child prints -- a path, a commit message, a translated error -- comes back as mojibake, or raises UnicodeDecodeError under strict decoding. #3219 was this class reaching users through the dashboard's file diffs; #3669 fixed the two confirmed sites inline. Nothing prevented the class from growing back: an AST survey found 336 unpinned text-mode calls across 154 files, 84+ of them decoding git/gh output.

Why it matters

Every Windows user with a non-English locale hits this on any code path that shells out to git and shows the result -- file diffs, worktree state, update checks, the auto-improvement pipeline. The two #3669 fixes cover two symptoms; without prevention, every new subprocess call site re-rolls the dice.

What changed (motivation -> approach -> change)

Two prevention pieces, per #5249:

1. A CI lint gate -- scripts/check_subprocess_encoding.py, wired into the backend-lint job (self-test first, same pattern as the brand gate). AST-based so multi-line calls are judged as one call. A call to run/Popen/check_output/check_call/call (plus the repo's kwargs-forwarding wrappers run_limited/popen_limited) that requests text mode without pinning the decode fails. A pin is encoding= with a non-None value or a **UTF8_TEXT splat; encoding=None is rejected because it IS the locale fallback. Deliberately-locale-dependent sites (ps, user shells) opt out with an inline # subprocess-encoding: locale comment -- a real COMMENT token (tokenize-verified), not a substring, so the phrase inside a string literal cannot exempt a call. A file that does not parse is a hard error, never "clean" (the same fail-loud rule the black gate pins).

Existing violations are grandfathered in .github/subprocess-encoding-baseline.txt (<count> <path> lines), mirroring check_black_formatting.py's shrink-only ratchet: files not listed must be clean, counts may not grow, a violation on an ADDED line fails even when the count is level (so fixing one old call while adding a new one cannot slip through), improved counts must be pruned, and --update-baseline only ever lowers counts and deletes lines. New-offender and prune verdicts are scoped to the change's own files, so a PR's colour never depends on someone else's hygiene.

2. A shared decode definition for knowable-encoding children -- src/kiro_crew/subprocess_utf8.py exports UTF8_TEXT, a read-only mapping {"text": True, "encoding": "utf-8", "errors": "replace"} (the shape #3669 established), plus decode_utf8() for binary-mode call sites. It is a mapping rather than a wrapper function for two structural reasons: the test suite patches <module>.subprocess.run by name in dozens of places (a wrapper would route around those patches, turning stubbed tests into real spawns), and test_spawn_audit rightly refuses a new generic spawn primitive with caller-controlled argv -- a mapping spawns nothing.

Converted the verifiably-knowable call sites: ~60 git/gh/Python-interpreter-child sites across the auto-improvement app, package core (cli_server, cloud/source, dashboard handlers, pod, platform, env, dep_sync, transcribe...), and standalone scripts (scripts/, prepare-pr skill scripts -- inline encoding="utf-8", errors="replace", since they cannot import the package). Sites already carrying a deliberate errors= policy kept it (update_capability.py's surrogateescape is untouched; only encoding="utf-8" was added). Diff payloads that round-trip into git apply (auto-improvement capture -> apply chain) use strict encoding="utf-8" with no errors=, preserving the pre-change loud-failure semantics instead of feeding git apply a silently corrupted patch. dep_sync.py keeps its stdlib-only contract (inline kwargs, no import). Everything not verifiably knowable (ps, lsof, systemctl, npm, node, xcrun, ffmpeg, AWS CLI, dynamic argv) stays in the baseline, per the issue's explicit non-goal.

Docs updated in the same commit: ci-and-reviews.md gate table, code-style.md (gate list, pre-commit block, new section), AGENTS.md pre-commit block, testing-conventions.md checkpoint row, and the prepare-pr profile gate floor.

Tests

• test/test_subprocess_encoding_gate.py -- CI wiring (gate present, self-test first), one probe per detector rule family (including encoding=None, dict-literal splat, marker-in-string non-exemption, fatal parse errors), ratchet verdict logic on synthetic inputs (scope filtering, grown counts, the added-line swap case, prune demands), baseline hygiene (refresh never adds a path or raises a count; committed entries all point at existing files; duplicates rejected).
• test/test_subprocess_utf8.py -- the mapping's exact contents and immutability, a real UTF-8 round-trip through subprocess.run(**UTF8_TEXT), malformed-byte replacement, decode_utf8 variants. Child env inherits os.environ (Windows SystemRoot).
• Mutation-verified: breaking the encoding pin, the universal_newlines rule, the encoding=None rule, and the comment-token marker rule each redden the suite; reverting one converted call site reddens the gate itself with the exact file:line.
• Full local gates green at the final SHA: gate self-test (8 flagged / 9 clean probes), gate pass (255 calls in 124 files baselined), black gate, isort, flake8, mypy (1051 files clean), test_spawn_audit, and 1120 tests across the converted modules (auto-improvement suite, prepare-pr scripts, dep_sync, pod, platform_compat, cli_server et al.). Two pre-existing host-env failures in test_source_providers.py reproduce identically at the pristine base commit.
• Two pre-push model-pinned review lanes ran; all findings fixed (spawn-audit conflict resolved by dropping the wrapper functions, payload chain made strict, detector holes closed, doc drift fixed) -- zero overrides.

Any other suggestions

The baseline holds 255 known calls in 124 files; burning it down file-by-file (and marker-annotating the deliberately-locale-dependent set) is mechanical follow-up work that any PR touching those files can carry incrementally.

Closes #5249

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of b2e4f73b681a8309c99b80118f8f624039641df1 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Both design pieces check out: the ratchet mirrors the proven black-gate pattern, and the mapping-instead-of-wrapper choice is grounded in real constraints (test patching by name, test_spawn_audit). The failure modes are fail-loud where it matters (unparseable file, missing baseline) and degrade conservatively elsewhere. Nothing rises above a suggestion.

Design-Verdict: PASS

Prevention gate plus shared pin closes the recurring Windows-mojibake class at its cause; the ratchet and mapping-not-wrapper choices are well-justified.

Suggestions

• _added_lines string-matches scope labels produced by check_black_formatting._changed_paths ("merge HEAD^1..HEAD", "…"); a label respelling there silently degrades the added-line rule to skipped with no red signal — pin the label↔diff-endpoint mapping in the self-test or fail loud on an unrecognized label in CI.

[DESIGN-REVIEWED] b2e4f73

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed b2e4f73b681a8309c99b80118f8f624039641df1 — this comment is updated in place on each push.

Review details

All load-bearing assumptions verify: every file newly referencing os.environ has import os; every **UTF8_TEXT splat replaced a removed text=True with no colliding text=/encoding=/errors= keyword; MappingProxyType splats correctly; the surrogateescape payloads round-trip losslessly through JSON's default ensure_ascii=True; the new gate and its scanned test files are clean under their own rules. The discovery pass found no candidates, and I re-derived nothing that meets the (a)/(b)/(c) bar.

No findings.

[OPUS-REVIEWED] b2e4f73

Verdict parsed from the review's SHA-scoped output markers for commit b2e4f73b681a8309c99b80118f8f624039641df1.

False positive or not applicable? A repository writer can comment:
/ai-review override fable b2e4f73b681a8309c99b80118f8f624039641df1: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of b2e4f73b681a8309c99b80118f8f624039641df1 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- scripts/check_subprocess_encoding.py:258 -- "text_kw" misses errors= alone, which activates locale-decoded text mode and passes CI -> Fix: treat non-None errors= as text mode, including dict splats.
[GPT-REVIEWED] b2e4f73

False positive or not applicable? A repository writer can comment:
/ai-review override gpt b2e4f73b681a8309c99b80118f8f624039641df1: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of b2e4f73b681a8309c99b80118f8f624039641df1 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is in. Composing the review now: the change itself is well-derived (reported defect #3219, prior point fix #3669, issue #5249), the UTF8_TEXT mapping has 20 counted consumers in src/ alone, but two things stand out — the description claims a decode_utf8() helper that exists nowhere in the diff or the tree (grep count: 0), and prove.py/driver.py carry ~150 lines of pure black reformatting that the description never mentions.

First-Principles-Verdict: CONCERNS

The fix is real and counted, but the description ships a decode_utf8() that the diff does not, and ~150 lines of reformatting ride along undeclared.

What this change ships

Intent: stop Windows non-English-locale users getting mojibake from git/gh output, and stop the bug class regrowing — a FIX plus its prevention gate.

1. CI gate fails unpinned text-mode subprocess decodes, self-test first — justified (Windows: UTF-8 output rendered as mojibake (subprocess text=True decodes with cp1252) #3219, Guard the unpinned-text-decode class: lint gate plus a UTF-8-known-writer helper #5249)
2. Shrink-only baseline grandfathers 255 existing calls in 124 files — justified (mirrors black ratchet; reuses its _changed_paths)
3. Shared UTF8_TEXT mapping — justified (20 consumers counted: from kiro_crew.subprocess_utf8 import UTF8_TEXT in src/)
4. ~60 git/gh/Python call sites now decode as UTF-8 — the fix
5. Diff payloads into git apply decode surrogateescape, byte-exact — justified
6. Python children pinned on the emit side (PYTHONIOENCODING) — justified
7. Opt-out comment marker, zero sites carry it yet — justified escape hatch, follow-up declared
8. Docs and pre-commit blocks name the gate — mandated same-commit doc rule
9. prove.py/driver.py black reformatting + black-baseline prune — rides along, undeclared
10. Described decode_utf8() helper for binary-mode sites — absent from the diff

Watch

• The description says the module exports "decode_utf8() for binary-mode call sites" and that tests cover "decode_utf8 variants"; grep for decode_utf8 across the tree and the patch counts 0. Evidently dropped with the wrapper functions ("resolved by dropping the wrapper functions") — the description is stale against what ships.
• prove.py (~100 lines of dict/quote reflow) and driver.py (argv reflow, black-baseline line removed) carry formatting unrelated to encoding. AGENTS.md: baseline-file formatting is "welcome but optional — do it in its own commit."

Subtractions

• Delete the decode_utf8() claims from the PR description — the code correctly shipped without it.
• Move the prove.py/driver.py reformatting and the .github/black-baseline.txt prune to their own commit, per AGENTS.md's own-commit rule.

[FIRST-PRINCIPLES-REVIEWED] b2e4f73

[chenmingwei23]

All three findings from the GPT 5.6 review of 3eb4788 are addressed in 6bce8fb:

BLOCKING 1 (transcribe.py:129 -- decode-side pin without emit-side pin): FIXED. Every converted Python-interpreter child now spawns with env={**os.environ, "PYTHONIOENCODING": "utf-8"} so the child emits UTF-8 into the pipe instead of the Windows ANSI code page: transcribe.py (sysconfig scripts path), dep_sync.py x3 (version probe, installed-package origin -- an absolute path, entry-point resolver), dashboard/handlers/core.py x2 (interpreter probes), cli_server.py (-m kiro_crew setup), cli_doctor.py (venv --version), platform_compat.py (version probe).

BLOCKING 2 (prove.py:141 -- lossy decode corrupts patches before git apply): FIXED. _git (the capture side of the mutation diffs) and both git apply input sites now use errors="surrogateescape", which round-trips non-UTF-8 bytes exactly; the chain is in-process only (no JSON persistence), so surrogates never escape into a serializer. The pytest-output site keeps errors="replace" -- that output is display-only.

FINDING (gate: **{"text": True, "encoding": None} reads as clean): FIXED. _splat_kind now treats a dict-literal splat whose encoding key is missing OR the literal None as unpinned, matching the keyword-argument rule; new self-test probe (--test now 9 flagged / 9 clean) and a matching pytest case added.

Local verification at 6bce8fb: gate self-test + gate pass, black gate pass, flake8/isort clean on all touched files, 403 tests across the affected modules green.

[chenmingwei23]

All three First Principles findings on 6bce8fb are addressed in cdc0db4:

BLOCKER (.heartbeat rides along): FIXED. The file is a local pipeline-agent liveness artifact that was re-written into the worktree between my first removal and a later git add -A. Deleted from the commit and the tree, and the worktree's info/exclude now lists it so no future staging can pick it up.

Subtraction 1 (code-style.md names run_utf8/check_output_utf8/popen_utf8): FIXED. The stale clause is gone -- those wrappers were removed for test_spawn_audit and the doc now describes only the mapping, plus the child-side PYTHONIOENCODING rule that round 1 added.

Subtraction 2 (decode_utf8 has zero consumers): FIXED. Removed from subprocess_utf8.py along with its tests; the module now ships exactly what the conversions use -- the UTF8_TEXT mapping.

Verification at cdc0db4: docs lint, gate self-test + gate, black gate, flake8, and the module/gate test suites all green.

[chenmingwei23]

GPT 5.6 finding on cdc0db4 (proposer.py _capture_diff strict decode crashes on non-UTF-8 text diffs): FIXED in b2e4f73 -- and it settles a policy the two review lanes had disagreed on.

Round 1 chose strict decode for the payload chain to preserve the pre-change loud-failure semantics. GPT's trigger path is right that "loud" here means aborting the whole improvement cycle on a legitimately non-UTF-8 fixture the candidate merely touched -- worse than the disease. Before switching, I verified the one hazard that made me avoid surrogateescape initially: the payload persists through the JSON ledger. Probe (run in-repo): b'line \xff end'.decode('utf-8','surrogateescape') survives json.dumps (default ensure_ascii=True escapes the surrogate losslessly) -> UTF-8 file write -> json.loads -> encode('utf-8','surrogateescape') returns the exact original bytes.

So the full chain is now errors="surrogateescape" end-to-end: _capture_diff (proposer.py), both git apply inputs (driver.py), and the queued-diff git apply --index (backend/commit.py). No crash on non-UTF-8 bytes, no U+FFFD corruption, byte-exact round-trip -- and it matches the policy subprocess_utf8.py's docstring documents for payloads.

Also in this push: check_black_formatting.py --update-baseline pruned spine/driver.py, which became black-clean as a side effect (the reformat is confined to the edited call sites; total driver.py diff vs base is 34 lines).

Verification at b2e4f73: auto-improvement suite 965 passed, subprocess-encoding gate + self-test pass, black gate pass, flake8 clean.