diff --git a/zenith/src/zenith_harness/acp_runner.py b/zenith/src/zenith_harness/acp_runner.py index a7a71a1..a90496b 100644 --- a/zenith/src/zenith_harness/acp_runner.py +++ b/zenith/src/zenith_harness/acp_runner.py @@ -90,7 +90,10 @@ class ACPError(Exception): def _augment_acp_command( - command: str, provider, reasoning_effort: str | None = None + command: str, + provider, + reasoning_effort: str | None = None, + model: str | None = None, ) -> str: """Append provider-specific config flags to the ACP launch command. @@ -103,22 +106,30 @@ def _augment_acp_command( `config.VALID_REASONING_EFFORTS` at discovery); None keeps the historical "xhigh" default. + `model` is the per-role pin from ZENITH__MODEL (validated against + `config.MODEL_ID_PATTERN` at discovery); None leaves codex on the model in + its own config. Claude takes its pin through ANTHROPIC_MODEL in + `_acp_subprocess_env` instead — claude-agent-acp has no model flag. + For hermes the command is passed through unchanged. """ name = getattr(provider, "name", None) if name == "codex": effort = reasoning_effort or "xhigh" - return ( + augmented = ( command + ' -c sandbox_mode="danger-full-access"' + ' -c approval_policy="never"' + f' -c model_reasoning_effort="{effort}"' ) + if model: + augmented += f' -c model="{model}"' + return augmented # hermes: no-op return command -def _acp_subprocess_env(provider) -> dict[str, str]: +def _acp_subprocess_env(provider, model: 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 @@ -126,6 +137,12 @@ def _acp_subprocess_env(provider) -> dict[str, str]: command line also receives `sandbox_mode="danger-full-access"` in `_augment_acp_command`. + For claude, a per-role `model` pin travels as ANTHROPIC_MODEL — the highest + priority input claude-agent-acp reads when picking a model, above + settings.json. An unset pin leaves any inherited value alone, which means + an unpinned lane runs on an ambient ANTHROPIC_MODEL if one is set rather + than on claude-agent-acp's own default. + For hermes the env is passed through unchanged. """ env = os.environ.copy() @@ -134,6 +151,8 @@ 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" + elif name == "claude" and model: + env["ANTHROPIC_MODEL"] = model # hermes: no special env needed return env @@ -579,6 +598,7 @@ async def run_node( acp_command, role_config.worker_provider, role_config.worker_reasoning_effort, + role_config.worker_model, ) workspace_dir = str(Path(cwd).expanduser().resolve() if cwd else store.workspace_dir(project_id)) @@ -638,7 +658,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_subprocess_env(role_config.worker_provider, role_config.worker_model), limit=SUBPROCESS_STREAM_LIMIT, ) progress_tracker = ACPProgressTracker(callback=progress_callback) @@ -749,6 +769,7 @@ async def run_terminal_review( acp_command, role_config.worker_provider, role_config.worker_reasoning_effort, + role_config.worker_model, ) workspace_dir = str(store.workspace_dir(project_id)) @@ -796,7 +817,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_subprocess_env(role_config.worker_provider, role_config.worker_model), limit=SUBPROCESS_STREAM_LIMIT, ) tracker = ACPProgressTracker(callback=progress_callback) diff --git a/zenith/src/zenith_harness/cli.py b/zenith/src/zenith_harness/cli.py index 22a8ce7..d315a38 100644 --- a/zenith/src/zenith_harness/cli.py +++ b/zenith/src/zenith_harness/cli.py @@ -8,7 +8,7 @@ import click from .assets import AssetLoader, iter_skill_directories -from .config import VALID_REASONING_EFFORTS, HarnessConfig +from .config import VALID_REASONING_EFFORTS, HarnessConfig, validate_model_id from .envelope import render_task_list from .providers import ( ProviderDefinition, @@ -37,11 +37,40 @@ "ZENITH_WORKER_REASONING_EFFORT", "ZENITH_VALIDATOR_REASONING_EFFORT", "ZENITH_TERMINAL_REVIEWER_REASONING_EFFORT", + # Deliberately absent: ZENITH_{WORKER,VALIDATOR,TERMINAL_REVIEWER}_MODEL. + # See the model_env comment in `init` — an ambient model pin carries no + # record of the provider it was chosen for, so forwarding it into a + # workspace config lands it on whatever provider that workspace uses. "ZAI_API_KEY", "ZAI_BASE_URL", ) +def _validate_model_flag(ctx, param, value): + """Click callback for the --*-model flags. + + The env vars are validated at `discover()`; flags need the same check but + reported as a usage error against the option the user actually typed. An + explicit empty string is rejected rather than dropped, because it reads as + "run this lane unpinned" and does not do that: a same-provider lane still + inherits the pin above it, and there is no flag that expresses "unpinned". + Refusing is honest; silently accepting a no-op would not be. + """ + if value is None: + return None + if not value: + raise click.BadParameter("model pin cannot be empty", ctx=ctx, param=param) + try: + return validate_model_id(value, env_var=param.name) + except ValueError: + raise click.BadParameter( + f"{value!r} is not a valid model identifier; " + "allowed characters are letters, digits, and ._:/@[]-", + ctx=ctx, + param=param, + ) from None + + @click.group() def cli() -> None: """Zenith CLI — set up + inspect long-running coding projects.""" @@ -77,6 +106,12 @@ def cli() -> None: @click.option("--worker-reasoning-effort", type=click.Choice(VALID_REASONING_EFFORTS), default=None) @click.option("--validator-reasoning-effort", type=click.Choice(VALID_REASONING_EFFORTS), default=None) @click.option("--terminal-reviewer-reasoning-effort", type=click.Choice(VALID_REASONING_EFFORTS), default=None) +@click.option("--worker-model", default=None, callback=_validate_model_flag, + help="Model pin for worker lanes (e.g. opus, gpt-5.5).") +@click.option("--validator-model", default=None, callback=_validate_model_flag, + help="Model pin for the validation gate; falls back to the worker pin.") +@click.option("--terminal-reviewer-model", default=None, callback=_validate_model_flag, + help="Model pin for terminal review; falls back to the validator pin.") @click.option("--zenith-home", type=click.Path(), default=None) @click.option("--workspace-dir", "workspace_dir", type=click.Path(exists=True), default=".") def init( @@ -91,6 +126,9 @@ def init( worker_reasoning_effort: str | None, validator_reasoning_effort: str | None, terminal_reviewer_reasoning_effort: str | None, + worker_model: str | None, + validator_model: str | None, + terminal_reviewer_model: str | None, zenith_home: str | None, workspace_dir: str, ) -> None: @@ -103,7 +141,33 @@ def init( workspace stays clean of `.zenith/` until `start_project` runs. """ workspace = Path(workspace_dir).resolve() - config = HarnessConfig.discover() + # discover() validates the ambient ZENITH_* settings. Its ValueError is a + # user-facing complaint about the caller's environment, not a harness + # fault, so it gets the same treatment as a bad flag rather than a + # traceback. + # + # A model pin that a flag replaces is exempt. Ambient pins are never + # written (see model_env below), so a broken one still deserves to stop + # init — it would reach a server launched from this same shell. But once a + # flag supplies that role's pin, the written config overrides the ambient + # value for every server this workspace starts, and failing on it would + # block an init that already ignores it. Restored immediately: the + # environment belongs to the caller. + shadowed = { + var: os.environ.pop(var) + for var, flag_value in ( + ("ZENITH_WORKER_MODEL", worker_model), + ("ZENITH_VALIDATOR_MODEL", validator_model), + ("ZENITH_TERMINAL_REVIEWER_MODEL", terminal_reviewer_model), + ) + if flag_value and var in os.environ + } + try: + config = HarnessConfig.discover() + except ValueError as exc: + raise click.UsageError(str(exc)) from None + finally: + os.environ.update(shadowed) loader = AssetLoader(config) selection = _resolve_selection( agent=agent, @@ -132,17 +196,228 @@ def init( ) if value } - _write_bootstrap_config(workspace, selection, storage_env, effort_env) + # Model pins take the flags but NOT the ambient env, which is where they + # part company with the reasoning efforts above. An effort is + # provider-neutral vocabulary, so forwarding an inherited one into the + # workspace is safe. A model id is not: an ambient ZENITH_WORKER_MODEL + # arrives with no record of which provider it was chosen for, so baking it + # into this workspace lands a leftover `gpt-5.5-codex` on a claude lane as + # ANTHROPIC_MODEL — the exact cross-provider landing `_inherited_model` + # refuses to make inside a single config. Pins therefore enter a workspace + # only through the flags, checked below against the provider resolved for + # that lane. An ambient ZENITH_*_MODEL still reaches a server the user + # launches from that same shell; init just does not make it durable. + # + # ANTHROPIC_MODEL is the exception, and it is deliberate: it is a + # provider-scoped variable rather than a lane-scoped one, it is the + # documented way to pin claude globally, and it stays in the forward + # allowlist above. So a workspace inited from a shell that exported it does + # carry it, and an "unpinned" claude lane runs on it — see the note on + # HarnessConfig.worker_model. + # + # Click has no Choice to validate an open-ended model id against, so the + # flags carry _validate_model_flag, which applies the same check discover() + # applies to the env vars. + model_env = { + var: value + for var, value in ( + ("ZENITH_WORKER_MODEL", worker_model), + ("ZENITH_VALIDATOR_MODEL", validator_model), + ("ZENITH_TERMINAL_REVIEWER_MODEL", terminal_reviewer_model), + ) + if value + } + # Resolve every role the way the server will, then stage what was + # resolved. These vars are not in RUNTIME_ENV_FORWARD_ALLOWLIST and + # ProviderSelection.env() emits a role provider only when it differs from + # the worker, so an ambient ZENITH_VALIDATOR_PROVIDER used to inform init's + # diagnostics while never reaching the config init wrote. The two answers + # could then disagree in both directions: a warning about a hermes + # validator whose written config said claude, and silence in the mirror + # case. Writing the resolved value closes that gap — the host agent is + # usually launched later, from a different shell, and reads only what is + # written here. + # + # Precedence per role: explicit flag, then the ambient var, then inherit + # down the chain (worker -> validator -> terminal reviewer). The flag wins + # so an explicit `--validator-provider claude` is never overruled by a + # stale export. + def _ambient(var: str) -> str | None: + return os.environ.get(var) or None + + def _pick(*candidates: tuple[str | None, str]) -> tuple[str, str]: + """First candidate with a value, paired with where it came from.""" + for value, source in candidates: + if value: + return value, source + raise AssertionError("the final candidate must always carry a value") + + # The worker is the exception: it is the only role with a default of its + # own (`--agent`, then default_worker_provider_name), and ProviderSelection + # has always written ZENITH_WORKER_PROVIDER unconditionally. Init therefore + # sets the worker rather than inheriting it, and an ambient + # ZENITH_WORKER_PROVIDER does not survive `zenith init`. The other two + # roles have no default but the lane above them, which is why they consult + # the environment. + worker_provider_name, worker_source = selection.worker.name, "--worker-provider" + validator_provider_name, validator_source = _pick( + (validator_provider, "--validator-provider"), + (_ambient("ZENITH_VALIDATOR_PROVIDER"), "ZENITH_VALIDATOR_PROVIDER"), + (worker_provider_name, worker_source), + ) + terminal_reviewer_provider_name, terminal_source = _pick( + (terminal_reviewer_provider, "--terminal-reviewer-provider"), + (_ambient("ZENITH_TERMINAL_REVIEWER_PROVIDER"), "ZENITH_TERMINAL_REVIEWER_PROVIDER"), + (validator_provider_name, validator_source), + ) + + # An ACP command names a binary, so it is the most provider-specific value + # in the config — more so than a model id. It is therefore taken from the + # environment only for a lane whose *provider* also came from the + # environment, so the pair stays together. Take one without the other and + # init manufactures the mismatch it warns about below: with + # `ZENITH_WORKER_PROVIDER=codex ZENITH_WORKER_ACP_COMMAND=codex-acp` in the + # shell, `zenith init --agent claude` sets the worker provider itself (see + # above) and would otherwise pair claude with codex-acp — permanently, and + # in the config it just wrote. A lane whose provider init chose gets its + # command from the flag or from that provider's default, never from a + # shell that was talking about some other provider. + # + # Honoring the paired case still fixes the original defect: an exported + # ZENITH_VALIDATOR_ACP_COMMAND used to inform nothing and never be written, + # so the lane silently fell back to the worker's command at runtime. + def _role_command( + flag_value: str | None, var: str, provider_source: str + ) -> str | None: + if flag_value: + return flag_value + return _ambient(var) if provider_source == var.replace("_ACP_COMMAND", "_PROVIDER") else None + + role_command = { + var: value + for var, value in ( + ( + "ZENITH_WORKER_ACP_COMMAND", + _role_command(worker_acp_command, "ZENITH_WORKER_ACP_COMMAND", worker_source), + ), + ( + "ZENITH_VALIDATOR_ACP_COMMAND", + _role_command( + validator_acp_command, "ZENITH_VALIDATOR_ACP_COMMAND", validator_source + ), + ), + ( + "ZENITH_TERMINAL_REVIEWER_ACP_COMMAND", + _role_command( + terminal_reviewer_acp_command, + "ZENITH_TERMINAL_REVIEWER_ACP_COMMAND", + terminal_source, + ), + ), + ) + if value + } + role_env = { + "ZENITH_VALIDATOR_PROVIDER": validator_provider_name, + "ZENITH_TERMINAL_REVIEWER_PROVIDER": terminal_reviewer_provider_name, + **role_command, + } + cli_env = {**effort_env, **model_env, **role_env} + + # Resolved before anything is written: the flags carry a click.Choice, but + # an ambient ZENITH_*_PROVIDER does not, and these names are dereferenced + # late — the terminal one at asset install, the validator one not until the + # first validate dispatch mid-mission. A typo should stop init, blamed on + # whatever actually supplied it. A flag that beat the ambient var means the + # ambient typo never reaches this check, which is the point. + def _resolve_provider(name: str, source: str): + try: + return get_provider(name) + except ValueError as exc: + raise click.UsageError(f"{exc} (from {source})") from None + + validator_provider_def = _resolve_provider(validator_provider_name, validator_source) + terminal_reviewer_provider_def = _resolve_provider( + terminal_reviewer_provider_name, terminal_source + ) - # 2) Per-provider agents + orchestrator prompt - for provider in selection.providers(): + def _runs_provider_binary(command: str, provider_def: ProviderDefinition) -> bool: + """Whether `command` looks like it launches `provider_def`'s own agent. + + Compares the executable — first token, basename only — so an absolute + path or added arguments (`/usr/local/bin/codex-acp`, `codex-acp + --verbose`) still reads as codex. Wrong only when a command genuinely + runs a different binary, which is the case worth a warning. + """ + default = provider_def.default_worker_acp_command + if not default: + return False + return Path(command.split()[0]).name == Path(default.split()[0]).name + + # Two independent hazards, reported independently: a hermes lane cannot act + # on a pin at all, and a lane whose command runs somebody else's binary + # will not get the provider-specific treatment the harness applies for the + # provider it thinks it has. The command hazard is NOT conditioned on a + # pin — dispatch keys on provider.name for sandbox flags, the codex config + # flags and the ACP session mode too, so it is a hazard on its own. + for role, flag, pin, provider_def, command_var in ( + ("worker", "--worker-model", worker_model, selection.worker, "ZENITH_WORKER_ACP_COMMAND"), + ( + "validator", + "--validator-model", + validator_model, + validator_provider_def, + "ZENITH_VALIDATOR_ACP_COMMAND", + ), + ( + "terminal reviewer", + "--terminal-reviewer-model", + terminal_reviewer_model, + terminal_reviewer_provider_def, + "ZENITH_TERMINAL_REVIEWER_ACP_COMMAND", + ), + ): + # The pins come from the flags only (see model_env), so the flag name + # is always the right thing to name here. + if pin and provider_def.name == "hermes": + click.echo( + f"Warning: {flag} is ignored for provider {provider_def.name} — " + "it exposes no model selection." + ) + command = role_command.get(command_var) + if command and not _runs_provider_binary(command, provider_def): + click.echo( + f"Warning: the {role} lane dispatches as provider " + f"{provider_def.name} but launches {command} — sandbox flags, " + "model pins and the ACP session mode are all applied the way " + f"{provider_def.name} expects, and will be wrong if that " + "command runs a different agent." + ) + + _write_bootstrap_config(workspace, selection, storage_env, cli_env) + + # 2) Per-provider agents + orchestrator prompt. ProviderSelection knows + # only the flags, so both roles that can be resolved from the + # environment are added here. Skipping either installs a workspace whose + # config names a provider that has no agents or skills on disk — a + # validator resolved from an ambient ZENITH_VALIDATOR_PROVIDER used to + # get assets only by accident, when the reviewer happened to inherit it. + asset_providers = list(selection.providers()) + for provider_def in (validator_provider_def, terminal_reviewer_provider_def): + if provider_def.name not in {p.name for p in asset_providers}: + asset_providers.append(provider_def) + for provider in asset_providers: _setup_provider_assets(workspace, loader, provider) click.echo( f"\nInitialized v5 project workspace at {workspace}: " f"orchestrator={selection.orchestrator.name}, " - f"worker={selection.worker.name}, " - f"validator={selection.resolved_validation_worker.name}." + f"worker={worker_provider_name}, " + # The resolved names, not selection's flag-only view — otherwise the + # summary contradicts the config written one line earlier whenever a + # role came from the environment. + f"validator={validator_provider_name}, " + f"terminal-reviewer={terminal_reviewer_provider_name}." ) click.echo( "Bucket lives at $ZENITH_HOME/projects// — created on the first " diff --git a/zenith/src/zenith_harness/config.py b/zenith/src/zenith_harness/config.py index 22adcc0..60ea555 100644 --- a/zenith/src/zenith_harness/config.py +++ b/zenith/src/zenith_harness/config.py @@ -2,6 +2,7 @@ from __future__ import annotations import os +import re from dataclasses import dataclass, replace from pathlib import Path from typing import Literal @@ -22,6 +23,15 @@ # that already orchestrates and validates per-lane work. VALID_REASONING_EFFORTS = ("minimal", "low", "medium", "high", "xhigh", "max") +# Model identifiers are open-ended (provider aliases like "opus", pinned ids +# like "claude-opus-5[1m]", Bedrock/Vertex ARNs), so they cannot be checked +# against an allowlist the way reasoning efforts are. They still reach a shell +# command line for codex (`-c model="..."`), so the character set is restricted +# to what real model identifiers use — no quotes, spaces, or shell operators. +# Matched with fullmatch: `$` would admit a trailing newline, which survives +# into .codex/config.toml as an unescaped newline inside a basic string. +MODEL_ID_PATTERN = re.compile(r"[A-Za-z0-9._:/@\[\]-]+") + def _bundled_dir() -> Path: return (Path(__file__).resolve().parent / "bundled").resolve() @@ -57,6 +67,20 @@ def _resolve_reasoning_effort(value: str | None, *, env_var: str) -> str | None: return value +def validate_model_id(value: str | None, *, env_var: str) -> str | None: + """None passes through (provider default); anything else must look like a + model identifier. The value is spliced into a shell command line for codex, + so a rejected string is a refusal to execute, not a cosmetic complaint.""" + if not value: + return None + if not MODEL_ID_PATTERN.fullmatch(value): + raise ValueError( + f"{env_var}={value!r} is not a valid model identifier; " + "allowed characters are letters, digits, and ._:/@[]-" + ) + return value + + @dataclass(frozen=True) class HarnessConfig: """Static configuration loaded from env. Per-call overrides allowed via `with_*`.""" @@ -77,6 +101,16 @@ class HarnessConfig: worker_reasoning_effort: str | None = None validator_reasoning_effort: str | None = None terminal_reviewer_reasoning_effort: str | None = None + # Per-role model pin. None means whatever the lane would run without one: + # codex uses the model in its own config, and claude-agent-acp reads an + # inherited ANTHROPIC_MODEL if the environment carries one (that var is in + # the CLI's runtime forward allowlist, so a workspace can carry it) before + # falling back to its first model. So an unpinned claude lane is not + # guaranteed to be on a provider default — including when `_inherited_model` + # declines to inherit a foreign-provider pin. + worker_model: str | None = None + validator_model: str | None = None + terminal_reviewer_model: str | None = None @classmethod def discover(cls) -> HarnessConfig: @@ -135,6 +169,18 @@ def discover(cls) -> HarnessConfig: os.environ.get("ZENITH_TERMINAL_REVIEWER_REASONING_EFFORT"), env_var="ZENITH_TERMINAL_REVIEWER_REASONING_EFFORT", ), + worker_model=validate_model_id( + os.environ.get("ZENITH_WORKER_MODEL"), + env_var="ZENITH_WORKER_MODEL", + ), + validator_model=validate_model_id( + os.environ.get("ZENITH_VALIDATOR_MODEL"), + env_var="ZENITH_VALIDATOR_MODEL", + ), + terminal_reviewer_model=validate_model_id( + os.environ.get("ZENITH_TERMINAL_REVIEWER_MODEL"), + env_var="ZENITH_TERMINAL_REVIEWER_MODEL", + ), ) # ------------------------------------------------------------------ @@ -224,35 +270,71 @@ def skill_dirs(self, project_id: str | None = None) -> list[Path]: # Role-specialized variants # ------------------------------------------------------------------ + def _inherited_model( + self, provider_name: str, chain: tuple[tuple[str | None, str], ...] + ) -> str | None: + """Walk a role's fallback chain, skipping links from other providers. + + Reasoning efforts are provider-neutral vocabulary, so they inherit + freely. Model ids are not — a codex worker's "gpt-5.5" handed to a + claude validator becomes ANTHROPIC_MODEL="gpt-5.5" and breaks every + session on that lane. So a pin only carries to a role running the same + provider; otherwise the role falls back to its provider's own default. + + `chain` is ordered nearest-first: (pin, provider that pin was set for). + """ + for pin, pin_provider_name in chain: + if pin and pin_provider_name == provider_name: + return pin + return None + def for_role( self, role: Literal["worker", "validator", "terminal_reviewer"] ) -> HarnessConfig: if role == "worker": return self if role == "validator": + provider_name = self.validator_provider_name or self.worker_provider_name return replace( self, - worker_provider_name=( - self.validator_provider_name or self.worker_provider_name - ), + worker_provider_name=provider_name, worker_acp_command=self.resolved_validator_acp_command, worker_reasoning_effort=( self.validator_reasoning_effort or self.worker_reasoning_effort ), + worker_model=self._inherited_model( + provider_name, + ( + (self.validator_model, provider_name), + (self.worker_model, self.worker_provider_name), + ), + ), ) if role == "terminal_reviewer": + validator_provider_name = ( + self.validator_provider_name or self.worker_provider_name + ) + provider_name = ( + self.terminal_reviewer_provider_name + or self.validator_provider_name + or self.worker_provider_name + ) return replace( self, - worker_provider_name=( - self.terminal_reviewer_provider_name - or self.validator_provider_name - or self.worker_provider_name - ), + worker_provider_name=provider_name, worker_acp_command=self.resolved_terminal_reviewer_acp_command, worker_reasoning_effort=( self.terminal_reviewer_reasoning_effort or self.validator_reasoning_effort or self.worker_reasoning_effort ), + worker_model=self._inherited_model( + provider_name, + ( + (self.terminal_reviewer_model, provider_name), + (self.validator_model, validator_provider_name), + (self.worker_model, self.worker_provider_name), + ), + ), ) raise ValueError(f"unknown role: {role}") diff --git a/zenith/tests/test_acp_runner.py b/zenith/tests/test_acp_runner.py index 412ea69..ca2d799 100644 --- a/zenith/tests/test_acp_runner.py +++ b/zenith/tests/test_acp_runner.py @@ -12,6 +12,7 @@ import os import shutil import sys +from dataclasses import replace from pathlib import Path import pytest @@ -124,6 +125,176 @@ async def _ready_immediately(*args, **kwargs): assert data["node_id"] == "w1" +class _SpawnCaptured(Exception): + """Raised by the spy to stop the runner once the launch args are known.""" + + def __init__(self, command: str, env: dict[str, str]): + super().__init__("captured") + self.command = command + self.env = env + + +def _capture_acp_spawn(monkeypatch: pytest.MonkeyPatch) -> None: + """Intercept the ACP agent launch and surface its command line + env. + + The helper tests below call `_augment_acp_command`/`_acp_subprocess_env` + directly, which cannot catch an un-threaded call site — drop + `role_config.worker_model` from run_node and they all still pass. This + spies on the real `create_subprocess_shell` the runner uses, so the + assertions fail if the pin never reaches the launch. + """ + + async def _spy(command, *args, **kwargs): + raise _SpawnCaptured(command, kwargs.get("env") or {}) + + monkeypatch.setattr(asyncio, "create_subprocess_shell", _spy) + + +def _run_node_capturing_spawn( + config: HarnessConfig, store, task: Task +) -> _SpawnCaptured: + runner = ACPNodeRunner(config=config, loader=AssetLoader(config)) + + async def _no_op_server(*args, **kwargs): + return None + + async def _ready_immediately(*args, **kwargs): + return None + + runner._start_worker_mcp_server = _no_op_server # type: ignore[method-assign] + runner._wait_for_server_ready = _ready_immediately # type: ignore[method-assign] + + with pytest.raises(_SpawnCaptured) as excinfo: + asyncio.run( + runner.run_node( + project_id="p1", + mission_id="mission-001", + task=task, + spawn_ts="2026-05-17T00-00-00Z", + store=store, + ) + ) + return excinfo.value + + +def test_run_node_passes_worker_model_to_claude_launch_env( + config: HarnessConfig, project_setup, monkeypatch: pytest.MonkeyPatch +): + monkeypatch.delenv("ANTHROPIC_MODEL", raising=False) + _capture_acp_spawn(monkeypatch) + pinned = replace(config, worker_provider_name="claude", worker_model="opus") + task = Task(id="w1", type="work", body="do it", targets=["VAL-001"], skill="s") + + captured = _run_node_capturing_spawn(pinned, project_setup, task) + + assert captured.env["ANTHROPIC_MODEL"] == "opus" + + +def test_run_node_passes_validator_model_to_validate_task( + config: HarnessConfig, project_setup, monkeypatch: pytest.MonkeyPatch +): + monkeypatch.delenv("ANTHROPIC_MODEL", raising=False) + _capture_acp_spawn(monkeypatch) + pinned = replace( + config, + worker_provider_name="claude", + worker_model="sonnet", + validator_model="opus", + ) + # A validate task must resolve the validator's pin, not the worker's. + task = Task(id="v1", type="validate", body="check it", targets=["VAL-001"], skill="s") + + captured = _run_node_capturing_spawn(pinned, project_setup, task) + + assert captured.env["ANTHROPIC_MODEL"] == "opus" + + +def test_run_node_passes_worker_model_to_codex_launch_command( + config: HarnessConfig, project_setup, monkeypatch: pytest.MonkeyPatch +): + _capture_acp_spawn(monkeypatch) + pinned = replace(config, worker_provider_name="codex", worker_model="gpt-5.5") + task = Task(id="w1", type="work", body="do it", targets=["VAL-001"], skill="s") + + captured = _run_node_capturing_spawn(pinned, project_setup, task) + + # Codex takes the pin on its command line, not through the env. + assert 'model="gpt-5.5"' in captured.command + + +def _run_terminal_review_capturing_spawn( + config: HarnessConfig, store +) -> _SpawnCaptured: + runner = ACPNodeRunner(config=config, loader=AssetLoader(config)) + + async def _no_op_server(*args, **kwargs): + return None + + async def _ready_immediately(*args, **kwargs): + return None + + runner._start_worker_mcp_server = _no_op_server # type: ignore[method-assign] + runner._wait_for_server_ready = _ready_immediately # type: ignore[method-assign] + + with pytest.raises(_SpawnCaptured) as excinfo: + asyncio.run( + runner.run_terminal_review( + project_id="p1", + mission_id="mission-001", + spawn_ts="2026-05-17T00-00-00Z", + store=store, + ) + ) + return excinfo.value + + +def test_run_terminal_review_passes_model_to_claude_launch_env( + config: HarnessConfig, project_setup, monkeypatch: pytest.MonkeyPatch +): + monkeypatch.delenv("ANTHROPIC_MODEL", raising=False) + _capture_acp_spawn(monkeypatch) + pinned = replace( + config, + worker_provider_name="claude", + worker_model="sonnet", + terminal_reviewer_model="opus", + ) + + captured = _run_terminal_review_capturing_spawn(pinned, project_setup) + + # run_terminal_review is a second, independent spawn path — it must resolve + # the terminal reviewer's own pin, not the worker's. + assert captured.env["ANTHROPIC_MODEL"] == "opus" + + +def test_run_terminal_review_passes_model_to_codex_launch_command( + config: HarnessConfig, project_setup, monkeypatch: pytest.MonkeyPatch +): + _capture_acp_spawn(monkeypatch) + pinned = replace( + config, + worker_provider_name="codex", + terminal_reviewer_model="gpt-5.5", + ) + + captured = _run_terminal_review_capturing_spawn(pinned, project_setup) + + assert 'model="gpt-5.5"' in captured.command + + +def test_run_node_without_model_pin_does_not_set_anthropic_model( + config: HarnessConfig, project_setup, monkeypatch: pytest.MonkeyPatch +): + monkeypatch.delenv("ANTHROPIC_MODEL", raising=False) + _capture_acp_spawn(monkeypatch) + unpinned = replace(config, worker_provider_name="claude", worker_model=None) + task = Task(id="w1", type="work", body="do it", targets=["VAL-001"], skill="s") + + captured = _run_node_capturing_spawn(unpinned, project_setup, task) + + assert "ANTHROPIC_MODEL" not in captured.env + + def test_synthesize_missing_handoff_records_failure( config: HarnessConfig, project_setup, workspace: Path ): @@ -172,6 +343,56 @@ def test_augment_acp_command_claude_untouched(): ) +def test_augment_acp_command_codex_model_override(): + out = _augment_acp_command("codex-acp", PROVIDERS["codex"], model="gpt-5.5") + assert 'model="gpt-5.5"' in out + # The bypass flags and effort are model-independent. + assert 'sandbox_mode="danger-full-access"' in out + assert 'model_reasoning_effort="xhigh"' in out + + +def test_augment_acp_command_codex_without_model_pins_nothing(): + out = _augment_acp_command("codex-acp", PROVIDERS["codex"]) + # Only the reasoning-effort key, never a bare `model=` — an unset pin must + # leave codex on whatever its own config selects. + assert " -c model=" not in out + + +def test_augment_acp_command_claude_untouched_by_model(): + # claude-agent-acp takes no model flag; the pin travels via ANTHROPIC_MODEL. + assert ( + _augment_acp_command("claude-agent-acp", PROVIDERS["claude"], model="opus") + == "claude-agent-acp" + ) + + +def test_claude_acp_env_pins_anthropic_model(): + env = _acp_subprocess_env(PROVIDERS["claude"], model="opus") + assert env["ANTHROPIC_MODEL"] == "opus" + + +def test_claude_acp_env_without_model_leaves_inherited_value( + monkeypatch: pytest.MonkeyPatch, +): + monkeypatch.setenv("ANTHROPIC_MODEL", "inherited-from-shell") + + env = _acp_subprocess_env(PROVIDERS["claude"]) + + # No pin means no opinion: the ambient value survives untouched. + assert env["ANTHROPIC_MODEL"] == "inherited-from-shell" + + +def test_codex_acp_env_does_not_set_anthropic_model( + monkeypatch: pytest.MonkeyPatch, +): + monkeypatch.delenv("ANTHROPIC_MODEL", raising=False) + + env = _acp_subprocess_env(PROVIDERS["codex"], model="gpt-5.5") + + # Codex takes its model on the command line, not through an Anthropic env var. + assert "ANTHROPIC_MODEL" not in env + + def test_codex_acp_env_preserves_node_path_when_bwrap_is_present( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ): diff --git a/zenith/tests/test_cli.py b/zenith/tests/test_cli.py index d5da40a..728ed24 100644 --- a/zenith/tests/test_cli.py +++ b/zenith/tests/test_cli.py @@ -2,6 +2,7 @@ from __future__ import annotations import json +import os import tomllib from pathlib import Path @@ -9,6 +10,7 @@ from click.testing import CliRunner from zenith_harness.cli import cli +from zenith_harness.config import HarnessConfig @pytest.fixture @@ -16,6 +18,18 @@ def runner() -> CliRunner: return CliRunner() +@pytest.fixture(autouse=True) +def _scrub_ambient_role_env(monkeypatch) -> None: + """Init reads ambient ZENITH_* role settings when resolving a lane, so a + developer's exported pin, provider or command would otherwise leak into + these + assertions. Tests that want one set it themselves. + """ + for role in ("WORKER", "VALIDATOR", "TERMINAL_REVIEWER"): + for suffix in ("MODEL", "PROVIDER", "ACP_COMMAND", "REASONING_EFFORT"): + monkeypatch.delenv(f"ZENITH_{role}_{suffix}", raising=False) + + @pytest.fixture def env(harness_home: Path, workspace: Path, monkeypatch) -> dict[str, str]: monkeypatch.setenv("ZENITH_HOME", str(harness_home)) @@ -236,9 +250,876 @@ def test_init_invalid_inherited_effort_env_fails_despite_flag( "max", ], ) - assert r.exit_code != 0 - assert isinstance(r.exception, ValueError) - assert "ZENITH_WORKER_REASONING_EFFORT" in str(r.exception) + # A complaint about the caller's environment, reported the way a bad + # flag is rather than as a traceback. + assert r.exit_code == 2, r.output + assert "ZENITH_WORKER_REASONING_EFFORT" in r.output + + def test_claude_init_writes_model_flags( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--worker-model", + "sonnet", + "--validator-model", + "opus", + "--terminal-reviewer-model", + "claude-opus-5[1m]", + ], + ) + assert r.exit_code == 0, r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + assert server_env["ZENITH_WORKER_MODEL"] == "sonnet" + assert server_env["ZENITH_VALIDATOR_MODEL"] == "opus" + assert server_env["ZENITH_TERMINAL_REVIEWER_MODEL"] == "claude-opus-5[1m]" + + def test_init_does_not_bake_ambient_model_pins_into_the_workspace( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # The pin a shell happens to carry says nothing about which provider it + # was chosen for. Forwarding it would make a leftover codex pin durable + # in a claude workspace and hand it to claude-agent-acp as + # ANTHROPIC_MODEL — the cross-provider landing _inherited_model exists + # to prevent. Ambient pins are honored only by a server the user + # launches from that same shell; init never writes them down. + monkeypatch.setenv("ZENITH_WORKER_MODEL", "gpt-5.5-codex") + monkeypatch.setenv("ZENITH_VALIDATOR_MODEL", "gpt-5.5-codex") + monkeypatch.setenv("ZENITH_TERMINAL_REVIEWER_MODEL", "gpt-5.5-codex") + + r = runner.invoke(cli, ["init", "--workspace-dir", str(workspace), "--agent", "claude"]) + assert r.exit_code == 0, r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + assert "ZENITH_WORKER_MODEL" not in server_env + assert "ZENITH_VALIDATOR_MODEL" not in server_env + assert "ZENITH_TERMINAL_REVIEWER_MODEL" not in server_env + + def test_init_model_flags_override_env( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # Scrubbed so an exported pin in the developer's own shell cannot leak + # in through _forwarded_runtime_env() and break the "not in" assert. + for var in ( + "ZENITH_VALIDATOR_MODEL", + "ZENITH_TERMINAL_REVIEWER_MODEL", + ): + monkeypatch.delenv(var, raising=False) + monkeypatch.setenv("ZENITH_WORKER_MODEL", "sonnet") + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--worker-model", + "opus", + "--validator-model", + "claude-opus-5[1m]", + ], + ) + assert r.exit_code == 0, r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + # Flag beats the inherited shell env. + assert server_env["ZENITH_WORKER_MODEL"] == "opus" + assert server_env["ZENITH_VALIDATOR_MODEL"] == "claude-opus-5[1m]" + assert "ZENITH_TERMINAL_REVIEWER_MODEL" not in server_env + + def test_init_invalid_inherited_model_env_fails_when_no_flag_replaces_it( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # Unreplaced, the broken pin still reaches a server launched from this + # shell, so init refuses rather than deferring the failure. + monkeypatch.setenv("ZENITH_WORKER_MODEL", "opus; touch /tmp/pwned") + + r = runner.invoke( + cli, ["init", "--workspace-dir", str(workspace), "--agent", "claude"] + ) + assert r.exit_code == 2, r.output + assert "ZENITH_WORKER_MODEL" in r.output + + def test_init_ignores_an_invalid_inherited_model_env_a_flag_replaces( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # Diverges from the reasoning-effort contract on purpose. An effort var + # is forwarded into the workspace, so a broken one stays live; a model + # pin is not, and the flag's value is what every server started from + # this workspace will read. Failing on a value init has already decided + # to ignore would be a dead end for the user. + monkeypatch.setenv("ZENITH_WORKER_MODEL", "opus; touch /tmp/pwned") + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--worker-model", + "sonnet", + ], + ) + assert r.exit_code == 0, r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + assert server_env["ZENITH_WORKER_MODEL"] == "sonnet" + # The caller's environment is left as it was found. + assert os.environ["ZENITH_WORKER_MODEL"] == "opus; touch /tmp/pwned" + + def test_init_rejects_model_flag_with_shell_metacharacters( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + for var in ( + "ZENITH_WORKER_MODEL", + "ZENITH_VALIDATOR_MODEL", + "ZENITH_TERMINAL_REVIEWER_MODEL", + ): + monkeypatch.delenv(var, raising=False) + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "codex", + "--worker-model", + 'gpt-5.5"; rm -rf /; #', + ], + ) + + # A flag value lands in the same shell command line as the env var, so + # it gets the same validation instead of being written out verbatim — + # but reported as a usage error naming the flag the user actually + # typed, not a traceback naming an env var they never set. + assert r.exit_code == 2 + assert "--worker-model" in r.output + assert "ZENITH_WORKER_MODEL" not in r.output + assert not (workspace / ".codex" / "config.toml").exists() + + def test_init_rejects_empty_model_flag( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # An inherited pin the user is trying to clear must not be silently + # forwarded as if they had said nothing. + monkeypatch.setenv("ZENITH_WORKER_MODEL", "sonnet") + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--worker-model", + "", + ], + ) + + assert r.exit_code == 2 + assert "--worker-model" in r.output + + def test_init_warns_when_model_pin_targets_hermes( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + for var in ( + "ZENITH_WORKER_MODEL", + "ZENITH_VALIDATOR_MODEL", + "ZENITH_TERMINAL_REVIEWER_MODEL", + ): + monkeypatch.delenv(var, raising=False) + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "hermes", + "--worker-model", + "some-model", + ], + ) + assert r.exit_code == 0, r.output + + # hermes takes no model selection, so the pin is a no-op. Init still + # succeeds — but silently discarding what the user typed is the bug. + assert "warning" in r.output.lower() + assert "--worker-model" in r.output + assert "hermes" in r.output + + def test_init_persists_terminal_reviewer_provider( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.delenv("ZENITH_TERMINAL_REVIEWER_PROVIDER", raising=False) + monkeypatch.delenv("ZENITH_TERMINAL_REVIEWER_ACP_COMMAND", raising=False) + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--terminal-reviewer-provider", + "codex", + "--terminal-reviewer-acp-command", + "codex-acp", + ], + ) + assert r.exit_code == 0, r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + # Accepted-and-discarded is worse than rejected: the flag reads as + # configured while every terminal review runs on the validator's + # provider instead. + assert server_env["ZENITH_TERMINAL_REVIEWER_PROVIDER"] == "codex" + assert server_env["ZENITH_TERMINAL_REVIEWER_ACP_COMMAND"] == "codex-acp" + + def test_init_warns_when_model_flag_targets_hermes( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "hermes", + "--worker-model", + "some-model", + ], + ) + assert r.exit_code == 0, r.output + + assert "warning" in r.output.lower() + assert "--worker-model" in r.output + assert "hermes" in r.output + + def test_init_warns_when_a_pinned_lane_launches_a_custom_command( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # Provider-specific treatment (ANTHROPIC_MODEL for claude, `-c model=` + # for codex) is applied for the provider the lane dispatches as, while + # the command is configured independently — point at the mismatch + # instead of letting the pin vanish into a binary that never reads it. + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--terminal-reviewer-acp-command", + "codex-acp", + "--terminal-reviewer-model", + "opus", + ], + ) + assert r.exit_code == 0, r.output + + assert "warning" in r.output.lower() + assert "terminal reviewer" in r.output + assert "codex-acp" in r.output + + def test_init_does_not_warn_when_the_command_is_the_providers_own_default( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # Spelling out the default a lane would have used anyway is not a + # mismatch. Warning here would train the user to ignore the warning + # above, which is the one that means something. + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--worker-acp-command", + "claude-agent-acp", + "--worker-model", + "opus", + ], + ) + assert r.exit_code == 0, r.output + + assert "warning" not in r.output.lower() + + def test_init_installs_assets_for_a_validator_resolved_from_the_environment( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # The reviewer is pinned elsewhere, so it cannot mask the gap by + # inheriting the validator's provider: a config naming a codex + # validator must come with codex agents and skills on disk. + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "codex") + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--terminal-reviewer-provider", + "claude", + ], + ) + assert r.exit_code == 0, r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + assert server_env["ZENITH_VALIDATOR_PROVIDER"] == "codex" + assert (workspace / ".codex" / "agents").is_dir() + + def test_init_terminal_reviewer_inherits_the_validator_not_the_worker( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # Matches for_role("terminal_reviewer"), which falls back to the + # validator. Init now WRITES this provider unconditionally, so a wrong + # inheritance here cannot be corrected by the runtime chain — the + # written value wins. + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--validator-provider", + "codex", + ], + ) + assert r.exit_code == 0, r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + assert server_env["ZENITH_TERMINAL_REVIEWER_PROVIDER"] == "codex" + + def test_init_command_flag_beats_ambient_command_env( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # Same precedence claim the provider flags carry, for commands. + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "claude") + monkeypatch.setenv("ZENITH_VALIDATOR_ACP_COMMAND", "stale-agent-acp") + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--validator-acp-command", + "claude-agent-acp --lane validate", + ], + ) + assert r.exit_code == 0, r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + assert server_env["ZENITH_VALIDATOR_ACP_COMMAND"] == "claude-agent-acp --lane validate" + + def test_init_drops_an_ambient_command_whose_provider_it_discarded( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # Init sets the worker provider itself, so a shell that was talking + # about a different provider must not get to set the worker's command. + # Keeping half of a stale pair manufactures a claude lane that launches + # codex-acp: no sandbox flags, no codex env, and a claude-only ACP mode + # id sent to codex — durably, in the config init just wrote. + monkeypatch.setenv("ZENITH_WORKER_PROVIDER", "codex") + monkeypatch.setenv("ZENITH_WORKER_ACP_COMMAND", "codex-acp") + + r = runner.invoke(cli, ["init", "--workspace-dir", str(workspace), "--agent", "claude"]) + assert r.exit_code == 0, r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + assert server_env["ZENITH_WORKER_PROVIDER"] == "claude" + assert server_env.get("ZENITH_WORKER_ACP_COMMAND") != "codex-acp" + + def test_init_keeps_an_ambient_command_paired_with_its_provider( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # The other half of the pairing rule: exported together, they describe + # one coherent lane, and dropping the command would send it back to the + # worker's binary at runtime — the original defect. + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "codex") + monkeypatch.setenv("ZENITH_VALIDATOR_ACP_COMMAND", "codex-acp --lane validate") + + r = runner.invoke(cli, ["init", "--workspace-dir", str(workspace), "--agent", "claude"]) + assert r.exit_code == 0, r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + assert server_env["ZENITH_VALIDATOR_PROVIDER"] == "codex" + assert server_env["ZENITH_VALIDATOR_ACP_COMMAND"] == "codex-acp --lane validate" + + def test_init_warns_about_a_foreign_command( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # The mismatch is a hazard on its own: sandbox flags, the codex config + # flags and the ACP session mode all key on provider.name, so a model + # pin is incidental to it and the warning is not gated on one. + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--worker-acp-command", + "codex-acp", + ], + ) + assert r.exit_code == 0, r.output + + assert "warning" in r.output.lower() + assert "codex-acp" in r.output + + @pytest.mark.parametrize( + "command", + ["claude-agent-acp --verbose", "/usr/local/bin/claude-agent-acp"], + ) + def test_init_does_not_warn_for_the_providers_own_binary( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + command: str, + ) -> None: + # An absolute path or extra arguments still run claude-agent-acp. The + # question is which binary runs, not whether the string matches. + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--worker-acp-command", + command, + ], + ) + assert r.exit_code == 0, r.output + + assert "warning" not in r.output.lower() + + def test_init_summary_reports_the_resolved_roles( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # The summary is the only thing most users read; it must not contradict + # the config written one line earlier. + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "codex") + + r = runner.invoke(cli, ["init", "--workspace-dir", str(workspace), "--agent", "claude"]) + assert r.exit_code == 0, r.output + + assert "validator=codex" in r.output + + def test_init_warns_when_ambient_provider_env_makes_the_lane_hermes( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # An ambient role provider is resolved AND written, so the warning + # describes the workspace init produced rather than the shell it ran + # in. Asserting the written value is the point: the host agent is + # normally launched later, from a different shell. + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "hermes") + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--validator-model", + "opus", + ], + ) + assert r.exit_code == 0, r.output + + assert "warning" in r.output.lower() + assert "--validator-model" in r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + assert server_env["ZENITH_VALIDATOR_PROVIDER"] == "hermes" + + def test_init_provider_flag_beats_ambient_provider_env( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # A stale export must not overrule what the user typed, and a typo in + # it must not fail an init that never consults it. + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "clyde") + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--validator-provider", + "claude", + "--validator-model", + "opus", + ], + ) + assert r.exit_code == 0, r.output + assert "warning" not in r.output.lower() + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + assert server_env["ZENITH_VALIDATOR_PROVIDER"] == "claude" + assert server_env["ZENITH_VALIDATOR_MODEL"] == "opus" + + def test_init_does_not_warn_when_ambient_provider_env_rescues_the_lane( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # Mirror case: the reviewer lane resolves to claude, so the pin is + # honored and warning about it would be a lie. The rescue has to be + # written down for that to stay true after init exits. + monkeypatch.setenv("ZENITH_TERMINAL_REVIEWER_PROVIDER", "claude") + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "hermes", + "--terminal-reviewer-model", + "opus", + ], + ) + assert r.exit_code == 0, r.output + + assert "--terminal-reviewer-model" not in r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + assert server_env["ZENITH_TERMINAL_REVIEWER_PROVIDER"] == "claude" + assert server_env["ZENITH_TERMINAL_REVIEWER_MODEL"] == "opus" + + def test_written_config_reproduces_init_resolution_in_a_clean_environment( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """The seam every other test in this file stops short of. + + Init resolves lanes; the server resolves them again from the written + config, in a later process launched from a different shell. Every + divergence between those two resolvers is a bug that per-lane + assertions on init's output cannot see, so this drives the real + `discover() + for_role()` over exactly what init wrote. + """ + # Validator lands on a different provider than the worker, so the + # worker's pin must not reach it; the reviewer lands back on the + # worker's provider, so it must jump the gap and pick the pin up. + # Both role settings that init can only learn from the environment are + # exercised — a provider and a command. + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "codex") + monkeypatch.setenv("ZENITH_VALIDATOR_ACP_COMMAND", "codex-acp --lane validate") + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--worker-model", + "opus", + "--worker-reasoning-effort", + "max", + "--terminal-reviewer-provider", + "claude", + ], + ) + assert r.exit_code == 0, r.output + + mcp = json.loads((workspace / ".mcp.json").read_text(encoding="utf-8")) + server_env = mcp["mcpServers"]["zenith"]["env"] + + # Stand in for the later launch: nothing survives but the file. + for var in list(os.environ): + if var.startswith("ZENITH_") or var == "ANTHROPIC_MODEL": + monkeypatch.delenv(var, raising=False) + for key, value in server_env.items(): + monkeypatch.setenv(key, value) + + config = HarnessConfig.discover() + + worker = config.for_role("worker") + assert worker.worker_provider_name == "claude" + assert worker.worker_model == "opus" + assert worker.worker_reasoning_effort == "max" + assert worker.resolved_worker_acp_command == "claude-agent-acp" + + validator = config.for_role("validator") + assert validator.worker_provider_name == "codex" + assert validator.worker_model is None + assert validator.resolved_worker_acp_command == "codex-acp --lane validate" + # Provider-neutral vocabulary, so this one does inherit across the gap. + assert validator.worker_reasoning_effort == "max" + + reviewer = config.for_role("terminal_reviewer") + assert reviewer.worker_provider_name == "claude" + assert reviewer.worker_model == "opus" + + def test_init_installs_assets_for_terminal_reviewer_provider( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.delenv("ZENITH_TERMINAL_REVIEWER_PROVIDER", raising=False) + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--terminal-reviewer-provider", + "codex", + ], + ) + assert r.exit_code == 0, r.output + + # The reviewer really runs codex-acp now, so it needs the same asset + # surface --validator-provider codex would have installed. + assert (workspace / ".codex" / "agents").is_dir() + + def test_init_rejects_unknown_ambient_terminal_reviewer_provider_before_writing( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + # The flags carry a click.Choice, but an exported var does not — and it + # is read late, so a typo used to raise ValueError only after the config + # had been written, leaving a half-initialized workspace behind. + monkeypatch.setenv("ZENITH_TERMINAL_REVIEWER_PROVIDER", "clyde") + + r = runner.invoke( + cli, ["init", "--workspace-dir", str(workspace), "--agent", "claude"] + ) + + assert r.exit_code != 0 + assert "clyde" in r.output + assert "ZENITH_TERMINAL_REVIEWER_PROVIDER" in r.output + assert not (workspace / ".mcp.json").exists() + + def test_init_error_names_the_variable_that_supplied_the_bad_provider( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.delenv("ZENITH_TERMINAL_REVIEWER_PROVIDER", raising=False) + # The terminal lane inherits this name, but blaming + # ZENITH_TERMINAL_REVIEWER_PROVIDER sends the user hunting for a + # variable they never set. + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "clyde") + + r = runner.invoke( + cli, ["init", "--workspace-dir", str(workspace), "--agent", "claude"] + ) + + assert r.exit_code != 0 + assert "ZENITH_VALIDATOR_PROVIDER" in r.output + assert "ZENITH_TERMINAL_REVIEWER_PROVIDER" not in r.output + assert not (workspace / ".mcp.json").exists() + + def test_init_validates_validator_provider_even_when_terminal_is_explicit( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "clyde") + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--terminal-reviewer-provider", + "codex", + ], + ) + + # An explicit terminal provider must not mask a broken validator lane: + # without this the typo detonates mid-mission at the first validate + # dispatch instead of at init. + assert r.exit_code != 0 + assert "ZENITH_VALIDATOR_PROVIDER" in r.output + assert not (workspace / ".mcp.json").exists() + + def test_init_does_not_warn_for_supported_provider_pin( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + for var in ( + "ZENITH_WORKER_MODEL", + "ZENITH_VALIDATOR_MODEL", + "ZENITH_TERMINAL_REVIEWER_MODEL", + ): + monkeypatch.delenv(var, raising=False) + + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--worker-model", + "opus", + ], + ) + assert r.exit_code == 0, r.output + assert "warning" not in r.output.lower() def test_claude_init_writes_runtime_validator_env_names( self, runner: CliRunner, workspace: Path, env: dict[str, str] diff --git a/zenith/tests/test_config.py b/zenith/tests/test_config.py index 83c9c49..f5f8391 100644 --- a/zenith/tests/test_config.py +++ b/zenith/tests/test_config.py @@ -14,11 +14,23 @@ ) +_MODEL_ENV_VARS = ( + "ZENITH_WORKER_MODEL", + "ZENITH_VALIDATOR_MODEL", + "ZENITH_TERMINAL_REVIEWER_MODEL", +) + + def _clear_effort_env(monkeypatch) -> None: for var in _EFFORT_ENV_VARS: monkeypatch.delenv(var, raising=False) +def _clear_model_env(monkeypatch) -> None: + for var in _MODEL_ENV_VARS: + monkeypatch.delenv(var, raising=False) + + def test_discover_defaults_to_four_parallel_nodes( monkeypatch, harness_home: Path, @@ -140,3 +152,214 @@ def test_for_role_reasoning_effort_explicit_override_wins( assert config.for_role("validator").worker_reasoning_effort == "low" # terminal_reviewer falls back to the validator setting first. assert config.for_role("terminal_reviewer").worker_reasoning_effort == "low" + + +def test_discover_model_defaults_to_none( + monkeypatch, + harness_home: Path, +) -> None: + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + _clear_model_env(monkeypatch) + + config = HarnessConfig.discover() + + # None means "whatever the provider picks" — no pin. + assert config.worker_model is None + assert config.validator_model is None + assert config.terminal_reviewer_model is None + + +def test_discover_model_per_role( + monkeypatch, + harness_home: Path, +) -> None: + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + monkeypatch.setenv("ZENITH_WORKER_MODEL", "opus") + monkeypatch.setenv("ZENITH_VALIDATOR_MODEL", "claude-opus-5[1m]") + monkeypatch.setenv("ZENITH_TERMINAL_REVIEWER_MODEL", "gpt-5.5") + + config = HarnessConfig.discover() + + assert config.worker_model == "opus" + assert config.validator_model == "claude-opus-5[1m]" + assert config.terminal_reviewer_model == "gpt-5.5" + + +def test_discover_model_with_shell_metacharacters_rejected( + monkeypatch, + harness_home: Path, +) -> None: + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + _clear_model_env(monkeypatch) + # The resolved value is spliced into a shell command line for codex + # (`-c model="..."`), so anything that could break out of the quotes is + # rejected at discovery rather than executed. + monkeypatch.setenv("ZENITH_WORKER_MODEL", 'opus"; rm -rf /; #') + + with pytest.raises(ValueError, match="ZENITH_WORKER_MODEL"): + HarnessConfig.discover() + + +def test_discover_model_with_trailing_newline_rejected( + monkeypatch, + harness_home: Path, +) -> None: + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + _clear_model_env(monkeypatch) + # `$` matches before a trailing newline, so a pattern anchored with it + # would accept "opus\n" — which then gets written into .codex/config.toml + # as an unescaped newline inside a TOML basic string and corrupts the file. + monkeypatch.setenv("ZENITH_WORKER_MODEL", "opus\n") + + with pytest.raises(ValueError, match="ZENITH_WORKER_MODEL"): + HarnessConfig.discover() + + +def test_for_role_model_cascade( + monkeypatch, + harness_home: Path, +) -> None: + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + _clear_model_env(monkeypatch) + _clear_provider_env(monkeypatch) + monkeypatch.setenv("ZENITH_WORKER_MODEL", "opus") + + config = HarnessConfig.discover() + + # Same inheritance chain as providers/commands/effort: + # terminal_reviewer -> validator -> worker. + assert config.for_role("worker").worker_model == "opus" + assert config.for_role("validator").worker_model == "opus" + assert config.for_role("terminal_reviewer").worker_model == "opus" + + +def _clear_provider_env(monkeypatch) -> None: + for var in ( + "ZENITH_WORKER_PROVIDER", + "ZENITH_VALIDATOR_PROVIDER", + "ZENITH_TERMINAL_REVIEWER_PROVIDER", + ): + monkeypatch.delenv(var, raising=False) + + +def test_for_role_model_does_not_cascade_across_providers( + monkeypatch, + harness_home: Path, +) -> None: + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + _clear_model_env(monkeypatch) + _clear_provider_env(monkeypatch) + monkeypatch.setenv("ZENITH_WORKER_PROVIDER", "codex") + monkeypatch.setenv("ZENITH_WORKER_MODEL", "gpt-5.5") + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "claude") + + config = HarnessConfig.discover() + + # Reasoning efforts are provider-neutral vocabulary, so they cascade freely. + # Model ids are not: inheriting the codex worker's pin would hand + # ANTHROPIC_MODEL="gpt-5.5" to a claude validator and break every + # validation session. An unpinned role on a different provider falls back + # to that provider's own default instead. + assert config.for_role("worker").worker_model == "gpt-5.5" + assert config.for_role("validator").worker_model is None + assert config.for_role("terminal_reviewer").worker_model is None + + +def test_for_role_model_cascades_when_provider_matches( + monkeypatch, + harness_home: Path, +) -> None: + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + _clear_model_env(monkeypatch) + _clear_provider_env(monkeypatch) + monkeypatch.setenv("ZENITH_WORKER_PROVIDER", "claude") + monkeypatch.setenv("ZENITH_WORKER_MODEL", "opus") + # Spelling the provider out explicitly must not defeat inheritance. + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "claude") + + config = HarnessConfig.discover() + + assert config.for_role("validator").worker_model == "opus" + assert config.for_role("terminal_reviewer").worker_model == "opus" + + +def test_for_role_model_explicit_pin_survives_provider_switch( + monkeypatch, + harness_home: Path, +) -> None: + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + _clear_model_env(monkeypatch) + _clear_provider_env(monkeypatch) + monkeypatch.setenv("ZENITH_WORKER_PROVIDER", "codex") + monkeypatch.setenv("ZENITH_WORKER_MODEL", "gpt-5.5") + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "claude") + monkeypatch.setenv("ZENITH_VALIDATOR_MODEL", "opus") + + config = HarnessConfig.discover() + + # Only inheritance is provider-gated; a pin set for this role is obeyed. + assert config.for_role("validator").worker_model == "opus" + + +def test_for_role_terminal_reviewer_model_does_not_inherit_across_providers( + monkeypatch, + harness_home: Path, +) -> None: + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + _clear_model_env(monkeypatch) + _clear_provider_env(monkeypatch) + monkeypatch.setenv("ZENITH_WORKER_PROVIDER", "claude") + monkeypatch.setenv("ZENITH_VALIDATOR_PROVIDER", "claude") + monkeypatch.setenv("ZENITH_VALIDATOR_MODEL", "opus") + monkeypatch.setenv("ZENITH_TERMINAL_REVIEWER_PROVIDER", "codex") + + config = HarnessConfig.discover() + + # The validator's claude pin must not reach a codex terminal reviewer. + assert config.for_role("terminal_reviewer").worker_model is None + + +def test_for_role_terminal_reviewer_explicit_model_beats_validator_pin( + monkeypatch, + harness_home: Path, +) -> None: + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + _clear_model_env(monkeypatch) + _clear_provider_env(monkeypatch) + monkeypatch.setenv("ZENITH_WORKER_MODEL", "sonnet") + monkeypatch.setenv("ZENITH_VALIDATOR_MODEL", "opus") + monkeypatch.setenv("ZENITH_TERMINAL_REVIEWER_MODEL", "claude-opus-5[1m]") + + config = HarnessConfig.discover() + + # Precedence within the chain: own pin first, then validator, then worker. + assert config.for_role("terminal_reviewer").worker_model == "claude-opus-5[1m]" + + +def test_for_role_model_explicit_override_wins( + monkeypatch, + harness_home: Path, +) -> None: + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + _clear_model_env(monkeypatch) + _clear_provider_env(monkeypatch) + monkeypatch.setenv("ZENITH_WORKER_MODEL", "sonnet") + monkeypatch.setenv("ZENITH_VALIDATOR_MODEL", "opus") + + config = HarnessConfig.discover() + + assert config.for_role("worker").worker_model == "sonnet" + assert config.for_role("validator").worker_model == "opus" + # terminal_reviewer falls back to the validator setting first. + assert config.for_role("terminal_reviewer").worker_model == "opus"