From cc5dff56c4e5f7699e9e36dea56db1c82869d25f Mon Sep 17 00:00:00 2001 From: Andy Batten Date: Fri, 7 Aug 2026 22:05:03 -0700 Subject: [PATCH 1/2] fix(cli): resolve every role at init and write what was resolved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `zenith init` read ambient ZENITH_*_PROVIDER when deciding what a lane would run, but never wrote those vars: ProviderSelection.env() emits a role provider only when it differs from the worker, and none of them are in RUNTIME_ENV_FORWARD_ALLOWLIST. The host agent is normally launched later, from a different shell, so init's view and the workspace's view could disagree — a diagnostic naming a hermes validator whose written config said claude, silence in the mirror case, and an init that failed on an ambient provider typo it would never have used. Init now resolves each role once (flag, then ambient var, then inherit down the chain) and writes the resolved provider and ACP command, so the runtime `discover() + for_role()` reproduces what init reported. The worker is deliberately exempt from the ambient step: it is the only role with a default of its own, and ZENITH_WORKER_PROVIDER has always been written unconditionally. An ACP command is taken from the environment only for a lane whose provider also came from the environment. Splitting a stale exported pair would pin one provider's binary to another provider's dispatch — no sandbox flags, no provider env, and a claude-only ACP mode id sent to codex. Also: assets are installed for every resolved role, not just the ones ProviderSelection knows about; a lane whose command does not run its provider's binary is warned about (comparing the executable, so an absolute path or extra arguments do not trip it); the summary reports resolved names instead of the flag-only view; and discover()'s ValueError is reported as a usage error rather than a traceback. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PwnPYNttumenbdFpPkvHWa --- zenith/src/zenith_harness/cli.py | 182 +++++++++++- zenith/tests/test_cli.py | 467 +++++++++++++++++++++++++++++-- 2 files changed, 621 insertions(+), 28 deletions(-) diff --git a/zenith/src/zenith_harness/cli.py b/zenith/src/zenith_harness/cli.py index 22a8ce7..120742f 100644 --- a/zenith/src/zenith_harness/cli.py +++ b/zenith/src/zenith_harness/cli.py @@ -103,7 +103,14 @@ 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. + try: + config = HarnessConfig.discover() + except ValueError as exc: + raise click.UsageError(str(exc)) from None loader = AssetLoader(config) selection = _resolve_selection( agent=agent, @@ -132,17 +139,180 @@ def init( ) if value } - _write_bootstrap_config(workspace, selection, storage_env, effort_env) + # 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. 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, **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 + + # 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: sandbox flags, the codex config flags and the ACP session + # mode all key on provider.name. + for role, provider_def, command_var in ( + ("worker", selection.worker, "ZENITH_WORKER_ACP_COMMAND"), + ("validator", validator_provider_def, "ZENITH_VALIDATOR_ACP_COMMAND"), + ( + "terminal reviewer", + terminal_reviewer_provider_def, + "ZENITH_TERMINAL_REVIEWER_ACP_COMMAND", + ), + ): + 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 " + "and the ACP session mode are 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/tests/test_cli.py b/zenith/tests/test_cli.py index d5da40a..9109e2f 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,17 @@ 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 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,13 +249,87 @@ 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_runtime_validator_env_names( - self, runner: CliRunner, workspace: Path, env: dict[str, str] + 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_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, [ @@ -253,41 +340,377 @@ def test_claude_init_writes_runtime_validator_env_names( "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", - "custom-validator-acp", + "claude-agent-acp --lane validate", ], ) assert r.exit_code == 0, r.output - mcp = json.loads((workspace / ".mcp.json").read_text()) - mcp_env = mcp["mcpServers"]["zenith"]["env"] - assert mcp_env["ZENITH_VALIDATOR_PROVIDER"] == "codex" - assert mcp_env["ZENITH_VALIDATOR_ACP_COMMAND"] == "custom-validator-acp" - assert "ZENITH_VALIDATION_WORKER_PROVIDER" not in mcp_env - assert "ZENITH_VALIDATION_WORKER_ACP_COMMAND" not in mcp_env + 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: + # Sandbox flags, the codex config flags and the ACP session mode all + # key on provider.name, so a lane that launches somebody else's + # binary gets all of them wrong. + 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_claude_init_forwards_only_allowed_model_env( + def test_init_summary_reports_the_resolved_roles( self, runner: CliRunner, workspace: Path, env: dict[str, str], monkeypatch: pytest.MonkeyPatch, ) -> None: - monkeypatch.setenv("ANTHROPIC_BASE_URL", "https://api.z.ai/api/anthropic") - monkeypatch.setenv("ANTHROPIC_MODEL", "glm-5.2[1m]") - monkeypatch.setenv("ZAI_API_KEY", "zai-test-key") - monkeypatch.setenv("DATABASE_URL", "postgres://should-not-forward") + # 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_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", + ], + ) + 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" + + 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. + """ + # Both role settings that init can only learn from the environment + # are exercised — a provider and a command — across a lane that + # lands on a different provider than the worker. + 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-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_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.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" + + 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_claude_init_writes_runtime_validator_env_names( + self, runner: CliRunner, workspace: Path, env: dict[str, str] + ) -> None: + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--validator-provider", + "codex", + "--validator-acp-command", + "custom-validator-acp", + ], + ) + assert r.exit_code == 0, r.output + mcp = json.loads((workspace / ".mcp.json").read_text()) mcp_env = mcp["mcpServers"]["zenith"]["env"] - assert mcp_env["ANTHROPIC_BASE_URL"] == "https://api.z.ai/api/anthropic" - assert mcp_env["ANTHROPIC_MODEL"] == "glm-5.2[1m]" - assert mcp_env["ZAI_API_KEY"] == "zai-test-key" - assert "DATABASE_URL" not in mcp_env - + assert mcp_env["ZENITH_VALIDATOR_PROVIDER"] == "codex" + assert mcp_env["ZENITH_VALIDATOR_ACP_COMMAND"] == "custom-validator-acp" + assert "ZENITH_VALIDATION_WORKER_PROVIDER" not in mcp_env + assert "ZENITH_VALIDATION_WORKER_ACP_COMMAND" not in mcp_env class TestListProjects: def test_empty(self, runner: CliRunner, env: dict[str, str]) -> None: From f704a8da105e2d24d9f1ddf48dd9addbf5054c4d Mon Sep 17 00:00:00 2001 From: Andy Batten Date: Fri, 7 Aug 2026 22:13:31 -0700 Subject: [PATCH 2/2] feat(config): per-role model pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds ZENITH_{WORKER,VALIDATOR,TERMINAL_REVIEWER}_MODEL and matching --*-model flags on `zenith init`, so a mission can run its workers on one model and its validation gate on another. codex receives the pin as `-c model="..."`; claude-agent-acp has no model flag, so its pin travels as ANTHROPIC_MODEL in the subprocess env. A pin never crosses a provider boundary. Reasoning efforts are provider-neutral vocabulary and inherit freely down the role chain, but a codex worker's "gpt-5.5" handed to a claude validator would become ANTHROPIC_MODEL="gpt-5.5" and break every session on that lane, so _inherited_model walks the chain skipping links from other providers. For the same reason the env vars are deliberately NOT in RUNTIME_ENV_FORWARD_ALLOWLIST: an ambient pin arrives with no record of which provider it was chosen for, so baking it into a workspace lands a leftover codex pin on whatever provider that workspace runs. Pins enter a workspace only through the flags, which are checked against the provider resolved for that lane. An ambient pin still reaches a server launched from the same shell; init just does not make it durable. An invalid one fails init unless a flag replaces it. Model ids are open-ended (provider aliases, pinned ids, Bedrock/Vertex ARNs), so they cannot be checked against an allowlist the way efforts are. They do reach a shell command line for codex, so the character set is restricted with fullmatch to what real model identifiers use — no quotes, spaces, or shell operators, and no trailing newline. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PwnPYNttumenbdFpPkvHWa --- zenith/src/zenith_harness/acp_runner.py | 31 +- zenith/src/zenith_harness/cli.py | 129 ++++++- zenith/src/zenith_harness/config.py | 98 ++++- zenith/tests/test_acp_runner.py | 221 +++++++++++ zenith/tests/test_cli.py | 472 +++++++++++++++++++++++- zenith/tests/test_config.py | 223 +++++++++++ 6 files changed, 1142 insertions(+), 32 deletions(-) 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 120742f..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: @@ -107,10 +145,29 @@ def init( # 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, @@ -139,6 +196,37 @@ def init( ) if value } + # 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 @@ -184,7 +272,7 @@ def _pick(*candidates: tuple[str | None, str]) -> tuple[str, str]: ) # An ACP command names a binary, so it is the most provider-specific value - # in the config. It is therefore taken from the + # 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 @@ -234,7 +322,7 @@ def _role_command( "ZENITH_TERMINAL_REVIEWER_PROVIDER": terminal_reviewer_provider_name, **role_command, } - cli_env = {**effort_env, **role_env} + 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 @@ -266,25 +354,42 @@ def _runs_provider_binary(command: str, provider_def: ProviderDefinition) -> boo return False return Path(command.split()[0]).name == Path(default.split()[0]).name - # 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: sandbox flags, the codex config flags and the ACP session - # mode all key on provider.name. - for role, provider_def, command_var in ( - ("worker", selection.worker, "ZENITH_WORKER_ACP_COMMAND"), - ("validator", validator_provider_def, "ZENITH_VALIDATOR_ACP_COMMAND"), + # 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 " - "and the ACP session mode are applied the way " + 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." ) 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 9109e2f..728ed24 100644 --- a/zenith/tests/test_cli.py +++ b/zenith/tests/test_cli.py @@ -21,7 +21,8 @@ def runner() -> 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 provider or command would otherwise leak into these + 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"): @@ -254,6 +255,250 @@ def test_init_invalid_inherited_effort_env_fails_despite_flag( 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, @@ -288,6 +533,90 @@ def test_init_persists_terminal_reviewer_provider( 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, @@ -428,9 +757,9 @@ def test_init_warns_about_a_foreign_command( env: dict[str, str], monkeypatch: pytest.MonkeyPatch, ) -> None: - # Sandbox flags, the codex config flags and the ACP session mode all - # key on provider.name, so a lane that launches somebody else's - # binary gets all of them wrong. + # 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, [ @@ -494,6 +823,40 @@ def test_init_summary_reports_the_resolved_roles( 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, @@ -515,6 +878,8 @@ def test_init_provider_flag_beats_ambient_provider_env( "claude", "--validator-provider", "claude", + "--validator-model", + "opus", ], ) assert r.exit_code == 0, r.output @@ -523,6 +888,40 @@ def test_init_provider_flag_beats_ambient_provider_env( 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, @@ -539,9 +938,11 @@ def test_written_config_reproduces_init_resolution_in_a_clean_environment( assertions on init's output cannot see, so this drives the real `discover() + for_role()` over exactly what init wrote. """ - # Both role settings that init can only learn from the environment - # are exercised — a provider and a command — across a lane that - # lands on a different provider than the worker. + # 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") @@ -553,6 +954,8 @@ def test_written_config_reproduces_init_resolution_in_a_clean_environment( str(workspace), "--agent", "claude", + "--worker-model", + "opus", "--worker-reasoning-effort", "max", "--terminal-reviewer-provider", @@ -575,17 +978,20 @@ def test_written_config_reproduces_init_resolution_in_a_clean_environment( 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, @@ -686,6 +1092,35 @@ def test_init_validates_validator_provider_even_when_terminal_is_explicit( 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] ) -> None: @@ -712,6 +1147,29 @@ def test_claude_init_writes_runtime_validator_env_names( assert "ZENITH_VALIDATION_WORKER_PROVIDER" not in mcp_env assert "ZENITH_VALIDATION_WORKER_ACP_COMMAND" not in mcp_env + def test_claude_init_forwards_only_allowed_model_env( + self, + runner: CliRunner, + workspace: Path, + env: dict[str, str], + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.setenv("ANTHROPIC_BASE_URL", "https://api.z.ai/api/anthropic") + monkeypatch.setenv("ANTHROPIC_MODEL", "glm-5.2[1m]") + monkeypatch.setenv("ZAI_API_KEY", "zai-test-key") + monkeypatch.setenv("DATABASE_URL", "postgres://should-not-forward") + + 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()) + mcp_env = mcp["mcpServers"]["zenith"]["env"] + assert mcp_env["ANTHROPIC_BASE_URL"] == "https://api.z.ai/api/anthropic" + assert mcp_env["ANTHROPIC_MODEL"] == "glm-5.2[1m]" + assert mcp_env["ZAI_API_KEY"] == "zai-test-key" + assert "DATABASE_URL" not in mcp_env + + class TestListProjects: def test_empty(self, runner: CliRunner, env: dict[str, str]) -> None: r = runner.invoke(cli, ["list-projects"]) 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"