From 4da272d2bfe1677c1bb10d42b7805bc74683ad3d Mon Sep 17 00:00:00 2001 From: Stephan Brezinsky Date: Sun, 19 Jul 2026 19:12:05 -0400 Subject: [PATCH 1/2] fix(acp): route codex -c overrides through CODEX_CONFIG for npm adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The npm @agentclientprotocol/codex-acp adapter (v1.1.0+, verified against 1.1.2) does not parse -c overrides from argv — it reads its startup configuration from the CODEX_CONFIG env var (JSON), parsed at src/CodexAcpClient.ts and spread unchanged into codex's thread/start config. The -c flags appended by _augment_acp_command (sandbox_mode, approval_policy, model_reasoning_effort) were silently discarded — sandbox, approval policy, and reasoning effort never reached the worker (issue #27). Build CODEX_CONFIG in _acp_subprocess_env in three layers, each overriding the previous: 1. User-supplied CODEX_CONFIG (ambient env). 2. -c overrides parsed from the augmented command string, via _parse_codex_c_overrides (handles both double- and single-quoted values). 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). _acp_subprocess_env now accepts reasoning_effort and acp_command; both call sites (run_node, run_terminal_review) thread role_config.worker_reasoning_effort and the augmented command through. Additionally, routing all -c overrides through CODEX_CONFIG lets a user-specified codex model reach the adapter — previously dropped for the same reason. A model supplied via ZENITH_*_ACP_COMMAND (-c model="...") or ambient CODEX_CONFIG is preserved (layers 1–2) while zenith's safety keys still override (layer 3). This model-override support was requested in the issue #27 comments; it is not part of the original issue but rides on the same mechanism. Verified end-to-end against the installed adapter: the adapter's startup log (app-server.log) shows codexConfig with model, sandbox_mode, approval_policy, and model_reasoning_effort all received. Tests: - _parse_codex_c_overrides extracts model + sandbox from double and single quoted -c flags; empty when no flags present. - CODEX_CONFIG JSON present with correct effort; defaults to xhigh. - User CODEX_CONFIG merged (preserves ambient model); command-string model wins over ambient model. - Custom model from augmented command preserved alongside zenith's safety keys. - Zenith's safety keys override user-supplied safety values; invalid user JSON replaced. Claude path has no CODEX_CONFIG. Fixes #27. --- zenith/src/zenith_harness/acp_runner.py | 78 ++++++++++++++-- zenith/tests/test_acp_runner.py | 116 ++++++++++++++++++++++++ 2 files changed, 188 insertions(+), 6 deletions(-) diff --git a/zenith/src/zenith_harness/acp_runner.py b/zenith/src/zenith_harness/acp_runner.py index a7a71a1..c77269b 100644 --- a/zenith/src/zenith_harness/acp_runner.py +++ b/zenith/src/zenith_harness/acp_runner.py @@ -5,6 +5,7 @@ import json import logging import os +import re import socket import subprocess import sys @@ -118,13 +119,46 @@ def _augment_acp_command( return command -def _acp_subprocess_env(provider) -> dict[str, str]: +_CODEX_C_OVERRIDE_RE = re.compile(r'-c\s+(\w+)=["\']([^"\']*)["\']') + + +def _parse_codex_c_overrides(command: str) -> dict[str, str]: + """Extract -c key="value" overrides from a codex-acp command string. + + Both _augment_acp_command and user-supplied ZENITH_*_ACP_COMMAND values + splice config via `-c key="value"`. The npm codex-acp adapter ignores + argv `-c` flags (issue #27), so we re-route them into CODEX_CONFIG. + """ + return dict(_CODEX_C_OVERRIDE_RE.findall(command)) + + +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 +168,30 @@ 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: + try: + parsed = json.loads(existing) + if isinstance(parsed, dict): + codex_config = parsed + except (json.JSONDecodeError, TypeError): + pass + # 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 + env["CODEX_CONFIG"] = json.dumps(codex_config) # hermes: no special env needed return env @@ -638,7 +696,11 @@ 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_subprocess_env( + role_config.worker_provider, + role_config.worker_reasoning_effort, + acp_command, + ), limit=SUBPROCESS_STREAM_LIMIT, ) progress_tracker = ACPProgressTracker(callback=progress_callback) @@ -796,7 +858,11 @@ 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_subprocess_env( + role_config.worker_provider, + role_config.worker_reasoning_effort, + acp_command, + ), 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..0e91dd5 100644 --- a/zenith/tests/test_acp_runner.py +++ b/zenith/tests/test_acp_runner.py @@ -20,6 +20,7 @@ ACPNodeRunner, _acp_subprocess_env, _augment_acp_command, + _parse_codex_c_overrides, ) from zenith_harness.providers import PROVIDERS from zenith_harness.assets import AssetLoader @@ -190,6 +191,121 @@ 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" + + +def test_codex_acp_env_invalid_user_codex_config_replaced( + monkeypatch: pytest.MonkeyPatch, +): + monkeypatch.setenv("CODEX_CONFIG", "not valid json") + env = _acp_subprocess_env(PROVIDERS["codex"], reasoning_effort="high") + config = json.loads(env["CODEX_CONFIG"]) + assert config["sandbox_mode"] == "danger-full-access" + assert config["approval_policy"] == "never" + assert config["model_reasoning_effort"] == "high" + + +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") From 21b65b2b6acb68b018063c8dde9093861a3a5c5e Mon Sep 17 00:00:00 2001 From: TueVNguyen Date: Sun, 6 Sep 2026 10:28:02 +0000 Subject: [PATCH 2/2] fix(acp): parse Codex overrides and reject invalid configuration --- zenith/src/zenith_harness/acp_runner.py | 77 ++++++++++++++++--------- zenith/tests/test_acp_runner.py | 77 ++++++++++++++++++++++--- 2 files changed, 120 insertions(+), 34 deletions(-) diff --git a/zenith/src/zenith_harness/acp_runner.py b/zenith/src/zenith_harness/acp_runner.py index c77269b..ec75364 100644 --- a/zenith/src/zenith_harness/acp_runner.py +++ b/zenith/src/zenith_harness/acp_runner.py @@ -5,10 +5,11 @@ import json import logging import os -import re +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 @@ -119,17 +120,35 @@ def _augment_acp_command( return command -_CODEX_C_OVERRIDE_RE = re.compile(r'-c\s+(\w+)=["\']([^"\']*)["\']') +def _parse_codex_c_overrides(command: str) -> dict[str, Any]: + """Read -c/--config assignments after shell quoting; the last value wins. - -def _parse_codex_c_overrides(command: str) -> dict[str, str]: - """Extract -c key="value" overrides from a codex-acp command string. - - Both _augment_acp_command and user-supplied ZENITH_*_ACP_COMMAND values - splice config via `-c key="value"`. The npm codex-acp adapter ignores - argv `-c` flags (issue #27), so we re-route them into CODEX_CONFIG. + 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. """ - return dict(_CODEX_C_OVERRIDE_RE.findall(command)) + 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( @@ -176,13 +195,14 @@ def _acp_subprocess_env( codex_config: dict[str, Any] = {} # Layer 1: user-supplied CODEX_CONFIG. existing = env.get("CODEX_CONFIG") - if existing: + if existing is not None: try: parsed = json.loads(existing) - if isinstance(parsed, dict): - codex_config = parsed - except (json.JSONDecodeError, TypeError): - pass + 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: @@ -191,7 +211,10 @@ def _acp_subprocess_env( codex_config["sandbox_mode"] = "danger-full-access" codex_config["approval_policy"] = "never" codex_config["model_reasoning_effort"] = effort - env["CODEX_CONFIG"] = json.dumps(codex_config) + 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 @@ -638,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)) @@ -696,11 +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, - role_config.worker_reasoning_effort, - acp_command, - ), + env=acp_env, limit=SUBPROCESS_STREAM_LIMIT, ) progress_tracker = ACPProgressTracker(callback=progress_callback) @@ -812,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)) @@ -858,11 +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, - role_config.worker_reasoning_effort, - acp_command, - ), + 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 0e91dd5..5d64aae 100644 --- a/zenith/tests/test_acp_runner.py +++ b/zenith/tests/test_acp_runner.py @@ -13,11 +13,14 @@ 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, @@ -230,17 +233,75 @@ def test_codex_acp_env_overrides_user_safety_values( assert config["approval_policy"] == "never" -def test_codex_acp_env_invalid_user_codex_config_replaced( - monkeypatch: pytest.MonkeyPatch, -): - monkeypatch.setenv("CODEX_CONFIG", "not valid json") - env = _acp_subprocess_env(PROVIDERS["codex"], reasoning_effort="high") - 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, ):