diff --git a/ringer.py b/ringer.py index 0dca41b..b22d083 100755 --- a/ringer.py +++ b/ringer.py @@ -127,6 +127,30 @@ def process_name(self) -> str: return Path(self.bin).name or self.name +@dataclass(frozen=True) +class EngineBinDiagnostic: + engine: str + config_key: str + value: str + path_value: str | None + path_was_set: bool + default_search_path: str = field(default_factory=lambda: os.defpath) + + @property + def searched_path_display(self) -> str: + if not self.path_was_set: + return f" (shutil default: {self.default_search_path!r})" + if self.path_value == "": + return "" + return repr(self.path_value) + + def warning(self) -> str: + return ( + f"ringer.py: warning: {self.config_key} = {self.value!r} is not resolvable; " + f"searched PATH: {self.searched_path_display}" + ) + + @dataclass(frozen=True) class PostgresEvalConfig: env_file: Path @@ -428,6 +452,7 @@ class AppConfig: artifact: ArtifactConfig steering: SteeringConfig = field(default_factory=SteeringConfig) update: UpdateConfig = field(default_factory=UpdateConfig) + engine_bin_diagnostics: tuple[EngineBinDiagnostic, ...] = () @classmethod def load(cls, path: Path | None = None) -> "AppConfig": @@ -452,7 +477,12 @@ def load(cls, path: Path | None = None) -> "AppConfig": hud_app_path = optional_path(data.get("hud_app_path")) allow_full_access = bool(data.get("allow_full_access", False)) eval_config = load_eval_config(data.get("eval"), state_dir) - engines = load_engines(data.get("engines")) + raw_engines = data.get("engines") + engines = load_engines(raw_engines) + engine_bin_diagnostics = collect_engine_bin_diagnostics( + engines, + engine_names=configured_engine_names(raw_engines), + ) artifact_config = load_artifact_config(data.get("artifact"), state_dir) update_config = load_update_config(data.get("update")) try: @@ -474,6 +504,7 @@ def load(cls, path: Path | None = None) -> "AppConfig": artifact=artifact_config, steering=steering_config, update=update_config, + engine_bin_diagnostics=engine_bin_diagnostics, ) @@ -926,6 +957,71 @@ def load_hud_port(raw: Any) -> int: return port +def configured_engine_names(raw: Any) -> tuple[str, ...]: + if not isinstance(raw, dict): + return () + names: list[str] = [] + for name in raw: + clean = str(name).strip() + if clean: + names.append(clean) + return tuple(names) + + +def has_path_separator(value: str) -> bool: + separators = tuple(sep for sep in (os.sep, os.altsep) if sep) + return any(sep in value for sep in separators) + + +def collect_engine_bin_diagnostics( + engines: dict[str, EngineConfig], + *, + engine_names: Iterable[str] | None = None, + path_value: str | None = None, + path_was_set: bool | None = None, +) -> tuple[EngineBinDiagnostic, ...]: + if path_was_set is None: + path_was_set = "PATH" in os.environ + if path_value is None and path_was_set: + path_value = os.environ.get("PATH", "") + search_path = path_value if path_was_set else os.defpath + names = tuple(engine_names) if engine_names is not None else tuple(engines) + + diagnostics: list[EngineBinDiagnostic] = [] + for name in names: + engine = engines.get(name) + if engine is None: + continue + bin_value = engine.bin + if has_path_separator(bin_value): + continue + if shutil.which(bin_value, path=search_path) is not None: + continue + diagnostics.append( + EngineBinDiagnostic( + engine=name, + config_key=f"engines.{name}.bin", + value=bin_value, + path_value=path_value, + path_was_set=path_was_set, + ) + ) + return tuple(diagnostics) + + +def print_engine_bin_diagnostics(config: AppConfig) -> None: + for diagnostic in config.engine_bin_diagnostics: + print(diagnostic.warning(), file=sys.stderr) + + +def print_engine_bin_diagnostics_if_config_loads(path: Path | None) -> None: + try: + config = AppConfig.load(path) + except Exception: + return + print_engine_bin_diagnostics(config) + + def load_engines(raw: Any) -> dict[str, EngineConfig]: engines: dict[str, EngineConfig] = {DEFAULT_ENGINE_NAME: built_in_codex_engine()} if raw is None: @@ -10053,6 +10149,7 @@ def main(argv: list[str] | None = None) -> int: if args.command == "lint": manifest = Manifest.from_path(args.manifest) + print_engine_bin_diagnostics_if_config_loads(args.config) findings = lint_manifest( manifest, allow_noncanonical_route=args.allow_noncanonical_route, @@ -10067,6 +10164,7 @@ def main(argv: list[str] | None = None) -> int: return run_catalog_command(args) config = AppConfig.load(args.config) + print_engine_bin_diagnostics(config) if args.command == "db": return run_db_command(config, args) if args.command == "models": diff --git a/tests/test_engine_bin_warning.py b/tests/test_engine_bin_warning.py new file mode 100644 index 0000000..05a07f7 --- /dev/null +++ b/tests/test_engine_bin_warning.py @@ -0,0 +1,228 @@ +from __future__ import annotations + +import contextlib +import io +import json +import os +import sys +import tempfile +import unittest +from dataclasses import FrozenInstanceError +from pathlib import Path +from unittest import mock + +ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(ROOT)) + +import ringer # noqa: E402 + + +LONG_SPEC = ( + "Create result.txt in the task directory with a clear success marker, keep the work scoped, " + "and make any failure easy to diagnose from the check output." +) +GOOD_CHECK = "test -s result.txt || { echo 'missing result.txt'; exit 1; }" +MISSING_BIN = "ringer-definitely-missing-engine-bin" + + +def toml_string(value: object) -> str: + return json.dumps(str(value)) + + +class EngineBinWarningTests(unittest.TestCase): + def setUp(self) -> None: + self.tmp = tempfile.TemporaryDirectory(prefix="ringer-engine-bin-") + self.root = Path(self.tmp.name) + self.config_path = self.root / "config.toml" + self.manifest_path = self.root / "manifest.json" + self.path_dir = self.root / "path" + self.path_dir.mkdir() + + def tearDown(self) -> None: + self.tmp.cleanup() + + def engine(self, name: str, bin_value: str) -> ringer.EngineConfig: + return ringer.EngineConfig( + name=name, + bin=bin_value, + args_template=("{spec}",), + full_access_args=(), + sandbox_args=(), + token_regex=None, + ) + + def write_config(self, engines: dict[str, str]) -> None: + lines = [ + f"state_dir = {toml_string(self.root / 'state')}", + "", + "[eval]", + f"jsonl_path = {toml_string(self.root / 'runs.jsonl')}", + "", + ] + for name, bin_value in engines.items(): + lines.extend( + [ + f"[engines.{name}]", + f"bin = {toml_string(bin_value)}", + 'args_template = ["{spec}"]', + "sandbox_args = []", + "full_access_args = []", + "", + ] + ) + self.config_path.write_text("\n".join(lines), encoding="utf-8") + + def write_manifest(self, *, engine: str = "worker", clean: bool = True) -> Path: + task = { + "key": "task-one", + "engine": engine, + "spec": LONG_SPEC if clean else "too short", + "check": GOOD_CHECK, + "expect_files": ["result.txt"], + "verified": "result.txt exists and is non-empty.", + "task_type": "code-feature", + } + data = { + "run_name": "engine-bin-warning-test", + "workdir": str(self.root / "work"), + "max_parallel": 1, + "tasks": [task], + } + self.manifest_path.write_text(json.dumps(data), encoding="utf-8") + return self.manifest_path + + def run_main(self, argv: list[str]) -> tuple[int, str, str]: + stdout = io.StringIO() + stderr = io.StringIO() + with mock.patch.dict( + os.environ, + {"RINGER_NO_SELF_UPDATE": "1", "PATH": str(self.path_dir)}, + clear=False, + ): + with contextlib.redirect_stdout(stdout), contextlib.redirect_stderr(stderr): + rc = ringer.main(argv) + return rc, stdout.getvalue(), stderr.getvalue() + + def test_pure_diagnostics_are_immutable_and_name_key_value_and_path(self) -> None: + diagnostics = ringer.collect_engine_bin_diagnostics( + {"missing": self.engine("missing", MISSING_BIN)}, + path_value=str(self.path_dir), + path_was_set=True, + ) + + self.assertEqual(1, len(diagnostics)) + diagnostic = diagnostics[0] + self.assertEqual("engines.missing.bin", diagnostic.config_key) + self.assertEqual(MISSING_BIN, diagnostic.value) + self.assertIn("engines.missing.bin", diagnostic.warning()) + self.assertIn(MISSING_BIN, diagnostic.warning()) + self.assertIn(str(self.path_dir), diagnostic.warning()) + with self.assertRaises(FrozenInstanceError): + diagnostic.value = "changed" # type: ignore[misc] + + def test_explicit_path_is_quiet_even_when_missing(self) -> None: + diagnostics = ringer.collect_engine_bin_diagnostics( + {"explicit": self.engine("explicit", str(self.root / "missing-tool"))}, + path_value=str(self.path_dir), + path_was_set=True, + ) + + self.assertEqual((), diagnostics) + + @unittest.skipIf(os.name == "nt", "POSIX executable-bit fixture") + def test_resolvable_bare_name_is_quiet(self) -> None: + executable = self.path_dir / "ok-engine" + executable.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + executable.chmod(0o700) + + diagnostics = ringer.collect_engine_bin_diagnostics( + {"ok": self.engine("ok", executable.name)}, + path_value=str(self.path_dir), + path_was_set=True, + ) + + self.assertEqual((), diagnostics) + + def test_unset_path_diagnostic_distinguishes_unset_from_empty(self) -> None: + diagnostics = ringer.collect_engine_bin_diagnostics( + {"missing": self.engine("missing", MISSING_BIN)}, + path_was_set=False, + ) + + self.assertEqual(1, len(diagnostics)) + warning = diagnostics[0].warning() + self.assertIn("searched PATH: ", warning) + self.assertNotIn("", warning) + + def test_lint_prints_warning_but_still_succeeds(self) -> None: + self.write_config({"missing": MISSING_BIN}) + self.write_manifest(engine="missing", clean=True) + + rc, stdout, stderr = self.run_main( + ["--config", str(self.config_path), "lint", str(self.manifest_path)] + ) + + self.assertEqual(0, rc) + self.assertIn("lint: clean", stdout) + self.assertIn("ringer.py: warning:", stderr) + self.assertIn("engines.missing.bin", stderr) + self.assertIn(MISSING_BIN, stderr) + + def test_malformed_config_does_not_change_lint_result(self) -> None: + self.config_path.write_text("engines = 1\n", encoding="utf-8") + self.write_manifest(clean=False) + + rc, stdout, stderr = self.run_main( + ["--config", str(self.config_path), "lint", str(self.manifest_path)] + ) + + self.assertEqual(1, rc) + self.assertIn("lint: task-one: spec is probably underspecified", stdout) + self.assertNotIn("config", stderr.lower()) + + def test_used_missing_engine_warns_before_fatal_preflight(self) -> None: + self.write_config({"missing": MISSING_BIN}) + self.write_manifest(engine="missing", clean=True) + + rc, _stdout, stderr = self.run_main( + [ + "--config", + str(self.config_path), + "run", + str(self.manifest_path), + "--no-dashboard", + "--identity", + "test-runner", + ] + ) + + self.assertEqual(2, rc) + warning_index = stderr.index("ringer.py: warning:") + fatal_index = stderr.index("ringer.py: error: engine 'missing' binary not found") + self.assertLess(warning_index, fatal_index) + + def test_unused_missing_engine_warns_without_fatal_preflight(self) -> None: + self.write_config({"worker": sys.executable, "unused": MISSING_BIN}) + self.write_manifest(engine="worker", clean=True) + + rc, stdout, stderr = self.run_main( + [ + "--config", + str(self.config_path), + "run", + str(self.manifest_path), + "--no-dashboard", + "--identity", + "test-runner", + "--dry-run", + ] + ) + + self.assertEqual(0, rc) + self.assertIn("DRY RUN:", stdout) + self.assertIn("engines.unused.bin", stderr) + self.assertNotIn("binary not found", stderr) + + +if __name__ == "__main__": + unittest.main(verbosity=2)