diff --git a/zenith/src/zenith_harness/config.py b/zenith/src/zenith_harness/config.py index 22adcc0..45a96a9 100644 --- a/zenith/src/zenith_harness/config.py +++ b/zenith/src/zenith_harness/config.py @@ -168,18 +168,44 @@ def resolved_worker_acp_command(self) -> str | None: @property def resolved_validator_acp_command(self) -> str | None: + """Explicit override, else inherit; provider default only on a + provider switch. + + Mirrors `ProviderSelection.resolved_validation_worker_acp_command` + (providers.py): a custom worker command must cascade to a validator + on the *same* provider. The provider default is a fallback for a + *different* validator provider, not a shadow over the inherited + command — `default_worker_acp_command` is always truthy, so putting + it ahead of inheritance in a plain or-chain silently discards the + user's custom command (model flags, wrapper scripts, mocks). + """ + if self.validator_acp_command: + return self.validator_acp_command + if self.validator_provider.name != self.worker_provider.name: + return ( + self.validator_provider.default_worker_acp_command + or self.resolved_worker_acp_command + ) return ( - self.validator_acp_command + self.resolved_worker_acp_command or self.validator_provider.default_worker_acp_command - or self.resolved_worker_acp_command ) @property def resolved_terminal_reviewer_acp_command(self) -> str | None: + """Same cascade as the validator, one level up: inherit the + validator's resolved command unless the reviewer switches provider. + """ + if self.terminal_reviewer_acp_command: + return self.terminal_reviewer_acp_command + if self.terminal_reviewer_provider.name != self.validator_provider.name: + return ( + self.terminal_reviewer_provider.default_worker_acp_command + or self.resolved_validator_acp_command + ) return ( - self.terminal_reviewer_acp_command + self.resolved_validator_acp_command or self.terminal_reviewer_provider.default_worker_acp_command - or self.resolved_validator_acp_command ) @property diff --git a/zenith/tests/test_config.py b/zenith/tests/test_config.py index 83c9c49..d4fab9a 100644 --- a/zenith/tests/test_config.py +++ b/zenith/tests/test_config.py @@ -6,6 +6,31 @@ import pytest 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() + _EFFORT_ENV_VARS = ( "ZENITH_WORKER_REASONING_EFFORT", @@ -140,3 +165,119 @@ 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_validator_inherits_custom_worker_command_same_provider( + monkeypatch, + harness_home: Path, +) -> None: + """A custom worker command cascades to a same-provider validator. + + Regression: the or-chain preferred the provider default (always + truthy) over inheritance, so validators silently ran the stock + adapter while workers ran the custom command (model flags, wrapper + scripts, mocks). + """ + selection = ProviderSelection( + orchestrator=get_provider("claude"), + worker=get_provider("claude"), + worker_acp_command="claude-agent-acp --model custom", + ) + + config = _apply_selection_env(monkeypatch, harness_home, selection) + + assert ( + config.resolved_validator_acp_command + == "claude-agent-acp --model custom" + ) + assert ( + config.resolved_terminal_reviewer_acp_command + == "claude-agent-acp --model custom" + ) + # for_role is the dispatch-time consumer of the cascade. + assert ( + config.for_role("validator").worker_acp_command + == "claude-agent-acp --model custom" + ) + assert ( + config.for_role("terminal_reviewer").worker_acp_command + == "claude-agent-acp --model custom" + ) + + +def test_validator_provider_switch_uses_provider_default( + monkeypatch, + harness_home: Path, +) -> None: + """A different validator provider must NOT inherit the worker command.""" + selection = ProviderSelection( + orchestrator=get_provider("claude"), + worker=get_provider("claude"), + worker_acp_command="claude-agent-acp --model custom", + validation_worker=get_provider("codex"), + ) + + config = _apply_selection_env(monkeypatch, harness_home, selection) + + assert config.resolved_validator_acp_command == "codex-acp" + # Reviewer cascades from the validator (same provider as validator). + assert config.resolved_terminal_reviewer_acp_command == "codex-acp" + + +def test_explicit_validator_command_beats_inheritance( + monkeypatch, + harness_home: Path, +) -> None: + selection = ProviderSelection( + orchestrator=get_provider("claude"), + worker=get_provider("claude"), + worker_acp_command="claude-agent-acp --model custom", + validation_worker=get_provider("claude"), + validation_worker_acp_command="claude-agent-acp --model validator", + ) + + config = _apply_selection_env(monkeypatch, harness_home, selection) + + assert ( + config.resolved_validator_acp_command + == "claude-agent-acp --model validator" + ) + # Reviewer inherits the validator's explicit command, not the worker's. + assert ( + config.resolved_terminal_reviewer_acp_command + == "claude-agent-acp --model validator" + ) + + +def test_config_resolution_matches_provider_selection( + monkeypatch, + harness_home: Path, +) -> None: + """config.py and providers.py implement the same cascade — the read + side of env() must resolve identically to the write side. + """ + cases = [ + ProviderSelection( + orchestrator=get_provider("claude"), + worker=get_provider("claude"), + worker_acp_command="claude-agent-acp --model custom", + ), + ProviderSelection( + orchestrator=get_provider("claude"), + worker=get_provider("claude"), + worker_acp_command="claude-agent-acp --model custom", + validation_worker=get_provider("codex"), + ), + ProviderSelection( + orchestrator=get_provider("claude"), + worker=get_provider("codex"), + worker_acp_command='codex-acp -c model="custom"', + validation_worker=get_provider("claude"), + ), + ] + for selection in cases: + config = _apply_selection_env(monkeypatch, harness_home, selection) + assert ( + config.resolved_validator_acp_command + == selection.resolved_validation_worker_acp_command + ), selection