From 1bb8474a4861e79b90106942c6257dcdf24fa4b3 Mon Sep 17 00:00:00 2001 From: Stephan Brezinsky Date: Fri, 17 Jul 2026 19:37:50 -0400 Subject: [PATCH] fix(cli): wire --terminal-reviewer-provider/acp-command through to config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The --terminal-reviewer-provider and --terminal-reviewer-acp-command flags were accepted by click but silently dropped in _resolve_selection: ProviderSelection had no terminal_reviewer fields, so the values never reached env() and were never written to .mcp.json or .codex/config.toml. HarnessConfig already reads ZENITH_TERMINAL_REVIEWER_PROVIDER and ZENITH_TERMINAL_REVIEWER_ACP_COMMAND with a full terminal_reviewer -> validator -> worker cascade — only the write side was broken. Add terminal_reviewer and terminal_reviewer_acp_command fields to ProviderSelection, with resolved_terminal_reviewer and resolved_terminal_reviewer_acp_command properties mirroring the validation_worker pattern. Update env() to emit the terminal-reviewer vars only when they differ from the validator's (the cascade parent), matching the validator's comparison against the worker. Update providers() and skill dirs to include the terminal reviewer for asset installation when it's a distinct provider. Issue creation is restricted on this repository, so a PR is filed directly per CONTRIBUTING.md's fallback for small fixes. Tests: - terminal-reviewer provider + acp-command land in .mcp.json env. - unset flags emit no terminal-reviewer env keys (cascade handles it). - three distinct providers (claude/codex/hermes) all write their env vars in a single init. - round-trip: a distinct terminal reviewer written by env() is read back intact by HarnessConfig.discover(). - round-trip: with no explicit terminal reviewer, env() omits the vars and discover() reconstructs the validator as the cascade fallback. Co-Authored-By: Claude Opus 4.8 (1M context) --- zenith/src/zenith_harness/cli.py | 2 + zenith/src/zenith_harness/config.py | 6 +++ zenith/src/zenith_harness/providers.py | 31 ++++++++++++ zenith/tests/test_cli.py | 63 +++++++++++++++++++++++++ zenith/tests/test_config.py | 65 ++++++++++++++++++++++++++ 5 files changed, 167 insertions(+) diff --git a/zenith/src/zenith_harness/cli.py b/zenith/src/zenith_harness/cli.py index f4cd9f4..04fb237 100644 --- a/zenith/src/zenith_harness/cli.py +++ b/zenith/src/zenith_harness/cli.py @@ -296,6 +296,8 @@ def _resolve_selection( validation_worker=get_provider(validator) if validator else None, worker_acp_command=worker_acp_command, validation_worker_acp_command=validator_acp_command, + terminal_reviewer=get_provider(terminal_reviewer) if terminal_reviewer else None, + terminal_reviewer_acp_command=terminal_reviewer_acp_command, ) diff --git a/zenith/src/zenith_harness/config.py b/zenith/src/zenith_harness/config.py index 0508f74..24b83ab 100644 --- a/zenith/src/zenith_harness/config.py +++ b/zenith/src/zenith_harness/config.py @@ -155,6 +155,12 @@ def provider_selection(self) -> ProviderSelection: ), worker_acp_command=self.worker_acp_command, validation_worker_acp_command=self.validator_acp_command, + terminal_reviewer=( + self.terminal_reviewer_provider + if self.terminal_reviewer_provider_name + else None + ), + terminal_reviewer_acp_command=self.terminal_reviewer_acp_command, ) # ------------------------------------------------------------------ diff --git a/zenith/src/zenith_harness/providers.py b/zenith/src/zenith_harness/providers.py index 69043c5..f61ecf9 100644 --- a/zenith/src/zenith_harness/providers.py +++ b/zenith/src/zenith_harness/providers.py @@ -38,6 +38,8 @@ class ProviderSelection: validation_worker: ProviderDefinition | None = None worker_acp_command: str | None = None validation_worker_acp_command: str | None = None + terminal_reviewer: ProviderDefinition | None = None + terminal_reviewer_acp_command: str | None = None @property def resolved_worker_acp_command(self) -> str | None: @@ -61,6 +63,24 @@ def resolved_validation_worker_acp_command(self) -> str | None: or self.resolved_validation_worker.default_worker_acp_command ) + @property + def resolved_terminal_reviewer(self) -> ProviderDefinition: + return self.terminal_reviewer or self.resolved_validation_worker + + @property + def resolved_terminal_reviewer_acp_command(self) -> str | None: + if self.terminal_reviewer_acp_command: + return self.terminal_reviewer_acp_command + if self.resolved_terminal_reviewer.name != self.resolved_validation_worker.name: + return ( + self.resolved_terminal_reviewer.default_worker_acp_command + or self.resolved_validation_worker_acp_command + ) + return ( + self.resolved_validation_worker_acp_command + or self.resolved_terminal_reviewer.default_worker_acp_command + ) + def env(self) -> dict[str, str]: env: dict[str, str] = { "ZENITH_ORCHESTRATOR_PROVIDER": self.orchestrator.name, @@ -76,6 +96,14 @@ def env(self) -> dict[str, str]: and validation_command is not None ): env["ZENITH_VALIDATOR_ACP_COMMAND"] = validation_command + if self.resolved_terminal_reviewer.name != self.resolved_validation_worker.name: + env["ZENITH_TERMINAL_REVIEWER_PROVIDER"] = self.resolved_terminal_reviewer.name + tr_command = self.resolved_terminal_reviewer_acp_command + if ( + tr_command != self.resolved_validation_worker_acp_command + and tr_command is not None + ): + env["ZENITH_TERMINAL_REVIEWER_ACP_COMMAND"] = tr_command return env def skill_install_dirs(self) -> tuple[str, ...]: @@ -83,6 +111,7 @@ def skill_install_dirs(self) -> tuple[str, ...]: self.orchestrator.skill_dirs + self.worker.skill_dirs + self.resolved_validation_worker.skill_dirs + + self.resolved_terminal_reviewer.skill_dirs ) def skill_alias_dirs(self) -> tuple[str, ...]: @@ -90,6 +119,7 @@ def skill_alias_dirs(self) -> tuple[str, ...]: self.orchestrator.skill_alias_dirs + self.worker.skill_alias_dirs + self.resolved_validation_worker.skill_alias_dirs + + self.resolved_terminal_reviewer.skill_alias_dirs ) def providers(self) -> tuple[ProviderDefinition, ...]: @@ -97,6 +127,7 @@ def providers(self) -> tuple[ProviderDefinition, ...]: self.orchestrator, self.worker, self.resolved_validation_worker, + self.resolved_terminal_reviewer, ) ordered: list[ProviderDefinition] = [] seen: set[str] = set() diff --git a/zenith/tests/test_cli.py b/zenith/tests/test_cli.py index e5f186d..eabfb10 100644 --- a/zenith/tests/test_cli.py +++ b/zenith/tests/test_cli.py @@ -147,6 +147,69 @@ def test_claude_init_forwards_only_allowed_model_env( assert mcp_env["ZAI_API_KEY"] == "zai-test-key" assert "DATABASE_URL" not in mcp_env + def test_claude_init_writes_terminal_reviewer_env_names( + self, runner: CliRunner, workspace: Path, env: dict[str, str] + ) -> None: + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--terminal-reviewer-provider", + "codex", + "--terminal-reviewer-acp-command", + "custom-tr-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["ZENITH_TERMINAL_REVIEWER_PROVIDER"] == "codex" + assert mcp_env["ZENITH_TERMINAL_REVIEWER_ACP_COMMAND"] == "custom-tr-acp" + + def test_claude_init_omits_terminal_reviewer_env_when_unset( + self, runner: CliRunner, workspace: Path, env: dict[str, str] + ) -> None: + 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 "ZENITH_TERMINAL_REVIEWER_PROVIDER" not in mcp_env + assert "ZENITH_TERMINAL_REVIEWER_ACP_COMMAND" not in mcp_env + + def test_three_distinct_providers_all_env_written( + self, runner: CliRunner, workspace: Path, env: dict[str, str] + ) -> None: + r = runner.invoke( + cli, + [ + "init", + "--workspace-dir", + str(workspace), + "--agent", + "claude", + "--validator-provider", + "codex", + "--terminal-reviewer-provider", + "hermes", + ], + ) + 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_ORCHESTRATOR_PROVIDER"] == "claude" + assert mcp_env["ZENITH_WORKER_PROVIDER"] == "claude" + assert mcp_env["ZENITH_VALIDATOR_PROVIDER"] == "codex" + assert mcp_env["ZENITH_TERMINAL_REVIEWER_PROVIDER"] == "hermes" + class TestListProjects: def test_empty(self, runner: CliRunner, env: dict[str, str]) -> None: diff --git a/zenith/tests/test_config.py b/zenith/tests/test_config.py index 55d64b9..eb3c89f 100644 --- a/zenith/tests/test_config.py +++ b/zenith/tests/test_config.py @@ -4,6 +4,30 @@ from pathlib import Path from zenith_harness.config import HarnessConfig +from zenith_harness.providers import ProviderSelection, get_provider + +_PROVIDER_ENV_KEYS = ( + "ZENITH_ORCHESTRATOR_PROVIDER", + "ZENITH_WORKER_PROVIDER", + "ZENITH_WORKER_ACP_COMMAND", + "ZENITH_VALIDATOR_PROVIDER", + "ZENITH_VALIDATOR_ACP_COMMAND", + "ZENITH_TERMINAL_REVIEWER_PROVIDER", + "ZENITH_TERMINAL_REVIEWER_ACP_COMMAND", +) + + +def _apply_selection_env( + monkeypatch, harness_home: Path, selection: ProviderSelection +) -> HarnessConfig: + """Write selection.env() to a clean environment and re-discover from it.""" + monkeypatch.setenv("ZENITH_HOME", str(harness_home)) + monkeypatch.delenv("ZENITH_PROJECT_BUCKET_DIR", raising=False) + for key in _PROVIDER_ENV_KEYS: + monkeypatch.delenv(key, raising=False) + for key, value in selection.env().items(): + monkeypatch.setenv(key, value) + return HarnessConfig.discover() def test_discover_defaults_to_four_parallel_nodes( @@ -44,3 +68,44 @@ def test_discover_invalid_parallelism_falls_back_to_default( config = HarnessConfig.discover() assert config.max_parallel_nodes == 4 + + +def test_terminal_reviewer_selection_round_trips_through_env( + monkeypatch, + harness_home: Path, +) -> None: + """A distinct terminal reviewer written by env() is read back intact.""" + selection = ProviderSelection( + orchestrator=get_provider("claude"), + worker=get_provider("claude"), + terminal_reviewer=get_provider("codex"), + terminal_reviewer_acp_command="custom-tr-acp", + ) + + config = _apply_selection_env(monkeypatch, harness_home, selection) + + assert config.terminal_reviewer_provider.name == "codex" + assert config.resolved_terminal_reviewer_acp_command == "custom-tr-acp" + + +def test_terminal_reviewer_cascades_to_validator_after_round_trip( + monkeypatch, + harness_home: Path, +) -> None: + """With no explicit terminal reviewer, discover() falls back to the validator. + + env() omits the terminal-reviewer vars because they match the validator + (the cascade parent); the read side must reconstruct the same resolution. + """ + selection = ProviderSelection( + orchestrator=get_provider("claude"), + worker=get_provider("claude"), + validation_worker=get_provider("codex"), + ) + + assert "ZENITH_TERMINAL_REVIEWER_PROVIDER" not in selection.env() + + config = _apply_selection_env(monkeypatch, harness_home, selection) + + assert config.terminal_reviewer_provider_name is None + assert config.terminal_reviewer_provider.name == "codex"