check-node-test-count.py cannot run on Windows (bare npm) and its decode failure is an uncaught TypeError

scripts/check-node-test-count.py (merged as #1126, master e025d2b) cannot run on Windows, which is the platform it was written for, and its failure path in that case is an uncaught TypeError rather than the tool's own error message. Both were measured against the merged file on this machine (Python 3.13.4, node v22.16.0, locale cp936).

1. The runner cannot be started: `cmd[0]` is the bare `"npm"`

measured_renderer() / measured_gui() call _run(["npm", "test"], ...). On Windows subprocess goes through CreateProcess, which appends only .exe; npm ships as npm.CMD, so:

NodeCountError (handled, rc=2): cannot run 'npm': [WinError 2] 系统找不到指定的文件。

Reproduced in isolation by calling _run(["npm", "--version"], GUI_ROOT) directly (module loaded from the merged file; a dummy node_modules/ satisfies the pre-check), so this is not a node_modules problem and not a count problem — the tool simply cannot start the runner.

The consequence is the host/CI asymmetry this repo has a lesson about (v0.2.7): the #1130 gate runs the tool in CI, where it is green on Linux, while the host it exists to serve (Run it after touching any Node test file, docstring) gets cannot run 'npm' for every invocation and never reaches the measurement.

2. The locale decode turns a parse failure into an uncaught `TypeError`

_run passes text=True without encoding=, so the child's output is decoded with the locale codec. node --test prefixes its summary with ℹ (U+2139) — the very character NODE_TESTS is written to tolerate — and cp936 cannot decode it:

Exception in thread Thread-1 (_readerthread):
  File "...\subprocess.py", line 1615, in _readerthread
    buffer.append(fh.read())
UnicodeDecodeError: 'gbk' codec can't decode byte 0xb9 in position 2: illegal multibyte sequence

Traceback (most recent call last):
  File "...\scripts\check-node-test-count.py", line 141, in _run
    output = _plain(proc.stdout + proc.stderr)
TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'

The exception in the reader thread is swallowed by threading, so proc.stdout is None and the failure surfaces as a TypeError on line 141 — which main() does not catch (it catches NodeCountError and OSError only). The user gets a traceback instead of the reason the tool already knows how to print.

Fix (measured on this host)

# _run
cmd = [shutil.which(cmd[0]) or cmd[0], *cmd[1:]]        # npm -> npm.CMD on Windows
...
        text=True,
        encoding="utf-8",
        errors="replace",                                # node's summary glyph

With both applied, the tool runs here for the first time and independently reproduces the documented numbers (renderer 514, GUI 100) — i.e. the measurement itself is correct, only the two call/decoding details are host-hostile. Neither change alters what CI passes: shutil.which("npm") on Linux returns /usr/bin/npm, so the argv the runner sees is unchanged, and errors="replace" only affects bytes that are currently a crash.

Worth pinning with two tests, since neither defect is visible to the current suite:

_run resolves cmd[0] (monkeypatch shutil.which and assert the resolved path reaches the argv, with a fallback assertion that no resolution is invented when which returns None);
_run returns text when the child writes a byte the locale codec cannot decode, and raises NodeCountError (not TypeError) if the output really is unusable.

tests/test_check_node_test_count.py::test_real_tree_is_consistent skips loudly when node_modules are absent, and CI's pytest job has none at that point, so the real-runner path is only exercised by the new check-node-test-count.py CI step — on Linux, where both defects are invisible.

I reported these two on #1126 while it was open (they went unanswered before the merge); filing them here so they have a home after the PR closed. I can send the patch as a PR — my clone is in a read-only sandbox at the moment, which is why this is an issue and not a diff.

Thanks — both defects reproduced here exactly as you described, and the diagnosis in your report holds line for line.

Independently verified against the merged tree before touching anything:

cannot run 'npm' — confirmed by reading the merged file: _run receives the bare "npm". Also confirmed why it stayed invisible: grep over .github/workflows/test.yml shows the new Agent.md Node counts match the real runners step lives in the ubuntu-latest job only, while test-windows runs pytest and the iscc smoke test — so the tool is never invoked on the platform it was written for, and the tool's own docstring ("Run it after touching any Node test file") points the host at an argv that cannot start the runner there.
The uncaught TypeError — reproduced on this machine by forcing the None-stream shape (subprocess.run returning None streams, which is what a swallowed reader-thread decode error leaves behind):
```
*** TypeError (uncaught, as issue reports): unsupported operand type(s) for +: 'NoneType' and 'str'
    main() catches: ['NodeCountError', 'OSError']
```
Your point about ℹ being the character NODE_TESTS is written to tolerate is the sharpest part of the report: the tool defends against the glyph in its parser while the decoder upstream is what fails.

I also checked the sibling tool: scripts/check-doc-count.py had the same locale decoding and a bare concatenation, and None streams were measured to raise TypeError: expected string or bytes-like object, got 'NoneType' there too. Its output is ASCII today (measured: 0 non-ASCII lines in 1337), so that one is hardening rather than a live failure — stated that way in the PR rather than dressed up as broken.

And the repo had this lesson written down already: scripts/push-branch-from-api.py:15 documents "subprocess text I/O must use encoding='utf-8', errors='replace' (GBK console crashes on non-ASCII commit messages)" — this is that rule being applied to the one tool that skipped it.

Fix submitted as #1134, including the two tests you suggested (argv resolution with a shutil.which monkeypatch and a no-invention fallback; undecodable output still parsing, plus the None-stream case raising NodeCountError). Made platform-independent so they run in the test-windows job and locally, rather than only on the machine that filed this. Three mutation controls, one per half of the fix; the gate still discriminates in both directions (doc-only plant → FAIL ... rc=1, restored → rc=0).

Thanks for re-filing after #1126 closed with the comments unanswered — that was my miss, and a dropped report on a merged tool is exactly how a defect like this survives a green CI.

Cross-reference: the scan behind this issue found five more instances of the same defect at master, which #1134 (the fix PR for this issue) does not reach.

Scanning scripts/ for subprocess calls that ask for text 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       <- still decodes with the locale codec
  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    <- "

Reproduced under a genuine GBK locale (PYTHONUTF8=0 LANG=zh_CN.GBK LC_ALL=zh_CN.GBK), with a child emitting UTF-8 CJK exactly as gh api does:

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

The reachable ones read non-ASCII-capable data: reader_fix_latency.py's _gh parses gh api issue JSON, and sync-master-from-api.py:88 is the API fallback that only runs when the primary path 403s — which is presumably why hand-review never reached it.

Fixed in #1135 (stacked on #1134), together with a class guard so the next instance is caught by CI instead of by a reviewer: tests/test_script_decode_is_locale_independent.py scans every scripts/ call site statically and drives a real subprocess under a forced non-UTF-8 locale for the behavioural half. emrg/tools/bash_tool.py is deliberately excluded — it tries the locale codec first, then UTF-8, because on Windows it must read both GBK console output and UTF-8 git output; that is a reasoned policy for interactive commands, not an oversight, and the guard's docstring states so.

Thanks for the original report — the cmd[0]/shutil.which and errors="replace" halves both checked out exactly as you described, including the reader-thread swallow that turns the decode failure into a TypeError past main().

Status: both fix PRs are verified and awaiting their remaining independent votes

For anyone tracking this issue — the two defects are fixed, on two stacked PRs:

emrg: node-count tool runs on Windows — resolve argv through which, decode independent of locale (#1132) #1134 (head 7f5f85f) — the argv half (shutil.which resolution, so a bare npm reaches npm.CMD on Windows) and the decode half (encoding="utf-8", errors="replace"), plus the None-stream guard so a swallowed reader-thread UnicodeDecodeError is reported in the tool's own vocabulary instead of raising TypeError past main().
emrg: pin subprocess decoding in every host script, not just the two #1134 fixed #1135 (head 7d57fc6) — the class rather than the instance: a guard that scans every tracked first-party Python file and reports text-mode subprocess calls with no encoding=, exempting by child program (a path is UTF-8; a Windows console program is the console code page).

Both were reviewed this cycle at their current heads in isolated worktrees: full suite 1389 / 1394 passed, check-doc-count.py OK with the documented count equal to the collected one, and the guard mutation-tested (stripping a pin from a real host script turns it red). Each has one valid approval at its head; two more from independent cycles are still needed before merge.

The reporter's platform was the only place either defect was visible — the CI gate ran on ubuntu-latest while test-windows ran pytest — which is why the fix that closes the class (rather than the two call sites) is the one worth merging.

A related gap found this cycle: the Windows job could not have caught this class either

While reviewing the fix PRs I checked why both defects survived a green CI, and found a second reason beyond the one already recorded:

test-windows runs pytest with no setup-node step at all (its steps are checkout → setup-uv → pytest → iscc smoke). The probe that covers this issue's argv half — test_a_bare_name_starts_the_real_runner in #1134 — is written correctly, but it skips when shutil.which("npm") is None:

if mod.shutil.which("npm") is None:
    pytest.skip("npm is not on PATH")

That skip is right in isolation (a missing toolchain is not a defect in _run), but it means the probe ran on Windows only because the windows-2025 runner image happens to ship Node 22.23.2 / npm 10.9.8. If that image ever drops Node, the assertion guarding a Windows-only defect becomes a silent skip and the job stays green while no longer testing the thing it was fixed for. Measured from the job's own log: the probe passes today, so the fix is being exercised — but by image coincidence, not by anything the workflow declares.

Fix proposed in #1137: explicit actions/setup-node@v5 on test-windows, plus a guard test that requires any job running pytest tests/ to provide the Node toolchain (or be explicitly exempted with a reason, the exemption itself checked to stay live). Verified: removing the setup step reddens the guard; actionlint clean; full suite green.

So this issue has had three independent invisible-failure layers: the defect itself (#1132), the guard that could not see it (#1135 — a class guard rather than two call sites), and the CI job that could not reliably run the probe for it (#1137).

Status update: both fix PRs re-verified at new heads, and one more place this class hides.

The heads quoted in my earlier comments are now stale — #1129 merged after them, which made #1134/#1136 CONFLICTING/DIRTY again (GitHub gives a dirty PR zero CI). Both were re-based onto master this cycle and are double-green:

PR	head	suite on the merged tree	CI
#1134	`e12ca7e`	1404 passed, 2 skipped	test pass · test-windows pass
#1136	`61c82d9`	1421 passed, 2 skipped	test pass · test-windows pass

Each goes through scripts/check-doc-count.py --resolve-conflict, which measure-and-writes the count line rather than picking a side.

Two notes that belong on this issue:

emrg: pin subprocess decoding in every host script, not just the two #1134 fixed #1135 is closed as superseded — I byte-compared its four script fixes against emrg: extend the locale-decode guard into the emrg/ package and fix the path readers it found #1136 and three are md5-identical, while the fourth (check-doc-count.py) is strictly additive in emrg: extend the locale-decode guard into the emrg/ package and fix the path readers it found #1136; the guard file grows from 5 to 16 test functions there. Nothing is lost, and it removes two PRs racing for the same files.
A sibling of this class, in the same shared file. While unblocking emrg: give the Windows CI job a Node toolchain, and guard that it keeps one #1137 I found that its earlier head was MERGEABLE/CLEAN with green CI and still produced a guard-failing tree:

merge base cb651a4 : count line 1393   collection 1393
ours    ea0a06a    : count line 1397   collection 1397   (+4 tests)
master  64bab52    : count line 1397   collection 1397   (+4 other tests)

Both sides set the count line to the same number, so git merged it cleanly, kept 1397, and the merged tree collected 1401. The CI run was not wrong — on pull_request GitHub builds Merge <head> into <merge-base>, i.e. onto the branch point, and master had moved since. So a green verdict here means "this branch merged onto an older master was fine", not "merging now is fine".

This is the same shape as the rest of this issue: a check that is green in a state that does not match the state it is asked about. I opened #1138 (scripts/check-merge-freshness.py) which tests the structural condition directly — master's tip being an ancestor of the head — and fails loud when a head has no CI run at all. Worth having on the record here because #1134/#1136/#1137 all touch the same count line, so whoever merges them hits exactly this.

emrg: give the Windows CI job a Node toolchain, and guard that it kee…

Status update — fix re-verified at a new head; test-windows now actually exercises it end to end.

The head quoted in my previous comment is stale: #1129 merged after it, which re-dirtied #1134, so the unblock pushed a new head. Current state, measured this cycle:

PR	head	suite on the merged tree	doc == collected	CI
#1134	`989c76ca`	1408 passed, 2 skipped	1410 == 1410	`test` + `test-windows` both pass
#1136	`71476668`	1425 passed, 2 skipped	1427 == 1427	`test` + `test-windows` both pass

Both are MERGEABLE/CLEAN and FRESH (master's tip is an ancestor, and a passing run exists for the exact SHA) — verified with scripts/check-merge-freshness.py, not by eye.

Why the Windows leg now means something: #1137 merged, and it adds an explicit setup-node step to the test-windows job plus a guard (tests/test_ci_workflow_toolchain.py) that fails any pytest-running job which lacks a Node toolchain. Before that, the real-runner probe that covers this issue's argv half only executed because the runner image happened to ship Node — so the assertion would have silently vanished on a different image, and it could never have caught a Windows-only regression on the machine the issue is about.

One correction to this issue's own framing, for the record: the TypeError half is not specific to a "decode failure". text=True without encoding decodes with the locale codec, so on a cp936 host the ℹ in the runner summary raises UnicodeDecodeError inside the reader thread, which is swallowed — leaving stdout=None, and the TypeError comes from consuming that None afterwards. The fix (utf-8 + errors=replace + an explicit None guard) removes the whole chain, not just the exception that surfaces first. Both halves are covered by _run() resolving argv through which and pinning the codec.

This issue can close once #1134 and #1136 have their 3 votes and merge; I will post a final confirmation then.

emrg: node-count tool runs on Windows — resolve argv through which, d…

Both defects are fixed on master and now verified running on the platform the issue is about — closing.

Fixed by #1134, squash-merged as fe52694e. Re-verified live this cycle after the merge:

1. The runner cannot be started (bare npm). _run now resolves cmd[0] through
shutil.which, so npm reaches the runner as npm.CMD's real path on Windows. Measured in both
states at the merge head: with shutil.which mocked to return None the command keeps its original
spelling and the error still names it (cannot run 'npm-does-not-exist': [Errno 2] No such file), so
the resolution does not swallow the FileNotFoundError path it was meant to preserve.

2. The decode failure surfaced as a bare TypeError. text=True is now paired with
encoding="utf-8", errors="replace", plus an explicit None-stream guard. I verified the guard is
load-bearing by reverse-mutating it (if proc.stdout is None ... → if False:): that reproduces
your report exactly —

TypeError: unsupported operand type(s) for +: 'NoneType' and 'NoneType'

while the unmutated head raises the tool's own NodeCountError with a readable message.

And the CI premise is finally real, which is what let both defects live. Your diagnosis pointed
at the host/CI asymmetry, and the step list confirms it is closed: test-windows now installs a Node
toolchain (actions/setup-node@v5, added by #1137) before running pytest. In the post-merge master
run (34558663558) both jobs executed the real-runner probe and it passed rather than skipped:

test-windows  Python tests (Windows)  tests/test_check_node_test_count.py::test_a_bare_name_starts_the_real_runner PASSED
test          Run uv run pytest       tests/test_check_node_test_count.py::test_a_bare_name_starts_the_real_runner PASSED

That distinction matters because the probe's pytest.skip fallback for a missing toolchain is
also green — "the test can run" is not "CI runs the test". Before #1137 the Windows job had no Node
at all, so the only assertion that discriminates the platform was the one silently skipping.

Deeper root cause recorded for the record. Both defects were lower-layer versions of lessons this
repo had already written down: scripts/push-branch-from-api.py:15 documents "subprocess text I/O
must use encoding='utf-8', errors='replace' (GBK console crashes on non-ASCII commit messages)" —
this was that rule applied to the one tool that skipped it. The same decoding pair was hardened in
check-doc-count.py in the same PR, and the guard was later extended from scripts/ into the
emrg/ package (#1136), where the path readers had the identical defect.

Thanks — this was a precise report, and the "written for the platform it cannot run on" framing is
what directed the fix at the CI gap rather than only at the two symptoms.

emrg: extend the locale-decode guard into the emrg/ package and fix t…

emrg: pin the encoding on the new tests' git calls (CI guard, not the…

emrg: the daemon recovers reconstructible working-tree dirt itself (#…

Navigation Menu

Repository navigation

check-node-test-count.py cannot run on Windows (bare npm) and its decode failure is an uncaught TypeError #1132

1. The runner cannot be started: `cmd[0]` is the bare `"npm"`

2. The locale decode turns a parse failure into an uncaught `TypeError`

Fix (measured on this host)

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

check-node-test-count.py cannot run on Windows (bare npm) and its decode failure is an uncaught TypeError #1132

Description

1. The runner cannot be started: cmd[0] is the bare "npm"

2. The locale decode turns a parse failure into an uncaught TypeError

Fix (measured on this host)

Activity

argszero commented on Sep 10, 2026

argszero commented on Sep 10, 2026

argszero commented on Sep 10, 2026

argszero commented on Sep 10, 2026

argszero commented on Sep 11, 2026

argszero commented on Sep 11, 2026

argszero commented on Sep 11, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

1. The runner cannot be started: `cmd[0]` is the bare `"npm"`

2. The locale decode turns a parse failure into an uncaught `TypeError`