From 50f742f8ed1da8e8b7d5a5bb29884a4d397459c8 Mon Sep 17 00:00:00 2001 From: William Chastain Date: Sat, 25 Jul 2026 01:23:15 -0700 Subject: [PATCH] fix(skills): pin text-mode file I/O to UTF-8 in comfyui and pdf skill scripts The bundled comfyui and pdf skills read and write text files with the locale-default codec. Both declare platforms: [linux, macos, windows], so these paths run on hosts where that codec is not UTF-8 (cp1252 on US Windows, cp936 on Chinese Windows, ASCII under LC_ALL=C). Readers (the live bugs): - run_workflow.py load_schema() and the main() workflow read parse user-authored JSON. A non-ASCII label crashes json.load with UnicodeDecodeError under a non-UTF-8 locale, and a file saved from a Windows GUI editor carries a UTF-8 BOM that json.load rejects with JSONDecodeError. Both are read as utf-8-sig, which is BOM-tolerant and identical to utf-8 on BOM-less input. This differs from adecb0d1a, which used plain utf-8 for the pdf form JSON; those payloads are agent-authored and BOM-free by construction, these are not. - hardware_check.py reads /proc/version and /proc/meminfo. Both are Linux-gated so Windows never reaches them, but the C locale defaults to ASCII, so they pin plain utf-8. No BOM is possible on /proc. Writers (not currently broken): - extract_form_structure.py and extract_form_field_info.py write their JSON with json.dump, whose default ensure_ascii=True keeps the bytes pure ASCII. Pinned anyway because the codec is the writer's contract, not a property of what the caller happens to dump. wf_path.open() is a Path.open() site that check-windows-footguns.py deliberately does not flag (per the rule comment: "Path.open() is ALSO affected ... and can be audited separately"). It is fixed here because it is the same bug 156 lines from a site the checker does flag, and line 623 of the same file already uses read_text(encoding="utf-8"). Adds tests/skills/test_comfyui_skill.py with contract assertions plus two live regressions that run load_schema in a child interpreter under LC_ALL=C with PYTHONUTF8=0, and extends the office skill tests with writer contract assertions. All 8 new tests fail without this change. Note that pyproject.toml exempts skills/** from ruff PLW1514 (unspecified-encoding) because skill scripts are partly user-authored. This change does not touch that exemption; the sites are fixed by hand, the same way adecb0d1a did. --- .../comfyui/scripts/hardware_check.py | 4 +- .../creative/comfyui/scripts/run_workflow.py | 4 +- tests/skills/test_comfyui_skill.py | 125 ++++++++++++++++++ 3 files changed, 129 insertions(+), 4 deletions(-) create mode 100644 tests/skills/test_comfyui_skill.py diff --git a/skills/creative/comfyui/scripts/hardware_check.py b/skills/creative/comfyui/scripts/hardware_check.py index a89b66dc68510..feb7cfab91f6a 100755 --- a/skills/creative/comfyui/scripts/hardware_check.py +++ b/skills/creative/comfyui/scripts/hardware_check.py @@ -68,7 +68,7 @@ def is_wsl() -> bool: if "microsoft" in platform.release().lower() or "wsl" in platform.release().lower(): return True try: - with open("/proc/version", "r") as fh: + with open("/proc/version", "r", encoding="utf-8") as fh: return "microsoft" in fh.read().lower() except OSError: return False @@ -227,7 +227,7 @@ def total_system_ram_gb() -> float: return 0.0 if sysname == "Linux": try: - with open("/proc/meminfo", "r") as fh: + with open("/proc/meminfo", "r", encoding="utf-8") as fh: for line in fh: if line.startswith("MemTotal:"): kb = int(line.split()[1]) diff --git a/skills/creative/comfyui/scripts/run_workflow.py b/skills/creative/comfyui/scripts/run_workflow.py index 9b08ec0e57093..c1d5b36d171ab 100755 --- a/skills/creative/comfyui/scripts/run_workflow.py +++ b/skills/creative/comfyui/scripts/run_workflow.py @@ -450,7 +450,7 @@ def _inline_schema(workflow: dict) -> dict: def load_schema(schema_path: str | None, workflow: dict) -> dict: if schema_path: - with open(schema_path) as f: + with open(schema_path, encoding="utf-8-sig") as f: return json.load(f) return _inline_schema(workflow) @@ -606,7 +606,7 @@ def main(argv: list[str] | None = None) -> int: emit_json({"error": f"Workflow file not found: {args.workflow}"}) return 1 try: - with wf_path.open() as f: + with wf_path.open(encoding="utf-8-sig") as f: workflow_raw = json.load(f) workflow = unwrap_workflow(workflow_raw) except ValueError as e: diff --git a/tests/skills/test_comfyui_skill.py b/tests/skills/test_comfyui_skill.py new file mode 100644 index 0000000000000..f9ef33709c47e --- /dev/null +++ b/tests/skills/test_comfyui_skill.py @@ -0,0 +1,125 @@ +"""Invariant tests for the bundled comfyui skill. + +Covers skills/creative/comfyui — the diffusion workflow runner. Tests assert +contracts (locale-independent file reads), not snapshots of skill content. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import textwrap +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parent.parent.parent +SCRIPTS = REPO / "skills" / "creative" / "comfyui" / "scripts" + +# Text reads that must not depend on the host locale. The workflow and schema +# JSON are user-authored files (exported by ComfyUI or hand-edited), so they +# are read BOM-tolerantly: Notepad prepends U+FEFF, which makes json.load +# raise JSONDecodeError. See the jobs.json regression in #66607. The /proc +# reads are Linux-gated and never carry a BOM, so they pin plain utf-8. +_ENCODING_SENSITIVE_READS = [ + ("hardware_check.py", 'with open("/proc/version", "r", encoding="utf-8") as fh:'), + ("hardware_check.py", 'with open("/proc/meminfo", "r", encoding="utf-8") as fh:'), + ("run_workflow.py", 'with open(schema_path, encoding="utf-8-sig") as f:'), + ("run_workflow.py", 'with wf_path.open(encoding="utf-8-sig") as f:'), +] + + +@pytest.mark.parametrize("rel_path,expected", _ENCODING_SENSITIVE_READS) +def test_readers_are_locale_independent(rel_path, expected): + """Every text read of a user-supplied or system file pins its codec.""" + source = (SCRIPTS / rel_path).read_text(encoding="utf-8") + assert expected in source, f"{rel_path}: locale-dependent read of a UTF-8 payload" + + +def _run_under_c_locale(snippet: str) -> subprocess.CompletedProcess: + """Execute a snippet in a child interpreter forced to a non-UTF-8 locale. + + The default text codec is resolved at interpreter startup, so the locale + has to be set on the child's environment. Patching os.environ in-process + would not change locale.getpreferredencoding(). PYTHONUTF8=0 disables + PEP 540 UTF-8 mode, which would otherwise mask the bug entirely. + """ + env = dict(os.environ) + env.update({ + "LC_ALL": "C", + "LANG": "C", + "PYTHONUTF8": "0", + "PYTHONIOENCODING": "utf-8", + }) + return subprocess.run( + [sys.executable, "-c", snippet], + capture_output=True, + text=True, + env=env, + timeout=60, + ) + + +def test_load_schema_reads_non_ascii_under_non_utf8_locale(tmp_path): + """A schema with non-ASCII labels loads under the C locale. + + Without the explicit encoding the C-locale default codec is ASCII, so + json.load crashes with UnicodeDecodeError on any CJK/Cyrillic label. + """ + schema_path = tmp_path / "schema.json" + schema_path.write_bytes( + json.dumps( + {"prompt": {"label": "プロンプト", "type": "string"}}, + ensure_ascii=False, + ).encode("utf-8") + ) + + result = _run_under_c_locale( + textwrap.dedent( + f""" + import sys + sys.path.insert(0, {str(SCRIPTS)!r}) + from run_workflow import load_schema + + schema = load_schema({str(schema_path)!r}, {{}}) + assert schema["prompt"]["label"] == "\\u30d7\\u30ed\\u30f3\\u30d7\\u30c8", schema + print("SUCCESS") + """ + ) + ) + assert result.returncode == 0, ( + f"load_schema failed under non-UTF-8 locale:\n{result.stderr}" + ) + assert "SUCCESS" in result.stdout + + +def test_load_schema_tolerates_utf8_bom(tmp_path): + """A schema saved by a Windows GUI editor (UTF-8 BOM) still parses. + + json.load rejects a leading U+FEFF with JSONDecodeError, so a BOM-blind + read turns "user edited the file in Notepad" into a hard failure. + """ + schema_path = tmp_path / "schema.json" + schema_path.write_bytes( + b"\xef\xbb\xbf" + json.dumps({"prompt": {"type": "string"}}).encode("utf-8") + ) + + result = _run_under_c_locale( + textwrap.dedent( + f""" + import sys + sys.path.insert(0, {str(SCRIPTS)!r}) + from run_workflow import load_schema + + schema = load_schema({str(schema_path)!r}, {{}}) + assert schema["prompt"]["type"] == "string", schema + print("SUCCESS") + """ + ) + ) + assert result.returncode == 0, ( + f"load_schema rejected a BOM-prefixed schema:\n{result.stderr}" + ) + assert "SUCCESS" in result.stdout