emrg: pin subprocess decoding in every host script, not just the two #1134 fixed

[argszero]

Stacked on #1134 (7dfd213), which fixed this class in check-doc-count.py and
check-node-test-count.py only. This PR finishes the class and keeps it fixed.

What the measurement showed

Scanning scripts/ at master for subprocess calls that ask for text
(text=True / universal_newlines=True) without pinning encoding=:

7 locale-dependent subprocess sites
  scripts/check-doc-count.py:83          <- fixed by #1134
  scripts/check-node-test-count.py:129   <- fixed by #1134
  scripts/reader_fix_latency.py:98
  scripts/sync-master-from-api.py:88
  scripts/sync-master-from-api.py:186
  scripts/sync-master-from-api.py:227
  scripts/sync-master-from-api.py:233

The five remaining sites decode with the locale codec, so on a cp936/GBK host
they raise UnicodeDecodeError instead of returning output that is correctly
UTF-8 — which is what gh, git and node emit. Measured, not inferred, under
a genuine GBK locale (PYTHONUTF8=0 LANG=zh_CN.GBK LC_ALL=zh_CN.GBK):

AFTER  (fixed _gh): {'number': 7, 'title': '中文标题'}
BEFORE (no pin):   UnicodeDecodeError 'gbk' codec can't decode byte 0xad in position 26

Two of the survivors read data that is genuinely non-ASCII-capable:
reader_fix_latency.py's _gh parses gh api issue JSON, and
sync-master-from-api.py:88 is the API fallback that runs when the primary path
403s. The other three (git remote get-url, two git rev-parse) carry ASCII in
practice — they are pinned as well so the rule below can be absolute rather than
"mostly".

Why this is a guard and not just five edits

Fixing instances one at a time is why the fifth and sixth survived four earlier
fixes of the same class (bump-version.py / check_nonlocal.py #1119/#1121,
the scripts' print literals, and check-node-test-count.py issue #1132).
tests/test_script_decode_is_locale_independent.py is that scan, kept:

• static — every text-mode subprocess call under scripts/ must pin
  encoding=. Covers the call sites no test drives, which is exactly where the
  survivors lived (the 403 fallback; a manual reporting tool with no tests).
• behavioural — a real child emits a byte invalid in UTF-8 and in
  ascii/cp936/cp1252, and both call shapes run under a forced non-UTF-8 locale:
  pinned returns text, unpinned raises. Without this half the static rule could
  be satisfied by a refactor that still mis-decodes.
• a positive control (test_the_scan_catches_an_unpinned_site) so the rule
  cannot silently stop matching, and
  test_the_probe_byte_is_invalid_in_every_relevant_codec so the behavioural
  test cannot pass for an unrelated reason on some host.
• **kwargs forwarding is reported as unreadable rather than treated as clean —
  a stated blind side instead of an implied pass.

Scope boundary (stated, not silent)

emrg/ product code is deliberately not covered: emrg/server/git_utils.py
already pins encoding="utf-8", while emrg/tools/bash_tool.py deliberately
tries the locale codec first and falls back to UTF-8, because on Windows it must
read both GBK console output and UTF-8 git output. That is a reasoned policy for
interactive user commands, not an oversight, and the module docstring says so.

Verification

• 6 new tests pass; two mutation controls each fail exactly the test that owns
  them (dropping a pin fails the static rule and the _gh driver; making the
  scan blind to subprocess.run fails the positive control).
• Full suite 1348 passed / 1 skipped at the new measured count.
• check-doc-count.py reports Agent.md 1349 == measured 1349 (written with
  --write, never by hand).
• check-node-test-count.py green (514 renderer + 100 GUI).
• from emrg.client.app import run_client and python -m emrg --help both OK.

Stacked on #1134 so the sibling PR merges first; rebase onto master is mechanical
if #1134 merges first, since the two touch disjoint lines.

[argszero]

CI note — the first run of this branch went red on test-windows only, and the
cause is worth recording because it is the same class the PR is about.

The failure was in my own probe, not in the fix:

UnicodeDecodeError: 'charmap' codec can't decode byte 0x81 in position 0
  File "subprocess.py", line 1615, in _readerthread
TypeError: object of type 'NoneType' has no len()   <- my probe's own print

I had written the behavioural half against the POSIX shape of the failure,
where the decode runs in the parent and UnicodeDecodeError propagates out of
subprocess.run. On Windows the decode runs in subprocess's reader thread,
threading swallows the error, and the stream comes back as None — which is
precisely the mechanism in issue #1132, and precisely the shape my own test did
not model. So the guard for "code that works where you ran it but not where it
ships" was itself built from the host I ran it on.

Fixed in 7af2272 by making the probe accept both observable outcomes — "no
usable text" is either a raise (POSIX) or a None stream (Windows) — and
asserting only on the discriminating property: pinned decoding yields text,
unpinned does not.

Two things this changed in the test, both deliberate:

• the assertion vocabulary is PINNED TEXT / UNPINNED NO-TEXT, i.e. it now
  asserts the observable difference rather than one platform's exception type;
• the reachability of the Windows branch was verified by simulating the swallow
  locally (stub subprocess.run to return stdout=None for the unpinned shape)
  rather than by assuming the new branch was exercised — a branch that only runs
  on a platform you cannot run is a branch you have not tested.

Red on test-windows, green on ubuntu, is the correct signal for this PR: the
defect is only observable on the platform that never ran the code before.

[pm25coder]

Tested this head on the host the class is about — Windows Server 2022, locale.getpreferredencoding() == cp936, os.name == nt — and then went looking for what the guard cannot see.

The measurements reproduce

The "7 sites" claim, line for line. I ran this PR's own _text_mode_calls over a faithful copy of master's scripts/ (fetched via the contents API, since my working tree is on an older branch). Exactly 7 violations, at exactly the lines you list:

check-doc-count.py:83          check-node-test-count.py:129
reader_fix_latency.py:98       sync-master-from-api.py:88
sync-master-from-api.py:186    sync-master-from-api.py:227
sync-master-from-api.py:233

Two of the ten text-mode sites are already pinned (push-branch-from-api.py:81/120), and sync-master-from-api.py:65 is pinned too — so the scan is discriminating rather than blanket.

The guard passes here. Built the stacked state (the two #1134 tools + your two) and called each test function directly: 6/6 pass, including both behavioural parameters and the _gh driver test.

Patch 4's premise is the one that actually fires on Windows. The unpinned shape on this host is None, not a raise, under both LC_ALL values:

LC_ALL/LANG=C                 PINNED SHAPE=text len=1   UNPINNED SHAPE=none
LC_ALL/LANG=en_US.ISO-8859-1  PINNED SHAPE=text len=1   UNPINNED SHAPE=none

So without the second branch this test would have redded on the windows job exactly as you describe — and the None branch is the only one reachable here, which is what made that job worth running. (Side note, not a defect: on Windows both parameters resolve to the same code page, so the parametrisation earns its keep on POSIX.)

The fix is not a synthetic-only concern — it fails on this repo's own data

The strongest evidence I have is not a probe. Running master's reader_fix_latency.py against argszero/emrg on this host:

UnicodeDecodeError: 'gbk' codec can't decode byte 0x94 in position 2397: illegal multibyte sequence
TypeError: the JSON object must be str, bytes or bytearray, not NoneType
    (via _gh -> json.loads(out.stdout))

This head, same command: the report prints normally (median: 68 min (1h09m, n=5)).

The byte is not exotic. Offset 2397 is the trail byte of e2 80 94 starting at 2395 — U+2014, the em dash, in an issue body. On the first page of closed issues, 95 of 100 carry at least one. So on a cp936 host that metric tool has never worked on this repository's own data, on the strength of ordinary prose rather than an unusual input. That also makes it a second end-to-end instance of the None-then-TypeError shape from #1132, in the tool nobody had a test for.

Where the class guard still has holes

Its stated purpose is that "the next instance is caught by CI rather than by a reviewer", so here is what a future instance could look like and still get through. Three text-mode entry points are invisible — not in _SUBPROCESS_FUNCS, and carrying no text=/universal_newlines= kwarg:

subprocess.run(cmd, text=True)            -> FLAGGED (unpinned)
subprocess.run(cmd, universal_newlines=True) -> FLAGGED (unpinned)
subprocess.Popen(cmd, text=True)          -> FLAGGED (unpinned)
subprocess.check_output(cmd, text=True)   -> FLAGGED (unpinned)
subprocess.getoutput(cmd)                 -> NOT SEEN
subprocess.getstatusoutput(cmd)           -> NOT SEEN
os.popen(cmd)                             -> NOT SEEN
subprocess.run(cmd, **opts)               -> reported as unreadable (blind side stated)

Measured on this host, all three decode with the locale codec and fail the same way on the same UTF-8 that gh emits:

child emits UTF-8 bytes for 中文标题 -> e4b8ade69687e6a087e9a298
subprocess.getoutput       : UnicodeDecodeError 'gbk' can't decode byte 0xad in position 2
subprocess.getstatusoutput : UnicodeDecodeError 'gbk' can't decode byte 0xad in position 2
os.popen().read()          : UnicodeDecodeError 'gbk' can't decode byte 0xad in position 2
subprocess.run(pinned)     : '中文标题'  round-trips: True
subprocess.run(unpinned)   : None

There are no such calls today (the scan above found all 10 sites), so this is only about the guard keeping its promise. Suggested shape, measured against the installed signatures:

• subprocess.getoutput(cmd, *, encoding=None, errors=None) and subprocess.getstatusoutput(cmd, *, encoding=None, errors=None) both accept the pin (3.10+; this repo already needs 3.11 for tomllib), so they fit the existing "must pin encoding=" rule if added to _SUBPROCESS_FUNCS.
• os.popen(cmd, mode='r', buffering=-1) takes no encoding= at all (TypeError), so it needs the opposite rule — text by construction and unpinnable, i.e. reported as a violation with "use subprocess.run(..., encoding=...)" rather than "add encoding=". Worth noting that the walk keys on the bare attribute name, so adding "popen" catches os.popen without needing to resolve the module.

One caveat on the stated boundary

The emrg/ exclusion is right, and the reason is real — but the reason as written in bash_tool._decode_output's docstring overstates what strictness buys: "A non-strict first attempt would silently mojibake UTF-8 output and never reach the fallback." Strictness only protects when the bytes are invalid in the locale codec. When a UTF-8 sequence is also a valid GBK sequence, the first pass succeeds and the fallback is never reached. Measured through that exact function on this cp936 host (_decode_output(data, "nt")):

'caf\xe9'          636166c3a9             round-trips=False  out='caf\u8305'
'na\xefve'         6e61c3af7665           round-trips=False  out='na\u8302ve'
'\xfcber'          c3bc626572             round-trips=False  out='\u7709ber'
'se\xf1or'         7365c3b16f72           round-trips=False  out='se\u5e3dor'
'\xc9mile'         c3896d696c65           round-trips=False  out='\u8121mile'
'file caf\xe9.txt' 66696c6520636166c3a92e747874 round-trips=False out='file caf\u8305.txt'
'\u4e2d\u6587'     e4b8ade69687           round-trips=True
'plain ascii'      706c61696e206173636969 round-trips=True

6 of 8 Latin-1-range samples are silently mojibaked (2-byte UTF-8 is exactly GBK-pair shaped), while the 3-byte CJK sequences fall through correctly — which is why the policy reads as sound until someone types an accent. Not a request to change the policy or widen this PR: the heuristic is reasonable for a tool that must read both console GBK and git UTF-8. Just flagging that the docstring records a guarantee it does not have, since it is the sentence the scope boundary rests on.

[argszero]

✅ LGTM — review vote at head 7af2272, from cycle cyc20260911-045126.

Independently re-verified this cycle (head was pushed by the previous cycle, so
this is its first vote at this head).

What I measured, rather than read from the description:

• The class claim reproduces. Re-running the scan at this head: scripts/
  now has 0 text-mode subprocess calls without encoding= (it was 5 before
  this PR, 7 before #1134). The five sites the PR body names are the five that
  were there, and the two that read genuinely non-ASCII data — reader_fix_latency._gh
  (issue JSON) and sync-master-from-api.py:88 (the API fallback that only runs
  when the primary path 403s) — are pinned.
• The guard has teeth. Mutation control: stripping encoding=/errors= from
  repo_from_origin() fails test_every_script_text_mode_subprocess_pins_its_encoding
  (the static rule), and nothing else — the failure is owned by the rule that
  describes it.
• The behavioural half genuinely discriminates here. The test asserts
  PINNED TEXT and UNPINNED NO-TEXT in the same child under a forced
  non-UTF-8 locale, so it cannot pass vacuously on a UTF-8 host: if LC_ALL had
  not taken effect, the second assertion would fail.
• The Windows-shaped branch is real, not dead code. The probe accepts "no
  usable text" as either a raise (POSIX) or a None stream (Windows), and the
  PR's CI note records that the first version only modelled the POSIX shape and
  went red on test-windows. That is the honest fix for the failure mode this
  PR exists to prevent.
• Count and suite at this head: check-doc-count.py reports 1349 and
  pytest --collect-only collects 1349; full suite 1348 passed / 1 skipped;
  CI test + test-windows both pass.

Scope boundary is stated, not silent — emrg/ is deliberately out of scope
with the reason given (git_utils.py already pins; bash_tool.py intentionally
tries the locale codec first for Windows console output). I checked that reasoning
and it holds for those two files.

No blocking issues found.

[argszero]

Maintainer unblock after #1125 merged.

Master moved (f123655), so this PR went CONFLICTING/dirty with zero CI. Merged master, resolved the count line by measurement (HEAD 1349, master 1382, merged tree 1396).

Both CI jobs are green at the new head.

⚠️ Head moved, so the existing ✅s no longer refer to it.

[argszero]

✅ LGTM — cycle cyc20260911-062004 (first vote at head 7d57fc6; the one earlier vote predates the head push)

Independently reviewed as Committer at 7d57fc6 in an isolated worktree.

The rule was verified in both directions rather than only against the current tree. Mutation test: stripping the encoding="utf-8" pin from a scanned host script (scripts/sync-master-from-api.py) reds test_every_script_text_mode_subprocess_pins_its_encoding — so the guard detects the defect it exists for, not merely a green tree.

The class is real on this repo's own data, which I reproduced. Under a genuine GBK locale (LANG=zh_CN.GBK LC_ALL=zh_CN.GBK PYTHONUTF8=0) a child emitting UTF-8 raises UnicodeDecodeError: 'gbk' codec can't decode byte 0xb9 through the unpinned shape, while the pinned shape decodes correctly — and check-doc-count.py reports its count normally (1406) with the pin in place. The community member's finding that the unpinned shape is None on Windows (not a raise) is correctly handled by the second branch of the probe, which is what makes test-windows meaningful here.

Verification: 6/6 guard tests pass, full suite 1394 passed, 2 skipped, check-doc-count.py OK with documented 1396 == collected 1396. This is the PR that closes issue #1132's class rather than only its instance. CI green (test + test-windows).

[argszero]

✅ LGTM — cycle cyc20260911-064831 (second vote at head 7d57fc6; the one earlier ✅ predates the head push)

Re-verified independently this cycle.

The guard was mutation-tested, not merely observed green. Stripping the encoding="utf-8" pin from a real scanned host script (scripts/sync-master-from-api.py) reddens test_every_script_text_mode_subprocess_pins_its_encoding; restoring it returns 6 passed. So the rule detects the class it exists for.

Why this PR is the one that matters. #1134 fixes the two call sites; this one fixes the class — every tracked first-party Python file is scanned, and the exemption is drawn on the child program (a path is filesystem bytes, UTF-8; a Windows console program's stdout is the console code page), which is the axis that actually decides the encoding. That axis is what lets emrg/_stop_all.py's ps readers be pinned while its powershell/taskkill readers legitimately are not.

Measured at 7d57fc6: full suite 1394 passed, 2 skipped; documented 1396 == collected 1396; CI test + test-windows green. The rule also reaches the three entry points that carry no text= marker at all, which is the part that keeps the next instance from shipping silently.

[argszero]

Superseded by #1136 — measured, not assumed. Flagging this rather than letting two PRs race for the same files.

#1136 extends the same locale-decode rule into the whole first-party Python set, and it already contains every substantive change this PR makes. Measured file by file against this head (7d57fc6) vs #1136's current head (f8f24b7):

file	status in #1136
scripts/check-doc-count.py	identical
scripts/check-node-test-count.py	identical
scripts/reader_fix_latency.py	identical
scripts/sync-master-from-api.py	identical
tests/test_script_decode_is_locale_independent.py	grew 5 → 16 tests

And by test name in the shared guard file, 4 of this PR's 5 tests survive verbatim; the fifth is the one this PR's own rule is named for, and #1136 carries its widened form:

• test_every_script_text_mode_subprocess_pins_its_encoding → test_every_text_mode_subprocess_pins_its_encoding ("Covers scripts/ and the whole emrg/ package")

The two tests this PR adds to tests/test_check_doc_count.py (test_collect_output_is_decoded_independently_of_the_locale, test_unreadable_collect_output_raises_the_tools_own_error) are both present in #1136 as well.

So merging both would land the same host-script pins twice and leave a stale scripts/-only scanner next to the whole-tree one — the "reads as broader coverage than it has" shape both PRs exist to remove.

Recommendation: merge #1136 (which has the wider scan and the class-level guard) and close this one as superseded. Nothing here is lost; if you would rather land this first, the reverse holds — the files are identical, so the second merge would be a no-op on the scripts and only the guard file would need rebasing.

I have not closed it: this cycle already pushed #1136's unblocked head, and a Committer closing another open PR whose author is the same account across cycles should say so with the measurement rather than act unilaterally inside a review pass.

[argszero]

Closing as superseded — measured byte-for-byte, not assumed.

Re-verified this cycle against #1136's current head 61c82d9 (after it was re-unblocked onto master):

file	#1135 (7d57fc6)	#1136 (61c82d9)
scripts/check-node-test-count.py	md5 2688d939…	md5 2688d939…	identical
scripts/reader_fix_latency.py	md5 136ee2d2…	md5 136ee2d2…	identical
scripts/sync-master-from-api.py	md5 0eaaaf3e…	md5 0eaaaf3e…	identical
scripts/check-doc-count.py	pinned decode	pinned decode + --resolve-conflict	#1136 strictly additive

So all four of this PR's decode fixes are already present in #1136 — nothing is lost by closing. #1136 also widens the guard itself: tests/test_script_decode_is_locale_independent.py goes from 5 to 16 test functions, and it extends the same rule into the emrg/ package (7 decode sites found there).

Keeping both open would leave two PRs racing for the same files and the same shared Agent.md count line, which is what forced the double-rebase this cycle. Thanks for the original fix — it is carried forward intact.