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"