fix(tailnet): decode the tailscale CLI as utf-8, not the host code page

[leonlaiyc]

Problem / Motivation

Both spawns of the tailscale CLI run in text mode with no encoding=:

proc = subprocess.run(
    [cli, *args],
    capture_output=True,
    text=True,          # <- decodes with locale.getpreferredencoding()
    ...
)

text=True without an encoding decodes the child with
locale.getpreferredencoding(): UTF-8 on POSIX, but the legacy ANSI code
page on Windows. What the child actually writes is not in doubt here —
tailscale is a Go binary, and tailscale ... --json emits JSON, which is
defined as UTF-8 (RFC 8259 §8.1). So this is a known encoding being decoded
with the wrong one, not a guess about an unknowable child.

The payload is operator-chosen free text: a tailnet device name. Non-ASCII
there is ordinary, not exotic.

Two distinct failures, both measured on a cp950 host, not inferred.

1 — bytes that are also legal in the host code page decode to different
characters, silently. 測試-desk encodes to e6 b8 ac e8 a9 a6, and each of
those is a legal Big5 lead or trail byte, so nothing raises — the name simply
comes back as other characters. _valid_magicdns_name then rejects a name that
was valid, and the tailnet origin is never derived. The red-before below shows
this as a plain inequality of two rendered names.

2 — bytes that decode under neither take a platform-specific path, and
neither is caught. Both sites guard with
except (OSError, subprocess.SubprocessError):

• POSIX — subprocess.run raises UnicodeDecodeError. That is a
  ValueError, so it is caught by neither arm and propagates out of the
  function.

• Windows — capture_output reads on a helper thread, so the same error
  kills that thread, prints an unhandled-thread traceback to the gateway's
  stderr, and leaves proc.stdout as None. Measured:

  Exception in thread Thread-1 (_readerthread):
    File ".../subprocess.py", line 1497, in _readerthread
      buffer.append(fh.read())
  UnicodeDecodeError: 'cp950' codec can't decode byte 0xff in position 20
  ...
  no exception; stdout = None
  

Why it matters

_run_json_detail states its contract in its own docstring, and both failures
break it:

Run the CLI and parse stdout as JSON. None on ANY failure.
Deliberately broad: the caller's contract is "a name or nothing", and every
failure mode here (no binary, daemon down, timeout, non-zero exit, non-JSON
output) means the same thing to it.

Failure 1 returns a wrong name rather than "a name or nothing". Failure 2
either escapes the function entirely (POSIX) or drops the whole payload and
leaves a stray traceback in the gateway's log (Windows). The listed failure
modes were all meant to converge on None; a decode failure is the one that
does not.

It is invisible where most development happens: on POSIX
locale.getpreferredencoding() already returns UTF-8, so every one of these
calls is correct there. Only a non-UTF-8 Windows host sees it, and only for an
operator who named a device in their own script.

What changed (motivation → approach → change)

Root cause: a child whose output encoding is knowable was decoded with the
host's locale instead.

• Both spawns now splat UTF8_TEXT — src/kiro_crew/subprocess_utf8.py,
  the repository's existing mapping for exactly this, whose own docstring names
  gh ("a Go binary; always writes UTF-8") as the same case. Three lines of
  production change plus two imports.

• errors="replace" comes with it, and it is what restores the documented
  contract on the undecodable path: a malformed byte costs one character inside
  the string, the surrounding JSON still parses, and the caller gets its answer.
  That is the policy fix(dashboard): decode file diffs as utf-8 #3669 established rather than a new one.

• tailnet_serve.py is fixed in the same commit, not left as a follow-up.
  Its own docstring gives the reason:

  Binary resolution and environment scrubbing are shared with the read
  path rather than re-implemented ... A second, subtly different copy of
  either is how one spawn comes to be hardened and its sibling not.

  Its _run has the identical spawn and the identical except (OSError, subprocess.SubprocessError) guard, and its stdout/stderr are shown to the
  operator verbatim.

• .github/subprocess-encoding-baseline.txt loses both entries. The repo's
  ratchet requires a file whose count shrinks to be pruned, so this is the
  gate's own bookkeeping, not an unrelated edit.

Deliberately NOT widened. The other 112 baselined files stay as they are.
Several of them genuinely should keep locale decoding — subprocess_utf8's
docstring names systeminfo and user shells, where pinning UTF-8 trades one
mojibake for another — so a sweep would need a per-child judgement each time and
would not be this defect.

Tests

Added TestTheCliIsDecodedAsUtf8NotTheHostCodePage to test/test_tailnet_e2e.py
— the suite that already spawns a real fake tailscale through the
production _cli_path and the production subprocess.run, so the decode under
test is the real process boundary rather than a mock.

The existing fake could not exercise this at all: it answers via
print(json.dumps(...)), which is ASCII by construction (ensure_ascii defaults
on) and re-encodes through the child's stdout encoding, so it can never place a
non-ASCII byte on the pipe. The new helper writes to sys.stdout.buffer, which
puts exactly the intended bytes there — what a Go binary does.

Red-before (product code reverted to origin/main, tests kept):

FAILED ...::test_undecodable_bytes_degrade_to_a_name_rather_than_escaping
  AssertionError: a malformed byte must not cost the whole payload
FAILED ...::test_a_non_ascii_device_name_round_trips
  AssertionError: assert '測試-desk.tail1a2b3c.ts.net' == '����-desk.tail1a2b3c.ts.net'
2 failed, 1 passed

The second failure is the mojibake itself, printed side by side.

The two halves are deliberately different in strength, and the difference is
stated rather than papered over:

• test_undecodable_bytes_degrade_to_a_name_rather_than_escaping is
  deterministic on every host, CI included. It does not depend on what the
  host code page is, only on the decode being strict, so it goes red on a UTF-8
  Linux runner (where it raises) and on Windows (where the reader thread dies)
  alike.
• test_a_non_ascii_device_name_round_trips is host-conditioned — a UTF-8
  runner decodes it correctly either way — so it is paired with the above rather
  than relied on alone. It is kept because it is the thing that actually breaks
  for the user.
• test_the_fixture_really_defeats_a_legacy_code_page passes on both builds by
  design: it asserts 測試-desk is a name cp950 reads differently, so the
  round-trip test cannot pass vacuously on a payload that was ASCII all along.

Green-after: 450 passed / 3 skipped across test_tailnet_e2e.py,
test_tailnet_serve.py, test_tailnet_cli.py, test_tailnet_origin.py,
test_tailnet_mobile.py, test_tailnet_peer.py, test_tailnet_governance.py
and test_spawn_audit.py — the last of which audits
dashboard/tailnet.py::_run_json_detail by name as a spawn primitive.

Gates: flake8, isort --check-only, mypy --platform linux and
scripts/check_subprocess_encoding.py all pass;
scripts/check_black_formatting.py passes (the two pre-existing black findings
in test_tailnet_e2e.py are baseline entries in untouched regions, so the file
was not reformatted).

Manual verification

N/A — unit coverage sufficient: the defect is a decode at a real process
boundary, and these tests exercise that exact boundary with a real child process
emitting real bytes, which is precisely what a manual run against a daemon would
do. The suite's own TestAgainstARealDaemon still covers a genuine tailscale
where one is installed.

Related Issues

None. Found by auditing .github/subprocess-encoding-baseline.txt — the repo's
own record of pre-existing sites in the failure class #3219 / #3669 / #5249
established — for entries whose child has a knowable encoding.

Contention checked immediately before opening: across all 357 open PRs, the only
other PR touching these files is #9333, a comments-only "strip history narration"
sweep with no overlap with these lines.

Pattern harvest

Rule candidate: review-prompt

Pattern: UnicodeDecodeError is a ValueError, so
except (OSError, subprocess.SubprocessError) does not catch it — the guard
around almost every spawn in this repository looks total and is not. Any function
that documents "None on ANY failure" while decoding a child in text mode is
making a promise its own except clause cannot keep. When reviewing a spawn, check
the exception type lattice against the decode, not just against the spawn.

Second lesson, and the one that cost the most to find here: the same decode
failure has two different shapes by platform. POSIX communicate() decodes on
the calling thread and raises into the caller; Windows capture_output decodes
on a reader thread, so the exception is swallowed by the threading machinery and
the caller silently receives stdout is None. A test asserting "it raises" would
have passed on Linux and proved nothing on Windows. Assert the post-condition
the caller depends on (a parsed payload came back) rather than the mechanism, and
one test covers both.

Third: an ensure_ascii fake cannot test a decode. The existing e2e fake
looked like a real process boundary and was structurally incapable of putting a
non-ASCII byte on the pipe. When the property under test is an encoding, the
fixture has to write bytes, not call print.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|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]

Design Review (Fable 5, fork) — ✅ PASS

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

All verified against the base tree: UTF8_TEXT exists with exactly the claimed semantics (its docstring names Go binaries like gh as the intended case), _run_json_detail's docstring does promise "None on ANY failure", both spawn sites match the diff, and every test seam (github_runner, _posix_candidate_trusted, _CLI_CANDIDATE_PATHS) exists. The fix pins the decode at the root cause rather than patching the exception guard (the symptom), covers the sibling spawn the module docstring itself says must not diverge, deliberately avoids a blanket sweep of the other baselined sites, and the deterministic test exercises the changed behavior on UTF-8 CI runners — pinning the encoding makes the code path platform-independent, so the "Manual verification: N/A" claim holds.

Design-Verdict: PASS

Root-cause fix using the repo's own established primitive, scoped to exactly the two sites whose child encoding is knowable, with a real-boundary test.

[DESIGN-REVIEWED] d6b1989

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] d6b1989

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] d6b1989

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of d6b198955fa26fdece396accb752b309a4dd8ea3 via the fork AI-review pipeline — 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 claims verified against the base: UTF8_TEXT (src/kiro_crew/subprocess_utf8.py:64) is the repo's existing mechanism whose docstring names Go binaries as exactly this case; the ratchet script demands pruning shrunk entries; the baseline holds 114 entries in base (112 after this PR, matching the description's count); the test file already has every import and both seams the new helper reuses.

First-Principles-Verdict: PASS

Verify the new .bat fake actually runs on the Windows CI shard — the quoting in _raw_cli is the one line this review could not execute.

What this change ships

Intent: make the tailnet dashboard survive non-ASCII tailscale device names on non-UTF-8 Windows hosts — a FIX.

Inventory (4 items)

1. Tailnet status read now decodes the CLI as UTF-8; non-ASCII device names round-trip — justified
2. Undecodable bytes now degrade to one replaced character instead of escaping or killing the reader thread — justified
3. The serve-path spawn gets the same decode pin — justified
4. Two entries leave the subprocess-encoding baseline — justified

Item 3 is the same defect (same child binary, same missing encoding=) fixed at cause level rather than left as an unfixed sibling; item 4 is mandated by the shrink-only ratchet in scripts/check_subprocess_encoding.py. The 112 remaining baselined files are governed by that existing ratchet, and subprocess_utf8.py's own docstring records why a blanket sweep would be wrong — the deferral is a decision this repository already made, not a gap.

[FIRST-PRINCIPLES-REVIEWED] d6b1989

[leonlaiyc]

CI triage on 67f31c5f2: one failure was mine and is fixed at d6b198955; the rest are base-side.

Branch-caused — fixed. Backend Lint & Type Check (3.12) failed the new comment-history gate, and the offender was mine:

comment-history gate FAILED: 1 new offender(s), 0 grown count(s)
  test/test_tailnet_e2e.py:436: #3669

A docstring I added cited a PR number, which that gate forbids in comments and docstrings (docs/system-specs/common/code-style.md: state current behaviour, the history is in git). The sentence now states the policy without the citation. test/test_tailnet_e2e.py is absent from comment-history-baseline.json, so it has to be clean rather than merely not-worse — it now is. The two pre-existing entries in tailnet.py and the two in tailnet_serve.py are baselined at exactly their current counts and are untouched.

Not branch-caused — Backend Tests (3.12, 3) and the Windows shards. The concrete assertions are test/test_session_summary_api.py and test/test_security_conductor_skill_contract.py:125 (assert not True), plus AttributeError: module 'os' has no attribute 'killpg' on Windows. This diff is two text=True → **UTF8_TEXT kwargs swaps on the tailscale spawns, one test file, and one baseline line; neither failing module imports tailnet (grep -c 'tailnet|tailscale' test/test_session_summary_api.py → 0), and nothing here can reach a process-group kill.

The decisive evidence is that the same test_security_conductor_skill_contract.py:125 failure appears on my #9243 — whose entire diff is one unrelated file, test/test_ci_surface_tests.py. Two disjoint diffs, the same assertion, the same base 2f9ed9724f852186cd497dc2b5ab7682a8948289. That is a property of the base, not of either branch. The security-conductor skill and its contract test landed on main very recently (#9270, #9271), and os.killpg does not exist on Windows at all.

I have therefore changed no product code for those, and pushed once. Coverage Gate and PR Readiness are derivative of the above.