From a109d2e456a7eae6d31508a034f1308611b59e21 Mon Sep 17 00:00:00 2001 From: scwf Date: Thu, 23 Jul 2026 14:54:12 +0800 Subject: [PATCH 1/7] fix(windows): harden CLI and host output on legacy code pages Legacy Windows code pages could crash CLI and host operations when user-facing output contained unsupported Unicode glyphs. Make CLI stdio configuration aware of terminals versus redirected streams so console output uses UTF-8 without breaking CP936 subprocess capture. Avoid changing the console input code page or leaking UTF-8 mode into unrelated child processes. Route host lifecycle and authentication messages through safe console output so encoding failures cannot interrupt tunnel handling after a runner has started or stopped. Initialize stdio in the standalone host daemon and force its captured stdout and stderr to UTF-8 with replacement, keeping raw output consistent with the UTF-8 log handler. Add regression coverage for GBK consoles, redirected streams, Unicode runner paths, daemon startup, and daemon log encoding. Signed-off-by: scwf --- omnigent/_platform.py | 82 ++++++++++++++++++++++++++ omnigent/cli.py | 10 +++- omnigent/host/_daemon_entry.py | 4 ++ omnigent/host/connect.py | 37 ++++++------ tests/cli/test_backend.py | 13 +++++ tests/host/test_connect.py | 66 +++++++++++++++++++++ tests/host/test_daemon_entry.py | 24 ++++++++ tests/inner/test_proc_and_platform.py | 84 +++++++++++++++++++++++++++ 8 files changed, 300 insertions(+), 20 deletions(-) create mode 100644 tests/host/test_daemon_entry.py diff --git a/omnigent/_platform.py b/omnigent/_platform.py index 5117825a32..8b2995b617 100644 --- a/omnigent/_platform.py +++ b/omnigent/_platform.py @@ -120,6 +120,88 @@ def resolve_cli_binary(name: str, *, env_var: str | None = None) -> str | None: #: True on macOS specifically (the seatbelt sandbox platform). IS_DARWIN = sys.platform == "darwin" +#: Windows console code page for UTF-8 (``chcp 65001``). +_WINDOWS_UTF8_CP = 65001 + + +def _stream_is_tty(stream: object) -> bool: + """Return whether *stream* is attached to an interactive terminal.""" + try: + return bool(stream.isatty()) # type: ignore[attr-defined] + except (AttributeError, OSError, ValueError): + return False + + +def _set_windows_console_output_utf8() -> None: + """Switch the attached Windows console output code page to UTF-8.""" + import ctypes + + kernel32 = ctypes.windll.kernel32 # type: ignore[attr-defined] + kernel32.SetConsoleOutputCP(_WINDOWS_UTF8_CP) + + +def configure_cli_stdio() -> bool: + """Make CLI stdout/stderr safe for Unicode on legacy Windows code pages. + + Chinese/Japanese Windows consoles often default to GBK (``cp936``). Writing + emoji or ``✓``/``✗`` then raises :exc:`UnicodeEncodeError`, which can crash + ``omnigent setup`` / ``config list`` and tear down the host tunnel reconnect + loop. On native Windows this reconfigures terminal streams to UTF-8 and + redirected streams to replacement-safe output without changing their + encoding, and switches the console output code page when possible. + + Idempotent and best-effort: failures are swallowed so a weird redirected + stream never blocks CLI startup. No-op on POSIX (including WSL). + + :returns: ``True`` when running on native Windows (configuration attempted), + ``False`` otherwise. + """ + if not IS_WINDOWS: + return False + has_console_stream = False + for stream in (sys.stdout, sys.stderr): + reconfigure = getattr(stream, "reconfigure", None) + if reconfigure is None: + continue + is_console = _stream_is_tty(stream) + has_console_stream = has_console_stream or is_console + with suppress(Exception): + if is_console: + reconfigure(encoding="utf-8", errors="replace") + else: + reconfigure(errors="replace") + if has_console_stream: + with suppress(Exception): + _set_windows_console_output_utf8() + return True + + +def safe_console_print( + message: str, + *, + file: object | None = None, + flush: bool = True, +) -> None: + """Print *message* without letting :exc:`UnicodeEncodeError` escape. + + Long-lived loops (the host tunnel) must not reconnect-loop because a + success/failure banner used a glyph the active code page cannot encode. + Falls back to a replacement-encoded line when the primary write fails. + + :param message: Text to print (may include emoji / box-drawing glyphs). + :param file: Stream to write to; defaults to :data:`sys.stdout`. + :param flush: Forwarded to :func:`print`. + """ + stream = sys.stdout if file is None else file + try: + print(message, file=stream, flush=flush) + except UnicodeEncodeError: + encoding = getattr(stream, "encoding", None) or "ascii" + fallback = message.encode(encoding, errors="replace").decode(encoding, errors="replace") + with suppress(Exception): + print(fallback, file=stream, flush=flush) + + #: Non-sensitive Windows environment variables that a spawned omnigent #: subprocess needs to function, for env-passthrough allowlists that otherwise #: assume POSIX names. Python uppercases env keys on Windows, so these match diff --git a/omnigent/cli.py b/omnigent/cli.py index 3d01129c10..218faafcbe 100644 --- a/omnigent/cli.py +++ b/omnigent/cli.py @@ -28,7 +28,7 @@ from rich.console import Console from rich.table import Table -from omnigent._platform import IS_WINDOWS, resolve_repo_symlink +from omnigent._platform import IS_WINDOWS, configure_cli_stdio, resolve_repo_symlink from omnigent.cli_common import ( RESUME_PICKER_SENTINEL as _RESUME_PICKER_SENTINEL, ) @@ -1596,6 +1596,11 @@ def main() -> None: so unhandled exceptions are captured even when the user didn't enable ``--log`` or ``--debug-events``. """ + # Windows GBK consoles raise UnicodeEncodeError on emoji / ✓ / ✗ and can + # tear down setup/config/host. Do this before any user-facing print + # (crash handler, Rich, click). No-op on POSIX. + configure_cli_stdio() + # Friendly crash handler: replaces Python's raw traceback with a # calm, branded crash screen + a one-tap path to file a GitHub issue # (browser opens the repo's pre-filled bug-report template with the @@ -2690,6 +2695,9 @@ def _build_host_daemon_env( for key, value in os.environ.items() if key in _RUNNER_ENV_ALLOWLIST or key.startswith(daemon_env_prefixes) } + # The daemon owns this binary log file. Keep raw stdout/stderr aligned with + # the UTF-8 logging handler instead of mixing locale-encoded print output. + env["PYTHONIOENCODING"] = "utf-8:replace" return env diff --git a/omnigent/host/_daemon_entry.py b/omnigent/host/_daemon_entry.py index 662dad6872..fec5715553 100644 --- a/omnigent/host/_daemon_entry.py +++ b/omnigent/host/_daemon_entry.py @@ -28,6 +28,10 @@ def main() -> None: :raises SystemExit: If neither / both of ``--server`` and ``--local`` are provided. """ + from omnigent._platform import configure_cli_stdio + + configure_cli_stdio() + parser = argparse.ArgumentParser( description="Background host daemon", ) diff --git a/omnigent/host/connect.py b/omnigent/host/connect.py index a3553cd6e8..35a4bd6e09 100644 --- a/omnigent/host/connect.py +++ b/omnigent/host/connect.py @@ -22,7 +22,7 @@ import websockets.asyncio.client from websockets.exceptions import InvalidStatus, InvalidURI -from omnigent._platform import WINDOWS_ENV_PASSTHROUGH +from omnigent._platform import WINDOWS_ENV_PASSTHROUGH, safe_console_print from omnigent.env_credentials import env_names_with_omnigent_prefix from omnigent.harness_aliases import canonicalize_harness from omnigent.harness_availability import HARNESS_BINARY_MISSING, HarnessAvailability @@ -1018,11 +1018,10 @@ def _fatal_upgrade_error(self, exc: InvalidURI | InvalidStatus) -> HostConnectEr # terminal — print once per redirect streak so a foreground # `omnigent host` shows the auth problem and its fix instead # of sitting silent while it retries. - print( + safe_console_print( f"⚠ {cause} Retrying — this also happens briefly while " f"the server restarts. {self._credentials_fix_hint()}", file=sys.stderr, - flush=True, ) return None return self._classify_http_status(exc.response.status_code) @@ -1194,11 +1193,10 @@ async def _handle_launch( # host's own terminal shows lifecycle lines, but the runner's real # output — the agent turn, tracebacks — lands only in this file. session_line = f"\n session: {frame.session_id}" if frame.session_id else "" - print( + safe_console_print( f" ↑ Runner started: {runner_id} (pid={proc.pid})\n" f" log: {_display_log_path(log_path)}" f"{session_line}", - flush=True, ) return HostLaunchRunnerResultFrame( request_id=frame.request_id, @@ -1232,9 +1230,8 @@ def _handle_stop( handle.proc.kill() handle.proc.wait() _logger.info("Stopped runner %s", frame.runner_id) - print( + safe_console_print( f" ↓ Runner stopped: {frame.runner_id}", - flush=True, ) return HostStopRunnerResultFrame( request_id=frame.request_id, @@ -2187,15 +2184,14 @@ async def _serve_frames(self, ws: websockets.asyncio.client.ClientConnection) -> for runner_id, error in list(self._unreported_exits.items()): del self._unreported_exits[runner_id] await self._report_runner_exit(runner_id, error) - # ``print`` (not ``_logger.warning``) so the user always sees the - # success line after the noisy ``databricks.sdk`` warnings — - # otherwise the terminal goes silent after auth and there's no - # signal the WS handshake actually completed. - print( + # User-facing banner (not ``_logger``) so the line is visible after + # noisy ``databricks.sdk`` warnings. ``safe_console_print`` so a + # legacy Windows code page cannot UnicodeEncodeError-out of the + # tunnel loop and trigger reconnect flaps. + safe_console_print( f"✓ Connected as {self._identity.name!r} " f"({self._identity.host_id}), {len(hello.runners)} live runner(s). " "Listening for sessions — Ctrl-C to disconnect.", - flush=True, ) loop = asyncio.get_running_loop() @@ -2356,20 +2352,20 @@ def run_host_process( path = config_path or CONFIG_PATH identity = load_or_create_host_identity(path) if not path.exists(): - print(f"Auto-generated {path} ({identity.host_id}, name: {identity.name})") - print(f"Connecting to {server_url} as {identity.name!r} ({identity.host_id})") + safe_console_print(f"Auto-generated {path} ({identity.host_id}, name: {identity.name})") + safe_console_print(f"Connecting to {server_url} as {identity.name!r} ({identity.host_id})") # Tell the user where logs land up front — `omnigent host` used to run # silently, so a stuck/quiet host gave no hint where to look. Session # work goes to per-runner files under the runner dir (the exact # file is printed when each runner launches). The host process's # own diagnostics go to the host destination. - print(f"Session logs: {_display_log_path(_runner_log_dir())}/") - print(f"This host's log: {_display_log_path(host_log_path)}") + safe_console_print(f"Session logs: {_display_log_path(_runner_log_dir())}/") + safe_console_print(f"This host's log: {_display_log_path(host_log_path)}") from omnigent.cli_diagnostics import current_cli_log_path _cli_log = current_cli_log_path() if _cli_log is not None and _cli_log != host_log_path: - print(f"CLI diagnostics: {_display_log_path(_cli_log)}") + safe_console_print(f"CLI diagnostics: {_display_log_path(_cli_log)}") host = HostProcess(identity, server_url) try: @@ -2378,5 +2374,8 @@ def run_host_process( # Fail loud: a permanent connection failure must not look like the # process is still working. Print the cause + fix, then exit non-zero # instead of the old behavior of reconnecting silently forever. - print(f"\n✗ Could not connect to {server_url}.\n{exc}", file=sys.stderr, flush=True) + safe_console_print( + f"\n✗ Could not connect to {server_url}.\n{exc}", + file=sys.stderr, + ) raise SystemExit(1) from exc diff --git a/tests/cli/test_backend.py b/tests/cli/test_backend.py index c6298fe262..20ef7bfd14 100644 --- a/tests/cli/test_backend.py +++ b/tests/cli/test_backend.py @@ -204,6 +204,19 @@ def test_ensure_host_daemon_local_inherits_data_dir_and_db_uri( assert env["OMNIGENT_DATABASE_URI"] == "postgresql://u:pw@h/db" +@pytest.mark.parametrize("server_url", [None, "https://example.databricksapps.com"]) +def test_build_host_daemon_env_forces_utf8_stdio( + monkeypatch: pytest.MonkeyPatch, + server_url: str | None, +) -> None: + """Daemon print output uses the same UTF-8 encoding as its log handler.""" + monkeypatch.setenv("PYTHONIOENCODING", "cp936") + + env = _build_host_daemon_env(server_url=server_url) + + assert env["PYTHONIOENCODING"] == "utf-8:replace" + + def test_build_host_daemon_env_local_preserves_server_credentials( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/tests/host/test_connect.py b/tests/host/test_connect.py index d8ee4f3ed1..a9648543e9 100644 --- a/tests/host/test_connect.py +++ b/tests/host/test_connect.py @@ -3,8 +3,10 @@ from __future__ import annotations import asyncio +import io import logging import subprocess +import sys import time from pathlib import Path from unittest.mock import patch @@ -481,6 +483,54 @@ def _fake_popen(args: list[str], **kwargs: object) -> subprocess.Popen[bytes]: _cleanup_host(host) +async def test_handle_launch_returns_result_when_banner_is_not_gbk_encodable( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A Unicode log path cannot interrupt launch after the runner is tracked.""" + + class _LiveProcess: + pid = 4242 + returncode: int | None = None + + def poll(self) -> int | None: + return self.returncode + + def terminate(self) -> None: + self.returncode = 0 + + def wait(self, timeout: float | None = None) -> int: + return self.returncode or 0 + + def kill(self) -> None: + self.returncode = -9 + + unicode_home = tmp_path / "home-😀" + unicode_home.mkdir() + workspace = tmp_path / "project" + workspace.mkdir() + monkeypatch.setattr(Path, "home", classmethod(lambda _cls: unicode_home)) + output = io.TextIOWrapper(io.BytesIO(), encoding="gbk", errors="strict", write_through=True) + monkeypatch.setattr(sys, "stdout", output) + monkeypatch.setattr( + "omnigent.host.connect.subprocess.Popen", + lambda *args, **kwargs: _LiveProcess(), + ) + host = _make_host_process() + + result = await host._handle_launch( + HostLaunchRunnerFrame( + request_id="req_unicode_log", + binding_token="tok_unicode_log", + workspace=str(workspace), + ) + ) + + assert result.status == "launched" + assert result.runner_id in host._runners + _cleanup_host(host) + + class _FakeTunnel: """In-memory stand-in for the host's WebSocket tunnel connection. @@ -2566,6 +2616,22 @@ async def test_login_redirect_prints_warning_to_terminal( assert "omnigent login https://app.example.databricks.com" in err +async def test_login_redirect_warning_survives_gbk_stderr( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The warning glyph cannot turn an authentication retry into a disconnect.""" + stderr = io.TextIOWrapper(io.BytesIO(), encoding="gbk", errors="strict", write_through=True) + monkeypatch.setattr(sys, "stderr", stderr) + host = _host() + + result = host._fatal_upgrade_error( + InvalidURI("https://w/oidc/authorize", "scheme isn't ws or wss") + ) + + assert result is None + assert host._login_redirect_streak == 1 + + async def test_fresh_host_fails_loud_after_persistent_login_redirects( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/tests/host/test_daemon_entry.py b/tests/host/test_daemon_entry.py new file mode 100644 index 0000000000..5d53ef8735 --- /dev/null +++ b/tests/host/test_daemon_entry.py @@ -0,0 +1,24 @@ +"""Tests for the background host daemon process entry point.""" + +from __future__ import annotations + +import sys + +import pytest + +from omnigent import _platform +from omnigent.host import _daemon_entry + + +def test_daemon_configures_stdio_before_argument_validation( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The standalone daemon receives the same Windows stdio hardening as the CLI.""" + calls: list[None] = [] + monkeypatch.setattr(_platform, "configure_cli_stdio", lambda: calls.append(None)) + monkeypatch.setattr(sys, "argv", ["omnigent.host._daemon_entry"]) + + with pytest.raises(SystemExit): + _daemon_entry.main() + + assert calls == [None] diff --git a/tests/inner/test_proc_and_platform.py b/tests/inner/test_proc_and_platform.py index b1e5c25b9f..5ccca6b3cc 100644 --- a/tests/inner/test_proc_and_platform.py +++ b/tests/inner/test_proc_and_platform.py @@ -9,8 +9,10 @@ from __future__ import annotations import contextlib +import io import os import subprocess +import sys import time from pathlib import Path @@ -21,6 +23,13 @@ from omnigent.inner import _proc +class _TTYTextIOWrapper(io.TextIOWrapper): + """Text stream stand-in that reports an attached terminal.""" + + def isatty(self) -> bool: + return True + + def _spin_cmd() -> list[str]: """A short-lived child process that does nothing but sleep.""" if os.name == "nt": @@ -40,6 +49,81 @@ def test_platform_flags_are_mutually_consistent() -> None: assert _platform.IS_WINDOWS != _platform.IS_POSIX +def test_configure_cli_stdio_noop_off_windows(monkeypatch: pytest.MonkeyPatch) -> None: + """POSIX (and forced-off Windows) must not touch streams.""" + monkeypatch.setattr(_platform, "IS_WINDOWS", False) + assert _platform.configure_cli_stdio() is False + + +def test_configure_cli_stdio_reconfigures_gbk_console_streams( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Legacy GBK TextIOWrappers become UTF-8 so emoji / ✓ are printable.""" + monkeypatch.setattr(_platform, "IS_WINDOWS", True) + output_cp_calls: list[None] = [] + monkeypatch.setattr( + _platform, + "_set_windows_console_output_utf8", + lambda: output_cp_calls.append(None), + ) + out = _TTYTextIOWrapper(io.BytesIO(), encoding="gbk", errors="strict", write_through=True) + err = _TTYTextIOWrapper(io.BytesIO(), encoding="gbk", errors="strict", write_through=True) + monkeypatch.setattr(sys, "stdout", out) + monkeypatch.setattr(sys, "stderr", err) + # Probe: the pre-fix stream rejects the host banner glyph. + with pytest.raises(UnicodeEncodeError): + out.write("✓") + out.seek(0) + out.truncate() + + assert _platform.configure_cli_stdio() is True + assert out.encoding == "utf-8" + assert err.encoding == "utf-8" + assert output_cp_calls == [None] + # Must not raise — this is the Phase 0 CP936 failure mode. + print("✓ Connected 🖥️", file=out, flush=True) + out.seek(0) + assert "✓" in out.read() + + +def test_configure_cli_stdio_preserves_redirected_stream_encoding( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Pipes stay locale-decodable while replacing unsupported glyphs.""" + monkeypatch.setattr(_platform, "IS_WINDOWS", True) + output_cp_calls: list[None] = [] + monkeypatch.setattr( + _platform, + "_set_windows_console_output_utf8", + lambda: output_cp_calls.append(None), + ) + buf = io.BytesIO() + out = io.TextIOWrapper(buf, encoding="gbk", errors="strict", write_through=True) + monkeypatch.setattr(sys, "stdout", out) + monkeypatch.setattr(sys, "stderr", out) + + assert _platform.configure_cli_stdio() is True + assert out.encoding == "gbk" + assert out.errors == "replace" + assert output_cp_calls == [] + + print("✓ Connected", file=out, flush=True) + assert buf.getvalue().decode("gbk").splitlines() == ["? Connected"] + + +def test_safe_console_print_survives_gbk_strict_stream( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A glyph the stream cannot encode must not raise (host-tunnel safety).""" + buf = io.BytesIO() + gbk = io.TextIOWrapper(buf, encoding="gbk", errors="strict", write_through=True) + monkeypatch.setattr(sys, "stdout", gbk) + _platform.safe_console_print("✓ Connected as 'host'") + gbk.flush() + # Replacement bytes were written; no exception escaped. + assert buf.getvalue() + + def test_default_shell_argv_runs_an_echo() -> None: argv = _platform.default_shell_argv("echo omnigent-shell-ok") out = subprocess.run(argv, capture_output=True, text=True, check=True) From 544c5e523008910e82d4ef2901e06de0601fd2a4 Mon Sep 17 00:00:00 2001 From: scwf Date: Thu, 23 Jul 2026 17:13:58 +0800 Subject: [PATCH 2/7] fix(windows): use UTF-8 at file and subprocess boundaries Read agent YAML, instruction files, skill metadata, MCP configs, and harness JSON as UTF-8 so Windows locale encodings cannot reject valid project content. Decode harness status output explicitly as UTF-8 and skip the POSIX orphan reaper when child-wait primitives are unavailable on Windows. Add regression coverage for UTF-8 artifacts, CLI status output, and Windows host cleanup. Signed-off-by: scwf --- 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 +- 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 +++++++++++++++++ 10 files changed, 103 insertions(+), 18 deletions(-) diff --git a/omnigent/host/connect.py b/omnigent/host/connect.py index 35a4bd6e09..f61c43c5b2 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 @@ -1912,11 +1919,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 a498685154..24825d6587 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 b5da00f100..0f864a67aa 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__}", @@ -1802,7 +1805,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 @@ -1843,7 +1846,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 @@ -2125,7 +2128,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 @@ -2393,7 +2396,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/tests/host/test_connect.py b/tests/host/test_connect.py index a9648543e9..28a22e4b16 100644 --- a/tests/host/test_connect.py +++ b/tests/host/test_connect.py @@ -16,6 +16,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, @@ -1180,6 +1181,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 1cbbc1bb97..cbe30cbdc6 100644 --- a/tests/inner/test_loader.py +++ b/tests/inner/test_loader.py @@ -420,6 +420,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 04409525f8..5b9efaf458 100644 --- a/tests/spec/test_parser.py +++ b/tests/spec/test_parser.py @@ -38,6 +38,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) From fc9b62e589f6b3696b9a6a4d051bb527d3f27b0c Mon Sep 17 00:00:00 2001 From: scwf Date: Thu, 23 Jul 2026 17:36:57 +0800 Subject: [PATCH 3/7] fix(runner): bypass proxies for server callbacks Windows system proxies could intercept loopback runner-to-server callbacks, causing managed token mint requests to return empty 502 responses and fail subsequent callbacks with misleading authentication errors. Disable environment proxy discovery for runner HTTP clients, allow managed mint refreshes with no token to fall back to a bare request, and clarify the non-managed refresh error. Add regression coverage for proxy bypass, transient mint failures, delegated-auth decline, and the updated error message. Signed-off-by: scwf --- omnigent/runner/_entry.py | 52 +++++++++++------ omnigent/runner/app.py | 2 + tests/runner/test_runner_entry.py | 97 ++++++++++++++++++++++++++++++- 3 files changed, 130 insertions(+), 21 deletions(-) diff --git a/omnigent/runner/_entry.py b/omnigent/runner/_entry.py index 08f3928b9a..d8406e4175 100644 --- a/omnigent/runner/_entry.py +++ b/omnigent/runner/_entry.py @@ -211,10 +211,12 @@ def auth_flow( ) -> Generator[httpx.Request, httpx.Response, None]: """Inject a fresh ``Authorization`` header before each request. - Fails closed: when the factory is configured but returns no - token (transient SDK failure), raises rather than silently - sending an unauthenticated request. Retries once with a freshly - minted token on either: + User-credential factories (OIDC / Databricks SDK) fail closed when + they return no token. Managed-mint factories that are declined or + temporarily empty send a bare request instead — local single-user + servers need no bearer, and auth-required servers answer with + 401/login redirect so the remint path below can recover. Retries + once with a freshly minted token on either: - HTTP 401 (the standard "your bearer is invalid" response), or - a 3xx redirect whose ``Location`` points at the Databricks @@ -228,9 +230,9 @@ def auth_flow( :param request: The outgoing httpx request. :yields: The request with the auth header set, or - unmodified when no factory is configured. - :raises httpx.RequestError: When the factory is configured - but returns no token. + unmodified when no factory is configured / mint declined. + :raises httpx.RequestError: When a user-credential factory is + configured but returns no token. """ # Workspace routing: name the workspace or the request routes to the # account (the forwarder's POST /events otherwise 403s). Empty when @@ -241,17 +243,24 @@ def auth_flow( request.headers.update(databricks_request_headers(self._server_url)) if self._factory is not None: token = self._factory() - if not token: - if getattr(self._factory, "declined", False): - # The server definitively refuses to mint for this runner - # (managed mint factory hit HTTP 400/404 after install — - # e.g. its construction probe lost a boot race to a - # no-auth server). Bare requests are correct there; do - # NOT fail closed or the runner bricks every callback. - yield request - return - raise httpx.RequestError("Databricks token refresh returned no token") - request.headers["Authorization"] = f"Bearer {token}" + if token: + request.headers["Authorization"] = f"Bearer {token}" + elif getattr(self._factory, "declined", False): + # Server definitively refuses to mint (HTTP 400/404 / Apps + # login redirect). Bare requests are correct for local + # single-user / header-mode servers. + pass + elif isinstance(self._factory, _ManagedMintTokenFactory): + # Mint returned nothing without declining (proxy 5xx, boot + # race 401, transient blip). Local servers accept bare + # requests; auth-required servers answer 401/login redirect + # and the remint path below recovers. + pass + else: + # User-credential factories (OIDC / Databricks SDK) must + # fail closed — a silent bare request would look like a + # mysterious 401 later. + raise httpx.RequestError("server auth token refresh returned no token") response = yield request if self._factory is None: return @@ -676,7 +685,9 @@ def _mint_managed_owner_token( RUNNER_TUNNEL_TOKEN_HEADER: binding_token, **databricks_request_headers(server_url), } - with httpx.Client(timeout=10.0) as client: + # Keep runner-to-server mint requests off system proxies, matching + # the local-server health checks. + with httpx.Client(timeout=10.0, trust_env=False) as client: response = client.post(mint_url, headers=headers) response.raise_for_status() payload = response.json() @@ -1031,6 +1042,9 @@ def create_app( # recorded for this server) routes these callbacks to the workspace. headers={"Origin": OMNIGENT_INTERNAL_WS_ORIGIN, **databricks_request_headers(server_url)}, timeout=httpx.Timeout(5.0, read=None), + # Bypass system proxies so loopback / Apps callbacks hit Omnigent + # directly (see ``_mint_managed_owner_token``). + trust_env=False, # NOTE: ``follow_redirects`` deliberately stays False. # ``_RunnerDatabricksAuth.auth_flow`` needs to *see* the # Databricks Apps OAuth login redirect (302 → diff --git a/omnigent/runner/app.py b/omnigent/runner/app.py index da423f3995..5183fd8b55 100644 --- a/omnigent/runner/app.py +++ b/omnigent/runner/app.py @@ -4116,6 +4116,7 @@ async def _codex_discover_thread_and_forward( headers=headers, auth=_RunnerDatabricksAuth(auth_factory), timeout=httpx.Timeout(10.0), + trust_env=False, ) as _ext_client: _ext_resp = await _ext_client.patch( f"/v1/sessions/{urllib.parse.quote(session_id, safe='')}", @@ -19852,6 +19853,7 @@ def create_runner_app_from_env() -> FastAPI: server_client = httpx.AsyncClient( base_url=server_url, timeout=httpx.Timeout(5.0, read=None), + trust_env=False, ) return create_runner_app(server_client=server_client) diff --git a/tests/runner/test_runner_entry.py b/tests/runner/test_runner_entry.py index adc32b57ef..b820783b4e 100644 --- a/tests/runner/test_runner_entry.py +++ b/tests/runner/test_runner_entry.py @@ -25,6 +25,7 @@ _load_runner_idle_timeout_s_from_config, _make_auth_token_factory, _make_managed_mint_factory, + _ManagedMintTokenFactory, _mint_managed_owner_token, _parent_is_orphaned, _parent_process_is_alive, @@ -584,12 +585,14 @@ def test_mint_managed_owner_token_posts_binding_token_and_parses_response( Locks the runner->server contract: POST /v1/runners/{id}/token with the tunnel binding token in ``X-Omnigent-Runner-Tunnel-Token``, - returning ``{"token", "expires_at"}``. + returning ``{"token", "expires_at"}``. Also asserts ``trust_env=False`` + so a Windows system HTTP proxy cannot swallow loopback mint calls + with an empty 502 that never reaches Omnigent. :param monkeypatch: Pytest environment patch fixture. :returns: None. """ - captured: dict[str, str] = {} + captured: dict[str, Any] = {} def _handler(request: httpx.Request) -> httpx.Response: """Capture the outgoing mint request and return a canned token.""" @@ -602,6 +605,7 @@ def _handler(request: httpx.Request) -> httpx.Response: def _fake_client(**kwargs: Any) -> httpx.Client: """Build a real sync client backed by the capturing MockTransport.""" + captured["client_kwargs"] = dict(kwargs) return real_client(transport=httpx.MockTransport(_handler), **kwargs) monkeypatch.setattr("omnigent.runner._entry.httpx.Client", _fake_client) @@ -617,6 +621,95 @@ def _fake_client(**kwargs: Any) -> httpx.Client: assert captured["method"] == "POST" assert captured["binding_token"] == "the-binding-token" assert captured["url"].endswith("/v1/runners/runner_token_abc/token") + assert captured["client_kwargs"].get("trust_env") is False + + +def test_managed_mint_transient_502_sends_bare_request( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A non-declining mint failure must not brick callbacks as Databricks. + + Reproduces the Windows + system-proxy failure mode: mint returns HTTP + 502 (empty body from the proxy) so the factory stays non-declined and + empty. Auth must send a bare request instead of raising the old + ``Databricks token refresh returned no token`` error that killed every + local single-user callback. + + :param monkeypatch: Pytest environment patch fixture. + :returns: None. + """ + + def _proxy_502(mint_url: str, server_url: str, binding_token: str) -> tuple[str, float]: + del server_url, binding_token + request = httpx.Request("POST", mint_url) + raise httpx.HTTPStatusError( + "Bad Gateway", + request=request, + response=httpx.Response(502, request=request), + ) + + monkeypatch.setattr("omnigent.runner._entry._mint_managed_owner_token", _proxy_502) + + factory = _ManagedMintTokenFactory( + "https://s.example.com/v1/runners/runner_token_abc/token", + "https://s.example.com", + "btok", + ) + assert factory() is None + assert factory.declined is False + + auth = _RunnerDatabricksAuth(factory) + request = httpx.Request("GET", "http://localhost:6767/v1/agents/ag_1/download") + sent = next(auth.auth_flow(request)) + assert "Authorization" not in sent.headers + + +def test_runner_auth_raises_clear_error_for_empty_user_credential_factory() -> None: + """OIDC/Databricks factories still fail closed with a neutral message. + + :returns: None. + """ + auth = _RunnerDatabricksAuth(lambda: None) + request = httpx.Request("GET", "http://server/v1/agents") + with pytest.raises(httpx.RequestError, match="server auth token refresh returned no token"): + next(auth.auth_flow(request)) + + +def test_make_auth_token_factory_declines_delegated_mint_on_local_header_mode( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Host ``DELEGATED_AUTH=1`` on a no-mint server ends with factory None. + + Local single-user / header mode returns HTTP 400 from mint. The + construction probe must decline so the runner uses bare requests + instead of installing a forever-empty managed-mint factory. + + :param monkeypatch: Pytest environment patch fixture. + :returns: None. + """ + from omnigent.inner.databricks_executor import DatabricksAuthError + + def _no_sdk(profile: str | None = None) -> tuple[Any, str]: + del profile + raise DatabricksAuthError("no Databricks credentials configured") + + def _refuse_mint(mint_url: str, server_url: str, binding_token: str) -> tuple[str, float]: + del server_url, binding_token + request = httpx.Request("POST", mint_url) + raise httpx.HTTPStatusError( + "unsupported in this auth mode", + request=request, + response=httpx.Response(400, request=request), + ) + + monkeypatch.setenv("RUNNER_SERVER_URL", "http://localhost:6767") + monkeypatch.setenv("OMNIGENT_RUNNER_TUNNEL_BINDING_TOKEN", "local-binding-token") + monkeypatch.setenv("OMNIGENT_RUNNER_DELEGATED_AUTH", "1") + monkeypatch.setattr("omnigent.cli_auth.load_token", lambda _url: None) + monkeypatch.setattr("omnigent.inner.databricks_executor._resolve_databricks_auth", _no_sdk) + monkeypatch.setattr("omnigent.runner._entry._mint_managed_owner_token", _refuse_mint) + + assert _make_auth_token_factory() is None def test_runner_databricks_auth_injects_fresh_token_per_request() -> None: From 131479779e1127b5881b838c29ccfaebc30dc494 Mon Sep 17 00:00:00 2001 From: scwf Date: Thu, 23 Jul 2026 19:42:28 +0800 Subject: [PATCH 4/7] fix(claude-sdk): spill system prompts to files Claude Agent SDK passed complete system prompts on the command line, exceeding the Windows CreateProcess command-length limit for larger agents such as Polly. Spill Windows and oversized prompts to temporary Markdown files, reuse them per session, and remove them when the session or executor closes. Add regression coverage for Polly-sized prompts, Windows short prompts, non-Windows inline prompts, cache reuse, updates, and cleanup. Signed-off-by: scwf --- omnigent/inner/claude_sdk_executor.py | 108 ++++++++++++++++++++- tests/inner/test_claude_sdk_executor.py | 120 +++++++++++++++++++++++- 2 files changed, 223 insertions(+), 5 deletions(-) diff --git a/omnigent/inner/claude_sdk_executor.py b/omnigent/inner/claude_sdk_executor.py index 618ba8fb06..9833b1cb7c 100644 --- a/omnigent/inner/claude_sdk_executor.py +++ b/omnigent/inner/claude_sdk_executor.py @@ -42,7 +42,7 @@ from types import ModuleType from typing import Any, Protocol, TypeAlias, cast -from omnigent._platform import resolve_cli_binary, stable_user_id +from omnigent._platform import IS_WINDOWS, resolve_cli_binary, stable_user_id from omnigent.inner import _proc from omnigent.inner.bundle_skills import ensure_bundle_plugin_manifest from omnigent.llms._usage_observer import notify_from_dict as _notify_usage_from_dict @@ -783,6 +783,52 @@ async def handler(args: ToolArgs) -> McpResponse: return mcp_tools +# Keep large prompts out of CLI argv; Windows CreateProcess has a +# particularly small command-line limit. +_CLI_INLINE_SYSTEM_PROMPT_MAX_CHARS = 4000 + + +def _system_prompt_for_cli( + system_prompt: str | None, + *, + spill_dir: pathlib.Path | None = None, +) -> str | dict[str, str] | None: + """Return a ClaudeAgentOptions ``system_prompt`` value safe for CLI argv. + + Short prompts stay inline. On Windows, or when the prompt exceeds + :data:`_CLI_INLINE_SYSTEM_PROMPT_MAX_CHARS`, write the text to a temp + file and return ``{"type": "file", "path": ...}`` so the Claude Agent + SDK passes ``--system-prompt-file`` instead of embedding the body in + the CreateProcess command line. + + :param system_prompt: Assembled system instructions, or ``None``. + :param spill_dir: Optional directory for spilled files; defaults to + the process temp dir. + :returns: The original string, a ``SystemPromptFile`` dict, or ``None``. + """ + if system_prompt is None: + return None + if not system_prompt: + return system_prompt + if not IS_WINDOWS and len(system_prompt) <= _CLI_INLINE_SYSTEM_PROMPT_MAX_CHARS: + return system_prompt + directory = spill_dir if spill_dir is not None else pathlib.Path(tempfile.gettempdir()) + directory.mkdir(parents=True, exist_ok=True) + # delete=False: the Claude CLI reads the path after we return; we unlink + # when the Omnigent session closes (see ClaudeSDKExecutor.close_session). + with tempfile.NamedTemporaryFile( + mode="w", + encoding="utf-8", + suffix=".md", + prefix="omnigent-claude-system-prompt-", + dir=directory, + delete=False, + ) as handle: + handle.write(system_prompt) + path = handle.name + return {"type": "file", "path": path} + + def _augment_system_prompt_for_omnigent_mcp_tools( system_prompt: str, tool_schemas: list[ToolSpec], @@ -1415,6 +1461,11 @@ def __init__( # Force-close tasks for clients evicted on turn cancellation, kept # referenced so they are not GC'd mid-close. self._cancel_close_tasks: set[asyncio.Task[None]] = set() + # Per-session source text and CLI-safe prompt value. + self._cli_system_prompts: dict[ + str, + tuple[str, str | dict[str, str] | None], + ] = {} # Prefer system-installed claude over the SDK's bundled CLI. # The bundled CLI may be older and send beta flags that the @@ -1498,6 +1549,56 @@ def __del__(self) -> None: if getattr(self, "_cli_wrapper_path", None): with suppress(Exception): pathlib.Path(self._cli_wrapper_path).unlink(missing_ok=True) + for _source, value in getattr(self, "_cli_system_prompts", {}).values(): + if isinstance(value, dict) and value.get("type") == "file": + with suppress(Exception): + pathlib.Path(value["path"]).unlink(missing_ok=True) + + def _resolve_cli_system_prompt( + self, + session_key: str, + system_prompt: str, + ) -> str | dict[str, str] | None: + """Return a CLI-safe system prompt, spilling to a file when needed. + + Caches per *session_key* so multi-turn reconnects reuse one spill + file instead of leaking a tempfile every turn. + + :param session_key: Omnigent session id used as the cache key. + :param system_prompt: Assembled system instructions for this turn. + :returns: Inline string, ``SystemPromptFile`` dict, or ``None``. + """ + cached = self._cli_system_prompts.get(session_key) + if cached is not None: + cached_source, cached_value = cached + if cached_source == system_prompt: + return cached_value + if isinstance(cached_value, dict) and cached_value.get("type") == "file": + should_spill = bool(system_prompt) and ( + IS_WINDOWS or len(system_prompt) > _CLI_INLINE_SYSTEM_PROMPT_MAX_CHARS + ) + if should_spill: + pathlib.Path(cached_value["path"]).write_text( + system_prompt, + encoding="utf-8", + ) + self._cli_system_prompts[session_key] = ( + system_prompt, + cached_value, + ) + return cached_value + pathlib.Path(cached_value["path"]).unlink(missing_ok=True) + resolved = _system_prompt_for_cli(system_prompt or None) + self._cli_system_prompts[session_key] = (system_prompt, resolved) + return resolved + + def _forget_cli_system_prompt(self, session_key: str) -> None: + """Drop and unlink any spilled system-prompt file for *session_key*.""" + cached = self._cli_system_prompts.pop(session_key, None) + value = cached[1] if cached is not None else None + if isinstance(value, dict) and value.get("type") == "file": + with suppress(OSError): + pathlib.Path(value["path"]).unlink(missing_ok=True) async def _route_options_through_gateway_shim(self, options: SdkOptions) -> None: """ @@ -1622,6 +1723,7 @@ def _tee_stderr(line: str) -> None: async def close_session(self, session_key: str) -> None: self._crashed_sessions.pop(session_key, None) + self._forget_cli_system_prompt(session_key) await self._close_live_client(session_key) async def _close_live_client(self, session_key: str) -> None: @@ -1673,7 +1775,7 @@ def _evict_client_on_cancel(self, session_key: str) -> None: task.add_done_callback(self._cancel_close_tasks.discard) async def close(self) -> None: - session_keys = list(self._clients) + session_keys = set(self._clients) | set(self._cli_system_prompts) for session_key in session_keys: await self.close_session(session_key) if self._gateway_shim is not None: @@ -2187,7 +2289,7 @@ def _on_stderr(line: str) -> None: bundle_plugins.append({"type": "local", "path": str(self._bundle_dir)}) options_kwargs: dict[str, Any] = { # type: ignore[explicit-any] # ClaudeAgentOptions accepts mixed-typed kwargs (str / list / dict / callable / etc.) "tools": base_tools, - "system_prompt": system_prompt or None, + "system_prompt": self._resolve_cli_system_prompt(session_key, system_prompt), "mcp_servers": mcp_servers if mcp_servers else {}, "allowed_tools": allowed_tools, "permission_mode": self._permission_mode, diff --git a/tests/inner/test_claude_sdk_executor.py b/tests/inner/test_claude_sdk_executor.py index 3e5c01610a..ed3ecb7560 100644 --- a/tests/inner/test_claude_sdk_executor.py +++ b/tests/inner/test_claude_sdk_executor.py @@ -35,6 +35,17 @@ def _run(coro): loop.close() +def _system_prompt_text(value: object) -> str | None: + """Resolve ClaudeAgentOptions.system_prompt whether inline or spilled to file.""" + if value is None: + return None + if isinstance(value, dict) and value.get("type") == "file": + return Path(value["path"]).read_text(encoding="utf-8") + if isinstance(value, str): + return value + raise AssertionError(f"unexpected system_prompt value: {value!r}") + + # --------------------------------------------------------------------------- # Tests: Prompt extraction # --------------------------------------------------------------------------- @@ -150,6 +161,111 @@ def test_historical_file_data_uri_is_replaced_with_compact_placeholder(self): self.assertIn("What does this document say?", prompt) +# --------------------------------------------------------------------------- +# Tests: Windows CLI argv limit / system-prompt-file spill +# --------------------------------------------------------------------------- + + +class TestSystemPromptForCli(unittest.TestCase): + """Windows CreateProcess argv limit requires --system-prompt-file spill.""" + + def test_oversized_prompt_spills_to_file(self): + from omnigent.inner.claude_sdk_executor import ( + _CLI_INLINE_SYSTEM_PROMPT_MAX_CHARS, + _system_prompt_for_cli, + ) + + body = "P" * (_CLI_INLINE_SYSTEM_PROMPT_MAX_CHARS + 1) + with tempfile.TemporaryDirectory() as tmp: + resolved = _system_prompt_for_cli(body, spill_dir=Path(tmp)) + self.assertIsInstance(resolved, dict) + assert isinstance(resolved, dict) + self.assertEqual(resolved["type"], "file") + path = Path(resolved["path"]) + self.assertTrue(path.is_file()) + self.assertEqual(path.read_text(encoding="utf-8"), body) + + def test_short_prompt_stays_inline_off_windows(self): + from omnigent.inner import claude_sdk_executor as mod + + with patch.object(mod, "IS_WINDOWS", False): + self.assertEqual(mod._system_prompt_for_cli("short prompt"), "short prompt") + + def test_short_prompt_spills_on_windows(self): + from omnigent.inner import claude_sdk_executor as mod + + with ( + patch.object(mod, "IS_WINDOWS", True), + tempfile.TemporaryDirectory() as tmp, + ): + resolved = mod._system_prompt_for_cli("windows short", spill_dir=Path(tmp)) + self.assertIsInstance(resolved, dict) + assert isinstance(resolved, dict) + self.assertEqual(resolved["type"], "file") + self.assertEqual( + Path(resolved["path"]).read_text(encoding="utf-8"), + "windows short", + ) + + def test_polly_sized_prompt_uses_file_shape(self): + """Regression: Polly's ~15KB prompt must not go on Windows argv.""" + import yaml + + from omnigent.inner.claude_sdk_executor import _system_prompt_for_cli + + polly_cfg = Path(__file__).resolve().parents[2] / "examples" / "polly" / "config.yaml" + if not polly_cfg.is_file(): + self.skipTest("examples/polly/config.yaml not present") + prompt = yaml.safe_load(polly_cfg.read_text(encoding="utf-8")).get("prompt") or "" + self.assertGreater(len(prompt), 8000) + with tempfile.TemporaryDirectory() as tmp: + resolved = _system_prompt_for_cli(prompt, spill_dir=Path(tmp)) + self.assertIsInstance(resolved, dict) + assert isinstance(resolved, dict) + self.assertEqual(resolved["type"], "file") + self.assertEqual(Path(resolved["path"]).read_text(encoding="utf-8"), prompt) + + def test_executor_caches_spill_per_session(self): + from omnigent.inner.claude_sdk_executor import ClaudeSDKExecutor + + executor = ClaudeSDKExecutor() + body = "X" * 9000 + first = executor._resolve_cli_system_prompt("sess-1", body) + second = executor._resolve_cli_system_prompt("sess-1", body) + self.assertEqual(first, second) + executor._forget_cli_system_prompt("sess-1") + if isinstance(first, dict): + self.assertFalse(Path(first["path"]).exists()) + + def test_executor_updates_cached_spill_when_prompt_changes(self): + from omnigent.inner.claude_sdk_executor import ClaudeSDKExecutor + + executor = ClaudeSDKExecutor() + first = executor._resolve_cli_system_prompt("sess-1", "A" * 9000) + second = executor._resolve_cli_system_prompt("sess-1", "B" * 9000) + self.assertEqual(first, second) + assert isinstance(second, dict) + self.assertEqual(Path(second["path"]).read_text(encoding="utf-8"), "B" * 9000) + executor._forget_cli_system_prompt("sess-1") + + def test_close_removes_cached_spill_without_live_client(self): + from omnigent.inner.claude_sdk_executor import ClaudeSDKExecutor + + async def _t(): + executor = ClaudeSDKExecutor() + resolved = executor._resolve_cli_system_prompt("sess-1", "X" * 9000) + assert isinstance(resolved, dict) + path = Path(resolved["path"]) + self.assertTrue(path.exists()) + + await executor.close() + + self.assertFalse(path.exists()) + self.assertEqual(executor._cli_system_prompts, {}) + + _run(_t()) + + # --------------------------------------------------------------------------- # Tests: Constructor and properties # --------------------------------------------------------------------------- @@ -1803,7 +1919,7 @@ async def _t(): self.assertIn("mcp__omnigent__sys_session_send", captured_options["allowed_tools"]) self.assertIn( "use `mcp__omnigent__sys_session_send` when instructions say `sys_session_send`", - captured_options["system_prompt"], + _system_prompt_text(captured_options["system_prompt"]), ) self.assertIsInstance(events[-1], TurnComplete) @@ -1898,7 +2014,7 @@ async def _t(): self.assertIn( "use `mcp__omnigent__sys_session_rename` when instructions say " "`sys_session_rename`", - captured_options["system_prompt"], + _system_prompt_text(captured_options["system_prompt"]), ) self.assertIsInstance(events[-1], TurnComplete) From c888eac5fc491c310d92a0bc2dff8477193a9005 Mon Sep 17 00:00:00 2001 From: scwf Date: Thu, 23 Jul 2026 20:35:23 +0800 Subject: [PATCH 5/7] fix(auth): make static key helpers Windows-safe Static API key helpers used the POSIX printf command, which is unavailable in Windows cmd.exe and allowed Claude Code to fall back to unrelated credentials from user settings. Generate a platform-safe Python helper with a base64 payload on Windows, use it across workflow and native provider paths, and run generic Codex gateway auth through PowerShell while retaining the Databricks shell path. Add regression coverage for exact key bytes, special Python paths, Claude and Codex provider wiring, Windows auth execution, and Databricks compatibility. Signed-off-by: scwf --- omnigent/_platform.py | 36 ++++++++++++ omnigent/claude_native.py | 6 +- omnigent/codex_native_app_server.py | 3 +- omnigent/inner/codex_executor.py | 42 +++++++++++--- omnigent/runtime/workflow.py | 17 +++--- tests/inner/test_codex_executor.py | 10 ++++ tests/inner/test_proc_and_platform.py | 43 ++++++++++++++ tests/runtime/test_provider_spawn_env.py | 41 ++++++++++---- tests/test_claude_native.py | 9 +-- tests/test_native_codex_provider.py | 71 ++++++++++++++++++++++-- 10 files changed, 239 insertions(+), 39 deletions(-) diff --git a/omnigent/_platform.py b/omnigent/_platform.py index 8b2995b617..92fbcf90d8 100644 --- a/omnigent/_platform.py +++ b/omnigent/_platform.py @@ -386,3 +386,39 @@ def resolve_repo_symlink(path: Path) -> Path: if candidate.exists(): return candidate.resolve() return path + + +def static_api_key_print_command(api_key: str) -> str: + """Return a shell command that prints *api_key* with no trailing newline. + + Used as Claude/Codex ``apiKeyHelper`` / gateway auth commands when the + credential is a static secret. POSIX hosts use ``printf %s`` (no newline, + unlike ``echo``). Native Windows ``cmd.exe`` has no ``printf``, so a bare + ``printf`` helper fails and Claude Code silently falls back to + ``~/.claude/settings.json`` tokens — which produced false "rate_limit 429" + errors against leftover coding-plan quotas. On Windows we therefore emit a + ``sys.executable -c …`` command with a base64 payload (shell-safe argv) so + both ``cmd.exe`` and Git Bash print the exact key bytes. + + :param api_key: The bearer / API key to print, e.g. ``"sk-..."``. + :returns: A shell command string suitable for ``apiKeyHelper``. + """ + import base64 + import shlex + + if not IS_WINDOWS: + return f"printf %s {shlex.quote(api_key)}" + payload = base64.b64encode(api_key.encode("utf-8")).decode("ascii") + # Forward slashes: accepted by cmd.exe and safe under Git Bash (where + # ``D:\code`` would otherwise treat ``\c`` as an escape). + # Normalize explicitly instead of relying on Path.as_posix(): Linux CI + # exercises this branch with a mocked Windows path, which pathlib treats + # as an opaque POSIX filename and therefore leaves backslashes unchanged. + exe_path = str(sys.executable).replace("\\", "/") + exe = f'"{exe_path}"' + # argv[1] is base64 (safe charset); -c string stays ASCII-only. + return ( + f"{exe} -c " + f'"import base64,sys;sys.stdout.buffer.write(base64.b64decode(sys.argv[1]))" ' + f"{payload}" + ) diff --git a/omnigent/claude_native.py b/omnigent/claude_native.py index b15ad8fdc8..49040b33f7 100644 --- a/omnigent/claude_native.py +++ b/omnigent/claude_native.py @@ -15,7 +15,6 @@ import os import re import secrets -import shlex import shutil import signal import subprocess @@ -47,6 +46,7 @@ from websockets.frames import Close from omnigent._native_resume_hint import echo_native_resume_hint +from omnigent._platform import static_api_key_print_command from omnigent._runner_startup import RunnerStartupProgress, runner_startup_progress from omnigent._startup_profile import StartupProfiler from omnigent._terminal_picker_theme import ( @@ -1723,13 +1723,13 @@ def _provider_config_for_native_claude(entry: ProviderEntry) -> ClaudeNativeUcod ) return None # Token delivery mirrors the claude-sdk executor: a dynamic auth_command - # is used verbatim; a static key becomes a ``printf`` apiKeyHelper (the + # is used verbatim; a static key becomes a platform-safe apiKeyHelper (the # runner env allowlist excludes ANTHROPIC_API_KEY, so the key must reach # Claude Code via the helper, not the environment). if family.auth_command: api_key_helper = family.auth_command elif family.api_key: - api_key_helper = f"printf %s {shlex.quote(family.api_key)}" + api_key_helper = static_api_key_print_command(family.api_key) else: _logger.warning( "native-claude: provider %r is the Claude default but has no usable " diff --git a/omnigent/codex_native_app_server.py b/omnigent/codex_native_app_server.py index 90c7beb6b0..5e7c1d5022 100644 --- a/omnigent/codex_native_app_server.py +++ b/omnigent/codex_native_app_server.py @@ -23,6 +23,7 @@ if TYPE_CHECKING: from omnigent.onboarding.provider_config import ProviderEntry +from omnigent._platform import static_api_key_print_command from omnigent.codex_native_bridge import write_policy_hook_config from omnigent.codex_native_process_registry import ( CodexNativeProcessOwnerLock, @@ -1369,7 +1370,7 @@ def _codex_provider_launch(entry: ProviderEntry, model: str | None) -> NativeCod if family.auth_command: auth_command = family.auth_command elif family.api_key: - auth_command = f"printf %s {shlex.quote(family.api_key)}" + auth_command = static_api_key_print_command(family.api_key) else: # Serves openai but carries no usable credential. return None diff --git a/omnigent/inner/codex_executor.py b/omnigent/inner/codex_executor.py index 50e1f0a5b1..7eb4d2996c 100644 --- a/omnigent/inner/codex_executor.py +++ b/omnigent/inner/codex_executor.py @@ -22,7 +22,7 @@ from pathlib import Path from typing import Any, Protocol, TypeAlias -from omnigent._platform import resolve_cli_binary +from omnigent._platform import IS_WINDOWS, resolve_cli_binary from omnigent.llms._usage_observer import notify_from_dict as _notify_usage_from_dict from omnigent.reasoning_effort import CODEX_EFFORTS, validate_effort from omnigent.runner.identity import OMNIGENT_SESSION_ENV_VAR @@ -810,6 +810,7 @@ def _databricks_codex_config_overrides( base_url: str, auth_command: str, auth_refresh_interval_ms: int | None = None, + platform_auth_shell: bool = False, ) -> list[str]: """Return TOML-fragment overrides for the Codex per-conversation config. @@ -821,10 +822,15 @@ def _databricks_codex_config_overrides( a bearer token, e.g. ``"databricks auth token --host ..."``. :param auth_refresh_interval_ms: Refresh cadence in milliseconds, e.g. ``900000``. + :param platform_auth_shell: Use the native platform shell for a + caller-supplied generic auth command. :returns: Codex TOML-fragment override strings. """ provider_name = "omnigent_databricks" - auth_command_json = json.dumps(auth_command) + auth_program, auth_args = _codex_auth_process( + auth_command, + platform_auth_shell=platform_auth_shell, + ) return [ f"model={json.dumps(model)}", f'model_provider="{provider_name}"', @@ -833,8 +839,8 @@ def _databricks_codex_config_overrides( "model_providers.omnigent_databricks=" '{name="Omnigent Databricks",' f"base_url={json.dumps(base_url)}," - 'auth={command="sh",' - f'args=["-c",{auth_command_json}],' + f"auth={{command={json.dumps(auth_program)}," + f"args={json.dumps(auth_args)}," "timeout_ms=5000," f"refresh_interval_ms={auth_refresh_interval_ms or _GATEWAY_AUTH_REFRESH_MS}" "}," @@ -875,7 +881,12 @@ def _provider_codex_config_overrides( :returns: Codex TOML-fragment override strings. """ provider_name = "omnigent_provider" - auth_command_json = json.dumps(auth_command) + auth_program, auth_args = _codex_auth_process( + auth_command, + platform_auth_shell=True, + ) + auth_program_json = json.dumps(auth_program) + auth_args_json = json.dumps(auth_args) # codex >= 0.137 removed the chat/completions wire from its config schema: # any provider block carrying wire_api="chat" makes codex hard-fail config # load ("wire_api = \"chat\" is no longer supported"), which broke OSS / @@ -894,8 +905,8 @@ def _provider_codex_config_overrides( f"model_providers.{provider_name}=" '{name="Omnigent Provider",' f"base_url={json.dumps(base_url)}," - 'auth={command="sh",' - f'args=["-c",{auth_command_json}],' + f"auth={{command={auth_program_json}," + f"args={auth_args_json}," "timeout_ms=5000," f"refresh_interval_ms={_GATEWAY_AUTH_REFRESH_MS}" "}," @@ -904,6 +915,20 @@ def _provider_codex_config_overrides( return overrides +def _codex_auth_process( + auth_command: str, + *, + platform_auth_shell: bool, +) -> tuple[str, list[str]]: + """Return the Codex auth executable and argv for *auth_command*.""" + if platform_auth_shell and IS_WINDOWS: + return ( + "powershell.exe", + ["-NoProfile", "-NonInteractive", "-Command", f"& {auth_command}"], + ) + return "sh", ["-c", auth_command] + + def _parse_optional_int(value: str | None) -> int | None: """Parse an optional integer env-var value. @@ -2223,6 +2248,7 @@ def __init__( self._gateway_uses_databricks_profile = False if gateway: host = self._gateway_host + platform_auth_shell = False # ``effective_model`` resolves to a concrete model for the codex # config. On the Databricks-profile-derivation branch (no gateway # host or base URL supplied directly) a ``databricks-*`` default is @@ -2270,6 +2296,7 @@ def __init__( ) base_url = base_url_override auth_command = gateway_auth_command + platform_auth_shell = True if model is None: # Directly-supplied neutral gateway: the Omnigent producer always # resolves a concrete model (spec > provider default > @@ -2290,6 +2317,7 @@ def __init__( base_url=base_url, auth_command=auth_command, auth_refresh_interval_ms=self._gateway_auth_refresh_interval_ms, + platform_auth_shell=platform_auth_shell, ) ) if not enable_web_search: diff --git a/omnigent/runtime/workflow.py b/omnigent/runtime/workflow.py index 9b32081e63..0c1de9e76c 100644 --- a/omnigent/runtime/workflow.py +++ b/omnigent/runtime/workflow.py @@ -9,7 +9,6 @@ import json import logging import os -import shlex from dataclasses import dataclass, replace from pathlib import Path from typing import TYPE_CHECKING, Any, Literal @@ -25,6 +24,7 @@ # import annotations`` is in effect). from omnigent.inner.datamodel import OSEnvSpec +from omnigent._platform import static_api_key_print_command from omnigent.entities import ( NON_CONTENT_ITEM_TYPES, CompactionData, @@ -465,20 +465,19 @@ def _provider_auth_command(family: FamilyConfig) -> str: Mirrors the executors' transport contract: the executors' gateway path invokes a shell command that prints the bearer token. A static ``api_key`` (already resolved to plaintext by - :meth:`ProviderEntry.family`) becomes ``printf %s ``; - a user-supplied dynamic ``auth_command`` passes through verbatim. + :meth:`ProviderEntry.family`) becomes a platform-safe print command via + :func:`omnigent._platform.static_api_key_print_command`; a user-supplied + dynamic ``auth_command`` passes through verbatim. :param family: The resolved provider family (``base_url`` + secret expanded by :meth:`ProviderEntry.family`). :returns: A shell command that prints the bearer token to stdout, e.g. - ``"printf %s sk-or-abc"`` or the literal ``auth_command``. + ``"printf %s sk-or-abc"`` on POSIX, or the literal ``auth_command``. :raises OmnigentError: If the family carries neither a static ``api_key`` nor an ``auth_command`` (should not happen post-parse). """ if family.api_key is not None: - # printf %s avoids the trailing newline ``echo`` would add and is - # shell-safe for keys with special characters via shlex.quote. - return f"printf %s {shlex.quote(family.api_key)}" + return static_api_key_print_command(family.api_key) if family.auth_command is not None: return family.auth_command raise OmnigentError( @@ -1205,12 +1204,12 @@ def _build_claude_sdk_spawn_env( # synthesized-provider path above, so no profile or ucode wiring remains # here. The executor strips ANTHROPIC_API_KEY to force subscription auth # inside Claude Code, so the key is threaded via the CLI's apiKeyHelper - # (a shell command the CLI invokes; shlex.quote keeps it shell-safe). + # (a shell command the CLI invokes; platform-safe via static_api_key_print_command). auth_from_spec = spec.executor.auth if auth_from_spec is None: auth_from_spec = _load_global_auth() if isinstance(auth_from_spec, ApiKeyAuth) and auth_from_spec.api_key: - _key_cmd = f"printf %s {shlex.quote(auth_from_spec.api_key)}" + _key_cmd = static_api_key_print_command(auth_from_spec.api_key) env["HARNESS_CLAUDE_SDK_API_KEY_HELPER"] = _key_cmd if auth_from_spec.base_url: env["HARNESS_CLAUDE_SDK_GATEWAY_BASE_URL"] = auth_from_spec.base_url diff --git a/tests/inner/test_codex_executor.py b/tests/inner/test_codex_executor.py index 3aa3dee9bb..9f5af1160f 100644 --- a/tests/inner/test_codex_executor.py +++ b/tests/inner/test_codex_executor.py @@ -246,10 +246,13 @@ def test_constructor_databricks_flag_with_profile_uses_profile_credentials(self) self.assertNotIn('--profile "test-profile" --force-refresh', auth_override) def test_constructor_databricks_flag_with_host_override_skips_profile_lookup(self): + import tomllib + with ( patch("omnigent.inner.codex_executor._find_codex_cli", return_value="/usr/bin/codex"), patch.dict("os.environ", {}, clear=True), patch("omnigent.inner.codex_executor._databricks_gateway_host") as gateway_host, + patch("omnigent.inner.codex_executor.IS_WINDOWS", True), ): executor = CodexExecutor( gateway=True, @@ -274,6 +277,13 @@ def test_constructor_databricks_flag_with_host_override_skips_profile_lookup(sel "databricks auth token --host" in item for item in executor._codex_config_overrides ) ) + parsed = tomllib.loads("\n".join(executor._codex_config_overrides)) + auth = parsed["model_providers"]["omnigent_databricks"]["auth"] + self.assertEqual(auth["command"], "powershell.exe") + self.assertEqual( + auth["args"], + ["-NoProfile", "-NonInteractive", "-Command", "& printf token"], + ) def test_constructor_databricks_flag_with_host_override_requires_base_url(self): with ( diff --git a/tests/inner/test_proc_and_platform.py b/tests/inner/test_proc_and_platform.py index 5ccca6b3cc..71bccf2463 100644 --- a/tests/inner/test_proc_and_platform.py +++ b/tests/inner/test_proc_and_platform.py @@ -49,6 +49,49 @@ def test_platform_flags_are_mutually_consistent() -> None: assert _platform.IS_WINDOWS != _platform.IS_POSIX +def test_static_api_key_print_command_posix(monkeypatch: pytest.MonkeyPatch) -> None: + """POSIX emits ``printf %s`` with a shell-quoted key (no trailing newline).""" + monkeypatch.setattr(_platform, "IS_WINDOWS", False) + assert _platform.static_api_key_print_command("sk-simple") == "printf %s sk-simple" + assert _platform.static_api_key_print_command("sk with spaces") == "printf %s 'sk with spaces'" + + +def test_static_api_key_print_command_windows_prints_exact_bytes( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Windows helper must run under cmd.exe and print the key with no newline. + + Regression for bare ``printf`` apiKeyHelper: cmd.exe has no printf, so Claude + Code fell back to ~/.claude/settings.json tokens and surfaced false 429s. + """ + monkeypatch.setattr(_platform, "IS_WINDOWS", True) + key = "sk-win-test-key-with-$pecial&chars" + cmd = _platform.static_api_key_print_command(key) + assert "printf" not in cmd + # Claude's apiKeyHelper is typically invoked via the process shell (cmd on + # native Windows). Exercise that path, not just a POSIX sh -c. + completed = subprocess.run( + cmd, + shell=True, + check=True, + capture_output=True, + ) + assert completed.stdout == key.encode("utf-8") + assert completed.stderr == b"" + + +def test_static_api_key_print_command_windows_always_quotes_executable( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Shell metacharacters in the Python path must remain inert.""" + monkeypatch.setattr(_platform, "IS_WINDOWS", True) + monkeypatch.setattr(_platform.sys, "executable", r"C:\Tools&More\python.exe") + + cmd = _platform.static_api_key_print_command("sk-test") + + assert cmd.startswith('"C:/Tools&More/python.exe" -c ') + + def test_configure_cli_stdio_noop_off_windows(monkeypatch: pytest.MonkeyPatch) -> None: """POSIX (and forced-off Windows) must not touch streams.""" monkeypatch.setattr(_platform, "IS_WINDOWS", False) diff --git a/tests/runtime/test_provider_spawn_env.py b/tests/runtime/test_provider_spawn_env.py index f0988e57a2..c84711ddc8 100644 --- a/tests/runtime/test_provider_spawn_env.py +++ b/tests/runtime/test_provider_spawn_env.py @@ -25,6 +25,7 @@ import pytest import yaml as _yaml +from omnigent._platform import static_api_key_print_command from omnigent.runtime.workflow import ( _build_claude_sdk_spawn_env, _build_codex_spawn_env, @@ -211,8 +212,10 @@ def test_claude_sdk_uses_anthropic_global_default(config_home: Path) -> None: assert env["HARNESS_CLAUDE_SDK_GATEWAY_BASE_URL"] == "https://anthropic.example.com/v1" # Host is the origin (scheme://netloc) of the base URL, not the full URL. assert env["HARNESS_CLAUDE_SDK_GATEWAY_HOST"] == "https://anthropic.example.com" - # The static key becomes a printf command carrying the resolved secret. - assert env["HARNESS_CLAUDE_SDK_GATEWAY_AUTH_COMMAND"] == "printf %s sk-ant-secret" + # The static key becomes a platform-safe print command carrying the resolved secret. + assert env["HARNESS_CLAUDE_SDK_GATEWAY_AUTH_COMMAND"] == ( + static_api_key_print_command("sk-ant-secret") + ) # No spec model → the family's models.default supplies the model. assert env["HARNESS_CLAUDE_SDK_MODEL"] == "claude-default-model" @@ -241,7 +244,9 @@ def test_detected_ambient_key_routes_with_no_config( assert env["HARNESS_CLAUDE_SDK_GATEWAY"] == "true" assert env["HARNESS_CLAUDE_SDK_GATEWAY_BASE_URL"] == "https://api.anthropic.com" # The ambient key is carried as the printf auth command (resolved, not leaked as a ref). - assert env["HARNESS_CLAUDE_SDK_GATEWAY_AUTH_COMMAND"] == "printf %s sk-ant-detected" + assert env["HARNESS_CLAUDE_SDK_GATEWAY_AUTH_COMMAND"] == ( + static_api_key_print_command("sk-ant-detected") + ) # No pinned model on the detected entry → the catalog default fills in # (non-empty), rather than leaving the model unset. assert env["HARNESS_CLAUDE_SDK_MODEL"] @@ -289,7 +294,9 @@ def test_codex_uses_openai_global_default(config_home: Path) -> None: assert env["HARNESS_CODEX_GATEWAY"] == "true" assert env["HARNESS_CODEX_GATEWAY_BASE_URL"] == "https://openai.example.com/v1" assert env["HARNESS_CODEX_GATEWAY_HOST"] == "https://openai.example.com" - assert env["HARNESS_CODEX_GATEWAY_AUTH_COMMAND"] == "printf %s sk-oai-secret" + assert env["HARNESS_CODEX_GATEWAY_AUTH_COMMAND"] == ( + static_api_key_print_command("sk-oai-secret") + ) assert env["HARNESS_CODEX_MODEL"] == "gpt-default-model" # Codex defaults to the Responses wire API when the family omits wire_api. assert env["HARNESS_CODEX_WIRE_API"] == "responses" @@ -337,7 +344,9 @@ def test_codex_falls_back_to_first_available_openai_credential( # The fallback credentialed the head — full gateway wiring, same as a default. assert env["HARNESS_CODEX_GATEWAY"] == "true" assert env["HARNESS_CODEX_GATEWAY_BASE_URL"] == "https://openai.example.com/v1" - assert env["HARNESS_CODEX_GATEWAY_AUTH_COMMAND"] == "printf %s sk-oai-secret" + assert env["HARNESS_CODEX_GATEWAY_AUTH_COMMAND"] == ( + static_api_key_print_command("sk-oai-secret") + ) # Resolved per spawn — the user's config is NOT mutated (no default written). assert (config_home / "config.yaml").read_text() == before # The fallback is spawn-only: the readout-style resolver (flag off, the @@ -381,7 +390,9 @@ def test_claude_sdk_falls_back_to_first_available_anthropic_credential( assert env["HARNESS_CLAUDE_SDK_GATEWAY"] == "true" assert env["HARNESS_CLAUDE_SDK_GATEWAY_BASE_URL"] == "https://anthropic.example.com/v1" - assert env["HARNESS_CLAUDE_SDK_GATEWAY_AUTH_COMMAND"] == "printf %s sk-ant-secret" + assert env["HARNESS_CLAUDE_SDK_GATEWAY_AUTH_COMMAND"] == ( + static_api_key_print_command("sk-ant-secret") + ) assert (config_home / "config.yaml").read_text() == before assert _resolve_provider_for_build(spec, harness_type="claude-sdk") is None @@ -471,7 +482,9 @@ def test_pi_uses_anthropic_global_default(config_home: Path) -> None: '{"claude": "https://anthropic.example.com/v1"}' ) assert env["HARNESS_PI_GATEWAY_HOST"] == "https://anthropic.example.com" - assert env["HARNESS_PI_GATEWAY_AUTH_COMMAND"] == "printf %s sk-ant-secret" + assert env["HARNESS_PI_GATEWAY_AUTH_COMMAND"] == ( + static_api_key_print_command("sk-ant-secret") + ) assert env["HARNESS_PI_MODEL"] == "claude-default-model" @@ -512,7 +525,9 @@ def test_named_provider_auth_selects_provider_over_global_default(config_home: P # The NAMED provider, not the default, supplies the endpoint + key. assert env["HARNESS_CLAUDE_SDK_GATEWAY_BASE_URL"] == "https://named.example.com/v1" - assert env["HARNESS_CLAUDE_SDK_GATEWAY_AUTH_COMMAND"] == "printf %s sk-named" + assert env["HARNESS_CLAUDE_SDK_GATEWAY_AUTH_COMMAND"] == ( + static_api_key_print_command("sk-named") + ) assert env["HARNESS_CLAUDE_SDK_MODEL"] == "named-model" @@ -655,7 +670,9 @@ def test_claude_sdk_falls_back_to_catalog_default_model(config_home: Path) -> No # var that may ever appear is the profile, which is absent here # because this is a key provider, not a databricks-kind one. assert env["HARNESS_CLAUDE_SDK_GATEWAY"] == "true" - assert env["HARNESS_CLAUDE_SDK_GATEWAY_AUTH_COMMAND"] == "printf %s sk-ant-secret" + assert env["HARNESS_CLAUDE_SDK_GATEWAY_AUTH_COMMAND"] == ( + static_api_key_print_command("sk-ant-secret") + ) assert not any(k.startswith("HARNESS_CLAUDE_SDK_DATABRICKS") for k in env) @@ -736,7 +753,9 @@ def test_qwen_uses_openai_global_default(config_home: Path) -> None: assert env["HARNESS_QWEN_GATEWAY"] == "true" # The base URL host is the origin of the gateway endpoint assert env["HARNESS_QWEN_GATEWAY_HOST"] == "https://openai.example.com" - assert env["HARNESS_QWEN_GATEWAY_AUTH_COMMAND"] == "printf %s sk-oai-secret" + assert env["HARNESS_QWEN_GATEWAY_AUTH_COMMAND"] == ( + static_api_key_print_command("sk-oai-secret") + ) # Model comes from provider's default_model assert env["HARNESS_QWEN_MODEL"] == "gpt-default-model" @@ -950,7 +969,7 @@ def test_no_provider_api_key_path_unchanged(config_home: Path) -> None: env = _build_claude_sdk_spawn_env(spec, workdir=None) # Existing api_key path emits exactly what it did before. - assert env["HARNESS_CLAUDE_SDK_API_KEY_HELPER"] == "printf %s sk-direct" + assert env["HARNESS_CLAUDE_SDK_API_KEY_HELPER"] == (static_api_key_print_command("sk-direct")) # No provider gateway vars leak in (the provider branch did not fire). assert "HARNESS_CLAUDE_SDK_GATEWAY_BASE_URL" not in env assert "HARNESS_CLAUDE_SDK_GATEWAY_AUTH_COMMAND" not in env diff --git a/tests/test_claude_native.py b/tests/test_claude_native.py index 8eafa3c869..7501b42a1d 100644 --- a/tests/test_claude_native.py +++ b/tests/test_claude_native.py @@ -22,6 +22,7 @@ from websockets.frames import Close from omnigent import claude_native +from omnigent._platform import static_api_key_print_command from omnigent._runner_startup import RunnerStartupProgress from omnigent._startup_profile import StartupProfiler from omnigent._terminal_picker_theme import PICKER_ACCENT, PICKER_MUTED @@ -6127,7 +6128,7 @@ def test_provider_config_for_native_claude_key_injects_base_url_and_helper() -> "CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS": "1", } # Static key delivered via the apiKeyHelper, never the env (allowlist). - assert cfg.api_key_helper == "printf %s sk-ant-test" + assert cfg.api_key_helper == static_api_key_print_command("sk-ant-test") assert cfg.model == "claude-sonnet-4-6" @@ -6278,7 +6279,7 @@ def test_resolve_native_claude_config_spec_provider_default( cfg = claude_native.resolve_native_claude_config(spec=_no_auth_claude_spec()) assert cfg is not None assert cfg.env["ANTHROPIC_BASE_URL"] == "https://api.anthropic.com" - assert cfg.api_key_helper == "printf %s sk-ant-default" + assert cfg.api_key_helper == static_api_key_print_command("sk-ant-default") def test_resolve_native_claude_config_subscription_uses_cli_login( @@ -6369,7 +6370,7 @@ def test_resolve_native_claude_config_ambient_key( assert cfg is not None assert cfg.env["ANTHROPIC_BASE_URL"] == "https://api.anthropic.com" # Resolved from the env ref, delivered via the helper (no secret in env). - assert cfg.api_key_helper == "printf %s sk-ant-ambient" + assert cfg.api_key_helper == static_api_key_print_command("sk-ant-ambient") def test_resolve_native_claude_config_ambient_prefixed_key( @@ -6386,7 +6387,7 @@ def test_resolve_native_claude_config_ambient_prefixed_key( "ANTHROPIC_BASE_URL": "https://api.anthropic.com", "CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS": "1", } - assert cfg.api_key_helper == "printf %s sk-ant-prefixed" + assert cfg.api_key_helper == static_api_key_print_command("sk-ant-prefixed") def test_bedrock_config_auth_command_failure_returns_none() -> None: diff --git a/tests/test_native_codex_provider.py b/tests/test_native_codex_provider.py index 505ed2374a..6f46376ea8 100644 --- a/tests/test_native_codex_provider.py +++ b/tests/test_native_codex_provider.py @@ -10,11 +10,14 @@ from __future__ import annotations +import subprocess from pathlib import Path import pytest +import tomllib import yaml +from omnigent._platform import IS_WINDOWS, static_api_key_print_command from omnigent.codex_native_app_server import resolve_native_codex_launch from omnigent.inner.codex_executor import _provider_codex_config_overrides @@ -77,10 +80,68 @@ def test_provider_codex_overrides_coerce_chat_wire_to_responses() -> None: # chat is coerced to responses; codex >= 0.137 rejects a chat config. assert 'wire_api="responses"' in joined assert 'wire_api="chat"' not in joined - # The token command is embedded as the sh auth command. + # The token command is embedded in the platform shell auth config. assert "printf %s sk-or-test" in joined +@pytest.mark.parametrize( + ("is_windows", "expected_command", "expected_args_prefix"), + [ + (False, "sh", ["-c"]), + ( + True, + "powershell.exe", + ["-NoProfile", "-NonInteractive", "-Command"], + ), + ], +) +def test_provider_codex_overrides_use_platform_auth_shell( + monkeypatch: pytest.MonkeyPatch, + is_windows: bool, + expected_command: str, + expected_args_prefix: list[str], +) -> None: + """The final Codex auth process must exist on the target platform.""" + monkeypatch.setattr("omnigent.inner.codex_executor.IS_WINDOWS", is_windows) + auth_command = static_api_key_print_command("sk-test") + + overrides = _provider_codex_config_overrides( + model="gpt-5.5", + base_url="https://api.openai.com/v1", + auth_command=auth_command, + wire_api="responses", + ) + + parsed = tomllib.loads("\n".join(overrides)) + auth = parsed["model_providers"]["omnigent_provider"]["auth"] + assert auth["command"] == expected_command + expected_command_arg = f"& {auth_command}" if is_windows else auth_command + assert auth["args"] == [*expected_args_prefix, expected_command_arg] + + +@pytest.mark.skipif(not IS_WINDOWS, reason="requires Windows PowerShell") +def test_provider_codex_windows_auth_config_prints_exact_key() -> None: + """Execute the final Codex command/args shape, not only the inner helper.""" + key = "sk-win-test-key-with-$pecial&chars" + overrides = _provider_codex_config_overrides( + model="gpt-5.5", + base_url="https://api.openai.com/v1", + auth_command=static_api_key_print_command(key), + wire_api="responses", + ) + parsed = tomllib.loads("\n".join(overrides)) + auth = parsed["model_providers"]["omnigent_provider"]["auth"] + + completed = subprocess.run( + [auth["command"], *auth["args"]], + check=True, + capture_output=True, + ) + + assert completed.stdout == key.encode("utf-8") + assert completed.stderr == b"" + + def test_provider_codex_overrides_preserve_responses_wire() -> None: """An explicit ``responses`` wire passes through unchanged.""" overrides = _provider_codex_config_overrides( @@ -133,7 +194,9 @@ def test_resolve_native_codex_launch_key_default_routes_via_overrides( assert launch.model == "gpt-5.5" joined = "\n".join(launch.config_overrides) assert 'base_url="https://api.openai.com/v1"' in joined - assert "printf %s sk-oai-default" in joined + # Auth command is shell/JSON-escaped in the override; the trailing payload + # (key on POSIX, base64 on Windows) still appears literally. + assert static_api_key_print_command("sk-oai-default").rsplit(None, 1)[-1] in joined def test_resolve_native_codex_launch_openrouter_coerces_chat_wire(_isolated: Path) -> None: @@ -272,7 +335,7 @@ def test_resolve_native_codex_launch_subscription_no_login_falls_through_to_key( assert launch.model == "gpt-5.5" joined = "\n".join(launch.config_overrides) assert 'base_url="https://api.openai.com/v1"' in joined - assert "printf %s sk-oai-real" in joined + assert static_api_key_print_command("sk-oai-real").rsplit(None, 1)[-1] in joined def test_resolve_native_codex_launch_subscription_no_login_no_alternative_uses_login( @@ -343,7 +406,7 @@ def test_resolve_native_codex_launch_ambient_key_routes( assert launch.profile is None joined = "\n".join(launch.config_overrides) assert 'base_url="https://api.openai.com/v1"' in joined - assert "printf %s sk-oai-ambient" in joined + assert static_api_key_print_command("sk-oai-ambient").rsplit(None, 1)[-1] in joined def test_resolve_native_codex_launch_cli_config_default_pins_provider( From a3ed6f6de56b1290e531db97ee20309f5008116d Mon Sep 17 00:00:00 2001 From: scwf Date: Fri, 24 Jul 2026 09:11:31 +0800 Subject: [PATCH 6/7] fix(os-env): create Windows helper config tmpdir Windows OS helpers receive configuration through a temporary file because subprocess pass_fds is unavailable. Inactive sandbox policies previously had no private temporary directory, so sandbox.type none failed before sys_os_shell or sys_os_read could start the helper. Create the private directory for every Windows helper while keeping sandbox-specific write roots, environment, and backend setup gated on an active sandbox. Add a cross-platform helper startup regression test and keep the PYTHONPATH integration assertion limited to POSIX shells. Signed-off-by: scwf --- omnigent/inner/os_env.py | 14 +++++++++++--- tests/inner/test_os_env.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/omnigent/inner/os_env.py b/omnigent/inner/os_env.py index a38a928472..c4f1f61214 100644 --- a/omnigent/inner/os_env.py +++ b/omnigent/inner/os_env.py @@ -441,6 +441,11 @@ def _start_locked(self) -> None: parent_env=credential_parent_env, ) env.update(credential_runtime.helper_env_updates) + elif IS_WINDOWS: + # Windows delivers helper config via ``--config-file`` (no + # ``pass_fds``). Inactive sandboxes still need a private scratch + # dir for that ephemeral file; POSIX uses a pipe and skips this. + self._tmpdir = create_private_tmpdir() # Start L7 egress proxy if rules are configured. The proxy # listens on a Unix socket in the scratch tmpdir; the helper @@ -490,9 +495,12 @@ def _start_locked(self) -> None: # ``pass_fds`` / fd inheritance in ``subprocess`` (CPython rejects # ``pass_fds`` outright there), so fall back to a short-lived file in the # helper's own private tmpdir, which the helper reads and unlinks - # immediately. Egress is POSIX-only (its proxy binds a Unix socket), so - # the Windows config carries no secret — only non-sensitive - # paths/booleans — but we still keep the file private and ephemeral. + # immediately. That tmpdir is created above for every Windows helper — + # including inactive ``sandbox.type: none`` — because the config-file + # path is mandatory on Windows regardless of sandbox activity. Egress + # is POSIX-only (its proxy binds a Unix socket), so the Windows config + # carries no secret — only non-sensitive paths/booleans — but we still + # keep the file private and ephemeral. r_fd: int | None = None if IS_WINDOWS: assert self._tmpdir is not None diff --git a/tests/inner/test_os_env.py b/tests/inner/test_os_env.py index 1654b560f0..ea6a3491a5 100644 --- a/tests/inner/test_os_env.py +++ b/tests/inner/test_os_env.py @@ -373,6 +373,7 @@ def test_child_shell_env_noop_without_pythonpath( # --------------------------------------------------------------------------- +@pytest.mark.posix_only def test_shell_command_does_not_see_omnigent_project_root( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -384,6 +385,12 @@ def test_shell_command_does_not_see_omnigent_project_root( sees the sibling project entry but not omnigent's, so project subprocesses resolve their own packages. + POSIX-only: Windows often resolves ``bash`` to WSL's ``bash.exe``, which + does not inherit the Windows process ``PYTHONPATH`` the way a native + POSIX shell does. The strip logic itself is covered by the + ``_child_shell_env`` unit tests above; helper startup on Windows is + covered by :func:`test_inactive_sandbox_helper_starts`. + :returns: None. """ project_entry = "/opt/venvs/proj/site-packages" @@ -401,3 +408,28 @@ def test_shell_command_does_not_see_omnigent_project_root( out = result.get("stdout", "") assert project_entry in out assert str(_project_root()) not in out + + +def test_inactive_sandbox_helper_starts() -> None: + """``sandbox.type: none`` must start the OS helper on every platform. + + Regression for Windows: config is delivered via ``--config-file`` in a + private tmpdir, but that tmpdir used to be created only when + ``sandbox.active``. Inactive policies then hit + ``assert self._tmpdir is not None`` before the helper could spawn, so + ``sys_os_shell`` / ``sys_os_read`` failed immediately. + + :returns: None. + """ + os_env = create_os_environment( + OSEnvSpec(type="caller_process", sandbox=OSEnvSandboxSpec(type="none")) + ) + assert os_env is not None + try: + result = asyncio.run(os_env.shell("echo omnigent-osenv-ok")) + finally: + os_env.close() + + assert "error" not in result, result + assert result.get("exit_code") == 0 + assert "omnigent-osenv-ok" in (result.get("stdout") or "") From 369483776dbaeb8b2422a0995650625ad7f6487e Mon Sep 17 00:00:00 2001 From: scwf Date: Fri, 24 Jul 2026 09:56:12 +0800 Subject: [PATCH 7/7] fix(claude-sdk): isolate managed gateway auth Claude Code user settings can inject authentication and endpoint variables with higher priority than process configuration, allowing stale local credentials to win the first managed gateway request. For Omnigent-managed gateways, provide invocation-local settings that clear competing credentials, pin the configured endpoint through the loopback shim, and fail loudly on invalid settings while preserving native authentication for non-gateway sessions. Add regression coverage for managed and non-managed authentication, gateway URL pinning, invalid settings, environment restoration, and shim startup safety. Signed-off-by: scwf --- omnigent/inner/claude_sdk_executor.py | 98 +++++++++++++-- tests/inner/test_claude_gateway_shim.py | 17 ++- tests/inner/test_claude_sdk_executor.py | 161 ++++++++++++++++++++++-- 3 files changed, 258 insertions(+), 18 deletions(-) diff --git a/omnigent/inner/claude_sdk_executor.py b/omnigent/inner/claude_sdk_executor.py index 9833b1cb7c..2284fca5d0 100644 --- a/omnigent/inner/claude_sdk_executor.py +++ b/omnigent/inner/claude_sdk_executor.py @@ -37,7 +37,7 @@ import tempfile import time from collections.abc import AsyncIterator, Awaitable, Callable, Iterator -from contextlib import contextmanager, suppress +from contextlib import contextmanager, nullcontext, suppress from dataclasses import dataclass from types import ModuleType from typing import Any, Protocol, TypeAlias, cast @@ -636,6 +636,71 @@ def _unset_env_var(name: str) -> Iterator[None]: os.environ[name] = previous +def _cli_settings_for_api_key_helper( + api_key_helper: str, + *, + base_url: str | None = None, + isolate_user_auth: bool = False, +) -> str: + """Build Claude CLI ``--settings`` JSON for an Omnigent apiKeyHelper. + + Every helper is passed through invocation-local settings. When Omnigent + manages the gateway, the same higher-precedence payload also blanks user + auth and pins the configured base URL so ``~/.claude/settings.json`` cannot + redirect the session. + + :param api_key_helper: Shell command that prints the bearer token. + :param base_url: Gateway base URL to pin when *isolate_user_auth* is true. + :param isolate_user_auth: Isolate the managed gateway from user auth env. + :returns: Compact JSON string for ``ClaudeAgentOptions.settings``. + :raises ValueError: If managed gateway isolation has no base URL. + """ + settings: dict[str, object] = {"apiKeyHelper": api_key_helper} + if isolate_user_auth: + if not isinstance(base_url, str) or not base_url: + raise ValueError("Managed Claude gateway auth requires ANTHROPIC_BASE_URL") + settings["env"] = { + # Empty string = unset for provider selection (Claude Code docs). + "ANTHROPIC_AUTH_TOKEN": "", + "ANTHROPIC_API_KEY": "", + "ANTHROPIC_BASE_URL": base_url, + } + return json.dumps(settings, separators=(",", ":")) + + +def _pin_cli_settings_base_url(options: SdkOptions, base_url: str) -> None: + """Rewrite ``options.settings`` env so ``ANTHROPIC_BASE_URL`` matches *base_url*. + + The gateway shim rewrites ``options.env`` after settings JSON is built; + without this pin, a higher-precedence ``--settings`` ``env`` block would + keep the upstream URL and bypass the shim. Gateway settings are generated + by Omnigent, so an invalid shape is a configuration error rather than a + condition to ignore. + + :param options: SDK options whose ``settings`` string may carry ``env``. + :param base_url: Final base URL the CLI must use, e.g. the shim loopback. + :raises RuntimeError: If managed gateway settings cannot be updated safely. + """ + settings = getattr(options, "settings", None) + if not isinstance(settings, str) or not settings: + raise RuntimeError("Claude gateway SDK options are missing managed settings") + try: + body = json.loads(settings) + except json.JSONDecodeError as exc: + raise RuntimeError("Claude gateway SDK settings are not valid JSON") from exc + if ( + not isinstance(body, dict) + or not isinstance(body.get("apiKeyHelper"), str) + or not body["apiKeyHelper"] + ): + raise RuntimeError("Claude gateway SDK settings are missing apiKeyHelper") + settings_env = body.get("env") + if not isinstance(settings_env, dict): + raise RuntimeError("Claude gateway SDK settings are missing managed auth env") + settings_env["ANTHROPIC_BASE_URL"] = base_url + options.settings = json.dumps(body, separators=(",", ":")) + + _CLOSE_ATTR: str = "close" _TRANSPORT_ATTR: str = "transport" _ACLOSE_ATTR: str = "aclose" @@ -1626,10 +1691,16 @@ async def _route_options_through_gateway_shim(self, options: SdkOptions) -> None "ClaudeSDKExecutor(gateway=True) built SDK options without " "env['ANTHROPIC_BASE_URL']; cannot route through the gateway shim." ) + # Validate managed settings before allocating the loopback listener. + # Pinning the current upstream is a no-op for routing and ensures an + # invalid payload fails without leaving a started shim behind. + _pin_cli_settings_base_url(options, env["ANTHROPIC_BASE_URL"]) if self._gateway_shim is None: self._gateway_shim = ClaudeGatewayShim(upstream_base_url=env["ANTHROPIC_BASE_URL"]) await self._gateway_shim.start() env["ANTHROPIC_BASE_URL"] = self._gateway_shim.base_url + # ``--settings`` env outranks process env; keep it on the shim URL. + _pin_cli_settings_base_url(options, self._gateway_shim.base_url) async def _get_or_create_client( self, @@ -1660,13 +1731,15 @@ def _tee_stderr(line: str) -> None: # error. The SDK merges ``os.environ`` with ``options.env``, # so we unset in ``os.environ`` for the spawn window. # - # ANTHROPIC_API_KEY is also stripped so the CLI uses its - # subscription auth rather than a developer API key that - # would charge separately. Safe even in Databricks mode: - # ``options.settings`` explicitly sets apiKeyHelper and - # ``options.env`` sets the Databricks base URL, so the - # Claude CLI does not need an inherited Anthropic key. - with _unset_env_var("CLAUDECODE"), _unset_env_var("ANTHROPIC_API_KEY"): + # Managed gateways strip ANTHROPIC_API_KEY / + # ANTHROPIC_AUTH_TOKEN so parent-shell exports cannot bypass + # apiKeyHelper. Non-gateway sessions retain Claude Code's + # native auth behavior. + with ( + _unset_env_var("CLAUDECODE"), + _unset_env_var("ANTHROPIC_API_KEY") if self._gateway else nullcontext(), + _unset_env_var("ANTHROPIC_AUTH_TOKEN") if self._gateway else nullcontext(), + ): await asyncio.wait_for(client.connect(), timeout=_CONNECT_TIMEOUT_SECONDS) except asyncio.TimeoutError as exc: await self._force_close_client(client) @@ -2225,8 +2298,15 @@ async def run_turn( # ``""`` here would still leave an empty key in the child env. env = dict(self._extra_env) api_key_helper = env.pop(_CLAUDE_API_KEY_HELPER_ENV_KEY, None) + # Every Omnigent helper is invocation-local. Only a managed gateway + # additionally overrides user auth and base URL; ordinary Claude + # sessions retain their native ~/.claude/settings.json behavior. settings_payload = ( - json.dumps({"apiKeyHelper": api_key_helper}, separators=(",", ":")) + _cli_settings_for_api_key_helper( + api_key_helper, + base_url=env.get("ANTHROPIC_BASE_URL"), + isolate_user_auth=self._gateway, + ) if api_key_helper else None ) diff --git a/tests/inner/test_claude_gateway_shim.py b/tests/inner/test_claude_gateway_shim.py index 0e79ef9b1c..8580d97ca2 100644 --- a/tests/inner/test_claude_gateway_shim.py +++ b/tests/inner/test_claude_gateway_shim.py @@ -341,7 +341,10 @@ async def test_gateway_executor_routes_new_client_through_shim(monkeypatch) -> N ``_get_or_create_client`` (the seam where ``options.env`` is consumed); spawning the real CLI is infeasible in unit tests. """ - from omnigent.inner.claude_sdk_executor import ClaudeSDKExecutor + from omnigent.inner.claude_sdk_executor import ( + ClaudeSDKExecutor, + _cli_settings_for_api_key_helper, + ) from omnigent.inner.databricks_executor import DatabricksCredentials monkeypatch.setattr( @@ -376,8 +379,14 @@ class _StubSDK: from types import SimpleNamespace + upstream_base_url = "https://example.databricks.com/ai-gateway/anthropic" options = SimpleNamespace( - env={"ANTHROPIC_BASE_URL": "https://example.databricks.com/ai-gateway/anthropic"}, + env={"ANTHROPIC_BASE_URL": upstream_base_url}, + settings=_cli_settings_for_api_key_helper( + "databricks auth token --host https://example.databricks.com", + base_url=upstream_base_url, + isolate_user_auth=True, + ), stderr=None, ) try: @@ -393,6 +402,10 @@ class _StubSDK: assert connect_env["ANTHROPIC_BASE_URL"].startswith("http://127.0.0.1:") assert executor._gateway_shim is not None assert connect_env["ANTHROPIC_BASE_URL"] == executor._gateway_shim.base_url + settings = json.loads(options.settings) + assert settings["env"]["ANTHROPIC_BASE_URL"] == executor._gateway_shim.base_url + assert settings["env"]["ANTHROPIC_AUTH_TOKEN"] == "" + assert settings["env"]["ANTHROPIC_API_KEY"] == "" finally: if executor._gateway_shim is not None: await executor._gateway_shim.aclose() diff --git a/tests/inner/test_claude_sdk_executor.py b/tests/inner/test_claude_sdk_executor.py index ed3ecb7560..7f42fad6fd 100644 --- a/tests/inner/test_claude_sdk_executor.py +++ b/tests/inner/test_claude_sdk_executor.py @@ -1288,6 +1288,14 @@ async def _t(): settings["apiKeyHelper"], "databricks auth token --host https://host", ) + # Higher-precedence --settings env blanks user ~/.claude/settings.json + # auth so a personal coding-plan token cannot 401 the first request. + self.assertEqual(settings["env"]["ANTHROPIC_AUTH_TOKEN"], "") + self.assertEqual(settings["env"]["ANTHROPIC_API_KEY"], "") + self.assertEqual( + settings["env"]["ANTHROPIC_BASE_URL"], + shim_upstream["base_url"], + ) # The CLI talks to the loopback shim; the shim forwards to the # real gateway. A direct gateway URL here would mean the shim was # bypassed and opus thinking.display stays stripped. @@ -2686,6 +2694,86 @@ async def _t(): # --------------------------------------------------------------------------- +def test_cli_settings_for_managed_gateway_blanks_user_auth_env() -> None: + """``--settings`` must blank AUTH_TOKEN/API_KEY and pin the gateway URL. + + Regression: user ``~/.claude/settings.json`` often carries a personal + coding-plan ``ANTHROPIC_AUTH_TOKEN`` + ``ANTHROPIC_BASE_URL``. Those + settings-file env values outrank the process env, so the first request + after restart 401s against the wrong credential unless Omnigent's + higher-precedence ``--settings`` clears them. + """ + from omnigent.inner.claude_sdk_executor import _cli_settings_for_api_key_helper + + payload = json.loads( + _cli_settings_for_api_key_helper( + "printf %s sk-test", + base_url="https://api.deepseek.com/anthropic", + isolate_user_auth=True, + ) + ) + assert payload["apiKeyHelper"] == "printf %s sk-test" + assert payload["env"]["ANTHROPIC_AUTH_TOKEN"] == "" + assert payload["env"]["ANTHROPIC_API_KEY"] == "" + assert payload["env"]["ANTHROPIC_BASE_URL"] == "https://api.deepseek.com/anthropic" + + +def test_cli_settings_for_helper_only_preserves_user_auth_env() -> None: + """A non-gateway helper must not override Claude Code's user settings.""" + from omnigent.inner.claude_sdk_executor import _cli_settings_for_api_key_helper + + payload = json.loads(_cli_settings_for_api_key_helper("printf %s sk-test")) + + assert payload == {"apiKeyHelper": "printf %s sk-test"} + + +def test_cli_settings_for_managed_gateway_requires_base_url() -> None: + """Managed auth must fail loud instead of falling back to a user URL.""" + from omnigent.inner.claude_sdk_executor import _cli_settings_for_api_key_helper + + with pytest.raises(ValueError, match="requires ANTHROPIC_BASE_URL"): + _cli_settings_for_api_key_helper( + "printf %s sk-test", + isolate_user_auth=True, + ) + + +@pytest.mark.parametrize( + "settings", + [ + None, + "not-json", + "{}", + '{"apiKeyHelper":"printf %s sk-test"}', + ], +) +def test_pin_cli_settings_base_url_rejects_invalid_managed_settings(settings) -> None: + """Gateway shim routing must not silently accept an unsafe settings shape.""" + from omnigent.inner.claude_sdk_executor import _pin_cli_settings_base_url + + options = SimpleNamespace(settings=settings) + with pytest.raises(RuntimeError, match="Claude gateway SDK"): + _pin_cli_settings_base_url(options, "http://127.0.0.1:12345") + + +@pytest.mark.asyncio +async def test_gateway_rejects_invalid_settings_before_starting_shim() -> None: + """Invalid managed settings must fail before allocating a loopback shim.""" + from omnigent.inner.claude_sdk_executor import ClaudeSDKExecutor + + executor = ClaudeSDKExecutor() + executor._gateway = True + options = SimpleNamespace( + env={"ANTHROPIC_BASE_URL": "https://gateway.example/anthropic"}, + settings="{}", + ) + + with pytest.raises(RuntimeError, match="missing apiKeyHelper"): + await executor._route_options_through_gateway_shim(options) + + assert executor._gateway_shim is None + + def test_unset_env_var_removes_and_restores(monkeypatch): """Env var present before ``with`` is absent during, restored after.""" from omnigent.inner.claude_sdk_executor import _unset_env_var @@ -2755,14 +2843,16 @@ def test_non_databricks_model_without_routing_does_not_raise() -> None: @pytest.mark.asyncio -async def test_anthropic_api_key_stripped_during_connect(monkeypatch): - """``_get_or_create_client`` must strip ``ANTHROPIC_API_KEY`` from - ``os.environ`` during the ``connect()`` window so the Claude CLI - uses subscription auth instead of a developer API key. +async def test_managed_gateway_auth_stripped_during_connect(monkeypatch): + """A managed gateway must strip ``ANTHROPIC_API_KEY`` and + ``ANTHROPIC_AUTH_TOKEN`` from ``os.environ`` during the ``connect()`` + window so the Claude CLI uses apiKeyHelper instead of a developer API + key or a parent-shell coding-plan token. This test drives ``_get_or_create_client`` with a stub SDK and captures ``os.environ`` at the moment ``connect()`` is invoked, - ensuring both ``CLAUDECODE`` and ``ANTHROPIC_API_KEY`` are absent. + ensuring ``CLAUDECODE``, ``ANTHROPIC_API_KEY``, and + ``ANTHROPIC_AUTH_TOKEN`` are absent. """ from omnigent.inner.claude_sdk_executor import ClaudeSDKExecutor @@ -2795,9 +2885,20 @@ class _StubSDK: ClaudeSDKClient = _StubClient monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-test-secret") + monkeypatch.setenv("ANTHROPIC_AUTH_TOKEN", "settings-token-secret") monkeypatch.setenv("CLAUDECODE", "parent-value") executor = ClaudeSDKExecutor() + executor._gateway = True + + async def _route_through_gateway_shim(options) -> None: + """The environment-isolation test does not need a live shim.""" + + monkeypatch.setattr( + executor, + "_route_options_through_gateway_shim", + _route_through_gateway_shim, + ) options = SimpleNamespace() await executor._get_or_create_client( _StubSDK, # type: ignore[arg-type] @@ -2806,15 +2907,61 @@ class _StubSDK: model=None, ) - # Both keys must be absent at the moment connect() ran. + # Keys must be absent at the moment connect() ran. assert "ANTHROPIC_API_KEY" not in connect_env, ( "ANTHROPIC_API_KEY leaked into connect() -- subscription auth bypassed" ) + assert "ANTHROPIC_AUTH_TOKEN" not in connect_env, ( + "ANTHROPIC_AUTH_TOKEN leaked into connect() -- coding-plan token wins" + ) assert "CLAUDECODE" not in connect_env, ( "CLAUDECODE leaked into connect() -- nested-session error risk" ) - # Both are restored after _get_or_create_client returns. + # Restored after _get_or_create_client returns. + assert os.environ["ANTHROPIC_API_KEY"] == "sk-ant-test-secret" + assert os.environ["ANTHROPIC_AUTH_TOKEN"] == "settings-token-secret" + assert os.environ["CLAUDECODE"] == "parent-value" + + +@pytest.mark.asyncio +async def test_non_gateway_auth_preserved_during_connect(monkeypatch): + """A normal Claude session must retain its native process authentication.""" + from omnigent.inner.claude_sdk_executor import ClaudeSDKExecutor + + connect_env: dict[str, str] = {} + + class _StubClient: + def __init__(self, options): + self.options = options + self._query = None + self._transport = None + + async def connect(self) -> None: + connect_env.update(os.environ) + + async def disconnect(self) -> None: + return None + + class _StubSDK: + ClaudeSDKClient = _StubClient + + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-test-secret") + monkeypatch.setenv("ANTHROPIC_AUTH_TOKEN", "native-token-secret") + monkeypatch.setenv("CLAUDECODE", "parent-value") + + executor = ClaudeSDKExecutor() + await executor._get_or_create_client( + _StubSDK, # type: ignore[arg-type] + session_key="native-auth-session", + options=SimpleNamespace(), + model=None, + ) + + assert connect_env["ANTHROPIC_API_KEY"] == "sk-ant-test-secret" + assert connect_env["ANTHROPIC_AUTH_TOKEN"] == "native-token-secret" + assert "CLAUDECODE" not in connect_env assert os.environ["ANTHROPIC_API_KEY"] == "sk-ant-test-secret" + assert os.environ["ANTHROPIC_AUTH_TOKEN"] == "native-token-secret" assert os.environ["CLAUDECODE"] == "parent-value"