From f1e09df20b771a78d85527709939229d79dabd8a Mon Sep 17 00:00:00 2001 From: scwf Date: Thu, 23 Jul 2026 17:13:58 +0800 Subject: [PATCH] fix(windows): prevent locale decode failures and orphan-reaper crashes Read agent configs, instruction files, skills, MCP configs, plugin settings, and CLI bundle inputs explicitly as UTF-8 so Windows locale encodings cannot reject valid Unicode content. Decode harness status subprocess output as UTF-8 with replacement to prevent locale-dependent reader failures. Skip orphan reaping when POSIX wait primitives are unavailable so Windows host startup and cleanup do not crash. Add regression coverage for UTF-8 parsing, bundle preprocessing, skill resources, CLI status output, and Windows host cleanup. Signed-off-by: scwf --- omnigent/cli.py | 4 +- omnigent/host/connect.py | 18 ++++++-- omnigent/inner/loader.py | 4 +- omnigent/onboarding/harness_install.py | 3 +- omnigent/spec/_omnigent_compat.py | 10 ++-- omnigent/spec/parser.py | 13 ++++-- omnigent/spec/skill_sources.py | 2 +- omnigent/tools/builtins/read_skill_file.py | 2 +- tests/cli/test_cli.py | 48 ++++++++++++++++++++ tests/host/test_connect.py | 22 +++++++++ tests/inner/test_loader.py | 10 ++++ tests/onboarding/test_harness_install.py | 22 +++++++++ tests/spec/test_parser.py | 17 +++++++ tests/tools/builtins/test_read_skill_file.py | 38 ++++++++++++++++ 14 files changed, 192 insertions(+), 21 deletions(-) diff --git a/omnigent/cli.py b/omnigent/cli.py index ace1cd9972..6c875958a1 100644 --- a/omnigent/cli.py +++ b/omnigent/cli.py @@ -4464,7 +4464,7 @@ def _resolve_bundle_env_vars(source: Path) -> dict[str, str]: # ── config.yaml ────────────────────────────────── config_path = source / "config.yaml" if config_path.exists(): - raw = yaml.safe_load(config_path.read_text()) + raw = yaml.safe_load(config_path.read_text(encoding="utf-8")) if isinstance(raw, dict): changed = _expand_config_env_vars(raw, expand_env_vars) if changed: @@ -4480,7 +4480,7 @@ def _resolve_bundle_env_vars(source: Path) -> dict[str, str]: mcp_dir = source / "tools" / "mcp" if mcp_dir.is_dir(): for yaml_file in sorted(mcp_dir.glob("*.yaml")): - raw = yaml.safe_load(yaml_file.read_text()) + raw = yaml.safe_load(yaml_file.read_text(encoding="utf-8")) if not isinstance(raw, dict): continue changed = False diff --git a/omnigent/host/connect.py b/omnigent/host/connect.py index a3553cd6e8..3c1d625706 100644 --- a/omnigent/host/connect.py +++ b/omnigent/host/connect.py @@ -178,6 +178,11 @@ def _display_log_path(path: Path) -> str: _ORPHAN_REAP_INTERVAL_S = 2.0 +def _orphan_reaping_supported() -> bool: + """Return whether this platform exposes the POSIX child-wait primitives.""" + return hasattr(os, "waitpid") and hasattr(os, "WNOHANG") + + def _install_child_subreaper() -> bool: """Make this process reap orphaned descendants (Linux only). @@ -796,6 +801,8 @@ def _reap_orphans_once(self) -> int: :returns: Count of orphan (non-runner) processes reaped this sweep. """ + if not _orphan_reaping_supported(): + return 0 if self._owned_subprocess_ops > 0: # A host-owned subprocess (e.g. a git worktree command) is running # in a worker thread. Its child is a DIRECT child of this process @@ -1915,11 +1922,12 @@ async def run(self) -> None: # runner dies (this host is PID 1 in a container, or a subreaper # otherwise). Without this they pile up as zombies and can # OOM the box on a long-blocked run (#1782). - if _install_child_subreaper(): - _logger.debug("installed PR_SET_CHILD_SUBREAPER; host will reap orphans") - self._reaper_task = asyncio.create_task( - self._orphan_reaper_loop(), name="host-orphan-reaper" - ) + if _orphan_reaping_supported(): + if _install_child_subreaper(): + _logger.debug("installed PR_SET_CHILD_SUBREAPER; host will reap orphans") + self._reaper_task = asyncio.create_task( + self._orphan_reaper_loop(), name="host-orphan-reaper" + ) backoff = _RECONNECT_BASE_S try: while True: diff --git a/omnigent/inner/loader.py b/omnigent/inner/loader.py index 880b55364a..f6b5db76b5 100644 --- a/omnigent/inner/loader.py +++ b/omnigent/inner/loader.py @@ -105,7 +105,7 @@ def load_agent_def( """ if isinstance(path_or_dict, (str, Path)): path = Path(path_or_dict) - with open(path) as f: + with path.open(encoding="utf-8") as f: data = yaml.load(f, Loader=_OmnigentYamlLoader) instructions_root: Path | None = path.parent else: @@ -176,7 +176,7 @@ def _read_contained_file(root: Path, value: str) -> str | None: try: resolved = candidate.resolve() if resolved.is_relative_to(root.resolve()) and resolved.is_file(): - return resolved.read_text() + return resolved.read_text(encoding="utf-8") except OSError: # Path too long or invalid characters — fall through to inline text. pass diff --git a/omnigent/onboarding/harness_install.py b/omnigent/onboarding/harness_install.py index 63cfa9ffe9..584afe0a4e 100644 --- a/omnigent/onboarding/harness_install.py +++ b/omnigent/onboarding/harness_install.py @@ -701,7 +701,8 @@ def harness_cli_logged_in(key: str) -> bool: check=False, timeout=30, capture_output=True, - text=True, + encoding="utf-8", + errors="replace", ) except (OSError, subprocess.TimeoutExpired): return False diff --git a/omnigent/spec/_omnigent_compat.py b/omnigent/spec/_omnigent_compat.py index 4986b7cce7..5b1ac4445b 100644 --- a/omnigent/spec/_omnigent_compat.py +++ b/omnigent/spec/_omnigent_compat.py @@ -229,8 +229,8 @@ def is_omnigent_yaml(path: Path) -> bool: if path.suffix.lower() not in {".yaml", ".yml"}: return False try: - raw = yaml.safe_load(path.read_text()) - except yaml.YAMLError: + raw = yaml.safe_load(path.read_text(encoding="utf-8")) + except (yaml.YAMLError, UnicodeDecodeError): return False if not isinstance(raw, dict): return False @@ -269,12 +269,14 @@ def diagnose_yaml_rejection(path: Path) -> str: if path.suffix.lower() not in {".yaml", ".yml"}: return f"file extension is {path.suffix!r}, expected '.yaml' or '.yml'" try: - raw = yaml.safe_load(path.read_text()) + raw = yaml.safe_load(path.read_text(encoding="utf-8")) except yaml.YAMLError as exc: # Strip trailing whitespace so the message stays one line — # PyYAML embeds the source location in its error string, # which is exactly what the user needs to fix the typo. return f"YAML parse error: {exc!s}".replace("\n", " ").rstrip() + except UnicodeDecodeError as exc: + return f"file is not valid UTF-8: {exc}" if raw is None: return "file is empty (or contains only YAML comments / null)" if not isinstance(raw, dict): @@ -377,7 +379,7 @@ def load_omnigent_yaml( # read resolves booleans the same way load_agent_def's YAML # parsing did — both loaders keep on/off as plain strings # instead of the YAML 1.1 bool aliases. - raw = _yaml.load(path.read_text(), Loader=_OmnigentYamlLoader) or {} + raw = _yaml.load(path.read_text(encoding="utf-8"), Loader=_OmnigentYamlLoader) or {} if not isinstance(raw, dict): raw = {} spec = agent_def_to_agent_spec(agent_def, raw_yaml=raw) diff --git a/omnigent/spec/parser.py b/omnigent/spec/parser.py index 4f2ea3226a..192afb82ac 100644 --- a/omnigent/spec/parser.py +++ b/omnigent/spec/parser.py @@ -182,7 +182,10 @@ def parse(root: Path, *, expand_env: bool = True) -> AgentSpec: if not config_path.exists(): raise FileNotFoundError(f"config.yaml not found in {root}") - raw = yaml.load(config_path.read_text(), Loader=_ConfigYamlLoader) + # Agent bundles are authored as UTF-8 (emoji / em dashes in prompts). + # Path.read_text() defaults to the locale encoding on Windows (often GBK), + # which raises UnicodeDecodeError on those bytes — force UTF-8. + raw = yaml.load(config_path.read_text(encoding="utf-8"), Loader=_ConfigYamlLoader) if not isinstance(raw, dict): raise OmnigentError( f"config.yaml must be a YAML mapping, got {type(raw).__name__}", @@ -1803,7 +1806,7 @@ def _read_contained_file(root: Path, value: str) -> str | None: try: resolved = candidate.resolve() if resolved.is_relative_to(root.resolve()) and resolved.is_file(): - return resolved.read_text() + return resolved.read_text(encoding="utf-8") except OSError: # Path too long or invalid characters — treat as inline text. pass @@ -1844,7 +1847,7 @@ def _resolve_instructions(root: Path, raw_value: object) -> str | None: candidate = root / filename try: if candidate.is_file(): - return candidate.read_text() + return candidate.read_text(encoding="utf-8") except OSError: pass return None @@ -2126,7 +2129,7 @@ def _parse_skill(skill_md: Path) -> SkillSpec: ``strict=False``) can catch them uniformly. """ try: - text = skill_md.read_text() + text = skill_md.read_text(encoding="utf-8") except (OSError, UnicodeDecodeError) as exc: # UnicodeDecodeError (a non-UTF-8 SKILL.md) is a ValueError, not an # OSError — funnel it through OmnigentError too so the lenient @@ -2394,7 +2397,7 @@ def _discover_mcp_servers( return [] servers: list[MCPServerConfig] = [] for yaml_file in sorted(mcp_dir.glob("*.yaml")): - raw = yaml.safe_load(yaml_file.read_text()) + raw = yaml.safe_load(yaml_file.read_text(encoding="utf-8")) if not isinstance(raw, dict): raise OmnigentError( f"MCP config must be a YAML mapping: {yaml_file}", diff --git a/omnigent/spec/skill_sources.py b/omnigent/spec/skill_sources.py index d2fddd4677..079e530f1d 100644 --- a/omnigent/spec/skill_sources.py +++ b/omnigent/spec/skill_sources.py @@ -121,7 +121,7 @@ def resolve_harness_skills(ctx: SkillSourceContext, harness: str | None) -> list def _read_json(path: Path) -> dict[str, Any] | None: """Best-effort JSON read; ``None`` on missing/unreadable/non-dict.""" try: - data = json.loads(path.read_text()) + data = json.loads(path.read_text(encoding="utf-8")) except (OSError, ValueError): return None return data if isinstance(data, dict) else None diff --git a/omnigent/tools/builtins/read_skill_file.py b/omnigent/tools/builtins/read_skill_file.py index b6f88ab583..595c28ffc1 100644 --- a/omnigent/tools/builtins/read_skill_file.py +++ b/omnigent/tools/builtins/read_skill_file.py @@ -155,4 +155,4 @@ def _read_file_safely( if not resolved.is_file(): return f"Error: file not found: {rel_path}" - return resolved.read_text() + return resolved.read_text(encoding="utf-8") diff --git a/tests/cli/test_cli.py b/tests/cli/test_cli.py index 75a1ae2c04..02bcce9e01 100644 --- a/tests/cli/test_cli.py +++ b/tests/cli/test_cli.py @@ -2370,6 +2370,54 @@ def test_resolve_bundle_expands_mcp_env( assert parsed["env"]["API_TOKEN"] == "stdio-tok-xyz" +def test_resolve_bundle_reads_config_and_mcp_as_utf8( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Bundle preprocessing reads every YAML input explicitly as UTF-8.""" + monkeypatch.setenv("BUNDLE_UTF8_TOKEN", "resolved-token") + config_path = tmp_path / "config.yaml" + config_path.write_bytes( + ( + "spec_version: 1\n" + "description: plans — and splits\n" + "llm:\n" + " connection:\n" + " api_key: ${BUNDLE_UTF8_TOKEN}\n" + ).encode() + ) + mcp_path = tmp_path / "tools" / "mcp" / "utf8.yaml" + mcp_path.parent.mkdir(parents=True) + mcp_path.write_bytes( + ( + "name: utf8\n" + "transport: http\n" + "url: http://localhost:9000/mcp\n" + "description: 中文工具\n" + "headers:\n" + " Authorization: Bearer ${BUNDLE_UTF8_TOKEN}\n" + ).encode() + ) + + original_read_text = Path.read_text + observed: dict[Path, str | None] = {} + + def _record_encoding(path: Path, *args: Any, **kwargs: Any) -> str: + if path in {config_path, mcp_path}: + observed[path] = kwargs.get("encoding") + return original_read_text(path, *args, **kwargs) + + monkeypatch.setattr(Path, "read_text", _record_encoding) + + resolved = _resolve_bundle_env_vars(tmp_path) + + assert observed == {config_path: "utf-8", mcp_path: "utf-8"} + config = yaml.safe_load(resolved["config.yaml"]) + mcp = yaml.safe_load(resolved[str(mcp_path.relative_to(tmp_path))]) + assert config["description"] == "plans — and splits" + assert mcp["description"] == "中文工具" + + def test_resolve_bundle_no_env_vars_returns_empty( tmp_path: Path, ) -> None: diff --git a/tests/host/test_connect.py b/tests/host/test_connect.py index d8ee4f3ed1..35c45c3765 100644 --- a/tests/host/test_connect.py +++ b/tests/host/test_connect.py @@ -14,6 +14,7 @@ from websockets.exceptions import ConnectionClosedError, InvalidStatus, InvalidURI from websockets.http11 import Response +import omnigent.host.connect as connect from omnigent.host.connect import ( HostConnectError, HostProcess, @@ -1130,6 +1131,27 @@ def test_reap_orphans_reaps_orphaned_children(tmp_path: Path) -> None: assert host._reap_orphans_once() == 0 +async def test_reap_orphans_is_noop_without_posix_wait_primitives( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Windows has no waitpid/WNOHANG, so its host must skip orphan reaping.""" + host = _make_host_process() + monkeypatch.delattr(connect.os, "waitpid", raising=False) + monkeypatch.delattr(connect.os, "WNOHANG", raising=False) + monkeypatch.setattr( + host, + "_reap_orphans_waitid", + lambda: (_ for _ in ()).throw(AssertionError("waitid reaper called")), + ) + monkeypatch.setattr( + host, + "_reap_orphans_waitpid", + lambda: (_ for _ in ()).throw(AssertionError("waitpid reaper called")), + ) + + assert host._reap_orphans_once() == 0 + + def test_reap_orphans_never_steals_tracked_runner_exit_code(tmp_path: Path) -> None: """The reaper must not consume a tracked runner's exit status (#1782). diff --git a/tests/inner/test_loader.py b/tests/inner/test_loader.py index c68603784a..4facebbb26 100644 --- a/tests/inner/test_loader.py +++ b/tests/inner/test_loader.py @@ -457,6 +457,16 @@ def test_workflow(self): class TestLoadFromYAML(unittest.TestCase): + def test_load_agent_def_reads_yaml_as_utf8(self): + """UTF-8 punctuation must not depend on the Windows locale codec.""" + with tempfile.TemporaryDirectory() as td: + path = Path(td) / "agent.yaml" + path.write_bytes(b"name: utf8-agent\nprompt: plans \xe2\x80\x94 and splits\n") + + agent = load_agent_def(path) + + self.assertEqual(agent.prompt, "plans — and splits") + def test_yaml_on_key_is_not_parsed_as_boolean(self): yaml_content = """ name: policy_agent diff --git a/tests/onboarding/test_harness_install.py b/tests/onboarding/test_harness_install.py index a2b9c05045..c94fdbc0b9 100644 --- a/tests/onboarding/test_harness_install.py +++ b/tests/onboarding/test_harness_install.py @@ -832,6 +832,28 @@ def _run(argv: list[str], **k: object): assert hi.harness_cli_logged_in(ANTHROPIC_FAMILY) is expected +def test_harness_cli_logged_in_decodes_status_output_as_utf8( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Status output uses the CLI's UTF-8 pipe encoding, not the system locale.""" + monkeypatch.setattr(hi.shutil, "which", lambda name: f"/usr/bin/{name}") + + def _run(argv: list[str], **kwargs: object): + assert argv == ["claude", "auth", "status"] + assert kwargs["encoding"] == "utf-8" + assert kwargs["errors"] == "replace" + assert "text" not in kwargs + return subprocess.CompletedProcess( + args=argv, + returncode=0, + stdout='{"loggedIn": true, "message": "signed in — ✓"}', + stderr="", + ) + + monkeypatch.setattr(hi.subprocess, "run", _run) + assert hi.harness_cli_logged_in(ANTHROPIC_FAMILY) is True + + @pytest.mark.parametrize( "stdout,returncode,expected", [ diff --git a/tests/spec/test_parser.py b/tests/spec/test_parser.py index ef774bcd5d..ba41789443 100644 --- a/tests/spec/test_parser.py +++ b/tests/spec/test_parser.py @@ -44,6 +44,23 @@ def test_parse_minimal(agent_dir: Path) -> None: assert spec.sub_agents == [] +def test_parse_utf8_em_dash_description(tmp_path: Path) -> None: + """Agent YAML with UTF-8 punctuation must load on Windows locale encodings. + + Polly's config uses U+2014 EM DASH (``—``, bytes ``e2 80 94``). On a CP936 + host, bare ``Path.read_text()`` decodes as GBK and raises UnicodeDecodeError + before YAML parsing — the Web UI surfaces that as + ``failed to load agent spec: 'gbk' codec can't decode…``. + """ + (tmp_path / "config.yaml").write_bytes( + b"spec_version: 1\nname: utf8-agent\ndescription: plans \xe2\x80\x94 and splits\n" + ) + spec = parse(tmp_path) + assert spec.name == "utf8-agent" + assert spec.description is not None + assert "\u2014" in spec.description + + def test_parse_missing_config_yaml(tmp_path: Path) -> None: with pytest.raises(FileNotFoundError, match=r"config.yaml not found"): parse(tmp_path) diff --git a/tests/tools/builtins/test_read_skill_file.py b/tests/tools/builtins/test_read_skill_file.py index 984d89473d..52e87733ae 100644 --- a/tests/tools/builtins/test_read_skill_file.py +++ b/tests/tools/builtins/test_read_skill_file.py @@ -4,6 +4,7 @@ import json from pathlib import Path +from typing import Any import pytest @@ -69,6 +70,43 @@ def test_read_skill_file_returns_content( assert "snake_case" in result +def test_read_skill_file_reads_resources_as_utf8( + skill_with_resources: SkillSpec, + tool_ctx: ToolContext, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Skill resources use UTF-8 rather than the Windows locale encoding.""" + assert skill_with_resources.skill_dir is not None + resource = skill_with_resources.skill_dir / "references" / "utf8.md" + resource.write_bytes("# 设计说明\n\nUse plans — then execute. ✓".encode()) + + original_read_text = Path.read_text + observed_encoding: str | None = None + + def _record_encoding(path: Path, *args: Any, **kwargs: Any) -> str: + nonlocal observed_encoding + if path == resource: + observed_encoding = kwargs.get("encoding") + return original_read_text(path, *args, **kwargs) + + monkeypatch.setattr(Path, "read_text", _record_encoding) + tool = ReadSkillFileTool([skill_with_resources]) + + result = tool.invoke( + json.dumps( + { + "skill_name": "code-review", + "path": "references/utf8.md", + } + ), + tool_ctx, + ) + + assert observed_encoding == "utf-8" + assert "设计说明" in result + assert "plans — then execute. ✓" in result + + def test_read_skill_file_unknown_skill( skill_with_resources: SkillSpec, tool_ctx: ToolContext,