diff --git a/CHANGELOG.md b/CHANGELOG.md index 07fb54e1..aa0accea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ### Fixed - Fixed an issue where malformed per-run metadata could prevent `batch-summary.json` from being written and, when configured, uploaded. +- Fixed the issue that an invalid judge model would lose the `run-meta.json` file. ## [0.9.2] - 2026-08-18 ### Added diff --git a/src/clawbench/runner/run.py b/src/clawbench/runner/run.py index e903e7b7..4c9bf3d3 100644 --- a/src/clawbench/runner/run.py +++ b/src/clawbench/runner/run.py @@ -21,6 +21,7 @@ HARNESSES, IMAGE, WORKSPACE_ROOT, + ModelConfigError, harness_image, load_model_config, load_runtime_env, @@ -213,10 +214,28 @@ def main(): safe_model = "human" harness_tag = "human" else: - model_cfg = load_model_config(args.model) + try: + model_cfg = load_model_config(args.model) + except ModelConfigError as e: + # No output_dir exists yet at this point, so there is nothing + # for a run-meta.json to record — exit immediately like other + # pre-flight validation above (e.g. missing PURELY_MAIL_* env). + print(f"ERROR: {e}") + sys.exit(1) safe_model = re.sub(r"[/:]+", "--", args.model) harness_tag = args.harness + # Resolve the judge model now rather than after the agent run. A bad + # --judge is a typo in the command line, and finding out about it 30 + # minutes later — once the agent has already finished — helps nobody. + startup_judge_cfg: dict | None = None + if not args.human and args.judge and not args.no_judge: + try: + startup_judge_cfg = load_model_config(args.judge) + except ModelConfigError as e: + print(f"ERROR: --judge {args.judge!r}: {e}") + sys.exit(1) + container = f"clawbench-{harness_tag}-{case_name}-{safe_model}-{int(time.time())}" run_dir_name = f"{harness_tag}-{case_name}-{safe_model}-{ts}" @@ -232,7 +251,7 @@ def main(): extra_info_warnings: list[str] = [] intercepted = False host_port: int | None = None - judge_cfg: dict | None = None + judge_cfg: dict | None = startup_judge_cfg personal_info_metadata: dict[str, Any] | None = None browser_session: BrowserSession | None = None browser_runtime_finalized = False @@ -601,7 +620,9 @@ def handle_sigint(sig, frame): try: from clawbench.runner.judge import judge_request - judge_cfg = load_model_config(args.judge) + # Validated at startup; reload only if that was skipped. + if judge_cfg is None: + judge_cfg = load_model_config(args.judge) instruction_text = ( task.get("instruction") if isinstance(task, dict) else "" ) or "" diff --git a/src/clawbench/runner/run_support/config.py b/src/clawbench/runner/run_support/config.py index b093ea59..37dc6a99 100644 --- a/src/clawbench/runner/run_support/config.py +++ b/src/clawbench/runner/run_support/config.py @@ -32,6 +32,7 @@ "MODELS_YAML", "WORKSPACE_ROOT", "HarnessRegistry", + "ModelConfigError", "harness_image", "load_dotenv", "load_harness_registry", @@ -44,6 +45,16 @@ ] +class ModelConfigError(Exception): + """Raised when a model config in models/models.yaml is missing or invalid. + + A plain Exception (not SystemExit) so callers that load a model mid-run + (e.g. the judge stage, after the agent has already produced results) can + catch it and continue instead of the process dying before run-meta.json + is written. + """ + + HARNESSES = HARNESS_REGISTRY.harnesses DEFAULT_HARNESS = HARNESS_REGISTRY.default BASE_IMAGE = HARNESS_REGISTRY.base_image @@ -107,12 +118,16 @@ def load_dotenv(path: Path) -> dict[str, str]: def load_models_yaml() -> dict: - """Load all model definitions from models/models.yaml.""" + """Load all model definitions from models/models.yaml. + + Raises ModelConfigError rather than exiting, for the same reason + load_model_config does: this runs inside the judge stage too, where a + SystemExit would escape the handlers and lose the run's metadata. + """ if not MODELS_YAML.exists(): - print( - f"ERROR: {MODELS_YAML} not found (copy models.example.yaml and fill in your keys)" + raise ModelConfigError( + f"{MODELS_YAML} not found (copy models.example.yaml and fill in your keys)" ) - sys.exit(1) return yaml.safe_load(MODELS_YAML.read_text()) or {} @@ -161,9 +176,10 @@ def load_model_config(model: str) -> dict: """ all_models = load_models_yaml() if model not in all_models: - print(f"ERROR: model '{model}' not found in {MODELS_YAML}") - print(f"Available models: {', '.join(sorted(all_models))}") - sys.exit(1) + raise ModelConfigError( + f"model '{model}' not found in {MODELS_YAML}. " + f"Available models: {', '.join(sorted(all_models))}" + ) # Validate model name characters. Note: '/' and ':' are valid in # vendor-prefixed ids like 'anthropic/claude-sonnet-4-6' or @@ -173,11 +189,10 @@ def load_model_config(model: str) -> dict: # that sanitization. bad = [c for c in ' \\*?"<>|' if c in model] if bad: - print( - f"ERROR: model name '{model}' contains illegal character(s): " + raise ModelConfigError( + f"model name '{model}' contains illegal character(s): " f"{' '.join(repr(c) for c in bad)}" ) - sys.exit(1) config = dict(all_models[model]) config["model"] = model # the YAML key IS the model name @@ -185,9 +200,9 @@ def load_model_config(model: str) -> dict: required = ["base_url", "api_type"] missing = [k for k in required if not config.get(k)] if missing: - for k in missing: - print(f"ERROR: Required field '{k}' missing for model '{model}'") - sys.exit(1) + raise ModelConfigError( + f"required field(s) missing for model '{model}': {', '.join(missing)}" + ) # Normalize API keys: api_keys list wins, else wrap api_key into list. if config.get("api_keys"): @@ -196,7 +211,6 @@ def load_model_config(model: str) -> dict: config["api_keys"] = [config["api_key"]] if not config.get("api_keys"): - print(f"ERROR: no api_key or api_keys for model '{model}'") - sys.exit(1) + raise ModelConfigError(f"no api_key or api_keys for model '{model}'") return config diff --git a/tests/test_run_judge_stage.py b/tests/test_run_judge_stage.py new file mode 100644 index 00000000..d911492f --- /dev/null +++ b/tests/test_run_judge_stage.py @@ -0,0 +1,291 @@ +"""Regression test for a missing/invalid --judge model (issue #302). + +Before the fix, load_model_config() called sys.exit() on a bad model name. +When that happened for the *judge* model — after the agent had already run +and produced results — the resulting SystemExit escaped both the judge +stage's `except Exception` and run()'s top-level `except Exception`, since +SystemExit is a BaseException, not an Exception. The process died without +ever calling write_run_meta(), silently discarding the run's results. + +The fix has two halves, one test each: + + * A bad --judge is now rejected at startup, before the agent runs at all, + so a command-line typo costs seconds instead of half an hour. + * A judge failure that only surfaces mid-run is caught and degraded to a + judge_setup_failed outcome, and the run still reaches write_run_meta(). + +These drive run_mod.main() through a fully mocked agent run, skipping the +docker/network/email side effects. +""" + +from __future__ import annotations + +import importlib +import json +import shutil +import sys +from pathlib import Path +from types import ModuleType +from typing import Any + +import pytest + + +def _import_run_module(monkeypatch: pytest.MonkeyPatch) -> ModuleType: + for module_name in ( + "clawbench.runner.run", + "clawbench.runner.run_support.metadata", + "clawbench.runner.run_support.docker", + "clawbench.runner.run_support.config", + ): + sys.modules.pop(module_name, None) + monkeypatch.delenv("CONTAINER_ENGINE", raising=False) + monkeypatch.setattr( + shutil, + "which", + lambda cmd: str(Path("mock-bin") / cmd) if cmd == "docker" else None, + ) + return importlib.import_module("clawbench.runner.run") + + +def _prepare_run( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + judge_model: str, +) -> tuple[ModuleType, list[tuple[Path, dict[str, Any]]], list[str]]: + """Mock out every side effect of a run; return the module, the + run-meta writes it performed, and a log of docker calls.""" + run_mod = _import_run_module(monkeypatch) + + task_dir = tmp_path / "case" + task_dir.mkdir() + (task_dir / "task.json").write_text( + json.dumps( + { + "instruction": "Do the task", + "eval_schema": {"url_pattern": "example", "method": "POST"}, + "time_limit": 1, + } + ) + ) + personal_info_dir = tmp_path / "personal-info" + personal_info_dir.mkdir() + + monkeypatch.setattr( + run_mod.sys, + "argv", + [ + "clawbench-run", + str(task_dir), + "model-a", + "--judge", + judge_model, + "--output-dir", + str(tmp_path / "out"), + "--no-build", + "--no-upload", + ], + ) + monkeypatch.setattr(run_mod, "ensure_workspace_templates", lambda: None) + monkeypatch.setattr( + run_mod, + "load_runtime_env", + lambda: { + "PURELY_MAIL_API_KEY": "pm-key", + "PURELY_MAIL_DOMAIN": "example.test", + }, + ) + + class FakeProvider: + name = "local" + + def start(self, task: dict, time_limit_s: int): + from clawbench.runner.run_support.browser_runtime.providers import ( + BrowserSession, + ) + + return BrowserSession( + provider="local", + cdp_url="http://127.0.0.1:9222", + mode="local", + recording_mode="x11", + local_viewer_port=6080, + ) + + def finalize(self, session, output_dir): + pass + + def cleanup(self, session): + pass + + monkeypatch.setattr( + run_mod, "make_browser_runtime_provider", lambda args, env: FakeProvider() + ) + monkeypatch.setattr(run_mod, "preflight_model_api", lambda model_cfg: None) + + def fake_load_model_config(model: str) -> dict: + if model == "missing-judge-model": + raise run_mod.ModelConfigError( + f"model '{model}' not found in models.yaml. Available models: model-a" + ) + return { + "model": model, + "base_url": "https://api.example.test/v1", + "api_type": "openai-completions", + "api_key": "secret-key", + "api_keys": ["secret-key"], + } + + monkeypatch.setattr(run_mod, "load_model_config", fake_load_model_config) + monkeypatch.setattr( + run_mod, + "create_email", + lambda pm_key, pm_domain: ("agent@example.test", "email-pw"), + ) + monkeypatch.setattr(run_mod, "delete_email", lambda pm_key, email: None) + monkeypatch.setattr( + run_mod, + "prepare_personal_info", + lambda shared_root, email, email_pw, output_dir: (personal_info_dir, {}), + ) + monkeypatch.setattr(run_mod, "copy_extra_info", lambda task, task_dir, dest: []) + monkeypatch.setattr(run_mod, "build_instruction", lambda task: "Do the task") + docker_calls: list[str] = [] + monkeypatch.setattr( + run_mod, "docker_run", lambda *args, **kwargs: docker_calls.append("run") + ) + monkeypatch.setattr(run_mod, "docker_wait", lambda *args, **kwargs: None) + monkeypatch.setattr(run_mod, "docker_logs", lambda *args, **kwargs: None) + monkeypatch.setattr(run_mod, "docker_copy", lambda *args, **kwargs: None) + monkeypatch.setattr(run_mod, "docker_rm", lambda *args, **kwargs: None) + monkeypatch.setattr(run_mod, "_fix_data_ownership", lambda *args, **kwargs: None) + monkeypatch.setattr(run_mod, "ensure_interception", lambda *args, **kwargs: None) + monkeypatch.setattr( + run_mod, "print_results", lambda *args, **kwargs: True + ) # stage 1 (intercepted) passed + monkeypatch.setattr( + run_mod, "remove_transient_usage_artifact", lambda *args, **kwargs: None + ) + monkeypatch.setattr( + run_mod, + "classify_run", + lambda *args, **kwargs: { + "result_category": "pass", + "failure_category": None, + "infra_failure": False, + "adjusted_eligible": True, + "infra_flags": [], + "metrics": {}, + }, + ) + + written: list[tuple[Path, dict[str, Any]]] = [] + monkeypatch.setattr( + run_mod, + "make_run_meta", + lambda **kwargs: {"failure_reason": kwargs.get("failure_reason")}, + ) + monkeypatch.setattr( + run_mod, + "write_run_meta", + lambda output_dir, meta: written.append((output_dir, meta)), + ) + + return run_mod, written, docker_calls + + +def test_bad_judge_model_is_rejected_before_the_agent_runs( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """Ask 1 of the issue: catch a --judge typo at startup, not 30 minutes in.""" + run_mod, written, docker_calls = _prepare_run( + monkeypatch, tmp_path, "missing-judge-model" + ) + + with pytest.raises(SystemExit) as excinfo: + run_mod.main() + + assert excinfo.value.code == 1 + assert docker_calls == [] # the agent never ran + assert written == [] # and there is no run to record + out = capsys.readouterr().out + assert "--judge" in out + assert "missing-judge-model" in out + + +def test_judge_failure_after_the_run_still_writes_run_meta( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + """Ask 2, and the invariant: a completed agent run always writes metadata. + + Before the fix, a ModelConfigError surfacing here was a SystemExit, which + escaped both the judge stage's `except Exception` and run()'s top-level + handler, killing the process before write_run_meta(). + """ + run_mod, written, docker_calls = _prepare_run(monkeypatch, tmp_path, "model-a") + + import clawbench.runner.judge as judge_mod + + def explode(*args: Any, **kwargs: Any) -> dict: + raise run_mod.ModelConfigError("judge backend went away mid-run") + + monkeypatch.setattr(judge_mod, "judge_request", explode) + + with pytest.raises(SystemExit) as excinfo: + run_mod.main() + + # Inconclusive judge -> normal exit 1, not an uncaught crash. + assert excinfo.value.code == 1 + assert docker_calls == ["run"] # the agent did run + + # The core regression: its results must not be silently discarded. + assert len(written) == 1 + meta = written[0][1] + assert meta["judge_match"] is None + assert "judge_setup_failed" in meta["judge"]["reason"] + assert "went away mid-run" in meta["judge"]["reason"] + assert meta["pass"] is False + + +def test_model_config_error_is_catchable_as_a_plain_exception( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The whole point: SystemExit is a BaseException and slips past + `except Exception`. ModelConfigError must not.""" + ModelConfigError = _import_run_module(monkeypatch).ModelConfigError + + assert issubclass(ModelConfigError, Exception) + assert not issubclass(ModelConfigError, SystemExit) + + try: + raise ModelConfigError("boom") + except Exception as e: + assert "boom" in str(e) + else: # pragma: no cover + pytest.fail("ModelConfigError was not caught by `except Exception`") + + +def test_missing_models_yaml_is_also_catchable( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """load_model_config() calls load_models_yaml(). If that still exited, + a SystemExit would escape the judge stage exactly as before.""" + config = _import_run_module(monkeypatch) + cfg_mod = sys.modules["clawbench.runner.run_support.config"] + monkeypatch.setattr(cfg_mod, "MODELS_YAML", tmp_path / "absent.yaml") + + with pytest.raises(config.ModelConfigError) as excinfo: + cfg_mod.load_models_yaml() + + assert "absent.yaml" in str(excinfo.value) + + # And the same call reached through load_model_config stays catchable. + try: + cfg_mod.load_model_config("anything") + except Exception as e: + assert isinstance(e, config.ModelConfigError) + else: # pragma: no cover + pytest.fail("expected ModelConfigError")