diff --git a/zenith/src/zenith_harness/acp_runner.py b/zenith/src/zenith_harness/acp_runner.py index a7a71a1..ec75364 100644 --- a/zenith/src/zenith_harness/acp_runner.py +++ b/zenith/src/zenith_harness/acp_runner.py @@ -5,9 +5,11 @@ import json import logging import os +import shlex import socket import subprocess import sys +import tomllib from dataclasses import dataclass, field from pathlib import Path from typing import Any, Awaitable, Callable, ClassVar, Coroutine, Literal @@ -118,13 +120,64 @@ def _augment_acp_command( return command -def _acp_subprocess_env(provider) -> dict[str, str]: +def _parse_codex_c_overrides(command: str) -> dict[str, Any]: + """Read -c/--config assignments after shell quoting; the last value wins. + + Values use TOML syntax where possible, with bare strings preserved as in + the Codex CLI. Parsing does not execute the command or expand shell values. + """ + try: + arguments = iter(shlex.split(command)) + except ValueError as exc: + raise ACPError("Invalid quoting in Codex ACP command") from exc + overrides: dict[str, Any] = {} + for argument in arguments: + if argument in ("-c", "--config"): + assignment = next(arguments, None) + if assignment is None: + raise ACPError("Codex config override requires key=value") + elif argument.startswith("--config="): + assignment = argument[len("--config="):] + else: + continue + key, separator, value = assignment.partition("=") + if not separator or not key.strip(): + raise ACPError("Codex config override requires key=value") + try: + parsed_value = tomllib.loads("value = " + value)["value"] + except tomllib.TOMLDecodeError: + parsed_value = value + overrides[key.strip()] = parsed_value + return overrides + + +def _acp_subprocess_env( + provider, + reasoning_effort: str | None = None, + acp_command: str | None = None, +) -> dict[str, str]: """Build the env handed to an ACP-agent subprocess. For codex we preserve PATH so node-based ACP adapters can launch via - `/usr/bin/env node`, and pass sandbox-disable hints through env. The - command line also receives `sandbox_mode="danger-full-access"` in - `_augment_acp_command`. + `/usr/bin/env node`, and pass sandbox-disable hints through env. + + The npm `@agentclientprotocol/codex-acp` adapter (v1.1.0) does not + parse `-c` overrides from argv — it reads configuration from the + `CODEX_CONFIG` env var (JSON). We build it here in three layers, + each overriding the previous: + + 1. User-supplied `CODEX_CONFIG` (ambient env, e.g. `{"model": "..."}`). + 2. `-c` overrides parsed from the augmented command string — these + include the user's custom model from `ZENITH_*_ACP_COMMAND` and + the sandbox/approval/effort flags appended by + `_augment_acp_command`. + 3. Zenith's three safety keys (`sandbox_mode`, `approval_policy`, + `model_reasoning_effort`) — always win, since sandbox/approval + are autonomy-safety requirements and effort is the per-role + resolved value. + + The `-c` flags remain in argv for `-c`-honoring codex-acp builds + (harmless if ignored by the npm adapter). For hermes the env is passed through unchanged. """ @@ -134,6 +187,34 @@ def _acp_subprocess_env(provider) -> dict[str, str]: # Env-var hints — harmless if codex ignores them. env["CODEX_SANDBOX"] = "danger-full-access" env["CODEX_DISABLE_SANDBOX"] = "1" + # The npm codex-acp adapter reads CODEX_CONFIG (JSON) for its + # configuration, not argv `-c` overrides. Build it in three + # layers so the user's custom model (from the command string) + # is preserved alongside zenith's safety config. + effort = reasoning_effort or "xhigh" + codex_config: dict[str, Any] = {} + # Layer 1: user-supplied CODEX_CONFIG. + existing = env.get("CODEX_CONFIG") + if existing is not None: + try: + parsed = json.loads(existing) + except json.JSONDecodeError as exc: + raise ACPError("CODEX_CONFIG must contain valid JSON") from exc + if not isinstance(parsed, dict): + raise ACPError("CODEX_CONFIG must be a JSON object") + codex_config = parsed + # Layer 2: -c overrides from the augmented command string + # (carries the user's custom model + zenith's -c flags). + if acp_command: + codex_config.update(_parse_codex_c_overrides(acp_command)) + # Layer 3: zenith's safety keys always win. + codex_config["sandbox_mode"] = "danger-full-access" + codex_config["approval_policy"] = "never" + codex_config["model_reasoning_effort"] = effort + try: + env["CODEX_CONFIG"] = json.dumps(codex_config, allow_nan=False) + except (TypeError, ValueError) as exc: + raise ACPError("Codex configuration must contain JSON-compatible values") from exc # hermes: no special env needed return env @@ -580,6 +661,11 @@ async def run_node( role_config.worker_provider, role_config.worker_reasoning_effort, ) + acp_env = _acp_subprocess_env( + role_config.worker_provider, + role_config.worker_reasoning_effort, + acp_command, + ) workspace_dir = str(Path(cwd).expanduser().resolve() if cwd else store.workspace_dir(project_id)) project_bucket = str(store.zenith_dir(project_id)) @@ -638,7 +724,7 @@ async def run_node( stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, cwd=workspace_dir, - env=_acp_subprocess_env(role_config.worker_provider), + env=acp_env, limit=SUBPROCESS_STREAM_LIMIT, ) progress_tracker = ACPProgressTracker(callback=progress_callback) @@ -750,6 +836,11 @@ async def run_terminal_review( role_config.worker_provider, role_config.worker_reasoning_effort, ) + acp_env = _acp_subprocess_env( + role_config.worker_provider, + role_config.worker_reasoning_effort, + acp_command, + ) workspace_dir = str(store.workspace_dir(project_id)) project_bucket = str(store.zenith_dir(project_id)) @@ -796,7 +887,7 @@ async def run_terminal_review( stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, cwd=workspace_dir, - env=_acp_subprocess_env(role_config.worker_provider), + env=acp_env, limit=SUBPROCESS_STREAM_LIMIT, ) tracker = ACPProgressTracker(callback=progress_callback) diff --git a/zenith/tests/test_acp_runner.py b/zenith/tests/test_acp_runner.py index 412ea69..5d64aae 100644 --- a/zenith/tests/test_acp_runner.py +++ b/zenith/tests/test_acp_runner.py @@ -13,13 +13,17 @@ import shutil import sys from pathlib import Path +from dataclasses import replace +from unittest.mock import AsyncMock import pytest from zenith_harness.acp_runner import ( ACPNodeRunner, + ACPError, _acp_subprocess_env, _augment_acp_command, + _parse_codex_c_overrides, ) from zenith_harness.providers import PROVIDERS from zenith_harness.assets import AssetLoader @@ -190,6 +194,179 @@ def test_codex_acp_env_preserves_node_path_when_bwrap_is_present( assert env["CODEX_DISABLE_SANDBOX"] == "1" +def test_codex_acp_env_includes_codex_config_json_with_effort(): + env = _acp_subprocess_env(PROVIDERS["codex"], reasoning_effort="max") + config = json.loads(env["CODEX_CONFIG"]) + assert config["sandbox_mode"] == "danger-full-access" + assert config["approval_policy"] == "never" + assert config["model_reasoning_effort"] == "max" + + +def test_codex_acp_env_codex_config_defaults_to_xhigh(): + env = _acp_subprocess_env(PROVIDERS["codex"]) + config = json.loads(env["CODEX_CONFIG"]) + assert config["model_reasoning_effort"] == "xhigh" + + +def test_codex_acp_env_merges_user_codex_config( + monkeypatch: pytest.MonkeyPatch, +): + monkeypatch.setenv("CODEX_CONFIG", json.dumps({"model": "gpt-5.6-luna"})) + env = _acp_subprocess_env(PROVIDERS["codex"], reasoning_effort="ultra") + config = json.loads(env["CODEX_CONFIG"]) + assert config["model"] == "gpt-5.6-luna" + assert config["sandbox_mode"] == "danger-full-access" + assert config["approval_policy"] == "never" + assert config["model_reasoning_effort"] == "ultra" + + +def test_codex_acp_env_overrides_user_safety_values( + monkeypatch: pytest.MonkeyPatch, +): + monkeypatch.setenv( + "CODEX_CONFIG", + json.dumps({"sandbox_mode": "read-only", "approval_policy": "on-failure"}), + ) + env = _acp_subprocess_env(PROVIDERS["codex"]) + config = json.loads(env["CODEX_CONFIG"]) + assert config["sandbox_mode"] == "danger-full-access" + assert config["approval_policy"] == "never" + + +@pytest.mark.parametrize("raw", ["not valid json", "", "[]", "null", '"text"', "42"]) +def test_invalid_codex_config_fails_explicitly(monkeypatch, raw): + monkeypatch.setenv("CODEX_CONFIG", raw) + with pytest.raises(ACPError, match="CODEX_CONFIG"): + _acp_subprocess_env(PROVIDERS["codex"], reasoning_effort="high") + + +@pytest.mark.parametrize("assignment", [ + 'model="chosen-model"', + "model=chosen-model", + "'model=\"chosen-model\"'", + 'model="old-model" -c model=chosen-model', +]) +def test_command_model_overrides_ambient_for_shell_quoting(monkeypatch, assignment): + monkeypatch.setenv("CODEX_CONFIG", '{"model":"ambient-model"}') + command = _augment_acp_command("codex-acp -c " + assignment, PROVIDERS["codex"], "high") + config = json.loads(_acp_subprocess_env(PROVIDERS["codex"], "high", command)["CODEX_CONFIG"]) + assert config["model"] == "chosen-model" + assert config["model_reasoning_effort"] == "high" + + +@pytest.mark.parametrize("command", [ + "codex-acp --config model=chosen-model", + "codex-acp --config=model=chosen-model", +]) +def test_long_config_option(command): + assert _parse_codex_c_overrides(command) == {"model": "chosen-model"} + + +@pytest.mark.parametrize("command", ["codex-acp -c", "codex-acp -c model", 'codex-acp -c model="']) +def test_malformed_override_is_reported(command): + with pytest.raises(ACPError): + _parse_codex_c_overrides(command) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("role", ["worker", "validator", "terminal_reviewer"]) +async def test_invalid_config_rejected_before_mcp_start(monkeypatch, config, role): + monkeypatch.setenv("CODEX_CONFIG", "invalid-json") + config = replace(config, worker_provider_name="codex", worker_acp_command="codex-acp") + runner = ACPNodeRunner(config, AssetLoader(config)) + worker_start = AsyncMock() + review_start = AsyncMock() + monkeypatch.setattr(runner, "_start_worker_mcp_server", worker_start) + monkeypatch.setattr(runner, "_start_terminal_reviewer_mcp", review_start) + store = ProjectStore(config) + with pytest.raises(ACPError, match="CODEX_CONFIG"): + if role == "terminal_reviewer": + await runner.run_terminal_review("p1", "mission-001", "attempt", store) + else: + task = Task(id="w1", type="validate" if role == "validator" else "work", + body="check", targets=[]) + await runner.run_node("p1", "mission-001", task, "attempt", store) + worker_start.assert_not_called() + review_start.assert_not_called() + + +@pytest.mark.parametrize("raw", ['{"value": NaN}', '{"value": Infinity}']) +def test_nonfinite_config_rejected(monkeypatch, raw): + monkeypatch.setenv("CODEX_CONFIG", raw) + with pytest.raises(ACPError, match="JSON-compatible"): + _acp_subprocess_env(PROVIDERS["codex"]) + + +def test_override_values_preserve_types_and_embedded_quotes(): + command = "codex-acp -c 'enabled=true' -c 'limit=3' -c 'label=\"a=b\"'" + assert _parse_codex_c_overrides(command) == {"enabled": True, "limit": 3, "label": "a=b"} + + +def test_claude_acp_env_has_no_codex_config( + monkeypatch: pytest.MonkeyPatch, +): + monkeypatch.delenv("CODEX_CONFIG", raising=False) + env = _acp_subprocess_env(PROVIDERS["claude"]) + assert "CODEX_CONFIG" not in env + + +def test_parse_codex_c_overrides_extracts_model(): + cmd = 'codex-acp -c model="gpt-5.6-luna" -c sandbox_mode="danger-full-access"' + overrides = _parse_codex_c_overrides(cmd) + assert overrides["model"] == "gpt-5.6-luna" + assert overrides["sandbox_mode"] == "danger-full-access" + + +def test_parse_codex_c_overrides_handles_single_quotes(): + cmd = "codex-acp -c model='gpt-5.6-luna'" + overrides = _parse_codex_c_overrides(cmd) + assert overrides["model"] == "gpt-5.6-luna" + + +def test_parse_codex_c_overrides_empty_when_no_flags(): + assert _parse_codex_c_overrides("codex-acp") == {} + + +def test_codex_acp_env_preserves_custom_model_from_command(): + """The user's custom model from ZENITH_*_ACP_COMMAND's -c model=... + must reach CODEX_CONFIG — the npm adapter drops all argv -c flags.""" + cmd = 'codex-acp -c model="gpt-5.6-luna" -c sandbox_mode="danger-full-access" -c approval_policy="never" -c model_reasoning_effort="max"' + env = _acp_subprocess_env( + PROVIDERS["codex"], reasoning_effort="max", acp_command=cmd + ) + config = json.loads(env["CODEX_CONFIG"]) + assert config["model"] == "gpt-5.6-luna" + assert config["sandbox_mode"] == "danger-full-access" + assert config["approval_policy"] == "never" + assert config["model_reasoning_effort"] == "max" + + +def test_codex_acp_env_model_from_command_overrides_user_codex_config( + monkeypatch: pytest.MonkeyPatch, +): + """The -c model in the command string (explicit per-role) wins over + an ambient CODEX_CONFIG model.""" + monkeypatch.setenv("CODEX_CONFIG", json.dumps({"model": "gpt-4o"})) + cmd = 'codex-acp -c model="gpt-5.6-luna"' + env = _acp_subprocess_env( + PROVIDERS["codex"], reasoning_effort="max", acp_command=cmd + ) + config = json.loads(env["CODEX_CONFIG"]) + assert config["model"] == "gpt-5.6-luna" + + +def test_codex_acp_env_without_command_keeps_user_codex_config_model( + monkeypatch: pytest.MonkeyPatch, +): + """When no acp_command is passed, a user-supplied CODEX_CONFIG model + is preserved (backward-compatible call signature).""" + monkeypatch.setenv("CODEX_CONFIG", json.dumps({"model": "gpt-4o"})) + env = _acp_subprocess_env(PROVIDERS["codex"], reasoning_effort="high") + config = json.loads(env["CODEX_CONFIG"]) + assert config["model"] == "gpt-4o" + assert config["model_reasoning_effort"] == "high" + + def test_attempt_path_naming(config: HarnessConfig, project_setup): store = project_setup p = store.attempt_path("p1", "mission-001", "2026-05-17T10-00-00Z", "w1")