test: read setup.cfg as utf-8 in the ci-surface testpaths guard

[leonlaiyc]

Problem / Motivation

test/test_ci_surface_tests.py reads setup.cfg through configparser without
naming an encoding:

parser = configparser.ConfigParser()
parser.read(_REPO_ROOT / "setup.cfg")

ConfigParser.read opens with no encoding, so the file is decoded with
locale.getpreferredencoding() — UTF-8 on POSIX, but the legacy ANSI code page
on Windows. setup.cfg contains non-ASCII: 48 bytes of UTF-8 em dashes in its
comments. On a Windows host whose code page is not UTF-8 — any CJK install — that
decode raises.

Measured on origin/main 6598107cb, unmodified, on a cp950 host:

FAILED test/test_ci_surface_tests.py::test_backend_roots_cover_every_configured_testpath
  UnicodeDecodeError: 'cp950' codec can't decode byte 0xe2 in position 514:
  illegal multibyte sequence

Two consequences, and the second is the one that matters:

1. The suite cannot run clean on such a host, so a contributor there cannot use
   python -m pytest as AGENTS.md prescribes.
2. The contract this test exists to enforce is never checked. It is the guard
   that every setup.cfg testpaths entry is a scanned selector root — a test
   under an unenumerated root is not "unclassified but still running", it never
   runs at all. The test dies at the read, before it reaches its own assertion, so
   on that host it protects nothing while still reporting as a failure for an
   unrelated-looking reason.

This is the same failure class the repository already guards elsewhere:
scripts/check_subprocess_encoding.py exists precisely because
locale.getpreferredencoding decoding produced mojibake and UnicodeDecodeError
for users (issue #3219, sites fixed in #3669, gate added in #5249). That gate
covers subprocess pipes. This is the same decode reached through a file read,
which it does not cover.

Why it matters

The testing contract in AGENTS.md is python -m pytest, unqualified. A
contributor on a CJK Windows box currently gets a red suite out of the box, from a
UnicodeDecodeError that names neither the real cause nor anything they changed —
the kind of failure that reads as "the repo is broken here" rather than "one read
is missing an argument".

The silent half is worse than the noisy half. test_backend_roots_cover_every_configured_testpath
is a guard against tests silently never running. On the affected hosts it is
itself silently not running, so the guard and the thing it guards fail together.

What changed (motivation → approach → change)

Root cause: a decode that inherits the host code page for a file that is
committed as UTF-8.

Approach: pin the encoding at the read, and pin the two facts that make it
load-bearing so the guard cannot rot on the machines that never feel it.

• parser.read(_REPO_ROOT / "setup.cfg", encoding="utf-8") — the file is
  UTF-8 in the repository, so it is read as UTF-8 everywhere. One argument, with
  a comment recording the failure class and pointing at the existing subprocess
  gate so the next reader does not have to rediscover why it is there.
• test_setup_cfg_carries_non_ascii_so_its_read_must_pin_utf8 — asserts the
  file really does carry non-ASCII, that those bytes really are undecodable under
  a legacy code page, and that the pinned read works. All three hold on every
  host, including the UTF-8 runners that cannot exercise the bug itself.

Deliberately NOT a repo-wide "missing encoding=" rule.
.github/workflows/cross-platform.yml ships no such rule and documents why: a
line regex breaks on nested calls (write_text(json.dumps(x), encoding=...)) and
on multi-line calls under --unified=0. An AST version would not have those
limits, but it means editing .github/workflows/**, which turns a one-argument
portability fix into a workflow-security change. This stays the size of the
defect; the broader gate is a separate decision for a maintainer.

Scope boundary, stated because the file makes it look tempting. Lines 280–282
of this module contain the string 'PARSER.read(ROOT / "setup.cfg")\n'. That is a
fixture written to a temp file so the surface selector can pattern-match it —
it is never executed, and editing it would change what
test_own_surface_config_references_stay_single_surface asserts. It is
intentionally left byte-identical.

Tests

Red-before, on unmodified origin/main 6598107cb:

FAILED test/test_ci_surface_tests.py::test_backend_roots_cover_every_configured_testpath
  UnicodeDecodeError: 'cp950' codec can't decode byte 0xe2 in position 514

Green-after, same host, same command: 2 passed (the repaired test plus the new
guard). Whole module: 45 passed, 1 failed — and that one failure,
test_explicit_cli_target_bypasses_collect_ignore
(AssertionError: recursive collection should honour collect_ignore), reproduces
identically on pristine origin/main with this branch's changes reverted, so it
is inherited by this environment and not attributable to this diff.

The honest limit of the red-before: the defect is host-conditioned. On a UTF-8
runner the repaired test passes before and after, because
locale.getpreferredencoding() already returns UTF-8 there. CI therefore cannot
show this one going red. That is exactly why the second test exists: its three
assertions — non-ASCII present, undecodable under cp950, decodable as UTF-8 —
hold on every host, so if setup.cfg ever becomes pure ASCII the guard fails
loudly and can be retired deliberately rather than quietly becoming decoration.

The cp950 decode is asserted with pytest.raises(UnicodeDecodeError) rather
than assumed, so the guard cannot pass vacuously on a build where those bytes
turned out to be decodable.

Gates: flake8 and isort --check-only clean on the changed file.
scripts/check_black_formatting.py passes — the file is in the repository's black
baseline (its base revision is already unformatted), so it is deliberately not
reformatted; running black on it would bury a 42-line diff under an unrelated
rewrite.

Manual verification

N/A — unit coverage sufficient: the defect reproduces and is fixed under
python -m pytest on the affected host, which is the same command a contributor
would run by hand.

Related Issues

None. Found while auditing locale.getpreferredencoding decode sites for the
failure class scripts/check_subprocess_encoding.py guards (#3219 / #3669 /
#5249), for the case where that decode is reached through a file read rather than
a subprocess pipe.

File-level overlap, disclosed: open PR #9223 (feat(aws-control): the crew container image and its runtime) also edits this file. The hunks are disjoint —
#9223 touches the imports, a reformat around line 318, and
test_explicit_cli_target_bypasses_collect_ignore around line 438; this PR
touches the setup.cfg read around line 134 and adds a test beside it. Whichever
lands second should need only a line-offset rebase. Worth noting that #9223's own
hunk at line 318 already reads with read_text(encoding="utf-8"), which is the
same discipline this PR applies to the one site in the module that still lacked
it.

Pattern harvest

Rule candidate: review-prompt

Pattern: a missing encoding= is only latent until you can name a writer that
emits non-ASCII. Most of this repository's unpinned reads are genuinely harmless
because json.dumps defaults to ensure_ascii=True, so the bytes on disk are
ASCII whatever the code page. The way to tell a real one from a theoretical one is
not to count call sites — it is to open the target file and look for a byte above
0x7F. Here there were 48 of them, committed, in setup.cfg.

Second lesson, about where these hide: a test that dies before its assertion is
a guard that is not running. The visible symptom was a red suite; the invisible
one was that a contract test against "tests that silently never run" was itself
silently not running. When triaging a decode error inside a test, ask what that
test was supposed to be checking before deciding the failure is cosmetic.

Third, for review prompts: an existing gate marks a class the project has
already agreed is real, and its scope boundary is where the next bug lives. The
subprocess encoding gate exists because this exact decode reached users. Its
edge — file reads — is where this one was.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

🤖 Generated with Claude Code

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 19a19b2a4b651e3b4ce0f6aed0fe4b9f4ffe8c71 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 19a19b2

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 19a19b2a4b651e3b4ce0f6aed0fe4b9f4ffe8c71 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Verified against the base tree: setup.cfg genuinely carries non-ASCII bytes (em dashes and even CJK text at line 134), and the base test_ci_surface_tests.py:135 reads it with no encoding — the stated defect is real, the fix is the root cause (pin the decode to the committed encoding), and the scope is deliberately held to the one defective site with the repo-wide gate explicitly deferred. The "Manual verification: N/A" line is backed by a concrete red-before/green-after run on the affected cp950 host in the Tests section, so the platform-branch trigger doesn't fire.

Design-Verdict: PASS

Root-cause one-argument fix for a verified host-conditioned decode, with a premise guard that keeps it honest on runners that can't reproduce it; proportionate scope.

[DESIGN-REVIEWED] 19a19b2

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 19a19b2a4b651e3b4ce0f6aed0fe4b9f4ffe8c71 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 19a19b2

[leonlaiyc]

CI attribution for the red on 7b0db9295 — inherited, not branch-caused. No SHA churn, and please do not rerun for green.

Causal chain, read rather than inferred:

• Run 34126818846, job 101759971693 — the single failing child is Backend Tests (Windows) (4). Every other child passes.
• Tree tested: Merge 7b0db9295b2d969d36d880adf9ac5d7138fd7088 into 6598107cbf4e13f9c4dead615292cfbafae936a8 (read from the checkout step, not assumed).
• Actual failure: test/test_work_ledger.py:1367 :: test_two_conductors_binding_one_worker_at_once_yield_exactly_one_binding — AssertionError: assert 2 == 3.

This branch cannot reach it. The diff is one file, test/test_ci_surface_tests.py, which imports only importlib.util, re, pathlib and pytest, defines one local fixture, and neither imports work_ledger nor touches shared state. The failing test is a four-thread race over a lock file in an unrelated module.

Upstream has already identified this exact failure. 7dd090fb6 — "test(work-ledger): name the cause when the binding race test shortfalls" (#9257) — opens by naming the same test, the same job, and the same message:

fails on Backend Tests (Windows) (4) intermittently and reports only "assert 2 == 3"

Its diagnosis: the bind helper caught only WorkLedgerError, so anything else — a bare OSError from a Windows sharing violation, per #9250's docstring — killed the thread silently, appended nothing, and shortened the count.

Two things follow, and the second matters more than the first:

1. #9257 is not in this run's base. git merge-base --is-ancestor 7dd090fb6 6598107cb → false; the base landed 12:15 UTC and test(work-ledger): name the cause when the binding race test shortfalls #9257 landed 16:06 UTC, ~2.5 h after this run finished.
2. test(work-ledger): name the cause when the binding race test shortfalls #9257 does not fix the flake. Its own message is explicit: "The == 1 and == 3 assertions are unchanged: this adds diagnosis, not tolerance." So rebasing onto it would not make this green — it would make the next Windows failure print the offending exception type instead of a bare count.

I am therefore not rebasing and not pushing. A rebase here would churn the SHA, discard four clean exact-head reviews (GPT 5.6, Opus 4.8 and Design all report no blocking findings on 7b0db9295), and buy a better error message rather than a pass. If a maintainer wants the improved diagnostic on this branch before merge, say so and I will rebase — but on the current evidence the red belongs to the known intermittent work_ledger binding race on the Windows shard, not to this diff.

[leonlaiyc]

Correcting my previous comment: the red was a real defect, not an intermittent, and a rebase does clear it. Rebased to 19a19b2a4.

I anchored that attribution on 7dd090fb6 (#9257), whose commit message describes the same test and the same assert 2 == 3 and says it "adds diagnosis, not tolerance" — and I concluded from that alone that no fix existed and a rebase would only buy a better error message. I stopped one commit short. 2b379bad9 (#9237), "fix(work-ledger): stop the lock-file open from truncating on Windows", is the actual fix, and issue #9248 states plainly that this failure is not flaky:

The work-ledger instance manifested as test_two_conductors_binding_one_worker_at_once_yield_exactly_one_binding asserting 2 == 3, which reads like a flaky test and is not one.

The mechanism: a lock file opened open(path, "w") truncates before the acquire. On Windows the acquire routes to msvcrt.locking, so a contending process crashes out of the critical section with a bare OSError — a bind thread dies, its outcome is never recorded, and the count comes up one short. #9257 then made that shortfall name its exception instead of reporting a bare count, which is why its message describes the symptom so exactly while changing no behaviour. Reading them in the wrong order is how I got this backwards.

Timeline, which is what makes it a stale base rather than a live defect here:

	UTC
this PR's CI base 6598107cb	12:15
CI run 34126818846 ran	13:29 – 13:47
#9237 (the fix) merged	14:03
#9257 (diagnostics) merged	16:06

git merge-base --is-ancestor 2b379bad9 6598107cb → false. The fix landed 16 minutes after the failing job finished.

What stands from the previous comment: the failure is still not branch-caused. This diff is one file, test/test_ci_surface_tests.py, which imports only importlib.util, re, pathlib and pytest and never touches work_ledger. What was wrong was the remedy, and that is the half that mattered.

Action taken: rebased onto 2f9ed9724, which contains #9237 (git merge-base --is-ancestor 2b379bad9 HEAD → true). The rebase is content-free — git patch-id --stable is identical before and after (f11ffd9a0a534e3666a89d14655bfecddcc9118b), so the only thing that changed is the base the shard runs against.

[leonlaiyc]

Post-rebase CI on 19a19b2a4: the original work_ledger failure is gone; the remaining red is base-side.

The rebase did what it was meant to. test_two_conductors_binding_one_worker_at_once_yield_exactly_one_binding no longer fails — #9237's non-truncating lock open is in this base, which is what the previous comment predicted.

What fails now on Backend Tests (Windows) (3) is different and unrelated:

test/test_security_conductor_skill_contract.py:125: AssertionError: assert not True
AttributeError: module 'os' has no attribute 'killpg'

This PR's entire diff is one file, test/test_ci_surface_tests.py, which imports only importlib.util, re, pathlib and pytest. It cannot reach the security-conductor skill contract, and nothing here kills a process group — os.killpg does not exist on Windows at all.

The decisive evidence is that the identical assertion fails on my #9351, whose diff is disjoint from this one (two kwargs swaps in dashboard/tailnet*.py). Two unrelated diffs, the same assertion, the same base 2f9ed9724f852186cd497dc2b5ab7682a8948289 — that is a property of the base. The security-conductor skill and its contract test landed on main very recently (#9270, #9271).

No product SHA churn for this one, and no rerun requested: a rerun cannot fix an assertion that fails for both branches on the same base.