diff --git a/docs/reference/cli.md b/docs/reference/cli.md index f3a6d129..120b0017 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -275,6 +275,7 @@ bench eval run --tasks-dir ./tasks --matrix matrix.yaml --trials 3 | `--canonical-jobs-dir` | — | Materialize selected rollout directories for trainer conversion | | `--retry-policy` | `default` | Retry policy label for reproducible eval artifacts: `default` or `unscored-only` | | `--retry-attempts` | — | Override retry attempts for the eval run | +| `--max-retries` | — | Alias for `--retry-attempts`; maximum retries per task | | `--retry-concurrency` | — | Reserved retry concurrency setting recorded in run config | | `--publish-hf` | — | Upload final eval artifacts to this Hugging Face dataset repo | | `--hf-prefix` | — | Path prefix inside the Hugging Face repo; requires `--publish-hf` | diff --git a/src/benchflow/_utils/scoring.py b/src/benchflow/_utils/scoring.py index b29efee3..9333bfd5 100644 --- a/src/benchflow/_utils/scoring.py +++ b/src/benchflow/_utils/scoring.py @@ -117,6 +117,10 @@ def classify_error(error: str | None) -> str | None: if not error: return None lower = error.lower() + if "missing credential" in lower or ( + "required for model" in lower and "not set" in lower + ): + return PROVIDER_AUTH if "agent idle for" in lower: return IDLE_TIMEOUT if "install failed" in lower: diff --git a/src/benchflow/agents/env.py b/src/benchflow/agents/env.py index ebf83e15..e2fd1fda 100644 --- a/src/benchflow/agents/env.py +++ b/src/benchflow/agents/env.py @@ -72,6 +72,10 @@ _AZURE_HOST_SUFFIXES = (".openai.azure.com", ".services.ai.azure.com") +class MissingAgentCredentialError(ValueError): + """Raised when an agent/model pair is missing a required credential.""" + + def _derive_azure_resource(agent_env: dict[str, str]) -> None: """Populate AZURE_RESOURCE from AZURE_API_ENDPOINT when not already set.""" if agent_env.get(_AZURE_RESOURCE_ENV): @@ -794,7 +798,7 @@ def resolve_agent_env( required_key, ) else: - raise ValueError( + raise MissingAgentCredentialError( f"{required_key} required for model {model!r} but not set. " "Pass it explicitly (for example via --agent-env/agent_env) " "or define it in .env." diff --git a/src/benchflow/cli/main.py b/src/benchflow/cli/main.py index 092e1617..8870ff05 100644 --- a/src/benchflow/cli/main.py +++ b/src/benchflow/cli/main.py @@ -557,6 +557,13 @@ def eval_run( int | None, typer.Option("--retry-attempts", help="Reserved retry-attempt override"), ] = None, + max_retries: Annotated[ + int | None, + typer.Option( + "--max-retries", + help="Alias for --retry-attempts; maximum retries per task.", + ), + ] = None, retry_concurrency: Annotated[ int | None, typer.Option("--retry-concurrency", help="Reserved retry concurrency"), @@ -592,6 +599,13 @@ def eval_run( """ _apply_dotenv_to_process_env() + if retry_attempts is not None and max_retries is not None: + print_error("--retry-attempts and --max-retries are aliases; pass only one") + raise typer.Exit(1) + retry_attempt_override = ( + retry_attempts if retry_attempts is not None else max_retries + ) + request = EvalCreateRequest( config_file=config_file, tasks_dir=tasks_dir, @@ -636,7 +650,7 @@ def eval_run( canonical_selection_out=canonical_selection_out, canonical_jobs_dir=canonical_jobs_dir, retry_policy=retry_policy, - retry_attempts=retry_attempts, + retry_attempts=retry_attempt_override, retry_concurrency=retry_concurrency, publish_hf=publish_hf, hf_prefix=hf_prefix, @@ -901,6 +915,8 @@ def _run_config_file_eval(plan: "EvalPlan") -> None: j._config.prompts = plan.eval_prompts if req.agent_idle_timeout is not None: j._config.agent_idle_timeout = plan.eval_agent_idle_timeout + if req.retry_attempts is not None: + j._config.retry.max_retries = req.retry_attempts if plan.usage_tracking_overridden: j._config.usage_tracking = j._config.usage_tracking.overlay( plan.eval_usage_tracking diff --git a/src/benchflow/evaluation.py b/src/benchflow/evaluation.py index 37fc5e1c..ceda266c 100644 --- a/src/benchflow/evaluation.py +++ b/src/benchflow/evaluation.py @@ -789,10 +789,12 @@ def from_yaml(cls, path: str | Path, **kwargs) -> Evaluation: # Detect format: legacy uses "agents" + "datasets", benchflow uses "agent" if "agents" in raw or "datasets" in raw: return cls._from_legacy_yaml(raw, **kwargs) - return cls._from_native_yaml(raw, **kwargs) + return cls._from_native_yaml(raw, config_path=path, **kwargs) @classmethod - def _from_native_yaml(cls, raw: dict, **kwargs) -> Evaluation: + def _from_native_yaml( + cls, raw: dict, config_path: Path | None = None, **kwargs + ) -> Evaluation: """Parse benchflow-native YAML.""" from benchflow._utils.benchmark_repos import ( TASK_ALIASES, @@ -826,9 +828,23 @@ def _from_native_yaml(cls, raw: dict, **kwargs) -> Evaluation: elif "tasks_dir" in raw: # Legacy single-string format (backward compat). ref = raw["tasks_dir"] + if not isinstance(ref, str | Path): + raise ValueError( + f"YAML 'tasks_dir' must be a path string; got {type(ref).__name__}." + ) tasks_dir = Path(ref) if not tasks_dir.exists() and ref in TASK_ALIASES: tasks_dir = ensure_tasks(ref) + elif not tasks_dir.exists() and str(ref) == ".cache/programbench-benchflow": + config_note = f" in {config_path}" if config_path is not None else "" + raise ValueError( + f"YAML tasks_dir not found{config_note}: {ref}. " + "Generate ProgramBench tasks first with " + "`python benchmarks/programbench/run_programbench.py " + "benchmarks/programbench/programbench-gemini-flash-lite.yaml` " + "or `python -m benchmarks.programbench.main --output-dir " + ".cache/programbench-benchflow`." + ) else: raise ValueError("YAML config must have 'source' or 'tasks_dir'") diff --git a/src/benchflow/rollout/__init__.py b/src/benchflow/rollout/__init__.py index 2567fc00..db89e259 100644 --- a/src/benchflow/rollout/__init__.py +++ b/src/benchflow/rollout/__init__.py @@ -75,6 +75,7 @@ from benchflow._utils.scoring import classify_error as classify_error from benchflow.acp.types import McpServerSpec from benchflow.agents.credentials import upload_credential +from benchflow.agents.env import MissingAgentCredentialError from benchflow.agents.registry import AGENTS from benchflow.contracts import ( AgentProtocolError, @@ -2039,6 +2040,9 @@ async def run(self) -> RolloutResult: # placeholder during cleanup. self._error = str(e) logger.error(str(e)) + except MissingAgentCredentialError as e: + self._error = f"Missing credential: {e}" + logger.error(self._error) except Exception as e: self._error = str(e) logger.error("Run failed", exc_info=True) diff --git a/tests/test_agent_idle_timeout_cli.py b/tests/test_agent_idle_timeout_cli.py index a38891a7..0d03f1a0 100644 --- a/tests/test_agent_idle_timeout_cli.py +++ b/tests/test_agent_idle_timeout_cli.py @@ -74,6 +74,57 @@ def test_eval_create_reasoning_effort_lands_in_config(tmp_path: Path): assert captured["config"].reasoning_effort == "max" +def test_eval_create_max_retries_alias_lands_in_config(tmp_path: Path): + """Guards the fix from PR #918 / Linear ENG-169: --max-retries reaches RetryConfig.""" + tasks = _make_tasks_dir(tmp_path) + captured: dict = {} + + with _stub_evaluation_run(captured): + eval_run( + tasks_dir=tasks, + max_retries=3, + jobs_dir=str(tmp_path / "jobs"), + ) + + assert captured["config"].retry.max_retries == 3 + + +def test_eval_create_max_retries_alias_overrides_yaml_config(tmp_path: Path): + """Guards the fix from PR #918 / Linear ENG-169 on --config eval runs.""" + tasks = _make_tasks_dir(tmp_path) + yaml_path = tmp_path / "config.yaml" + yaml_path.write_text( + "tasks_dir: " + str(tasks) + "\nagent: oracle\nmax_retries: 1\n" + ) + captured: dict = {} + + with _stub_evaluation_run(captured): + eval_run( + config_file=yaml_path, + max_retries=4, + jobs_dir=str(tmp_path / "jobs"), + ) + + assert captured["config"].retry.max_retries == 4 + + +def test_eval_create_rejects_duplicate_retry_aliases(tmp_path: Path): + """Guards the fix from PR #918 / Linear ENG-169 against ambiguous retry aliases.""" + tasks = _make_tasks_dir(tmp_path) + + try: + eval_run( + tasks_dir=tasks, + retry_attempts=1, + max_retries=2, + jobs_dir=str(tmp_path / "jobs"), + ) + except typer.Exit as exc: + assert exc.exit_code == 1 + else: + raise AssertionError("expected typer.Exit for duplicate retry aliases") + + def test_eval_create_reasoning_effort_overrides_yaml_config(tmp_path: Path): """Guards SkillsBench PR #825 so CLI effort overrides YAML defaults.""" tasks = _make_tasks_dir(tmp_path) diff --git a/tests/test_cli_edge_case_hardening.py b/tests/test_cli_edge_case_hardening.py index 3bb97a04..e9863c06 100644 --- a/tests/test_cli_edge_case_hardening.py +++ b/tests/test_cli_edge_case_hardening.py @@ -9,6 +9,7 @@ import json import os +from pathlib import Path from types import SimpleNamespace import pytest @@ -230,6 +231,46 @@ def test_yaml_string_prompts_are_wrapped_in_a_list(tmp_path): assert ev._config.prompts == ["do the thing"] +def test_from_yaml_rejects_missing_tasks_dir_with_programbench_hint(tmp_path): + """Guards the fix from PR #918 / Linear ENG-162: missing ProgramBench tasks fail cleanly.""" + from benchflow.evaluation import Evaluation + + p = tmp_path / "programbench.yaml" + p.write_text("tasks_dir: .cache/programbench-benchflow\nagent: oracle\n") + + with pytest.raises(ValueError, match=r"run_programbench\.py"): + Evaluation.from_yaml(p) + + +def test_from_yaml_accepts_existing_programbench_tasks_dir(tmp_path, monkeypatch): + """Guards the fix from PR #918 / Linear ENG-162: generated ProgramBench dirs load.""" + from benchflow.evaluation import Evaluation + + (tmp_path / ".cache" / "programbench-benchflow").mkdir(parents=True) + p = tmp_path / "programbench.yaml" + p.write_text("tasks_dir: .cache/programbench-benchflow\nagent: oracle\n") + + monkeypatch.chdir(tmp_path) + + ev = Evaluation.from_yaml(p) + + assert ev._tasks_dir == Path(".cache/programbench-benchflow") + + +def test_eval_create_config_missing_tasks_dir_no_raw_traceback(tmp_path): + """Guards the fix from PR #918 / Linear ENG-162 at the CLI config boundary.""" + p = tmp_path / "programbench.yaml" + p.write_text("tasks_dir: .cache/programbench-benchflow\nagent: oracle\n") + + res = runner.invoke(app, ["eval", "create", "--config", str(p)]) + + assert res.exit_code == 1 + assert "Traceback (most recent call last)" not in res.output + normalized = " ".join(res.output.split()) + assert "YAML tasks_dir not found" in normalized + assert "run_programbench.py" in res.output + + # ── markup in user input crashes error handlers (H10/M13) ───────────────────── diff --git a/tests/test_provider_auth_detection.py b/tests/test_provider_auth_detection.py index 1f745f09..3de9c014 100644 --- a/tests/test_provider_auth_detection.py +++ b/tests/test_provider_auth_detection.py @@ -395,6 +395,30 @@ def spy_classify(e): assert rollout._provider_auth_status_cached == 401 +@pytest.mark.asyncio +async def test_run_missing_credential_error_is_clean_provider_auth( + tmp_path, monkeypatch, caplog +): + """Guards the fix from PR #918 / Linear ENG-257: missing credentials avoid tracebacks.""" + from benchflow._utils.scoring import PROVIDER_AUTH, classify_error + from benchflow.agents.env import MissingAgentCredentialError + + rollout = _auth_rollout(tmp_path, usage_source="unavailable") + rollout._rollout_dir = None + missing = MissingAgentCredentialError( + "GEMINI_API_KEY required for model 'gemini/gemini-3.1-flash' but not set." + ) + + monkeypatch.setattr(rollout, "setup", AsyncMock(side_effect=missing)) + caplog.set_level("ERROR", logger="benchflow.rollout") + + result = await rollout.run() + + assert result.error == f"Missing credential: {missing}" + assert classify_error(result.error) == PROVIDER_AUTH + assert "Run failed" not in caplog.text + + def test_enforcement_still_fires_when_no_error_and_usage_missing(tmp_path): """Negative control for FIX 1 (PR #564 / issue #546): on a clean run with no ACP error and no captured provider usage, the legitimate required-usage